From 5089c84dbf852a43ac879217d271ce3b8df9a3b6 Mon Sep 17 00:00:00 2001 From: Sage Date: Mon, 6 Jul 2026 22:16:42 -0700 Subject: [PATCH] fix(mcp): reap orphaned stdio MCP children on ungraceful parent death A stdio MCP server (e.g. `npx -y mcp-remote `) 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. --- tests/tools/test_mcp_tool_issue_948.py | 13 ++- tools/mcp_stdio_watchdog.py | 156 +++++++++++++++++++++++++ tools/mcp_tool.py | 45 +++++++ 3 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 tools/mcp_stdio_watchdog.py diff --git a/tests/tools/test_mcp_tool_issue_948.py b/tests/tools/test_mcp_tool_issue_948.py index c258cd570c7..eadb7397a40 100644 --- a/tests/tools/test_mcp_tool_issue_948.py +++ b/tests/tools/test_mcp_tool_issue_948.py @@ -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() diff --git a/tools/mcp_stdio_watchdog.py b/tools/mcp_stdio_watchdog.py new file mode 100644 index 00000000000..46761c16337 --- /dev/null +++ b/tools/mcp_stdio_watchdog.py @@ -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 +``) 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 --create-time \\ + -- ... +""" + +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()) diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 3aff482f2e7..1d45a079027 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -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