Repository navigation
Conversation
When a stateless LocalShellTool command timed out, background and double-forked jobs were not killed, and the call waited until they released stdout and stderr. The timeout now signals the whole process group, also after the shell has exited, keeps the descendant walk, and bounds the final output drain. A Linux test checks that both kinds of job are killed and that the call returns promptly.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Abandoned drains retain pipe descriptors, and zombie-only groups unnecessarily consume the full grace period.
2 open issues
What changed in this PR
Addresses #9482 by improving stateless shell timeout cleanup for background jobs.
Changes:
- Signals POSIX process groups even after the shell exits.
- Bounds output draining after timeout cleanup.
- Adds background-job regression tests and updates termination tests.
| File | Description |
|---|---|
python/packages/tools/tests/test_shell_killtree.py |
Tests process-group signalling after shell exit. |
python/packages/tools/tests/test_local_shell_tool.py |
Adds background-job timeout regressions. |
python/packages/tools/agent_framework_tools/shell/_killtree.py |
Adds group signalling and grace-period polling. |
python/packages/tools/agent_framework_tools/shell/_executor.py |
Limits post-timeout output draining. |
Review effort: Balanced
Abandoning the post-timeout drain left both pipe transports open while a detached child held them, so repeated timeouts could leak descriptors; the subprocess transport is now closed when the drain gives up. The process-group wait also treated a group of unreaped zombies as alive and spent the whole grace period on it; it now looks for a member that is not a zombie.
Eduard van Valkenburg (eavanvalkenburg)
approved these changes
Oct 22, 2026
Eduard van Valkenburg (eavanvalkenburg)
left a comment
Member
There was a problem hiding this comment.
The timeout cleanup now signals the POSIX process group after leader exit, preserves descendant termination, bounds abandoned pipe drains, closes their transports, and avoids zombie-only grace delays. The Linux regressions cover the critical lifecycle cases.
This branch was successfully deployed
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.

Motivation & Context
closes #9482
With
LocalShellTool(mode="stateless"), a timed-out command that left a background job holding stdout or stderr did not return until that job exited on its own.kill_process_treereturned early when the shell had already exited (sleep 6 &), and the psutil walk missed double-forked jobs that were reparented away from the shell, so neither was signalled and the unbounded drain inrun_statelesswaited for them.Description & Review Guide
kill_process_treeon POSIX now signals the process group withos.killpg(proc.pid, ...), SIGTERM first and SIGKILL after the grace period, even when the shell has already exited. The shell is started withstart_new_session=True, so its pid is the group id, and that still holds after the shell has been reaped, whenos.getpgid(proc.pid)would fail.run_statelessis bounded, so a process that left the session and still holds the pipes cannot block the call.timed_out=True, and background and double-forked jobs started by the command are killed._wait_for_exit, which pollsos.killpg(pgid, 0)because a process group has no handle to await.Tests:
test_stateless_timeout_kills_background_jobs_holding_pipes(Linux only) runssleep 5 & echo $! > <pidfile>and(sleep 5 & echo $! > <pidfile>); sleep 30withtimeout=0.5and checks that each call returns within two seconds withtimed_out=Trueand that the recorded background pid is gone. Both cases took 5.0 s and failed before the change.test_kill_via_psutil_signals_process_group_after_shell_exitedchecks that the group is signalled for a shell that has already exited.os.killpg, and the stdlib test expects the group id to be the pid.Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix); a workflow keeps the label and title prefix in sync automatically.