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." +} 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()) }) }) }