From 02b398bda591fbe568ce5df1ad2481ce56ff58e5 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 12 Sep 2026 07:20:50 -0700 Subject: [PATCH] fix(mcp): recovered application errors keep the breaker strike The pick returns the real result after a transport recovery instead of dropping it, but it also skipped the breaker bookkeeping. Application errors counting as strikes is the point of the breaker (3ff18ffe1408, #10447: a server answering errors made the model hammer it 8x in 10s). Route the recovered result through _record_call_outcome so the caller sees the tool's answer and the counter still moves the right way. --- tests/tools/test_mcp_tool_session_expired.py | 3 +++ tools/mcp_tool_handlers.py | 9 ++++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/tools/test_mcp_tool_session_expired.py b/tests/tools/test_mcp_tool_session_expired.py index 1320d9fd77..5c824d8f53 100644 --- a/tests/tools/test_mcp_tool_session_expired.py +++ b/tests/tools/test_mcp_tool_session_expired.py @@ -204,6 +204,9 @@ def test_call_tool_handler_rebuilds_configured_server_transport( assert call_count["n"] == 1 else: assert parsed == {"error" if application_error else "result": "reconnected"} + # The recovered result is the tool's real answer either way; an application error is + # still one breaker strike (#10447), a success resets the counter. + assert mcp_tool._server_error_counts.get("resumed", 0) == (1 if application_error else 0) assert call_count["n"] == 2 assert routes == [expected_route, expected_route] assert configs == [transport_config, transport_config] diff --git a/tools/mcp_tool_handlers.py b/tools/mcp_tool_handlers.py index dbb96f5811..c494f03b67 100644 --- a/tools/mcp_tool_handlers.py +++ b/tools/mcp_tool_handlers.py @@ -132,16 +132,15 @@ def _lookup_reconnectable_server(server_name: str, require_loop: bool = False): def _retry_once(server_name: str, retry_call, op_description: str, what: str): - """Re-run ``retry_call`` after a recovery step. Returns the result (closing the breaker) - when the RPC completed; None when the retry raised (caller falls through).""" + """Re-run ``retry_call`` after a recovery step. Returns the result when the RPC completed + (an application error is still the tool's real answer, and still a breaker strike per #10447); + None when the retry raised (caller falls through).""" try: result = retry_call() except Exception as retry_exc: logger.warning("MCP %s/%s retry after %s failed: %s", server_name, op_description, what, retry_exc) return None - # An application error still proves the recovered transport completed a round-trip. - _core._reset_server_error(server_name) - return result + return _record_call_outcome(server_name, result) def _handle_auth_error_and_retry(server_name: str, exc: BaseException, retry_call, op_description: str):