fix: Cleanup provisional cycle head memos when query panics (#993)

* fix: Cleanup provisional cycle head memos on panic

* Update test outputs

src/function/execute.rs

@@ -5,7 +5,7 @@ use crate::function::{Configuration, IngredientImpl};
 use crate::sync::atomic::{AtomicBool, Ordering};
 use crate::tracked_struct::Identity;
 use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase};
-use crate::zalsa_local::ActiveQueryGuard;
+use crate::zalsa_local::{ActiveQueryGuard, QueryRevisions};
 use crate::{Event, EventKind, Id};
 
 impl<C> IngredientImpl<C>
@@ -141,6 +141,7 @@ where
         // only when a cycle is actually encountered.
         let mut opt_last_provisional: Option<&Memo<'db, C>> = None;
         let mut last_stale_tracked_ids: Vec<(Identity, Id)> = Vec::new();
+        let _guard = ClearCycleHeadIfPanicking::new(self, zalsa, id, memo_ingredient_index);
 
         loop {
             let previous_memo = opt_last_provisional.or(opt_old_memo);
@@ -210,6 +211,9 @@ where
                     // `iteration_count` can't overflow as we check it against `MAX_ITERATIONS`
                     // which is less than `u32::MAX`.
                     iteration_count = iteration_count.increment().unwrap_or_else(|| {
+                        tracing::warn!(
+                            "{database_key_index:?}: execute: too many cycle iterations"
+                        );
                         panic!("{database_key_index:?}: execute: too many cycle iterations")
                     });
                     zalsa.event(&|| {
@@ -222,10 +226,7 @@ where
                     completed_query
                         .revisions
                         .update_iteration_count(iteration_count);
-                    crate::tracing::debug!(
-                        "{database_key_index:?}: execute: iterate again, revisions: {revisions:#?}",
-                        revisions = &completed_query.revisions
-                    );
+                    crate::tracing::info!("{database_key_index:?}: execute: iterate again...",);
                     opt_last_provisional = Some(self.insert_memo(
                         zalsa,
                         id,
@@ -297,3 +298,55 @@ where
         (new_value, active_query.pop())
     }
 }
+
+/// Replaces any inserted memo with a fixpoint initial memo without a value if the current thread panics.
+///
+/// A regular query doesn't insert any memo if it panics and the query
+/// simply gets re-executed if any later called query depends on the panicked query (and will panic again unless the query isn't deterministic).
+///
+/// Unfortunately, this isn't the case for cycle heads because Salsa first inserts the fixpoint initial memo and later inserts
+/// provisional memos for every iteration. Detecting whether a query has previously panicked
+/// in `fetch` (e.g., `validate_same_iteration`) and requires re-execution is probably possible but not very straightforward
+/// and it's easy to get it wrong, which results in infinite loops where `Memo::provisional_retry` keeps retrying to get the latest `Memo`
+/// but `fetch` doesn't re-execute the query for reasons.
+///
+/// Specifically, a Memo can linger after a panic, which is then incorrectly returned
+/// by `fetch_cold_cycle` because it passes the `shallow_verified_memo` check instead of inserting
+/// a new fix point initial value if that happens.
+///
+/// We could insert a fixpoint initial value here, but it seems unnecessary.
+struct ClearCycleHeadIfPanicking<'a, C: Configuration> {
+    ingredient: &'a IngredientImpl<C>,
+    zalsa: &'a Zalsa,
+    id: Id,
+    memo_ingredient_index: MemoIngredientIndex,
+}
+
+impl<'a, C: Configuration> ClearCycleHeadIfPanicking<'a, C> {
+    fn new(
+        ingredient: &'a IngredientImpl<C>,
+        zalsa: &'a Zalsa,
+        id: Id,
+        memo_ingredient_index: MemoIngredientIndex,
+    ) -> Self {
+        Self {
+            ingredient,
+            zalsa,
+            id,
+            memo_ingredient_index,
+        }
+    }
+}
+
+impl<C: Configuration> Drop for ClearCycleHeadIfPanicking<'_, C> {
+    fn drop(&mut self) {
+        if std::thread::panicking() {
+            let revisions =
+                QueryRevisions::fixpoint_initial(self.ingredient.database_key_index(self.id));
+
+            let memo = Memo::new(None, self.zalsa.current_revision(), revisions);
+            self.ingredient
+                .insert_memo(self.zalsa, self.id, memo, self.memo_ingredient_index);
+        }
+    }
+}

src/function/sync.rs

@@ -97,9 +97,11 @@ impl ClaimGuard<'_> {
             syncs.remove(&self.key_index).expect("key claimed twice?");
 
         if anyone_waiting {
+            let database_key = DatabaseKeyIndex::new(self.sync_table.ingredient, self.key_index);
             self.zalsa.runtime().unblock_queries_blocked_on(
-                DatabaseKeyIndex::new(self.sync_table.ingredient, self.key_index),
+                database_key,
                 if thread::panicking() {
+                    tracing::info!("Unblocking queries blocked on {database_key:?} after a panick");
                     WaitResult::Panicked
                 } else {
                     WaitResult::Completed

src/runtime.rs

@@ -90,7 +90,7 @@ impl Running<'_> {
             })
         });
 
-        crate::tracing::debug!(
+        crate::tracing::info!(
             "block_on: thread {thread_id:?} is blocking on {database_key:?} in thread {other_id:?}",
         );
 

tests/compile-fail/get-set-on-private-input-field.stderr

@@ -1,7 +1,7 @@
 error[E0624]: method `field` is private
   --> tests/compile-fail/get-set-on-private-input-field.rs:12:11
    |
-2  |     #[salsa::input]
+ 2 |     #[salsa::input]
    |     --------------- private method defined here
 ...
 12 |     input.field(&db);
@@ -10,7 +10,7 @@ error[E0624]: method `field` is private
 error[E0624]: method `set_field` is private
   --> tests/compile-fail/get-set-on-private-input-field.rs:13:11
    |
-2  |     #[salsa::input]
+ 2 |     #[salsa::input]
    |     --------------- private method defined here
 ...
 13 |     input.set_field(&mut db).to(23);

tests/compile-fail/input_struct_incompatibles.stderr

@@ -55,7 +55,7 @@ error: cannot find attribute `tracked` in this scope
    |
 help: consider importing one of these attribute macros
    |
-1  + use salsa::tracked;
+ 1 + use salsa::tracked;
    |
-1  + use salsa_macros::tracked;
+ 1 + use salsa_macros::tracked;
    |

tests/compile-fail/interned_struct_incompatibles.stderr

@@ -49,7 +49,7 @@ error: cannot find attribute `tracked` in this scope
    |
 help: consider importing one of these attribute macros
    |
-1  + use salsa::tracked;
+ 1 + use salsa::tracked;
    |
-1  + use salsa_macros::tracked;
+ 1 + use salsa_macros::tracked;
    |

tests/compile-fail/span-input-setter.stderr

@@ -11,10 +11,10 @@ error[E0308]: mismatched types
 note: method defined here
   --> tests/compile-fail/span-input-setter.rs:3:5
    |
-1  | #[salsa::input]
+ 1 | #[salsa::input]
    | ---------------
-2  | pub struct MyInput {
-3  |     field: u32,
+ 2 | pub struct MyInput {
+ 3 |     field: u32,
    |     ^^^^^
 help: consider mutably borrowing here
    |

tests/compile-fail/tracked_fn_return_not_update.stderr

@@ -7,18 +7,18 @@ error[E0369]: binary operation `==` cannot be applied to type `&NotUpdate`
 note: an implementation of `PartialEq` might be missing for `NotUpdate`
   --> tests/compile-fail/tracked_fn_return_not_update.rs:7:1
    |
-7  | struct NotUpdate;
+ 7 | struct NotUpdate;
    | ^^^^^^^^^^^^^^^^ must implement `PartialEq`
 help: consider annotating `NotUpdate` with `#[derive(PartialEq)]`
    |
-7  + #[derive(PartialEq)]
-8  | struct NotUpdate;
+ 7 + #[derive(PartialEq)]
+ 8 | struct NotUpdate;
    |
 
 error[E0599]: the function or associated item `maybe_update` exists for struct `UpdateDispatch<NotUpdate>`, but its trait bounds were not satisfied
   --> tests/compile-fail/tracked_fn_return_not_update.rs:10:56
    |
-7  | struct NotUpdate;
+ 7 | struct NotUpdate;
    | ---------------- doesn't satisfy `NotUpdate: PartialEq` or `NotUpdate: Update`
 ...
 10 | fn tracked_fn<'db>(db: &'db dyn Db, input: MyInput) -> NotUpdate {
@@ -40,6 +40,6 @@ note: the trait `Update` must be implemented
    | ^^^^^^^^^^^^^^^^^^^^^^^
 help: consider annotating `NotUpdate` with `#[derive(PartialEq)]`
    |
-7   + #[derive(PartialEq)]
-8   | struct NotUpdate;
+  7 + #[derive(PartialEq)]
+  8 | struct NotUpdate;
     |

tests/compile-fail/tracked_impl_incompatibles.stderr

@@ -55,7 +55,7 @@ error: unexpected token
 error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
   --> tests/compile-fail/tracked_impl_incompatibles.rs:12:1
    |
-7  | impl<'db> std::default::Default for MyTracked<'db> {
+ 7 | impl<'db> std::default::Default for MyTracked<'db> {
    | -------------------------------------------------- first implementation here
 ...
 12 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -64,7 +64,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
 error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
   --> tests/compile-fail/tracked_impl_incompatibles.rs:17:1
    |
-7  | impl<'db> std::default::Default for MyTracked<'db> {
+ 7 | impl<'db> std::default::Default for MyTracked<'db> {
    | -------------------------------------------------- first implementation here
 ...
 17 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -73,7 +73,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
 error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
   --> tests/compile-fail/tracked_impl_incompatibles.rs:22:1
    |
-7  | impl<'db> std::default::Default for MyTracked<'db> {
+ 7 | impl<'db> std::default::Default for MyTracked<'db> {
    | -------------------------------------------------- first implementation here
 ...
 22 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -82,7 +82,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
 error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
   --> tests/compile-fail/tracked_impl_incompatibles.rs:27:1
    |
-7  | impl<'db> std::default::Default for MyTracked<'db> {
+ 7 | impl<'db> std::default::Default for MyTracked<'db> {
    | -------------------------------------------------- first implementation here
 ...
 27 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -91,7 +91,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
 error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
   --> tests/compile-fail/tracked_impl_incompatibles.rs:32:1
    |
-7  | impl<'db> std::default::Default for MyTracked<'db> {
+ 7 | impl<'db> std::default::Default for MyTracked<'db> {
    | -------------------------------------------------- first implementation here
 ...
 32 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -100,7 +100,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
 error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
   --> tests/compile-fail/tracked_impl_incompatibles.rs:37:1
    |
-7  | impl<'db> std::default::Default for MyTracked<'db> {
+ 7 | impl<'db> std::default::Default for MyTracked<'db> {
    | -------------------------------------------------- first implementation here
 ...
 37 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -109,7 +109,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
 error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
   --> tests/compile-fail/tracked_impl_incompatibles.rs:42:1
    |
-7  | impl<'db> std::default::Default for MyTracked<'db> {
+ 7 | impl<'db> std::default::Default for MyTracked<'db> {
    | -------------------------------------------------- first implementation here
 ...
 42 | impl<'db> std::default::Default for MyTracked<'db> {
@@ -118,7 +118,7 @@ error[E0119]: conflicting implementations of trait `Default` for type `MyTracked
 error[E0119]: conflicting implementations of trait `Default` for type `MyTracked<'_>`
   --> tests/compile-fail/tracked_impl_incompatibles.rs:47:1
    |
-7  | impl<'db> std::default::Default for MyTracked<'db> {
+ 7 | impl<'db> std::default::Default for MyTracked<'db> {
    | -------------------------------------------------- first implementation here
 ...
 47 | impl<'db> std::default::Default for MyTracked<'db> {

tests/compile-fail/tracked_struct_not_update.stderr

@@ -29,6 +29,6 @@ note: the trait `Update` must be implemented
   = note: this error originates in the attribute macro `salsa::tracked` (in Nightly builds, run with -Z macro-backtrace for more info)
 help: consider annotating `NotUpdate` with `#[derive(PartialEq)]`
   |
-7   + #[derive(PartialEq)]
-8   | struct NotUpdate;
+  7 + #[derive(PartialEq)]
+  8 | struct NotUpdate;
     |

tests/compile_fail.rs

@@ -1,6 +1,6 @@
 #![cfg(all(feature = "inventory", feature = "persistence"))]
 
-#[rustversion::all(stable, since(1.89))]
+#[rustversion::all(stable, since(1.90))]
 #[test]
 fn compile_fail() {
     let t = trybuild::TestCases::new();