From 9cf9200bdb74cd2f2e32a231beef652adc5309f6 Mon Sep 17 00:00:00 2001 From: shiva Date: Thu, 6 Aug 2026 19:08:41 +0530 Subject: [PATCH 1/2] fix(jobs): kill_graceful reaches descendants that escaped into their own process group Found live: letting the (now-fixed) supervisor/work dispatch pipeline run item #13 end-to-end surfaced a real orphaned agent CLI process once its job hit its timeout. Two independent layers each isolate their own child into a fresh process group so their own timeout can kill it: agentflare-jobs::Supervisor does this to the `agentflare work` process, and agent_launch::run_captured does the same, again, to *its* child (the actual agent CLI) -- needed so it can still enforce a timeout when `agentflare work` is run standalone by a human, with no outer supervisor at all. When the outer layer's timeout fires first, its group-kill only reaches whatever is still in `agentflare work`'s group -- the agent CLI has already been re-grouped by the inner layer and is left running, unsupervised, past the timeout. kill_graceful now also walks /proc//task/*/children recursively (Linux-only; no /proc on macOS/Windows) to find every descendant by parent-child relationship, which re-grouping never changes, and signals each one directly alongside the existing group signal. New test reproduces the exact escape (a setsid'd grandchild) and confirms it's killed on timeout rather than orphaned. --- crates/agentflare-jobs/src/supervisor.rs | 105 ++++++++++++++++++++--- 1 file changed, 93 insertions(+), 12 deletions(-) diff --git a/crates/agentflare-jobs/src/supervisor.rs b/crates/agentflare-jobs/src/supervisor.rs index 7df7b199..a849f84e 100644 --- a/crates/agentflare-jobs/src/supervisor.rs +++ b/crates/agentflare-jobs/src/supervisor.rs @@ -145,23 +145,59 @@ impl Supervisor { } } +/// PIDs of every live descendant of `pid` (children, grandchildren, ...), +/// walked via `/proc//task/*/children` -- Linux-only (no `/proc` on +/// macOS/Windows). Needed because a descendant that called +/// `process_group(0)` on itself (see `agent_launch::run_captured`, which +/// does exactly this so *it* can kill a runaway agent CLI) has left the +/// process group `-{pid}` targets below; signaling it by group alone won't +/// reach it, so `kill_graceful` also signals every PID this returns +/// directly. +#[cfg(target_os = "linux")] +fn descendant_pids(pid: u32) -> Vec { + fn children_of(pid: u32) -> Vec { + let task_dir = format!("/proc/{pid}/task"); + let Ok(entries) = std::fs::read_dir(&task_dir) else { + return Vec::new(); + }; + let mut out = Vec::new(); + for entry in entries.flatten() { + let children_path = entry.path().join("children"); + let Ok(contents) = std::fs::read_to_string(&children_path) else { + continue; + }; + out.extend(contents.split_whitespace().filter_map(|s| s.parse::().ok())); + } + out + } + + let mut all = Vec::new(); + let mut frontier = children_of(pid); + while let Some(next_pid) = frontier.pop() { + all.push(next_pid); + frontier.extend(children_of(next_pid)); + } + all +} + +#[cfg(not(target_os = "linux"))] +fn descendant_pids(_pid: u32) -> Vec { + Vec::new() +} + fn kill_graceful(child: &mut std::process::Child, kill_after: Duration) { #[cfg(unix)] { let pid = child.id(); - let _ = Command::new("kill") - .arg("-s") - .arg("TERM") - .arg("--") - .arg(format!("-{pid}")) - .status(); + let signal = |sig: &str, pid: u32| { + let _ = Command::new("kill").arg("-s").arg(sig).arg("--").arg(format!("-{pid}")).status(); + for descendant in descendant_pids(pid) { + let _ = Command::new("kill").arg("-s").arg(sig).arg("--").arg(descendant.to_string()).status(); + } + }; + signal("TERM", pid); std::thread::sleep(kill_after); - let _ = Command::new("kill") - .arg("-s") - .arg("KILL") - .arg("--") - .arg(format!("-{pid}")) - .status(); + signal("KILL", pid); } #[cfg(windows)] { @@ -184,3 +220,48 @@ fn kill_graceful(child: &mut std::process::Child, kill_after: Duration) { let _ = child.kill(); } } + +#[cfg(test)] +mod tests { + use super::*; + + // `setsid` moves the grandchild into a brand-new session/process group of + // its own -- exactly what `agent_launch::run_captured` does to its own + // child so *it* can kill a runaway agent CLI independently. That leaves + // the grandchild outside the group `kill -TERM -- -` targets, so a + // fix that only sends the group signal would leave it running past the + // timeout. Only `/proc` (Linux) backs `descendant_pids`, so this is + // scoped to Linux CI rather than xfailing on macOS/Windows runners. + #[cfg(target_os = "linux")] + #[test] + fn spawn_times_out_and_kills_a_descendant_that_escaped_into_its_own_process_group() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("still-alive"); + let mut supervisor = Supervisor::new( + "test".to_string(), + "sh".to_string(), + vec![ + "-c".to_string(), + format!("setsid sh -c 'sleep 5; touch {}' & wait", marker.display()), + ], + vec![], + None, + 0, // times out immediately — the point is what happens on timeout + 0, + dir.path().to_path_buf(), + ); + + let (output, _state) = supervisor.spawn().unwrap(); + assert!(output.timed_out, "should report timeout"); + // The 5s `sleep` inside the setsid'd grandchild would still have it + // alive if `kill_graceful` only signaled the direct child's process + // group; give it a moment past `spawn()` returning, then confirm it + // never reached its `touch`. + std::thread::sleep(Duration::from_millis(500)); + assert!( + !marker.exists(), + "a descendant that escaped into its own process group must still be \ + killed on timeout, not just orphaned" + ); + } +} From fd584c50a424eb1ffa4455b0c4019810297da43f Mon Sep 17 00:00:00 2001 From: shiva Date: Fri, 7 Aug 2026 11:56:50 +0530 Subject: [PATCH 2/2] fix(jobs): match CI rustfmt output --- crates/agentflare-jobs/src/supervisor.rs | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/crates/agentflare-jobs/src/supervisor.rs b/crates/agentflare-jobs/src/supervisor.rs index a849f84e..38e168e4 100644 --- a/crates/agentflare-jobs/src/supervisor.rs +++ b/crates/agentflare-jobs/src/supervisor.rs @@ -166,7 +166,11 @@ fn descendant_pids(pid: u32) -> Vec { let Ok(contents) = std::fs::read_to_string(&children_path) else { continue; }; - out.extend(contents.split_whitespace().filter_map(|s| s.parse::().ok())); + out.extend( + contents + .split_whitespace() + .filter_map(|s| s.parse::().ok()), + ); } out } @@ -190,9 +194,19 @@ fn kill_graceful(child: &mut std::process::Child, kill_after: Duration) { { let pid = child.id(); let signal = |sig: &str, pid: u32| { - let _ = Command::new("kill").arg("-s").arg(sig).arg("--").arg(format!("-{pid}")).status(); + let _ = Command::new("kill") + .arg("-s") + .arg(sig) + .arg("--") + .arg(format!("-{pid}")) + .status(); for descendant in descendant_pids(pid) { - let _ = Command::new("kill").arg("-s").arg(sig).arg("--").arg(descendant.to_string()).status(); + let _ = Command::new("kill") + .arg("-s") + .arg(sig) + .arg("--") + .arg(descendant.to_string()) + .status(); } }; signal("TERM", pid);