diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py index 6f8d7f5b802a..bf226ce7c0a3 100644 --- a/hermes_cli/_subprocess_compat.py +++ b/hermes_cli/_subprocess_compat.py @@ -32,7 +32,7 @@ import shutil import subprocess import sys -from typing import Mapping, Sequence +from typing import Mapping, Optional, Sequence __all__ = [ "IS_WINDOWS", @@ -42,6 +42,7 @@ "windows_detach_flags_without_breakaway", "windows_hide_flags", "windows_detach_popen_kwargs", + "bounded_captured_run", "bounded_git_probe", "noninteractive_git_env", ] @@ -345,26 +346,25 @@ def noninteractive_git_env( # ----------------------------------------------------------------------------- -# Bounded, fail-open git probing (Windows post-kill deadlock guard) +# Bounded, fail-open probing (Windows post-kill deadlock guard) # ----------------------------------------------------------------------------- -def _kill_git_process_tree(proc: "subprocess.Popen") -> None: +def _kill_process_tree(proc: "subprocess.Popen") -> None: """Best-effort terminate *proc* and its descendants on both platforms. ``proc.kill()`` alone only terminates the direct child. On Windows a - suspended descendant ``git.exe`` can survive holding duplicates of the - captured pipe handles, which keeps the pipes from reaching EOF and leaks two - reader threads + the process per fired timeout — ``taskkill /T /F`` takes the - whole tree down so the bounded drain that follows can actually reach EOF. - On POSIX the same class exists: killing the launcher leaves descendants - (credential helpers, ``git-remote-https``, hook children) running and - holding the pipe write ends. The probe is spawned in its own process group - (``process_group=0`` in :func:`bounded_git_probe`), so when — and only - when — the child leads its own group (``pgid == pid``), the entire group is - signalled with ``os.killpg``. The ownership check means a fallback spawn - that shares our group can never cause us to kill unrelated processes. - Ported from openai/codex#36793 ("Terminate timed-out Git process trees"). + suspended descendant can survive holding duplicates of the captured pipe + handles, so ``taskkill /T /F`` takes down the whole tree. On POSIX the probe + is spawned in its own process group (``process_group=0`` in + :func:`bounded_captured_run`), so the group is signalled only when the child + actually leads it (``pgid == pid``). The ownership check prevents a fallback + spawn that shares our group from taking down unrelated processes. + + On Windows the tree kill must run *before* ``proc.kill()``: once the + launcher exits, Windows can no longer reliably discover its descendants, so + kill-then-taskkill leaves orphans. ``proc.kill()`` remains as a fallback if + taskkill fails or is unavailable. All failures are swallowed — this is cleanup on an already-failing path, and the caller's contract is to fail open. ``kill()`` can raise (access denied, @@ -373,7 +373,20 @@ def _kill_git_process_tree(proc: "subprocess.Popen") -> None: re-enter the deadlock class it fixes: it captures no pipes (DEVNULL), so its own timeout cleanup has no reader threads to join. """ - if not IS_WINDOWS: + if IS_WINDOWS: + try: + subprocess.run( + ["taskkill", "/T", "/F", "/PID", str(proc.pid)], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + stdin=subprocess.DEVNULL, + timeout=2, + check=False, + creationflags=windows_hide_flags(), + ) + except Exception: + pass + else: # Group-kill first: verify the child actually leads its own process # group before signalling it, so we never blast a shared group. try: @@ -388,77 +401,100 @@ def _kill_git_process_tree(proc: "subprocess.Popen") -> None: proc.kill() except OSError: pass - if IS_WINDOWS: - try: - subprocess.run( - ["taskkill", "/T", "/F", "/PID", str(proc.pid)], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - stdin=subprocess.DEVNULL, - timeout=2, - check=False, - creationflags=windows_hide_flags(), - ) - except Exception: - pass -def bounded_git_probe(argv: Sequence[str], *, timeout: float) -> str: - """Run a short, throwaway ``git`` probe and return stripped stdout, or ``""`` - on ANY failure (nonzero exit, timeout, spawn error, decode error). +# Back-compat alias for the original git-probe name. +_kill_git_process_tree = _kill_process_tree + + +def bounded_captured_run( + argv: Sequence[str], + *, + timeout: float, + env: Optional[Mapping[str, str]] = None, +) -> subprocess.CompletedProcess: + """Deadlock-safe ``subprocess.run(..., capture_output=True, text=True, timeout=)``. - This is the shared, deadlock-safe replacement for - ``subprocess.run(["git", ...], timeout=...)`` at fail-open probe call sites - (``tui_gateway.git_probe.run_git``, ``agent.coding_context._git``). + Shared replacement for short fail-open probes that previously used + ``subprocess.run(timeout=...)`` (``bounded_git_probe``, Windows Git Bash + ``_bash_starts``, ...). Why not ``subprocess.run``: on Windows, ``run()``'s post-timeout cleanup - calls an *unbounded* ``communicate()`` after killing git. Killing the - PATH-resolved launcher can leave a suspended descendant ``git.exe`` holding - duplicates of the captured stdout/stderr handles, so the pipes never reach - EOF and the reader-thread join blocks forever. On the Desktop agent-build - path (``_start_agent_build → _session_info → branch() → run_git``) that turned - an optional branch label into ``agent initialization timed out`` - (issues #68609 / #66037). + calls an *unbounded* ``communicate()`` after killing the child. Killing only + the launcher can leave a suspended descendant holding duplicates of the + captured stdout/stderr handles, so the pipes never reach EOF and the + reader-thread join blocks forever — turning an optional probe into a hung + tool/session startup (issues #68609 / #66037; same class for Git Bash + ``true``/``cat`` children under Mandatory ASLR / stuck MSYS spawns). The bounded flow: an explicit ``communicate(timeout)``, then on any failure a - tree-kill (see :func:`_kill_git_process_tree`) plus a bounded 1s post-kill + tree-kill (see :func:`_kill_process_tree`) plus a bounded 1s post-kill drain; if the pipes are still held after that, they're abandoned (the orphaned reader threads are daemonic and cost nothing). - The normal-path spawn contract mirrors the previous ``run`` call byte-for-byte: - PIPE/PIPE/DEVNULL, ``text`` with UTF-8 ``errors="replace"`` decoding, and the - hidden-window ``creationflags`` on Windows only. On POSIX the probe is - additionally placed in its own process group (``process_group=0``, - Python ≥3.11) so timeout cleanup can take down descendants — credential - helpers, ``git-remote-https``, hook children — with the launcher instead of - orphaning them (see :func:`_kill_git_process_tree`; port of - openai/codex#36793). ``process_group`` only changes which group the child - belongs to; it does not detach the terminal or alter the fast path. + Spawn contract: PIPE/PIPE/DEVNULL, ``text`` with UTF-8 ``errors="replace"``, + and hidden-window ``creationflags`` on Windows only. On POSIX the probe is + placed in its own process group so timeout cleanup can kill descendants. + ``stdin`` is always ``DEVNULL`` so ACP/TUI JSON-RPC hosts that keep stdin + open cannot stall the probe. + + On timeout / communicate failure returns ``CompletedProcess`` with + ``returncode=-1`` and empty stdout (does not raise). ``stderr`` carries a + short diagnostic (``timed out after Ns`` or the communicate error text) so + callers like ``_bash_starts`` can still populate probe-detail caches. + Spawn failures (``FileNotFoundError``, ...) propagate so callers can + distinguish "not installed" from "timed out". """ - _popen_kwargs: dict = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {"process_group": 0} - try: - proc = subprocess.Popen( - list(argv), - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - stdin=subprocess.DEVNULL, - text=True, - encoding="utf-8", - errors="replace", - **_popen_kwargs, - ) - except Exception: - return "" + _popen_kwargs: dict = ( + {"creationflags": windows_hide_flags()} + if IS_WINDOWS + else {"process_group": 0} + ) + if env is not None: + _popen_kwargs["env"] = dict(env) + proc = subprocess.Popen( + list(argv), + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + stdin=subprocess.DEVNULL, + text=True, + encoding="utf-8", + errors="replace", + **_popen_kwargs, + ) try: - stdout, _ = proc.communicate(timeout=timeout) - except Exception: + stdout, stderr = proc.communicate(timeout=timeout) + except Exception as exc: # Timeout OR any other communicate() failure (torn-down pipe, decode # error): terminate the child + descendants and drain bounded. Leaving # it running would leak the same suspended-descendant class this guards. - _kill_git_process_tree(proc) + _kill_process_tree(proc) try: proc.communicate(timeout=1) except Exception: pass + if isinstance(exc, subprocess.TimeoutExpired): + detail = f"timed out after {timeout}s" + else: + detail = (str(exc) or type(exc).__name__)[:2000] + return subprocess.CompletedProcess(list(argv), -1, "", detail) + return subprocess.CompletedProcess( + list(argv), + proc.returncode if proc.returncode is not None else -1, + stdout or "", + stderr or "", + ) + + +def bounded_git_probe(argv: Sequence[str], *, timeout: float) -> str: + """Run a short, throwaway ``git`` probe and return stripped stdout, or ``""`` + on ANY failure (nonzero exit, timeout, spawn error, decode error). + + Thin fail-open wrapper around :func:`bounded_captured_run` for + ``tui_gateway.git_probe.run_git`` and ``agent.coding_context._git``. + """ + try: + result = bounded_captured_run(argv, timeout=timeout) + except Exception: return "" - return stdout.strip() if proc.returncode == 0 else "" + return result.stdout.strip() if result.returncode == 0 else "" diff --git a/tests/test_windows_subprocess_no_window_flags.py b/tests/test_windows_subprocess_no_window_flags.py index a3d67af7dff8..1c45aba18b64 100644 --- a/tests/test_windows_subprocess_no_window_flags.py +++ b/tests/test_windows_subprocess_no_window_flags.py @@ -1,6 +1,8 @@ from __future__ import annotations +import os import subprocess +import sys from pathlib import Path from types import SimpleNamespace @@ -10,6 +12,35 @@ _CREATE_NO_WINDOW = 0x08000000 +def _run_with_retained_stdin(cmd, *, env, cwd, timeout=10): + """Run *cmd* while keeping the stdin writer open (ACP JSON-RPC-like).""" + proc = subprocess.Popen( + cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + cwd=cwd, + env=env, + ) + try: + try: + proc.wait(timeout=timeout) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + stderr = proc.stderr.read() if proc.stderr else "" + raise AssertionError( + f"helper hung under retained stdin writer (stderr={stderr!r})" + ) + stdout = proc.stdout.read() if proc.stdout else "" + stderr = proc.stderr.read() if proc.stderr else "" + return subprocess.CompletedProcess(cmd, proc.returncode, stdout, stderr) + finally: + if proc.stdin: + proc.stdin.close() + + class _Completed: def __init__(self, stdout: str | bytes = "ok\n", returncode: int = 0): self.stdout = stdout @@ -138,22 +169,121 @@ def boom(cmd, **kwargs): assert _subprocess_compat.bounded_git_probe(["git", "-C", "/repo", "status"], timeout=1.5) == "" +def test_bounded_captured_run_fast_path_spawn_contract_windows(monkeypatch): + """``bounded_captured_run`` keeps PIPE/PIPE/DEVNULL + hide flags on Windows.""" + from hermes_cli import _subprocess_compat + spawns = [] + class _FakePopen: + def __init__(self, cmd, **kwargs): + spawns.append((cmd, kwargs)) + self.returncode = 0 + def communicate(self, timeout=None): + return ("ok\n", "warn\n") + def kill(self): # pragma: no cover + raise AssertionError("kill() must not run when the child returns in time") + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", True) + monkeypatch.setattr(_subprocess_compat, "windows_hide_flags", lambda: _CREATE_NO_WINDOW) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _FakePopen) + result = _subprocess_compat.bounded_captured_run( + ["bash", "--noprofile", "--norc", "-c", "true"], timeout=15 + ) + assert result.returncode == 0 + assert result.stdout == "ok\n" + assert result.stderr == "warn\n" + assert len(spawns) == 1 + _cmd, kwargs = spawns[0] + assert kwargs["stdout"] == subprocess.PIPE + assert kwargs["stderr"] == subprocess.PIPE + assert kwargs["stdin"] == subprocess.DEVNULL + assert kwargs["text"] is True + assert kwargs["encoding"] == "utf-8" + assert kwargs["errors"] == "replace" + assert kwargs["creationflags"] == _CREATE_NO_WINDOW +def test_bounded_captured_run_timeout_tree_kills_on_windows(monkeypatch): + """Timeout path: taskkill tree first, then kill fallback, then bounded drain.""" + from hermes_cli import _subprocess_compat + events = [] + class _HangingPopen: + def __init__(self, cmd, **kwargs): + self.returncode = None + self.pid = 5150 + def communicate(self, timeout=None): + events.append(f"comm:{timeout}") + if timeout != 1: + raise subprocess.TimeoutExpired(cmd="bash", timeout=timeout) + return ("", "") + def kill(self): + events.append("kill") + def fake_run(cmd, **kwargs): + if cmd and cmd[0] == "taskkill": + events.append(("taskkill", list(cmd))) + return _Completed() + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", True) + monkeypatch.setattr(_subprocess_compat, "windows_hide_flags", lambda: _CREATE_NO_WINDOW) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _HangingPopen) + monkeypatch.setattr(_subprocess_compat.subprocess, "run", fake_run) + + result = _subprocess_compat.bounded_captured_run(["bash", "-c", "true"], timeout=15) + assert result.returncode == -1 + assert result.stdout == "" + assert result.stderr == "timed out after 15s" + # Windows: walk the live tree before killing the launcher, then drain. + assert events == [ + "comm:15", + ("taskkill", ["taskkill", "/T", "/F", "/PID", "5150"]), + "kill", + "comm:1", + ] +def test_bounded_git_probe_timeout_returns_empty(monkeypatch): + """Wrapper must fail open to "" when the shared helper times out.""" + from hermes_cli import _subprocess_compat + + def _timeout_run(argv, *, timeout, env=None): + return subprocess.CompletedProcess(list(argv), -1, "", f"timed out after {timeout}s") + + monkeypatch.setattr(_subprocess_compat, "bounded_captured_run", _timeout_run) + assert _subprocess_compat.bounded_git_probe(["git", "status"], timeout=1.5) == "" + + +def test_bounded_captured_run_returns_under_retained_stdin(): + """ACP/TUI hosts keep stdin open; probes must use DEVNULL or hang forever.""" + blocker = ( + "import sys\n" + "# Block if stdin is an open pipe with no EOF (inherited ACP stdin).\n" + "sys.stdin.buffer.read(1)\n" + "raise SystemExit(0)\n" + ) + helper = ( + "from hermes_cli._subprocess_compat import bounded_captured_run\n" + "import sys\n" + f"r = bounded_captured_run([sys.executable, '-c', {blocker!r}], timeout=5)\n" + "print(r.returncode)\n" + ) + repo_root = str(Path(__file__).resolve().parents[1]) + proc = _run_with_retained_stdin( + [sys.executable, "-c", helper], + env={**os.environ, "PYTHONPATH": repo_root}, + cwd=repo_root, + timeout=10, + ) + assert proc.returncode == 0, proc.stderr + assert proc.stdout.strip() == "0" @pytest.mark.windows_only diff --git a/tests/tools/test_find_shell.py b/tests/tools/test_find_shell.py index e84fdef0ce13..4458eee7b660 100644 --- a/tests/tools/test_find_shell.py +++ b/tests/tools/test_find_shell.py @@ -8,6 +8,8 @@ import os import platform import subprocess +import sys +from pathlib import Path from unittest.mock import patch import pytest @@ -102,6 +104,144 @@ def test_find_bash_still_prefers_bash(self): assert len(result) > 0 +def _run_with_retained_stdin(cmd, *, env, cwd, timeout=10): + """Run *cmd* while keeping the stdin writer open (ACP JSON-RPC-like). + + Unlike ``subprocess.run(input="")``, this does not close the pipe writer + until the child exits — so a nested probe that inherits stdin and reads + from it will hang unless it was spawned with ``stdin=DEVNULL``. + """ + proc = subprocess.Popen( + cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + cwd=cwd, + env=env, + ) + try: + try: + proc.wait(timeout=timeout) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + stderr = proc.stderr.read() if proc.stderr else "" + raise AssertionError( + f"helper hung under retained stdin writer (stderr={stderr!r})" + ) + stdout = proc.stdout.read() if proc.stdout else "" + stderr = proc.stderr.read() if proc.stderr else "" + return subprocess.CompletedProcess(cmd, proc.returncode, stdout, stderr) + finally: + if proc.stdin: + proc.stdin.close() + + +class TestBashStartsBoundedProbe: + """Windows tool startup hangs if ``_bash_starts`` uses unbounded run().""" + + def test_bash_starts_uses_bounded_captured_run(self, tmp_path, monkeypatch): + import tools.environments.local as local_mod + + bash = tmp_path / "bash" + bash.write_text("", encoding="utf-8") + bash.chmod(0o755) + local_mod._bash_starts_cache.clear() + captured: dict = {} + + def _fake_bounded(argv, *, timeout, env=None): + captured["argv"] = list(argv) + captured["timeout"] = timeout + captured["env"] = env + return subprocess.CompletedProcess(list(argv), 0, "", "") + + monkeypatch.setattr(local_mod, "bounded_captured_run", _fake_bounded) + assert local_mod._bash_starts(str(bash)) is True + assert captured["timeout"] == 15 + assert captured["argv"][:3] == [str(bash), "--noprofile", "--norc"] + assert "-c" in captured["argv"] + + def test_bash_starts_timeout_returns_false_with_detail(self, tmp_path, monkeypatch): + import tools.environments.local as local_mod + + bash = tmp_path / "bash" + bash.write_text("", encoding="utf-8") + bash.chmod(0o755) + local_mod._bash_starts_cache.clear() + local_mod._bash_probe_details_cache.clear() + + def _fake_bounded(argv, *, timeout, env=None): + return subprocess.CompletedProcess( + list(argv), -1, "", f"timed out after {timeout}s" + ) + + monkeypatch.setattr(local_mod, "bounded_captured_run", _fake_bounded) + assert local_mod._bash_starts(str(bash)) is False + assert "timed out after 15s" in local_mod._bash_probe_details_cache[str(bash)] + + def test_bash_starts_returns_under_retained_stdin(self, tmp_path): + """Regression: retained open stdin must not stall the bash probe.""" + if sys.platform == "win32": + # CreateProcess cannot run a shebang script. Drive the same + # DEVNULL contract through _bash_starts by rewriting the spawn to a + # python stdin-blocker while keeping the real bounded_captured_run. + bash = tmp_path / "bash.exe" + bash.write_text("", encoding="utf-8") + blocker = ( + "import sys\n" + "sys.stdin.buffer.read(1)\n" + "raise SystemExit(0)\n" + ) + helper = ( + "import sys\n" + "from hermes_cli import _subprocess_compat as sc\n" + "from tools.environments import local as local_mod\n" + "orig = sc.bounded_captured_run\n" + f"blocker = {blocker!r}\n" + "def _wrap(argv, *, timeout, env=None):\n" + " assert argv[:3] == [sys.argv[1], '--noprofile', '--norc']\n" + " return orig([sys.executable, '-c', blocker], timeout=timeout, env=env)\n" + "sc.bounded_captured_run = _wrap\n" + "local_mod.bounded_captured_run = _wrap\n" + "local_mod._bash_starts_cache.clear()\n" + "print(local_mod._bash_starts(sys.argv[1]))\n" + ) + repo_root = str(Path(__file__).resolve().parents[2]) + proc = _run_with_retained_stdin( + [sys.executable, "-c", helper, str(bash)], + env={**os.environ, "PYTHONPATH": repo_root}, + cwd=repo_root, + timeout=10, + ) + else: + fake_bash = tmp_path / "bash" + fake_bash.write_text( + "#!{}\n" + "import sys\n" + "# Block if stdin is an open pipe with no EOF (inherited ACP stdin).\n" + "sys.stdin.buffer.read(1)\n" + "sys.exit(0)\n".format(sys.executable), + encoding="utf-8", + ) + fake_bash.chmod(0o755) + + helper = ( + "from tools.environments.local import _bash_starts, _bash_starts_cache\n" + "_bash_starts_cache.clear()\n" + f"print(_bash_starts({str(fake_bash)!r}))\n" + ) + repo_root = str(Path(__file__).resolve().parents[2]) + proc = _run_with_retained_stdin( + [sys.executable, "-c", helper], + env={**os.environ, "PYTHONPATH": repo_root}, + cwd=repo_root, + timeout=10, + ) + assert proc.returncode == 0, proc.stderr + assert proc.stdout.strip() == "True" + + class TestFindBashSkipsBrokenCustomPath: """Stale HERMES_GIT_BASH_PATH must not brick Windows terminal startup.""" @@ -137,22 +277,23 @@ class TestGitBashExternalProgramProbe: def test_probe_runs_external_msys_programs(self, monkeypatch): """``_bash_starts`` builds the same external-program probe argv on - every host, so this stays on the Linux runner with ``subprocess.run`` - mocked — no platform faking needed.""" + every host, so this stays on the Linux runner with + ``bounded_captured_run`` mocked — no platform faking needed.""" import tools.environments.local as local_mod local_mod._bash_starts_cache.clear() local_mod._bash_probe_details_cache.clear() calls = [] - def fake_run(argv, **kwargs): - calls.append((argv, kwargs)) - return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + def fake_bounded(argv, *, timeout, env=None): + calls.append((list(argv), timeout, env)) + return subprocess.CompletedProcess(list(argv), 0, stdout="", stderr="") - monkeypatch.setattr(local_mod.subprocess, "run", fake_run) + monkeypatch.setattr(local_mod, "bounded_captured_run", fake_bounded) assert local_mod._bash_starts(r"C:\Git\bin\bash.exe") is True assert calls[0][0][-1] == "/usr/bin/true; /usr/bin/cat --version >/dev/null" + assert calls[0][1] == 15 @pytest.mark.windows_only def test_aslr_failure_surfaces_targeted_windows_command( diff --git a/tools/environments/local.py b/tools/environments/local.py index 330d46c53dd6..29efaf72be37 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -15,7 +15,7 @@ from pathlib import Path from tools.environments.base import BaseEnvironment, _pipe_stdin -from hermes_cli._subprocess_compat import windows_hide_flags +from hermes_cli._subprocess_compat import bounded_captured_run, windows_hide_flags _IS_WINDOWS = platform.system() == "Windows" @@ -908,12 +908,14 @@ def _bash_starts(bash: str) -> bool: return cached try: - result = subprocess.run( + # Same Windows post-timeout deadlock class as git probes: bash -c + # spawns MSYS ``true``/``cat`` children; ``subprocess.run(timeout=)`` + # can hang forever in unbounded post-kill communicate(). Use the + # shared bounded Popen + tree-kill + 1s drain helper instead. + # stdin=DEVNULL is part of that contract (ACP/TUI JSON-RPC hosts). + result = bounded_captured_run( [bash, "--noprofile", "--norc", "-c", _BASH_EXTERNAL_PROGRAM_PROBE], - capture_output=True, - text=True, encoding="utf-8", errors="replace", timeout=15, - creationflags=windows_hide_flags() if _IS_WINDOWS else 0, ) ok = result.returncode == 0 if not ok: