fix: incorrect caching for queries participating in fixpoint (#843)

Author: MichaReiser

src/function/backdate.rs

@@ -17,6 +17,15 @@ where
         revisions: &mut QueryRevisions,
         value: &C::Output<'db>,
     ) {
+        // We've seen issues where queries weren't re-validated when backdating provisional values
+        // in ty. This is more of a bandaid because we're close to a release and don't have the time to prove
+        // right now whether backdating could be made safe for queries participating in queries.
+        // TODO: Write a test that demonstrates that backdating queries participating in a cycle isn't safe
+        // OR write many tests showing that it is (and fixing the case where it didn't correctly account for today).
+        if !revisions.cycle_heads.is_empty() {
+            return;
+        }
+
         if let Some(old_value) = &old_memo.value {
             // Careful: if the value became less durable than it
             // used to be, that is a "breaking change" that our

src/function/maybe_changed_after.rs

@@ -412,20 +412,14 @@ where
                 // in rev 1 but not in rev 2.
                 VerifyResult::changed()
             }
-            QueryOrigin::FixpointInitial => {
-                let is_provisional = old_memo.may_be_provisional();
-
-                // If the value is from the same revision but is still provisional, consider it changed
-                // because we're now in a new iteration.
-                if shallow_update_possible && is_provisional {
-                    return VerifyResult::Changed(CycleHeads::initial(database_key_index));
-                }
-
-                VerifyResult::Unchanged(
-                    InputAccumulatedValues::Empty,
-                    CycleHeads::initial(database_key_index),
-                )
-            }
+            // Return `Unchanged` similar to the initial value that we insert
+            // when we hit the cycle. Any dependencies accessed when creating the fixpoint initial
+            // are tracked by the outer query. Nothing should have changed assuming that the
+            // fixpoint initial function is deterministic.
+            QueryOrigin::FixpointInitial => VerifyResult::Unchanged(
+                InputAccumulatedValues::Empty,
+                CycleHeads::initial(database_key_index),
+            ),
             QueryOrigin::DerivedUntracked(_) => {
                 // Untracked inputs? Have to assume that it changed.
                 VerifyResult::changed()
@@ -458,8 +452,11 @@ where
                                     last_verified_at,
                                     !cycle_heads.is_empty(),
                                 ) {
-                                    VerifyResult::Changed(_) => {
-                                        break 'cycle VerifyResult::Changed(cycle_heads)
+                                    VerifyResult::Changed(heads) => {
+                                        // Carry over the heads from the inner query to avoid
+                                        // backdating the outer query.
+                                        cycle_heads.extend(&heads);
+                                        break 'cycle VerifyResult::Changed(cycle_heads);
                                     }
                                     VerifyResult::Unchanged(input_accumulated, cycles) => {
                                         cycle_heads.extend(&cycles);