fix(F6): bash timeout kills whole process group, not just direct child (#81)

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.

Co-authored-by: Yogthos <yogthos@gmail.com>

Author: yogthos

Cargo.lock

@@ -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 = [

Cargo.toml

@@ -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<Output, ToolError> {
+    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<PermCheck>,
     pub ask_tx: Option<AskSender>,
@@ -67,24 +128,19 @@ impl Tool for BashTool {
     async fn call(&self, args: BashArgs) -> Result<String, ToolError> {
         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");
+    }
+}