fix(tools): bound env probe subprocess so a Windows inherited pipe can't wedge sessions (#67964) - #67991
fix(tools): bound env probe subprocess so a Windows inherited pipe can't wedge sessions (#67964)#67991PRATHAMESH75 wants to merge 1 commit into
Conversation
…n't wedge sessions (NousResearch#67964)
Related: focused fix PR for open #67964's Windows inherited-pipe environment-probe deadlock; the targeted env-probe suite passes. |
|
Merged via #68045 — your commit was cherry-picked onto current main with your authorship preserved in git log (e77ffdc). Context: while your PR was open, #67999 landed a different fix for the same deadlock (#67964) — process-tree kill on timeout plus a fail-open background worker so prompt builds never wait on a stuck probe. Your temp-file capture approach was the better mechanism for Thanks for the well-aimed fix — first submission of the capture mechanism, and it made the final design better. |
What does this PR do?
Fixes #67964. On native Windows, the local Python toolchain probe can deadlock every new session's system-prompt build.
tools/env_probe.py::_runcaptures subprocess output withcapture_output=True(OS pipes). A console-script launcher such aspip.exespawns a descendant (python.exe … pip --version) that inherits the captured stdout/stderr write ends and can outlive its parent. With pipes, the reader threads insidesubprocess.communicate()then block until that grandchild closes the write end — which the 3stimeoutdoes not bound, because killing the direct child leaves the grandchild holding the pipe. In the reported incident the warm-probe thread stayed blocked ~28 min.Because
get_environment_probe_line()holds the module-level_CACHE_LOCKacross the probe, every new session that needs a system prompt then waits on that lock indefinitely — a partial outage (new chats stuckworking, zero API calls; older sessions with a persisted prompt unaffected).Fix
Capture through
tempfile.TemporaryFile()instead ofcapture_outputpipes. Temp files have no reader threads, sosubprocess'stimeout/wait()only ever waits on the direct child. A lingering grandchild holding an inherited handle can no longer block the parent, the documented 3s timeout genuinely bounds the whole call, and the probe fails open — satisfying the issue's stated expectation. This mirrors the "don't wait on a Windows-inherited pipe handle" pattern already used in #67373 forBaseEnvironment.execute().Single-purpose change confined to
_run; the happy-path return contract(returncode, stdout, stderr)is unchanged, so_CACHE_LOCKis now held for at most the bounded probe duration.Testing
TestRunBoundedByTimeoutintests/tools/test_env_probe.py: a direct child printsok, spawns a 20s-sleeping grandchild inheriting its stdout, then exits. Asserts_runreturnsrc=0,out="ok"in< 3.0s. Verified this test fails on the oldcapture_outputpath (blocks the full 3s →rc=-1) and passes with temp-file capture.tests/tools/test_env_probe.py(11) +tests/run_agent/test_run_agent.py— 447 passed.ruff checkclean (incl. PLW1514);check-windows-footguns.pyandcheck_subprocess_stdin.pypass.The arm64-fork-Docker CI job is expected to fail on fork PRs.