mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(mcp): reap orphaned stdio MCP children on ungraceful parent death
A stdio MCP server (e.g. `npx -y mcp-remote <url>`) is spawned as a direct
child of the Hermes process. Existing teardown (MCPServerTask.shutdown() /
_kill_orphaned_mcp_children()) reaps it correctly on a clean exit, but a
kill -9 / crash / force-quit of the Hermes process skips that path entirely
-- the child (and its own descendants, e.g. mcp-remote's spawned node
process) is orphaned and keeps running. Repeated ungraceful restarts pile up
N orphaned processes racing to hold the same upstream SSE session, producing
errors like 'Invalid request parameters' on legitimate reconnects.
macOS/Linux have no portable equivalent of prctl(PR_SET_PDEATHSIG) at the
Python subprocess level, so this adds a thin supervisor
(tools/mcp_stdio_watchdog.py) that:
- execs the real command as its own child in its own process group
- passes stdin/stdout/stderr through untouched (MCP stdio protocol
talks directly over those streams)
- polls the original spawning PID with the same orphan-detection
algorithm already proven in tui_gateway/slash_worker.py (ppid
comparison + psutil creation-time guard against PID reuse)
- SIGTERM-then-SIGKILL's the child's process group the moment the
original parent is gone
Wired into _run_stdio via a new _wrap_command_with_watchdog() helper,
POSIX-only (matches the existing killpg-based cleanup's platform scope),
fails open (any error resolving pid/create-time falls back to the
unwrapped command) so this can never be the reason a working MCP server
stops starting.
Verified: reproduced the exact orphan scenario standalone (fake parent
process spawns watchdog + fake long-running MCP child, kill -9 the fake
parent, confirm the watchdog reaps the child within its poll window with
zero leaked processes). Updated test_mcp_tool_issue_948.py's resolved-path
assertion to check the watchdog-wrapped command instead of the raw
resolved binary. Full test_mcp_tool.py + test_mcp_stability.py +
test_mcp_tool_issue_948.py suite: 232 passed. Full -k mcp sweep across the
whole test tree: 1003 passed, 2 skipped, 0 failed.
This commit is contained in:
parent
4638f3b433
commit
5089c84dbf
3 changed files with 213 additions and 1 deletions
|
|
@ -1,5 +1,6 @@
|
|||
import asyncio
|
||||
import os
|
||||
import sys
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
|
|
@ -119,8 +120,18 @@ def test_run_stdio_uses_resolved_command_and_prepended_path(tmp_path):
|
|||
server = MCPServerTask("srv")
|
||||
await server.start({"command": "npx", "args": ["-y", "pkg"], "env": {"PATH": "/usr/bin"}})
|
||||
|
||||
# The real (resolved) command no longer reaches StdioServerParameters
|
||||
# directly -- it's now wrapped in the parent-death watchdog
|
||||
# supervisor (tools/mcp_stdio_watchdog.py) so an ungraceful exit of
|
||||
# this process can't orphan it. Assert the resolved npx path and
|
||||
# its args still flow through correctly as the watchdog's target
|
||||
# command, preserving this test's original path-resolution intent.
|
||||
call_kwargs = mock_params.call_args.kwargs
|
||||
assert call_kwargs["command"] == str(npx_path)
|
||||
assert call_kwargs["command"] == sys.executable
|
||||
assert call_kwargs["args"][0].endswith("mcp_stdio_watchdog.py")
|
||||
assert "--" in call_kwargs["args"]
|
||||
sep = call_kwargs["args"].index("--")
|
||||
assert call_kwargs["args"][sep + 1:] == [str(npx_path), "-y", "pkg"]
|
||||
assert call_kwargs["env"]["PATH"].split(os.pathsep)[0] == str(node_bin)
|
||||
|
||||
await server.shutdown()
|
||||
|
|
|
|||
156
tools/mcp_stdio_watchdog.py
Normal file
156
tools/mcp_stdio_watchdog.py
Normal file
|
|
@ -0,0 +1,156 @@
|
|||
#!/usr/bin/env python3
|
||||
"""Parent-death watchdog supervisor for stdio MCP subprocesses.
|
||||
|
||||
Problem this fixes (#TBD): a stdio MCP server (e.g. ``npx -y mcp-remote
|
||||
<url>``) is spawned as a direct child of the Hermes process. Hermes's own
|
||||
teardown path (``MCPServerTask.shutdown()`` / ``_kill_orphaned_mcp_children``
|
||||
at final exit) reaps it cleanly on a *graceful* exit. But if the spawning
|
||||
Hermes process dies hard — ``kill -9``, an OS-level crash, a force-quit of
|
||||
the TUI/desktop app — that teardown code never runs, and the child (plus any
|
||||
of its own descendants, e.g. mcp-remote's spawned ``node`` process) is
|
||||
orphaned. macOS has no direct equivalent of Linux's
|
||||
``prctl(PR_SET_PDEATHSIG)`` to make the kernel auto-kill a child when its
|
||||
parent dies, so nothing reaps these until the next Hermes startup's opt-in
|
||||
``_kill_orphaned_mcp_children()`` sweep — which only runs if something calls
|
||||
it. Repeated ungraceful session restarts can pile up N orphaned processes,
|
||||
all racing to hold the same upstream SSE session, producing errors like
|
||||
"Invalid request parameters" / "Received request before initialization was
|
||||
complete" on the *legitimate* new connection.
|
||||
|
||||
Fix: don't spawn the MCP server command directly. Spawn this supervisor
|
||||
instead, which:
|
||||
1. execs the real command as its own child (own process group via
|
||||
``start_new_session``, so it doesn't inherit the supervisor's
|
||||
controlling terminal weirdly and so we can killpg it cleanly);
|
||||
2. transparently passes stdin/stdout/stderr through — the MCP stdio
|
||||
protocol talks directly over those pipes, so the supervisor must be a
|
||||
no-op relay, not a bytes-in-the-middle proxy;
|
||||
3. runs a background thread that polls the ORIGINAL parent PID using the
|
||||
exact same orphan-detection algorithm already proven in
|
||||
``tui_gateway/slash_worker.py`` (``_is_orphaned``): compare current
|
||||
``getppid()`` against the recorded original, and guard PID reuse via
|
||||
``psutil`` process creation time;
|
||||
4. the instant the original parent is gone, terminates the real child's
|
||||
process group (SIGTERM, grace period, then SIGKILL) and exits.
|
||||
|
||||
This is intentionally a thin, dependency-light script (``psutil`` only,
|
||||
already a hard dependency via ``tui_gateway/slash_worker.py``) so it starts
|
||||
fast and can't itself become a resource leak.
|
||||
|
||||
Usage (see ``tools/mcp_tool.py::_run_stdio``)::
|
||||
|
||||
python3 -m tools.mcp_stdio_watchdog \\
|
||||
--ppid <original_parent_pid> --create-time <original_parent_create_time> \\
|
||||
-- <real_command> <arg1> <arg2> ...
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import os
|
||||
import signal
|
||||
import subprocess
|
||||
import sys
|
||||
import threading
|
||||
import time
|
||||
|
||||
try:
|
||||
import psutil
|
||||
except ImportError: # pragma: no cover - psutil is a hard dependency elsewhere
|
||||
psutil = None
|
||||
|
||||
_POLL_INTERVAL_S = 2.0
|
||||
_TERM_GRACE_S = 3.0
|
||||
|
||||
|
||||
def _is_orphaned(original_ppid: int, parent_create_time: float, getppid=os.getppid) -> bool:
|
||||
"""Mirrors ``tui_gateway.slash_worker._is_orphaned`` exactly.
|
||||
|
||||
True once the process that spawned us is gone. Never trusts a bare
|
||||
``getppid() == 1`` check (Linux reparents orphans to a subreaper, not
|
||||
always PID 1), and guards against PID reuse via the recorded creation
|
||||
time of the original parent.
|
||||
"""
|
||||
if getppid() != original_ppid:
|
||||
return True
|
||||
if psutil is None:
|
||||
# No reliable staleness check available; fall back to the ppid
|
||||
# comparison alone (still catches the common case).
|
||||
return False
|
||||
try:
|
||||
if not psutil.pid_exists(original_ppid):
|
||||
return True
|
||||
return psutil.Process(original_ppid).create_time() != parent_create_time
|
||||
except psutil.Error:
|
||||
return True
|
||||
|
||||
|
||||
def _terminate_process_group(proc: subprocess.Popen) -> None:
|
||||
"""Best-effort SIGTERM-then-SIGKILL of the child's process group."""
|
||||
try:
|
||||
pgid = os.getpgid(proc.pid)
|
||||
except (ProcessLookupError, OSError):
|
||||
return
|
||||
for sig in (signal.SIGTERM, signal.SIGKILL):
|
||||
try:
|
||||
os.killpg(pgid, sig)
|
||||
except (ProcessLookupError, PermissionError, OSError):
|
||||
return
|
||||
try:
|
||||
proc.wait(timeout=_TERM_GRACE_S)
|
||||
return
|
||||
except subprocess.TimeoutExpired:
|
||||
continue
|
||||
|
||||
|
||||
def _watchdog_loop(proc: subprocess.Popen, original_ppid: int, parent_create_time: float) -> None:
|
||||
while proc.poll() is None:
|
||||
if _is_orphaned(original_ppid, parent_create_time):
|
||||
_terminate_process_group(proc)
|
||||
return
|
||||
time.sleep(_POLL_INTERVAL_S)
|
||||
|
||||
|
||||
def main(argv: list[str] | None = None) -> int:
|
||||
parser = argparse.ArgumentParser(
|
||||
description="Parent-death watchdog for a stdio MCP subprocess.",
|
||||
)
|
||||
parser.add_argument("--ppid", type=int, required=True)
|
||||
parser.add_argument("--create-time", type=float, required=True)
|
||||
parser.add_argument("command", nargs=argparse.REMAINDER)
|
||||
args = parser.parse_args(argv)
|
||||
|
||||
real_argv = list(args.command)
|
||||
if real_argv and real_argv[0] == "--":
|
||||
real_argv = real_argv[1:]
|
||||
if not real_argv:
|
||||
print("mcp_stdio_watchdog: no command given after '--'", file=sys.stderr)
|
||||
return 2
|
||||
|
||||
# New process group so we can killpg() the whole tree the real command
|
||||
# may spawn (e.g. mcp-remote's own child `node` process), without
|
||||
# touching our own group or the (already-gone) original parent's.
|
||||
proc = subprocess.Popen(
|
||||
real_argv,
|
||||
stdin=sys.stdin,
|
||||
stdout=sys.stdout,
|
||||
stderr=sys.stderr,
|
||||
start_new_session=True,
|
||||
)
|
||||
|
||||
watchdog = threading.Thread(
|
||||
target=_watchdog_loop,
|
||||
args=(proc, args.ppid, args.create_time),
|
||||
daemon=True,
|
||||
)
|
||||
watchdog.start()
|
||||
|
||||
try:
|
||||
return proc.wait()
|
||||
except KeyboardInterrupt:
|
||||
_terminate_process_group(proc)
|
||||
return 130
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
|
|
@ -616,6 +616,41 @@ def _resolve_stdio_command(command: str, env: dict) -> tuple[str, dict]:
|
|||
return resolved_command, resolved_env
|
||||
|
||||
|
||||
def _wrap_command_with_watchdog(command: str, args: list) -> tuple[str, list]:
|
||||
"""Wrap a stdio MCP server command in the parent-death watchdog supervisor.
|
||||
|
||||
See ``tools/mcp_stdio_watchdog.py`` module docstring for the full
|
||||
rationale. Returns the (command, args) unchanged on any platform/failure
|
||||
where the wrap can't safely apply, so this can never be the reason a
|
||||
previously-working MCP server stops starting.
|
||||
"""
|
||||
if os.name != "posix":
|
||||
# Relies on process groups (os.getpgid/os.killpg); no POSIX
|
||||
# equivalent wired up here yet, matching the existing killpg-based
|
||||
# orphan cleanup's platform scope (Windows falls back to plain
|
||||
# os.kill there too).
|
||||
return command, args
|
||||
try:
|
||||
my_pid = os.getpid()
|
||||
try:
|
||||
import psutil
|
||||
create_time = psutil.Process(my_pid).create_time()
|
||||
except ImportError:
|
||||
create_time = time.time()
|
||||
except Exception:
|
||||
# Never let watchdog bookkeeping failure block a real MCP connection.
|
||||
return command, args
|
||||
watchdog_args = [
|
||||
os.path.join(os.path.dirname(os.path.abspath(__file__)), "mcp_stdio_watchdog.py"),
|
||||
"--ppid", str(my_pid),
|
||||
"--create-time", repr(create_time),
|
||||
"--",
|
||||
command,
|
||||
*args,
|
||||
]
|
||||
return sys.executable, watchdog_args
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# MCP ImageContent block → Hermes MEDIA tag
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -1914,6 +1949,16 @@ class MCPServerTask:
|
|||
safe_env = _build_safe_env(user_env)
|
||||
command, safe_env = _resolve_stdio_command(command, safe_env)
|
||||
|
||||
# Wrap the real command in a parent-death watchdog supervisor so an
|
||||
# ungraceful exit of this Hermes process (kill -9, crash, force-quit)
|
||||
# can't leave the stdio MCP child (and its own descendants, e.g.
|
||||
# mcp-remote's spawned `node`) running forever. On a clean exit,
|
||||
# MCPServerTask.shutdown() / _kill_orphaned_mcp_children() still do
|
||||
# the reaping as before -- this only covers the case where that code
|
||||
# never gets to run. POSIX-only (relies on process groups); no-op
|
||||
# elsewhere, matching existing killpg-based cleanup's platform scope.
|
||||
command, args = _wrap_command_with_watchdog(command, args)
|
||||
|
||||
# Check package against OSV malware database before spawning.
|
||||
# Run off the event loop (the urllib HTTPS call is blocking) and bound
|
||||
# it with a wall-clock timeout so a stalled SSL handshake can't freeze
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue