Skip to content

fix(mcp): add lock protection to circuit breaker error counters - #60666

Open
isheng-eqi wants to merge 1 commit into
NousResearch:mainfrom
isheng-eqi:fix/mcp-circuit-breaker-lock
Open

fix(mcp): add lock protection to circuit breaker error counters#60666
isheng-eqi wants to merge 1 commit into
NousResearch:mainfrom
isheng-eqi:fix/mcp-circuit-breaker-lock

Conversation

@isheng-eqi

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds thread-safety to MCP circuit breaker error tracking. _bump_server_error and _reset_server_error were accessing shared dicts (_server_error_counts, _server_breaker_opened_at) without holding _lock, risking lost updates under concurrent access from the MCP loop thread and caller thread.

Also fixes _handle_session_expired_and_retry which was directly assigning _server_error_counts[name] = 0 instead of calling _reset_server_error(name), leaving stale breaker timestamps.

Type of Change

  • Bug fix

Changes Made

  • tools/mcp_tool.py: wrapped _bump_server_error and _reset_server_error bodies in with _lock. Replaced two direct dict assignments with _reset_server_error calls.
  • tests/tools/test_mcp_circuit_breaker.py: added test verifying reset clears both count and timestamp.

How to Test

python -m pytest tests/tools/test_mcp_circuit_breaker.py -v

@alt-glitch alt-glitch added type/bug Something isn't working tool/mcp MCP client and OAuth P3 Low — cosmetic, nice to have labels Jul 8, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing a real MCP circuit-breaker race. Current main still has the unlocked read-modify-write at tools/mcp_tool.py:3001-3004, and the helper locking plus session-expiry helper calls target that defect directly.

Problems

  • tests/tools/test_mcp_circuit_breaker.py:572 only invokes _bump_server_error and _reset_server_error. Current main already clears both breaker maps in _reset_server_error (tools/mcp_tool.py:3014-3015), so the test passes without either production change in this PR. It does not cover the changed successful-retry branches at tools/mcp_tool.py:3414 and 3417.

Suggested changes

  • Exercise _handle_session_expired_and_retry after a tripped breaker and assert a successful retry clears both the count and timestamp.
  • Add deterministic concurrent-bump coverage so the locking regression is observable.

Automated hermes-sweeper review.

@@ -569,3 +569,29 @@ async def _run_stdio(self, config):
run_task.cancel()

asyncio.run(_scenario())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes on current main because _reset_server_error already sets the count to zero and removes the timestamp (tools/mcp_tool.py:3014-3015). Please exercise the successful _handle_session_expired_and_retry retry path, which is where this PR changes reset behavior.

_bump_server_error and _reset_server_error performed read-modify-write on shared dicts without holding _lock, risking lost updates under concurrent access from MCP loop thread and caller thread. Also replaced two direct _server_error_counts assignments in _handle_session_expired_and_retry with _reset_server_error calls to ensure breaker timestamp is properly cleared.
@isheng-eqi
isheng-eqi force-pushed the fix/mcp-circuit-breaker-lock branch from 5e6b227 to 9367f94 Compare July 10, 2026 15:17
@isheng-eqi

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review! Added two tests addressing both concerns:

  1. test_handle_session_expired_retry_clears_breaker: Exercises _handle_session_expired_and_retry with a pre-tripped breaker and asserts that both code paths — valid JSON (line ~3414) and non-JSON/decode-error (line ~3418) — clear the error count AND the breaker timestamp after a successful retry. The old test only covered _bump_server_error + _reset_server_error directly; this one exercises the actual retry branches the lock change touched.

  2. test_concurrent_bump_server_error_is_race_free: 8 threads × 10 bumps each, asserts the final count equals exactly 80 — proving the lock prevents lost increments under deterministic concurrency.

Please take another look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants