Repository navigation
Conversation
Address review on google#7521 / google#7507: on CancelledError kill the process group, cancel completion and drain readers, gather them, then re-raise; reject max_output_bytes_per_stream < 0; add exact-at-limit and cancellation tests.
BichengWang
left a comment
There was a problem hiding this comment.
Reviewed head 9cf04517672d6b3f2a868ba2d5fc3eb81bf40234, including the cancellation follow-up discussed in #7507.
[P2] Retrieve the original completion future after cancellation — bash_tool.py:385–390
completion.cancel() cancels the original gather(), but the cleanup only awaits a different gather over readers. Since wait_for() has already cancelled the shield, nothing retrieves completion's CancelledError. A normal, handled cancellation consequently reaches the event loop's exception handler as:
_GatheringFuture exception was never retrieved
asyncio.exceptions.CancelledError
This produces an unhandled-error log and invokes application exception-reporting hooks for expected cancellation. It reproduces with a single sleeping subprocess; no background descendant is needed. The existing cancellation test checks pending reader tasks, so it passes despite this error.
Please await/consume the original completion future after cancelling it, while preserving cancellation propagation. A regression can install a temporary loop exception handler, start a command that writes a readiness marker and sleeps, wait for the marker, cancel run_async, catch its expected CancelledError, yield/collect finished futures, and assert that the handler received no errors. I verified that awaiting asyncio.gather(completion, return_exceptions=True) after completion.cancel() clears this reproduction. The timeout path also needs to consume completion when _finish_readers() cancels readers: a detached descendant holding a pipe open reproduced the same warning there.
Validation on macOS / Python 3.13.16, offline:
PYTHONPATH=src python -m pytest -q -p no:cacheprovider tests/unittests/tools/test_bash_tool.py tests/unittests/tools/test_bash_tool_windows.py: 37 passed.- The real-subprocess cancellation regression passes against the PR base
62745ccc62513514bc0f30f5cfdaf1c593d156ca, fails against this head with the error above, and passes with the completion-await remedy applied only to an in-memory module copy. - Feeding 64 MiB in 64 KiB chunks into
_BoundedOutputwith a 1 MiB limit retained exactly 1,048,576 bytes, reported 66,060,288 omitted bytes, and peaked at 1,106,090 traced bytes during accumulation. This supports the bounded-capture approach independently of the returned string length.
I did not run the full multi-version tox suite.
|
Thanks @BichengWang — addressed in e8581a2: the cancelled completion future is now awaited so its CancelledError is retrieved while cancellation still propagates, and I added the regression test with a temporary loop exception handler as you described. |
e8581a2 to
78ed767
Compare
Address review on google#7521 / google#7507: on CancelledError kill the process group, cancel completion and drain readers, gather them, then re-raise; reject max_output_bytes_per_stream < 0; add exact-at-limit and cancellation tests.
Replace process.communicate() with concurrent 64 KiB chunked reads of stdout and stderr into per-stream buffers capped by the new BashToolPolicy.max_output_bytes_per_stream (default 1 MiB, None disables). Excess output is drained and discarded so the child never blocks on a full pipe, and the returned text ends with a marker noting how many bytes were omitted. Timeout handling keeps draining the pipes after killing the process group so the subprocess is reaped. Fixes google#7507
Address review on google#7521 / google#7507: on CancelledError kill the process group, cancel completion and drain readers, gather them, then re-raise; reject max_output_bytes_per_stream < 0; add exact-at-limit and cancellation tests.
78ed767 to
c9aff13
Compare
Drop gather+shield so a setsid descendant holding a pipe cannot leave process.wait() pending after timeout, and so Python 3.14 does not log CancelledError from shielded futures. Track readers and an explicit waiter task, finish all three on timeout/cancel, and cover the setsid timeout path with a regression test.
Problem
ExecuteBashTool.run_async()collected subprocess output withprocess.communicate(), which buffers all of stdout and stderr in the agent (parent) process with no size limit.RLIMIT_AS/RLIMIT_FSIZEapply to the child, not to the pipes, so a user-approved but verbose command could use up the agent process's memory before the timeout fires. The timeout path also returned everything it had captured, with no cap.Fix
BashToolPolicy.max_output_bytes_per_stream(default 1 MiB,Nonedisables)....[stdout truncated: N of M bytes omitted; limit is L bytes].wait_forcancellation, so the pipes keep draining after the process group is SIGKILLed. Then the process is reaped (with a short grace period, after which any leftover readers are cancelled). The process-group cleanup infinallyis unchanged.Tests
New
TestOutputLimitsintests/unittests/tools/test_bash_tool.py:Noneturns the limit offreturncode == -SIGKILL)I updated the existing mocked-process tests to stub
stdout/stderrinstead ofcommunicate().Red/green: the new tests fail against
main(7 failed) and pass with the fix.pytest tests/unittests/tools/test_bash_tool.py tests/unittests/tools/test_bash_tool_windows.py→ 33 passed. Formatted with pyink + isort.Fixes #7507