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")))