From 9ea525ea7912b4cf29bbfa96a71996e932fef226 Mon Sep 17 00:00:00 2001 From: kylei Date: Fri, 17 Jul 2026 09:53:28 -0700 Subject: [PATCH 1/2] Generated from a GitHub Pull Request. Run 'jf sync' on this diff to load the correct commit data. Differential Revision: D112566493 --- pyrefly/lib/test/incremental.rs | 72 ++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/pyrefly/lib/test/incremental.rs b/pyrefly/lib/test/incremental.rs index c0e485671e..8d9af0396f 100644 --- a/pyrefly/lib/test/incremental.rs +++ b/pyrefly/lib/test/incremental.rs @@ -39,6 +39,7 @@ struct IncrementalData(Arc>>>); /// Helper for writing incrementality tests. struct Incremental { data: IncrementalData, + files: Vec, require: Option, 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) -> 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::>(); 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())); } +// Synthetic defense-in-depth reproducer for https://github.com/facebook/pyrefly/issues/4171. +#[test] +#[should_panic(expected = "Transaction has uncommitted changes")] +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 From 00ad074b6cbc411edddff92f57e9ce6f08ee0458 Mon Sep 17 00:00:00 2001 From: Kyle Into Date: Fri, 17 Jul 2026 12:35:30 -0700 Subject: [PATCH 2/2] Fix deep-chain recheck panic in the MAX_EPOCHS fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: An export change propagating through a dependency chain deeper than the 100-epoch cap panicked at commit with `Transaction has uncommitted changes` (https://github.com/facebook/pyrefly/issues/4171). The fine-grained recheck path advances one dependency edge per epoch, so a long chain exhausts the cap before the change reaches the top importer. The coarse fallback was meant to invalidate the whole affected closure and recompute it, but it seeded `invalidate_rdeps` from `changed` — which the final loop iteration had already drained — making it a no-op. A single `run_step` then left `changed`/`dirty` populated, tripping the `commit_transaction` invariant. Seed the coarse invalidation from the pending `dirty` frontier instead, then recompute to a fixpoint via `stabilize_after_invalidate`. This converges quickly because lookups pull their dependencies: once the closure is invalidated, a couple of `run_step`s recompute it with correct values and drain the bookkeeping the commit invariant requires. A bounded loop with a final residual-clear keeps a pathological oscillation from hanging or committing inconsistent state. Differential Revision: D112591162 --- pyrefly/lib/state/state.rs | 53 ++++++++++++++++++++------------- pyrefly/lib/test/incremental.rs | 4 +-- 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/pyrefly/lib/state/state.rs b/pyrefly/lib/state/state.rs index de0c4c6832..ddb50e43b7 100644 --- a/pyrefly/lib/state/state.rs +++ b/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, 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( diff --git a/pyrefly/lib/test/incremental.rs b/pyrefly/lib/test/incremental.rs index 8d9af0396f..5f1f6606cb 100644 --- a/pyrefly/lib/test/incremental.rs +++ b/pyrefly/lib/test/incremental.rs @@ -880,9 +880,9 @@ fn test_overlapping_exports_cycle_detected() { assert!(res.changed.contains(&"bar".to_owned())); } -// Synthetic defense-in-depth reproducer for https://github.com/facebook/pyrefly/issues/4171. +// 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] -#[should_panic(expected = "Transaction has uncommitted changes")] fn test_deep_scc_chain_stabilizes_after_epoch_cap() { const LEVELS: usize = 208;