From 1f6836cd81aeadeae4c19d184c8925bf363ef497 Mon Sep 17 00:00:00 2001 From: rainbowgits <164521089+rainbowgits@users.noreply.github.com> Date: Mon, 6 Jul 2026 08:37:58 +0300 Subject: [PATCH] fix(mcp): bound stdio initialize handshake to stop subprocess/FD leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/tools/test_mcp_stdio_init_timeout.py | 94 ++++++++++++++++++++++ tools/mcp_tool.py | 18 ++++- 2 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 tests/tools/test_mcp_stdio_init_timeout.py diff --git a/tests/tools/test_mcp_stdio_init_timeout.py b/tests/tools/test_mcp_stdio_init_timeout.py new file mode 100644 index 00000000000..d8d379b10c5 --- /dev/null +++ b/tests/tools/test_mcp_stdio_init_timeout.py @@ -0,0 +1,94 @@ +"""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." + ) diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 1861e2b352a..55d9eead4f7 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -2007,7 +2007,23 @@ class MCPServerTask: async with ClientSession( read_stream, write_stream, **sampling_kwargs ) as session: - self.initialize_result = await session.initialize() + # Bound the MCP handshake. A stdio server that never + # completes ``initialize`` (e.g. emits a non-JSON-RPC frame + # and then blocks on stdin) otherwise hangs this coroutine + # forever on the background loop: ``connect_timeout`` only + # bounds the caller's ``.result()`` wait, not the coroutine + # itself. Because the connect never unwinds, the cleanup + # ``finally`` below never runs, so the spawned child and its + # stdio pipes/pidfd leak on every discovery retry — unbounded + # until the gateway hits EMFILE. Timing out here converts the + # hang into a normal failure, letting the ``finally`` reap the + # child. See #59349. + connect_timeout = float( + config.get("connect_timeout", _DEFAULT_CONNECT_TIMEOUT) + ) + self.initialize_result = await asyncio.wait_for( + session.initialize(), timeout=connect_timeout + ) self.session = session await self._discover_tools() self._ready.set()