From 86fa1fcd4f08e44be44fd44230720eecec1acb80 Mon Sep 17 00:00:00 2001 From: codexbt Date: Fri, 28 Aug 2026 15:49:24 +0000 Subject: [PATCH] fix(mcp): treat silent ping drop as unsupported rather than dead transport (Closes #97245) A stdio server that never answers the optional ping (no -32601, no response at all) produced a bare TimeoutError that _keepalive_probe classified as a dead transport, tearing down and respawning a healthy subprocess on every keepalive tick. On a first ping timeout, confirm with list_tools before declaring death; if it answers, latch _ping_unsupported and use list_tools from then on. If both fail, propagate as before. --- tests/tools/test_mcp_capability_gating.py | 50 ++++++++++++++++++++++ tools/mcp_tool.py | 51 ++++++++++++++++------- 2 files changed, 87 insertions(+), 14 deletions(-) diff --git a/tests/tools/test_mcp_capability_gating.py b/tests/tools/test_mcp_capability_gating.py index 5facbd24e4..25dd7084aa 100644 --- a/tests/tools/test_mcp_capability_gating.py +++ b/tests/tools/test_mcp_capability_gating.py @@ -295,4 +295,54 @@ class TestKeepaliveProbeFallback: assert task._ping_unsupported is False + async def test_silent_ping_drop_falls_back_to_list_tools(self): + """Regression for #97245: a server that silently drops ping (no + response at all) produces a TimeoutError. If list_tools succeeds, + the transport is alive — latch _ping_unsupported and return + normally instead of reconnect-looping.""" + task = MCPServerTask("test") + task.initialize_result = _caps(tools=SimpleNamespace()) + task.session = SimpleNamespace( + send_ping=AsyncMock(side_effect=asyncio.TimeoutError()), + list_tools=AsyncMock(return_value=SimpleNamespace(tools=[])), + ) + + # Should NOT raise — the server is alive. + await task._keepalive_probe() + + task.session.send_ping.assert_awaited_once() + task.session.list_tools.assert_awaited_once() + assert task._ping_unsupported is True + + async def test_silent_ping_drop_both_fail_propagates(self): + """When both ping AND list_tools time out, it is a genuine liveness + failure — propagate so the caller reconnects.""" + task = MCPServerTask("test") + task.initialize_result = _caps(tools=SimpleNamespace()) + task.session = SimpleNamespace( + send_ping=AsyncMock(side_effect=asyncio.TimeoutError()), + list_tools=AsyncMock(side_effect=asyncio.TimeoutError()), + ) + + with pytest.raises((TimeoutError, asyncio.TimeoutError)): + await task._keepalive_probe() + + assert task._ping_unsupported is False + + async def test_silent_ping_drop_no_tools_propagates(self): + """A server that has no tools capability and times out on ping has no + fallback probe — the timeout must propagate immediately.""" + task = MCPServerTask("test") + task.initialize_result = _caps(prompts=SimpleNamespace()) # no tools + task.session = SimpleNamespace( + send_ping=AsyncMock(side_effect=asyncio.TimeoutError()), + list_tools=AsyncMock(), + ) + + with pytest.raises((TimeoutError, asyncio.TimeoutError)): + await task._keepalive_probe() + + # list_tools must not be called — no tools capability advertised. + task.session.list_tools.assert_not_called() + assert task._ping_unsupported is False diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 0c3aeb8a18..7f92122c3f 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -2871,21 +2871,44 @@ class MCPServerTask: 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): + if _is_method_not_found_error(exc): + # Structural -32601 or "Unknown method" — ping is + # definitively unsupported. + if not self._advertises_tools(): + 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, + ) + elif isinstance(exc, (TimeoutError, asyncio.TimeoutError)) and self._advertises_tools(): + # A server that silently drops ping (no response at all) + # produces a TimeoutError indistinguishable from a dead + # transport. Before declaring it dead, try list_tools as + # a confirmation probe (#97245). If the transport is + # genuinely broken, list_tools will also fail and we + # propagate that failure. + try: + await asyncio.wait_for(self.session.list_tools(), timeout=30.0) + except Exception: + # Both probes failed — genuine liveness failure. + raise exc from None + # Transport alive, ping just isn't answered. Latch the + # fallback so subsequent keepalives skip the 30s wait. + self._ping_unsupported = True + logger.info( + "MCP server '%s': ping timed out but list_tools " + "succeeded — server silently drops ping; using " + "'list_tools' for keepalive on this connection.", + self.name, + ) + return + else: + # Any other error (closed transport, session expired, + # etc.) is a real liveness failure — propagate. 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)