fix(shell): kill + reap the child process group on timeout (AB-006) - #305
Merged
Conversation
WHAT: ShellTool's safe-subset engine detached the blocking worker on timeout and killed NOTHING — the child (and its descendants) ran to completion; the caller saw "timed_out" while the system had only stopped waiting. Now each pipeline stage is spawned as its own process-group leader (`process_group(0)`), and `run_pipeline` enforces the deadline itself: it polls the stages with `try_wait` and, on the deadline, SIGKILLs each stage's process GROUP (catching grandchildren) and reaps them. `Captured`/`SpawnCfg` carry the confirmed-timeout flag + the deadline; `timed_out` is derived from a real termination. The outer tokio timeout now only bounds when `invoke` returns — even if it fires and detaches the worker, the worker keeps running `run_pipeline`, which reaches the same deadline and kills. Mirrors the Brush engine's existing `kill_worker_tree` supervisor. `rustix` (already a dep for Brush) is made non-optional to provide the safe `kill_process_group` under the crate's `#![forbid(unsafe_code)]`. WHY: #269 / AB-006 — a fail-open resource/persistence defect. Regression E2E (non-privileged, real spawns): a `sleep 3; touch MARKER` child under a 1s timeout reports timed_out AND the marker is never written; a nested-`sh` variant proves the process-group kill reaches a grandchild. Before the fix the detached child wrote the marker. Co-Authored-By: Claude Opus 4.8 <[email protected]>
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
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.
Summary
Fixes #269 / AB-006. ShellTool's safe-subset engine, on timeout, detached the blocking worker and killed nothing — the child and its descendants ran to completion; the caller saw
timed_out: truewhile the system had merely stopped waiting (the post-sleep side effects still happened).Now the timeout is enforced where the children live:
process_group(0));run_pipelinepolls the stages withtry_waitto a deadline and, on the deadline, SIGKILLs each stage's process group (so grandchildren die too) and reaps them;Capturedcarries a confirmed-timeout flag; the envelope'stimed_outis derived from a real termination.The outer
tokiotimeout now only bounds wheninvokereturns — even if it fires and detaches the worker, that worker keeps runningrun_pipeline, which reaches the same deadline and does the kill+reap. This mirrors the Brush engine's existingkill_worker_treesupervisor.rustix(already a Brush dep) is made non-optional to provide the safekill_process_groupunder the crate's#![forbid(unsafe_code)].Test plan
Non-privileged real-spawn E2E regressions (validated locally):
real_timed_out_child_is_killed_and_never_writes_marker—sh -c 'sleep 3; touch M'under a 1s timeout →timed_out: trueand M never appears after the sleep window. Before the fix the detached child wrote it.real_timed_out_grandchild_is_killed— nestedsh -c 'sh -c "…"'proves the process-group kill reaches a grandchild.clippy -D warnings+fmtclean; full pre-push gate incl. bare-Brush[pre-push] OK.Scope
agent-bridle-tool-shell. The Brush engine already handled this; this brings the safe-subset engine to parity. Windows Job-Object teardown (vs the Unix process group) remains a follow-up — the#[cfg(unix)]group-kill is a no-op there and falls back tochild.kill().Fixes #269
🤖 Generated with Claude Code