From 028c283ff972d43cb57c3912ce1c8b38ef5ef16b Mon Sep 17 00:00:00 2001 From: atlas Date: Sat, 10 Oct 2026 13:44:10 +0200 Subject: [PATCH] claude runtime: interrupting a session kills every process it started hive-claude 0.1.3 spawns claude as the leader of its own process group, and its `Cancel::cancel` and idle watchdog signal that group instead of claude's pid alone. Commands claude started (a `nix build`, a test run) used to survive a subagent `interrupt` and keep running. Bump hive-claude to 0.1.3. The subagent `interrupt` call site needs no code change: `Cancel::cancel(force)` kept its signature. The harness's `/cancel` and `/logout` did not go through hive-claude: they ran `kill -INT ` on the claude child they found in /proc. They now SIGINT the group that child leads, through `libc::kill`. A `claude` child that leads no group (the `claude plugin` calls) shares the harness's group and is still signalled alone. Tests: an interrupted subagent's grandchild dies, for both SIGINT and `force`, and fails on hive-claude 0.1.2; a harness cancel stops a turn's grandchild, and fails when only the pid is signalled. --- Cargo.lock | 4 +- Cargo.toml | 2 +- docs/tools/subagent.md | 5 ++ docs/web-ui/agent.md | 14 +-- hive-agent/src/web_ui/actions.rs | 2 +- hive-agent/src/web_ui/auth.rs | 2 +- hive-agent/src/web_ui/mod.rs | 145 ++++++++++++++++++++++++++++--- hive-subagent-mcp/src/session.rs | 94 ++++++++++++++++++++ 8 files changed, 244 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fef61d4b..e8c42a52 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1835,9 +1835,9 @@ dependencies = [ [[package]] name = "hive-claude" -version = "0.1.2" +version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e43e06fd09a7b6d6cca3e70f06d00fa795ceddfe1555121ca0cf4fc578eb9c3" +checksum = "8c87ac627823f7bfc38478d7494e614e3f680dda6007dbcd306af51d3eed3bb3" dependencies = [ "libc", "serde", diff --git a/Cargo.toml b/Cargo.toml index 082db1a5..ab3840c7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -97,7 +97,7 @@ hive-jobq-metrics = { path = "hive-jobq-metrics" } hive-jobq-wire = { path = "hive-jobq-wire" } hive-log = { path = "hive-log" } hive-core-agent-sock = { path = "hive-core-agent-sock" } -hive-claude = "0.1.2" +hive-claude = "0.1.3" hive-host-sock = { path = "hive-host-sock" } hive-priv-sock = { path = "hive-priv-sock" } hive-runtime = { path = "hive-runtime" } diff --git a/docs/tools/subagent.md b/docs/tools/subagent.md index 12ef7381..0d59ec8c 100644 --- a/docs/tools/subagent.md +++ b/docs/tools/subagent.md @@ -166,6 +166,11 @@ from how the child exited. `continue` clears it and hands the session a fresh turn allowance, which is what makes an interrupt a pause you can undo rather than a session you have to abandon. +On `claude`, the signal goes to claude's whole process group, so the +commands the subagent started (a `nix build`, a test run) get it too. The +signal doesn't reach a command that moved itself out of the group +(`setsid`). + When the session knows where its report goes — `start`'s `report_file`, or the path the subagent names when it signals — the daemon appends the stop reason to that file as well, so the artifact you were going to read diff --git a/docs/web-ui/agent.md b/docs/web-ui/agent.md index 40cfcec7..ea836349 100644 --- a/docs/web-ui/agent.md +++ b/docs/web-ui/agent.md @@ -227,11 +227,11 @@ Slash commands today: - `/help` — list commands locally. - `/clear` — wipe the local terminal view (server history kept). -- `/cancel` — `POST /api/cancel` → host shellouts `pkill -INT - claude`, emits a Note. On an ACP agent it sends the agent - `session/cancel` instead, and kills it if the turn hasn't ended 10s - later. Also surfaces as a `■ cancel turn` button in the state row - while state=thinking. +- `/cancel` — `POST /api/cancel` → SIGINT to the turn's claude and every + command it started (its process group), emits a Note. On an ACP agent + it sends the agent `session/cancel` instead, and kills it if the turn + hasn't ended 10s later. Also surfaces as a `■ cancel turn` button in + the state row while state=thinking. - `/compact` — `POST /api/compact` → sets the deferred `Bus::request_compact()` flag and returns immediately. The harness runs the `/compact` at the next turn boundary (end of the in-flight @@ -272,8 +272,8 @@ shaped). - `POST /login/{start,code,cancel}` — claude OAuth login flow. Start/cancel emit `LiveEvent::StatusChanged` to flip the badge to/from `needs_login_in_progress`. -- `POST /api/cancel` — SIGINT the in-flight claude turn. Emits a - `LiveEvent::Note`. +- `POST /api/cancel` — SIGINT the in-flight claude turn and the + commands it started. Emits a `LiveEvent::Note`. - `POST /api/compact` — run `/compact` on the persistent session (same MCP config + system prompt + allowed tools as a normal turn — only the stdin payload differs). Flips state to diff --git a/hive-agent/src/web_ui/actions.rs b/hive-agent/src/web_ui/actions.rs index 21d2243f..ebd3d8bc 100644 --- a/hive-agent/src/web_ui/actions.rs +++ b/hive-agent/src/web_ui/actions.rs @@ -69,7 +69,7 @@ pub(super) async fn post_cancel_turn(State(state): State) -> Response .emit(crate::events::LiveEvent::Note { text: note.into() }); return (axum::http::StatusCode::OK, "ok").into_response(); } - let out = super::sigint_claude().await; + let out = super::sigint_claude(); let note = match out { SigintOutcome::Signalled => { // A process actually got signalled — the *next* turn's wake diff --git a/hive-agent/src/web_ui/auth.rs b/hive-agent/src/web_ui/auth.rs index ea02fb3a..bbc664b3 100644 --- a/hive-agent/src/web_ui/auth.rs +++ b/hive-agent/src/web_ui/auth.rs @@ -84,7 +84,7 @@ pub(super) async fn post_login_cancel(State(state): State) -> Response /// preservation invariants. pub(super) async fn post_logout(State(state): State) -> Response { // Step 1: SIGINT claude (best-effort, matches `post_cancel_turn`). - let _ = super::sigint_claude().await; + let _ = super::sigint_claude(); // Step 2: delete OAuth credential files only — login::clear_session owns // the file set and preserves session-history files alongside them. let dir = crate::paths::claude_dir(); diff --git a/hive-agent/src/web_ui/mod.rs b/hive-agent/src/web_ui/mod.rs index 88d848e6..2b8a1c0b 100644 --- a/hive-agent/src/web_ui/mod.rs +++ b/hive-agent/src/web_ui/mod.rs @@ -340,21 +340,40 @@ fn find_claude_child() -> Option { /// (best-effort). See [`find_claude_child`] for how it's identified — /// **not** a name-based `pkill claude`, which would also hit an unrelated /// `claude` process someone is running interactively in the container. -async fn sigint_claude() -> SigintOutcome { +fn sigint_claude() -> SigintOutcome { let Some(pid) = find_claude_child() else { return SigintOutcome::NoProcess; }; - match tokio::process::Command::new("kill") - .args(["-INT", &pid.to_string()]) - .status() - .await - { - Ok(status) if status.success() => SigintOutcome::Signalled, - // Non-zero from `kill` means "no such process" (ESRCH) — it already - // exited between the check above and the signal. Same as never - // having found it. - Ok(_) => SigintOutcome::NoProcess, - Err(e) => SigintOutcome::Failed(e), + sigint_turn(pid) +} + +/// SIGINT `pid`, and its whole process group when it leads one, so the +/// commands a turn's claude started stop with it. `hive-claude` spawns the +/// turn's claude as the leader of its own group. A `claude` child that +/// leads none (`plugins.rs`'s `claude plugin` calls) shares the harness's +/// group, so it is signalled alone. +fn sigint_turn(pid: u32) -> SigintOutcome { + // A child is never pid 0 or 1, and `-0` / `-1` would reach the + // harness's own group or every process it may signal. + let Some(pid) = i32::try_from(pid).ok().filter(|&pid| pid > 1) else { + return SigintOutcome::NoProcess; + }; + // SAFETY: `getpgid(2)` only reads; a gone pid returns -1, never `pid`. + let target = if unsafe { libc::getpgid(pid) } == pid { + -pid + } else { + pid + }; + // SAFETY: plain `kill(2)` on the child or the group it leads. + if unsafe { libc::kill(target, libc::SIGINT) } == 0 { + return SigintOutcome::Signalled; + } + let err = std::io::Error::last_os_error(); + if err.raw_os_error() == Some(libc::ESRCH) { + // It exited between the scan and the signal: same as never found. + SigintOutcome::NoProcess + } else { + SigintOutcome::Failed(err) } } @@ -419,3 +438,105 @@ fn broker_error_response(err: &BrokerError, action: &str) -> Response { ), } } + +#[cfg(test)] +mod tests { + use std::os::unix::process::CommandExt as _; + use std::path::Path; + use std::process::{Command, Stdio}; + use std::time::Duration; + + use super::{SigintOutcome, sigint_turn}; + + /// Poll `path` until it holds a complete, newline-terminated pid. + 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"); + } + std::thread::sleep(Duration::from_millis(20)); + } + 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; + } + !std::fs::read_to_string(format!("/proc/{pid}/stat")).is_ok_and(|stat| { + stat.rsplit_once(") ") + .is_some_and(|(_, rest)| rest.starts_with('Z')) + }) + } + + #[test] + fn a_cancel_stops_the_commands_a_turn_started() { + let dir = tempfile::tempdir().expect("tempdir"); + let pidfile = dir.path().join("grandchild.pid"); + // A turn's claude as hive-claude spawns it: leading its own group, + // with a command of its own running in the foreground (a + // non-interactive shell starts `&` jobs with SIGINT ignored). The + // trailing `exit` keeps `sh -c` from exec'ing into that command. + let mut turn = Command::new("/bin/sh") + .args([ + "-c", + r#"sh -c 'echo $$ >"$1"; exec sleep 300' sh "$1"; exit"#, + "sh", + ]) + .arg(&pidfile) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .process_group(0) + .spawn() + .expect("spawn the stand-in turn"); + let grandchild = read_pid(&pidfile); + assert_ne!( + i32::try_from(turn.id()).ok(), + Some(grandchild), + "the command must be a child of the turn, not the turn itself" + ); + + assert!(matches!(sigint_turn(turn.id()), SigintOutcome::Signalled)); + + let mut alive = is_running(grandchild); + for _ in 0..250 { + if !alive { + break; + } + std::thread::sleep(Duration::from_millis(20)); + alive = is_running(grandchild); + } + if alive { + // SAFETY: `kill(2)` on a pid this test started; keeps a failed + // run from leaving a 300s `sleep` behind. + unsafe { + libc::kill(grandchild, libc::SIGKILL); + } + } + let _ = turn.wait(); + assert!(!alive, "grandchild {grandchild} outlived the cancel"); + } + + #[test] + fn a_claude_sharing_the_harness_group_is_signalled_alone() { + // In this test process's own group, as `claude plugin` runs. A group + // signal here would SIGINT the test binary itself. + let mut child = Command::new("sleep") + .arg("300") + .spawn() + .expect("spawn sleep"); + + assert!(matches!(sigint_turn(child.id()), SigintOutcome::Signalled)); + + let status = child.wait().expect("reap sleep"); + assert_eq!( + std::os::unix::process::ExitStatusExt::signal(&status), + Some(libc::SIGINT) + ); + } +} diff --git a/hive-subagent-mcp/src/session.rs b/hive-subagent-mcp/src/session.rs index 75031cd1..8411e45a 100644 --- a/hive-subagent-mcp/src/session.rs +++ b/hive-subagent-mcp/src/session.rs @@ -3731,6 +3731,100 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + /// A stand-in for claude with a command of its own still running: an + /// inner `sh` writes its pid to `pidfile` and execs `sleep`. It runs in + /// the foreground because a non-interactive shell starts `&` jobs with + /// SIGINT ignored, and its output goes to /dev/null so it cannot hold the + /// driver's pipes open. + fn busy_claude(dir: &Path, pidfile: &Path) -> PathBuf { + let script = dir.join("busy-claude"); + write_executable( + &script, + &format!( + "#!/bin/sh\nsh -c 'echo $$ >\"$1\"; exec sleep 300' sh '{}' >/dev/null 2>&1\n", + pidfile.display() + ), + ); + script + } + + /// 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; + } + !std::fs::read_to_string(format!("/proc/{pid}/stat")).is_ok_and(|stat| { + stat.rsplit_once(") ") + .is_some_and(|(_, rest)| rest.starts_with('Z')) + }) + } + + #[tokio::test] + async fn an_interrupt_stops_the_commands_the_subagent_started() { + for force in [false, true] { + let dir = std::env::temp_dir().join(format!( + "hive-subagent-group-{}-{:?}-{force}", + std::process::id(), + std::thread::current().id() + )); + std::fs::create_dir_all(&dir).expect("scratch dir"); + let pidfile = dir.join("grandchild.pid"); + let state = Arc::new(mid_run("n", "carry on indefinitely", 5)); + let config = Config { + program: Some(busy_claude(&dir, &pidfile)), + ..Config::default() + }; + spawn_and_track( + &state, + "n", + &config, + &Attach::Create("n".to_owned()), + "turn one".to_owned(), + None, + ) + .expect("the fake claude spawns"); + let grandchild = read_pid(&pidfile).await; + + interrupt(&state, "n", force).expect("a running turn is interruptible"); + + let mut alive = is_running(grandchild); + for _ in 0..250 { + if !alive { + break; + } + tokio::time::sleep(Duration::from_millis(20)).await; + alive = is_running(grandchild); + } + if alive { + // SAFETY: `kill(2)` on a pid this test started; keeps a failed + // run from leaving a 300s `sleep` behind. + unsafe { + libc::kill(grandchild, libc::SIGKILL); + } + } + std::fs::remove_dir_all(&dir).ok(); + assert!( + !alive, + "grandchild {grandchild} outlived interrupt(force = {force})" + ); + } + } + #[test] fn a_cancelled_run_stops_with_turns_still_on_its_clock() { // The loop half of the same fact, asserted directly: `Cancelled` is