fix(env-probe): capture probe output via temp files so descendants can't stall the read (#67964 follow-up) - #68045
Merged
Merged
Conversation
…n't wedge sessions (#67964)
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.
Summary
Salvage of #67991 by @PRATHAMESH75: the env probe now captures subprocess output through temp files instead of pipes, so
timeoutbounds the whole call and a lingering pipe-holding descendant can't stall the probe at all — improving on the tree-kill approach merged in #67999 for the same bug (#67964).Why this is better than what #67999 shipped in
_run: temp files have no reader threads, sowait()only ever waits on the direct child. A fastpipthat leaves a lingering descendant now returns its real output in milliseconds (probe line survives), where the pipe-based path burned the full 3s timeout and discarded the output. The_kill_process_treehelper from #67999 becomes unnecessary and is removed; the fail-open worker/event layer around callers (also from #67999) stays — it guards against any future probe hang class.Changes
tools/env_probe.py::_run— capture viatempfile.TemporaryFilefor stdout/stderr; onTimeoutExpiredreturn immediately (no pipe read possible);_kill_process_treeremoved (cherry-picked from fix(tools): bound env probe subprocess so a Windows inherited pipe can't wedge sessions (#67964) #67991, conflict-resolved on top of fix(env-probe): stuck Windows probe can no longer deadlock system-prompt builds #67999)tests/tools/test_env_probe.py— contributor'sTestRunBoundedByTimeout: fast child + 20s pipe-holding grandchild must return the child's real output within milliseconds, not the timeoutValidation
tests/tools/test_env_probe.pytests/run_agent/test_run_agent.pyContributor authorship preserved via cherry-pick; merge with rebase.
Credit: @PRATHAMESH75 submitted the temp-file capture mechanism in #67991 before #67999 was opened.
Infographic