mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-09 13:21:42 +00:00
1f6836cd81
A stdio MCP server that never completes `initialize` (e.g. emits a non-JSON-RPC frame and then blocks on stdin) leaks a child process plus its stdio pipes/pidfd on every discovery-retry cycle — unbounded, until the gateway hits EMFILE and every new open()/spawn fails (#59349). Root cause (confirmed by instrumenting the live repro, and different from the issue's own hypothesis): the spawned child IS captured in `new_pids`, so the report's "new_pids empty at finally" guess is not it. The real cause is that `session.initialize()` hangs forever on the garbage stream. `connect_timeout` only bounds the caller's `.result()` wait on the foreground thread — it does NOT cancel the `_run_stdio` coroutine on the background MCP loop. So the coroutine is stuck at `await session.initialize()` permanently, its cleanup `finally` never runs, the child is never reaped, and it stays invisible to the orphan-reaper (whose `_orphan_stdio_pids` set never gets populated). Fix: wrap `session.initialize()` in `asyncio.wait_for(..., connect_timeout)` so a stalled handshake fails instead of hanging. The TimeoutError unwinds through the SDK context managers (closing the child's stdin -> EOF -> exit) and lets the existing `finally` reap any straggler. Cross-platform — no signals/pgid/proc. Scope: stdio only. The HTTP path has the same `await session.initialize()` shape but spawns no subprocess (so it can't cause this leak) and already has httpx transport timeouts. Verified: the reporter's repro goes from unbounded growth to draining to zero; added a hermetic regression test (fake transport whose `initialize()` hangs, asserts the connect is bounded by connect_timeout) that fails on the pre-fix code and passes on the fix; 566 existing MCP tests pass; ruff clean. Repro confirmed on macOS (pipe FDs); the Linux-specific pidfd growth in the report should be equivalent — the reporter offered to validate on Linux. Closes #59349
94 lines
3.8 KiB
Python
94 lines
3.8 KiB
Python
"""Regression test for the stdio-MCP subprocess/FD leak (#59349).
|
|
|
|
A stdio MCP server that never completes ``initialize`` (e.g. emits a
|
|
non-JSON-RPC frame and then blocks on stdin) used to hang ``_run_stdio``
|
|
forever on the background event loop: ``connect_timeout`` bounded only the
|
|
*caller's* ``.result()`` wait, not the coroutine itself. Because the connect
|
|
never unwound, the cleanup ``finally`` in ``_run_stdio`` never ran, so the
|
|
spawned child process and its stdio pipes / pidfd leaked on *every* discovery
|
|
retry — unbounded, until the gateway hit EMFILE.
|
|
|
|
The fix wraps ``session.initialize()`` in
|
|
``asyncio.wait_for(..., timeout=connect_timeout)`` so a stalled handshake fails
|
|
instead of hanging, which lets the existing ``finally`` reap the child.
|
|
|
|
This test drives the *real* ``_run_stdio`` with a fake transport whose
|
|
``initialize()`` hangs, and asserts the connect is bounded by
|
|
``connect_timeout`` rather than blocking forever. It is fully hermetic — no real
|
|
subprocess, no network (the drain-to-zero behaviour was additionally verified
|
|
manually against the reporter's live repro).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import asyncio
|
|
import time
|
|
from unittest.mock import patch
|
|
|
|
import pytest
|
|
|
|
pytest.importorskip("mcp")
|
|
|
|
|
|
class _HangingSession:
|
|
"""Stand-in ClientSession whose handshake never completes."""
|
|
|
|
async def initialize(self):
|
|
await asyncio.sleep(3600)
|
|
|
|
|
|
class _FakeAsyncCM:
|
|
"""Minimal async context manager yielding a fixed value; spawns nothing."""
|
|
|
|
def __init__(self, value):
|
|
self._value = value
|
|
|
|
async def __aenter__(self):
|
|
return self._value
|
|
|
|
async def __aexit__(self, *_exc):
|
|
return False
|
|
|
|
|
|
def _fake_stdio_client(*_args, **_kwargs):
|
|
# `async with stdio_client(...) as (read, write)` — no subprocess spawned.
|
|
return _FakeAsyncCM((object(), object()))
|
|
|
|
|
|
def _fake_client_session(*_args, **_kwargs):
|
|
# `async with ClientSession(...) as session` -> a session that hangs.
|
|
return _FakeAsyncCM(_HangingSession())
|
|
|
|
|
|
class TestStdioInitializeTimeout:
|
|
def test_hanging_initialize_is_bounded_not_leaked(self):
|
|
"""A stdio server that hangs at ``initialize`` must fail within
|
|
``connect_timeout`` — not block ``_run_stdio`` forever (#59349)."""
|
|
from tools import mcp_tool
|
|
|
|
server = mcp_tool.MCPServerTask("leak-guard")
|
|
config = {"command": "fake-mcp", "args": [], "connect_timeout": 0.2}
|
|
|
|
async def drive():
|
|
with patch.object(mcp_tool, "stdio_client", _fake_stdio_client), \
|
|
patch.object(mcp_tool, "ClientSession", _fake_client_session), \
|
|
patch.object(mcp_tool, "_resolve_stdio_command", lambda c, e: (c, e)), \
|
|
patch.object(mcp_tool, "_write_stderr_log_header", lambda *_a, **_k: None), \
|
|
patch.object(mcp_tool, "_get_mcp_stderr_log", lambda: None), \
|
|
patch("tools.osv_check.check_package_for_malware",
|
|
lambda *_a, **_k: None):
|
|
start = time.monotonic()
|
|
# The outer 5s guard exists ONLY so a regression can't hang the
|
|
# whole suite. With the fix, the inner connect_timeout (0.2s)
|
|
# fires first; the elapsed assertion below is what actually
|
|
# distinguishes "bounded" (fixed) from "hung" (regressed).
|
|
with pytest.raises(asyncio.TimeoutError):
|
|
await asyncio.wait_for(server._run_stdio(config), timeout=5.0)
|
|
return time.monotonic() - start
|
|
|
|
elapsed = asyncio.run(drive())
|
|
assert elapsed < 2.0, (
|
|
f"_run_stdio blocked {elapsed:.1f}s on a hanging initialize() — the "
|
|
f"connect_timeout ({config['connect_timeout']}s) bound was not applied; "
|
|
f"the #59349 subprocess/FD leak has regressed."
|
|
)
|