fix: MCP circuit breaker should only count infrastructure errors by nightq · Pull Request #11128 · NousResearch/hermes-agent

nightq · 2026-04-16T16:29:57Z

Summary

The MCP circuit breaker was counting any tool error toward the threshold, including business-level errors like DNS failures, HTTP 4xx/5xx, and page load failures. This caused healthy MCP servers to be marked as unreachable after just 3 bad URLs.

Root Cause

The circuit breaker incremented the error count for:

Any JSON response containing an "error" key (including tool-level errors)
Any exception raised during the call (including expected tool errors)

Fix

Added _is_infrastructure_error() helper function to distinguish:

Infrastructure errors (connection lost, timeout, process crash, broken pipe) → count toward circuit breaker
Tool-level errors (DNS failure, HTTP errors, page crashes from Playwright) → don't count

Also changed the JSON response handling to not count tool-level errors (when the server returns a valid response with an error field).

Test Plan

Code review confirms the fix addresses the issue
Infrastructure errors (ConnectionError, TimeoutError) will count
Tool-level errors (DNS failures, HTTP errors) won't count

Closes #11113

… tool-level errors The circuit breaker was counting any tool error toward the threshold, including business-level errors like DNS failures, HTTP 4xx/5xx, and page load failures. This caused healthy MCP servers to be marked as unreachable after just 3 bad URLs. Added _is_infrastructure_error() to distinguish: - Infrastructure errors (connection lost, timeout, process crash) → count - Tool-level errors (DNS failure, HTTP errors, page crashes) → don't count Fixes NousResearch#11113

teknium1

Thanks for isolating the distinction between a responsive MCP server returning a domain error and a broken server.

Problems

The proposed pass at tools/mcp_tool.py:1467 avoids an increment but does not reset an existing streak. On current main, the breaker is explicitly consecutive-state machinery (tools/mcp_tool.py:2994-3015), so a responsive {"error": ...} response should close it with _reset_server_error().
The marker-only exception predicate would not preserve the current half-open contract: tests/tools/test_mcp_circuit_breaker.py:184-218 uses RuntimeError("still broken") and requires that failed probe to re-arm the breaker. This predicate would not count that exception.
The diff has no tests for error envelopes or exception classification. Current tools/mcp_tool.py:4000-4004 still bumps every error envelope, so this needs a regression test on the current state machine.

Suggested changes

Salvage onto current main using _reset_server_error() after every completed tool response, including error envelopes.
Add tests for repeated result.isError responses and for the half-open failed-probe path.
Classify exception paths with a contract that preserves the existing breaker safety behavior.

Automated hermes-sweeper review.

teknium1 · 2026-07-12T13:50:36Z

                if "error" in parsed:
-                    _server_error_counts[server_name] = _server_error_counts.get(server_name, 0) + 1
+                    # Tool-level error — server is responsive, don't count
+                    pass


A responsive error envelope should reset, not merely avoid incrementing, the consecutive-failure streak. Otherwise two earlier transport failures remain live and the next transport failure trips a breaker whose failures were not consecutive.

teknium1 · 2026-07-12T13:50:36Z

        except Exception as exc:
-            _server_error_counts[server_name] = _server_error_counts.get(server_name, 0) + 1
+            # Only count infrastructure errors toward circuit breaker
+            if _is_infrastructure_error(exc):


Please add a regression for this classifier. Current main’s half-open breaker test treats RuntimeError("still broken") as a failed probe that must re-arm the breaker; this marker-only predicate returns false for it and would leave the probe path unprotected after salvage.

karlesnine · 2026-07-22T12:39:33Z

Additional reproduction case: HTTP MCP with header-based authentication

Environment

