Repository navigation
ExecuteBashTool buffers unbounded stdout/stderr in the parent process #7507
Description
Summary
"ExecuteBashTool.run_async()" uses "process.communicate()" to collect subprocess stdout and stderr without an output-size limit.
Although "BashToolPolicy" supports timeouts and child-process resource limits, these do not limit memory allocated by the parent Python process while collecting output.
A user-approved command producing verbose output can therefore exhaust the agent process's memory before its execution timeout is reached.
Root cause
In "src/google/adk/tools/bash_tool.py":
- Both stdout and stderr are configured as "asyncio.subprocess.PIPE".
- "process.communicate()" buffers both streams until the process exits.
- "RLIMIT_AS" applies to the child process, not to the parent process collecting output.
- "RLIMIT_FSIZE" does not constrain pipe output.
- Decoding captured bytes and returning them as strings creates additional memory allocations.
The timeout path also collects and returns captured output without a size cap.
Safe reproduction
The following reproduces the subprocess capture pattern without requiring API credentials or excessive memory consumption.
import asyncio
import sys
async def main():
proc = await asyncio.create_subprocess_exec(
sys.executable,
"-c",
"import sys; sys.stdout.buffer.write(b'x' * (2 * 1024 * 1024))",
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
start_new_session=True,
)
stdout, stderr = await asyncio.wait_for(
proc.communicate(), timeout=5
)
print(len(stdout), len(stderr), proc.returncode)
asyncio.run(main())
Observed output:
2097152 0 0
The entire 2 MiB output was retained in memory.
This verifies the underlying capture behavior. A complete ADK integration test has not yet been executed.
Expected behavior
Introduce a configurable output-capture budget enforced while reading the subprocess pipes.
The implementation should:
- Bound stdout and stderr memory usage before decoding.
- Read both pipes concurrently without risking deadlock.
- Terminate or truncate output when the budget is exceeded.
- Explicitly report output truncation or an output-limit error.
- Preserve timeout, cancellation, and process-group cleanup behavior.
Regression tests
- Small output remains unchanged.
- Large output cannot cause proportional parent-process memory growth.
- Concurrent stdout and stderr cannot bypass the limit.
- Timeout and cancellation correctly reap the subprocess.
- Existing child resource limits continue working.
Validation
- Inspected the current implementation.
- Executed a safe, bounded standalone asyncio reproduction.
- Checked existing BashTool tests and searched for duplicate reports.
- Full ADK integration tests — not yet run.
This issue concerns resource reliability for user-approved commands, not bypassing tool confirmation.
Activity
Hugo-DDT commentedon Oct 12, 2026
Hi, I'd like to work on this. My plan: read stdout/stderr incrementally instead of
communicate(), cap the captured bytes per stream (a configurablemax_output_bytesonBashToolPolicywith a sensible default), keep draining but discard beyond the cap so the child doesn't block, and append a truncation marker to the returned output. The timeout path would use the same capped buffers. I'd add unit tests covering output over the cap, under the cap, and the timeout case. Does this approach sound good, or would you prefer a different limit/config shape?qiyuhuating commentedon Oct 12, 2026
@Hugo-DDT Thanks for offering to work on this. Yes, incremental reads with bounded capture, concurrent draining of both pipes, and discarding excess bytes sound like a good approach for this issue. Letting the command continue while explicitly marking truncated output is reasonable.
For the configuration, a per-stream cap is fine, provided its semantics are explicit: with a cap of
Nbytes per stream, retained stdout and stderr together can reach2N, plus read/transport buffers and decoding overhead. I would suggestmax_output_bytes_per_streamto make that distinction clear, with a finite, documented default. The final API name and default should be agreed with the maintainers.A few implementation details are important:
communicate(). Timeout and cancellation need process-group cleanup, subprocess reaping, and reader-task cleanup, with cancellation still propagating. Cleanup should not hang if a descendant holds a pipe open.For regression coverage, please include under/exactly-at/over-limit output, stderr-only and simultaneous stdout/stderr, and output without newlines. At least one real-subprocess test should exceed pipe capacity and verify completion without deadlock. Timeout should preserve the captured prefix and report the timeout; cancellation should propagate without leaving child processes or reader tasks behind. Please also verify that retained capture buffers remain bounded as generated output increases—checking only the final returned string length would miss buffering before truncation. Existing confirmation and resource-limit tests should continue to pass.
I rechecked the current implementation and tests, but have not run full ADK integration tests. These are suggestions from me as the reporter; the maintainers have the final say. I also noticed the issue is currently assigned to
surajksharma07, so it would be good to coordinate with them and the maintainers to avoid overlapping work.Hugo-DDT commentedon Oct 12, 2026
Hi @surajksharma07, I see this was assigned to you shortly after my comment. Are you already working on it? If so, I'm happy to step back. If not, I'd be glad to take it on following @qiyuhuating's suggestions above (
max_output_bytes_per_stream, raw-byte cap with fixed-size reads, concurrent draining, bounded timeout path, and the listed regression tests). Thanks!surajksharma07 commentedon Oct 13, 2026
Reproduced this on 2.11.0 @qiyuhuating: a 128 MiB burst came back in full and the agent process peaked around 270 MB even with max_memory_bytes set to 64 MiB on the child.
@Hugo-DDT haven't started a PR so please go ahead and take it. Your plan matches what I tried locally as a workaround: two concurrent readers doing 64 KiB reads, a per-stream byte cap with a truncation marker and the timeout path reusing those buffers instead of calling communicate() again. With a 1 MiB cap peak memory dropped to roughly 4.5 MB.
Could you check that this approach holds up on your side too especially for cancellation and for a background child that keeps the pipe open after the kill? Haven't been able to cover those two cases yet.
Please run the full bash tool tests plus the regression cases listed above before raising the PR. The name and default for max_output_bytes_per_stream will still need a maintainer's sign-off.
qiyuhuating commentedon Oct 13, 2026
Thanks for reproducing this on 2.11.0 and for sharing the memory measurements. The 128 MiB case makes the parent-side buffering issue much more concrete, and the drop from roughly 270 MB to about 4.5 MB with bounded capture is a useful validation of the proposed direction.
The two remaining cases you called out are the ones I would consider most important before the implementation is finalized:
If @Hugo-DDT's implementation handles those cases while preserving the existing timeout and process-group cleanup behavior, that would address the main reliability concern raised here.
I agree that the exact configuration name and default limit should remain a maintainer decision. From the issue-reporting side, I think the current reproduction and the proposed bounded concurrent-reader approach are now well supported. Thanks both for validating it further.
surajksharma07 commentedon Oct 14, 2026
Went through #7521 @Hugo-DDT and 2.12.0 still uses communicate() so this isn't fixed in a release yet. With a 64 MiB/stream flood peak allocation dropped from about 257 MiB on main to about 6 MiB with your branch. A timeout where a descendant keeps the pipe open now returns after timeout + 1s instead of hanging for the child's whole lifetime (30s in my test).
One gap I hit: if run_async is cancelled while a descendant still holds the pipe the CancelledError propagates fine but both _drain_stream tasks and Process.wait are still pending afterwards. An except asyncio.CancelledError branch that kills the group, cancels completion and the readers, gathers them and then re-raises cleared it for me locally. Could you try that and maybe add tests for cancellation and for exactly-at-limit output?
Small one: max_output_bytes_per_stream=-1 is currently accepted so might be worth validating >= 0 in post_init. The 1 MiB default is a behaviour change so it still needs a maintainer's sign-off.
fix(tools): clean up bash drain on cancel; validate output limit
Hugo-DDT commentedon Oct 16, 2026
Thanks for the suggestions! I've addressed all three in #7521: cancellation cleanup no longer hangs (and the cancelled future is retrieved),
max_output_bytes_per_stream >= 0is validated, and there are new exactly-at-limit and cancellation tests. Would appreciate another look.fix(tools): bound ExecuteBashTool stdout/stderr capture
fix(tools): clean up bash drain on cancel; validate output limit
surajksharma07 commentedon Oct 19, 2026
Went through 78ed767 @Hugo-DDT. The cancel cleanup and the >= 0 check look good and the cancel path is clean on 3.12/3.13 now. 2.12.0 is still the latest release and main still uses communicate() so #7521 is still the only fix for this.
The timeout path still has the case @BichengWang mentioned. When a descendant holds the pipe _finish_readers cancels the readers but nobody consumes completion and the process.wait() inside that gather stays pending until the descendant closes the pipe. On 3.14 (ADK supports it) shield also logs "CancelledError exception in shielded future" on both the timeout and cancel paths even with the new gather(completion). The new test only looks for "never retrieved" so it misses that.
What worked in a small asyncio sim on 3.12/3.13/3.14: drop the gather + shield and use explicit tasks, e.g. waiter = create_task(process.wait()) then asyncio.wait([*readers, waiter], timeout=...). On timeout kill the group and run _finish_readers over all three. On cancel cancel and gather the same three. That gave 0 pending tasks and no loop errors on all three versions.
Could you try that, add a timeout test with a setsid’d background child holding the pipe and run the bash tool tests on 3.14 too? The 1 MiB default still needs a maintainer’s sign-off.
fix(tools): bound ExecuteBashTool stdout/stderr capture
fix(tools): clean up bash drain on cancel; validate output limit
Hugo-DDT commentedon Oct 21, 2026
Thanks @surajksharma07 - addressed in 28f23be on #7521: dropped gather+shield for an explicit process.wait waiter and unified _finish_tasks cleanup on timeout/cancel; added test_timeout_setsid_child_holding_pipe_cleans_up. Bash tool tests: 38 passed on 3.13, 39 on 3.14.