mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(mcp): reap orphaned subprocesses before spawning new ones on retry
When an MCP stdio subprocess fails to connect (token expiry, port contention, timeout), the run() reconnect loop retries with backoff. Each retry calls _run_stdio() which spawns a new process pair, but the previous failed pair was only detected as orphaned (added to _orphan_stdio_pids) — never actually killed. This caused rapid zombie accumulation: 5 failed attempts × 2 procs each = 10 orphans competing for the same port. Add a _kill_orphaned_mcp_children() call at the top of _run_stdio(), before the _snapshot_child_pids() baseline, so any orphans from prior failed attempts are reaped before a new subprocess is spawned. Fixes #57355
This commit is contained in:
parent
79f4f78fa4
commit
086596ca2b
2 changed files with 69 additions and 0 deletions
|
|
@ -199,6 +199,68 @@ class TestStdioPidTracking:
|
|||
with _lock:
|
||||
assert fake_pid not in _orphan_stdio_pids
|
||||
|
||||
def test_run_stdio_reaps_orphans_before_spawn(self):
|
||||
"""_run_stdio kills orphaned PIDs from prior failed attempts (#57355)."""
|
||||
from tools.mcp_tool import (
|
||||
_kill_orphaned_mcp_children,
|
||||
_orphan_stdio_pids,
|
||||
_stdio_pids,
|
||||
_stdio_pgids,
|
||||
_lock,
|
||||
MCPServerTask,
|
||||
)
|
||||
from unittest.mock import patch, MagicMock, AsyncMock
|
||||
|
||||
# Seed an orphan PID that belongs to a prior failed connection.
|
||||
fake_pid = 999999997
|
||||
with _lock:
|
||||
_orphan_stdio_pids.add(fake_pid)
|
||||
|
||||
server = MCPServerTask.__new__(MCPServerTask)
|
||||
server.name = "test-zombie-reap"
|
||||
server._ready = MagicMock()
|
||||
server._shutdown_event = MagicMock()
|
||||
server._shutdown_event.is_set.return_value = True
|
||||
server._reconnect_event = MagicMock()
|
||||
server._sampling = None
|
||||
server._elicitation = None
|
||||
server._registered_tool_names = []
|
||||
|
||||
config = {"command": "echo", "args": ["hello"]}
|
||||
|
||||
import asyncio
|
||||
|
||||
async def _run():
|
||||
# _run_stdio should reap orphans before it gets to the
|
||||
# stdio_client spawn. Patch the OSV check (local import)
|
||||
# and stdio_client so no real subprocess is spawned.
|
||||
with patch("tools.mcp_tool._MCP_AVAILABLE", True), \
|
||||
patch("tools.mcp_tool._build_safe_env", return_value={}), \
|
||||
patch("tools.mcp_tool._resolve_stdio_command",
|
||||
return_value=("echo", {})), \
|
||||
patch("tools.mcp_tool._write_stderr_log_header"), \
|
||||
patch("tools.mcp_tool._get_mcp_stderr_log",
|
||||
return_value=None), \
|
||||
patch("tools.mcp_tool.check_package_for_malware",
|
||||
return_value=None, create=True), \
|
||||
patch("tools.osv_check.check_package_for_malware",
|
||||
return_value=None):
|
||||
# Patch stdio_client to raise so the test exits quickly
|
||||
cm = MagicMock()
|
||||
cm.__aenter__ = AsyncMock(side_effect=RuntimeError("test"))
|
||||
cm.__aexit__ = AsyncMock(return_value=False)
|
||||
with patch("tools.mcp_tool.stdio_client", return_value=cm):
|
||||
try:
|
||||
await server._run_stdio(config)
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
asyncio.run(_run())
|
||||
|
||||
# The orphan must have been reaped before the spawn attempt.
|
||||
with _lock:
|
||||
assert fake_pid not in _orphan_stdio_pids
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Fix 2b: stdio descendant reaping via process group (issue #23799)
|
||||
|
|
|
|||
|
|
@ -1951,6 +1951,13 @@ class MCPServerTask:
|
|||
if _MCP_LOGGING_CALLBACK_SUPPORTED:
|
||||
sampling_kwargs["logging_callback"] = self._make_logging_callback()
|
||||
|
||||
# Reap any orphaned subprocesses from a prior failed connection
|
||||
# attempt before spawning a new one. Without this, each retry in
|
||||
# the run() reconnect loop spawns a fresh process pair while the
|
||||
# previous failed pair lingers — leading to rapid zombie
|
||||
# accumulation (see #57355).
|
||||
_kill_orphaned_mcp_children()
|
||||
|
||||
# Snapshot child PIDs before spawning so we can track the new one.
|
||||
pids_before = _snapshot_child_pids()
|
||||
new_pids: set = set()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue