fix(terminal): deadlock-proof _bash_starts probe on Windows - #78023
Closed
ffouladgar-ux wants to merge 1 commit into
Closed
fix(terminal): deadlock-proof _bash_starts probe on Windows#78023ffouladgar-ux wants to merge 1 commit into
ffouladgar-ux wants to merge 1 commit into
Conversation
Replace subprocess.run(timeout=15) with Popen + explicit communicate(timeout=15). On Windows, subprocess.run's timeout cleanup kills only the direct child then runs an UNBOUNDED communicate(); when MSYS bash hangs in external-program spawn, orphaned grandchild processes hold the stdout pipe open and the cleanup blocks forever, stalling every tool that inits a LocalEnvironment (terminal, file reads) indefinitely inside the ACP adapter process. On timeout: taskkill /PID <pid> /T /F tree-kill on Windows + bounded 5s post-kill read. Verified end-to-end on Windows: ACP probes complete in 3-12s and real Multica issue tasks finalize.
Collaborator
Author
|
Thanks for the triage — agreed, this is a duplicate of #69083, which is the more complete fix (shared helper, corrected kill ordering, tests). Closing this one in favor of it. One data point to add for #69083 since I hit this independently: the hang is confirmed end-to-end in the wild — a Multica-spawned |
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.
What does this PR do?
Fixes a permanent hang of the bash availability probe (
_bash_starts) on Windows. The probe previously usedsubprocess.run(timeout=15), whose Windows cleanup path has a fatal flaw: when the timeout fires, Python kills only the direct child and then runs an unboundedcommunicate()cleanup. If MSYS bash (Git-for-Windows) hangs while spawning an external program (/usr/bin/true; /usr/bin/cat --version), orphaned grandchild processes keep the stdout pipe open, so the cleanupcommunicate()blocks forever.Because
_bash_startsis called during everyLocalEnvironmentinit, the hang stalled every tool that uses the local environment (terminal, file reads) indefinitely. Inside the ACP adapter process this meant ACP turns never completed and every delegated task hung forever.The fix replaces
subprocess.runwith aPopen+ explicitcommunicate(timeout=15)pair, and on timeout:taskkill /PID <pid> /T /Fon Windows (not just the direct child),Verified end-to-end on Windows 11: standalone ACP probes (
read_file-> 11.6s full reply;terminal echo hi-> 3.3s, exit 0) and a real Multica issue that completed in 1 minute and posted its finalization comment — the exact action that had hung for days.Type of Change
Changes Made
tools/environments/local.py— rewrite_bash_startsprobe:Popenwithstdin=DEVNULL,stdout/stderr=PIPE(wascapture_output=True),communicate(timeout=15),TimeoutExpired:taskkill /PID <pid> /T /Ftree-kill on Windows +proc.kill()fallback, then a bounded 5scommunicate()read,returncoderead from thePopenobject.How to Test
False; tools initialize and complete._bash_startsstill returnsTruefor a healthy bash.Checklist