From 8d206f124f9e1f0327d14990346108b9cdc89deb Mon Sep 17 00:00:00 2001 From: atlas Date: Fri, 9 Oct 2026 23:57:07 +0200 Subject: [PATCH] hive-runtime: kill an ACP agent's whole process group, not just its pid The ACP agent was spawned with only `kill_on_drop`, which SIGKILLs the agent's own pid. Commands the agent had started (a `nix build`, a test run) were reparented and kept running after the agent was killed: when a subagent run ends after `interrupt`, when an agent ignores `session/cancel` past the grace, and when the harness drops the runtime. The agent now leads its own process group (`process_group(0)`), and dropping its `Connection` SIGKILLs that group. The soft path is unchanged: `interrupt` / `Canceller::cancel` still sends `session/cancel`. Refs #4999. The claude half needs hive-claude 0.1.3, which is not on crates.io yet. --- Cargo.lock | 1 + docs/tools/subagent.md | 13 ++--- hive-runtime/Cargo.toml | 1 + hive-runtime/README.md | 3 ++ hive-runtime/src/acp/mod.rs | 99 +++++++++++++++++++++++++++++++++++++ hive-runtime/src/acp/rpc.rs | 39 ++++++++++++++- 6 files changed, 149 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 885fd1c1..fef61d4b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2012,6 +2012,7 @@ name = "hive-runtime" version = "0.1.0" dependencies = [ "hive-claude", + "libc", "serde", "serde_json", "swarm-secret-client", diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index b4b7dda5..d6dace69 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -317,18 +317,19 @@ page applies as written. On `acp`: -- each run spawns its own copy of the parent's ACP agent, which exits - when the run ends. The unit loads `backendEnvironmentFile` for it. On the - `opencode` preset, the daemon reads a provider key that file leaves unset - from the swarm secret store at `swarm/agents//acp-provider`, as the - harness does. +- each run spawns its own copy of the parent's ACP agent. The end of the + run kills it and every command it started. The unit loads + `backendEnvironmentFile` for it. On the `opencode` preset, the daemon + reads a provider key that file leaves unset from the swarm secret store + at `swarm/agents//acp-provider`, as the harness does. - the session id lives in `subagent-acp-session-` under the harness dir. `start` moves an old one aside, `continue` loads it into a fresh agent process, and a `continue` with none recorded fails straight away. - a role goes in front of the session's first prompt, since ACP has no system prompt. - `interrupt` sends `session/cancel`, and `force` changes nothing. The - runtime kills an agent that ignores the cancel for 10s. + runtime kills an agent that ignores the cancel for 10s, with every + command it started. - the runtime sets `model` and `effort` on the session when the agent offers that value for its `model` or `thought_level` config option. Any other value does nothing (`hive_runtime::AcpRuntime`). diff --git a/hive-runtime/Cargo.toml b/hive-runtime/Cargo.toml index 4c200a71..4c827025 100644 --- a/hive-runtime/Cargo.toml +++ b/hive-runtime/Cargo.toml @@ -9,6 +9,7 @@ workspace = true [dependencies] hive-claude.workspace = true +libc.workspace = true serde.workspace = true serde_json.workspace = true # The ACP agent's provider key, read under the agent's own certificate diff --git a/hive-runtime/README.md b/hive-runtime/README.md index 8850f3f1..11183df4 100644 --- a/hive-runtime/README.md +++ b/hive-runtime/README.md @@ -40,6 +40,9 @@ with `AcpError::IdleTimeout` (`IdleKilled` if the agent was killed). This is the only way a provider error the agent retries without reporting it, such as an HTTP 429, ends the turn. +The agent leads its own process group. Killing it, and dropping the runtime, +`SIGKILL`s that group, so the commands the agent started die with it. + ## ACP backend: compaction The same `CompactionPolicy` as the claude backend decides when: after a turn diff --git a/hive-runtime/src/acp/mod.rs b/hive-runtime/src/acp/mod.rs index 1bf1edb1..b93be539 100644 --- a/hive-runtime/src/acp/mod.rs +++ b/hive-runtime/src/acp/mod.rs @@ -1062,6 +1062,10 @@ mod tests { const AGENT: &str = r#" n=0 prompt= model=m/think effort=low printf 'start\n' >> "$1.methods" +# With GRANDCHILD set: a background `sleep`, its pid in .grandchild. +if [ -n "$GRANDCHILD" ]; then + sh -c 'echo $$ >"$1.grandchild"; exec sleep 300' sh "$1" >/dev/null 2>&1 & +fi update() { printf '{"jsonrpc":"2.0","method":"session/update","params":{"sessionId":"%s","update":%s}}\n' "$1" "$2" } @@ -1430,6 +1434,101 @@ done assert_eq!(count(dir.path(), "start"), 2); } + /// Poll `path` until it holds a complete, newline-terminated pid. + async fn read_pid(path: &Path) -> i32 { + for _ in 0..250 { + if let Ok(text) = std::fs::read_to_string(path) + && text.ends_with('\n') + { + return text.trim().parse().expect("pid file holds a pid"); + } + tokio::time::sleep(Duration::from_millis(20)).await; + } + unreachable!("no pid in {} within 5s", path.display()); + } + + /// Whether `pid` is still running. A zombie counts as gone: it has + /// exited, and `kill(pid, 0)` only keeps finding it until it is reaped. + fn is_running(pid: i32) -> bool { + // SAFETY: signal 0 delivers nothing; it only checks that `pid` exists. + if unsafe { libc::kill(pid, 0) } != 0 { + return false; + } + // Field 3 of /proc//stat, right after the parenthesised comm, is + // the state letter. Without /proc the process counts as running. + !std::fs::read_to_string(format!("/proc/{pid}/stat")).is_ok_and(|stat| { + stat.rsplit_once(") ") + .is_some_and(|(_, rest)| rest.starts_with('Z')) + }) + } + + /// Require `pid` to be gone within 5s, killing it if it is not. + async fn assert_gone(pid: i32) { + let mut alive = is_running(pid); + for _ in 0..250 { + if !alive { + break; + } + tokio::time::sleep(Duration::from_millis(20)).await; + alive = is_running(pid); + } + if alive { + // SAFETY: `kill(2)` on a pid this test started; keeps a failed + // run from leaving a 300s `sleep` behind. + unsafe { + libc::kill(pid, libc::SIGKILL); + } + } + assert!(!alive, "grandchild {pid} outlived the agent"); + } + + #[tokio::test] + async fn an_agent_ignoring_a_cancel_is_killed_with_the_commands_it_started() { + let dir = tempfile::tempdir().unwrap(); + let mut runtime = agent( + dir.path(), + "deaf", + &[("GRANDCHILD", "1")], + PercentPolicy::default(), + ); + runtime.cancel_grace = Duration::from_millis(200); + let (canceller, config) = (runtime.canceller().unwrap(), config(dir.path())); + + let (done, grandchild) = tokio::join!(runtime.run(&config, "one", &NoopSink), async { + prompt_sent(dir.path()).await; + let grandchild = read_pid(&dir.path().join("prompts.grandchild")).await; + assert!(canceller.cancel()); + grandchild + }); + assert!( + matches!(done, Err(Error::Acp(AcpError::CancelIgnored { .. }))), + "{done:?}" + ); + assert_gone(grandchild).await; + } + + /// A subagent's run drops its runtime when it ends, an interrupted one + /// included. + #[tokio::test] + async fn dropping_the_runtime_kills_the_commands_its_agent_started() { + let dir = tempfile::tempdir().unwrap(); + let runtime = agent( + dir.path(), + "ok", + &[("GRANDCHILD", "1")], + PercentPolicy::default(), + ); + runtime + .run(&config(dir.path()), "one", &NoopSink) + .await + .unwrap(); + let grandchild = read_pid(&dir.path().join("prompts.grandchild")).await; + assert!(is_running(grandchild)); + + drop(runtime); + assert_gone(grandchild).await; + } + fn sent(session: &str, text: &str) -> (String, String) { (session.to_owned(), text.to_owned()) } diff --git a/hive-runtime/src/acp/rpc.rs b/hive-runtime/src/acp/rpc.rs index acccdb8b..65681f02 100644 --- a/hive-runtime/src/acp/rpc.rs +++ b/hive-runtime/src/acp/rpc.rs @@ -30,8 +30,15 @@ pub(super) enum Incoming { type Reply = Result; type Pending = Arc)>>>; +/// The agent runs as the leader of its own process group; dropping the +/// connection `SIGKILL`s that group, so the commands the agent started die +/// with it. A descendant that left the group (`setsid`, `setpgid`) is not +/// reached. pub(super) struct Connection { child: Child, + /// The agent's pid, which is its group's id. Read at spawn: a reaped + /// child has no id, and the group can outlive its leader. + pgid: Option, stdin: Arc>, next_id: AtomicU64, pending: Pending, @@ -58,12 +65,14 @@ impl Connection { .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) + .process_group(0) .kill_on_drop(true) .spawn() .map_err(|source| AcpError::Spawn { program: command.command.clone(), source, })?; + let pgid = group_id(child.id()); let (Some(stdin), Some(stdout), Some(stderr)) = (child.stdin.take(), child.stdout.take(), child.stderr.take()) else { @@ -106,6 +115,7 @@ impl Connection { Ok(Self { child, + pgid, stdin, next_id: AtomicU64::new(1), pending, @@ -159,6 +169,25 @@ impl Connection { } } +/// The process group a leader with `pid` heads, as `kill(2)` takes it. +/// Never 0: `kill(-0, …)` signals the caller's own group. +fn group_id(pid: Option) -> Option { + pid.and_then(|pid| i32::try_from(pid).ok()) + .filter(|&pgid| pgid > 0) +} + +impl Drop for Connection { + fn drop(&mut self) { + if let Some(pgid) = self.pgid { + // SAFETY: plain `kill(2)`; `-pgid` targets the agent's own + // process group, which `process_group(0)` made it lead. + unsafe { + libc::kill(-pgid, libc::SIGKILL); + } + } + } +} + /// The stdout side: routes responses to their waiting request, answers the /// agent's own requests, and queues its notifications. struct Reader { @@ -309,7 +338,7 @@ pub(super) fn permission_outcome( #[cfg(test)] mod tests { - use super::permission_outcome; + use super::{group_id, permission_outcome}; use crate::acp::{PermissionAsk, PermissionPolicy}; use serde_json::json; use std::sync::Arc; @@ -406,4 +435,12 @@ mod tests { json!({ "outcome": { "outcome": "cancelled" } }) ); } + + #[test] + fn a_group_id_is_a_positive_pid() { + assert_eq!(group_id(Some(42)), Some(42)); + assert_eq!(group_id(Some(0)), None); + assert_eq!(group_id(Some(u32::MAX)), None); + assert_eq!(group_id(None), None); + } }