Skip to content

fix(tools): kill process group when bash task aborts - #41

Merged
Blankeos merged 1 commit into
Blankeos:mainfrom
yan-ad:fix/kill-node-descendants-on-tool-abort
Aug 31, 2026
Merged

fix(tools): kill process group when bash task aborts#41
Blankeos merged 1 commit into
Blankeos:mainfrom
yan-ad:fix/kill-node-descendants-on-tool-abort

Conversation

@yan-ad

@yan-ad yan-ad commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add an RAII guard for foreground bash process groups
  • kill all descendants when a tool future is aborted or dropped
  • add a regression test covering a long-running Node descendant

Root cause

Command::kill_on_drop only terminates the spawned shell. When the tool future was aborted, Node descendants could survive with an active event loop and continue consuming memory.

Validation

  • cargo fmt --all
  • cargo test tools::bash::tests
  • cargo check
  • git diff --check

@Blankeos

Copy link
Copy Markdown
Owner

PR Review

fix/kill-node-descendants-on-tool-abortmain

PR: #41

Merge confidence: 4.5/5


Suggested GitHub PR body

Foreground bash already killpgs when the shell exits, but Command::kill_on_drop only SIGKILLs the spawned shell. User-cancel drops the tool future (select! on cancel_token in aisdk/response.rs, or StreamTextResponse aborting its join handle) without running terminate_child, so Node/setInterval descendants in the same process group survived and kept an event loop.

RAII ProcessGroupGuard killpg(SIGKILL)s on Drop (and still take()s the pid on the happy-path shell-exit kill). Regression test aborts an in-flight node -e 'setInterval(...)' and asserts the descendant is gone.


What changed

File Role
src/tools/bash.rs ProcessGroupGuard in execute_foreground only; Drop → kill_process_group; explicit .kill() still runs when the shell exits so pipes EOF; unix test aborted_tool_future_kills_node_descendants

1 commit: 311782ffix(tools): kill process group when bash task aborts

+86 / −2. Background + interactive paths untouched.


Checks run (read-only)

Check Result
gh pr view 41 OPEN, MERGEABLE, CLEAN, not draft
origin/main ancestor of HEAD yes (54afa0c)
git diff --check origin/main...HEAD clean
cargo fmt --all -- --check clean
cargo check --bin crabcode ok
cargo test --bin crabcode tools::bash::tests 6/6 including new abort test (0.08s; node v24 present so it did not skip)
GitHub checks plan pass; Release build jobs skipped (no test workflow on PRs)

Regressions?

No functional regression on the covered path. Foreground success still kills the group when the shell exits (successful_command_kills_background_processes still passes). Abort/drop now matches that cleanup.

Intentional behavior: cancelling a foreground bash now SIGKILLs the whole process group, including & children (node, sleep, etc.). That is already true on successful shell exit; abort was the hole. Long-running servers belong in mode=background (ledger jobs) — unchanged.

Remaining risks (not blockers):

  1. Test skips if node is missing (return; with no #[ignore]). Local run had node so it executed. CI here does not run this test job anyway.
  2. Windows still a no-op (kill_process_group empty; no process_group). Pre-existing; test is #[cfg(unix)].
  3. terminate_child does not take() the guard pid. Cancel-via-ctx.is_aborted() killpg's, then Drop killpg's again. Harmless unless the pgid is reused in that window (vanishingly unlikely).
  4. No pid == 0 guard (jobs/spawn.rs has one). child.id() at spawn is never 0. Pre-existing.

Migrations?

No. Prefs/history SQLite (src/persistence/migrations.rs) and auth.json / MCP creds are untouched.


Merge checklist

  • Open GitHub PR (#41)
  • Scoped to foreground bash only (background/interactive not wrapped in the guard)
  • cargo fmt --all -- --check
  • cargo check
  • cargo test tools::bash::tests — 6/6, new test actually ran (node present)
  • git diff --check
  • Optional: fail or sleep-fallback instead of silent skip when node is missing
  • Optional: terminate_child should process_group_guard.kill() so Drop is a no-op
  • Optional: pid == 0 early-return in kill_process_group (match jobs/spawn.rs)

Confidence bumps

If you do this New score
Don't silently skip the abort test (sleep+wait fallback, or #[ignore] + require-node in CI) 4.7/5
+ terminate_child disarms the guard via .kill() / .take() 4.9/5
+ pid == 0 guard in kill_process_group 5/5

The leak itself is covered: Drop of the foreground future now killpgs, which is exactly how production cancel drops tool_work.

@Blankeos
Blankeos merged commit 65a7bf6 into Blankeos:main Aug 31, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants