fix(mcp): register stdio MCP helper children in the spawn ledger and reap orphans (#61514)
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.
This commit is contained in:
@@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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()
|
||||
@@ -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).
|
||||
|
||||
Reference in New Issue
Block a user