From 5423abaabda2cebef71c0b07a0185bf658a4a7de Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Mon, 26 Oct 2026 03:56:42 +0100 Subject: [PATCH 1/8] mason: reclaim deleted roots without kill-thread loops Use a non-waiting root-scoped task predicate at every deleted-root eviction gate. Root deletion is a definitive scope end: terminate terminal survivor groups as well as running tasks, and sample unknown terminal groups off the frame loop before declaring them settled. Cache task root identities at registration, space per-root kill submissions by 30 seconds, and expose typed in-flight/backoff/registry-busy census reasons. Regression fixtures use only owned isolated process groups. Before the fix the four regressions failed, including 100 kill threads for 100 idle sweeps; focused Linux tests and the Windows compile gate pass. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../next/reclaim-deleted-roots.md | 1 + crates/aft/src/bash_background/registry.rs | 118 ++++++++- .../src/bash_background/root_reclaim_tests.rs | 167 ++++++++++++ crates/aft/src/context.rs | 46 +++- .../src/subc/deleted_root_reclaim_tests.rs | 239 ++++++++++++++++++ crates/aft/src/subc/health.rs | 40 ++- crates/aft/src/subc/mod.rs | 55 +++- 7 files changed, 640 insertions(+), 26 deletions(-) create mode 100644 .alfonso/release-notes/next/reclaim-deleted-roots.md create mode 100644 crates/aft/src/bash_background/root_reclaim_tests.rs create mode 100644 crates/aft/src/subc/deleted_root_reclaim_tests.rs diff --git a/.alfonso/release-notes/next/reclaim-deleted-roots.md b/.alfonso/release-notes/next/reclaim-deleted-roots.md new file mode 100644 index 000000000..6d4b2f212 --- /dev/null +++ b/.alfonso/release-notes/next/reclaim-deleted-roots.md @@ -0,0 +1 @@ +Fixed deleted project roots being retained indefinitely by terminal bash task records or another root's tasks. Reclaim now stops the deleted root's running tasks and terminal-task survivor groups before releasing its artifacts. Failed cleanup attempts are spaced at least 30 seconds apart per root, with in-flight, retry-backoff, and busy-registry reasons reported in the idle-reap census. diff --git a/crates/aft/src/bash_background/registry.rs b/crates/aft/src/bash_background/registry.rs index 75c2aa13b..f877fcdcd 100644 --- a/crates/aft/src/bash_background/registry.rs +++ b/crates/aft/src/bash_background/registry.rs @@ -12,12 +12,19 @@ use std::path::{Path, PathBuf}; use std::process::Command; #[cfg(windows)] use std::process::{Child, Stdio}; +#[cfg(test)] +use std::sync::atomic::AtomicUsize; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; + #[cfg(unix)] use std::sync::OnceLock; use std::sync::{Arc, Mutex, RwLock}; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; +#[cfg(all(test, unix))] +#[path = "root_reclaim_tests.rs"] +pub(crate) mod root_reclaim_tests; + use crate::db::TrackedConnection; use serde::Serialize; @@ -1302,6 +1309,10 @@ pub(crate) struct RegistryInner { retired_undelivered_completions: AtomicU64, /// Watchdog candidates only; terminal history stays in `tasks` for delivery. pub(crate) watchdog_tasks: Mutex>>, + #[cfg(test)] + reclaim_kill_attempts: AtomicUsize, + #[cfg(test)] + reclaim_kill_noop: AtomicBool, /// Finished tasks whose scratch directory is kept on disk until its /// delete time, keyed by task id. See `scratch_retention`. retained_scratch: Mutex>, @@ -1494,6 +1505,9 @@ pub(crate) struct BgTask { pub(crate) session_id: String, pub(crate) paths: TaskPaths, artifact_root: PathBuf, + /// Canonical root identity captured at registration and retained on deletion. + /// Reclaim probes compare it without filesystem calls on the frame loop. + project_root: PathBuf, pub(crate) started: Instant, /// Whether this task holds one of the project root's background slots /// (`max_background_bash_tasks`): true for a background or PTY launch and @@ -1914,6 +1928,10 @@ impl BgTaskRegistry { persisted_gc_storage: Mutex::new(BTreeSet::new()), retired_undelivered_completions: AtomicU64::new(0), watchdog_tasks: Mutex::new(HashMap::new()), + #[cfg(test)] + reclaim_kill_attempts: AtomicUsize::new(0), + #[cfg(test)] + reclaim_kill_noop: AtomicBool::new(false), retained_scratch: Mutex::new(HashMap::new()), revoked_scopes: Mutex::new(HashSet::new()), held_remote_workers: Mutex::new(None), @@ -4741,6 +4759,12 @@ impl BgTaskRegistry { session_id, paths: paths.clone(), artifact_root: canonical_artifact_root(&paths), + project_root: canonicalized_path( + metadata + .project_root + .as_deref() + .unwrap_or(&metadata.workdir), + ), started: Instant::now(), terminal_reader_done: Arc::new(AtomicBool::new(true)), holds_background_slot: AtomicBool::new(slot.holds_slot()), @@ -5132,6 +5156,12 @@ impl BgTaskRegistry { session_id, paths: paths.clone(), artifact_root: canonical_artifact_root(&paths), + project_root: canonicalized_path( + metadata + .project_root + .as_deref() + .unwrap_or(&metadata.workdir), + ), started: Instant::now(), terminal_reader_done, // A PTY always runs in the background. @@ -5435,6 +5465,12 @@ impl BgTaskRegistry { session_id, paths: paths.clone(), artifact_root: canonical_artifact_root(&paths), + project_root: canonicalized_path( + metadata + .project_root + .as_deref() + .unwrap_or(&metadata.workdir), + ), started: Instant::now(), terminal_reader_done: Arc::new(AtomicBool::new(true)), holds_background_slot: AtomicBool::new(slot.holds_slot()), @@ -8979,9 +9015,10 @@ impl BgTaskRegistry { self.kill_with_status_reason(task_id, session_id, BgTaskStatus::Killed, None) } - /// Terminate live tasks whose project root has been confirmed absent by - /// subc's consecutive directory-absence scans, so tasks cannot keep using - /// a root that has been verified for reclamation. + /// Terminate live tasks and terminal tasks' surviving process groups after + /// subc's consecutive directory-absence scans confirm their root is absent. + /// Root deletion is a definitive end, like scope termination: descendants + /// must not outlive the root just because their command already completed. /// /// The absence signal intentionally cannot distinguish deletion from /// renaming: a task's cwd handle can follow a moved directory even while @@ -8989,24 +9026,35 @@ impl BgTaskRegistry { /// registry identity, so killing those tasks is accepted; operational /// guidance is to rename a project only after its live tasks have ended. pub fn kill_running_tasks_for_root(&self, project_root: &Path) -> usize { + #[cfg(test)] + { + self.inner + .reclaim_kill_attempts + .fetch_add(1, Ordering::SeqCst); + if self.inner.reclaim_kill_noop.load(Ordering::SeqCst) { + return 0; + } + } let canonical_root = canonicalized_path(project_root); let tasks = self.task_arcs(); let targets: Vec<_> = tasks .into_iter() + .filter(|task| task.project_root == canonical_root) .filter_map(|task| { - let task_root = { + let sample_missing = { let state = task.lock_state().ok()?; - if !BgTask::state_is_running(&state) { - return None; - } - state - .metadata - .project_root - .clone() - .unwrap_or_else(|| state.metadata.workdir.clone()) + state.metadata.status.is_terminal() + && state.metadata.pgid.is_some() + && state.metadata.live_descendants.is_none() }; - (canonicalized_path(&task_root) == canonical_root) - .then(|| (task.task_id.clone(), task.session_id.clone())) + if sample_missing { + self.sample_task_process_group(&task); + } + let needs_reclaim = { + let state = task.lock_state().ok()?; + BgTask::state_needs_root_reclaim(&state) + }; + needs_reclaim.then(|| (task.task_id.clone(), task.session_id.clone())) }) .collect(); @@ -11286,6 +11334,30 @@ impl BgTaskRegistry { } } + /// Non-waiting counterpart of [`Self::kill_running_tasks_for_root`]. Terminal + /// records awaiting delivery or scratch cleanup do not keep artifacts live; + /// `metadata.live_descendants` and `live_descendants_omitted` still do. A + /// terminal group with no sample also waits for the kill worker to sample + /// it; terminal status can be visible before descendant sampling finishes. + /// The kill path refreshes those samples after signaling. A cached routing + /// identity avoids filesystem calls or process enumeration on the frame loop. + pub(crate) fn try_has_reclaimable_tasks_for_root( + &self, + project_root: &crate::path_identity::ProjectRootId, + ) -> Option { + let tasks = self.inner.tasks.try_lock().ok()?; + for task in tasks + .values() + .filter(|task| task.project_root == project_root.as_path()) + { + let state = task.state.try_lock().ok()?; + if BgTask::state_needs_root_reclaim(&state) { + return Some(true); + } + } + Some(false) + } + /// Hold the watch map lock for the duration of `while_held`, so a test can /// prove a caller does not wait for it. #[cfg(test)] @@ -11356,6 +11428,12 @@ impl BgTaskRegistry { session_id, paths: paths.clone(), artifact_root: canonical_artifact_root(&paths), + project_root: canonicalized_path( + metadata + .project_root + .as_deref() + .unwrap_or(&metadata.workdir), + ), started, terminal_reader_done: Arc::new(AtomicBool::new(true)), // A task reloaded after a restart has no caller waiting on it any @@ -15271,6 +15349,18 @@ impl BgTask { } } + fn state_needs_root_reclaim(state: &BgTaskState) -> bool { + !state.metadata.status.is_terminal() + || (cfg!(unix) + && state.metadata.pgid.is_some() + && (state + .metadata + .live_descendants + .as_ref() + .is_none_or(|members| !members.is_empty()) + || state.metadata.live_descendants_omitted > 0)) + } + fn state_is_running(state: &BgTaskState) -> bool { state.metadata.status == BgTaskStatus::Running || (state.metadata.mode == BgMode::Pty diff --git a/crates/aft/src/bash_background/root_reclaim_tests.rs b/crates/aft/src/bash_background/root_reclaim_tests.rs new file mode 100644 index 000000000..dfa48daef --- /dev/null +++ b/crates/aft/src/bash_background/root_reclaim_tests.rs @@ -0,0 +1,167 @@ +use super::*; + +pub(crate) fn register_task( + registry: &BgTaskRegistry, + storage: &Path, + root: &Path, + task_id: &str, + status: BgTaskStatus, + pid: Option, +) { + let layout = + super::super::persistence::create_task_layout(storage, "reclaim", task_id).unwrap(); + let mut metadata = PersistedTask::starting( + task_id.into(), + "reclaim".into(), + "owned reclaim fixture".into(), + root.to_path_buf(), + Some(root.to_path_buf()), + None, + true, + false, + ); + metadata.status = status; + metadata.child_pid = pid; + metadata.pgid = pid.map(|pid| pid as i32); + metadata.live_descendants = pid.map(|pid| { + vec![super::super::process::LiveDescendant { + pid, + comm: "sleep".into(), + argv0: "sleep".into(), + }] + }); + super::super::persistence::write_task(&layout.paths.json, &metadata).unwrap(); + registry + .insert_rehydrated_task(metadata, layout.paths, true) + .unwrap(); + // Keep terminal fixtures in the watchdog index without starting a watchdog. + let task = registry.task_for_session(task_id, "reclaim").unwrap(); + registry + .inner + .watchdog_tasks + .lock() + .unwrap() + .insert(task_id.into(), task); +} + +pub(crate) fn refuse_reclaim_kills(registry: &BgTaskRegistry) { + registry + .inner + .reclaim_kill_noop + .store(true, Ordering::SeqCst); +} + +pub(crate) fn kill_attempts(registry: &BgTaskRegistry) -> usize { + registry.inner.reclaim_kill_attempts.load(Ordering::SeqCst) +} + +pub(crate) struct OwnedGroup(pub(crate) std::process::Child); + +impl OwnedGroup { + pub(crate) fn spawn() -> Self { + use std::os::unix::process::CommandExt; + Self( + std::process::Command::new("sleep") + .arg("60") + .process_group(0) + .spawn() + .unwrap(), + ) + } +} + +impl Drop for OwnedGroup { + fn drop(&mut self) { + let _ = self.0.kill(); + let _ = self.0.wait(); + } +} + +#[test] +fn root_reclaim_probe_ignores_foreign_tasks_and_terminal_history() { + let registry = BgTaskRegistry::default(); + let storage = tempfile::tempdir().unwrap(); + let root = tempfile::tempdir().unwrap(); + let root_id = crate::path_identity::ProjectRootId::from_path(root.path()).unwrap(); + let foreign = tempfile::tempdir().unwrap(); + register_task( + ®istry, + storage.path(), + root.path(), + "bash-0000000000000010", + BgTaskStatus::Completed, + None, + ); + register_task( + ®istry, + storage.path(), + foreign.path(), + "bash-0000000000000011", + BgTaskStatus::Running, + None, + ); + let foreign_task = registry + .task_for_session("bash-0000000000000011", "reclaim") + .unwrap(); + let _held = foreign_task.state.lock().unwrap(); + ROOT_LOCK_PROBE.with(|probe| { + *probe.borrow_mut() = Some(Box::new(|| panic!("reclaim probe touched filesystem"))) + }); + assert_eq!( + registry.try_has_reclaimable_tasks_for_root(&root_id), + Some(false) + ); + ROOT_LOCK_PROBE.with(|probe| *probe.borrow_mut() = None); +} + +#[test] +fn root_reclaim_probe_defers_on_own_task_or_registry_contention() { + let registry = BgTaskRegistry::default(); + let storage = tempfile::tempdir().unwrap(); + let root = tempfile::tempdir().unwrap(); + let root_id = crate::path_identity::ProjectRootId::from_path(root.path()).unwrap(); + let id = "bash-0000000000000012"; + register_task( + ®istry, + storage.path(), + root.path(), + id, + BgTaskStatus::Running, + None, + ); + assert_eq!( + registry.try_has_reclaimable_tasks_for_root(&root_id), + Some(true) + ); + let task = registry.task_for_session(id, "reclaim").unwrap(); + let held = task.state.lock().unwrap(); + assert_eq!(registry.try_has_reclaimable_tasks_for_root(&root_id), None); + drop(held); + let _held = registry.inner.tasks.lock().unwrap(); + assert_eq!(registry.try_has_reclaimable_tasks_for_root(&root_id), None); +} + +pub(crate) fn finish_task(registry: &BgTaskRegistry, id: &str) { + registry + .task_for_session(id, "reclaim") + .unwrap() + .lock_state() + .unwrap() + .metadata + .status = BgTaskStatus::Killed; +} + +pub(crate) fn clear_descendant_sample(registry: &BgTaskRegistry, id: &str) { + registry + .task_for_session(id, "reclaim") + .unwrap() + .lock_state() + .unwrap() + .metadata + .live_descendants = None; +} + +pub(crate) fn while_task_map_locked(registry: &BgTaskRegistry, call: impl FnOnce() -> T) -> T { + let _held = registry.inner.tasks.lock().unwrap(); + call() +} diff --git a/crates/aft/src/context.rs b/crates/aft/src/context.rs index 48e3b1436..9ce70c8f9 100644 --- a/crates/aft/src/context.rs +++ b/crates/aft/src/context.rs @@ -13931,6 +13931,22 @@ impl AppContext { pub(crate) fn try_artifact_eviction_blocker( &self, include_pending: bool, + ) -> Option { + self.try_artifact_eviction_blocker_for_root(include_pending, None) + } + + pub(crate) fn try_artifact_eviction_blocker_for_deleted_root( + &self, + include_pending: bool, + root: &crate::path_identity::ProjectRootId, + ) -> Option { + self.try_artifact_eviction_blocker_for_root(include_pending, Some(root)) + } + + fn try_artifact_eviction_blocker_for_root( + &self, + include_pending: bool, + deleted_root: Option<&crate::path_identity::ProjectRootId>, ) -> Option { use ArtifactEvictionBlocker as Blocker; if self.standing_artifact_exempt.load(Ordering::Acquire) { @@ -13957,7 +13973,13 @@ impl AppContext { Some(true) => return Some(Blocker::InspectTier2), Some(false) => {} } - match self.bash_background.try_has_running_tasks() { + let bash_busy = match deleted_root { + Some(root) => self + .bash_background + .try_has_reclaimable_tasks_for_root(root), + None => self.bash_background.try_has_running_tasks(), + }; + match bash_busy { None => return Some(Blocker::LockContended), Some(true) => return Some(Blocker::BashRunning), Some(false) => {} @@ -14056,7 +14078,27 @@ impl AppContext { /// when eviction is unsafe or a lock was contended; the reaper retries on /// its next sweep. pub(crate) fn try_evict_idle_artifacts(&self) -> bool { - if self.try_artifact_eviction_blocker(true).is_some() { + self.try_evict_idle_artifacts_for_root(None) + } + + /// Deleted-root reclamation uses the same root-scoped task gate both before + /// and during eviction. Artifact handles belong to this root, so only its + /// tasks block reclamation of those handles. + pub(crate) fn try_evict_idle_artifacts_for_deleted_root( + &self, + root: &crate::path_identity::ProjectRootId, + ) -> bool { + self.try_evict_idle_artifacts_for_root(Some(root)) + } + + fn try_evict_idle_artifacts_for_root( + &self, + deleted_root: Option<&crate::path_identity::ProjectRootId>, + ) -> bool { + if self + .try_artifact_eviction_blocker_for_root(true, deleted_root) + .is_some() + { return false; } let Some(mut callgraph_store) = try_write_unpoisoned(&self.callgraph_store) else { diff --git a/crates/aft/src/subc/deleted_root_reclaim_tests.rs b/crates/aft/src/subc/deleted_root_reclaim_tests.rs new file mode 100644 index 000000000..d1462278b --- /dev/null +++ b/crates/aft/src/subc/deleted_root_reclaim_tests.rs @@ -0,0 +1,239 @@ +use super::*; +use crate::bash_background::{registry::root_reclaim_tests::*, BgTaskStatus}; + +struct ReclaimFixture { + _dir: tempfile::TempDir, + storage: tempfile::TempDir, + root: ProjectRootId, + ctx: Arc, + executor: Arc, + roots: HashMap, + metrics: DispatchPathMetrics, + now: Instant, +} + +impl ReclaimFixture { + fn new() -> Self { + let (dir, root) = test_support::test_root("deleted-root-reclaim"); + let ctx = test_support::test_ctx(); + let executor = Arc::new(Executor::new()); + assert!(executor.register_actor(root.clone(), Arc::clone(&ctx))); + let now = Instant::now(); + let mut meta = RootMeta::new(now); + // Two independent probes have already confirmed absence; each test + // exercises the task cleanup sweeps that follow that confirmation. + meta.consecutive_missing_sweeps = 2; + let roots = HashMap::from([(root.clone(), meta)]); + std::fs::remove_dir_all(root.as_path()).unwrap(); + let deadline = now + Duration::from_secs(5); + while executor.dispatch_loop_liveness().has_pending_work() { + assert!(Instant::now() < deadline, "registration did not settle"); + std::thread::yield_now(); + } + Self { + _dir: dir, + storage: tempfile::tempdir().unwrap(), + root, + ctx, + executor, + roots, + metrics: DispatchPathMetrics::new(), + now, + } + } + + fn task(&self, id: &str, root: &Path, status: BgTaskStatus, pid: Option) { + register_task( + self.ctx.bash_background(), + self.storage.path(), + root, + id, + status, + pid, + ); + } + + fn sweep(&mut self) -> IdleReapOutcome { + reap_idle_roots_with_presence( + self.now, + &mut self.roots, + &HashMap::new(), + &HashMap::new(), + &self.executor, + &self.metrics, + &RootPresence::default(), + ) + } + + fn wait_for_kill(&self) { + let deadline = Instant::now() + Duration::from_secs(10); + while self.roots[&self.root] + .reclaim_kill_in_flight + .as_ref() + .is_some_and(|flag| flag.load(Ordering::Acquire)) + { + assert!(Instant::now() < deadline, "reclaim kill did not complete"); + std::thread::sleep(Duration::from_millis(1)); + } + } + + fn assert_reclaimed(&self, outcome: IdleReapOutcome) { + assert_eq!( + outcome.forgotten_deleted_roots, + vec![self.root.clone()], + "root retained: {}", + self.metrics.reap_census_for_test() + ); + assert!(!self.roots.contains_key(&self.root)); + assert!(self.executor.actor_context(&self.root).is_none()); + } +} + +#[test] +fn deleted_root_terminal_survivor_is_killed_and_reclaimed_in_two_sweeps() { + assert_terminal_survivor_reclaimed(false); +} + +#[test] +fn deleted_root_terminal_group_with_no_sample_is_checked_before_reclaim() { + assert_terminal_survivor_reclaimed(true); +} + +fn assert_terminal_survivor_reclaimed(clear_sample: bool) { + let mut fixture = ReclaimFixture::new(); + let mut group = OwnedGroup::spawn(); + fixture.task( + "bash-0000000000000001", + fixture.root.as_path(), + BgTaskStatus::Completed, + Some(group.0.id()), + ); + if clear_sample { + clear_descendant_sample(fixture.ctx.bash_background(), "bash-0000000000000001"); + } + assert_eq!(fixture.sweep().evicted, 0); + fixture.wait_for_kill(); + assert!( + group.0.try_wait().unwrap().is_some(), + "terminal task's owned group survived reclaim" + ); + let outcome = fixture.sweep(); + fixture.assert_reclaimed(outcome); +} + +#[test] +fn deleted_root_is_not_blocked_by_another_roots_running_task() { + let mut fixture = ReclaimFixture::new(); + let other = tempfile::tempdir().unwrap(); + let mut group = OwnedGroup::spawn(); + fixture.task( + "bash-0000000000000002", + other.path(), + BgTaskStatus::Running, + Some(group.0.id()), + ); + // Actor retirement detaches its registry. Keep the task itself so the + // assertion observes its state rather than racing the registry teardown. + let foreign = fixture + .ctx + .bash_background() + .task_for_test("bash-0000000000000002") + .unwrap(); + let outcome = fixture.sweep(); + fixture.assert_reclaimed(outcome); + assert!(foreign.is_running()); + assert!( + group.0.try_wait().unwrap().is_none(), + "foreign task must not be signaled" + ); + assert_eq!(kill_attempts(fixture.ctx.bash_background()), 0); +} + +#[test] +fn deleted_root_unkillable_task_backs_off_counted_kill_threads() { + let mut fixture = ReclaimFixture::new(); + fixture.task( + "bash-0000000000000003", + fixture.root.as_path(), + BgTaskStatus::Running, + None, + ); + refuse_reclaim_kills(fixture.ctx.bash_background()); + for _ in 0..100 { + assert_eq!(fixture.sweep().evicted, 0); + fixture.wait_for_kill(); + fixture.now += Duration::from_millis(100); + } + assert_eq!( + kill_attempts(fixture.ctx.bash_background()), + 1, + "100 sweeps over 10 seconds must spawn only the initial kill thread" + ); + assert_eq!( + fixture.metrics.reap_census_for_test()["deleted_task_blockers"]["task_kill_backoff"], + 1 + ); + fixture.now += RECLAIM_KILL_RETRY; + assert_eq!(fixture.sweep().evicted, 0); + fixture.wait_for_kill(); + assert_eq!( + kill_attempts(fixture.ctx.bash_background()), + 2, + "retry once the backoff expires" + ); + finish_task(fixture.ctx.bash_background(), "bash-0000000000000003"); + let outcome = fixture.sweep(); + fixture.assert_reclaimed(outcome); +} + +#[test] +fn deleted_root_running_task_is_killed_before_reclaim() { + let mut fixture = ReclaimFixture::new(); + let mut group = OwnedGroup::spawn(); + fixture.task( + "bash-0000000000000004", + fixture.root.as_path(), + BgTaskStatus::Running, + Some(group.0.id()), + ); + assert_eq!(fixture.sweep().evicted, 0); + assert_eq!( + fixture.metrics.reap_census_for_test()["deleted_task_blockers"]["task_kill_in_flight"], + 1 + ); + assert!(fixture.roots.contains_key(&fixture.root)); + fixture.wait_for_kill(); + assert!( + group.0.try_wait().unwrap().is_some(), + "running task survived reclaim" + ); + assert_eq!( + fixture + .ctx + .bash_background() + .observed_status("bash-0000000000000004", "reclaim", 0) + .unwrap() + .info + .status, + BgTaskStatus::Killed + ); + let outcome = fixture.sweep(); + fixture.assert_reclaimed(outcome); +} + +#[test] +fn deleted_root_task_registry_contention_is_censused_without_kill_threads() { + let mut fixture = ReclaimFixture::new(); + let ctx = Arc::clone(&fixture.ctx); + assert_eq!( + while_task_map_locked(ctx.bash_background(), || fixture.sweep()).evicted, + 0 + ); + assert_eq!( + fixture.metrics.reap_census_for_test()["deleted_task_blockers"]["task_registry_busy"], + 1 + ); + assert_eq!(kill_attempts(ctx.bash_background()), 0); + let outcome = fixture.sweep(); + fixture.assert_reclaimed(outcome); +} diff --git a/crates/aft/src/subc/health.rs b/crates/aft/src/subc/health.rs index 3f49ba4b6..d121fc355 100644 --- a/crates/aft/src/subc/health.rs +++ b/crates/aft/src/subc/health.rs @@ -75,6 +75,19 @@ impl UnboundRetention { } } +#[derive(Clone, Copy, Debug)] +pub(super) enum DeletedTaskRetention { + KillInFlight, + KillBackoff, + RegistryBusy, +} + +const DELETED_TASK_RETENTION_LABELS: [&str; 3] = [ + "task_kill_in_flight", + "task_kill_backoff", + "task_registry_busy", +]; + #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub(super) struct ReapBlockerCensus { pub(super) deleted_retained: usize, @@ -89,6 +102,7 @@ pub(super) struct ReapBlockerCensus { pub(super) actor_state_busy: usize, pub(super) artifact_eviction_blocked: usize, pub(super) artifact_eviction_failed: usize, + deleted_task_blockers: [usize; 3], /// Existing roots with no bound route that are past their idle TTL but /// were not evicted, counted once each under the reason in /// `unbound_blockers`. @@ -97,6 +111,12 @@ pub(super) struct ReapBlockerCensus { } impl ReapBlockerCensus { + pub(super) fn note_deleted_task_retained(&mut self, reason: DeletedTaskRetention) { + self.deleted_retained += 1; + self.artifact_eviction_blocked += 1; + self.deleted_task_blockers[reason as usize] += 1; + } + pub(super) fn note_unbound_retained(&mut self, reason: UnboundRetention) { self.unbound_retained += 1; self.unbound_blockers[reason.index()] += 1; @@ -112,7 +132,7 @@ impl ReapBlockerCensus { } pub(super) fn blocker_histogram(&self) -> String { format!( - "absence_unconfirmed={},bound_routes={},unbound_quiesced={},bash_waits={},maintenance_pending={},maintenance_queued={},pending_binds={},actor_busy={},actor_state_busy={},artifact_eviction_blocked={},artifact_eviction_failed={}", + "absence_unconfirmed={},bound_routes={},unbound_quiesced={},bash_waits={},maintenance_pending={},maintenance_queued={},pending_binds={},actor_busy={},actor_state_busy={},artifact_eviction_blocked={},artifact_eviction_failed={},task_kill_in_flight={},task_kill_backoff={},task_registry_busy={}", self.absence_unconfirmed, self.bound_routes, self.unbound_quiesced, @@ -124,6 +144,9 @@ impl ReapBlockerCensus { self.actor_state_busy, self.artifact_eviction_blocked, self.artifact_eviction_failed, + self.deleted_task_blockers[0], + self.deleted_task_blockers[1], + self.deleted_task_blockers[2], ) } } @@ -147,6 +170,7 @@ struct ReapMetrics { actor_state_busy: AtomicUsize, artifact_eviction_blocked: AtomicUsize, artifact_eviction_failed: AtomicUsize, + deleted_task_blockers: [AtomicUsize; 3], unbound_retained: AtomicUsize, unbound_blockers: [AtomicUsize; UNBOUND_RETENTION_KINDS], } @@ -168,6 +192,7 @@ impl ReapMetrics { actor_state_busy: AtomicUsize::new(0), artifact_eviction_blocked: AtomicUsize::new(0), artifact_eviction_failed: AtomicUsize::new(0), + deleted_task_blockers: std::array::from_fn(|_| AtomicUsize::new(0)), unbound_retained: AtomicUsize::new(0), unbound_blockers: std::array::from_fn(|_| AtomicUsize::new(0)), } @@ -215,6 +240,13 @@ impl ReapMetrics { .store(census.artifact_eviction_blocked, Ordering::Relaxed); self.artifact_eviction_failed .store(census.artifact_eviction_failed, Ordering::Relaxed); + for (slot, count) in self + .deleted_task_blockers + .iter() + .zip(census.deleted_task_blockers) + { + slot.store(count, Ordering::Relaxed); + } self.unbound_retained .store(census.unbound_retained, Ordering::Relaxed); for (slot, count) in self @@ -228,6 +260,11 @@ impl ReapMetrics { } fn snapshot(&self) -> Value { + let deleted_task_blockers = DELETED_TASK_RETENTION_LABELS + .iter() + .zip(self.deleted_task_blockers.iter()) + .map(|(label, count)| ((*label).to_string(), json!(count.load(Ordering::Relaxed)))) + .collect::>(); let unbound_blockers = UNBOUND_RETENTION_LABELS .iter() .zip(self.unbound_blockers.iter()) @@ -235,6 +272,7 @@ impl ReapMetrics { .collect::>(); json!({ "deleted_retained": self.deleted_retained.load(Ordering::Relaxed), + "deleted_task_blockers": deleted_task_blockers, "blockers": { "absence_unconfirmed": self.absence_unconfirmed.load(Ordering::Relaxed), "bound_routes": self.bound_routes.load(Ordering::Relaxed), diff --git a/crates/aft/src/subc/mod.rs b/crates/aft/src/subc/mod.rs index 447f8f8ac..4a3effab3 100644 --- a/crates/aft/src/subc/mod.rs +++ b/crates/aft/src/subc/mod.rs @@ -212,6 +212,8 @@ mod bash_selector_tests; mod codemode; mod consent; pub(crate) use consent::directory_scope; +#[cfg(all(test, unix))] +mod deleted_root_reclaim_tests; mod drain; mod flow_read; mod health; @@ -1413,6 +1415,10 @@ struct RootMeta { /// tasks. The kill waits out signal grace periods, so it never runs on /// the frame loop; the reaper retains the root until it finishes. reclaim_kill_in_flight: Option>, + /// Minimum spacing between kill submissions, including failed spawns and + /// kills that leave surviving processes. Sweeps must not create a thread + /// on every tick while a process cannot be terminated. + reclaim_kill_submitted_at: Option, /// When the idle reaper last submitted the unbound-teardown persist for /// this root. Sweeps run every few hundred milliseconds, so a persist that /// could not clear its blocker (cache lock contention, a failed write) is @@ -1650,6 +1656,7 @@ impl RootMeta { consecutive_missing_sweeps: 0, presence_generation_seen: 0, reclaim_kill_in_flight: None, + reclaim_kill_submitted_at: None, teardown_persist_submitted_at: None, ttl_retention_logged: None, configure_tail_in_flight: false, @@ -2134,6 +2141,8 @@ struct IdleReapOutcome { /// Minimum gap between unbound-teardown persist attempts for one root. const UNBOUND_TEARDOWN_PERSIST_RETRY: Duration = Duration::from_secs(60); +const RECLAIM_KILL_RETRY: Duration = Duration::from_secs(30); + /// Count an unbound root retained past its idle TTL and log the reason once /// per distinct blocker. fn note_unbound_ttl_retention( @@ -2505,14 +2514,32 @@ fn reap_idle_roots_with_presence( meta.reclaim_kill_in_flight .get_or_insert_with(|| Arc::new(AtomicBool::new(false))), ); - let tasks_settled = !kill_in_flight.load(Ordering::Acquire) - && ctx.bash_background().try_has_running_tasks() == Some(false); - if !tasks_settled { - if !kill_in_flight.swap(true, Ordering::AcqRel) { - spawn_reclaimed_root_task_kill(&ctx, &root_id, kill_in_flight); + let retention = if kill_in_flight.load(Ordering::Acquire) { + Some(health::DeletedTaskRetention::KillInFlight) + } else { + match ctx + .bash_background() + .try_has_reclaimable_tasks_for_root(&root_id) + { + Some(false) => None, + None => Some(health::DeletedTaskRetention::RegistryBusy), + Some(true) => { + let retry_due = + meta.reclaim_kill_submitted_at.is_none_or(|submitted| { + now.saturating_duration_since(submitted) >= RECLAIM_KILL_RETRY + }); + if retry_due && !kill_in_flight.swap(true, Ordering::AcqRel) { + meta.reclaim_kill_submitted_at = Some(now); + spawn_reclaimed_root_task_kill(&ctx, &root_id, kill_in_flight); + Some(health::DeletedTaskRetention::KillInFlight) + } else { + Some(health::DeletedTaskRetention::KillBackoff) + } + } } - census.deleted_retained += 1; - census.artifact_eviction_blocked += 1; + }; + if let Some(retention) = retention { + census.note_deleted_task_retained(retention); continue; } } @@ -2526,7 +2553,12 @@ fn reap_idle_roots_with_presence( continue; }; let taken_pending = Some(taken_pending); - if let Some(blocker) = ctx.try_artifact_eviction_blocker(false) { + let blocker = if deleted { + ctx.try_artifact_eviction_blocker_for_deleted_root(false, &root_id) + } else { + ctx.try_artifact_eviction_blocker(false) + }; + if let Some(blocker) = blocker { if let Some(pending) = taken_pending { ctx.restore_pending_reconciliation_state_without_waiting(pending); } @@ -2557,7 +2589,12 @@ fn reap_idle_roots_with_presence( continue; } let memory_before = ctx.memory_root_snapshot(); - if !ctx.try_evict_idle_artifacts() { + let evicted = if deleted { + ctx.try_evict_idle_artifacts_for_deleted_root(&root_id) + } else { + ctx.try_evict_idle_artifacts() + }; + if !evicted { if let Some(pending) = taken_pending { ctx.restore_pending_reconciliation_state_without_waiting(pending); } From e3003baccf69f95a6eff6aa102b8188428fc4314 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Mon, 26 Oct 2026 09:51:40 +0100 Subject: [PATCH 2/8] mason: resolve task root identity before registry admission Hoist root canonicalization ahead of local, PTY and remote admission and saved-remote claims. Pass the resolved identity into task assembly and failed-spawn rehydration instead of repeating filesystem I/O while those locks are held. The reported lifecycle guard passes on origin/main and fails on d63 only because it expects two lookups: caching removed the per-task lookup, leaving one input lookup. Preserve both lock assertions and require exactly that one lookup. Add guards for pre-admission resolution and precomputed registration under admission/claim locks, including a removed symlink alias. Registry (532 passed, 5 ignored), deleted_root (26 passed), root_reclaim (15 passed), formatting and Windows compile checks pass. Verification used linux,1c after the parent approved stepping down from 4c/2c admission refusals. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/bash_background/registry.rs | 112 ++++++++----- crates/aft/src/bash_background/remote.rs | 2 + .../src/bash_background/root_reclaim_tests.rs | 147 ++++++++++++++++++ 3 files changed, 224 insertions(+), 37 deletions(-) diff --git a/crates/aft/src/bash_background/registry.rs b/crates/aft/src/bash_background/registry.rs index f877fcdcd..3a8b9de5e 100644 --- a/crates/aft/src/bash_background/registry.rs +++ b/crates/aft/src/bash_background/registry.rs @@ -4260,10 +4260,21 @@ impl BgTaskRegistry { super::memory_limit::Monitor::prepare(mib, task_id).map(Some) } - fn fail_memory_spawn(&self, metadata: &mut PersistedTask, paths: &TaskPaths, error: &str) { + fn fail_memory_spawn( + &self, + metadata: &mut PersistedTask, + paths: &TaskPaths, + error: &str, + canonical_project_root: PathBuf, + ) { metadata.mark_terminal(BgTaskStatus::Failed, None, Some(error.into())); let _ = self.persist_task(paths, metadata); - let _ = self.insert_rehydrated_task(metadata.clone(), paths.clone(), true); + let _ = self.insert_rehydrated_task_with_root( + metadata.clone(), + paths.clone(), + true, + canonical_project_root, + ); self.record_live_delivery_session(&metadata.session_id); self.enqueue_completion_if_needed(metadata, Some(paths), true); } @@ -4547,6 +4558,8 @@ impl BgTaskRegistry { compressed: bool, project_root: Option, ) -> Result { + let canonical_project_root = + canonicalized_path(project_root.as_deref().unwrap_or(&workdir)); self.start_watchdog(); #[cfg(target_os = "linux")] @@ -4702,7 +4715,12 @@ impl BgTaskRegistry { metadata.notify_on_completion = true; metadata.mark_terminal(BgTaskStatus::Failed, None, Some(error.clone())); let _ = self.persist_task(&paths, &metadata); - let _ = self.insert_rehydrated_task(metadata.clone(), paths.clone(), true); + let _ = self.insert_rehydrated_task_with_root( + metadata.clone(), + paths.clone(), + true, + canonical_project_root, + ); self.record_live_delivery_session(&metadata.session_id); self.enqueue_completion_if_needed(&metadata, Some(&paths), true); } else { @@ -4717,7 +4735,7 @@ impl BgTaskRegistry { if let Err(error) = monitor.attach(child_pid) { super::process::terminate_pgid(child_pid as i32, None); let _ = child.wait(); - self.fail_memory_spawn(&mut metadata, &paths, &error); + self.fail_memory_spawn(&mut metadata, &paths, &error, canonical_project_root); return Err(error.into()); } metadata.memory_policy = Some(monitor.policy.clone()); @@ -4742,7 +4760,12 @@ impl BgTaskRegistry { Some("spawn aborted".to_string()), ); let _ = self.persist_task(&paths, &metadata); - let _ = self.insert_rehydrated_task(metadata, paths.clone(), true); + let _ = self.insert_rehydrated_task_with_root( + metadata, + paths.clone(), + true, + canonical_project_root, + ); return Err(format!("failed to persist background task metadata: {error}").into()); } metadata.mark_running(child_pid, child_pid as i32); @@ -4759,12 +4782,7 @@ impl BgTaskRegistry { session_id, paths: paths.clone(), artifact_root: canonical_artifact_root(&paths), - project_root: canonicalized_path( - metadata - .project_root - .as_deref() - .unwrap_or(&metadata.workdir), - ), + project_root: canonical_project_root, started: Instant::now(), terminal_reader_done: Arc::new(AtomicBool::new(true)), holds_background_slot: AtomicBool::new(slot.holds_slot()), @@ -4983,6 +5001,8 @@ impl BgTaskRegistry { rows: u16, cols: u16, ) -> Result { + let canonical_project_root = + canonicalized_path(project_root.as_deref().unwrap_or(&workdir)); self.start_watchdog(); let _admission = @@ -5115,7 +5135,12 @@ impl BgTaskRegistry { metadata.pgid = spawned_pgid; metadata.mark_terminal(BgTaskStatus::Failed, None, Some(error.clone())); let _ = self.persist_task(&paths, &metadata); - let _ = self.insert_rehydrated_task(metadata.clone(), paths.clone(), true); + let _ = self.insert_rehydrated_task_with_root( + metadata.clone(), + paths.clone(), + true, + canonical_project_root, + ); self.record_live_delivery_session(&metadata.session_id); self.enqueue_completion_if_needed(&metadata, Some(&paths), true); } else { @@ -5156,12 +5181,7 @@ impl BgTaskRegistry { session_id, paths: paths.clone(), artifact_root: canonical_artifact_root(&paths), - project_root: canonicalized_path( - metadata - .project_root - .as_deref() - .unwrap_or(&metadata.workdir), - ), + project_root: canonical_project_root, started: Instant::now(), terminal_reader_done, // A PTY always runs in the background. @@ -5336,6 +5356,8 @@ impl BgTaskRegistry { compressed: bool, project_root: Option, ) -> Result { + let canonical_project_root = + canonicalized_path(project_root.as_deref().unwrap_or(&workdir)); self.start_watchdog(); let _admission = self.background_admission_guard(slot); @@ -5421,7 +5443,7 @@ impl BgTaskRegistry { if let Err(error) = monitor.attach(child_pid) { let _ = child.kill(); let _ = child.wait(); - self.fail_memory_spawn(&mut metadata, &paths, &error); + self.fail_memory_spawn(&mut metadata, &paths, &error, canonical_project_root); return Err(error.into()); } use std::io::Write; @@ -5436,7 +5458,7 @@ impl BgTaskRegistry { monitor.terminate(); let _ = child.kill(); let _ = child.wait(); - self.fail_memory_spawn(&mut metadata, &paths, &error); + self.fail_memory_spawn(&mut metadata, &paths, &error, canonical_project_root); return Err(error.into()); } metadata.memory_policy = Some(monitor.policy.clone()); @@ -5465,12 +5487,7 @@ impl BgTaskRegistry { session_id, paths: paths.clone(), artifact_root: canonical_artifact_root(&paths), - project_root: canonicalized_path( - metadata - .project_root - .as_deref() - .unwrap_or(&metadata.workdir), - ), + project_root: canonical_project_root, started: Instant::now(), terminal_reader_done: Arc::new(AtomicBool::new(true)), holds_background_slot: AtomicBool::new(slot.holds_slot()), @@ -10982,6 +10999,12 @@ impl BgTaskRegistry { paths: TaskPaths, ) -> Result<(), String> { let task_id = metadata.task_id.clone(); + let canonical_project_root = canonicalized_path( + metadata + .project_root + .as_deref() + .unwrap_or(&metadata.workdir), + ); // Held while the task is built and its worker started, so another // registry can neither build a second copy nor start a worker first. // Released before this registry publishes the task's pending @@ -11012,7 +11035,7 @@ impl BgTaskRegistry { return Ok(()); } } - self.insert_rehydrated_task(metadata, paths, true)?; + self.insert_rehydrated_task_with_root(metadata, paths, true, canonical_project_root)?; let task = self .task(&task_id) .ok_or_else(|| format!("{task_id}: restored remote task missing"))?; @@ -11378,7 +11401,25 @@ impl BgTaskRegistry { paths: TaskPaths, detached: bool, ) -> Result<(), String> { - self.insert_registered_task(metadata, paths, detached, None) + let canonical_project_root = canonicalized_path( + metadata + .project_root + .as_deref() + .unwrap_or(&metadata.workdir), + ); + self.insert_rehydrated_task_with_root(metadata, paths, detached, canonical_project_root) + } + + /// Admission and remote-claim holders pass the root resolved before locking. + /// Rehydrating a failed spawn must not repeat root filesystem I/O under them. + fn insert_rehydrated_task_with_root( + &self, + metadata: PersistedTask, + paths: TaskPaths, + detached: bool, + canonical_project_root: PathBuf, + ) -> Result<(), String> { + self.insert_registered_task(metadata, paths, detached, None, canonical_project_root) } fn insert_registered_task( @@ -11387,6 +11428,7 @@ impl BgTaskRegistry { paths: TaskPaths, detached: bool, changed_files: Option>, + canonical_project_root: PathBuf, ) -> Result<(), String> { let task_id = metadata.task_id.clone(); let session_id = metadata.session_id.clone(); @@ -11428,12 +11470,7 @@ impl BgTaskRegistry { session_id, paths: paths.clone(), artifact_root: canonical_artifact_root(&paths), - project_root: canonicalized_path( - metadata - .project_root - .as_deref() - .unwrap_or(&metadata.workdir), - ), + project_root: canonical_project_root, started, terminal_reader_done: Arc::new(AtomicBool::new(true)), // A task reloaded after a restart has no caller waiting on it any @@ -17178,9 +17215,10 @@ mod tests { }); registry.kill_running_tasks_for_root(dir.path()); ROOT_LOCK_PROBE.with(|hook| hook.borrow_mut().take()); - assert!( - calls.load(Ordering::SeqCst) >= 2, - "probe never inspected a task's root lookup" + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "root reclaim resolves only its input; task root identities are cached at registration" ); } diff --git a/crates/aft/src/bash_background/remote.rs b/crates/aft/src/bash_background/remote.rs index 2a18071a8..79dc2a025 100644 --- a/crates/aft/src/bash_background/remote.rs +++ b/crates/aft/src/bash_background/remote.rs @@ -2086,6 +2086,7 @@ impl BgTaskRegistry { compressed: bool, root: Option, ) -> Result { + let canonical_project_root = canonicalized_path(root.as_deref().unwrap_or(&workdir)); let _admission = self.background_admission_guard(slot); self.check_background_slot_typed(slot, &session_id)?; let layout = match plan.prepared_task() { @@ -2194,6 +2195,7 @@ impl BgTaskRegistry { layout.paths, false, crate::bash_changed_files::current(), + canonical_project_root, )?; let task = self.task(&task_id).unwrap(); task.holds_background_slot diff --git a/crates/aft/src/bash_background/root_reclaim_tests.rs b/crates/aft/src/bash_background/root_reclaim_tests.rs index dfa48daef..efc46b458 100644 --- a/crates/aft/src/bash_background/root_reclaim_tests.rs +++ b/crates/aft/src/bash_background/root_reclaim_tests.rs @@ -165,3 +165,150 @@ pub(crate) fn while_task_map_locked(registry: &BgTaskRegistry, call: impl FnO let _held = registry.inner.tasks.lock().unwrap(); call() } + +#[test] +fn root_reclaim_identity_is_resolved_before_spawn_admission() { + for kind in ["pipes", "pty", "remote"] { + let registry = BgTaskRegistry::default(); + registry + .inner + .watchdog_started + .store(true, Ordering::SeqCst); + let root = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + let calls = Arc::new(AtomicUsize::new(0)); + let probe_calls = Arc::clone(&calls); + let probe_registry = registry.clone(); + ROOT_LOCK_PROBE.with(|hook| { + *hook.borrow_mut() = Some(Box::new(move || { + probe_calls.fetch_add(1, Ordering::SeqCst); + assert!( + probe_registry.inner.background_admission.try_lock().is_ok(), + "root identity I/O held admission lock" + ); + assert!( + probe_registry.inner.tasks.try_lock().is_ok(), + "root identity I/O held registry lock" + ); + })) + }); + // A zero slot limit stops before allocating or spawning a process, but + // still takes admission. Root resolution must happen before that lock. + let result = match kind { + "pipes" => registry.spawn( + SpawnPlan::Unsandboxed, + "true", + "reclaim".into(), + root.path().into(), + HashMap::new(), + super::super::HardKill::After(Duration::from_secs(30)), + None, + storage.path().into(), + 0, + false, + false, + Some(root.path().into()), + ), + "pty" => registry.spawn_pty( + SpawnPlan::Unsandboxed, + "true", + "reclaim".into(), + root.path().into(), + HashMap::new(), + super::super::HardKill::After(Duration::from_secs(30)), + None, + storage.path().into(), + 0, + false, + false, + Some(root.path().into()), + 24, + 80, + ), + "remote" => registry + .spawn_remote_typed( + super::super::RemoteLaunch { + requested_platform: None, + params: Default::default(), + explicit_runon: true, + connection_file: None, + harness: "reclaim".into(), + session: "reclaim".into(), + requested_vcpus: None, + requested_network: false, + }, + SpawnPlan::Unsandboxed, + "true", + "/bin/sh".into(), + "reclaim".into(), + root.path().into(), + HashMap::new(), + super::super::HardKill::After(Duration::from_secs(30)), + None, + storage.path().into(), + super::super::TaskSlot::Background { max: 0 }, + false, + false, + Some(root.path().into()), + ) + .map_err(|error| error.to_string()), + _ => unreachable!(), + }; + ROOT_LOCK_PROBE.with(|hook| hook.borrow_mut().take()); + assert!( + result.is_err(), + "{kind}: slot limit must refuse before spawning" + ); + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "{kind}: root lookup was not exercised" + ); + } +} + +#[test] +fn root_reclaim_registration_reuses_identity_under_admission_and_claim_locks() { + let registry = BgTaskRegistry::default(); + let root = tempfile::tempdir().unwrap(); + let storage = tempfile::tempdir().unwrap(); + let alias = storage.path().join("root-alias"); + std::os::unix::fs::symlink(root.path(), &alias).unwrap(); + let canonical_root = canonicalized_path(&alias); + let layout = super::super::persistence::create_task_layout( + storage.path(), + "reclaim", + "bash-0000000000000013", + ) + .unwrap(); + let metadata = PersistedTask::starting( + "bash-0000000000000013".into(), + "reclaim".into(), + "no process".into(), + alias.clone(), + Some(alias.clone()), + None, + false, + false, + ); + std::fs::remove_file(alias).unwrap(); + let _admission = + registry.background_admission_guard(super::super::TaskSlot::Background { max: 1 }); + let _claims = saved_remote_task_claims().lock().unwrap(); + ROOT_LOCK_PROBE.with(|hook| { + *hook.borrow_mut() = Some(Box::new(|| { + panic!("registration repeated root identity I/O under locks") + })) + }); + registry + .insert_rehydrated_task_with_root(metadata, layout.paths, true, canonical_root.clone()) + .unwrap(); + ROOT_LOCK_PROBE.with(|hook| hook.borrow_mut().take()); + assert_eq!( + registry + .task_for_session("bash-0000000000000013", "reclaim") + .unwrap() + .project_root, + canonical_root + ); +} From c122859625a22c0a25decc4d57e5f28b530fc3f0 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Mon, 26 Oct 2026 10:23:08 +0100 Subject: [PATCH 3/8] release notes and search-quality descriptor for deleted-root-reclaim Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- .../slice-descriptors/land-deleted-root-reclaim.json | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 benchmarks/aft-search/slice-descriptors/land-deleted-root-reclaim.json diff --git a/benchmarks/aft-search/slice-descriptors/land-deleted-root-reclaim.json b/benchmarks/aft-search/slice-descriptors/land-deleted-root-reclaim.json new file mode 100644 index 000000000..ebd574711 --- /dev/null +++ b/benchmarks/aft-search/slice-descriptors/land-deleted-root-reclaim.json @@ -0,0 +1,8 @@ +{ + "targeted_mechanism": "none", + "kind": "harness", + "slice_class": "non_ranking", + "fixtures": [ + "harness-goldens" + ] +} From c950c8f66b2fbc73ad71b334c21f3e2712abd023 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Mon, 26 Oct 2026 10:34:26 +0100 Subject: [PATCH 4/8] Fill RemoteLaunch's configured_env_names in the reclaim test fixture Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/bash_background/root_reclaim_tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/aft/src/bash_background/root_reclaim_tests.rs b/crates/aft/src/bash_background/root_reclaim_tests.rs index efc46b458..520621a70 100644 --- a/crates/aft/src/bash_background/root_reclaim_tests.rs +++ b/crates/aft/src/bash_background/root_reclaim_tests.rs @@ -236,6 +236,7 @@ fn root_reclaim_identity_is_resolved_before_spawn_admission() { session: "reclaim".into(), requested_vcpus: None, requested_network: false, + configured_env_names: Vec::new(), }, SpawnPlan::Unsandboxed, "true", From 59b9bed6957b0da00d294e55a71e09a1c668cdf9 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Mon, 26 Oct 2026 11:31:13 +0100 Subject: [PATCH 5/8] mason: compare Windows reclaim roots without verbatim spelling drift Use the existing Windows path normalizer on both sides of root-reclaim comparisons. An admitted task retains fs::canonicalize's verbatim spelling, while a deleted directory's plain routing path can no longer be canonicalized. Keep stored task identities unchanged and perform no new filesystem lookup at comparison time. Add host-independent drive/UNC/drive-case/separator spelling tests with foreign-root and device-namespace negatives. The test failed first with raw equality. Focused reclaim tests (16 + 2) and the Windows compile gate pass; the single native Windows attempt was canceled after remaining queued for nearly three minutes, as requested. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> (cherry picked from commit b16a14731b9e98de2ac257524c7610b4bc298d4e) --- crates/aft/src/bash_background/registry.rs | 55 ++++++++++++++++++++-- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/crates/aft/src/bash_background/registry.rs b/crates/aft/src/bash_background/registry.rs index 3a8b9de5e..bc6985530 100644 --- a/crates/aft/src/bash_background/registry.rs +++ b/crates/aft/src/bash_background/registry.rs @@ -9056,7 +9056,9 @@ impl BgTaskRegistry { let tasks = self.task_arcs(); let targets: Vec<_> = tasks .into_iter() - .filter(|task| task.project_root == canonical_root) + .filter(|task| { + reclaim_root_paths_match(&task.project_root, &canonical_root, cfg!(windows)) + }) .filter_map(|task| { let sample_missing = { let state = task.lock_state().ok()?; @@ -11369,10 +11371,9 @@ impl BgTaskRegistry { project_root: &crate::path_identity::ProjectRootId, ) -> Option { let tasks = self.inner.tasks.try_lock().ok()?; - for task in tasks - .values() - .filter(|task| task.project_root == project_root.as_path()) - { + for task in tasks.values().filter(|task| { + reclaim_root_paths_match(&task.project_root, project_root.as_path(), cfg!(windows)) + }) { let state = task.state.try_lock().ok()?; if BgTask::state_needs_root_reclaim(&state) { return Some(true); @@ -14743,6 +14744,18 @@ fn modified_within(path: &Path, grace: Duration) -> bool { .unwrap_or(false) } +/// Windows canonicalization adds a verbatim prefix, but a deleted root can +/// only supply its plain routing path. Normalize the comparison, not the +/// stored identity, without probing a directory that may no longer exist. +fn reclaim_root_paths_match(stored: &Path, requested: &Path, windows: bool) -> bool { + if windows { + crate::windows_path::normalize_windows_path(stored) + == crate::windows_path::normalize_windows_path(requested) + } else { + stored == requested + } +} + fn canonicalized_path(path: &Path) -> PathBuf { #[cfg(test)] ROOT_LOCK_PROBE.with(|hook| { @@ -17222,6 +17235,38 @@ mod tests { ); } + #[test] + fn root_reclaim_compares_windows_verbatim_spellings() { + for (verbatim, plain) in [ + (r"\\?\C:\work\gone", r"C:\work\gone"), + (r"\\?\UNC\server\share\gone", r"\\server\share\gone"), + (r"\\?\c:\work\gone", "C:/work/gone"), + ] { + assert!( + reclaim_root_paths_match(Path::new(verbatim), Path::new(plain), true), + "{verbatim} must match {plain}" + ); + assert!( + reclaim_root_paths_match(Path::new(plain), Path::new(verbatim), true), + "{plain} must match {verbatim}" + ); + assert!( + !reclaim_root_paths_match(Path::new(verbatim), Path::new(plain), false), + "non-Windows comparisons must keep literal path identity" + ); + } + assert!(!reclaim_root_paths_match( + Path::new(r"\\?\C:\work\gone"), + Path::new(r"C:\work\other"), + true + )); + assert!(!reclaim_root_paths_match( + Path::new(r"\\?\Volume{1234}\gone"), + Path::new(r"Volume{1234}\gone"), + true + )); + } + #[test] fn lifecycle_detach_reaches_all_active_waits() { let registry = BgTaskRegistry::default(); From 3ee14ba3bfc80ace0cb147b7057d69010dbae387 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 27 Oct 2026 03:41:24 +0100 Subject: [PATCH 6/8] Fill LiveDescendant's user and command in the reclaim fixture (fields added on main) Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/bash_background/root_reclaim_tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/aft/src/bash_background/root_reclaim_tests.rs b/crates/aft/src/bash_background/root_reclaim_tests.rs index 520621a70..ccc1208f3 100644 --- a/crates/aft/src/bash_background/root_reclaim_tests.rs +++ b/crates/aft/src/bash_background/root_reclaim_tests.rs @@ -28,6 +28,8 @@ pub(crate) fn register_task( pid, comm: "sleep".into(), argv0: "sleep".into(), + user: None, + command: None, }] }); super::super::persistence::write_task(&layout.paths.json, &metadata).unwrap(); From 7ccb7f797cf5c61dba8118244fd84fd3bea98237 Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 27 Oct 2026 05:00:04 +0100 Subject: [PATCH 7/8] mason: authenticate terminal reclaim fixture processes The synthetic terminal task supplied a PID and cached survivor row but no saved start-time identity. The terminal-group safety guard therefore correctly refused to signal it. Record the owned child's real identity through mark_running before assigning the fixture's requested status; do not weaken product PID-reuse protection. Wait on the owned Child's exit and require SIGTERM or SIGKILL rather than assuming try_wait is ready when the kill worker clears its flag. Natural command completion cannot satisfy that assertion. The reported test also failed on Linux at 3ee14ba3bf before this change; all six deleted-root reclaim fixtures now pass on Linux. Native macOS confirmation is requested separately. Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> (cherry picked from commit 36efc369e199211713096506956121fe65d82ef5) --- crates/aft/src/bash_background/root_reclaim_tests.rs | 12 ++++++++++-- crates/aft/src/subc/deleted_root_reclaim_tests.rs | 10 ++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/crates/aft/src/bash_background/root_reclaim_tests.rs b/crates/aft/src/bash_background/root_reclaim_tests.rs index ccc1208f3..0f3aafad3 100644 --- a/crates/aft/src/bash_background/root_reclaim_tests.rs +++ b/crates/aft/src/bash_background/root_reclaim_tests.rs @@ -20,9 +20,17 @@ pub(crate) fn register_task( true, false, ); + // Terminal group kills require a saved process identity so a reused PID + // cannot authorize signaling an unrelated group. Record the real spawn's + // ownership evidence before overriding its status for the fixture. + if let Some(pid) = pid { + metadata.mark_running(pid, i32::try_from(pid).expect("owned PGID fits i32")); + assert!( + metadata.child_start_time_ms.is_some(), + "owned fixture child must have a readable process identity" + ); + } metadata.status = status; - metadata.child_pid = pid; - metadata.pgid = pid.map(|pid| pid as i32); metadata.live_descendants = pid.map(|pid| { vec![super::super::process::LiveDescendant { pid, diff --git a/crates/aft/src/subc/deleted_root_reclaim_tests.rs b/crates/aft/src/subc/deleted_root_reclaim_tests.rs index d1462278b..3beb44ab8 100644 --- a/crates/aft/src/subc/deleted_root_reclaim_tests.rs +++ b/crates/aft/src/subc/deleted_root_reclaim_tests.rs @@ -100,6 +100,8 @@ fn deleted_root_terminal_group_with_no_sample_is_checked_before_reclaim() { } fn assert_terminal_survivor_reclaimed(clear_sample: bool) { + use std::os::unix::process::ExitStatusExt; + let mut fixture = ReclaimFixture::new(); let mut group = OwnedGroup::spawn(); fixture.task( @@ -113,9 +115,13 @@ fn assert_terminal_survivor_reclaimed(clear_sample: bool) { } assert_eq!(fixture.sweep().evicted, 0); fixture.wait_for_kill(); + // The worker settles group liveness, not this test's Child handle. Reap + // the child to observe the signal rather than requiring try_wait to be + // ready immediately after the worker clears its in-flight flag. + let status = group.0.wait().unwrap(); assert!( - group.0.try_wait().unwrap().is_some(), - "terminal task's owned group survived reclaim" + matches!(status.signal(), Some(libc::SIGTERM) | Some(libc::SIGKILL)), + "terminal task's owned group was not killed by a reclaim signal: {status}" ); let outcome = fixture.sweep(); fixture.assert_reclaimed(outcome); From 8ff4ce7ee256228381326b82f8903406b9386d9c Mon Sep 17 00:00:00 2001 From: ualtinok <94532+ualtinok@users.noreply.github.com> Date: Tue, 27 Oct 2026 05:11:34 +0100 Subject: [PATCH 8/8] Bound the reclaim test's wait for the killed group Co-authored-by: alfonso-aft <289616620+alfonso-aft@users.noreply.github.com> --- crates/aft/src/subc/deleted_root_reclaim_tests.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/crates/aft/src/subc/deleted_root_reclaim_tests.rs b/crates/aft/src/subc/deleted_root_reclaim_tests.rs index 3beb44ab8..9d1d81f21 100644 --- a/crates/aft/src/subc/deleted_root_reclaim_tests.rs +++ b/crates/aft/src/subc/deleted_root_reclaim_tests.rs @@ -117,8 +117,19 @@ fn assert_terminal_survivor_reclaimed(clear_sample: bool) { fixture.wait_for_kill(); // The worker settles group liveness, not this test's Child handle. Reap // the child to observe the signal rather than requiring try_wait to be - // ready immediately after the worker clears its in-flight flag. - let status = group.0.wait().unwrap(); + // ready immediately after the worker clears its in-flight flag. The wait + // is bounded so a group that survived fails the test instead of hanging it. + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + let status = loop { + if let Some(status) = group.0.try_wait().unwrap() { + break status; + } + assert!( + std::time::Instant::now() < deadline, + "terminal task's owned group survived reclaim" + ); + std::thread::sleep(std::time::Duration::from_millis(20)); + }; assert!( matches!(status.signal(), Some(libc::SIGTERM) | Some(libc::SIGKILL)), "terminal task's owned group was not killed by a reclaim signal: {status}"