diff --git a/agent/coding_context.py b/agent/coding_context.py index db38ab3daa8a..4a0cb8410308 100644 --- a/agent/coding_context.py +++ b/agent/coding_context.py @@ -55,13 +55,12 @@ import json import logging import os import re -import subprocess import tempfile from dataclasses import dataclass from pathlib import Path from typing import Any, Optional -from hermes_cli._subprocess_compat import IS_WINDOWS, windows_hide_flags +from hermes_cli._subprocess_compat import bounded_git_probe logger = logging.getLogger("hermes.coding_context") @@ -689,18 +688,14 @@ def _enabled_mcp_servers(config: Optional[dict[str, Any]]) -> list[str]: def _git(cwd: Path, *args: str) -> str: - _popen_kwargs = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {} - try: - out = subprocess.run( - ["git", "-C", str(cwd), *args], - capture_output=True, - text=True, - timeout=_GIT_TIMEOUT, - **_popen_kwargs, - ) - except (OSError, subprocess.SubprocessError): - return "" - return out.stdout.strip() if out.returncode == 0 else "" + """``git -C `` → stripped stdout, or ``""`` on any failure. + + Uses the shared :func:`bounded_git_probe` so the post-kill cleanup is bounded + on Windows — a plain ``subprocess.run(timeout=...)`` here deadlocked the agent + turn inside ``build_coding_workspace_block`` when a killed git left a suspended + descendant holding the pipe handles (issue #66037). + """ + return bounded_git_probe(["git", "-C", str(cwd), *args], timeout=_GIT_TIMEOUT) def _parse_status(porcelain: str) -> tuple[dict[str, str], dict[str, int]]: diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py index 607a9a3e6a4d..4086d42f303b 100644 --- a/hermes_cli/_subprocess_compat.py +++ b/hermes_cli/_subprocess_compat.py @@ -28,6 +28,7 @@ guarantee. from __future__ import annotations import shutil +import subprocess import sys from typing import Sequence @@ -38,6 +39,7 @@ __all__ = [ "windows_detach_flags_without_breakaway", "windows_hide_flags", "windows_detach_popen_kwargs", + "bounded_git_probe", ] @@ -232,3 +234,98 @@ def windows_detach_popen_kwargs() -> dict: if IS_WINDOWS: return {"creationflags": windows_detach_flags()} return {"start_new_session": True} + + +# ----------------------------------------------------------------------------- +# Bounded, fail-open git probing (Windows post-kill deadlock guard) +# ----------------------------------------------------------------------------- + + +def _kill_git_process_tree(proc: "subprocess.Popen") -> None: + """Best-effort terminate *proc* and, on Windows, its descendants. + + ``proc.kill()`` alone only terminates the PATH-resolved ``git`` launcher; 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. + + 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, + already reaped); an unhandled raise here would escape the caller's ``except`` + handler and break that contract. The ``taskkill`` spawn itself cannot + re-enter the deadlock class it fixes: it captures no pipes (DEVNULL), so its + own timeout cleanup has no reader threads to join. + """ + try: + 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). + + 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``). + + 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). + + 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 + 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. + """ + _popen_kwargs = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {} + 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 "" + try: + stdout, _ = proc.communicate(timeout=timeout) + except Exception: + # 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) + try: + proc.communicate(timeout=1) + except Exception: + pass + return "" + return stdout.strip() if proc.returncode == 0 else "" diff --git a/tests/test_windows_subprocess_no_window_flags.py b/tests/test_windows_subprocess_no_window_flags.py index ae13ba1a44f4..4c02f5769903 100644 --- a/tests/test_windows_subprocess_no_window_flags.py +++ b/tests/test_windows_subprocess_no_window_flags.py @@ -35,37 +35,289 @@ def _spawns(captured, *needles): ] -def test_tui_gateway_git_probe_hides_git_windows(monkeypatch): - from tui_gateway import git_probe +def _is_git_spawn(cmd) -> bool: + """True only for a ``git -C ...`` spawn. - captured = [] + ``bounded_git_probe`` lives in ``hermes_cli._subprocess_compat`` and both + probe call sites delegate to it, so these tests patch + ``_subprocess_compat.subprocess.Popen`` — which is the shared ``subprocess`` + module singleton, i.e. a process-wide patch. Any unrelated daemon spawn + (e.g. an import-time update-check thread) must stay benign and out of the + recorded spawns, mirroring the ``_spawns`` scoping the other tests use. + """ + return bool(cmd) and cmd[:2] == ["git", "-C"] + + +def _make_fake_popen(spawns, *, stdout="ok\n", returncode=0): + """Fast-path Popen stand-in: git returns within the budget.""" + + class _FakePopen: + def __init__(self, cmd, **kwargs): + if _is_git_spawn(cmd): + spawns.append((cmd, kwargs)) + self.returncode = returncode + + def communicate(self, timeout=None): + return (stdout, "") + + def kill(self): # pragma: no cover - never reached on the fast path + raise AssertionError("kill() must not run when git returns in time") + + return _FakePopen + + +def test_bounded_git_probe_fast_path_spawn_contract_windows(monkeypatch): + """The normal-path spawn contract survives the run()->Popen rewrite: + PIPE/PIPE/DEVNULL, text + utf-8/replace, hidden-window flags on Windows.""" + from hermes_cli import _subprocess_compat + + spawns = [] + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", True) + monkeypatch.setattr(_subprocess_compat, "windows_hide_flags", lambda: _CREATE_NO_WINDOW) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _make_fake_popen(spawns, stdout="main\n")) + + out = _subprocess_compat.bounded_git_probe( + ["git", "-C", "C:/repo", "branch", "--show-current"], timeout=1.5 + ) + assert out == "main" + assert len(spawns) == 1, spawns + cmd, kwargs = spawns[0] + assert cmd == ["git", "-C", "C:/repo", "branch", "--show-current"] + 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_git_probe_no_hide_flags_off_windows(monkeypatch): + from hermes_cli import _subprocess_compat + + spawns = [] + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", False) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _make_fake_popen(spawns, stdout="main\n")) + + assert _subprocess_compat.bounded_git_probe(["git", "-C", "/repo", "status"], timeout=1.5) == "main" + assert len(spawns) == 1, spawns + assert "creationflags" not in spawns[0][1] + + +def test_bounded_git_probe_nonzero_returncode_returns_empty(monkeypatch): + from hermes_cli import _subprocess_compat + + spawns = [] + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", False) + monkeypatch.setattr( + _subprocess_compat.subprocess, + "Popen", + _make_fake_popen(spawns, stdout="garbage-should-not-leak\n", returncode=1), + ) + + assert _subprocess_compat.bounded_git_probe(["git", "-C", "/repo", "status"], timeout=1.5) == "" + + +def test_bounded_git_probe_timeout_kills_and_returns_empty(monkeypatch): + """A hung git is killed and cleaned up with a *bounded* second + communicate(), and the probe returns "" — never subprocess.run()'s + unbounded post-kill reader-thread join, which on Windows deadlocks when a + suspended descendant git.exe retains the captured handles and blocks Desktop + agent initialization behind it (issues #68609 / #66037).""" + from hermes_cli import _subprocess_compat + + events = [] + + class _HangingPopen: + def __init__(self, cmd, **kwargs): + self._probe = _is_git_spawn(cmd) + self.returncode = None + self.pid = 4242 + + def communicate(self, timeout=None): + if not self._probe: + return ("", "") + events.append(f"comm:{timeout}") + if timeout != 1: + raise subprocess.TimeoutExpired(cmd="git", timeout=timeout) + return ("", "") # bounded post-kill drain succeeds + + def kill(self): + if self._probe: + events.append("kill") + + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", False) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _HangingPopen) + + assert _subprocess_compat.bounded_git_probe(["git", "-C", "/repo", "status"], timeout=1.5) == "" + assert events == ["comm:1.5", "kill", "comm:1"] + + +def test_bounded_git_probe_timeout_tree_kills_on_windows(monkeypatch): + """On Windows the timeout path must escalate past ``proc.kill()`` to + ``taskkill /T /F`` so the suspended descendant git.exe holding the pipe + writers dies too — otherwise the bounded drain can't reach EOF and the + process + reader threads leak per fired timeout (the #68609 leak).""" + from hermes_cli import _subprocess_compat + + taskkills = [] + + class _HangingPopen: + def __init__(self, cmd, **kwargs): + self._probe = _is_git_spawn(cmd) + self.returncode = None + self.pid = 4242 + + def communicate(self, timeout=None): + if self._probe and timeout != 1: + raise subprocess.TimeoutExpired(cmd="git", timeout=timeout) + return ("", "") + + def kill(self): + pass def fake_run(cmd, **kwargs): - captured.append((cmd, kwargs)) - return _Completed(stdout="main\n") + taskkills.append((cmd, kwargs)) + return _Completed() - monkeypatch.setattr(git_probe, "IS_WINDOWS", True) - monkeypatch.setattr(git_probe, "windows_hide_flags", lambda: _CREATE_NO_WINDOW) - monkeypatch.setattr(git_probe.subprocess, "run", fake_run) + 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) + + assert _subprocess_compat.bounded_git_probe(["git", "-C", "C:/repo", "status"], timeout=1.5) == "" + kills = [c for c, _ in taskkills if c and c[0] == "taskkill"] + assert kills == [["taskkill", "/T", "/F", "/PID", "4242"]], taskkills + assert taskkills[0][1].get("creationflags") == _CREATE_NO_WINDOW + + +def test_bounded_git_probe_kill_failure_still_fails_open(monkeypatch): + """kill() raising (access denied, already-reaped) must not escape — the + contract is "" on ANY failure. A raise inside the except handler would + otherwise propagate.""" + from hermes_cli import _subprocess_compat + + class _UnkillablePopen: + def __init__(self, cmd, **kwargs): + self._probe = _is_git_spawn(cmd) + self.returncode = None + self.pid = 4242 + + def communicate(self, timeout=None): + if self._probe and timeout != 1: + raise subprocess.TimeoutExpired(cmd="git", timeout=timeout) + return ("", "") + + def kill(self): + if self._probe: + raise OSError("access denied") + + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", False) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _UnkillablePopen) + + assert _subprocess_compat.bounded_git_probe(["git", "-C", "/repo", "status"], timeout=1.5) == "" + + +def test_bounded_git_probe_nontimeout_failure_kills_child(monkeypatch): + """A non-timeout communicate() failure (torn-down pipe, decode error) must + still terminate the child and fail open, not leave it running.""" + from hermes_cli import _subprocess_compat + + events = [] + + class _BrokenPipePopen: + def __init__(self, cmd, **kwargs): + self._probe = _is_git_spawn(cmd) + self.returncode = None + self.pid = 4242 + + def communicate(self, timeout=None): + if not self._probe: + return ("", "") + if timeout != 1: + raise ValueError("I/O operation on closed file") + events.append("drain") + return ("", "") + + def kill(self): + if self._probe: + events.append("kill") + + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", False) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _BrokenPipePopen) + + assert _subprocess_compat.bounded_git_probe(["git", "-C", "/repo", "status"], timeout=1.5) == "" + assert events == ["kill", "drain"] + + +def test_bounded_git_probe_cleanup_failure_is_swallowed(monkeypatch): + """If the bounded post-kill drain itself still times out (descendant keeps + the handles), the probe abandons the pipes and honours the ""-on-failure + contract instead of hanging.""" + from hermes_cli import _subprocess_compat + + class _StuckPopen: + def __init__(self, cmd, **kwargs): + self._probe = _is_git_spawn(cmd) + self.returncode = None + self.pid = 4242 + + def communicate(self, timeout=None): + if self._probe: + raise subprocess.TimeoutExpired(cmd="git", timeout=timeout or 0) + return ("", "") + + def kill(self): + pass + + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", False) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _StuckPopen) + + assert _subprocess_compat.bounded_git_probe(["git", "-C", "/repo", "status"], timeout=1.5) == "" + + +def test_bounded_git_probe_spawn_failure_returns_empty(monkeypatch): + """A spawn failure (git not on PATH) fails open to "".""" + from hermes_cli import _subprocess_compat + + def boom(cmd, **kwargs): + raise FileNotFoundError("git not found") + + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", False) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", boom) + + assert _subprocess_compat.bounded_git_probe(["git", "-C", "/repo", "status"], timeout=1.5) == "" + + +def test_tui_gateway_git_probe_delegates_to_bounded_probe(monkeypatch): + """run_git wires cwd/args through the shared bounded helper (hidden-window + flags reach the spawn on Windows) and preserves its own timeout.""" + from tui_gateway import git_probe + from hermes_cli import _subprocess_compat + + spawns = [] + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", True) + monkeypatch.setattr(_subprocess_compat, "windows_hide_flags", lambda: _CREATE_NO_WINDOW) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _make_fake_popen(spawns, stdout="main\n")) assert git_probe.run_git("C:/repo", "branch", "--show-current") == "main" + assert len(spawns) == 1, spawns + cmd, kwargs = spawns[0] + assert cmd == ["git", "-C", "C:/repo", "branch", "--show-current"] + assert kwargs["creationflags"] == _CREATE_NO_WINDOW + assert kwargs["stdin"] == subprocess.DEVNULL - git_calls = _spawns(captured, "branch", "--show-current") - assert git_calls == [ - ( - ["git", "-C", "C:/repo", "branch", "--show-current"], - { - "capture_output": True, - "text": True, - "encoding": "utf-8", - "errors": "replace", - "timeout": git_probe._GIT_TIMEOUT, - "check": False, - "stdin": subprocess.DEVNULL, - "creationflags": _CREATE_NO_WINDOW, - }, - ) - ] + +def test_tui_gateway_git_probe_empty_cwd_short_circuits(monkeypatch): + """run_git returns "" for a falsy cwd without spawning git.""" + from tui_gateway import git_probe + from hermes_cli import _subprocess_compat + + def boom(*a, **k): # pragma: no cover - must not be called + raise AssertionError("git must not spawn for an empty cwd") + + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", boom) + assert git_probe.run_git("", "branch", "--show-current") == "" def test_tui_gateway_fuzzy_file_listing_hides_git_windows(monkeypatch): @@ -94,21 +346,23 @@ def test_tui_gateway_fuzzy_file_listing_hides_git_windows(monkeypatch): assert ls_files[0][1].get("creationflags") == _CREATE_NO_WINDOW -def test_coding_context_git_hides_git_windows(monkeypatch): +def test_coding_context_git_delegates_to_bounded_probe(monkeypatch): + """_git wires cwd/args through the shared bounded helper (hidden-window flags + reach the spawn on Windows), stringifying the Path cwd.""" from agent import coding_context + from hermes_cli import _subprocess_compat - captured = [] - - def fake_run(cmd, **kwargs): - captured.append((cmd, kwargs)) - return _Completed(stdout="clean\n") - - monkeypatch.setattr(coding_context, "IS_WINDOWS", True) - monkeypatch.setattr(coding_context, "windows_hide_flags", lambda: _CREATE_NO_WINDOW) - monkeypatch.setattr(coding_context.subprocess, "run", fake_run) + spawns = [] + monkeypatch.setattr(_subprocess_compat, "IS_WINDOWS", True) + monkeypatch.setattr(_subprocess_compat, "windows_hide_flags", lambda: _CREATE_NO_WINDOW) + monkeypatch.setattr(_subprocess_compat.subprocess, "Popen", _make_fake_popen(spawns, stdout="clean\n")) assert coding_context._git(Path("C:/repo"), "status", "--short") == "clean" - assert captured[0][1]["creationflags"] == _CREATE_NO_WINDOW + assert len(spawns) == 1, spawns + cmd, kwargs = spawns[0] + assert cmd == ["git", "-C", str(Path("C:/repo")), "status", "--short"] + assert kwargs["creationflags"] == _CREATE_NO_WINDOW + assert kwargs["stdin"] == subprocess.DEVNULL def test_context_reference_git_and_rg_hide_windows(monkeypatch): diff --git a/tui_gateway/git_probe.py b/tui_gateway/git_probe.py index 72582ebf5dc5..693ac417032c 100644 --- a/tui_gateway/git_probe.py +++ b/tui_gateway/git_probe.py @@ -25,13 +25,12 @@ mutation. from __future__ import annotations import os -import subprocess import threading import time from collections.abc import Iterable from concurrent.futures import ThreadPoolExecutor -from hermes_cli._subprocess_compat import IS_WINDOWS, windows_hide_flags +from hermes_cli._subprocess_compat import bounded_git_probe _GIT_TIMEOUT = 1.5 _WARM_WORKERS = 8 @@ -44,25 +43,16 @@ _NEG_TTL = 30.0 def run_git(cwd: str, *args: str) -> str: - """``git -C `` → stripped stdout, or ``""`` on any failure.""" + """``git -C `` → stripped stdout, or ``""`` on any failure. + + Uses the shared :func:`bounded_git_probe` so the post-kill cleanup is bounded + on Windows — a plain ``subprocess.run(timeout=...)`` here deadlocked Desktop + session readiness when a killed git left a suspended descendant holding the + pipe handles (issue #68609). + """ if not cwd: return "" - _popen_kwargs = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {} - try: - result = subprocess.run( - ["git", "-C", cwd, *args], - capture_output=True, - text=True, - encoding="utf-8", - errors="replace", - timeout=_GIT_TIMEOUT, - check=False, - stdin=subprocess.DEVNULL, - **_popen_kwargs, - ) - return result.stdout.strip() if result.returncode == 0 else "" - except Exception: - return "" + return bounded_git_probe(["git", "-C", cwd, *args], timeout=_GIT_TIMEOUT) def branch(cwd: str) -> str: