fix(bash): kill the command's whole process group on timeout, cancel and drop

CHANGELOG.md

@@ -6,6 +6,10 @@ adheres to [Semantic Versioning](https://semver.org/).
 
 ## Unreleased
 
+### Fixed
+
+- **`BashTool` kills what a command started** (#277, suggested by @shahidcodes). On Unix, `bash` now leads its own process group, and a timeout, a cancel or dropping the call kills the whole group: pipeline stages, `&&` lists and background jobs no longer keep running. Only a process that starts its own session (`setsid`) escapes. A command that finishes on its own leaves a background job alone when the job's output is redirected (`cmd >log 2>&1 &`); one still writing to the tool's output keeps the call open until the timeout, which now kills it. **Behaviour change:** the command is no longer in the terminal's foreground group, so the terminal's Ctrl+C does not reach it — cancel or drop the call on Ctrl+C (the `cli` example now aborts the run), and a command prompting on `/dev/tty` (`sudo`, SSH) stops until the timeout. Windows is unchanged (the `bash` process only). Adds `libc` as a Unix-only dependency (already in the tree through tokio). Tests: timeout, cancel, drop and normal completion in `tests/tools_test.rs`, mutation-checked.
+
 ### Tests
 
 - **HTTP MCP on wasm32.** `tests/wasm32.rs` runs an agent that connects to an HTTP MCP server, discovers its tool and calls it from the loop, through the host's `fetch` (a scripted server replaces the global `fetch`): handshake, `Mcp-Session-Id` replay and the tool result. The Workers guide notes that a stalled MCP stream has no idle read timeout of its own on wasm32, and that custom headers are not supported yet (#275).

CLAUDE.md

@@ -240,7 +240,7 @@ Tests that mutate the global layers or assert on logs live in their own serializ
   - Sandboxed tools do their I/O on `PathSandbox::io_path` (the checked path); unsandboxed tools use the caller's spelling.
   - `search` passes `--regexp=` / `-e`, then `--`, then the path. It has no per-file `--max-count`; it reads lines until `max_results` and kills the search at the next one (`details.truncated`). An exit 2 (some file errored) with matches returns the matches plus stderr under `Warnings:` (`details.warnings` = that text, capped, or null; kept on the early-stop path too); only an error with no matches fails.
   - `list_files` guards `find` with `not_an_option`. `find`'s stderr is read: errors (an unreadable subdirectory) are appended to the listing under `Warnings` and in `details.warnings` (capped), so a partial listing never reads as complete; only an error with no files fails (#260).
-  - `bash` spawns with `kill_on_drop`, which kills the `bash` process only, and with `stdin(Stdio::null())` (`spawn` inherits stdin, while `output()` did not; `search` sets it too). Each stream goes through `Capture` (bytes kept ≤ `max_output_bytes`, the rest drained; a cut flag and a read error survive a timeout), and output is decoded after cutting.
+  - `bash` spawns with `kill_on_drop` and, on Unix, `process_group(0)`: a `GroupKill` guard (declared after the child, so it fires before the child is reaped and the pgid cannot be reused) `killpg`s the whole group on timeout, cancel or drop, and is disarmed when the command finishes on its own (a background job survives only if its output is redirected — one holding the pipes keeps the call open until the timeout kills it); std's `CommandExt::process_group` (tokio's needs 1.40); the command is outside the terminal's foreground group, so hosts cancel on Ctrl+C (`examples/cli.rs` aborts the run); `libc` is a `cfg(unix)` dependency, the crate's only `unsafe`. Also `stdin(Stdio::null())` (`spawn` inherits stdin, while `output()` did not; `search` sets it too). Each stream goes through `Capture` (bytes kept ≤ `max_output_bytes`, the rest drained; a cut flag and a read error survive a timeout), and output is decoded after cutting.
   - Tool `execute` is built and awaited inside `catch_unwind` in `execute_single_tool`, the same as middleware. A panic is an error result carrying the payload text, logged at `error!` with the `tool_call_id`.
   - The Gemini key goes in `x-goog-api-key`, trimmed, never the URL. It is skipped when the user's headers set `x-goog-api-key`, or when the key is empty. An `Authorization` header doesn't count.
 - Retry logic (`retry.rs`) uses exponential backoff with ±20% jitter; only retries `RateLimited` and `Network` errors (`classify` maps 429, 503 and 529 to `RateLimited` and `warn!`s the body, since `RateLimited` carries no message; `RATE_LIMIT_CODES` includes Anthropic's `overloaded_error` and Google's `unavailable`, and an in-stream numeric `code` of 503/529 counts — 429 / `RESOURCE_EXHAUSTED` in-stream stays `Api` to keep its message; other 5xx stay `Api`). In `stream_assistant_response` each attempt's forwarder is **drained, never aborted** (what consumers see must not depend on the scheduler), an attempt that sent `MessageStart` without `MessageEnd` is closed with `StopReason::Error` (`AttemptEvents::needs_end`), and a retried attempt is followed by `AgentEvent::ProviderRetry` — consumers that treat an error `MessageEnd` as failure look at the next event first (GASP holds it back; `release_smoke` / `long_horizon` clear it; `retry::RetrySafeEvents` / `retry_safe_events(rx)` is the consumer-side filter for append-only sinks: a private `Held` enum (Nothing / Open / Failed) holds an assistant attempt until its `MessageEnd`, drops a retried one down to its `ProviderRetry`, releases a final failure as start + `Error`/`Aborted` end without deltas, and drops (never releases, `warn!`s) an attempt left open by a retry, a second start or the stream's end; other events overtake a held attempt; a sub-agent's `ToolExecutionUpdate` text is not covered; the documented terminal pattern (abort on a `ProviderRetry` after streamed text) is tested in `tests/retry_safe_test.rs`). An `Ok` attempt that left its message open (a provider that never sent `Done`) is closed with the returned message; a failed forwarder `JoinError` is `warn!`ed. The backoff sleep races `cancel`; a cancellation ends the turn as `StopReason::Aborted` (not `Error`, so `on_error` is not called). A cancel caught at the top of a turn (while tools ran, or between turns) appends `CANCELLED_MARKER` (`[Agent stopped: cancelled]`, via `push_stop_marker`) when the run produced an assistant message; `sub_agent::extract_error` treats it, like a loop abort, as a failure (a limit marker is still partial success). `Batched { size: 0 }` runs as size 1. Providers must drop every `tx` clone before `stream` returns, or the drain waits

Cargo.toml

@@ -188,6 +188,11 @@ inherits = "test"
 debug = false
 opt-level = 1
 
+# `BashTool` kills a command's whole process group on timeout or cancel
+# (`killpg`). Already in the tree through tokio.
+[target.'cfg(unix)'.dependencies]
+libc = "0.2"
+
 # wasm32 (e.g. Cloudflare Workers): randomness from the JS host, and a
 # JS-backed executor/timer in place of a Tokio runtime.
 [target.'cfg(target_arch = "wasm32")'.dependencies]

docs/concepts/tools.md

@@ -196,7 +196,7 @@ async fn execute(&self, params: serde_json::Value, _ctx: ToolContext) -> Result<
 }
 ```
 
-**Exception: BashTool.** The built-in `BashTool` returns `Ok` even on non-zero exit codes, with both stdout and stderr in the result. This is intentional — the LLM needs to see the actual error output (compilation errors, test failures, etc.) to diagnose and fix issues. Only failures outside the command return `Err`: a matched deny pattern, a refused confirmation, a timeout (its message carries the output printed so far), cancellation, or `bash` failing to start. A command that does not exist is an ordinary non-zero exit (127). A timeout or cancel kills the `bash` process; what it started (pipeline stages, commands in a `&&` list, background jobs) can keep running.
+**Exception: BashTool.** The built-in `BashTool` returns `Ok` even on non-zero exit codes, with both stdout and stderr in the result. This is intentional — the LLM needs to see the actual error output (compilation errors, test failures, etc.) to diagnose and fix issues. Only failures outside the command return `Err`: a matched deny pattern, a refused confirmation, a timeout (its message carries the output printed so far), cancellation, or `bash` failing to start. A command that does not exist is an ordinary non-zero exit (127). On Unix, a timeout, a cancel or dropping the call kills the command's whole process group — pipeline stages, commands in a `&&` list and background jobs included; only a process that starts its own session (`setsid`) escapes. A command that finishes on its own leaves a background job alone if the job's output is redirected away from the tool (`npm run dev >dev.log 2>&1 &`); a job still writing to the tool's output keeps the call open until the timeout, which kills it. Because the command runs in its own process group, the terminal's Ctrl+C no longer reaches it: a host should cancel or drop the call on Ctrl+C (the CLI example aborts the run), and a command that prompts on `/dev/tty` (`sudo`, an SSH passphrase) stops until the timeout. On Windows only the `bash` process is killed.
 
 **A panicking tool** is contained by the loop, the same as a panicking middleware. The call ends as an error result (`tool '<name>' panicked: …`) that the model sees, and the run continues.
 

examples/cli.rs

@@ -151,11 +151,22 @@ async fn main() {
         .with_skills(skills.clone())
         .with_tools(default_tools());
 
-    // Graceful Ctrl+C exit
-    tokio::spawn(async {
-        tokio::signal::ctrl_c().await.ok();
-        println!("\n{DIM}  bye 👋{RESET}\n");
-        std::process::exit(0);
+    // Ctrl+C at the prompt exits. During a run it stops the run instead
+    // (the event loop below aborts it): commands `bash` started are in their
+    // own process group, so the terminal's Ctrl+C does not reach them, and
+    // exiting here would leave them running.
+    let running = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false));
+    tokio::spawn({
+        let running = running.clone();
+        async move {
+            loop {
+                tokio::signal::ctrl_c().await.ok();
+                if !running.load(std::sync::atomic::Ordering::SeqCst) {
+                    println!("\n{DIM}  bye 👋{RESET}\n");
+                    std::process::exit(0);
+                }
+            }
+        }
     });
 
     print_banner();
@@ -210,8 +221,21 @@ async fn main() {
         let mut rx = agent.prompt(input).await;
         let mut session_stats = SessionStats::default();
         let mut in_text = false;
+        running.store(true, std::sync::atomic::Ordering::SeqCst);
 
-        while let Some(event) = rx.recv().await {
+        loop {
+            let event = tokio::select! {
+                event = rx.recv() => match event {
+                    Some(event) => event,
+                    None => break,
+                },
+                _ = tokio::signal::ctrl_c() => {
+                    // Cancels the run, which kills a running command's whole
+                    // process group; the run then ends as aborted.
+                    agent.abort();
+                    continue;
+                }
+            };
             match event {
                 AgentEvent::ToolExecutionStart {
                     tool_name, args, ..
@@ -309,6 +333,7 @@ async fn main() {
                 _ => {}
             }
         }
+        running.store(false, std::sync::atomic::Ordering::SeqCst);
 
         if in_text {
             println!();

src/tools/bash.rs

@@ -161,7 +161,13 @@ impl AgentTool for BashTool {
             }
         }
 
-        let mut cmd = Command::new("bash");
+        #[cfg_attr(not(unix), allow(unused_mut))]
+        let mut cmd = std::process::Command::new("bash");
+        // std's `process_group` (Rust 1.64): tokio's own needs tokio 1.40,
+        // newer than the `tokio = "1"` this crate asks for.
+        #[cfg(unix)]
+        std::os::unix::process::CommandExt::process_group(&mut cmd, 0);
+        let mut cmd = Command::from(cmd);
         cmd.arg("-c").arg(command);
 
         // Keep credentials out of model-authored commands when configured.
@@ -187,11 +193,12 @@ impl AgentTool for BashTool {
         // Capture output
         cmd.stdout(std::process::Stdio::piped());
         cmd.stderr(std::process::Stdio::piped());
-        // A timeout or cancel must not leave the command running. This kills
-        // the `bash` process only: what it started — pipeline stages,
-        // commands in a `;` / `&&` list, background jobs — is not in that
-        // kill and can keep running. Run the agent in a container if that
-        // matters.
+        // A timeout or cancel must not leave the command running. On Unix
+        // `bash` leads its own process group and `GroupKill` kills the whole
+        // group: non-interactive bash has no job control, so pipeline stages,
+        // `;` / `&&` lists and background jobs stay in it. Only a process that
+        // starts its own session (`setsid`) escapes. Elsewhere this kills the
+        // `bash` process only.
         cmd.kill_on_drop(true);
 
         let timeout = self.timeout;
@@ -203,6 +210,9 @@ impl AgentTool for BashTool {
         let mut child = cmd
             .spawn()
             .map_err(|e| ToolError::Failed(format!("Failed to execute: {}", e)))?;
+        // Declared after `child`, so it is dropped first: the group is killed
+        // while `bash` is not yet reaped, and its id cannot have been reused.
+        let mut group = GroupKill::new(child.id());
         let (Some(child_out), Some(child_err)) = (child.stdout.take(), child.stderr.take()) else {
             return Err(ToolError::Failed("Failed to capture output".into()));
         };
@@ -236,6 +246,9 @@ impl AgentTool for BashTool {
                 )));
             }
             Some(Ok(status)) => {
+                // Finished on its own: what it left running in the background
+                // (a server it started, say) is the command's business.
+                group.disarm();
                 status.map_err(|e| ToolError::Failed(format!("Failed to execute: {}", e)))?
             }
         };
@@ -250,6 +263,44 @@ impl AgentTool for BashTool {
     }
 }
 
+/// Kills the command's process group when dropped — on a timeout, a cancel,
+/// or the tool's future being dropped — unless the command finished first.
+struct GroupKill {
+    #[cfg(unix)]
+    pgid: Option<libc::pid_t>,
+}
+
+impl GroupKill {
+    fn new(pid: Option<u32>) -> Self {
+        #[cfg(not(unix))]
+        let _ = pid;
+        Self {
+            #[cfg(unix)]
+            pgid: pid.and_then(|p| libc::pid_t::try_from(p).ok()),
+        }
+    }
+
+    fn disarm(&mut self) {
+        #[cfg(unix)]
+        {
+            self.pgid = None;
+        }
+    }
+}
+
+impl Drop for GroupKill {
+    fn drop(&mut self) {
+        #[cfg(unix)]
+        if let Some(pgid) = self.pgid {
+            // SAFETY: `killpg` takes two integers and touches no memory of
+            // ours; a failure (the group already gone) is fine to ignore.
+            unsafe {
+                libc::killpg(pgid, libc::SIGKILL);
+            }
+        }
+    }
+}
+
 /// One output stream: the bytes kept, whether more were discarded, and a
 /// read error if the stream ended on one.
 #[derive(Default)]

tests/tools_test.rs

@@ -1109,3 +1109,151 @@ async fn bash_env_allowlist_hides_other_variables() {
         std::env::remove_var("YOAGENT_TEST_KEEP");
     }
 }
+
+// ---------------------------------------------------------------------------
+// BashTool: what a command started goes with it (#277)
+// ---------------------------------------------------------------------------
+
+/// Whether `pid` still runs. A killed process nobody has reaped yet (no init
+/// in a container, say) is a zombie: `kill -0` still succeeds on it, so a
+/// `Z` state counts as gone.
+#[cfg(unix)]
+fn alive(pid: &str) -> bool {
+    let exists = std::process::Command::new("kill")
+        .args(["-0", pid])
+        .stderr(std::process::Stdio::null())
+        .status()
+        .is_ok_and(|s| s.success());
+    let zombie = std::process::Command::new("ps")
+        .args(["-o", "stat=", "-p", pid])
+        .output()
+        .is_ok_and(|out| {
+            String::from_utf8_lossy(&out.stdout)
+                .trim_start()
+                .starts_with('Z')
+        });
+    exists && !zombie
+}
+
+/// Waits until the command has written its background job's pid.
+#[cfg(unix)]
+async fn background_pid(file: &std::path::Path) -> String {
+    for _ in 0..200 {
+        if let Ok(pid) = std::fs::read_to_string(file) {
+            if !pid.trim().is_empty() {
+                return pid.trim().to_owned();
+            }
+        }
+        tokio::time::sleep(std::time::Duration::from_millis(10)).await;
+    }
+    panic!("the command never wrote its background pid");
+}
+
+/// A SIGKILL is delivered asynchronously and the orphan is reaped by init.
+#[cfg(unix)]
+async fn assert_gone(pid: &str) {
+    for _ in 0..200 {
+        if !alive(pid) {
+            return;
+        }
+        tokio::time::sleep(std::time::Duration::from_millis(10)).await;
+    }
+    panic!("background job {pid} outlived the command");
+}
+
+#[cfg(unix)]
+fn backgrounding_command(pid_file: &std::path::Path) -> serde_json::Value {
+    serde_json::json!({
+        "command": format!("sleep 30 & echo $! > {}; sleep 30 | cat", pid_file.display())
+    })
+}
+
+#[cfg(unix)]
+#[tokio::test]
+async fn bash_timeout_kills_background_jobs_and_pipeline_stages() {
+    let dir = tempfile::tempdir().unwrap();
+    let pid_file = dir.path().join("pid");
+    // Long enough that `bash` is up and has written the pid even when other
+    // tests load the machine; the pid is read before the timeout fires.
+    let run = tokio::spawn({
+        let args = backgrounding_command(&pid_file);
+        async move {
+            BashTool::new()
+                .with_timeout(std::time::Duration::from_secs(3))
+                .execute(args, ctx("bash"))
+                .await
+        }
+    });
+    let pid = background_pid(&pid_file).await;
+    assert!(alive(&pid));
+    assert!(run
+        .await
+        .unwrap()
+        .unwrap_err()
+        .to_string()
+        .contains("timed out"));
+    assert_gone(&pid).await;
+}
+
+#[cfg(unix)]
+#[tokio::test]
+async fn bash_cancel_kills_background_jobs() {
+    let dir = tempfile::tempdir().unwrap();
+    let pid_file = dir.path().join("pid");
+    let cancel = CancellationToken::new();
+    let run = tokio::spawn({
+        let (cancel, args) = (cancel.clone(), backgrounding_command(&pid_file));
+        async move {
+            BashTool::new()
+                .execute(args, ctx_with_cancel("bash", cancel))
+                .await
+        }
+    });
+    let pid = background_pid(&pid_file).await;
+    assert!(alive(&pid));
+    cancel.cancel();
+    assert!(matches!(run.await.unwrap(), Err(ToolError::Cancelled)));
+    assert_gone(&pid).await;
+}
+
+#[cfg(unix)]
+#[tokio::test]
+async fn dropping_the_bash_call_kills_background_jobs() {
+    let dir = tempfile::tempdir().unwrap();
+    let pid_file = dir.path().join("pid");
+    let run = tokio::spawn({
+        let args = backgrounding_command(&pid_file);
+        async move { BashTool::new().execute(args, ctx("bash")).await }
+    });
+    let pid = background_pid(&pid_file).await;
+    assert!(alive(&pid));
+    // The caller gives up: the call's future is dropped mid-run.
+    run.abort();
+    assert!(run.await.unwrap_err().is_cancelled());
+    assert_gone(&pid).await;
+}
+
+/// A command that finishes on its own may leave a job running on purpose
+/// (a server it started): only a timeout, cancel or drop kills the group.
+#[cfg(unix)]
+#[tokio::test]
+async fn a_finished_command_leaves_its_detached_background_job_alone() {
+    let dir = tempfile::tempdir().unwrap();
+    let pid_file = dir.path().join("pid");
+    let result = BashTool::new()
+        .execute(
+            serde_json::json!({
+                "command": format!("sleep 30 >/dev/null 2>&1 & echo $! > {}", pid_file.display())
+            }),
+            ctx("bash"),
+        )
+        .await
+        .unwrap();
+    assert_eq!(result.details["exit_code"], 0);
+    let pid = background_pid(&pid_file).await;
+    assert!(alive(&pid), "the job outlives a command that finished");
+    std::process::Command::new("kill")
+        .arg(&pid)
+        .status()
+        .unwrap();
+}