fix(windows): share one bounded, tree-killing git probe across both call sites (#68997)

subprocess.run(["git", ...], timeout=...) deadlocks 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 — leaking a process +
two reader threads per fired timeout (the accumulating git.exe load behind
Windows Defender CPU spikes).

Two fail-open probe call sites had this identical flaw:
  - tui_gateway/git_probe.py::run_git — on the Desktop agent-build path
    (_start_agent_build -> _session_info -> branch() -> run_git), where the
    hang turned an optional branch label into "agent initialization timed
    out" (#68609).
  - agent/coding_context.py::_git — hangs the agent turn inside
    build_coding_workspace_block under an ACP host (#66037).

Consolidate both onto one shared bounded_git_probe() in
hermes_cli/_subprocess_compat.py (both files already import from there, so
no new import surface):
  - explicit communicate(timeout), then on ANY failure a tree-kill —
    proc.kill() AND, on Windows, best-effort taskkill /T /F so the suspended
    descendant that holds the pipe writers dies too — plus a bounded 1s
    post-kill drain; if the pipes are still held they're abandoned (the
    orphaned reader threads are daemonic and cost nothing).
  - fail open to "" on every path: spawn error, timeout, kill() raising
    (access denied / already reaped — a raise inside the except handler
    previously escaped the contract), and non-timeout communicate() failures
    now also terminate the child instead of leaving it running.
  - the taskkill spawn can't re-enter the deadlock class: it captures no
    pipes (DEVNULL), so its own timeout cleanup has no reader threads to join.

Normal-path spawn contract is preserved byte-for-byte: PIPE/PIPE/DEVNULL,
text + utf-8 errors="replace", hidden-window creationflags on Windows only,
nonzero returncode -> "". Each call site keeps its own timeout (1.5s / 2.5s).

Supersedes #68622 (Sora-bluesky — git_probe fix + tree-kill) and #66038
(iamwongeeeee — coding_context fix), folding both into one shared helper so
the two sites can't drift and every timeout tree-kills the descendant. Tests
consolidated onto the helper, incl. the previously-missing assertion that a
Windows timeout escalates to taskkill /T /F.

Co-authored-by: Sora-bluesky <sora.bluesky.dev@gmail.com>
Co-authored-by: iamwongeeeee <wykim777@naver.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
brooklyn! 2026-07-21 20:18:43 -05:00 committed by GitHub
parent 75be8fb463
commit 967e078ae4
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 404 additions and 68 deletions

View file

@ -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 <cwd> <args>`` → 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]]:

View file

@ -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 ""

View file

@ -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 <cwd> ...`` 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):

View file

@ -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 <cwd> <args>`` → stripped stdout, or ``""`` on any failure."""
"""``git -C <cwd> <args>`` → 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: