fix(F6): bash timeout kills whole process group, not just direct child - #81
Merged
Conversation
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.
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
dirge-code#81) Track F-HIGH dirge-code#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Track F-HIGH #6. Previous
tokio::time::timeout(d, cmd.output())left bash's subprocess tree orphaned on timeout. Newrun_with_timeouthelper spawns in a new process group on Unix and sendskillpg(-pid, SIGKILL)on timeout. 2 new Unix-only tests, 660 pass.