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.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
+37
-14
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user