Skip to content

fix(mcp): structured circuit breaker errors to prevent AI retry storms #899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
murdore merged 1 commit into from
Mar 25, 2026
Merged

fix(mcp): structured circuit breaker errors to prevent AI retry storms #899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ jobs:
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: "20"
node-version: "22"
cache: "pnpm"

- name: Install ffmpeg
Expand Down Expand Up @@ -197,7 +197,7 @@ jobs:
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: "20"
node-version: "22"
cache: "pnpm"

- name: Install ffmpeg
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/copilot-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ jobs:
- name: Setup PNPM
uses: pnpm/action-setup@v4
with:
version: 8
version: 9

- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: "20"
node-version: "22"
cache: "pnpm"

- name: Install dependencies
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ jobs:
- name: Setup PNPM
uses: pnpm/action-setup@v4
with:
version: 8
version: 9

- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: "18"
node-version: "22"
cache: "pnpm"
registry-url: https://registry.npmjs.org/

Expand Down Expand Up @@ -73,7 +73,7 @@ jobs:
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: "20"
node-version: "22"
registry-url: "https://registry.npmjs.org"
cache: "pnpm"

Expand Down Expand Up @@ -111,7 +111,7 @@ jobs:
- name: Setup Node for GitHub Packages
uses: actions/setup-node@v4
with:
node-version: "20"
node-version: "22"
registry-url: "https://npm.pkg.github.com"
scope: "@juspay"

Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,8 @@
"@types/ws": "^8.18.1",
"@types/xml2js": "^0.4.14",
"@types/yargs": "^17.0.35",
"@typescript-eslint/eslint-plugin": "^8.56.1",
"@typescript-eslint/parser": "^8.56.1",
"@typescript-eslint/eslint-plugin": "^8.57.2",
"@typescript-eslint/parser": "^8.57.2",
"@vercel/ncc": "^0.38.4",
"@vitest/coverage-v8": "^4.1.0",
"concurrently": "^9.2.1",
Expand Down
120 changes: 60 additions & 60 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,7 @@ export {
isTokenExpired,
listMCPs,
// Circuit Breaker
CircuitBreakerOpenError,
MCPCircuitBreaker,
mcpLogger,
NeuroLinkOAuthProvider,
Expand Down
1 change: 1 addition & 0 deletions src/lib/mcp/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export {
CircuitBreakerManager,
globalCircuitBreakerManager,
MCPCircuitBreaker,
CircuitBreakerOpenError,
} from "./mcpCircuitBreaker.js";
// Core MCP Components
export { MCPToolRegistry } from "./toolRegistry.js";
Expand Down
36 changes: 30 additions & 6 deletions src/lib/mcp/mcpCircuitBreaker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ import type {
CircuitBreakerEvents,
} from "../types/mcpTypes.js";

import { CircuitBreakerOpenError } from "../types/circuitBreakerErrors.js";

// Re-export CircuitBreakerOpenError from shared types to preserve public API
export { CircuitBreakerOpenError } from "../types/circuitBreakerErrors.js";

/**
* Default operation timeout for circuit breaker protected operations.
* Configurable via MCP_OPERATION_TIMEOUT env var (in milliseconds).
Expand Down Expand Up @@ -69,10 +74,18 @@ export class MCPCircuitBreaker extends EventEmitter {
try {
// Check if circuit is open
if (this.state === "open") {
if (Date.now() - this.lastFailureTime < this.config.resetTimeout) {
throw new Error(
`Circuit breaker '${this.name}' is open. Next retry at ${new Date(this.lastFailureTime + this.config.resetTimeout)}`,
);
const retryAfterMs =
this.config.resetTimeout - (Date.now() - this.lastFailureTime);
if (retryAfterMs > 0) {
throw new CircuitBreakerOpenError({
breakerName: this.name,
retryAfter: new Date(
this.lastFailureTime + this.config.resetTimeout,
),
retryAfterMs,
breakerState: "open",
failureCount: this.getStats().failedCalls,
});
}
Comment on lines 76 to 89

@coderabbitai coderabbitai Bot Mar 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Semantic mismatch: failureCount vs "consecutive failures" message.

The error message at line 45 says "consecutive failures" but this.getStats().failedCalls returns the total failed calls within the statistics window, not consecutive failures. The circuit breaker opens based on failureThreshold being exceeded, but that's compared against total failures in the window, not a consecutive count.

Consider either:

  1. Updating the message text to say "failures" instead of "consecutive failures"
  2. Or tracking actual consecutive failures separately if that's the intended semantic
🔧 Suggested message fix 
     super(
       `Circuit breaker '${options.breakerName}' is ${options.breakerState}. ` +
-        `Tool temporarily unavailable after ${options.failureCount} consecutive failures. ` +
+        `Tool temporarily unavailable after ${options.failureCount} failures. ` +
         `Retry after: ${retryAfterStr} (${Math.ceil(options.retryAfterMs / 1000)}s).`,
     );
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/lib/mcp/mcpCircuitBreaker.ts` around lines 110 - 123, The
CircuitBreakerOpenError currently reports failureCount using
this.getStats().failedCalls but the message text implies "consecutive failures";
update the error payload to reflect the actual metric or change the message:
either replace any "consecutive failures" wording with "failures" (or "failed
calls") to match this.getStats().failedCalls, or implement a separate
consecutive failure counter (e.g., track consecutiveFailures on the circuit
breaker instance, increment on failure and reset on success, and use that value
in the error payload). Ensure changes reference the existing symbols:
CircuitBreakerOpenError, this.getStats().failedCalls, failureThreshold,
lastFailureTime, config.resetTimeout, and this.name so the error fields remain
consistent.


// Transition to half-open
Expand All @@ -84,9 +97,20 @@ export class MCPCircuitBreaker extends EventEmitter {
this.state === "half-open" &&
this.halfOpenCalls >= this.config.halfOpenMaxCalls
) {
throw new Error(
`Circuit breaker '${this.name}' is half-open but call limit reached`,
// Half-open call limit exceeded — revert to open with fresh cooldown
this.lastFailureTime = Date.now();
this.changeState(
"open",
"Half-open call limit reached, reverting to open",
);

throw new CircuitBreakerOpenError({
breakerName: this.name,
retryAfter: new Date(this.lastFailureTime + this.config.resetTimeout),
retryAfterMs: this.config.resetTimeout,
breakerState: "open",
failureCount: this.getStats().failedCalls,
});
Comment on lines 80 to +113

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

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

CircuitBreakerOpenError message and field docs say "consecutive failures", but failureCount is populated from this.getStats().failedCalls, which is the total failed calls in the history/window (not consecutive). This makes the message/metadata inaccurate and can mislead downstream handling. Either track and pass a true consecutive-failure counter, or change the property/message wording to reflect what’s actually measured (e.g., "failed calls in window").

Copilot uses AI. Check for mistakes.
}

// NLK-GAP-009: Record half-open test event when executing in half-open state
Expand Down
Loading
Loading