From e1e72f109c9540d6ed2aceeaf1b5bfcacbab5efc Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:02:20 -0700 Subject: [PATCH] fix(mcp): register stdio MCP helper children in the spawn ledger and reap orphans (#61514) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stdio MCP helper subprocesses (npx/binary servers) never import Hermes code, so they could not self-register in the machine spawn ledger and an unclean parent exit left them running invisibly forever. - process_identity.register_child(pid, purpose): ledger mirror of register_self for spawned children — records the CHILD (pid, create_time) with this process as spawner. Refuses pid-only entries a PID reuse could forge. Writes go through the single _append_entry path under _LEDGER_LOCK (prune + atomic tmp/replace unchanged). - 'mcp-helper' added to REAPABLE_PURPOSES so the updater's _ledger_reapable_backend_pids rung flows helpers through its existing spawner_is_dead gate (live spawner => never reaped). - tools/mcp_tool.py: best-effort register_child(pid, 'mcp-helper') at the post-spawn PID capture; never breaks MCP startup. - reap_orphaned_mcp_helpers(): startup sweep mirroring _reap_orphaned_desktop_local_serves but ledger-driven — kills only helpers whose recorded spawner is PROVABLY dead, with a create_time re-check at kill time. Wired next to the desktop serve reap in web_server.py. --- hermes_cli/process_identity.py | 127 +++++++++++- hermes_cli/web_server.py | 13 ++ tests/hermes_cli/test_mcp_helper_ledger.py | 220 +++++++++++++++++++++ tools/mcp_tool.py | 17 ++ 4 files changed, 376 insertions(+), 1 deletion(-) create mode 100644 tests/hermes_cli/test_mcp_helper_ledger.py diff --git a/hermes_cli/process_identity.py b/hermes_cli/process_identity.py index 81deb1d42e..c84015d7c7 100644 --- a/hermes_cli/process_identity.py +++ b/hermes_cli/process_identity.py @@ -53,7 +53,7 @@ LEDGER_FILENAME = "spawn-ledger.json" #: Purposes a reaper may treat as "safe to kill when the owner is gone". #: Interactive processes (chat, REPLs) are deliberately NOT in this set. -REAPABLE_PURPOSES = frozenset({"serve", "dashboard", "gateway"}) +REAPABLE_PURPOSES = frozenset({"serve", "dashboard", "gateway", "mcp-helper"}) _IS_WINDOWS = platform.system() == "Windows" @@ -298,6 +298,17 @@ def register_self( except Exception: pass + return _append_entry(entry) + + +def _append_entry(entry: LedgerEntry) -> bool: + """Prune dead entries and append ``entry`` — the ONLY ledger write path. + + Serialized under ``_LEDGER_LOCK`` with an atomic tmp+replace, exactly as + ``register_self`` has always written (kept single so #91660's lock- + serialization guarantees hold: no writer ever touches the file outside + this function). + """ path = _ledger_path() with _LEDGER_LOCK: entries = _read_ledger(path) @@ -325,6 +336,60 @@ def register_self( return False +def register_child( + pid: int, + purpose: str, + *, + project_root: Optional[Path] = None, +) -> bool: + """Record a CHILD process this process just spawned. Best-effort. + + Mirror of :func:`register_self` for children that cannot register + themselves (stdio MCP helper subprocesses, #61514: arbitrary + ``npx``/binary servers never import Hermes code). The entry records the + child's ``(pid, create_time)`` with THIS process as the spawner, so + reapers get the same positive-identity contract: + + - a live helper whose spawner is still alive is never reaped + (``spawner_is_dead`` → ``False``); + - a helper whose spawner ``(pid, create_time)`` is provably gone is a + reapable orphan. + + Never raises; returns ``False`` when the child already exited (no + provable ``create_time`` means no forge-proof identity — don't record a + pid-only entry a reuse could impersonate) or the write failed. + """ + try: + pid = int(pid) + except (TypeError, ValueError): + return False + if pid <= 0: + return False + try: + import psutil + + child_create: Optional[float] = float(psutil.Process(pid).create_time()) + except Exception: + return False + entry = LedgerEntry( + pid=pid, + create_time=child_create, + purpose=purpose, + install=install_id(project_root), + spawner_pid=os.getpid(), + spawner_create=_own_create_time(), + registered_at=time.time(), + argv="", + ) + try: + import psutil + + entry.argv = " ".join(psutil.Process(pid).cmdline()[:10]) + except Exception: + pass + return _append_entry(entry) + + def ledger_entries(*, project_root: Optional[Path] = None) -> list[dict]: """Live-verified ledger entries for THIS install. @@ -369,6 +434,66 @@ def spawner_is_dead(entry: dict) -> Optional[bool]: return not alive +def reap_orphaned_mcp_helpers( + *, + project_root: Optional[Path] = None, + kill_fn=None, +) -> list[int]: + """Kill ledger-registered stdio MCP helpers whose spawner is provably dead. + + Startup-sweep rung mirroring ``_reap_orphaned_desktop_local_serves`` + (dashboard_procs.py), but ledger-driven instead of cmdline-heuristic: + a helper is reaped ONLY when + + - it has a live ``(pid, create_time)`` ledger entry for THIS install with + purpose ``mcp-helper`` (``ledger_entries`` already excludes dead/ + foreign entries), and + - its recorded spawner is **provably dead** (``spawner_is_dead`` is + ``True`` — never ``None``/unprovable, never a live spawner). + + Best-effort, never raises; returns the PIDs it terminated. ``kill_fn`` + is injectable for tests (defaults to psutil terminate→wait→kill). + """ + reaped: list[int] = [] + try: + entries = ledger_entries(project_root=project_root) + except Exception: + return reaped + own_pid = os.getpid() + for entry in entries: + try: + if entry.get("purpose") != "mcp-helper": + continue + pid = entry.get("pid") + if not isinstance(pid, int) or pid <= 0 or pid == own_pid: + continue + if spawner_is_dead(entry) is not True: + continue # live or unprovable spawner → never touch + if kill_fn is not None: + kill_fn(pid) + else: + import psutil + + proc = psutil.Process(pid) + # Re-verify identity at the moment of kill (PID-reuse guard). + create = entry.get("create_time") + if create is not None and abs( + float(proc.create_time()) - float(create) + ) >= 2.0: + continue + proc.terminate() + try: + proc.wait(timeout=2.0) + except psutil.TimeoutExpired: + proc.kill() + reaped.append(pid) + except Exception: + logger.debug("mcp-helper orphan reap failed for %s", entry, exc_info=True) + if reaped: + logger.info("reaped %d orphaned stdio MCP helper(s): %s", len(reaped), reaped) + return reaped + + # --------------------------------------------------------------------------- # Layer 3 — Windows job-object self-attach # --------------------------------------------------------------------------- diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index ce54ffbf7b..419a4bb00c 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -19825,6 +19825,19 @@ def start_server( except Exception as exc: _log.debug("orphan desktop-local serve reap skipped: %s", exc) + # Same sweep for stdio MCP helper children (#61514): ledger- + # identified helpers whose recorded spawner is provably dead are + # corpses from a prior unclean exit — reap them before this + # backend stacks a fresh MCP tree on top. Positive identity only + # (spawn ledger + spawner_is_dead); a helper whose spawner is + # alive or unprovable is never touched. + try: + from hermes_cli.process_identity import reap_orphaned_mcp_helpers + + reap_orphaned_mcp_helpers() + except Exception as exc: + _log.debug("orphan MCP helper reap skipped: %s", exc) + # tui_gateway/slash_worker.py::_start_parent_death_watchdog. No-op # for standalone `hermes serve` (no HERMES_PARENT_PID env). _start_parent_death_watchdog() diff --git a/tests/hermes_cli/test_mcp_helper_ledger.py b/tests/hermes_cli/test_mcp_helper_ledger.py new file mode 100644 index 0000000000..ab05210213 --- /dev/null +++ b/tests/hermes_cli/test_mcp_helper_ledger.py @@ -0,0 +1,220 @@ +"""Tests for stdio MCP helper children in the spawn ledger (#61514). + +Covers ``register_child`` (the ledger mirror of ``register_self`` for +subprocesses that never import Hermes code), the live-spawner protection +contract, dead-spawner reap eligibility through BOTH consumers (the updater's +``_ledger_reapable_backend_pids`` rung and the startup +``reap_orphaned_mcp_helpers`` sweep), and prune-on-write of exited children. + +Uses REAL subprocesses (``sleep``) and the real psutil so the +``(pid, create_time)`` identity pair is exercised end-to-end, with the ledger +redirected to a tmp path. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import time +from unittest.mock import patch + +import psutil +import pytest + +from hermes_cli import process_identity as pi + +pytestmark = pytest.mark.skipif( + sys.platform == "win32", reason="uses POSIX sleep children" +) + + +@pytest.fixture +def ledger(tmp_path): + path = tmp_path / pi.LEDGER_FILENAME + with patch.object(pi, "_ledger_path", return_value=path): + yield path + + +@pytest.fixture +def child(): + """A real live child process of THIS test process.""" + proc = subprocess.Popen(["sleep", "300"]) + try: + yield proc + finally: + proc.kill() + proc.wait() + + +def _dead_process_identity() -> tuple[int, float]: + """(pid, create_time) of a real process that is provably dead.""" + proc = subprocess.Popen(["sleep", "300"]) + create = psutil.Process(proc.pid).create_time() + proc.kill() + proc.wait() + # Ensure the corpse is fully reaped (no zombie ambiguity). + deadline = time.time() + 5 + while time.time() < deadline and psutil.pid_exists(proc.pid): + time.sleep(0.05) + return proc.pid, create + + +# --------------------------------------------------------------------------- +# register_child — entry shape +# --------------------------------------------------------------------------- + +def test_register_child_entry_shape(ledger, child): + assert pi.register_child(child.pid, "mcp-helper") is True + entries = json.loads(ledger.read_text(encoding="utf-8")) + assert len(entries) == 1 + e = entries[0] + assert e["pid"] == child.pid + assert e["purpose"] == "mcp-helper" + assert e["install"] == pi.install_id() + assert e["spawner_pid"] == os.getpid() + assert e["spawner_create"] == pytest.approx( + psutil.Process(os.getpid()).create_time(), abs=2.0 + ) + assert e["create_time"] == pytest.approx( + psutil.Process(child.pid).create_time(), abs=2.0 + ) + assert e["registered_at"] == pytest.approx(time.time(), abs=30.0) + # Visible through the live-verified reader too. + live = pi.ledger_entries() + assert [x["pid"] for x in live] == [child.pid] + + +def test_register_child_rejects_dead_or_invalid_pids(ledger): + dead_pid, _ = _dead_process_identity() + assert pi.register_child(dead_pid, "mcp-helper") is False + assert pi.register_child(0, "mcp-helper") is False + assert pi.register_child(-5, "mcp-helper") is False + assert pi.register_child("junk", "mcp-helper") is False # type: ignore[arg-type] + assert not ledger.exists() or json.loads(ledger.read_text()) == [] + + +def test_mcp_helper_is_reapable_purpose(): + assert "mcp-helper" in pi.REAPABLE_PURPOSES + # Interactive purposes still excluded. + assert "chat" not in pi.REAPABLE_PURPOSES + + +# --------------------------------------------------------------------------- +# Spawner liveness gate +# --------------------------------------------------------------------------- + +def test_live_spawner_protection(ledger, child): + """A helper whose spawner (this process) lives is never reap-eligible.""" + pi.register_child(child.pid, "mcp-helper") + (entry,) = pi.ledger_entries() + assert pi.spawner_is_dead(entry) is False + + killed: list[int] = [] + reaped = pi.reap_orphaned_mcp_helpers(kill_fn=killed.append) + assert reaped == [] and killed == [] + assert psutil.pid_exists(child.pid) + + +def _orphan_entry_for(child_pid: int) -> None: + """Rewrite the child's ledger entry so its spawner is a real dead process.""" + path = pi._ledger_path() + dead_pid, dead_create = _dead_process_identity() + entries = json.loads(path.read_text(encoding="utf-8")) + for e in entries: + if e["pid"] == child_pid: + e["spawner_pid"] = dead_pid + e["spawner_create"] = dead_create + path.write_text(json.dumps(entries), encoding="utf-8") + + +def test_dead_spawner_reap_eligibility(ledger, child): + pi.register_child(child.pid, "mcp-helper") + _orphan_entry_for(child.pid) + (entry,) = pi.ledger_entries() + assert pi.spawner_is_dead(entry) is True + + killed: list[int] = [] + assert pi.reap_orphaned_mcp_helpers(kill_fn=killed.append) == [child.pid] + assert killed == [child.pid] + + +def test_reap_selection_mixes_live_and_dead_spawners(ledger): + """Only the orphan is selected; the live-spawner helper is untouched.""" + live_proc = subprocess.Popen(["sleep", "300"]) + orphan_proc = subprocess.Popen(["sleep", "300"]) + try: + pi.register_child(live_proc.pid, "mcp-helper") + pi.register_child(orphan_proc.pid, "mcp-helper") + _orphan_entry_for(orphan_proc.pid) + + killed: list[int] = [] + reaped = pi.reap_orphaned_mcp_helpers(kill_fn=killed.append) + assert reaped == [orphan_proc.pid] + assert live_proc.pid not in killed + finally: + for p in (live_proc, orphan_proc): + p.kill() + p.wait() + + +def test_reap_actually_kills_orphan(ledger, child): + """Default kill path really terminates the orphaned helper.""" + pi.register_child(child.pid, "mcp-helper") + _orphan_entry_for(child.pid) + assert pi.reap_orphaned_mcp_helpers() == [child.pid] + deadline = time.time() + 5 + while time.time() < deadline and child.poll() is None: + time.sleep(0.05) + assert child.poll() is not None + + +def test_reap_ignores_non_mcp_purposes(ledger, child): + pi.register_child(child.pid, "serve-child-not-helper") + _orphan_entry_for(child.pid) + assert pi.reap_orphaned_mcp_helpers() == [] + assert psutil.pid_exists(child.pid) + + +# --------------------------------------------------------------------------- +# Updater rung (_ledger_reapable_backend_pids) flow-through +# --------------------------------------------------------------------------- + +def test_updater_ledger_rung_flows_mcp_helper(ledger, child): + from hermes_cli import update_cmd + + pi.register_child(child.pid, "mcp-helper") + matches = [(child.pid, "python", "sleep 300")] + + # Live spawner (this process) → never selected. + assert update_cmd._ledger_reapable_backend_pids(matches) == [] + + # Provably dead spawner → positively identified as reapable. + _orphan_entry_for(child.pid) + assert update_cmd._ledger_reapable_backend_pids(matches) == [child.pid] + + +# --------------------------------------------------------------------------- +# Prune-on-write of exited children +# --------------------------------------------------------------------------- + +def test_ledger_prunes_exited_children_on_next_write(ledger): + doomed = subprocess.Popen(["sleep", "300"]) + pi.register_child(doomed.pid, "mcp-helper") + doomed.kill() + doomed.wait() + deadline = time.time() + 5 + while time.time() < deadline and psutil.pid_exists(doomed.pid): + time.sleep(0.05) + + survivor = subprocess.Popen(["sleep", "300"]) + try: + pi.register_child(survivor.pid, "mcp-helper") + entries = json.loads(ledger.read_text(encoding="utf-8")) + pids = [e["pid"] for e in entries] + assert survivor.pid in pids + assert doomed.pid not in pids + finally: + survivor.kill() + survivor.wait() diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index c1c80e5adb..3654a90d39 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -3314,6 +3314,23 @@ class MCPServerTask: for _pid in new_pids: _stdio_pids[_pid] = self.name _stdio_pgids.update(new_pgids) + # Positive identity for the machine spawn ledger (#61514): + # record each helper child as (pid, create_time, + # 'mcp-helper', spawner=this process) so startup sweeps + # can reap orphans left after an unclean parent exit. + # Best-effort — never let ledger I/O break MCP startup. + for _pid in new_pids: + try: + from hermes_cli.process_identity import register_child + + register_child(_pid, "mcp-helper") + except Exception: + logger.debug( + "spawn-ledger register_child failed for MCP " + "helper pid %s", + _pid, + exc_info=True, + ) # Track the spawned children on the connection object for # fast-fail of in-flight calls when the subprocess dies # (#81995).