diff --git a/cli-config.yaml.example b/cli-config.yaml.example index 8d3525019c..942b3252e2 100644 --- a/cli-config.yaml.example +++ b/cli-config.yaml.example @@ -817,6 +817,10 @@ platform_toolsets: # Optional per-server settings: # timeout: tool call timeout in seconds (default: 120) # connect_timeout: initial connection timeout (default: 60) +# keepalive_interval: liveness ping cadence in seconds (default: 180). +# Lower it below the server's session TTL for servers that expire idle +# sessions quickly (e.g. Unreal Engine editor MCP, ~15s), otherwise idle +# tool calls hit an expired session and pay a slow reconnect. Floored at 5s. # # mcp_servers: # time: diff --git a/tests/tools/test_mcp_capability_gating.py b/tests/tools/test_mcp_capability_gating.py index b4f91d16bb..551af1340d 100644 --- a/tests/tools/test_mcp_capability_gating.py +++ b/tests/tools/test_mcp_capability_gating.py @@ -2,12 +2,18 @@ Prompt-only / resource-only MCP servers do not implement the ``tools/*`` request family. Per the MCP spec, ``InitializeResult.capabilities.tools`` -is non-None iff the server supports it. Before this fix, Hermes always -called ``tools/list`` during discovery and as the keepalive probe — both -raised ``McpError(-32601 Method not found)`` against such servers, so a -prompt-only server could never stay connected. +is non-None iff the server supports it. Before the capability gate, Hermes +always called ``tools/list`` during discovery, which raised +``McpError(-32601 Method not found)`` against such servers, so a prompt-only +server could never stay connected. Discovery/refresh remain capability-gated. -Ported from anomalyco/opencode#31271. +The keepalive probe uses ``ping`` (MCP base-protocol liveness) for every +server regardless of capability: it works uniformly and stays a few bytes +instead of pulling the full ``tools/list`` payload (which is ~1 MB on large +servers like Unreal Engine's editor MCP). Its cadence is configurable via +``keepalive_interval`` so servers with short session TTLs stay alive. + +Discovery gating ported from anomalyco/opencode#31271. """ import asyncio from types import SimpleNamespace @@ -143,7 +149,10 @@ class TestKeepaliveProbe: task.session.send_ping.assert_awaited_once() task.session.list_tools.assert_not_called() - async def test_keepalive_uses_list_tools_for_tool_capable_server(self): + async def test_keepalive_uses_ping_for_tool_capable_server(self): + """Keepalive uses ``ping`` even for tool-capable servers, so the probe + stays a few bytes regardless of tool count (no ``list_tools`` payload). + Tool-list changes still arrive via tools/list_changed notifications.""" task = MCPServerTask("test") task.initialize_result = _caps(tools=SimpleNamespace()) task.session = SimpleNamespace( @@ -154,5 +163,195 @@ class TestKeepaliveProbe: reason = await self._run_one_keepalive_cycle(task) assert reason == "shutdown" + task.session.send_ping.assert_awaited_once() + task.session.list_tools.assert_not_called() + + async def test_keepalive_uses_ping_legacy_fallback(self): + """No captured capabilities → still pings (no spurious list_tools).""" + task = MCPServerTask("test") + assert task.initialize_result is None + task.session = SimpleNamespace( + list_tools=AsyncMock(), + send_ping=AsyncMock(), + ) + + reason = await self._run_one_keepalive_cycle(task) + + assert reason == "shutdown" + task.session.send_ping.assert_awaited_once() + task.session.list_tools.assert_not_called() + + +class TestKeepaliveInterval: + """The keepalive cadence is configurable so servers with short session + TTLs (e.g. Unreal Engine editor MCP, ~15s) can refresh fast enough to keep + the session alive instead of hitting an expired session on every idle call. + """ + + async def _captured_interval(self, config): + """Run one keepalive cycle and capture the ``asyncio.wait`` timeout.""" + task = MCPServerTask("test") + task._config = config + task.session = SimpleNamespace(send_ping=AsyncMock()) + captured = {} + real_wait = asyncio.wait + + async def fake_wait(tasks, timeout=None, return_when=None): + captured["timeout"] = timeout + task._shutdown_event.set() + return await real_wait( + tasks, timeout=0.5, return_when=return_when or asyncio.FIRST_COMPLETED + ) + + import tools.mcp_tool as mcp_mod + orig = mcp_mod.asyncio.wait + mcp_mod.asyncio.wait = fake_wait + try: + await task._wait_for_lifecycle_event() + finally: + mcp_mod.asyncio.wait = orig + return captured["timeout"] + + @pytest.mark.asyncio + async def test_default_interval_when_unset(self): + from tools.mcp_tool import _DEFAULT_KEEPALIVE_INTERVAL + assert await self._captured_interval({}) == _DEFAULT_KEEPALIVE_INTERVAL + + @pytest.mark.asyncio + async def test_configured_interval_honored(self): + assert await self._captured_interval({"keepalive_interval": 10}) == 10 + + @pytest.mark.asyncio + async def test_interval_clamped_to_floor(self): + from tools.mcp_tool import _MIN_KEEPALIVE_INTERVAL + # A sub-floor value must clamp up, never busy-loop the keepalive. + assert ( + await self._captured_interval({"keepalive_interval": 0.1}) + == _MIN_KEEPALIVE_INTERVAL + ) + + +def _mcp_error(code, message="boom"): + """Build a real McpError carrying a JSON-RPC error code.""" + from mcp.shared.exceptions import McpError + from mcp.types import ErrorData + return McpError(ErrorData(code=code, message=message)) + + +class TestMethodNotFoundDetection: + """``_is_method_not_found_error`` underpins the ping→list_tools fallback.""" + + def test_structural_code_match(self): + from tools.mcp_tool import _is_method_not_found_error + assert _is_method_not_found_error(_mcp_error(-32601)) is True + + def test_other_mcp_error_code_is_not_match(self): + from tools.mcp_tool import _is_method_not_found_error + # Invalid params (-32602) is a real error, NOT "ping unsupported". + assert _is_method_not_found_error(_mcp_error(-32602)) is False + + def test_substring_fallback(self): + from tools.mcp_tool import _is_method_not_found_error + assert _is_method_not_found_error(Exception("Method not found")) is True + + def test_unrelated_exception_is_not_match(self): + from tools.mcp_tool import _is_method_not_found_error + assert _is_method_not_found_error(TimeoutError()) is False + assert _is_method_not_found_error(Exception("session terminated")) is False + + +@pytest.mark.asyncio +class TestKeepaliveProbeFallback: + """The probe prefers ``ping`` but falls back to ``list_tools`` for servers + that don't implement the optional ping utility — without reconnect-looping, + and without regressing servers that DO support ping.""" + + async def test_uses_ping_when_supported(self): + task = MCPServerTask("test") + task.initialize_result = _caps(tools=SimpleNamespace()) + task.session = SimpleNamespace( + send_ping=AsyncMock(), + list_tools=AsyncMock(), + ) + + await task._keepalive_probe() + + task.session.send_ping.assert_awaited_once() + task.session.list_tools.assert_not_called() + assert task._ping_unsupported is False + + async def test_falls_back_to_list_tools_on_method_not_found(self): + task = MCPServerTask("test") + task.initialize_result = _caps(tools=SimpleNamespace()) + task.session = SimpleNamespace( + send_ping=AsyncMock(side_effect=_mcp_error(-32601)), + list_tools=AsyncMock(return_value=SimpleNamespace(tools=[])), + ) + + await task._keepalive_probe() + + # First cycle: ping tried, failed -32601, list_tools used as fallback. + task.session.send_ping.assert_awaited_once() task.session.list_tools.assert_awaited_once() - task.session.send_ping.assert_not_called() + assert task._ping_unsupported is True + + async def test_latch_skips_ping_on_subsequent_cycles(self): + task = MCPServerTask("test") + task.initialize_result = _caps(tools=SimpleNamespace()) + task.session = SimpleNamespace( + send_ping=AsyncMock(side_effect=_mcp_error(-32601)), + list_tools=AsyncMock(return_value=SimpleNamespace(tools=[])), + ) + + await task._keepalive_probe() # latches _ping_unsupported + await task._keepalive_probe() # should NOT ping again + + task.session.send_ping.assert_awaited_once() # only the first cycle + assert task.session.list_tools.await_count == 2 + + async def test_real_liveness_failure_propagates_not_swallowed(self): + """A non-(-32601) ping error is a genuine connection failure: it must + propagate so the caller reconnects, and must NOT latch the fallback.""" + task = MCPServerTask("test") + task.initialize_result = _caps(tools=SimpleNamespace()) + task.session = SimpleNamespace( + send_ping=AsyncMock(side_effect=Exception("session terminated")), + list_tools=AsyncMock(), + ) + + with pytest.raises(Exception, match="session terminated"): + await task._keepalive_probe() + + task.session.list_tools.assert_not_called() + assert task._ping_unsupported is False + + async def test_no_ping_no_tools_propagates_method_not_found(self): + """A server advertising neither working ping nor tools has no cheaper + probe — the -32601 must propagate rather than calling list_tools on a + server that doesn't support it.""" + task = MCPServerTask("test") + task.initialize_result = _caps(prompts=SimpleNamespace()) # not tool-capable + task.session = SimpleNamespace( + send_ping=AsyncMock(side_effect=_mcp_error(-32601)), + list_tools=AsyncMock(), + ) + + with pytest.raises(Exception): + await task._keepalive_probe() + + task.session.list_tools.assert_not_called() + + async def test_discover_resets_latch(self): + """A fresh connection (_discover_tools) re-enables the cheap ping path.""" + task = MCPServerTask("test") + task.initialize_result = _caps(tools=SimpleNamespace()) + task._ping_unsupported = True + task.session = SimpleNamespace( + list_tools=AsyncMock(return_value=SimpleNamespace(tools=[])), + ) + + await task._discover_tools() + + assert task._ping_unsupported is False + + diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 48cb390855..69917ec6a8 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -19,6 +19,10 @@ Example config:: env: {} timeout: 120 # per-tool-call timeout in seconds (default: 300) connect_timeout: 60 # initial connection timeout (default: 60) + keepalive_interval: 10 # liveness ping cadence in seconds (default: + # 180). Set below the server's session TTL for + # servers that GC idle sessions quickly (e.g. + # Unreal Engine editor MCP, ~15s). Floored at 5s. github: command: "npx" args: ["-y", "@modelcontextprotocol/server-github"] @@ -276,6 +280,17 @@ _MAX_RECONNECT_RETRIES = 5 _MAX_INITIAL_CONNECT_RETRIES = 3 # retries for the very first connection attempt _MAX_BACKOFF_SECONDS = 60 +# Keepalive cadence for HTTP/SSE sessions. The MCP spec lets a server expire +# idle sessions on any TTL it chooses (Streamable HTTP "Session Management"), +# so a client that wants a session to survive idle periods MUST refresh faster +# than that TTL. The default suits long LB/NAT idle windows (commonly +# 300-600s); servers with short session TTLs (e.g. Unreal Engine's editor MCP, +# ~15s) need a smaller ``keepalive_interval`` in their config or every idle +# tool call lands on a dead session and pays the full reconnect path. The floor +# stops a misconfigured tiny interval from busy-looping the keepalive. +_DEFAULT_KEEPALIVE_INTERVAL = 180 # seconds between liveness pings +_MIN_KEEPALIVE_INTERVAL = 5 # clamp floor for configured intervals + # Environment variables that are safe to pass to stdio subprocesses _SAFE_ENV_KEYS = frozenset({ "PATH", "HOME", "USER", "LANG", "LC_ALL", "TERM", "SHELL", "TMPDIR", @@ -382,6 +397,40 @@ def _exc_str(exc: BaseException) -> str: return text if text else repr(exc) +# JSON-RPC "method not found" — the error a server returns when it does not +# implement a requested method (e.g. a tool-capable server that never wired up +# the optional ``ping`` utility). Defined locally with a fallback so detection +# works even on SDK builds that don't export the constant. +try: + from mcp.types import METHOD_NOT_FOUND as _JSONRPC_METHOD_NOT_FOUND +except Exception: # pragma: no cover — older/newer SDK without the constant + _JSONRPC_METHOD_NOT_FOUND = -32601 + + +def _is_method_not_found_error(exc: BaseException) -> bool: + """Return True if *exc* is a JSON-RPC ``method not found`` (-32601). + + ``ping`` is an *optional* MCP utility (spec: "optional ping mechanism"). + A server that doesn't implement it answers a ping with -32601 rather than + an empty result. Structurally inspect ``McpError.error.code`` first, then + fall back to a substring match so detection survives SDK version drift and + servers that surface the condition as a plain message. + """ + # Structural: mcp.shared.exceptions.McpError carries ErrorData.code. + err = getattr(exc, "error", None) + code = getattr(err, "code", None) + if code == _JSONRPC_METHOD_NOT_FOUND: + return True + msg = str(exc).lower() + if not msg: + return False + return ( + str(_JSONRPC_METHOD_NOT_FOUND) in msg + or "method not found" in msg + or "not found: ping" in msg + ) + + # --------------------------------------------------------------------------- # MCP tool description content scanning # --------------------------------------------------------------------------- @@ -1362,7 +1411,7 @@ class MCPServerTask: "_registered_tool_names", "_auth_type", "_refresh_lock", "_rpc_lock", "_pending_refresh_tasks", "_pending_call_context", - "initialize_result", + "initialize_result", "_ping_unsupported", ) def __init__(self, name: str): @@ -1410,6 +1459,12 @@ class MCPServerTask: # ``.capabilities.prompts``) instead of assuming every ``ClientSession`` # method attribute corresponds to a supported server method. See #18051. self.initialize_result: Optional[Any] = None + # Set True the first time a keepalive ``ping`` returns JSON-RPC + # -32601 (method not found): the server is tool-capable but doesn't + # implement the optional ``ping`` utility. Subsequent keepalives fall + # back to ``list_tools`` (the pre-ping probe) so we neither spam pings + # nor reconnect-loop. Reset on each fresh transport connection. + self._ping_unsupported: bool = False def _is_http(self) -> bool: """Check if this server uses HTTP transport.""" @@ -1564,6 +1619,46 @@ class MCPServerTask: self.name, len(self._registered_tool_names), ) + async def _keepalive_probe(self) -> None: + """Exercise the session to detect a stale/expired connection. + + Uses ``ping`` (cheap, transport-agnostic liveness) by default. ``ping`` + is an OPTIONAL MCP utility: a server that doesn't implement it answers + JSON-RPC -32601. The first time that happens we latch + ``_ping_unsupported`` and fall back to the pre-ping probe — capability + permitting, ``list_tools``; otherwise ``ping`` is the only option and + the -32601 propagates (a server advertising neither a working ping nor + tools has no liveness primitive left). The latch resets on each fresh + transport connection so a server that gains ping support after a + reconnect is re-probed with the cheap path. + + Raises on a genuine connection failure so the caller triggers a + reconnect; returns normally when the session is alive. + """ + if not self._ping_unsupported: + try: + await asyncio.wait_for(self.session.send_ping(), timeout=30.0) + return + except Exception as exc: + # Only a "method not found" means ping is unsupported. Any + # other error (timeout, closed transport, session expired) is + # a real liveness failure — propagate so we reconnect. + if not _is_method_not_found_error(exc): + raise + if not self._advertises_tools(): + # No ping, no tools → no cheaper probe to fall back to. + raise + self._ping_unsupported = True + logger.info( + "MCP server '%s': does not implement the optional 'ping' " + "utility (-32601); using 'list_tools' for keepalive on " + "this connection.", + self.name, + ) + + # Fallback probe for servers without ping support. + await asyncio.wait_for(self.session.list_tools(), timeout=30.0) + async def _wait_for_lifecycle_event(self) -> str: """Block until either _shutdown_event or _reconnect_event fires. @@ -1577,13 +1672,29 @@ class MCPServerTask: Shutdown takes precedence if both events are set simultaneously. - Periodically sends a lightweight keepalive (``list_tools``) to - prevent TCP connections from going stale during long idle - periods (#17003). If the keepalive fails, triggers a reconnect. + Periodically sends a lightweight keepalive (``ping``, with a + ``list_tools`` fallback for servers that don't implement the optional + ping utility — see :meth:`_keepalive_probe`) to prevent TCP/session + state from going stale during idle periods (#17003). If the keepalive + fails, triggers a reconnect. + + The cadence is ``keepalive_interval`` from server config (default + :data:`_DEFAULT_KEEPALIVE_INTERVAL`, floored at + :data:`_MIN_KEEPALIVE_INTERVAL`). Servers that GC idle sessions on a + short TTL (e.g. Unreal Engine's editor MCP, ~15s) need an interval + below that TTL, otherwise every idle tool call lands on an + already-expired session and pays the full reconnect path. """ - # Keepalive interval in seconds. Must be shorter than typical - # LB / NAT idle-timeout (commonly 300-600s). - _KEEPALIVE_INTERVAL = 180 # 3 minutes + # Refresh faster than the server's session TTL. ``ping`` (MCP base + # protocol liveness) is used rather than ``list_tools`` so the probe + # stays a few bytes regardless of how many tools the server exposes — + # a ``list_tools`` keepalive against an 830-tool server would pull + # ~1 MB every cycle. Tool-list changes still arrive out-of-band via + # ``notifications/tools/list_changed`` → ``_refresh_tools``. + keepalive_interval = max( + _MIN_KEEPALIVE_INTERVAL, + float(self._config.get("keepalive_interval", _DEFAULT_KEEPALIVE_INTERVAL)), + ) shutdown_task = asyncio.create_task(self._shutdown_event.wait()) reconnect_task = asyncio.create_task(self._reconnect_event.wait()) @@ -1591,30 +1702,23 @@ class MCPServerTask: while True: done, _pending = await asyncio.wait( {shutdown_task, reconnect_task}, - timeout=_KEEPALIVE_INTERVAL, + timeout=keepalive_interval, return_when=asyncio.FIRST_COMPLETED, ) if done: break - # Timeout — no lifecycle event fired. Send a keepalive - # to exercise the connection and detect stale sockets. - # Prompt-only / resource-only servers don't implement - # ``tools/list`` (McpError -32601), so use the universal - # ``ping`` request for them instead — otherwise every - # keepalive cycle would trigger a spurious reconnect. + # Timeout — no lifecycle event fired. Probe the connection + # to detect stale/expired sessions. Prefer ``ping`` (MCP base + # protocol liveness): it works uniformly and stays a few bytes + # regardless of tool count, unlike ``list_tools`` (~1 MB on an + # 830-tool server). ``ping`` is an OPTIONAL utility, so a + # tool-capable server that doesn't implement it answers -32601; + # in that case fall back to the pre-ping ``list_tools`` probe + # for the rest of this connection rather than reconnect-looping. if self.session: try: - if self._advertises_tools(): - await asyncio.wait_for( - self.session.list_tools(), - timeout=30.0, - ) - else: - await asyncio.wait_for( - self.session.send_ping(), - timeout=30.0, - ) + await self._keepalive_probe() except Exception as exc: logger.warning( "MCP server '%s' keepalive failed, " @@ -2040,6 +2144,10 @@ class MCPServerTask: server doesn't advertise the ``tools`` capability. (Ported from anomalyco/opencode#31271.) """ + # Fresh transport connection → re-probe with the cheap ``ping`` path. + # Clears any latch from a prior connection in case the server gained + # ping support across the reconnect. + self._ping_unsupported = False if self.session is None: return if not self._advertises_tools():