Hermes Agent latest main
MCP server: AgentMail (Streamable HTTP at https://mcp.agentmail.to/mcp)
Auth: header-based (x-api-key: am_us_...)
Transport: HTTP (not stdio)

Scenario

API key got corrupted during a Hermes update → replaced 70-char key with masked placeholder (am_us_...e251, 13 chars)
Every tools/call returned 403 Forbidden (business error — isError: true from AgentMail)
After 3+ consecutive 403s → circuit breaker tripped
API key corrected (restored real 70-char key from pre-update snapshot)
Gateway fully killed + clean restart (killed zombie MCP processes, wiped mcp-stderr.log)

Observed behavior after clean restart

initialize → 200 OK (header auth works)
tools/list → 200 OK (24 tools discovered)

tools/call → blocked locally — NEVER reaches the network

This was confirmed via an HTTP proxy capture (Python http.server interposed between the gateway and AgentMail):

GET  /                          ✅ captured
POST / (initialize)             ✅ captured → 200
POST / (notifications/init)     ✅ captured → 202
POST / (tools/list)             ✅ captured → 200
POST / (tools/call)             ❌ NOT captured — blocked by Hermes

Error returned to the agent: "Forbidden (HTTP 403)" — generated by Hermes locally, not by AgentMail
The x-api-key header is correctly present (70 chars) on all requests that DO reach the proxy
The circuit breaker survives a full gateway kill + clean restart

Expected behavior

After config correction + gateway restart, tools/call should work
Business errors (403 from a temporarily misconfigured auth header) should not permanently disable the server
The circuit breaker should reset on a clean restart

Why this matters for this PR

This demonstrates the circuit breaker can create a permanent lockout for HTTP MCP servers — worse than the ~60s temporary lockout reported for stdio transports. For production setups where MCP config may be temporarily misconfigured (e.g., rotated API keys, config migration), the current behavior requires manual intervention beyond a simple restart.

The fix in this PR (distinguishing infrastructure errors from business errors, resetting on isError: true) would directly prevent this class of issues.

Local test results: patch applied on HTTP MCP with header auth

I cherry-picked this PR onto my local Hermes instance to test against an AgentMail HTTP MCP server (header-based x-api-key auth).

Test setup

Hermes Agent latest main
MCP server: AgentMail (Streamable HTTP, x-api-key header)
Trigger: 3+ consecutive tools/call returning 403 Forbidden (temporarily misconfigured API key)
After fix: corrected API key, gateway restarted fresh

Results

Check	Result
`hermes mcp test agentmail`	✅ 26 tools, 1068ms — connection works fine
JSON error path (`"error" in parsed` → `pass`)	✅ Working — JSON business errors no longer bump counter
`mcp__agentmail__list_inboxes()`	❌ Still returns `403 Forbidden`
Counter after 2 calls	❌ Incremented (10 → 11 consecutive failures)

Why the counter still increments

The 403 Forbidden from AgentMail likely arrives as an httpx.HTTPStatusError exception (not a JSON isError response), which goes through the except Exception as exc: path. The _is_infrastructure_error() guard correctly returns False for "403 Forbidden" (none of the patterns match), so the counter should NOT be bumped — but it still is.

I suspect the 403 triggers a reconnect path (via _handle_auth_error_and_retry or _handle_session_expired_and_retry) which, on failure, calls _bump_server_error unconditionally at line ~3913 — outside the scope of this PR's changes.

Suggestion

The _is_infrastructure_error() pattern list could be extended to explicitly exclude HTTP status errors:

# HTTP 4xx/5xx from a responsive server are NOT infrastructure errors
if isinstance(exc, httpx.HTTPStatusError):
    return False

Or the unconditional _bump_server_error at line ~3913 (auth recovery failure path) should also be guarded.

Overall

The PR fixes the JSON business error case — confirmed. For HTTP auth errors (403), additional paths need coverage. This is still the right direction.

Port of upstream issue NousResearch#11113 / PR NousResearch#11128, re-based onto current main's consecutive-failure breaker state machine and revised per the PR review. Root cause: a tool-level error comes back from _call as a returned {"error": ...} result (isError content: bad path, HTTP 4xx upstream, validation rejection, "old_text not found"). The success path bumped the breaker on any such result, so a few innocent errors from a *healthy* server (reproduced on victoria as mcp_vault_edit "old_text not found") tripped it for 60s and every vault tool then short-circuited as 'unreachable'. Fix: the success path now resets the consecutive-failure streak on ANY returned result — a response means the server is responsive, and resetting (not merely skipping the increment) is what the breaker's consecutive-state machinery needs so earlier transport blips don't linger and sum with a later one. The exception path is left as upstream's unconditional _bump_server_error: those exceptions are genuine transport/protocol failures (call_tool raised, not isError), and the half-open probe relies on a failed probe re-arming the breaker (tests/tools/test_mcp_circuit_breaker.py:: test_circuit_breaker_reopens_on_probe_failure). This drops the earlier hand-port's _is_infrastructure_error exception classifier, which would have left that probe path unprotected. Tests: repeated isError results keep the breaker fully closed; a responsive error result resets a partial transport streak. Local port pending upstream NousResearch#11128 merging.

Port of upstream issue NousResearch#11113 / PR NousResearch#11128, re-based onto current main's consecutive-failure breaker state machine and revised per the PR review. Root cause: a tool-level error comes back from the call as a returned {"error": ...} result (isError content: bad path, HTTP 4xx upstream, validation rejection, "old_text not found"). _record_call_outcome bumped the breaker on any such result, so a few innocent errors from a *healthy* server (reproduced on victoria as mcp_vault_edit "old_text not found") tripped it for 60s and every vault tool then short-circuited as 'unreachable'. Fix: _record_call_outcome now resets the consecutive-failure streak on ANY returned result — a response means the server is responsive, and resetting (not merely skipping the increment) is what the breaker's consecutive-state machinery needs so earlier transport blips don't linger and sum with a later one. The transport paths (_strike and the _bump_server_error call sites around acquire/reconnect) are left as upstream's: those are genuine transport failures, and the half-open probe relies on a failed probe re-arming the breaker (test_circuit_breaker_reopens_on_probe_failure). Tests: repeated isError results keep the breaker fully closed; a responsive error result resets a partial transport streak. Both fail on unpatched upstream and pass with the fix. Re-ported this sync for two upstream moves: the breaker bookkeeping now lives in tools/mcp_tool_handlers.py::_record_call_outcome rather than inline in _make_tool_handler, and the tests' loop helper moved to tools.mcp_tool_loop. Local port pending upstream NousResearch#11128 merging.

teknium1 · 2026-09-12T14:01:05Z

Thanks @nightq — #109114 lands the structural form of this (a completed RPC resets the breaker; only transport exceptions strike), which avoids classifying exception strings. Will close this with credit once it merges.

teknium1 · 2026-09-12T15:51:03Z

Closing with a maintainer ruling rather than a quality verdict. Tool isError payloads keep counting as circuit-breaker strikes: that is #10447's original intent (a server answering errors made the model hammer it 8x in 10s) and #109180 reasserted it on main today. The real symptom behind these reports is the open-breaker message: after three rejected calls the model was told the server was "unreachable" and gave up on the task. #109245 fixes the wording ("rejected the last 3 calls — fix the arguments"), keeps the strikes, and restores "unreachable" the moment a transport strike appears. Thanks for digging into this; the symptom analysis was right, the fix landed at a different layer.

Local divergence from upstream, re-ported onto this sync's shape. Root cause: a tool-level error comes back from the call as a returned {"error": ...} result (isError content: bad path, HTTP 4xx upstream, validation rejection, "old_text not found"). _record_call_outcome counts any such result as a breaker strike, so a few innocent errors from a *healthy* server (reproduced on victoria as mcp_vault_edit "old_text not found") trip it for 60s and every tool on that server then short-circuits — including the search tool needed to find the right old_text. Fix: _record_call_outcome resets the consecutive-failure streak on ANY returned result — a response means the server is responsive, and resetting (not merely skipping the increment) is what the breaker's consecutive-state machinery needs so earlier transport blips don't linger and sum with a later one. The transport paths (_strike and the _bump_server_error call sites around acquire/reconnect) are left as upstream's: those are genuine transport failures, and the half-open probe relies on a failed probe re-arming the breaker (test_circuit_breaker_reopens_on_probe_failure). Upstream status changed this sync: PR NousResearch#11128 was CLOSED UNMERGED and issue NousResearch#11113 closed as COMPLETED by a different change. Upstream's resolution keeps counting returned error payloads as strikes (citing NousResearch#10447) and only adds an `application=True` flag so the open-breaker wording says "rejected the last N calls (it is reachable)" instead of "unreachable". That fixes the message, not the 60s lockout we were bitten by, so this patch stays. Because of that, upstream's new test_breaker_opened_by_tool_errors_says_rejected_not_unreachable asserts the behaviour we override. Adapted: it now stages the application strikes via direct _bump_server_error(application=True) calls — exactly what its own second half already does — so it still pins the wording behaviour without asserting that a returned error payload is a strike. Tests: repeated isError results keep the breaker fully closed; a responsive error result resets a partial transport streak. Both fail on unpatched upstream and pass with the fix.

alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/mcp MCP client and OAuth labels Apr 25, 2026

alt-glitch mentioned this pull request May 26, 2026

fix(mcp): don't trip circuit breaker on tool-level semantic errors #32728

Closed

13 tasks

alt-glitch mentioned this pull request Jun 17, 2026

MCP circuit breaker triggered by business errors (isError) instead of actual server failures #47851

Open

teknium1 reviewed Jul 12, 2026

View reviewed changes

teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 12, 2026

This was referenced Jul 19, 2026

MCP circuit breaker treats tool-level errors (DNS failure, HTTP 4xx/5xx) as server failures, triggers false circuit break #11113

Closed

fix(mcp): don't count tool-level errors toward the circuit breaker (#11113) #67340

Closed

This was referenced Sep 12, 2026

fix(mcp): tool-level error payloads no longer trip the server circuit breaker (#11113, salvage #61555) #109114

Closed

fix(mcp): semantic tool errors must not trip circuit breaker #61555

Closed

teknium1 mentioned this pull request Sep 12, 2026

fix(mcp): breaker opened by tool errors tells the model the calls were rejected, not that the server is unreachable (#11113) #109245

Merged

teknium1 closed this Sep 12, 2026

Navigation Menu

fix: MCP circuit breaker should only count infrastructure errors - #11128

fix: MCP circuit breaker should only count infrastructure errors#11128
nightq wants to merge 1 commit into
NousResearch:mainfrom
nightq:fix/issue-11113-mcp-circuit-breaker-tool-errors

nightq commented Apr 16, 2026

Uh oh!

teknium1 left a comment

Uh oh!

teknium1 Jul 12, 2026

Uh oh!

teknium1 Jul 12, 2026

Uh oh!

karlesnine commented Jul 22, 2026

Uh oh!

karlesnine commented Jul 22, 2026

Uh oh!

teknium1 commented Sep 12, 2026

Uh oh!

teknium1 commented Sep 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

nightq commented Apr 16, 2026

Summary

Root Cause

Fix

Test Plan

Uh oh!

teknium1 left a comment

Choose a reason for hiding this comment

Problems

Suggested changes

Uh oh!

teknium1 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

teknium1 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

karlesnine commented Jul 22, 2026

Additional reproduction case: HTTP MCP with header-based authentication

Environment

Scenario

Observed behavior after clean restart

Expected behavior

Why this matters for this PR

Related

Uh oh!

karlesnine commented Jul 22, 2026

Local test results: patch applied on HTTP MCP with header auth

Test setup

Results

Why the counter still increments

Suggestion

Overall

Uh oh!

teknium1 commented Sep 12, 2026

Uh oh!

teknium1 commented Sep 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants