Fix deep-chain recheck panic in the MAX_EPOCHS fallback

pyrefly/lib/state/state.rs

@@ -2101,18 +2101,32 @@ impl<'a> Transaction<'a> {
         // cases. This avoids false positives where independent exports happen to
         // be processed in the same module across different epochs.
         //
-        // As a defense-in-depth measure, we also cap the total number of epochs to prevent
-        // runaway computation in case of unforeseen edge cases.
+        // As a defense-in-depth measure, we also cap the number of fine-grained epochs. On
+        // a cycle or on hitting the cap we abandon fine-grained propagation and switch to
+        // coarse mode: invalidate the entire rdep closure of the modules that just changed,
+        // then keep recomputing until stable. Coarse mode settles in a few epochs (unlike
+        // the one-edge-per-epoch fine-grained path) because lookups pull their dependencies
+        // — once the closure is dirty, a `run_step` recomputes it with correct values and
+        // the rest just drain the `changed`/`dirty` bookkeeping that `commit_transaction`
+        // requires to be empty.
         const MAX_EPOCHS: usize = 100;
         let mut seen_deps: SmallMap<ArcId<ModuleDataMut>, ModuleChanges> = SmallMap::new();
+        let mut coarse = false;
 
-        for i in 1..=MAX_EPOCHS {
+        for i in 1..=(2 * MAX_EPOCHS) {
             debug!("Running epoch {i} of run {run_number}");
             self.run_step(handles, require, custom_thread_pool)?;
             let changed = mem::take(&mut *self.data.changed.lock());
             if changed.is_empty() {
+                // `dirty` is only extended alongside `changed`, so no changes means the
+                // transaction is clean and safe to commit.
                 return Ok(());
             }
+            if coarse {
+                // Closure already invalidated; keep draining until stable.
+                continue;
+            }
+
             // Check for cycle: any module with overlapping export changes indicates
             // a mutable dependency cycle (e.g., A depends on B depends on A, and exports
             // keep oscillating).
@@ -2122,16 +2136,14 @@ impl<'a> Transaction<'a> {
                     .is_some_and(|seen| seen.overlaps(changed_dep))
             });
 
-            if has_cycle {
-                debug!(
-                    "Mutable dependency cycle detected: overlapping export changes. \
-                     Invalidating cycle."
-                );
-                // We are in a cycle of mutual dependencies, so give up.
-                // Just invalidate everything in the cycle and recompute it all.
-                // Use coarse-grained invalidation to ensure all cyclic modules reach stable state
+            if has_cycle || i >= MAX_EPOCHS {
+                debug!("Abandoning fine-grained propagation; coarsely invalidating closure.");
+                // Give up on fine-grained propagation — either a mutual-dependency cycle, or
+                // a chain deeper than the cap. Coarsely invalidate every transitive rdep of
+                // the modules that just changed, then let the loop recompute to a fixpoint.
                 self.invalidate_rdeps(changed.into_map(|(m, _)| m));
-                return self.run_step(handles, require, custom_thread_pool);
+                coarse = true;
+                continue;
             }
 
             // No cycle detected. Merge the new deps into our tracking set.
@@ -2146,16 +2158,17 @@ impl<'a> Transaction<'a> {
                 }
             }
         }
-        // If we reach here, we've exceeded MAX_EPOCHS without stabilizing.
-        // This should be extremely rare and indicates an unexpected edge case.
-        // Force invalidation and one final run as a fallback.
+
+        // Even coarse invalidation did not stabilize (a pathological oscillation). Clear the
+        // residual `changed`/`dirty` so the transaction commits in a consistent state
+        // (last-computed values) rather than tripping a `commit_transaction` assertion.
         tracing::warn!(
-            "Exceeded maximum epochs ({MAX_EPOCHS}) without stabilizing. \
-             This may indicate an unexpected dependency pattern. Forcing invalidation."
+            "Recheck did not stabilize within {} epochs; clearing residual state to commit consistently.",
+            2 * MAX_EPOCHS
         );
-        let changed = mem::take(&mut *self.data.changed.lock());
-        self.invalidate_rdeps(changed.into_map(|(m, _)| m));
-        self.run_step(handles, require, custom_thread_pool)
+        self.data.changed.lock().clear();
+        self.data.dirty.lock().clear();
+        Ok(())
     }
 
     pub fn run(

pyrefly/lib/test/incremental.rs

@@ -39,6 +39,7 @@ struct IncrementalData(Arc<Mutex<SmallMap<ModuleName, Arc<String>>>>);
 /// Helper for writing incrementality tests.
 struct Incremental {
     data: IncrementalData,
+    files: Vec<String>,
     require: Option<Require>,
     state: State,
     to_set: Vec<(String, String)>,
@@ -74,13 +75,17 @@ impl Incremental {
     const USER_FILES: &[&str] = &["main", "foo", "bar", "baz"];
 
     fn new() -> Self {
+        Self::with_files(Self::USER_FILES.map(|x| (*x).to_owned()))
+    }
+
+    fn with_files(files: Vec<String>) -> Self {
         init_test();
         let data = IncrementalData::default();
 
         let mut config = ConfigFile::default();
         config.python_environment.set_empty_to_default();
         let mut sourcedb = MapDatabase::new(config.get_sys_info());
-        for file in Self::USER_FILES {
+        for file in &files {
             sourcedb.insert(
                 ModuleName::from_str(file),
                 ModulePath::memory(PathBuf::from(file)),
@@ -92,6 +97,7 @@ impl Incremental {
 
         Self {
             data: data.dupe(),
+            files,
             require: None,
             state: State::new(ConfigFinder::new_constant(config), TEST_THREAD_COUNT),
             to_set: Vec::new(),
@@ -141,7 +147,11 @@ impl Incremental {
             None,
             None,
         );
-        let loaded = Self::USER_FILES.map(|x| self.handle(x));
+        let loaded = self
+            .files
+            .iter()
+            .map(|x| self.handle(x))
+            .collect::<Vec<_>>();
         let errors = self.state.transaction().get_errors(&loaded);
         let project_root = PathBuf::new();
         print_errors(project_root.as_path(), &errors.collect_display_errors());
@@ -870,6 +880,64 @@ fn test_overlapping_exports_cycle_detected() {
     assert!(res.changed.contains(&"bar".to_owned()));
 }
 
+// Regression test for https://github.com/facebook/pyrefly/issues/4171: an export change
+// propagating through a chain deeper than MAX_EPOCHS must still converge, not panic.
+#[test]
+fn test_deep_scc_chain_stabilizes_after_epoch_cap() {
+    const LEVELS: usize = 208;
+
+    let mut files = vec!["leaf".to_owned(), "main".to_owned()];
+    for level in 0..LEVELS {
+        files.push(format!("a_{level}"));
+        files.push(format!("b_{level}"));
+    }
+    let mut i = Incremental::with_files(files);
+
+    i.set("leaf", "value: int = 0");
+    for level in 0..LEVELS {
+        let previous = if level == 0 {
+            "leaf".to_owned()
+        } else {
+            format!("a_{}", level - 1)
+        };
+        i.set(
+            &format!("a_{level}"),
+            &format!(
+                "from typing import TYPE_CHECKING\nfrom {previous} import value as value\nif TYPE_CHECKING:\n    from b_{level} import Marker\n"
+            ),
+        );
+        i.set(
+            &format!("b_{level}"),
+            &format!(
+                "from typing import TYPE_CHECKING\nif TYPE_CHECKING:\n    from a_{level} import value\nclass Marker: pass\n"
+            ),
+        );
+    }
+    i.set(
+        "main",
+        &format!(
+            "from a_{} import value\nobserved: int = value\n",
+            LEVELS - 1
+        ),
+    );
+
+    let initial = i.unchecked(&["main"]);
+    assert_eq!(initial.errors.collect_display_errors().len(), 0);
+
+    i.set("leaf", "value: str = 'changed'");
+    let changed = i.unchecked(&["main"]);
+    let errors = changed
+        .errors
+        .collect_display_errors()
+        .map(|error| error.msg().to_owned());
+    assert_eq!(
+        errors.len(),
+        1,
+        "expected final importer error, got {errors:?}"
+    );
+    assert!(errors[0].contains("not assignable to `int`"), "{errors:?}");
+}
+
 /// Test a more complex non-overlapping case with a chain of re-exports.
 ///
 /// This models a longer dependency chain where multiple intermediate modules