fix(cli): bound the Windows process-scan probes so a slow WMI scan cannot wedge hermes update (#87134)
subprocess.run(capture_output=True, timeout=N) is not hang-safe on Windows: after the 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) holding duplicated pipe handles keeps the pipes from EOF and the join never returns. _scan_gateway_pids() runs its wmic / Get-CimInstance Win32_Process scans exactly that way, and on machines where the full process scan genuinely exceeds its 10/15s budget (cold WMI on first boot, ARM VMs, heavy Update/AV activity) hermes update wedged forever inside _pause_windows_gateways_for_update() before printing a single line — observed live on a fresh Windows 11 ARM64 VM with a faulthandler stack pinning the main thread in subprocess._communicate and only a conhost.exe child surviving. The single-flight update lock then blocks retries until the wedged process is killed by hand. This is the same deadlock class bounded_git_probe already fixed for git probes (#68609 / #66037). Generalize that proven pattern into a shared bounded_probe_run() — explicit communicate(timeout), kill_process_tree on failure, bounded 1s drain, then abandon the daemonic readers — and migrate the whole call-site class onto it: - hermes_cli/gateway.py _scan_gateway_pids (the site that hung; reached from hermes update, cron, gateway restart/status, dashboard) - hermes_cli/dashboard_procs.py wmic scan (same shape, reached on update) - hermes_cli/claw.py tasklist + PowerShell probes (same shape; its try/except cannot catch a hang because a hang raises nothing) - bounded_git_probe now delegates to bounded_probe_run (identical contract, one copy of the cleanup logic) Unlike bounded_git_probe, bounded_probe_run returns the CompletedProcess (or None) rather than collapsing to stdout, because the gateway scan branches on returncode to trip its wmic -> powershell fallback. Tests: tests/hermes_cli/test_bounded_probe_run.py covers success, nonzero-exit passthrough, spawn failure, bounded timeout (fails against the old unbounded semantics — verified by sabotage), errors= decoding, DEVNULL stdin, POSIX process-group placement, and the bounded_git_probe delegation contract. Existing test_git_probe_tree_kill.py passes unchanged against the delegated implementation. Closes #87134
This commit is contained in:
@@ -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.
|
||||
|
||||
+12
-6
@@ -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
|
||||
|
||||
@@ -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"):
|
||||
|
||||
+31
-36
@@ -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 []
|
||||
|
||||
@@ -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"
|
||||
Reference in New Issue
Block a user