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.
agent-framework-automation
Bot
added
the
python
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
| # Drain any queued output. | ||
| try: | ||
| stdout_bytes, stderr_bytes = await proc.communicate() | ||
| stdout_bytes, stderr_bytes = await asyncio.wait_for(proc.communicate(), timeout=_DRAIN_GRACE) |
There was a problem hiding this comment.
You're right: once the drain deadline cancelled communicate(), both pipe transports stayed registered with the loop until the detached child exited, so each such timeout kept two descriptors open. The new commit closes the subprocess transport when the drain gives up, which closes both pipes. test_stateless_timeout_closes_pipes_held_by_detached_child starts a setsid child that keeps the pipes, lets the call time out, and checks that the count in /proc/self/fd is back to where it started.
| await asyncio.wait_for(proc.wait(), timeout=grace) | ||
| return | ||
| os.killpg(os.getpgid(proc.pid), signal.SIGKILL) | ||
| os.killpg(proc.pid, 0) |
There was a problem hiding this comment.
Agreed, killpg(pgid, 0) succeeds for a group that only holds unreaped zombies, and I reproduced the 2.5 second timeout with an orphan reaper that never reaps. After the killpg check, _group_alive now uses psutil to look for a group member whose status is not zombie, so the grace wait ends once every job has exited. test_stateless_timeout_does_not_wait_for_unreaped_zombies makes the test process the child subreaper, leaves the orphaned jobs as zombies, and asserts the 0.5 second timeout returns in under 2 seconds.
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.
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.