fix(mcp): don't trip circuit breaker on tool errors - #74795
fix(mcp): don't trip circuit breaker on tool errors#74795renga-kogahara wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused MCP classification fix. Current main still converts completed CallToolResult.isError responses into an error envelope at tools/mcp_tool.py:4812-4827 and then increments the server-wide breaker for that envelope at tools/mcp_tool.py:4896-4905, so the reported behavior is present.
Problems
- This patch is redundant with the still-open #74045: its production changes cover the same auth retry, session-reconnect retry, and completed-call classification sites. #74045 also exercises recovery through
_make_tool_handler, rather than only calling the shared retry helpers directly.
Suggested changes
- If retained separately, test the auth and session recovery paths through
_make_tool_handlerwith a realCallToolResult(isError=True)fixture, covering the conversion path attools/mcp_tool.py:4812-4827as well as the recovery helper.
This is an automated hermes-sweeper review.
| server = MagicMock() | ||
| server._reconnect_event = MagicMock() | ||
| mcp_tool._servers["srv"] = server | ||
| mcp_tool._server_error_counts["srv"] = 2 |
There was a problem hiding this comment.
This invokes the private recovery helper with a prebuilt JSON error, so it does not exercise the real CallToolResult.isError → tool_error(...) conversion in _make_tool_handler. Consider a handler-level regression like #74045's recovery coverage.
|
|
||
| try: | ||
| result = mcp_tool._handle_session_expired_and_retry( | ||
| "srv-domain-error", |
There was a problem hiding this comment.
This likewise verifies the helper return value but not the actual post-reconnect CallToolResult.isError conversion path. A handler-level fixture would cover the end-to-end behavior this PR changes.
What does this PR do?
Treats completed MCP
CallToolResult(isError=True)responses as tool/domain errors rather than server connectivity failures.A completed RPC proves the MCP transport is reachable. Counting a tool-level validation error toward the server circuit breaker can incorrectly mark a healthy MCP server as unreachable and prevent subsequent valid calls.
Related Issue
Supersedes #74718.
Type of Change
Changes Made
tools/callRPC, includingisError=Trueresults.How to Test
.venv/bin/python -m pytest tests/tools/test_mcp*.py -q -o 'addopts='.CallToolResult(isError=True)responses do not open the server connectivity breaker.Focused result:
385 passed.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — docstrings updatedcli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/AScreenshots / Logs
N/A