From 0cd93f0268cd2c152c36217cc72a42c55c5c7a65 Mon Sep 17 00:00:00 2001 From: Austin Pickett Date: Fri, 25 Sep 2026 14:48:00 -0400 Subject: [PATCH] fix(mcp): kill Windows stdio MCP orphan trees (#61059) Windows has no POSIX parent-death supervisor/killpg safety net, so an ungraceful exit of the hermes process left every stdio MCP child tree (npx.cmd -> node.exe) running as orphans with ParentId=null, piling up across session restarts. - _run_stdio now attaches the process to a KILL_ON_JOB_CLOSE job object before spawning stdio children (self-guarded no-op off Windows), so the whole child tree dies with the parent at the kernel level. - Windows reaps kill the process tree (direct child + descendants) in the lifecycle orphan sweep and the spawn-ledger startup sweep, where there is no pgid to group-kill. Fixes #61059 --- hermes_cli/process_identity.py | 46 ++++++- tests/tools/test_mcp_windows_orphan_fix.py | 143 +++++++++++++++++++++ tools/mcp_tool_lifecycle.py | 30 +++++ tools/mcp_tool_transport.py | 12 ++ 4 files changed, 226 insertions(+), 5 deletions(-) create mode 100644 tests/tools/test_mcp_windows_orphan_fix.py diff --git a/hermes_cli/process_identity.py b/hermes_cli/process_identity.py index 62094c5c70..c1d9520643 100644 --- a/hermes_cli/process_identity.py +++ b/hermes_cli/process_identity.py @@ -399,11 +399,17 @@ def reap_orphaned_mcp_helpers(*, project_root: Optional[Path] = None, kill_fn=No proc = psutil.Process(pid) if not _same_incarnation(proc, entry.get("create_time")): continue # PID reused since registration - proc.terminate() - try: - proc.wait(timeout=2.0) - except psutil.TimeoutExpired: - proc.kill() + # Windows: descendants (npx.cmd → node.exe) have no pgid to group-kill and + # reparent with ParentId=null when the direct child exits first (#61059), so + # reap the whole tree. On POSIX the killpg-based sweep already reaches them. + if _IS_WINDOWS: + _kill_process_tree_windows(proc) + else: + 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) @@ -415,6 +421,36 @@ def reap_orphaned_mcp_helpers(*, project_root: Optional[Path] = None, kill_fn=No # Layer 3 — Windows job-object self-attach +def _kill_process_tree_windows(proc) -> None: + """Terminate *proc* and every still-alive descendant (npx.cmd → node.exe), Windows-only + (#61059): without a pgid there is no group-kill, and grandchildren reparent to nothing + (ParentId=null) once the direct child exits, so they must be reached through the tree.""" + import psutil + + try: + descendants = proc.children(recursive=True) + except (psutil.NoSuchProcess, psutil.AccessDenied, OSError): + descendants = [] + for child in descendants: + try: + child.terminate() + except Exception: # noqa: BLE001 - raced away or refused; keep going + pass + try: + proc.terminate() + except Exception: # noqa: BLE001 + pass + try: + _, alive = psutil.wait_procs(descendants + [proc], timeout=2.0) + except Exception: # noqa: BLE001 - broken fake/raced process; nothing more to force-kill + return + for survivor in alive: + try: + survivor.kill() + except Exception: # noqa: BLE001 + pass + + def attach_self_to_kill_on_close_job() -> bool: """Place this process in a job that dies (whole tree) when we die. Windows-only, idempotent. diff --git a/tests/tools/test_mcp_windows_orphan_fix.py b/tests/tools/test_mcp_windows_orphan_fix.py new file mode 100644 index 0000000000..00b484905a --- /dev/null +++ b/tests/tools/test_mcp_windows_orphan_fix.py @@ -0,0 +1,143 @@ +"""Tests for the Windows MCP orphan fix (#61059). + +Windows has no POSIX parent-death supervisor / killpg safety net, so orphaned +npx → node.exe trees accumulated across session restarts. The fix has two rungs: + +1. ``_run_stdio`` attaches this process to a KILL_ON_JOB_CLOSE job object before + spawning stdio children, so any child/grandchild created after the attach dies + with the parent at the kernel level — even on an ungraceful exit. +2. Windows reaps kill the whole process tree (direct child + descendants), since + there is no pgid to group-kill and grandchildren reparent with ParentId=null. + +The job-attach call itself is ctypes/Win32 and untestable off Windows; here we +verify it is invoked on the Windows spawn path and not on POSIX, plus the +best-effort contract of the tree-kill helpers. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from tools.mcp_tool import MCPServerTask, _MCP_AVAILABLE + +pytestmark = pytest.mark.skipif(not _MCP_AVAILABLE, reason="MCP SDK not installed") + + +def _run_stdio_with_mocks(os_name: str, attach_mock) -> None: + """Drive _run_stdio's pre-spawn path with the transport mocked out.""" + import tools.mcp_tool_transport as transport_mod + + mock_session = MagicMock() + mock_session.initialize = AsyncMock() + + async def _serve(session, timeout, **kwargs): + return "ok" + + task = MCPServerTask("test-win-orphan") + task._serve_session = _serve + task._session_kwargs = lambda: {} + + async def fake_preflight(name, command, args): + return command, args + + with ( + patch("hermes_cli.process_identity.attach_self_to_kill_on_close_job", attach_mock), + patch("tools.mcp_tool.StdioServerParameters"), + patch("tools.mcp_tool.stdio_client", return_value=(mock_stdio_cm := MagicMock())), + patch("tools.mcp_tool.ClientSession", return_value=(mock_session_cm := MagicMock())), + patch("tools.mcp_tool._preflight_stdio_command", fake_preflight), + patch("tools.mcp_tool_lifecycle._snapshot_child_pids", return_value=set()), + patch("tools.mcp_tool_lifecycle._kill_orphaned_mcp_children"), + patch("tools.mcp_tool_config._write_stderr_log_header"), + ): + mock_stdio_cm.__aenter__ = AsyncMock(return_value=(object(), object())) + mock_stdio_cm.__aexit__ = AsyncMock(return_value=False) + mock_session_cm.__aenter__ = AsyncMock(return_value=mock_session) + mock_session_cm.__aexit__ = AsyncMock(return_value=False) + import asyncio + + # invoke the mixin method directly: start() would enter the reconnect loop because the + # mocked session never completes a real handshake. + asyncio.run(task._run_stdio({"command": "echo", "args": ["hello"]})) + + +def test_windows_spawn_attaches_kill_on_close_job(): + """On Windows the stdio spawn path must self-attach to a kill-on-close job (#61059).""" + attach = MagicMock(return_value=True) + _run_stdio_with_mocks("nt", attach) + attach.assert_called_once_with() + + +def test_spawn_always_attempts_attach(): + """The attach call is unconditional and self-guarded (no-op off Windows); the spawn + path must invoke it so Windows sessions get job coverage without any os.name checks + that would be hard to mock at the spawn site.""" + attach = MagicMock(return_value=False) # POSIX no-op return + _run_stdio_with_mocks("posix", attach) + attach.assert_called_once_with() + + +def test_windows_attach_failure_never_blocks_spawn(): + """A failed job attach (e.g. nested-job refusal) must not fail the MCP connection.""" + attach = MagicMock(side_effect=OSError("access denied")) + _run_stdio_with_mocks("nt", attach) # raises → would fail the test + attach.assert_called_once_with() + + +class TestWindowsTreeKillHelpers: + """Best-effort contract of the tree-kill helpers (Win32 paths can't run here).""" + + def test_tree_kill_swallows_missing_process(self): + import tools.mcp_tool_lifecycle as lifecycle + + # Must not raise for a PID that raced away. (_kill_windows_process_tree carries no + # os.name gate itself — the gate lives at the _signal_mcp_process call site — so this + # exercises the helper directly without patching os.name, which breaks pathlib.) + lifecycle._kill_windows_process_tree(999999999, 15) + + def test_tree_kill_terminates_descendants(self, monkeypatch): + import tools.mcp_tool_lifecycle as lifecycle + + terminated = [] + killed = [] + + class FakeProc: + def __init__(self, pid): + self.pid = pid + + def children(self, recursive=True): + return [FakeProc(2), FakeProc(3)] + + def terminate(self): + terminated.append(self.pid) + + def kill(self): + killed.append(self.pid) + + fake_psutil = MagicMock() + fake_psutil.Process = FakeProc + fake_psutil.wait_procs = lambda procs, timeout: ([], []) # all exited gracefully + monkeypatch.setitem(__import__("sys").modules, "psutil", fake_psutil) + + lifecycle._kill_windows_process_tree(1, 15) + # SIGTERM pass: descendants only — the direct child was already signalled by the caller. + assert sorted(terminated) == [2, 3] + assert killed == [] + + # Force pass: survivors of the wait are killed. wait_procs reports everyone alive. + fake_psutil.wait_procs = lambda procs, timeout: ([], list(procs)) + lifecycle._kill_windows_process_tree(1, 9) + assert sorted(killed) == [2, 3] + + def test_ledger_tree_kill_helper_swallows_errors(self): + """_kill_process_tree_windows never raises, even for a vanished process.""" + from hermes_cli.process_identity import _kill_process_tree_windows + + class Boom: + def children(self, recursive=True): + raise OSError("gone") + + def terminate(self): + raise OSError("gone") + + _kill_process_tree_windows(Boom()) # must not raise diff --git a/tools/mcp_tool_lifecycle.py b/tools/mcp_tool_lifecycle.py index c8f4e27e18..a755658ff5 100644 --- a/tools/mcp_tool_lifecycle.py +++ b/tools/mcp_tool_lifecycle.py @@ -266,6 +266,36 @@ def _signal_mcp_process(pid: int, sig: int, server_name: str, pgid: Optional[int os.kill(pid, sig) except (ProcessLookupError, PermissionError, OSError): pass + if os.name == "nt": # Windows has no pgid reaching reparented grandchildren — kill the tree + _kill_windows_process_tree(pid, sig) + + +def _kill_windows_process_tree(pid: int, sig: int) -> None: + """Windows counterpart of the POSIX killpg path (#61059): after the direct child is signalled, + terminate every still-alive descendant (npx.cmd → node.exe) so graceful teardown cannot leave + orphans reparented with ParentId=null. Best-effort, per-descendant; never raises.""" + import signal as _signal + try: + import psutil + except ImportError: + return + try: + parent = psutil.Process(pid) + descendants = parent.children(recursive=True) + except (psutil.NoSuchProcess, psutil.AccessDenied, OSError): + return + for child in descendants: + try: + child.terminate() + except Exception: # noqa: BLE001 - raced away or refused; sweep continues + pass + if sig == getattr(_signal, "SIGKILL", _signal.SIGTERM): # force pass: don't wait for graceful exit + _, alive = psutil.wait_procs(descendants, timeout=0) + for child in alive: + try: + child.kill() + except Exception: # noqa: BLE001 + pass def _kill_orphaned_mcp_children(include_active: bool = False, server_name: Optional[str] = None) -> None: diff --git a/tools/mcp_tool_transport.py b/tools/mcp_tool_transport.py index e4cbb9c620..dbc1254413 100644 --- a/tools/mcp_tool_transport.py +++ b/tools/mcp_tool_transport.py @@ -363,6 +363,18 @@ class MCPServerTransportMixin: command=command, args=args, env=safe_env or None, cwd=stdio_cwd, # Windows pipes can split non-UTF-8 bytes at chunk boundaries; substitute, don't raise. encoding_error_handler="replace") + # Windows has no POSIX parent-death supervisor / killpg safety net (#61059): when this + # process dies ungracefully (crash, force-quit), the stdio child trees — npx.cmd → + # node.exe — survive as orphans with ParentId=null and pile up across restarts. Attaching + # THIS process to a KILL_ON_JOB_CLOSE job before the spawn makes every child (and + # grandchild) created after it die with the parent at the kernel level, so no orphan can + # outlive us. Idempotent; BREAKAWAY_OK keeps deliberate breakaway children escaping. + # Self-guards: a cheap no-op returning False on non-Windows. + try: + from hermes_cli.process_identity import attach_self_to_kill_on_close_job + attach_self_to_kill_on_close_job() + except Exception: + logger.debug("job-object self-attach failed before stdio spawn", exc_info=True) # Reap orphans of prior attempts first (else retries pile up zombie pairs); unscoped on purpose; # off-loop because the reaper blocks up to 2s. await asyncio.to_thread(_lifecycle._kill_orphaned_mcp_children)