bug: Fix missing cycle inputs

Author: MichaReiser

src/active_query.rs

@@ -66,6 +66,20 @@ pub(crate) struct ActiveQuery {
 }
 
 impl ActiveQuery {
+    pub(super) fn seed_iteration(
+        &mut self,
+        durability: Durability,
+        changed_at: Revision,
+        edges: &[QueryEdge],
+        untracked_read: bool,
+    ) {
+        assert!(self.input_outputs.is_empty());
+        self.input_outputs = edges.iter().cloned().collect();
+        self.durability = self.durability.min(durability);
+        self.changed_at = self.changed_at.max(changed_at);
+        self.untracked_read = untracked_read;
+    }
+
     pub(super) fn add_read(
         &mut self,
         input: DatabaseKeyIndex,

src/function/backdate.rs

@@ -1,6 +1,7 @@
 use crate::function::memo::Memo;
 use crate::function::{Configuration, IngredientImpl};
 use crate::zalsa_local::QueryRevisions;
+use crate::DatabaseKeyIndex;
 
 impl<C> IngredientImpl<C>
 where
@@ -12,6 +13,7 @@ where
     pub(super) fn backdate_if_appropriate<'db>(
         &self,
         old_memo: &Memo<C::Output<'db>>,
+        index: DatabaseKeyIndex,
         revisions: &mut QueryRevisions,
         value: &C::Output<'db>,
     ) {
@@ -24,7 +26,7 @@ where
                 && C::values_equal(old_value, value)
             {
                 tracing::debug!(
-                    "value is equal, back-dating to {:?}",
+                    "{index:?} value is equal, back-dating to {:?}",
                     old_memo.revisions.changed_at,
                 );
 

src/function/execute.rs

@@ -101,7 +101,7 @@ where
             // really change, even if some of its inputs have. So we can
             // "backdate" its `changed_at` revision to be the same as the
             // old value.
-            self.backdate_if_appropriate(old_memo, &mut revisions, &new_value);
+            self.backdate_if_appropriate(old_memo, database_key_index, &mut revisions, &new_value);
 
             // Diff the new outputs with the old, to discard any no-longer-emitted
             // outputs and update the tracked struct IDs for seeding the next revision.
@@ -115,12 +115,20 @@ where
                 provisional,
             );
         }
-        self.insert_memo(
+        let memo = self.insert_memo(
             zalsa,
             id,
             Memo::new(Some(new_value), zalsa.current_revision(), revisions),
             memo_ingredient_index,
-        )
+        );
+
+        tracing::info!(
+            "{:?}: executed query {:?}",
+            database_key_index,
+            memo.tracing_debug()
+        );
+
+        memo
     }
 
     #[inline]
@@ -255,27 +263,25 @@ where
         current_revision: Revision,
         id: Id,
     ) -> (C::Output<'db>, QueryRevisions) {
-        // If we already executed this query once, then use the tracked-struct ids from the
-        // previous execution as the starting point for the new one.
         if let Some(old_memo) = opt_old_memo {
+            // If we already executed this query once, then use the tracked-struct ids from the
+            // previous execution as the starting point for the new one.
             active_query.seed_tracked_struct_ids(&old_memo.revisions.tracked_struct_ids);
-        }
-
-        // Query was not previously executed, or value is potentially
-        // stale, or value is absent. Let's execute!
-        let new_value = C::execute(db, C::id_to_input(db, id));
 
-        if let Some(old_memo) = opt_old_memo {
             // Copy over all outputs from a previous iteration.
             // This is necessary to ensure that tracked struct created during the previous iteration
             // (and are owned by the query) are alive even if the query in this iteration no longer creates them.
             // The query not re-creating the tracked struct doesn't guarantee that there
             // aren't any other queries depending on it.
             if old_memo.may_be_provisional() && old_memo.verified_at.load() == current_revision {
-                active_query.append_outputs(old_memo.revisions.origin.outputs());
+                active_query.seed_iteration(&old_memo.revisions);
             }
         }
 
+        // Query was not previously executed, or value is potentially
+        // stale, or value is absent. Let's execute!
+        let new_value = C::execute(db, C::id_to_input(db, id));
+
         (new_value, active_query.pop())
     }
 }

src/function/fetch.rs

@@ -4,7 +4,7 @@ use crate::function::sync::ClaimResult;
 use crate::function::{Configuration, IngredientImpl, VerifyResult};
 use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase};
 use crate::zalsa_local::QueryRevisions;
-use crate::Id;
+use crate::{Durability, Id};
 
 impl<C> IngredientImpl<C>
 where
@@ -20,6 +20,12 @@ where
 
         self.lru.record_use(id);
 
+        tracing::debug!(
+            "now: {:?}, medium: {:?}",
+            zalsa.current_revision(),
+            zalsa.last_changed_revision(Durability::MEDIUM)
+        );
+
         zalsa_local.report_tracked_read(
             self.database_key_index(id),
             memo.revisions.durability,

src/function/maybe_changed_after.rs

@@ -188,7 +188,7 @@ where
         memo: &Memo<C::Output<'_>>,
     ) -> Option<ShallowUpdate> {
         tracing::debug!(
-            "{database_key_index:?}: shallow_verify_memo(memo = {memo:#?})",
+            "{database_key_index:?}: shallow_verify_memo(memo = {memo:?})",
             memo = memo.tracing_debug()
         );
         let verified_at = memo.verified_at.load();

src/function/memo.rs

@@ -294,7 +294,10 @@ impl<V> Memo<V> {
                         },
                     )
                     .field("verified_at", &self.memo.verified_at)
-                    .field("revisions", &self.memo.revisions)
+                    .field("changed_at", &self.memo.revisions.changed_at)
+                    .field("durability", &self.memo.revisions.durability)
+                    .field("cycle_heads", &self.memo.revisions.cycle_heads)
+                    // .field("revisions", &self.memo.revisions)
                     .finish()
             }
         }

src/function/specify.rs

@@ -76,7 +76,7 @@ where
 
         let memo_ingredient_index = self.memo_ingredient_index(zalsa, key);
         if let Some(old_memo) = self.get_memo_from_table_for(zalsa, key, memo_ingredient_index) {
-            self.backdate_if_appropriate(old_memo, &mut revisions, &value);
+            self.backdate_if_appropriate(old_memo, database_key_index, &mut revisions, &value);
             self.diff_outputs(
                 zalsa,
                 db,

src/zalsa_local.rs

@@ -372,6 +372,7 @@ pub(crate) struct QueryRevisions {
 impl QueryRevisions {
     pub(crate) fn fixpoint_initial(query: DatabaseKeyIndex, revision: Revision) -> Self {
         Self {
+            // TODO: I think it would be fine to use Revision::start here
             changed_at: revision,
             durability: Durability::MAX,
             origin: QueryOrigin::FixpointInitial,
@@ -424,6 +425,16 @@ impl QueryOrigin {
         };
         opt_edges.into_iter().flat_map(|edges| edges.outputs())
     }
+
+    pub(crate) fn edges(&self) -> &[QueryEdge] {
+        let opt_edges = match self {
+            QueryOrigin::Derived(edges) | QueryOrigin::DerivedUntracked(edges) => Some(edges),
+            QueryOrigin::Assigned(_) | QueryOrigin::FixpointInitial => None,
+        };
+        opt_edges
+            .map(|edges| &*edges.input_outputs)
+            .unwrap_or_default()
+    }
 }
 
 /// The edges between a memoized value and other queries in the dependency graph.
@@ -508,18 +519,17 @@ impl ActiveQueryGuard<'_> {
     }
 
     /// Append the given `outputs` to the query's output list.
-    pub(crate) fn append_outputs<I>(&self, outputs: I)
-    where
-        I: IntoIterator<Item = DatabaseKeyIndex> + UnwindSafe,
-    {
+    pub(crate) fn seed_iteration(&self, previous: &QueryRevisions) {
+        let durability = previous.durability;
+        let changed_at = previous.changed_at;
+        let edges = previous.origin.edges();
+        let untracked_read = matches!(previous.origin, QueryOrigin::DerivedUntracked(_));
+
         self.local_state.with_query_stack_mut(|stack| {
             #[cfg(debug_assertions)]
             assert_eq!(stack.len(), self.push_len);
             let frame = stack.last_mut().unwrap();
-
-            for output in outputs {
-                frame.add_output(output);
-            }
+            frame.seed_iteration(durability, changed_at, edges, untracked_read);
         })
     }
 

tests/cycle_conditional_input_dependency.rs

@@ -0,0 +1,73 @@
+//! Test for cycle where only the first iteration of a query depends on the input value.
+mod common;
+
+use crate::common::EventLoggerDatabase;
+use salsa::{CycleRecoveryAction, Database, Durability, Setter};
+
+#[salsa::input(debug)]
+struct Input {
+    value: u32,
+    max: u32,
+}
+
+#[salsa::interned(debug)]
+struct Output<'db> {
+    #[return_ref]
+    value: u32,
+}
+
+#[salsa::tracked(cycle_fn=query_a_recover, cycle_initial=query_a_initial)]
+fn query_a<'db>(db: &'db dyn salsa::Database, input: Input) -> u32 {
+    query_b(db, input)
+}
+
+// Query b also gets low durability because of query_a. How can we avoid that?
+// Or is the bug that we loose the durability somehow?
+#[salsa::tracked]
+fn query_b<'db>(db: &'db dyn salsa::Database, input: Input) -> u32 {
+    let value = query_a(db, input);
+
+    if value < input.max(db) {
+        // Only the first iteration depends on value but the entire
+        // cycle must re-run if input changes.
+        let result = value + input.value(db);
+        Output::new(db, result);
+        result
+    } else {
+        value
+    }
+}
+
+// Note: Also requires same output or backdating won't happen.  but other query output needs to be different at least once to fixpint
+fn query_a_initial<'db>(db: &'db dyn Database, input: Input) -> u32 {
+    0
+}
+
+fn query_a_recover<'db>(
+    _db: &'db dyn Database,
+    _output: &u32,
+    _count: u32,
+    _input: Input,
+) -> CycleRecoveryAction<u32> {
+    CycleRecoveryAction::Iterate
+}
+
+#[test_log::test]
+fn main() {
+    let mut db = EventLoggerDatabase::default();
+
+    let input = Input::builder(4, 5).durability(Durability::MEDIUM).new(&db);
+
+    {
+        let result = query_a(&db, input);
+
+        assert_eq!(result, 8);
+    }
+
+    {
+        input.set_value(&mut db).to(3);
+
+        let result = query_a(&db, input);
+        assert_eq!(result, 6);
+    }
+}