From b468c2687cbd63394112c4990b02746cb1d9a531 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Sun, 18 Oct 2026 00:18:00 +0200 Subject: [PATCH 1/2] mason: make two Windows-flaky tests causal (configure cancel ladder, checkpoint dir rollback) configure_cancellation_stops_the_git_probe_retry_ladder failed on Windows CI with "stubbed git probe did not start": the test gave configure a 2 s wall-clock budget to reach the stubbed git probe, and everything configure does first (real git spawns, storage setup) can exceed that on a loaded runner. The stub now holds its first attempt in flight until the test releases it, and the test waits for the probe to start or for configure to finish without reaching it (120 s hang guard only). Because the cancel now lands while attempt 1 is known to be held, the cancellation assertion is tightened from "at most one more attempt" to "no further attempt". checkpoint_restore_failure_removes_created_parent_dirs failed on Windows CI inside its fixture: tempfile::NamedTempFile::new() in the runner's shared TEMP returned "Access is denied" (os error 5) before any checkpoint code ran. The checkpoint_file helper now builds the entry in memory. Reading the test also showed it never reached the rollback: its blocking path was a pre-existing directory, which the pre-restore snapshot refuses before anything is written, so a no-op rollback_created_dirs left it green. The failing second path is now the directory the first restore creates, so the write really happens and really fails, and the rollback must remove the created parents. The old "pre-existing blocking directory remains" check is replaced by asserting the pre-existing parent directory remains, since there is no pre-existing blocking directory any more. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/checkpoint.rs | 38 +++++++++++++++----- crates/aft/src/commands/configure.rs | 54 ++++++++++++++++++++++------ crates/aft/src/search_index.rs | 31 ++++++++++------ 3 files changed, 94 insertions(+), 29 deletions(-) diff --git a/crates/aft/src/checkpoint.rs b/crates/aft/src/checkpoint.rs index ff9cbad11..361efc626 100644 --- a/crates/aft/src/checkpoint.rs +++ b/crates/aft/src/checkpoint.rs @@ -2242,10 +2242,22 @@ mod tests { } } + /// A regular-file checkpoint entry built in memory, the shape a durable + /// checkpoint has after loading from disk (no captured metadata or mode). + /// + /// It deliberately touches no filesystem. Round-tripping through a scratch + /// file in the shared system temp directory made unrelated tests fail on + /// Windows CI whenever creating that file was refused with "Access is + /// denied", which happens there transiently, before any checkpoint code + /// runs. fn checkpoint_file(content: &str) -> CheckpointFile { - let file = tempfile::NamedTempFile::new().unwrap(); - fs::write(file.path(), content).unwrap(); - CheckpointFile::read(file.path()).unwrap() + CheckpointFile { + metadata: None, + mode: None, + kind: CheckpointFileKind::Regular { + bytes: content.as_bytes().into(), + }, + } } #[cfg(unix)] @@ -3424,9 +3436,16 @@ mod tests { fn checkpoint_restore_failure_removes_created_parent_dirs() { let dir = tempfile::tempdir().unwrap(); let missing_root = dir.path().join("created"); - let path_a = missing_root.join("nested").join("a.txt"); - let path_b = dir.path().join("blocking-dir"); - fs::create_dir(&path_b).unwrap(); + let nested = missing_root.join("nested"); + let path_a = nested.join("a.txt"); + // Neither path exists when the restore snapshots the current state, so + // both snapshots succeed and the restore starts writing. Restoring + // `path_a` creates `nested` as its parent directory; restoring `path_b` + // then fails because `path_b` is that same `nested`, now a directory, + // and a regular file cannot be written over it. A directory that + // already blocks a path would not do: the pre-restore snapshot refuses + // it before anything is written, so no rollback would run at all. + let path_b = nested.clone(); let checkpoint = Checkpoint { name: "dir-cleanup".to_string(), @@ -3442,14 +3461,17 @@ mod tests { assert!( result.is_err(), - "second restore write should fail on directory" + "restoring a regular file over a directory must fail" ); assert!(!path_a.exists(), "restored file should be rolled back"); assert!( !missing_root.exists(), "new parent directories should be removed on rollback" ); - assert!(path_b.is_dir(), "pre-existing blocking directory remains"); + assert!( + dir.path().is_dir(), + "a parent directory that existed before the restore remains" + ); } #[cfg(unix)] diff --git a/crates/aft/src/commands/configure.rs b/crates/aft/src/commands/configure.rs index ea60f0b15..8cfd0ade7 100644 --- a/crates/aft/src/commands/configure.rs +++ b/crates/aft/src/commands/configure.rs @@ -12416,11 +12416,12 @@ mod tests { init_git_fixture(&root); let canonical_root = std::fs::canonicalize(&root).unwrap(); let probe_attempts = Arc::new(AtomicUsize::new(0)); + let (release_probe, probe_release) = mpsc::channel(); let _probe_guard = - crate::search_index::force_git_root_commit_probe_slow_transient_for_paths_for_test( + crate::search_index::force_git_root_commit_probe_held_transient_for_paths_for_test( vec![canonical_root.clone()], - Duration::from_millis(150), Arc::clone(&probe_attempts), + probe_release, ); let ctx = Arc::new(AppContext::new( @@ -12443,26 +12444,56 @@ mod tests { }))], })); let request_id = request.id.clone(); - let (response_rx, cancellation) = executor.submit_cancellable_async( + let (mut response_rx, cancellation) = executor.submit_cancellable_async( root_id.clone(), crate::executor::Lane::Mutating, request_id, Box::new(move |ctx| super::handle_configure(&request, ctx)), ); - - let probe_deadline = Instant::now() + Duration::from_secs(2); + // Rebound here so it is the last binding and drops first if the test + // panics: until it drops, the held probe attempt keeps the executor's + // worker busy (the executor joins it on drop) and keeps the + // probe-override lock that the probe guard's drop takes. + let release_probe = release_probe; + + // Cancelling only tests the ladder once the ladder is running, so wait + // for the first probe attempt. Everything configure does before it + // (real git spawns, storage setup) can take seconds on a loaded Windows + // runner, so the wait ends on one of the two events that settle it: + // the probe starting, or configure finishing without ever reaching it. + // The held stub cannot finish the ladder on its own, so the second + // event really does mean the probe was never reached. The deadline is + // only a hang guard. + let hang_guard = Instant::now() + Duration::from_secs(120); while probe_attempts.load(Ordering::SeqCst) == 0 { + match response_rx.try_recv() { + Ok(response) => panic!( + "configure finished without reaching the stubbed git probe: {}", + response.data + ), + Err(tokio::sync::oneshot::error::TryRecvError::Closed) => { + panic!( + "configure dropped its completion sender before the stubbed git probe ran" + ) + } + Err(tokio::sync::oneshot::error::TryRecvError::Empty) => {} + } assert!( - Instant::now() < probe_deadline, - "stubbed git probe did not start" + Instant::now() < hang_guard, + "configure neither reached the stubbed git probe nor finished" ); std::thread::sleep(Duration::from_millis(5)); } + // The first attempt is held in flight, so it is the only one counted. let attempts_at_cancel = probe_attempts.load(Ordering::SeqCst); + assert_eq!(attempts_at_cancel, 1); assert_eq!( executor.cancel_job(&root_id, &cancellation), crate::executor::JobCancelOutcome::RunningSignalled ); + release_probe + .send(()) + .expect("held git probe is waiting for release"); // The probe returns Transient, so an uncancelled configure keeps going // round the retry ladder. What cancellation has to do is stop it, and @@ -12482,11 +12513,12 @@ mod tests { }); assert!(!response.success); assert_eq!(response.data["code"], "request_cancelled"); - // At most the attempt already in flight when the signal landed may - // finish; anything beyond that is the ladder continuing past a cancel. + // The cancel landed while the first attempt was held in flight, so the + // ladder must stop when that attempt returns; any further attempt is + // the ladder continuing past a cancel. let attempts_after = probe_attempts.load(Ordering::SeqCst); - assert!( - attempts_after <= attempts_at_cancel + 1, + assert_eq!( + attempts_after, attempts_at_cancel, "cancelled configure kept retrying the git probe: {attempts_at_cancel} attempts at cancel, {attempts_after} after" ); } diff --git a/crates/aft/src/search_index.rs b/crates/aft/src/search_index.rs index de561644f..db4856be7 100644 --- a/crates/aft/src/search_index.rs +++ b/crates/aft/src/search_index.rs @@ -7279,23 +7279,34 @@ pub(crate) fn force_git_root_commit_probe_transient_for_paths_for_test( } #[cfg(test)] -/// Force the git root-commit probe to be slow and transient for the given -/// roots, counting the attempts it makes. +/// Force the git root-commit probe to return Transient for the given roots, +/// counting the attempts it makes and holding the first attempt in flight +/// until `release` delivers a message or its sender is dropped. /// -/// The count is what lets a caller assert that cancellation actually stopped -/// the retry ladder: a transient result sends the probe round again, so a -/// cancelled configure must stop making attempts, which is observable without -/// reference to how long anything took. -pub(crate) fn force_git_root_commit_probe_slow_transient_for_paths_for_test( +/// A transient result sends the probe round the retry ladder again, so a +/// cancelled configure must stop making attempts. Holding the first attempt +/// lets a test cancel while that attempt is known to be in flight, so the +/// attempt count before and after the cancel is fixed by the order of events +/// rather than by how fast the machine runs. Dropping the sender also releases +/// the hold, so a test that panics never leaves the probing thread parked. +pub(crate) fn force_git_root_commit_probe_held_transient_for_paths_for_test( roots: Vec, - delay: Duration, attempts: Arc, + release: std::sync::mpsc::Receiver<()>, ) -> GitRootCommitProbeOverrideGuard { + let release = Mutex::new(Some(release)); install_git_root_commit_probe_override_for_test(move |project_root| { roots.iter().any(|root| root == project_root).then(|| { attempts.fetch_add(1, Ordering::SeqCst); - std::thread::sleep(delay); - RootCommitProbe::Transient("stubbed slow git probe".to_string()) + let first_attempt_hold = release + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .take(); + if let Some(release) = first_attempt_hold { + // Either outcome ends the hold: a message, or a dropped sender. + let _ = release.recv(); + } + RootCommitProbe::Transient("stubbed held git probe".to_string()) }) }) } From 9d68b1828556d927ec1eca09f5f1a8a6f34ee57d Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Sun, 18 Oct 2026 01:14:43 +0200 Subject: [PATCH 2/2] release notes and search-quality descriptor for windows-configure-checkpoint-tests Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../land-windows-configure-checkpoint-tests.json | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 benchmarks/aft-search/slice-descriptors/land-windows-configure-checkpoint-tests.json diff --git a/benchmarks/aft-search/slice-descriptors/land-windows-configure-checkpoint-tests.json b/benchmarks/aft-search/slice-descriptors/land-windows-configure-checkpoint-tests.json new file mode 100644 index 000000000..cba46e5e6 --- /dev/null +++ b/benchmarks/aft-search/slice-descriptors/land-windows-configure-checkpoint-tests.json @@ -0,0 +1,12 @@ +{ + "targeted_mechanism": "none", + "kind": "harness", + "slice_class": "engine_unwired", + "fixtures": [ + "crates/aft/tests/integration/tool_call_parity_test.rs" + ], + "fenced_files": [ + "crates/aft/src/search_index.rs" + ], + "note": "The only fenced change renames a test-only git probe stub in search_index.rs. Ranking and routing are unchanged; benchmark rows must remain byte-identical." +}