Conversation
run_shell awaited communicate() under a bare wait_for, so a timeout — or a cancelled tool call, since tool_exec wraps every invocation in its own wait_for — left the shell and everything it had started still running. The model was told the command had stopped while it kept burning CPU and writing the workspace. Start the command in its own session and SIGKILL that group in a finally, the contract _CurrentCommands.run already uses on the bare host for the same reason. Terminating only the shell is not enough: a grandchild keeps the capture pipes open, asyncio wakes wait() only once those close, so that fix both leaves the grandchild writing and blocks until it exits on its own. The wait after the kill is bounded, because a setsid escapee is out of killpg's reach and the caller still has to get its TimeoutError. The regression test drives a backgrounded subshell, which survives a shell-only kill and is the case a plain terminate/kill pair passes by luck.
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup must handle exited shells with live child processes, and the regression test should cover that case.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR prevents timed-out or cancelled native shell commands from leaving background processes running.
Changes:
- Runs commands in separate sessions and bounds cleanup waits.
- Adds regression coverage for backgrounded shell children.
File summaries
| File | Summary |
|---|---|
apodex/tests/test_native.py |
Adds timeout cleanup coverage, but misses the case where the shell exits while a child holds capture pipes. |
apodex/sandbox.py |
Adds process-group cleanup, which can be skipped after the shell exits before communicate() completes. |
Review details
Suppressed comments (1)
apodex/tests/test_native.py:213
- This regression test keeps the shell alive with
wait, soproc.returncoderemainsNoneand it does not exercise the case where the shell exits while its background child still holds the capture pipes. Use a background command withoutwait(or add a separate case) so the test would catch the cleanup guard above.
"(sleep 2; touch marker) & wait", str(tmp_path), 1,
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| try: | ||
| out, err = await asyncio.wait_for(proc.communicate(), timeout=timeout) | ||
| finally: | ||
| if proc.returncode is None: |
|
Thanks for the fix and the regression test! I tried one more case where the shell exits before its background child: (sleep 1; touch marker) & exit 0With a short timeout, Could we attempt the group kill on timeout or cancellation even when the shell has exited, and add a regression test for this case? |
Summary
run_shelltimes out, instead of leaving the shell and its children runningsetsidescapee cannot hold the call openRoot cause
run_shellawaitedproc.communicate()under a bareasyncio.wait_for. On timeout nothing killed the child, so the model was told the command had stopped while it kept burning CPU and writing the workspace. The same leak happens on cancellation:tool_execwraps every tool invocation in its ownwait_for, so an outer tool timeout or a user interrupt cancelsrun_shellat the same await.Terminating only the shell is not enough. A grandchild — anything the command backgrounds — keeps the capture pipes open, and asyncio resolves
Process.wait()only once those pipes close (base_subprocess.py: exit waiters are woken from_call_connection_lost). A terminate-then-wait fix therefore leaves the grandchild writing the workspace and blocks the caller until it exits on its own:This follows
_CurrentCommands.runinplugins/tools/_sandbox.py, which already runs the shell in its own session and SIGKILLs the group on the bare host for exactly this reason, bounded wait included. Only thehost/native/containerpath needed it; thebwrappath reaps throughsandbox.commands.run.Reproduction
The child sleeps, then writes a marker; the caller passes
timeout=1. A child that was reaped can never write the marker.On
main@ 9e533db:On this branch:
Validation
uv run pytest -q— 1718 passed, 4 skipped (run twice)uv run pytest apodex/tests -q— 823 passed, from the committed branchuv run ruff check apodex/sandbox.py apodex/tests/test_native.py— passeduv run pyright apodex/sandbox.py apodex/tests/test_native.py— 0 errors-W error::ResourceWarning; before the fix it failed and emitted the leaked-transport warning the issue mentionssetsidescapee returns after timeout + 5s instead of hangingNotes
sudofails fast instead of fighting the TUI; the native backend inplugins/tools/_sandbox.pyalready behaves this wayos.killpgis POSIX-only; native Windows is not in the release matrix (docs/install/linux.md), so Windows is covered through WSL2Fixes #40