From 254307a23b1a41a2f6d213dfc7909faba7d43271 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 20 Mar 2026 11:35:13 +0100 Subject: [PATCH 1/4] Don't panic if a query branched on an untracked state --- src/function/backdate.rs | 23 +++++++- tests/backdate_untracked_db_field.rs | 83 ++++++++++++++++++++++++++++ tests/debug.rs | 52 +++++++++++++++++ 3 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 tests/backdate_untracked_db_field.rs diff --git a/src/function/backdate.rs b/src/function/backdate.rs index 330fba7c4..260713cdd 100644 --- a/src/function/backdate.rs +++ b/src/function/backdate.rs @@ -39,7 +39,28 @@ where old_memo.revisions.changed_at, ); - assert!(old_memo.revisions.changed_at <= revisions.changed_at); + if old_memo.revisions.changed_at > revisions.changed_at { + let message = format_args!( + "query {index:?} returned the same value, but the previous execution \ + changed at {:?} and the new execution changed at {:?}. This usually \ + means the query re-executed because an input changed, but then branched \ + on untracked state (for example, a global variable, a non-salsa field \ + on the database, or filesystem state read outside salsa) and no longer \ + read that input. This is usually a bug in the query implementation. \ + Queries that branch on untracked state can also produce stale results. \ + If the query has no untracked reads, please open a salsa issue.", + old_memo.revisions.changed_at, revisions.changed_at, + ); + + if cfg!(debug_assertions) { + panic!("{message}"); + } else { + crate::tracing::warn!("{message}"); + // Fallthrough to still use the old memo's changed_at + // to ensure `changed_at` is never decreasing. + } + } + revisions.changed_at = old_memo.revisions.changed_at; } } diff --git a/tests/backdate_untracked_db_field.rs b/tests/backdate_untracked_db_field.rs new file mode 100644 index 000000000..bb91218d9 --- /dev/null +++ b/tests/backdate_untracked_db_field.rs @@ -0,0 +1,83 @@ +#![cfg(feature = "inventory")] + +use salsa::Setter; + +#[salsa::input(debug)] +struct MyInput { + field: u32, +} + +#[salsa::db] +trait Db: salsa::Database { + /// Untracked database state. Reading this inside a tracked query is a footgun. + fn extra(&self) -> u32; +} + +#[salsa::db] +#[derive(Default, Clone)] +struct ExtraFieldDatabase { + storage: salsa::Storage, + extra: u32, +} + +#[salsa::db] +impl salsa::Database for ExtraFieldDatabase {} + +#[salsa::db] +impl Db for ExtraFieldDatabase { + fn extra(&self) -> u32 { + self.extra + } +} + +impl ExtraFieldDatabase { + fn set_extra(&mut self, value: u32) { + self.extra = value; + } +} + +#[salsa::tracked] +fn dep_a_db(db: &dyn Db, input: MyInput) -> u32 { + input.field(db) +} + +#[salsa::tracked] +fn dep_b_db(db: &dyn Db, input: MyInput) -> u32 { + input.field(db) +} + +#[salsa::tracked] +fn db_field_branch_query(db: &dyn Db, a: MyInput, b: MyInput) -> u32 { + if db.extra() == 0 { + dep_a_db(db, a) + } else { + dep_b_db(db, b) + } +} + +#[test] +#[cfg_attr(debug_assertions, should_panic(expected = "cannot backdate query"))] +fn db_field_branch_can_trip_backdate_assertion() { + let mut db = ExtraFieldDatabase::default(); + db.set_extra(0); + + let a = MyInput::new(&db, 0); + let b = MyInput::new(&db, 0); + + // R1: depends on `a`, returns 0 + assert_eq!(db_field_branch_query(&db, a, b), 0); + + // R2: force the memo to have a recent changed_at. + a.set_field(&mut db).to(1); + assert_eq!(db_field_branch_query(&db, a, b), 1); + + // R3: return to 0 (still depending on `a`). + a.set_field(&mut db).to(0); + assert_eq!(db_field_branch_query(&db, a, b), 0); + + // R4/R5: switch branch via untracked db field, and force re-execution by changing `a`. + // New execution returns 0 (equal) but depends only on older `b`, triggering the backdate check. + db.set_extra(1); + a.set_field(&mut db).to(1); + let _ = db_field_branch_query(&db, a, b); +} diff --git a/tests/debug.rs b/tests/debug.rs index da184b40a..58489082a 100644 --- a/tests/debug.rs +++ b/tests/debug.rs @@ -65,6 +65,58 @@ fn untracked_dependencies() { assert!(s.contains(", field: 22 }")); } +#[salsa::tracked] +fn dep_a(db: &dyn salsa::Database, input: MyInput) -> u32 { + input.field(db) +} + +#[salsa::tracked] +fn dep_b(db: &dyn salsa::Database, input: MyInput) -> u32 { + input.field(db) +} + +#[salsa::tracked] +fn debug_branch_query(db: &dyn salsa::Database, selector: MyInput, a: MyInput, b: MyInput) -> u32 { + // `Debug` for salsa structs is explicitly untracked; branching on it is unsound. + // This test demonstrates it can break the backdate invariant. + let s = format!("{selector:?}"); + if s.contains("field: 0") { + dep_a(db, a) + } else { + dep_b(db, b) + } +} + +/// Backdating warns about branching on the output of a Salsa struct's derived `Debug` output, +/// because it doesn't track its reads (can lead to stale results). +#[test] +#[cfg_attr(debug_assertions, should_panic(expected = "cannot backdate query"))] +fn debug_branch_can_trip_backdate_assertion() { + let mut db = salsa::DatabaseImpl::new(); + + let selector = MyInput::new(&db, 0); + let a = MyInput::new(&db, 0); + let b = MyInput::new(&db, 0); + + // R1: depends on `a`, returns 0 + assert_eq!(debug_branch_query(&db, selector, a, b), 0); + + // R2: force `debug_branch_query` to change (0 -> 1) so its memo's changed_at advances. + a.set_field(&mut db).to(1); + assert_eq!(debug_branch_query(&db, selector, a, b), 1); + + // R3: change back to 0; memo value is 0 but changed_at is now "recent". + a.set_field(&mut db).to(0); + assert_eq!(debug_branch_query(&db, selector, a, b), 0); + + // R4/R5: change `selector` (untracked) so the query switches to `b`, and change `a` + // to force re-execution. New execution returns 0 (equal) but depends only on older `b`, + // so `new.changed_at < old.changed_at` and backdating asserts. + selector.set_field(&mut db).to(1); + a.set_field(&mut db).to(1); + let _ = debug_branch_query(&db, selector, a, b); +} + #[salsa::tracked] struct DerivedCustom<'db> { my_input: MyInput, From d2381f819b9b5a9f3ab9e75be2ffc7b0b8c88ecf Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 20 Mar 2026 11:53:31 +0100 Subject: [PATCH 2/4] Don't use `format_args --- src/function/backdate.rs | 21 +++++++++++---------- tests/backdate_untracked_db_field.rs | 2 +- tests/debug.rs | 2 +- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/function/backdate.rs b/src/function/backdate.rs index 260713cdd..7e0cd76cd 100644 --- a/src/function/backdate.rs +++ b/src/function/backdate.rs @@ -40,16 +40,17 @@ where ); if old_memo.revisions.changed_at > revisions.changed_at { - let message = format_args!( - "query {index:?} returned the same value, but the previous execution \ - changed at {:?} and the new execution changed at {:?}. This usually \ - means the query re-executed because an input changed, but then branched \ - on untracked state (for example, a global variable, a non-salsa field \ - on the database, or filesystem state read outside salsa) and no longer \ - read that input. This is usually a bug in the query implementation. \ - Queries that branch on untracked state can also produce stale results. \ - If the query has no untracked reads, please open a salsa issue.", - old_memo.revisions.changed_at, revisions.changed_at, + let message = format!( + "query {index:?} returned the same value, but the previous execution changed at \ + {:?} and the new execution changed at {:?}. This usually means the query \ + re-executed because an input changed, but then branched on untracked state \ + (for example, a global variable, a non-salsa field on the database, or \ + filesystem state read outside salsa) and no longer read that input. This is \ + usually a bug in the query implementation. Queries that branch on untracked \ + state can also produce stale results. If the query has no untracked reads, \ + please open a salsa issue.", + old_memo.revisions.changed_at, + revisions.changed_at, ); if cfg!(debug_assertions) { diff --git a/tests/backdate_untracked_db_field.rs b/tests/backdate_untracked_db_field.rs index bb91218d9..6488b6d90 100644 --- a/tests/backdate_untracked_db_field.rs +++ b/tests/backdate_untracked_db_field.rs @@ -56,7 +56,7 @@ fn db_field_branch_query(db: &dyn Db, a: MyInput, b: MyInput) -> u32 { } #[test] -#[cfg_attr(debug_assertions, should_panic(expected = "cannot backdate query"))] +#[cfg_attr(debug_assertions, should_panic(expected = "returned the same value"))] fn db_field_branch_can_trip_backdate_assertion() { let mut db = ExtraFieldDatabase::default(); db.set_extra(0); diff --git a/tests/debug.rs b/tests/debug.rs index 58489082a..60a34f18c 100644 --- a/tests/debug.rs +++ b/tests/debug.rs @@ -90,7 +90,7 @@ fn debug_branch_query(db: &dyn salsa::Database, selector: MyInput, a: MyInput, b /// Backdating warns about branching on the output of a Salsa struct's derived `Debug` output, /// because it doesn't track its reads (can lead to stale results). #[test] -#[cfg_attr(debug_assertions, should_panic(expected = "cannot backdate query"))] +#[cfg_attr(debug_assertions, should_panic(expected = "returned the same value"))] fn debug_branch_can_trip_backdate_assertion() { let mut db = salsa::DatabaseImpl::new(); From 5b240dc28724e8c77c79696589508c642ff9747c Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 20 Mar 2026 12:11:32 +0100 Subject: [PATCH 3/4] Include backtrace in tracing log --- src/function/backdate.rs | 77 +++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 17 deletions(-) diff --git a/src/function/backdate.rs b/src/function/backdate.rs index 7e0cd76cd..3dc1c141e 100644 --- a/src/function/backdate.rs +++ b/src/function/backdate.rs @@ -1,7 +1,9 @@ use crate::function::memo::Memo; use crate::function::{Configuration, IngredientImpl}; use crate::zalsa_local::QueryRevisions; +use crate::Backtrace; use crate::DatabaseKeyIndex; +use std::fmt; impl IngredientImpl where @@ -40,26 +42,11 @@ where ); if old_memo.revisions.changed_at > revisions.changed_at { - let message = format!( - "query {index:?} returned the same value, but the previous execution changed at \ - {:?} and the new execution changed at {:?}. This usually means the query \ - re-executed because an input changed, but then branched on untracked state \ - (for example, a global variable, a non-salsa field on the database, or \ - filesystem state read outside salsa) and no longer read that input. This is \ - usually a bug in the query implementation. Queries that branch on untracked \ - state can also produce stale results. If the query has no untracked reads, \ - please open a salsa issue.", + report_backdate_violation( + index, old_memo.revisions.changed_at, revisions.changed_at, ); - - if cfg!(debug_assertions) { - panic!("{message}"); - } else { - crate::tracing::warn!("{message}"); - // Fallthrough to still use the old memo's changed_at - // to ensure `changed_at` is never decreasing. - } } revisions.changed_at = old_memo.revisions.changed_at; @@ -67,3 +54,59 @@ where } } } + +#[cold] +#[inline(never)] +fn report_backdate_violation( + index: DatabaseKeyIndex, + old_changed_at: crate::Revision, + new_changed_at: crate::Revision, +) { + if cfg!(debug_assertions) { + let message = BackdateViolation { + index, + old_changed_at, + new_changed_at, + backtrace: None, + }; + panic!("{message}"); + } else { + let message = BackdateViolation { + index, + old_changed_at, + new_changed_at, + backtrace: Backtrace::capture(), + }; + crate::tracing::warn!("{message}"); + } +} + +struct BackdateViolation { + index: DatabaseKeyIndex, + old_changed_at: crate::Revision, + new_changed_at: crate::Revision, + backtrace: Option, +} + +impl fmt::Display for BackdateViolation { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "query {:?} returned the same value, but the previous execution changed at {:?} and \ + the new execution changed at {:?}. This usually means the query re-executed because \ + an input changed, but then branched on untracked state (for example, a global \ + variable, a non-salsa field on the database, or filesystem state read outside salsa) \ + and no longer read that input. This is usually a bug in the query implementation. \ + Queries that branch on untracked state can also produce stale results. If the query \ + has no untracked reads, please open a Salsa issue.", + self.index, self.old_changed_at, self.new_changed_at, + )?; + + match &self.backtrace { + Some(backtrace) => write!(f, "\n{backtrace}")?, + None => {} + } + + Ok(()) + } +} From 16993799450536e592d95a03ee6a748ed7dd7d8c Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 20 Mar 2026 12:13:10 +0100 Subject: [PATCH 4/4] Clippy --- src/function/backdate.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/function/backdate.rs b/src/function/backdate.rs index 3dc1c141e..97eb626a0 100644 --- a/src/function/backdate.rs +++ b/src/function/backdate.rs @@ -102,9 +102,8 @@ impl fmt::Display for BackdateViolation { self.index, self.old_changed_at, self.new_changed_at, )?; - match &self.backtrace { - Some(backtrace) => write!(f, "\n{backtrace}")?, - None => {} + if let Some(backtrace) = &self.backtrace { + write!(f, "\n{backtrace}")?; } Ok(())