diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 624d0f10109f7..4596c9759cd84 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -10172,6 +10172,72 @@ def _is_electron_packaged_web_dist(path: str) -> bool: return "app.asar" in path.replace("\\", "/") +# ── Parent-death watchdog for the desktop-spawned `serve` backend ───────── +# Desktop Electron spawns this backend and tears it down in `before-quit` +# (SIGTERM, then forceKillProcessTree over the primary and the pool). That +# teardown is correct but structurally cannot cover a force-quit / SIGKILL / +# fatal GPU abort — main.ts says so itself ("FATAL GPU aborts skip +# before-quit"). macOS has no PR_SET_PDEATHSIG (documented in +# tools/mcp_stdio_watchdog.py), so the kernel reparents us to pid 1 rather +# than reaping us and we keep our 127.0.0.1 LISTEN socket and ~100 MB +# forever. Three such orphans accumulated in one 3-minute restart burst. +# No parent-side fix can close this — the parent's code is exactly what did +# not run — so the child reaps itself, as tui_gateway/slash_worker.py does. + +def _env_float(name: str, default: float) -> float: + """Parse a float env knob, falling back to *default* on absent/malformed + values. A bare ``float(os.environ.get(...))`` would raise ValueError at + import time on a typo (e.g. ``HERMES_SERVE_WATCHDOG_POLL_S=2s``) and take + down every `hermes` invocation, not just the watchdog.""" + raw = os.environ.get(name) + if not raw: + return default + try: + return float(raw) + except (TypeError, ValueError): + return default + + +# Env-overridable so the integration test can drive sub-second timing. +_SERVE_WATCHDOG_POLL_S = max(0.05, _env_float("HERMES_SERVE_WATCHDOG_POLL_S", 2.0)) + + +def _serve_is_orphaned(original_ppid, getppid=os.getppid) -> bool: + """Return whether this backend no longer has its original POSIX parent.""" + return getppid() != original_ppid + + +def _should_start_serve_watchdog(headless_backend, env, os_name) -> bool: + """Gate the watchdog to the one launch shape that can leak. + + - ``headless_backend`` — cmd_dashboard backs BOTH `dashboard` and `serve`; + a human's foreground `hermes dashboard` must never self-reap. + - ``HERMES_DESKTOP=1`` — only the desktop's own backend is in scope. A + deliberate ``nohup hermes serve &`` legitimately reparents to pid 1 when + its shell exits, and killing that would be a regression, not a fix. + - POSIX — the named-profile re-exec below is ``os.execvpe`` here, which + preserves pid/ppid so the recorded value stays valid; on Windows it is + ``subprocess.Popen``, where it would not. Same gate tools/mcp_tool.py + uses for its watchdog wrap. + """ + return bool(headless_backend) and env.get("HERMES_DESKTOP") == "1" and os_name == "posix" + + +def _start_serve_parent_death_watchdog(original_ppid) -> None: + def _loop(): + while not _serve_is_orphaned(original_ppid): + _time.sleep(_SERVE_WATCHDOG_POLL_S) + # os._exit, not sys.exit: uvicorn's loop owns the main thread and would + # swallow a SystemExit raised here. The kernel releases the LISTEN + # socket either way, and a backend whose parent is gone has no shutdown + # work worth flushing. + os._exit(0) + + threading.Thread( + target=_loop, name="serve-parent-death-watchdog", daemon=True + ).start() + + def cmd_dashboard(args): """Start the web UI server, or (with --stop/--status) manage running ones.""" _token_file = getattr(args, "ssh_session_token_file", None) @@ -10323,6 +10389,12 @@ def cmd_dashboard(args): else: os.execvpe(sys.executable, reexec_argv, env) + # Past the re-exec, so the ppid we record is the one we will actually be + # orphaned from. os.execvpe above preserves pid/ppid; the Windows branch + # spawns a fresh process instead, which is why the gate is POSIX-only. + if _should_start_serve_watchdog(_headless_backend, os.environ, os.name): + _start_serve_parent_death_watchdog(os.getppid()) + if _token_file: _ssh_session_token = _read_ssh_session_token_file(_token_file) diff --git a/tests/hermes_cli/test_serve_parent_death_watchdog.py b/tests/hermes_cli/test_serve_parent_death_watchdog.py new file mode 100644 index 0000000000000..66f1b11c33a2a --- /dev/null +++ b/tests/hermes_cli/test_serve_parent_death_watchdog.py @@ -0,0 +1,211 @@ +"""Contract for the parent-death watchdog on the headless ``hermes serve`` backend. + +The desktop Electron app spawns its backend as ``hermes serve --host 127.0.0.1 +--port 0`` and tears it down in ``before-quit``. That teardown is correct but +structurally cannot cover a force-quit / SIGKILL / fatal GPU abort, because the +parent's code never runs. macOS has no ``PR_SET_PDEATHSIG``, so the kernel +reparents the backend to pid 1 instead of reaping it and it keeps its LISTEN +socket and its ~100 MB forever. Three such orphans were recovered from one +3-minute restart burst. + +No parent-side fix can close this, so the backend reaps itself: it records its +ppid at startup and exits once that ppid changes. These tests pin the +predicate, the three gates that keep it off every other launch shape, its +placement after the re-exec, and — end to end — that a real serve-shaped child +actually dies when its parent is SIGKILLed. + +Mirrors ``tests/test_slash_worker_watchdog.py``, whose watchdog this copies. +""" + +from __future__ import annotations + +import inspect +import os +import signal +import subprocess +import sys +import time +from pathlib import Path + +import pytest + +from hermes_cli import main as main_mod + +REPO_ROOT = Path(__file__).resolve().parents[2] + + +# ── the orphan predicate ────────────────────────────────────────────────── + + +def test_is_orphaned_true_when_ppid_changes(): + # Our parent went away and we were reparented to init/a subreaper. + assert main_mod._serve_is_orphaned(1234, getppid=lambda: 1) is True + + +def test_is_orphaned_false_when_direct_parent_is_unchanged(): + original_ppid = 1234 + assert main_mod._serve_is_orphaned(original_ppid, getppid=lambda: original_ppid) is False + + +# ── the three gates ─────────────────────────────────────────────────────── + + +def test_watchdog_runs_for_a_desktop_spawned_serve_on_posix(): + assert main_mod._should_start_serve_watchdog( + headless_backend=True, env={"HERMES_DESKTOP": "1"}, os_name="posix" + ) is True + + +def test_watchdog_skips_the_interactive_dashboard(): + # cmd_dashboard backs BOTH `dashboard` and `serve`. A human's foreground + # `hermes dashboard` must never self-reap. + assert main_mod._should_start_serve_watchdog( + headless_backend=False, env={"HERMES_DESKTOP": "1"}, os_name="posix" + ) is False + + +def test_watchdog_skips_a_standalone_serve(): + # `nohup hermes serve &` legitimately reparents to pid 1 when the launching + # shell exits — that is a deliberate daemon, not an orphan. Only the + # desktop's own backend (HERMES_DESKTOP=1) is in scope. + assert main_mod._should_start_serve_watchdog( + headless_backend=True, env={}, os_name="posix" + ) is False + + +def test_watchdog_skips_windows(): + # The profile re-exec is os.execvpe on POSIX (pid/ppid preserved, so the + # recorded ppid stays valid) but subprocess.Popen on Windows, where it + # would not be. Same gate tools/mcp_tool.py uses. + assert main_mod._should_start_serve_watchdog( + headless_backend=True, env={"HERMES_DESKTOP": "1"}, os_name="nt" + ) is False + + +# ── contract + placement ────────────────────────────────────────────────── + + +def test_watchdog_contract_has_no_create_time_plumbing(): + assert list(inspect.signature(main_mod._serve_is_orphaned).parameters) == [ + "original_ppid", + "getppid", + ] + assert list( + inspect.signature(main_mod._start_serve_parent_death_watchdog).parameters + ) == ["original_ppid"] + + +def test_cmd_dashboard_starts_the_watchdog_after_the_reexec(): + # The ppid must be recorded on the far side of the named-profile re-exec. + # Recorded before it, the value would be the pre-exec parent's — and on the + # Windows branch (subprocess.Popen, not execvpe) a whole different process. + src = inspect.getsource(main_mod.cmd_dashboard) + assert "_start_serve_parent_death_watchdog" in src, ( + "cmd_dashboard never starts the watchdog — the helper is dead code" + ) + assert src.index("os.execvpe") < src.index("_start_serve_parent_death_watchdog"), ( + "watchdog must be started AFTER the re-exec block, not before" + ) + + +# ── end to end: a real orphan reaps itself ──────────────────────────────── + + +def _child_source(pidfile: Path) -> str: + """A serve-shaped child: holds a LISTEN socket, runs the real watchdog.""" + return ( + "import os, socket, sys, time\n" + f"sys.path.insert(0, {str(REPO_ROOT)!r})\n" + "from hermes_cli.main import _start_serve_parent_death_watchdog\n" + # The orphans each held a live 127.0.0.1 LISTEN socket; keep one so the + # test exercises the same shape rather than a bare sleeper. + "s = socket.socket(); s.bind(('127.0.0.1', 0)); s.listen(8)\n" + "_start_serve_parent_death_watchdog(os.getppid())\n" + f"open({str(pidfile)!r}, 'w').write(str(os.getpid()))\n" + "while True: time.sleep(0.05)\n" + ) + + +def _parent_source(child_script: Path) -> str: + # Exit if the child dies, so a child that fails to start surfaces as an + # immediate "parent died before the child came up" instead of a timeout. + return ( + "import subprocess, sys\n" + f"sys.exit(subprocess.Popen([sys.executable, {str(child_script)!r}]).wait())\n" + ) + + +def _alive(pid: int) -> bool: + try: + os.kill(pid, 0) + except (ProcessLookupError, PermissionError): + return False + return True + + +def _read_pid(pidfile: Path) -> int | None: + try: + return int(pidfile.read_text().strip()) + except (FileNotFoundError, ValueError): + return None + + +def _reap(pid: int | None) -> None: + if pid is None: + return + try: + os.kill(pid, signal.SIGKILL) + except Exception: + # Cleanup must never raise: it runs in `finally`, and an exception + # here would replace the AssertionError that names the real failure. + pass + + +# Real SIGKILL delivery is the whole point — a mocked signal cannot orphan a +# process. Both PIDs are spawned by this test; the child is deliberately +# reparented to init, which is exactly what the subtree guard refuses. +@pytest.mark.live_system_guard_bypass +def test_serve_child_self_reaps_when_its_parent_is_sigkilled(tmp_path): + pidfile = tmp_path / "child.pid" + child_script = tmp_path / "child.py" + parent_script = tmp_path / "parent.py" + child_script.write_text(_child_source(pidfile)) + parent_script.write_text(_parent_source(child_script)) + + env = dict(os.environ) + env["HERMES_DESKTOP"] = "1" + env["HERMES_SERVE_WATCHDOG_POLL_S"] = "0.1" # read at import; keeps this fast + env["PYTHONPATH"] = os.pathsep.join( + [str(REPO_ROOT), env.get("PYTHONPATH", "")] + ).rstrip(os.pathsep) + + parent = subprocess.Popen([sys.executable, str(parent_script)], env=env) + child_pid = None + try: + deadline = time.monotonic() + 60 + while time.monotonic() < deadline: + child_pid = _read_pid(pidfile) + if child_pid is not None: + break + if parent.poll() is not None: + raise AssertionError("parent died before the child came up") + time.sleep(0.05) + assert child_pid is not None, "child never started" + assert _alive(child_pid), "child exited before the parent was killed" + + # Force-quit / fatal abort: the parent's teardown code never runs. + os.kill(parent.pid, signal.SIGKILL) + parent.wait(timeout=10) + + deadline = time.monotonic() + 15 + while time.monotonic() < deadline: + if not _alive(child_pid): + return # self-reaped + time.sleep(0.05) + raise AssertionError( + f"orphaned serve child {child_pid} survived its parent's SIGKILL — " + "this is the leak the watchdog exists to prevent" + ) + finally: + _reap(parent.pid) + _reap(child_pid if child_pid is not None else _read_pid(pidfile))