fix(tools): prevent terminal probe hang and kill orphaned processes on Windows - #103402
Open
ericmaddox wants to merge 1 commit into
Open
ericmaddox wants to merge 1 commit into
ericmaddox wants to merge 1 commit into
Conversation
…n Windows When checking whether Git Bash candidates can launch MSYS2 child processes on Windows, `_bash_starts` used `subprocess.run(timeout=15)`. When a candidate deadlocked during probe (e.g. MSYS fork deadlock), `subprocess.run`'s timeout kill only terminated the parent wrapper (`bin\bash.exe`), leaving the re-exec'd child (`usr\bin\bash.exe`) alive. Because the child held the stdout/stderr pipe handles open, Python's `communicate()` inside `subprocess.run` hung indefinitely past the timeout. Fix this by: 1. Replacing unbounded communicate/kill with `_run_probe_command` which executes taskkill /F /T on Windows to terminate the entire process tree on timeout. 2. Tightening probe timeout from 15s to 5s. 3. Adding direct `usr\bin\bash.exe` candidate paths to `_windows_bash_candidates` so Git for Windows installations can be probed directly. 4. Adding timeout and deadlock markers to `_MSYS_SPAWN_FAILURE_MARKERS` to surface actionable remediation instructions if all candidates fail. Closes NousResearch#103398
Related: #83413 (earlier open PR, same tree-kill approach for the orphaned |
This was referenced Sep 5, 2026
This branch has not been deployed
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?
When testing whether candidate Git Bash executables on Windows can successfully launch MSYS child processes,
_bash_startsruns an external program probe (/usr/bin/true; /usr/bin/cat --version >/dev/null).On Windows, Git for Windows'
<root>\bin\bash.exeis a wrapper shim that spawns<root>\usr\bin\bash.exe. When the probe child deadlocks (e.g. MSYS2 fork deadlock),subprocess.run(..., timeout=15)times out and callsproc.kill(). However, on Windowsproc.kill()(viaTerminateProcess) only terminates the direct wrapper process, leaving the re-exec'dusr\bin\bash.exeprocess alive and holding open the stdout/stderr pipe handles.Because Python's standard library
subprocess.runimplementation callsproc.communicate()without a timeout inside itsexcept TimeoutExpiredblock to drain pipes, it blocks indefinitely waiting for EOF on the still-open pipes. This causes the entireterminalenvironment creation to hang for minutes past the timeout.Fix
_run_probe_commandwith explicit process tree termination on Windows (taskkill /F /T /PID <pid>) upon timeout before killing the process, ensuring all child/grandchild processes are stopped and pipes are closed._windows_bash_candidatesto probe<root>\usr\bin\bash.exedirectly alongside<root>\bin\bash.exe, avoiding the extra wrapper process layer._MSYS_SPAWN_FAILURE_MARKERSto recognize timeout/deadlock probe diagnostics as MSYS spawn failures, ensuring actionable remediation guidance is presented if all candidates fail.Related Issue
Fixes #103398
Type of Change
Changes Made
tools/environments/local_gitbash_probe.py: Replace unboundedsubprocess.runwith_run_probe_commandwhich executestaskkill /F /T /PID <pid>on Windows timeout. Tighten timeout to 5s. Add timeout/deadlock markers to_MSYS_SPAWN_FAILURE_MARKERS.tools/environments/local.py: In_windows_bash_candidates, includeusr/binbash candidate paths and resolve directusr�in�ash.exefor custom paths.tests/tools/test_find_shell.py: Update unit tests to mock_run_probe_commandand addtest_probe_timeout_records_deadlock_diagnosticasserting that probe timeouts fail cleanly and record MSYS failure diagnostics.How to Test
pytest tests/tools/test_find_shell.py -vtest_probe_timeout_records_deadlock_diagnosticvalidates that a hung probe times out and registers as an MSYS failure.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A