diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py index 66f0d90a0b..8a27dd3fca 100644 --- a/hermes_cli/_subprocess_compat.py +++ b/hermes_cli/_subprocess_compat.py @@ -44,6 +44,7 @@ __all__ = [ "windows_hide_flags", "windows_detach_popen_kwargs", "bounded_git_probe", + "bounded_probe_run", "noninteractive_git_env", ] @@ -442,6 +443,67 @@ def kill_process_tree(proc: "subprocess.Popen") -> None: pass +def bounded_probe_run( + argv: Sequence[str], + *, + timeout: float, + errors: str = "replace", +) -> "subprocess.CompletedProcess[str] | None": + """Deadlock-safe ``subprocess.run(argv, capture_output=True, timeout=...)`` + for fail-open probe call sites. Returns a ``CompletedProcess`` when the + child finished within *timeout* (any exit code), or ``None`` on spawn + failure or timeout. + + Why not ``subprocess.run``: on Windows, ``run()``'s post-timeout cleanup + calls an *unbounded* ``communicate()`` after killing the direct child. + Killing it can leave a descendant (``git.exe`` under a launcher shim, + ``conhost.exe`` under wmic/powershell) holding duplicates of the captured + stdout/stderr handles, so the pipes never reach EOF and the reader-thread + join blocks forever. The wmic / ``Get-CimInstance Win32_Process`` gateway + scan hit exactly this during ``hermes update`` on slow-WMI machines + (#87134); the git probes hit it first (#68609 / #66037). + + The bounded flow: an explicit ``communicate(timeout)``, then on any + failure a 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 spawn contract mirrors the ``run`` calls it replaces: PIPE/PIPE/DEVNULL, + ``text`` with UTF-8 decoding (*errors* configurable — the process scans use + ``"ignore"``), and the hidden-window ``creationflags`` on Windows only. On + POSIX the child is placed in its own process group (``process_group=0``, + Python ≥3.11) so timeout cleanup can take down descendants with the + launcher instead of orphaning them. + """ + _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=errors, + **_popen_kwargs, + ) + except Exception: + return None + try: + stdout, stderr = 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_process_tree(proc) + try: + proc.communicate(timeout=1) + except Exception: + pass + return None + return subprocess.CompletedProcess(list(argv), proc.returncode, stdout, stderr) + + 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). @@ -474,33 +536,10 @@ def bounded_git_probe(argv: Sequence[str], *, timeout: float) -> str: openai/codex#36793). ``process_group`` only changes which group the child belongs to; it does not detach the terminal or alter the fast path. """ - _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: + result = bounded_probe_run(argv, timeout=timeout) + if result is None or result.returncode != 0: 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_process_tree(proc) - try: - proc.communicate(timeout=1) - except Exception: - pass - return "" - return stdout.strip() if proc.returncode == 0 else "" + return (result.stdout or "").strip() # Backward-compat alias — existing call sites/tests import the historical name. diff --git a/hermes_cli/claw.py b/hermes_cli/claw.py index 8f58d9520e..8b78902a19 100644 --- a/hermes_cli/claw.py +++ b/hermes_cli/claw.py @@ -77,13 +77,19 @@ def _detect_openclaw_processes() -> list[str]: # -- process scan ------------------------------------------------------ if sys.platform == "win32": + # bounded_probe_run: a plain subprocess.run(timeout=...) can hang + # forever on Windows in post-timeout cleanup when a conhost.exe + # descendant holds duplicated pipe handles (#87134) — and a hang is + # not an exception, so the try/except here can't save the caller. + from hermes_cli._subprocess_compat import bounded_probe_run + try: for exe in ("openclaw.exe", "clawd.exe"): - result = subprocess.run( + result = bounded_probe_run( ["tasklist", "/FI", f"IMAGENAME eq {exe}"], - capture_output=True, text=True, encoding='utf-8', errors='replace', timeout=5, + timeout=5, ) - if exe in result.stdout.lower(): + if result is not None and exe in (result.stdout or "").lower(): found.append(f"process: {exe}") # Node.js-hosted OpenClaw — tasklist doesn't show command lines, @@ -93,11 +99,11 @@ def _detect_openclaw_processes() -> list[str]: 'Where-Object { $_.CommandLine -match "openclaw|clawd" } | ' 'Select-Object -First 1 ProcessId' ) - result = subprocess.run( + result = bounded_probe_run( ["powershell", "-NoProfile", "-Command", ps_cmd], - capture_output=True, text=True, encoding='utf-8', errors='replace', timeout=5, + timeout=5, ) - if result.stdout.strip(): + if result is not None and (result.stdout or "").strip(): found.append(f"node.exe process with openclaw in command line (PID {result.stdout.strip()})") except Exception: pass diff --git a/hermes_cli/dashboard_procs.py b/hermes_cli/dashboard_procs.py index 84bd48573e..3ee58ff266 100644 --- a/hermes_cli/dashboard_procs.py +++ b/hermes_cli/dashboard_procs.py @@ -76,21 +76,22 @@ def _scan_dashboard_processes( # here is errors="ignore": it prevents a reader-thread # UnicodeDecodeError from leaving result.stdout=None and turning # the later .split() into an AttributeError (#17049). - # CREATE_NO_WINDOW hides the conhost flash: this scan can run from - # the windowless pythonw.exe desktop/gateway backend during an - # update, where a bare wmic spawn would pop a console window. - from hermes_cli._subprocess_compat import windows_hide_flags + # bounded_probe_run (rather than subprocess.run with a timeout) + # keeps a slow scan from wedging the caller forever: run()'s + # post-timeout cleanup joins the pipe reader threads unbounded, + # and a conhost.exe descendant holding duplicated pipe handles + # blocks that join indefinitely (#87134). It also passes + # CREATE_NO_WINDOW: this scan can run from the windowless + # pythonw.exe desktop/gateway backend during an update, where a + # bare wmic spawn would pop a console window. + from hermes_cli._subprocess_compat import bounded_probe_run - result = subprocess.run( + result = bounded_probe_run( ["wmic", "process", "get", "ProcessId,CommandLine", "/FORMAT:LIST"], - capture_output=True, - text=True, timeout=10, - encoding="utf-8", errors="ignore", - creationflags=windows_hide_flags(), ) - if result.returncode != 0 or result.stdout is None: + if result is None or result.returncode != 0 or result.stdout is None: return [] current_cmd = "" for line in result.stdout.split("\n"): diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index 11eca304ec..cdfee12111 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -529,35 +529,35 @@ def _scan_gateway_pids( # Prefer wmic when present (fast, stable output format). On # modern Windows 11 / Win 10 late builds, wmic has been # removed as part of the WMIC deprecation — fall back to - # PowerShell's Get-CimInstance. Any OSError here (FileNotFoundError - # on missing wmic) trips the fallback. - # Hide the console window: this scan runs inside the windowless - # pythonw.exe gateway/desktop backend, so a bare wmic/powershell - # spawn would flash a conhost window on every watchdog probe. - from hermes_cli._subprocess_compat import windows_hide_flags + # PowerShell's Get-CimInstance. A spawn failure or timeout + # (result is None) trips the fallback. + # The scans go through ``bounded_probe_run`` — NOT plain + # ``subprocess.run(timeout=...)`` — because on Windows ``run()``'s + # post-timeout cleanup joins the pipe reader threads unbounded; a + # descendant (conhost.exe) holding duplicated pipe handles then + # wedges the caller forever. ``hermes update`` hung exactly there + # on slow-WMI machines where the full Win32_Process scan exceeds + # its budget (#87134). + # bounded_probe_run also hides the console window: this scan runs + # inside the windowless pythonw.exe gateway/desktop backend, so a + # bare wmic/powershell spawn would flash a conhost window on every + # watchdog probe. + from hermes_cli._subprocess_compat import bounded_probe_run - _no_window = {"creationflags": windows_hide_flags()} wmic_path = shutil.which("wmic") result = None if wmic_path is not None: - try: - result = subprocess.run( - [ - wmic_path, - "process", - "get", - "ProcessId,CommandLine", - "/FORMAT:LIST", - ], - capture_output=True, - text=True, - encoding="utf-8", - errors="ignore", - timeout=10, - **_no_window, - ) - except (OSError, subprocess.TimeoutExpired): - result = None + result = bounded_probe_run( + [ + wmic_path, + "process", + "get", + "ProcessId,CommandLine", + "/FORMAT:LIST", + ], + timeout=10, + errors="ignore", + ) if result is None or result.returncode != 0 or not (result.stdout or ""): # Fallback: PowerShell Get-CimInstance, emit LIST-style output # so the downstream parser below doesn't need to branch. @@ -572,17 +572,12 @@ def _scan_gateway_pids( " '' " "}" ) - try: - result = subprocess.run( - [powershell, "-NoProfile", "-Command", ps_cmd], - capture_output=True, - text=True, - encoding="utf-8", - errors="ignore", - timeout=15, - **_no_window, - ) - except (OSError, subprocess.TimeoutExpired): + result = bounded_probe_run( + [powershell, "-NoProfile", "-Command", ps_cmd], + timeout=15, + errors="ignore", + ) + if result is None: return [] if result.returncode != 0 or result.stdout is None: return [] diff --git a/tests/hermes_cli/test_bounded_probe_run.py b/tests/hermes_cli/test_bounded_probe_run.py new file mode 100644 index 0000000000..d2ea846f11 --- /dev/null +++ b/tests/hermes_cli/test_bounded_probe_run.py @@ -0,0 +1,112 @@ +"""``bounded_probe_run`` — deadlock-safe capture for fail-open probes (#87134). + +On Windows, ``subprocess.run(..., capture_output=True, timeout=N)`` can hang +FOREVER after its timeout fires: run()'s cleanup kills the direct child and +then joins the pipe reader threads with an unbounded ``communicate()``. A +descendant (``conhost.exe`` under wmic/powershell, ``git.exe`` under a +launcher shim) holding duplicated pipe handles keeps the pipes from EOF and +the join never returns. ``hermes update`` wedged exactly there inside +``_scan_gateway_pids`` on machines where the full ``Win32_Process`` scan +exceeds its budget. + +``bounded_probe_run`` is the shared, generalized form of the fix that +``bounded_git_probe`` already proved for git probes (#68609 / #66037): +explicit ``communicate(timeout)``, tree-kill on failure, bounded 1s drain, +then abandon. + +These tests use REAL subprocesses (no mocks) for the semantics: a mock cannot +reproduce pipe-handle inheritance or timeout behavior. The POSIX-only +descendant-survival cases live in ``test_git_probe_tree_kill.py`` and now +exercise the same code path through the ``bounded_git_probe`` delegation. +""" + +import subprocess +import sys +import time + +import pytest + +from hermes_cli._subprocess_compat import bounded_git_probe, bounded_probe_run + +_PY = sys.executable + + +def test_success_returns_completed_process(): + result = bounded_probe_run([_PY, "-c", "print('ok'); import sys; sys.exit(0)"], timeout=30) + assert result is not None + assert result.returncode == 0 + assert result.stdout.strip() == "ok" + + +def test_nonzero_exit_is_returned_not_swallowed(): + """Unlike bounded_git_probe, callers see the real returncode — the gateway + scan branches on ``returncode != 0`` to trip the wmic→powershell fallback.""" + result = bounded_probe_run([_PY, "-c", "print('partial'); import sys; sys.exit(3)"], timeout=30) + assert result is not None + assert result.returncode == 3 + assert result.stdout.strip() == "partial" + + +def test_spawn_failure_returns_none(): + result = bounded_probe_run(["definitely-not-a-real-binary-87134"], timeout=5) + assert result is None + + +def test_timeout_returns_none_within_bounded_time(): + """A child that sleeps past the timeout must produce ``None`` promptly — + timeout + tree-kill + 1s bounded drain, not an unbounded join.""" + start = time.monotonic() + result = bounded_probe_run( + [_PY, "-c", "import time; time.sleep(300)"], + timeout=1.0, + ) + elapsed = time.monotonic() - start + assert result is None + # 1s timeout + tree-kill + 1s drain + slack. The pre-fix failure mode is + # an indefinite hang, so any bound proves the property; keep it loose for + # slow CI runners. + assert elapsed < 30 + + +def test_decode_errors_configurable(): + """The process scans pass errors='ignore' (wmic emits system code page); + undecodable bytes must not raise or None out stdout (#17049 class).""" + result = bounded_probe_run( + [_PY, "-c", "import sys; sys.stdout.buffer.write(b'ok\\xff\\xfe')"], + timeout=30, + errors="ignore", + ) + assert result is not None + assert result.returncode == 0 + assert "ok" in result.stdout + + +def test_stdin_is_devnull_not_inherited(): + """A probe must never block reading the caller's stdin.""" + result = bounded_probe_run( + [_PY, "-c", "import sys; print(repr(sys.stdin.read()))"], + timeout=30, + ) + assert result is not None + assert result.returncode == 0 + assert result.stdout.strip() == "''" + + +def test_bounded_git_probe_delegates_same_contract(): + """The historical git-probe wrapper keeps its exact contract on top of + bounded_probe_run: stripped stdout on rc==0, '' on any failure.""" + assert bounded_git_probe([_PY, "-c", "print(' x ')"], timeout=30) == "x" + assert bounded_git_probe([_PY, "-c", "import sys; sys.exit(1)"], timeout=30) == "" + assert bounded_git_probe(["definitely-not-a-real-binary-87134"], timeout=5) == "" + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process-group check") +def test_posix_child_gets_own_process_group(): + """POSIX spawns use process_group=0 so timeout cleanup can killpg the + whole tree (same contract bounded_git_probe had).""" + result = bounded_probe_run( + [_PY, "-c", "import os; print(os.getpgid(0) == os.getpid())"], + timeout=30, + ) + assert result is not None + assert result.stdout.strip() == "True"