diff --git a/crates/meshloop-adapters/src/harness.rs b/crates/meshloop-adapters/src/harness.rs index cfc8e1c..35cb041 100644 --- a/crates/meshloop-adapters/src/harness.rs +++ b/crates/meshloop-adapters/src/harness.rs @@ -197,6 +197,9 @@ impl HarnessCapabilities for CliHarness { // Idempotent against an already-exited/never-tracked process, per ADR 0003. if let Some((mut child, _, _)) = registry.remove(&Self::attempt_key(handle)) { child.kill_tree(); + } else if let Some(pid) = handle.pid { + // Not ours: a previous Meshloop process started it and is gone (#30). + crate::process::kill_orphaned_tree(pid, &self.config.executable); } Ok(()) } diff --git a/crates/meshloop-adapters/src/process.rs b/crates/meshloop-adapters/src/process.rs index af0ca08..3d1c5c2 100644 --- a/crates/meshloop-adapters/src/process.rs +++ b/crates/meshloop-adapters/src/process.rs @@ -103,6 +103,57 @@ impl ProcessView for NullPidIsDead { } } +/// Kills the process tree of an attempt this process no longer holds a handle for, such as +/// a harness left running when a previous Meshloop process crashed (ADR 0025 amendment, #30). +/// +/// PIDs can be reused, so the tree is killed only when `pid` is alive, runs the expected +/// executable, and on POSIX still leads its own process group (as `spawn_owned` arranges). +/// Returns whether a kill was issued. +pub fn kill_orphaned_tree(pid: u32, executable: &std::path::Path) -> bool { + if pid == 0 || !orphan_identity_matches(pid, executable) { + return false; + } + kill_process_tree(pid); + true +} + +#[cfg(windows)] +fn orphan_identity_matches(pid: u32, executable: &std::path::Path) -> bool { + let Some(stem) = executable.file_stem() else { + return false; + }; + let image = format!("{}.exe", stem.to_string_lossy()); + WindowsProcessView.is_live(&ProcessHint { + pid, + image_name: Some(image), + }) == LiveCheck::Live +} + +#[cfg(not(windows))] +fn orphan_identity_matches(pid: u32, executable: &std::path::Path) -> bool { + let Some(name) = executable + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + else { + return false; + }; + let Ok(out) = Command::new("ps") + .args(["-o", "pgid=,stat=,comm=", "-p", &pid.to_string()]) + .output() + else { + return false; + }; + let text = String::from_utf8_lossy(&out.stdout); + let mut fields = text.split_whitespace(); + let (Some(pgid), Some(stat), Some(comm)) = (fields.next(), fields.next(), fields.next()) else { + return false; + }; + // Linux truncates comm to 15 bytes; macOS reports a path. + let comm = comm.rsplit('/').next().unwrap_or(comm); + let name_matches = comm == name || (comm.len() == 15 && name.starts_with(comm)); + pgid == pid.to_string() && !stat.starts_with('Z') && name_matches +} + /// Kills a process and all of its descendants (the entire process tree). /// /// On Windows, executes `taskkill /F /T /PID ` to terminate child compilers, diff --git a/crates/meshloop-adapters/tests/harness_contract.rs b/crates/meshloop-adapters/tests/harness_contract.rs index c3e63df..f64d0c8 100644 --- a/crates/meshloop-adapters/tests/harness_contract.rs +++ b/crates/meshloop-adapters/tests/harness_contract.rs @@ -190,3 +190,115 @@ fn collect_on_an_unknown_handle_is_unsupported_not_a_panic() { Err(HarnessError::ProcessFault { .. }) )); } + +fn wait_until_dead(pid: u32) -> bool { + use meshloop_engine::ports::{LiveCheck, ProcessHint, ProcessView}; + let view = meshloop_adapters::process::HostProcessView {}; + let hint = ProcessHint { + pid, + image_name: None, + }; + let start = std::time::Instant::now(); + while start.elapsed() < Duration::from_secs(5) { + if view.is_live(&hint) != LiveCheck::Live { + return true; + } + std::thread::sleep(Duration::from_millis(50)); + } + false +} + +#[test] +fn cancel_kills_an_orphaned_tree_left_by_a_previous_process() { + // Simulates recovery after a crash (#30): the attempt's process tree is still running but + // this harness instance never started it, so only the recorded pid is known. + let dir = std::env::temp_dir().join(format!("meshloop-test-{}-orphan", std::process::id())); + fs::create_dir_all(&dir).unwrap(); + let pid_file = dir.join("grandchild.pid"); + let mut cmd = std::process::Command::new(fixture_path()); + cmd.arg("--spawn-grandchild") + .arg(&pid_file) + .arg(dir.join("heartbeat.txt")); + let orphan = meshloop_adapters::process::spawn_owned(cmd).expect("spawn orphan tree"); + let leader = orphan.id(); + let start = std::time::Instant::now(); + let grandchild: u32 = loop { + if let Some(pid) = fs::read_to_string(&pid_file) + .ok() + .and_then(|s| s.trim().parse().ok()) + { + break pid; + } + assert!( + start.elapsed() < Duration::from_secs(5), + "grandchild never started" + ); + std::thread::sleep(Duration::from_millis(50)); + }; + // Drop ownership without killing, as a crashed process would. + std::mem::forget(orphan); + + let harness = CliHarness::new(CliHarnessConfig { + name: "fixture".into(), + executable: fixture_path(), + version_args: vec!["--version".into()], + probe_timeout: Duration::from_secs(10), + invoke_args_template: vec![], + }); + let handle = HarnessHandle { + attempt_id: AttemptId(4242), + pid: Some(leader), + pane_id: None, + }; + harness + .cancel(&handle) + .expect("cancel of an untracked attempt"); + + assert!( + wait_until_dead(leader), + "orphaned leader {leader} survived cancel" + ); + assert!( + wait_until_dead(grandchild), + "orphaned grandchild {grandchild} survived cancel" + ); + fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn cancel_leaves_an_unrelated_process_with_a_recorded_pid_alone() { + // A recycled pid must not be killed: the recorded pid now belongs to another executable. + #[cfg(windows)] + let mut other = std::process::Command::new("powershell") + .args(["-NoProfile", "-Command", "Start-Sleep -Seconds 30"]) + .spawn() + .expect("spawn unrelated process"); + #[cfg(not(windows))] + let mut other = std::process::Command::new("sleep") + .arg("30") + .spawn() + .expect("spawn unrelated process"); + let harness = CliHarness::new(CliHarnessConfig { + name: "fixture".into(), + executable: fixture_path(), + version_args: vec!["--version".into()], + probe_timeout: Duration::from_secs(10), + invoke_args_template: vec![], + }); + let handle = HarnessHandle { + attempt_id: AttemptId(4243), + pid: Some(other.id()), + pane_id: None, + }; + harness + .cancel(&handle) + .expect("cancel of an untracked attempt"); + std::thread::sleep(Duration::from_millis(300)); + let still_running = other.try_wait().expect("poll unrelated process").is_none(); + let _ = other.kill(); + let _ = other.wait(); + assert!( + still_running, + "cancel killed a process that is not this harness" + ); +} diff --git a/crates/meshloop-engine/src/run_loop.rs b/crates/meshloop-engine/src/run_loop.rs index 753ff17..41b1e21 100644 --- a/crates/meshloop-engine/src/run_loop.rs +++ b/crates/meshloop-engine/src/run_loop.rs @@ -711,6 +711,17 @@ impl<'a> RunLoop<'a> { }; match self.attempt_live(&attempt) { LiveCheck::Dead => { + // The harness no longer tracks this attempt, but a previous Meshloop + // process may have left its process tree running: end it before the + // node is retried in a fresh attempt (#30). Cancel is idempotent. + if let Some(h) = attempt + .harness + .as_deref() + .and_then(|n| self.harnesses.get(n)) + { + let _ = + h.cancel(&Self::attempt_handle(&attempt, self.resolve_pane(&attempt))); + } let rec = self.append( graph_id, node.id, diff --git a/docs/architecture/adr/0025-process-tree-ownership.md b/docs/architecture/adr/0025-process-tree-ownership.md index 3b5650c..a4452d4 100644 --- a/docs/architecture/adr/0025-process-tree-ownership.md +++ b/docs/architecture/adr/0025-process-tree-ownership.md @@ -47,10 +47,25 @@ Constraints: - `crates/meshloop-adapters/src/process/job.rs`: Win32 Job Object abstraction (`JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`) and POSIX `setpgid(0, 0)` via `spawn_owned`. - `crates/meshloop-adapters/tests/process_tree.rs`: - `grandchild_dies_on_timeout`: confirms grandchild termination on timeout/kill_tree. - - `grandchild_dies_on_parent_abort`: confirms OS kernel terminates grandchildren when parent abruptly aborts (`std::process::abort()`). + - `grandchild_dies_on_parent_abort` (Windows only): confirms OS kernel terminates grandchildren when parent abruptly aborts (`std::process::abort()`). POSIX: see the 2026-10-07 recovery amendment. - `xtask bench`: metric `conc.orphan_process_count` reports `0` (PASS). - 100% passing tests in `cargo test -p meshloop-adapters` (24 passed) and `cargo test --workspace` (131 passed). +## Amendment 2026-10-07: POSIX orphans are reclaimed during recovery + +- Status: Accepted +- Approval evidence: Samuel (human) chose option 1 ("recovery reconciliation") for issue #30 on 2026-10-07, in the AgentFlow/Meshloop setup session. +- Executor: Claude Code (Opus 5.5) + +**Context.** The guarantee that grandchildren die when Meshloop aborts holds only on Windows, where the Job Object closes with the process. A POSIX process group is torn down only when Meshloop calls `kill_tree`. If Meshloop itself aborts or is SIGKILLed, the harness group keeps running; on WSL2 Ubuntu 24.04 the abort fixture left two processes alive for more than 11 minutes. `grandchild_dies_on_parent_abort` is therefore limited to Windows. + +**Decision.** Reclaim orphans during recovery, in safe Rust. When `RunLoop` reattaches a `Running` attempt that the current harness instance does not track, it calls `HarnessCapabilities::cancel` before recording `HarnessCrashedOrTimeout` and retrying. For an untracked attempt with a recorded pid, `CliHarness::cancel` calls `process::kill_orphaned_tree`. That function kills the tree only when the pid is alive and runs the harness executable (image name on Windows; `comm` on POSIX, which Linux truncates to 15 bytes), and, on POSIX, still leads its own process group. This guards against pid reuse. + +**Alternatives rejected.** `prctl(PR_SET_PDEATHSIG)` is Linux-only, covers only the direct child, and needs `unsafe`. A subreaper supervisor process adds moving parts. + +**Consequences.** Orphans keep running between the crash and the next `resume` or `status` of that graph; they are not reclaimed immediately. A harness launched through a shim whose process image differs from the configured executable is not matched, so it is left alone rather than risk killing an unrelated process. + +**Verification.** `crates/meshloop-adapters/tests/harness_contract.rs`: `cancel_kills_an_orphaned_tree_left_by_a_previous_process` fails without the untracked-pid branch ("orphaned leader survived cancel") and passes with it. `cancel_leaves_an_unrelated_process_with_a_recorded_pid_alone` covers the pid-reuse guard. ## Amendment 2026-10-07: detached spawns do not inherit caller handles - Status: Accepted