From 8cf999368381d669a233621da0fc9714029d3afb Mon Sep 17 00:00:00 2001 From: Diego Casati Date: Thu, 13 Aug 2026 14:41:13 -0600 Subject: [PATCH] fix(python): treat any MCP ping JSON-RPC error as unsupported, not just -32601 MCPTool._ensure_connected() used a periodic `ping` as a connection health check. When the server rejected `ping` with an McpError, the code only treated error code -32601 ("Method not found", the JSON-RPC-spec-correct code for an unimplemented method) as "ping unsupported, keep using this connection". Any other error code fell through to a reconnect attempt. Some real-world MCP servers -- e.g. Microsoft Fabric's Data Agent MCP endpoint -- reject an unsupported `ping` with a different code (-32600 "Invalid Request") instead. Since that server only implements initialize, notifications/initialized, tools/list, and tools/call, every single tool call triggered this same failed-ping path, and instead of just disabling future pings, the client attempted an unnecessary reconnect. That reconnect can itself fail in ways unrelated to the original (harmless) ping rejection, breaking an otherwise fully working MCP tool. Any well-formed JSON-RPC error response to ping -- regardless of its specific error code -- already proves the connection is alive: the server received the request and replied. It just means the server doesn't support (or accept) the optional ping method. This change treats any McpError from send_ping() the same way: disable future pings and continue using the existing connection, without attempting a reconnect. Only non-McpError exceptions (timeouts, transport/connection failures) -- where we genuinely don't know if the server is still reachable -- still trigger a reconnect, unchanged from before. Added a regression test (nonstandard error code -32600) alongside the existing -32601 test to cover this. --- python/packages/core/agent_framework/_mcp.py | 34 +++++++++++++------- python/packages/core/tests/core/test_mcp.py | 21 ++++++++++++ 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/python/packages/core/agent_framework/_mcp.py b/python/packages/core/agent_framework/_mcp.py index 9abafdad27..24cb43a572 100644 --- a/python/packages/core/agent_framework/_mcp.py +++ b/python/packages/core/agent_framework/_mcp.py @@ -1985,19 +1985,29 @@ async def _ensure_connected(self) -> None: try: await self.session.send_ping() # type: ignore[union-attr] except McpError as mcp_exc: - if mcp_exc.error.code == -32601: - self._ping_available = False - logger.debug("Skipping future MCP pings because the server does not support ping.") - return - logger.info("MCP connection invalid or closed. Reconnecting...") - try: - await self._reconnect_without_loading() - except Exception as ex: - raise ToolExecutionException( - "Failed to establish MCP connection.", - inner_exception=ex, - ) from ex + # Any well-formed JSON-RPC error response -- regardless of the + # specific error code -- proves the server received our request + # and replied over a live connection; it just doesn't support + # (or accept) the optional `ping` method. Some MCP server + # implementations return a nonstandard code for this (e.g. -32600 + # "Invalid Request" instead of the JSON-RPC-spec-correct -32601 + # "Method not found"), so we don't gate this on the exact code. + # Treating it as "connection invalid, reconnect" would be both + # unnecessary and, since ping is best-effort here, riskier than + # just disabling future pings and continuing to use the + # existing, clearly-still-open connection. + logger.debug( + "Skipping future MCP pings because the server responded to ping with a JSON-RPC error (code=%s): %s", + mcp_exc.error.code, + mcp_exc.error.message, + ) + self._ping_available = False + return except Exception: + # Unlike an McpError (a valid JSON-RPC error response), any other + # exception here (timeout, transport/connection error, etc.) + # means we don't know whether the server is still reachable, so + # attempting a reconnect is warranted. logger.info("MCP connection invalid or closed. Reconnecting...") try: await self._reconnect_without_loading() diff --git a/python/packages/core/tests/core/test_mcp.py b/python/packages/core/tests/core/test_mcp.py index 028cac027b..1204abf93c 100644 --- a/python/packages/core/tests/core/test_mcp.py +++ b/python/packages/core/tests/core/test_mcp.py @@ -5317,6 +5317,27 @@ async def test_ensure_connected_skips_future_pings_when_ping_is_not_available() assert tool._ping_available is False +async def test_ensure_connected_skips_future_pings_on_nonstandard_mcp_error_code() -> None: + """Some MCP servers reject an unsupported `ping` with a JSON-RPC error + code other than the spec-correct -32601 (e.g. -32600 "Invalid Request"). + Any McpError response proves the connection is alive -- the server + replied -- so this should be treated the same as the -32601 case: + disable future pings and keep using the existing connection, without + reconnecting.""" + tool = MCPTool(name="test_tool") # type: ignore[abstract] + tool.session = Mock( + send_ping=AsyncMock(side_effect=McpError(types.ErrorData(code=-32600, message="Unsupported MCP method: ping"))) + ) + + with patch.object(tool, "_reconnect_without_loading", AsyncMock()) as mock_reconnect: + await tool._ensure_connected() + await tool._ensure_connected() + + tool.session.send_ping.assert_awaited_once() + mock_reconnect.assert_not_awaited() + assert tool._ping_available is False + + async def test_ensure_connected_reconnects_on_failed_ping() -> None: tool = MCPTool(name="test_tool") # type: ignore[abstract] tool.session = Mock(send_ping=AsyncMock(side_effect=RuntimeError("closed")))