From 5ae7044739f32049211206046178c9d3efcea2e4 Mon Sep 17 00:00:00 2001 From: Copilot Date: Mon, 4 May 2026 22:55:32 +0000 Subject: [PATCH 1/2] Fix MCP client infinite loop when server does not support ping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an MCP server does not implement the optional ping method (per the MCP spec), _ensure_connected() would catch the McpError and trigger a reconnection. With load_tools=True, reconnection calls load_tools() which calls _ensure_connected() again, creating an infinite loop. Fix: Detect the JSON-RPC -32601 (Method not found) error code in the McpError response from send_ping(). When this specific error is returned, treat the connection as valid since the server is clearly responsive — it just doesn't implement the optional ping method. Fixes #4940 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- python/packages/core/agent_framework/_mcp.py | 20 +++++++ python/packages/core/tests/core/test_mcp.py | 55 ++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/python/packages/core/agent_framework/_mcp.py b/python/packages/core/agent_framework/_mcp.py index 9dfb29932f..ffd55478df 100644 --- a/python/packages/core/agent_framework/_mcp.py +++ b/python/packages/core/agent_framework/_mcp.py @@ -1039,8 +1039,28 @@ async def _ensure_connected(self) -> None: Raises: ToolExecutionException: If reconnection fails. """ + from mcp.shared.exceptions import McpError + + # JSON-RPC error code for "Method not found" + METHOD_NOT_FOUND = -32601 + try: await self.session.send_ping() # type: ignore[union-attr] + except McpError as mcp_exc: + # If the server responds with "Method not found", it means the + # connection is alive but the server doesn't implement the optional + # ping method (per the MCP spec, ping is optional). Treat as connected. + if mcp_exc.error.code == METHOD_NOT_FOUND: + logger.debug("MCP server does not support ping (method not found). Connection is valid.") + return + logger.info("MCP connection invalid or closed. Reconnecting...") + try: + await self.connect(reset=True) + except Exception as ex: + raise ToolExecutionException( + "Failed to establish MCP connection.", + inner_exception=ex, + ) from ex except Exception: logger.info("MCP connection invalid or closed. Reconnecting...") try: diff --git a/python/packages/core/tests/core/test_mcp.py b/python/packages/core/tests/core/test_mcp.py index 01cf1717bd..442c747a8f 100644 --- a/python/packages/core/tests/core/test_mcp.py +++ b/python/packages/core/tests/core/test_mcp.py @@ -3662,6 +3662,61 @@ async def test_ensure_connected_wraps_reconnect_failure() -> None: await tool._ensure_connected() +async def test_ensure_connected_treats_ping_method_not_found_as_connected() -> None: + """When the MCP server does not support the optional ping method, treat as connected.""" + tool = MCPTool(name="test_tool") + tool.session = Mock( + send_ping=AsyncMock( + side_effect=McpError(types.ErrorData(code=-32601, message="Unsupported method: ping")) + ) + ) + + with patch.object(tool, "connect", AsyncMock()) as mock_connect: + await tool._ensure_connected() + + mock_connect.assert_not_awaited() + + +async def test_ensure_connected_reconnects_on_non_method_not_found_mcp_error() -> None: + """McpError with a code other than -32601 should still trigger reconnection.""" + tool = MCPTool(name="test_tool") + tool.session = Mock( + send_ping=AsyncMock(side_effect=McpError(types.ErrorData(code=-32603, message="Internal error"))) + ) + + with patch.object(tool, "connect", AsyncMock()) as mock_connect: + await tool._ensure_connected() + + mock_connect.assert_awaited_once_with(reset=True) + + +async def test_ensure_connected_no_infinite_loop_when_ping_unsupported() -> None: + """Verify that load_tools does not loop infinitely when the server lacks ping support.""" + tool = MCPTool(name="test_tool", load_tools=True, load_prompts=False) + mock_session = Mock(spec=ClientSession) + mock_session.send_ping = AsyncMock( + side_effect=McpError(types.ErrorData(code=-32601, message="Method not found: ping")) + ) + mock_session.list_tools = AsyncMock( + return_value=types.ListToolsResult( + tools=[ + types.Tool( + name="example_tool", + description="An example tool", + inputSchema={"type": "object", "properties": {}}, + ) + ] + ) + ) + tool.session = mock_session + tool.is_connected = True + + await tool.load_tools() + + assert len(tool._functions) == 1 + assert tool._functions[0].name == "example_tool" + + async def test_mcp_tool_filters_framework_kwargs(): """Test that call_tool filters out framework-specific kwargs before calling MCP session. From 6d599b55fe40479130723dfd6decc0fd8a0e665f Mon Sep 17 00:00:00 2001 From: Giles Odigwe Date: Wed, 6 May 2026 11:58:56 -0700 Subject: [PATCH 2/2] Address review comments on MCP ping infinite loop fix - Cache _ping_supported flag to avoid repeated failing round-trips on every pagination page for servers that don't implement ping - Add _reconnecting guard to prevent re-entrant reconnection loops through connect() -> load_tools() -> _ensure_connected() - Treat all McpError responses from ping as 'transport alive' (not just -32601), consistent with how call_tool() handles McpError - Reset _ping_supported on reconnect to re-probe new sessions - Inline METHOD_NOT_FOUND constant per maintainer feedback - Expand test coverage: caching, recursion guard, reconnect-failure wrapping, and non-32601 McpError no longer triggering reconnect Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- python/packages/core/agent_framework/_mcp.py | 35 +++++----- python/packages/core/tests/core/test_mcp.py | 69 +++++++++++++++++++- 2 files changed, 86 insertions(+), 18 deletions(-) diff --git a/python/packages/core/agent_framework/_mcp.py b/python/packages/core/agent_framework/_mcp.py index ffd55478df..b7a1251668 100644 --- a/python/packages/core/agent_framework/_mcp.py +++ b/python/packages/core/agent_framework/_mcp.py @@ -248,6 +248,8 @@ def __init__( self.is_connected: bool = False self._tools_loaded: bool = False self._prompts_loaded: bool = False + self._ping_supported: bool = True + self._reconnecting: bool = False def __str__(self) -> str: return f"MCPTool(name={self.name}, description={self.description})" @@ -651,6 +653,7 @@ async def _connect_on_owner(self, *, reset: bool = False) -> None: await self._safe_close_exit_stack() self.session = None self.is_connected = False + self._ping_supported = True self._exit_stack = AsyncExitStack() if not self.session: try: @@ -1041,28 +1044,28 @@ async def _ensure_connected(self) -> None: """ from mcp.shared.exceptions import McpError - # JSON-RPC error code for "Method not found" - METHOD_NOT_FOUND = -32601 + # Avoid re-entrant reconnection when connect() calls load_tools()/load_prompts() + if self._reconnecting: + return + + # If the server previously indicated it doesn't support ping, skip the check. + # The actual list_tools/list_prompts call will surface transport errors if any. + if not self._ping_supported: + return try: await self.session.send_ping() # type: ignore[union-attr] except McpError as mcp_exc: - # If the server responds with "Method not found", it means the - # connection is alive but the server doesn't implement the optional - # ping method (per the MCP spec, ping is optional). Treat as connected. - if mcp_exc.error.code == METHOD_NOT_FOUND: + # McpError means the server received the request and responded—the + # transport is alive. If the error is "Method not found" (-32601), + # the server doesn't implement the optional ping method per MCP spec. + if mcp_exc.error.code == -32601: # JSON-RPC "Method not found" + self._ping_supported = False logger.debug("MCP server does not support ping (method not found). Connection is valid.") - return - logger.info("MCP connection invalid or closed. Reconnecting...") - try: - await self.connect(reset=True) - except Exception as ex: - raise ToolExecutionException( - "Failed to establish MCP connection.", - inner_exception=ex, - ) from ex + return except Exception: logger.info("MCP connection invalid or closed. Reconnecting...") + self._reconnecting = True try: await self.connect(reset=True) except Exception as ex: @@ -1070,6 +1073,8 @@ async def _ensure_connected(self) -> None: "Failed to establish MCP connection.", inner_exception=ex, ) from ex + finally: + self._reconnecting = False async def call_tool(self, tool_name: str, **kwargs: Any) -> str | list[Content]: """Call a tool with the given arguments. diff --git a/python/packages/core/tests/core/test_mcp.py b/python/packages/core/tests/core/test_mcp.py index 442c747a8f..c67286788f 100644 --- a/python/packages/core/tests/core/test_mcp.py +++ b/python/packages/core/tests/core/test_mcp.py @@ -3675,10 +3675,22 @@ async def test_ensure_connected_treats_ping_method_not_found_as_connected() -> N await tool._ensure_connected() mock_connect.assert_not_awaited() + assert tool._ping_supported is False -async def test_ensure_connected_reconnects_on_non_method_not_found_mcp_error() -> None: - """McpError with a code other than -32601 should still trigger reconnection.""" +async def test_ensure_connected_skips_ping_when_previously_unsupported() -> None: + """After ping is marked unsupported, _ensure_connected skips the ping call entirely.""" + tool = MCPTool(name="test_tool") + tool._ping_supported = False + tool.session = Mock(send_ping=AsyncMock()) + + await tool._ensure_connected() + + tool.session.send_ping.assert_not_awaited() + + +async def test_ensure_connected_does_not_reconnect_on_any_mcp_error() -> None: + """Any McpError from ping means the transport is alive—do not reconnect.""" tool = MCPTool(name="test_tool") tool.session = Mock( send_ping=AsyncMock(side_effect=McpError(types.ErrorData(code=-32603, message="Internal error"))) @@ -3687,7 +3699,9 @@ async def test_ensure_connected_reconnects_on_non_method_not_found_mcp_error() - with patch.object(tool, "connect", AsyncMock()) as mock_connect: await tool._ensure_connected() - mock_connect.assert_awaited_once_with(reset=True) + mock_connect.assert_not_awaited() + # Non -32601 errors don't cache ping as unsupported + assert tool._ping_supported is True async def test_ensure_connected_no_infinite_loop_when_ping_unsupported() -> None: @@ -3715,6 +3729,55 @@ async def test_ensure_connected_no_infinite_loop_when_ping_unsupported() -> None assert len(tool._functions) == 1 assert tool._functions[0].name == "example_tool" + # After first ping failure, subsequent pages skip ping entirely + assert tool._ping_supported is False + + +async def test_ensure_connected_recursion_guard_prevents_loop() -> None: + """When _reconnecting is True, _ensure_connected returns immediately (prevents recursion).""" + tool = MCPTool(name="test_tool") + tool._reconnecting = True + tool.session = Mock(send_ping=AsyncMock(side_effect=RuntimeError("should not be called"))) + + # Should return immediately without calling ping or connect + await tool._ensure_connected() + + tool.session.send_ping.assert_not_awaited() + + +async def test_ensure_connected_reconnect_resets_ping_supported() -> None: + """After a successful reconnect, _ping_supported is reset to True.""" + tool = MCPTool(name="test_tool") + tool._ping_supported = False + tool.session = Mock(send_ping=AsyncMock(side_effect=RuntimeError("closed"))) + + async def mock_connect(*, reset: bool = False) -> None: + # connect(reset=True) resets _ping_supported + if reset: + tool._ping_supported = True + tool.session = Mock(send_ping=AsyncMock()) + + # First, enable ping so _ensure_connected actually checks + tool._ping_supported = True + + with patch.object(tool, "connect", AsyncMock(side_effect=mock_connect)): + await tool._ensure_connected() + + assert tool._ping_supported is True + + +async def test_ensure_connected_wraps_reconnect_failure_on_transport_error() -> None: + """Transport error from ping + reconnect failure should raise ToolExecutionException.""" + tool = MCPTool(name="test_tool") + tool.session = Mock(send_ping=AsyncMock(side_effect=ConnectionError("connection lost"))) + + with ( + patch.object(tool, "connect", AsyncMock(side_effect=RuntimeError("still closed"))), + pytest.raises(ToolExecutionException, match="Failed to establish MCP connection"), + ): + await tool._ensure_connected() + + assert tool._reconnecting is False async def test_mcp_tool_filters_framework_kwargs():