kshitij
4e3de140c1
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
113 lines
4.4 KiB
Python
113 lines
4.4 KiB
Python
"""``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"
|