Python: fix(python): treat any MCP ping JSON-RPC error as unsupported, not just -32601 - #7656
Conversation
…st -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.
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
github-actions
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
Updates MCP connection health checks to treat any JSON-RPC ping error as evidence of a live connection.
Changes:
- Disables future pings after any
McpError. - Retains reconnect behavior for transport and other non-MCP failures.
- Adds regression coverage for error code
-32600.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
python/packages/core/agent_framework/_mcp.py |
Broadens unsupported-ping handling. |
python/packages/core/tests/core/test_mcp.py |
Tests nonstandard ping error handling. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
| "Failed to establish MCP connection.", | ||
| inner_exception=ex, | ||
| ) from ex | ||
| # Any well-formed JSON-RPC error response -- regardless of the |
There was a problem hiding this comment.
Should we preserve reconnects for client-synthesized McpErrors before disabling ping? In MCP SDK 1.29, a read timeout becomes code 408, a closed receive stream becomes CONNECTION_CLOSED, and streamable HTTP maps an expired session to "Session terminated", so none proves the server answered the ping. This branch now suppresses those signals, leaves _ping_available false, and lets the following list_tools or list_prompts request hit the dead session; could we route _is_connection_lost(mcp_exc) and 408 through _reconnect_without_loading() and only disable ping for actual server rejections?
There was a problem hiding this comment.
Good catch, agreed. I was trying to handle nonstandard server-side ping rejections like -32600, but I overgeneralized McpError.
Let me rework this. I’ll keep reconnect behavior for timeout and session-loss cases, and only disable pings for actual server ping rejections.
|
Can you please properly fill out the pull request template and also attach an open issue. |
Hi,
MCPTool._ensure_connected() used a periodic
pingas a connection health check.When the server rejected
pingwith 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.It seems like some real-world MCP servers (like the Microsoft Fabric's Data Agent MCP endpoint) reject an unsupported
pingwith 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.
Motivation & Context
Description & Review Guide
Related Issue
Fixes #
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.