diff --git a/CHANGELOG.md b/CHANGELOG.md index 9314957..0b84770 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/CLAUDE.md b/CLAUDE.md index c0a30c2..9bdf1af 100644 --- a/CLAUDE.md +++ b/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 diff --git a/Cargo.toml b/Cargo.toml index 27880f7..d1bfce2 100644 --- a/Cargo.toml +++ b/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] diff --git a/docs/concepts/tools.md b/docs/concepts/tools.md index b0a9ac3..e9b781e 100644 --- a/docs/concepts/tools.md +++ b/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 '' panicked: …`) that the model sees, and the run continues. diff --git a/examples/cli.rs b/examples/cli.rs index 2733900..9363b14 100644 --- a/examples/cli.rs +++ b/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!(); diff --git a/src/tools/bash.rs b/src/tools/bash.rs index 757eff8..a75e09b 100644 --- a/src/tools/bash.rs +++ b/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, +} + +impl GroupKill { + fn new(pid: Option) -> 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)] diff --git a/tests/tools_test.rs b/tests/tools_test.rs index dd3918e..a9e5b74 100644 --- a/tests/tools_test.rs +++ b/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(); +}