From e77ffdc28cdf407f47ad0070c6358e343b390174 Mon Sep 17 00:00:00 2001 From: PRATHAMESH75 Date: Mon, 20 Jul 2026 16:08:22 +0530 Subject: [PATCH] fix(tools): bound env probe subprocess so a Windows inherited pipe can't wedge sessions (#67964) --- tests/tools/test_env_probe.py | 40 +++++++++++++++ tools/env_probe.py | 92 ++++++++++++----------------------- 2 files changed, 71 insertions(+), 61 deletions(-) diff --git a/tests/tools/test_env_probe.py b/tests/tools/test_env_probe.py index a8aacd396649..d20f3c234c19 100644 --- a/tests/tools/test_env_probe.py +++ b/tests/tools/test_env_probe.py @@ -290,3 +290,43 @@ class TestRunTimeoutIsBounded: # stdlib subprocess.run on Windows would hang here for the full 20s # (unbounded post-kill communicate). Our bound: timeout + reap slack. assert elapsed < 6 + + +class TestRunBoundedByTimeout: + """``_run`` must return as soon as the *direct* child exits, even when a + descendant inherited the captured stdout/stderr handles and outlives it. + + This is the deadlock from #67964: on native Windows a ``pip.exe`` launcher + can leave a grandchild holding the captured pipe open, and ``capture_output`` + reader threads then block far past the timeout while ``_CACHE_LOCK`` is held, + wedging every new session. Capturing through temp files removes the reader + threads, so a lingering grandchild can't block the parent's ``wait()``. + + Cross-platform repro: the direct child prints ``ok`` and exits immediately + after spawning a long-sleeping grandchild that inherits its stdout. With the + old pipe-based capture, ``_run`` blocks until the grandchild exits (or hits + the 3s timeout and returns ``-1``); with temp-file capture it returns the + child's real output within a few milliseconds. + """ + + def test_returns_before_inheriting_grandchild_exits(self): + import time + + grandchild_sleep = 20 # far longer than _run's timeout + # Direct child: emit "ok", spawn a detached grandchild that inherits + # this process's stdout (no stdout= redirect), then exit right away. + child_code = ( + "import subprocess, sys; " + "subprocess.Popen([sys.executable, '-c', " + f"'import time; time.sleep({grandchild_sleep})']); " + "sys.stdout.write('ok'); sys.stdout.flush()" + ) + + start = time.monotonic() + rc, out, err = env_probe._run([sys.executable, "-c", child_code], timeout=3.0) + elapsed = time.monotonic() - start + + # Must not wait on the grandchild, and must not have hit the timeout. + assert elapsed < 3.0, f"_run blocked on grandchild for {elapsed:.1f}s" + assert rc == 0, f"expected clean exit, got rc={rc} err={err!r}" + assert out == "ok" diff --git a/tools/env_probe.py b/tools/env_probe.py index f4e98c43b660..95427cc1c9af 100644 --- a/tools/env_probe.py +++ b/tools/env_probe.py @@ -34,6 +34,7 @@ import os import shutil import subprocess import sys +import tempfile import threading from typing import Optional @@ -75,77 +76,46 @@ _REMOTE_BACKENDS = frozenset({ }) -def _kill_process_tree(proc: "subprocess.Popen") -> None: - """Best-effort kill of ``proc`` and its descendants. - - On Windows, killing only the direct child leaves any descendants it - spawned (e.g. the ``python.exe`` a ``pip.exe`` console-script launcher - starts) alive and holding inherited stdout/stderr write handles, which - is exactly what wedges pipe readers (#67964). ``taskkill /T`` sweeps - the tree while it is still intact; it cannot catch descendants that - were already orphaned, which is why ``_run`` must additionally never - perform an unbounded pipe read after a timeout. - """ - if sys.platform == "win32": - try: - # No pipes → this call cannot itself deadlock on captured output. - subprocess.run( - ["taskkill", "/T", "/F", "/PID", str(proc.pid)], - stdin=subprocess.DEVNULL, - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - timeout=2, - check=False, - ) - except Exception: - pass - try: - proc.kill() - except Exception: - pass - - def _run(cmd: list[str], timeout: float = 3.0) -> tuple[int, str, str]: """Run a short subprocess. Returns (returncode, stdout, stderr). Failures (binary missing, timeout, OSError) return (-1, "", ""). - Deliberately NOT ``subprocess.run``: on Windows, ``run()`` reacts to a - ``TimeoutExpired`` by calling ``communicate()`` a second time with **no - timeout** to collect partial output. That second call joins the pipe - reader threads unboundedly — and if the child spawned a descendant that - inherited the stdout/stderr write handles and outlives it (an orphaned - ``pip --version`` grandchild in incident #67964), the pipes never hit - EOF and the join blocks forever. Here a timeout kills the process tree - and abandons the (daemon) reader threads instead: bounded always, at - the cost of discarding partial output we don't need anyway. + Output is captured through temporary files rather than ``capture_output`` + pipes so ``timeout`` bounds the *whole* call — even on native Windows. A + console-script launcher (e.g. ``pip.exe``) can spawn a descendant that + inherits the captured stdout/stderr handles and outlives its parent. With + OS pipes, the reader threads inside ``subprocess.communicate()`` then block + until that descendant closes the write end — which the timeout does *not* + cover, because killing the direct child leaves the grandchild holding the + pipe. A whole warm probe could hang for ~28 min this way while holding + ``_CACHE_LOCK``, wedging every new session's system-prompt build. + + Temp files have no reader threads, so ``wait()`` only ever waits on the + direct child; a lingering grandchild holding the handle can't block us, and + the probe genuinely fails open on timeout. """ - proc = None try: - proc = subprocess.Popen( - cmd, - stdin=subprocess.DEVNULL, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - ) - stdout, stderr = proc.communicate(timeout=timeout) - return proc.returncode, (stdout or "").strip(), (stderr or "").strip() + with tempfile.TemporaryFile() as out_f, tempfile.TemporaryFile() as err_f: + try: + result = subprocess.run( + cmd, + stdout=out_f, + stderr=err_f, + timeout=timeout, + check=False, + stdin=subprocess.DEVNULL, + ) + except subprocess.TimeoutExpired: + return -1, "", "timeout" + out_f.seek(0) + err_f.seek(0) + out = out_f.read().decode("utf-8", "replace").strip() + err = err_f.read().decode("utf-8", "replace").strip() + return result.returncode, out, err except FileNotFoundError: return -1, "", "not found" - except subprocess.TimeoutExpired: - if proc is not None: - _kill_process_tree(proc) - try: - # Bounded reap of the direct child. Does NOT read the pipes, - # so an orphaned descendant holding them open cannot block us. - proc.wait(timeout=1) - except Exception: - pass - return -1, "", "timeout" except OSError as exc: - if proc is not None: - _kill_process_tree(proc) return -1, "", f"oserror: {exc}"