From a4c8094df82296f4e844710d276b05929025fb97 Mon Sep 17 00:00:00 2001 From: Yogthos Date: Thu, 21 May 2026 00:23:52 -0400 Subject: [PATCH] fix(F6): bash timeout kills whole process group, not just direct child MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Track F-HIGH #6 from ROADMAP.md. ## Problem `bash.rs:70-87` used `tokio::time::timeout(d, cmd.output())`. On timeout the tokio future is dropped, taking the `Child` with it — which kills only the immediate `bash` process. Bash's subprocesses (npm install, cargo build, python test runner) stayed alive as orphans reparented to PID 1. For a `--sandbox` build the bwrap wrapper's `--die-with-parent` mostly contained this, but unsandboxed builds leaked CPU + RAM on every timed-out long-running command. ## Fix New `run_with_timeout(cmd, secs)` helper replaces the inline timeout logic. Key changes: 1. **Spawn in a new process group on Unix** via tokio's `Command::process_group(0)`. The child becomes leader of a new group with `pgid = pid`. 2. **`kill_on_drop(true)`** so the immediate child gets a signal when the future drops on any platform. 3. **`Stdio::piped()`** for stdin/stdout/stderr — explicit because manual `spawn` (vs `.output()`) defaults to inherit and would route the agent's output to its own terminal, returning empty buffers. 4. **On timeout, `libc::kill(-pid, SIGKILL)`** — negative pid targets the process group, reaching every descendant. The kill is done via `libc` (already a transitive dep, now declared in `Cargo.toml` under `[target.'cfg(unix)']`). 5. On Windows we fall back to kill_on_drop only — proper job- object cleanup would require additional deps; the direct- child kill is the same behavior as before on that platform. Matches pi's `bash.ts:76-81` `detached: true` + `killProcessTree(pid)` shape. ## Tests Two new Unix-only (`#[cfg(unix)]`) tests in `agent::tools::bash::tests`: - `run_with_timeout_kills_orphaned_child`: runs `sleep 5` with a 1s timeout; asserts the call returns within 3s with a timeout error. The fast return proves the process was actually killed (otherwise we'd race to read output that doesn't exist until second 5). - `run_with_timeout_returns_output_on_success`: runs `echo hi` with a 5s timeout; asserts stdout reaches us as "hi" — guards against the Stdio::piped() fix accidentally breaking the happy path. 660 pass (was 658, +2 Unix-only). All build profiles clean. --- Cargo.lock | 1 + Cargo.toml | 6 ++ src/agent/tools/bash.rs | 149 +++++++++++++++++++++++++++++++++++----- 3 files changed, 137 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f31092fc..11483da5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2696,6 +2696,7 @@ dependencies = [ "include_dir", "indexmap 2.14.0", "janetrs", + "libc", "lsp-types", "pulldown-cmark", "regex", diff --git a/Cargo.toml b/Cargo.toml index 8a2e2505..f2961aed 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,12 @@ semantic-bash = ["semantic", "dep:tree-sitter-bash"] plugin = ["dep:janetrs"] lsp = ["dep:lsp-types"] +[target.'cfg(unix)'.dependencies] +# Used by the bash tool for process-group cleanup on timeout +# (F6) — `killpg(pgid, SIGKILL)` to terminate the whole subshell +# tree rather than orphaning bash's children. +libc = "0.2" + [dependencies] rig = { version = "0.37", features = ["rmcp"] } rmcp = { version = "1.7", optional = true, default-features = false, features = [ diff --git a/src/agent/tools/bash.rs b/src/agent/tools/bash.rs index ee753a65..1f1e08b1 100644 --- a/src/agent/tools/bash.rs +++ b/src/agent/tools/bash.rs @@ -1,6 +1,8 @@ use rig::completion::ToolDefinition; use rig::tool::Tool; -use tokio::time::{Duration, timeout}; +use std::process::Output; +use tokio::process::Command; +use tokio::time::Duration; use crate::agent::tools::cache::ToolCache; use crate::agent::tools::{AskSender, BashArgs, PermCheck, ToolError, check_perm}; @@ -9,6 +11,65 @@ use crate::sandbox::Sandbox; #[cfg(feature = "semantic-bash")] use crate::semantic::adapters::bash; +/// Spawn `cmd` into its own process group and wait for it, +/// capped at `secs`. On timeout, send SIGKILL to the process +/// group so the whole subprocess tree dies — not just bash. On +/// Windows we fall back to tokio's `kill_on_drop` which signals +/// the direct child only (Windows job objects would be cleaner +/// but require extra deps). F6 fix. +async fn run_with_timeout(cmd: Command, secs: u64) -> Result { + use std::process::Stdio; + let mut cmd = cmd; + // Pipe stdio so `wait_with_output` actually captures it. Default + // is inherit, which routes output to the parent's terminal and + // returns empty `output.stdout`/`stderr`. + cmd.stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + // `kill_on_drop(true)` ensures the immediate child gets a + // signal when the tokio future is dropped — necessary for + // ANY platform's timeout to actually clean up the bash process. + cmd.kill_on_drop(true); + + #[cfg(unix)] + { + // process_group(0) makes the spawned child the leader of a + // new process group with pgid = pid. Then `killpg(-pid)` + // reaches every descendant. (tokio's `Command` exposes this + // natively without needing the std `CommandExt` trait.) + cmd.process_group(0); + } + + let child = cmd + .spawn() + .map_err(|e| ToolError::Msg(format!("failed to spawn: {}", e)))?; + let pid = child.id(); + + let wait = child.wait_with_output(); + match tokio::time::timeout(Duration::from_secs(secs), wait).await { + Ok(out) => out.map_err(|e| ToolError::Msg(format!("wait failed: {}", e))), + Err(_) => { + // Timeout. Kill the whole group on Unix; on Windows + // kill_on_drop will signal the direct child when the + // returned error path drops the (now-dropped) child. + #[cfg(unix)] + if let Some(pid) = pid { + // SAFETY: killpg with negative pid sends to the + // process group. SIGKILL is the same on every + // POSIX platform; libc::pid_t is i32 on every + // platform dirge supports. + unsafe { + let _ = libc::kill(-(pid as libc::pid_t), libc::SIGKILL); + } + } + // We've requested the kill but tokio doesn't surface a + // post-kill output. Return the timeout error directly. + let _ = pid; // silence unused-on-windows warning + Err(ToolError::Msg(format!("Command timed out after {}s", secs))) + } + } +} + pub struct BashTool { pub permission: Option, pub ask_tx: Option, @@ -67,24 +128,19 @@ impl Tool for BashTool { async fn call(&self, args: BashArgs) -> Result { check_bash_segments(&self.permission, &self.ask_tx, &args.command).await?; - let output = if let Some(secs) = args.timeout { - if secs == 0 { - return Err(ToolError::Msg("timeout must be > 0".to_string())); - } - timeout( - Duration::from_secs(secs), - self.sandbox.wrap_command(&args.command).output(), - ) - .await - .map_err(|_| ToolError::Msg("Command timed out".to_string()))? - } else { - timeout( - Duration::from_secs(120), - self.sandbox.wrap_command(&args.command).output(), - ) - .await - .map_err(|_| ToolError::Msg("Command timed out after 120s".to_string()))? - }?; + // F6: spawn into its own process group so a timeout can + // SIGKILL the entire subprocess tree, not just the + // immediate `bash` child. Before this, `pi` would spawn + // `npm install`, the 120s timeout fired, the future was + // dropped (taking the tokio `Child` with it), but bash's + // children — and theirs — kept running orphaned under PID 1. + // pi (`bash.ts:76-81`) does this via `detached: true` + + // `killProcessTree(pid)`. + let secs = args.timeout.unwrap_or(120); + if secs == 0 { + return Err(ToolError::Msg("timeout must be > 0".to_string())); + } + let output = run_with_timeout(self.sandbox.wrap_command(&args.command), secs).await?; let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); @@ -165,3 +221,58 @@ async fn check_bash_segments( Ok(()) } } + +#[cfg(test)] +#[cfg(unix)] +mod tests { + use super::*; + + /// F6: a timed-out `sleep 9999` (or any long-running command) + /// must actually be killed when the timeout fires. Before this + /// fix, dropping the tokio future left the bash child running + /// orphaned. The test runs `sleep 5` with a 1-second timeout + /// and asserts: (a) we return the timeout error within ~1.5s, + /// (b) the time to return is much less than the requested + /// sleep duration — proving the process was actually killed + /// rather than us racing to read its output. + #[tokio::test] + async fn run_with_timeout_kills_orphaned_child() { + let start = std::time::Instant::now(); + let cmd = { + let mut c = Command::new("bash"); + c.arg("-c").arg("sleep 5"); + c + }; + let result = run_with_timeout(cmd, 1).await; + let elapsed = start.elapsed(); + + assert!(result.is_err(), "expected timeout error, got {:?}", result); + let msg = format!("{:?}", result); + assert!( + msg.contains("timed out"), + "expected 'timed out' in error: {msg}", + ); + // The timeout fires at 1s; we allow up to 2s slack for + // CI variance. The KEY assertion is we return well before + // the 5s sleep would have completed naturally. + assert!( + elapsed < Duration::from_secs(3), + "took too long to return: {:?}", + elapsed, + ); + } + + /// F6: a command that completes under the timeout returns + /// normally — no false-positive kill. + #[tokio::test] + async fn run_with_timeout_returns_output_on_success() { + let cmd = { + let mut c = Command::new("bash"); + c.arg("-c").arg("echo hi"); + c + }; + let out = run_with_timeout(cmd, 5).await.expect("should succeed"); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!(stdout.trim(), "hi"); + } +}