fix(tools): stop the bash tool hanging when a detached child holds the pipes#875
Open
abhay-codes07 wants to merge 1 commit into
Open
fix(tools): stop the bash tool hanging when a detached child holds the pipes#875abhay-codes07 wants to merge 1 commit into
abhay-codes07 wants to merge 1 commit into
Conversation
…e pipes A command that spawns a detached or backgrounded child inheriting stdout/stderr left the bash tool blocked until its timeout even though the command itself exited immediately: communicate() waits for EOF on the pipes, and asyncio's Process.wait() has the same behavior because BaseSubprocessTransport only wakes exit waiters once every pipe has disconnected, not when the process exits. The tool now pumps stdout and stderr concurrently into bounded buffers, polls returncode (set as soon as the process exits) with a deadline for the timeout, and gives the pipes a short grace period to reach EOF before returning with whatever output was captured. Bounding the buffers also stops a command that floods stdout from buffering unbounded output in memory; reading continues past the cap so the child never blocks on a full pipe. kill_async_subprocess had the same pipe dependency through proc.wait() after killing, which could stall the timeout path forever; it now polls returncode as well. Fixes mistralai#831
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.
Fixes #831
A tool call that spawns a detached or backgrounded child process left the bash tool stuck until its timeout (300s by default) even though the command itself finished instantly, exactly as the standalone reproducer in #831 shows.
Root cause, two layers deep:
proc.communicate()waits for EOF on stdout/stderr, and a detached child that inherited those pipes keeps them open for as long as it lives.proc.wait()does not help: asyncio'sBaseSubprocessTransportonly wakes exit waiters in_call_connection_lost, which requires the process to have exited AND every pipe transport to have disconnected (_try_finishin CPython's asyncio/base_subprocess.py). Sowait()is pipe-hostage too. I verified this empirically before landing on the approach below.Change in the bash tool:
max_output_bytes(reading continues past the cap so a chatty child never blocks on a full pipe; this also stops a flood of output from buffering unbounded in memory, which communicate() did)returncode, which the transport sets as soon as the process exits, with the timeout as deadlinekill_async_subprocesshad the same pipe dependency throughproc.wait()after killing, which could stall the timeout path indefinitely when the surviving pipe holder was outside the killed group; it pollsreturncodenow as well.Testing:
@michelTho this closes out the fuzzer-style reproducer in #831.