Repository navigation
fix(mcp): structured circuit breaker errors to prevent AI retry storms - #899
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Tool Caller
participant ToolDiscoveryService
participant MCPCircuitBreaker
participant ErrorHandler
Tool Caller->>ToolDiscoveryService: executeTool()
ToolDiscoveryService->>MCPCircuitBreaker: execute(operation)
alt Breaker is Open & Retry Timeout Not Elapsed
MCPCircuitBreaker->>MCPCircuitBreaker: Compute retryAfterMs
MCPCircuitBreaker->>MCPCircuitBreaker: Create CircuitBreakerOpenError
MCPCircuitBreaker-->>ToolDiscoveryService: throw CircuitBreakerOpenError
else Breaker is Half-Open & Max Calls Exceeded
MCPCircuitBreaker->>MCPCircuitBreaker: Compute retryAfterMs (clamped)
MCPCircuitBreaker->>MCPCircuitBreaker: Create CircuitBreakerOpenError
MCPCircuitBreaker-->>ToolDiscoveryService: throw CircuitBreakerOpenError
else Breaker Closed or Available
MCPCircuitBreaker->>MCPCircuitBreaker: Execute operation
MCPCircuitBreaker-->>ToolDiscoveryService: return result
end
ToolDiscoveryService->>ErrorHandler: catch CircuitBreakerOpenError
ErrorHandler->>ErrorHandler: Extract breaker state & retry timing
ErrorHandler->>ErrorHandler: Build user-facing message with cooldown info
ErrorHandler-->>Tool Caller: return {success: false, data: {text, circuitBreaker metadata}}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull request overview
This PR introduces a typed, metadata-rich circuit-breaker error and attempts to surface an AI-actionable “do not retry” message when MCP tool execution is blocked by an open/half-open circuit breaker, to prevent runaway retry storms.
Changes:
- Added
CircuitBreakerOpenErrorwith structured metadata (breaker name/state, retry-after, failure count). - Updated circuit breaker implementations to throw
CircuitBreakerOpenErrorwhen blocking execution. - Updated MCP tool execution to catch
CircuitBreakerOpenErrorand return an AI-readable tool error payload.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/utils/errorHandling.ts | Updates the generic CircuitBreaker to throw CircuitBreakerOpenError when open. |
| src/lib/mcp/toolDiscoveryService.ts | Catches CircuitBreakerOpenError and builds a structured isError tool result with “do NOT retry” guidance. |
| src/lib/mcp/mcpCircuitBreaker.ts | Adds CircuitBreakerOpenError and uses it in MCPCircuitBreaker.execute() for open/half-open blocking cases. |
| src/lib/mcp/index.ts | Re-exports CircuitBreakerOpenError for downstream consumers. |
| package.json | Bumps @typescript-eslint/* devDependencies. |
| pnpm-lock.yaml | Lockfile updates for the @typescript-eslint/* bump (and transitive changes). |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return { | ||
| success: false, | ||
| data: { | ||
| isError: true, | ||
| content: [ | ||
| { | ||
| type: "text" as const, | ||
| text: | ||
| `TOOL TEMPORARILY UNAVAILABLE: "${toolName}" has been disabled after ` + | ||
| `${error.failureCount} consecutive failures. ` + | ||
| `This is a circuit breaker protection — do NOT retry this tool. ` + | ||
| `It will become available again after ${Math.ceil(error.retryAfterMs / 1000)} seconds ` + | ||
| `(at ${error.retryAfter}). ` + | ||
| `Instead, inform the user that the operation failed and suggest trying again later.`, | ||
| }, | ||
| ], | ||
| }, | ||
| duration, | ||
| metadata: { | ||
| toolName, | ||
| serverId, | ||
| timestamp: Date.now(), | ||
| circuitBreaker: { | ||
| state: error.breakerState, | ||
| retryAfter: error.retryAfter, | ||
| retryAfterMs: error.retryAfterMs, | ||
| failureCount: error.failureCount, | ||
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The CircuitBreakerOpenError branch returns success: false with an isError payload in data, but it does not set the top-level error field. Callers like ExternalServerManager.executeTool() throw new Error(result.error || "Tool execution failed"), so the actionable circuit-breaker message will be dropped and replaced with a generic error. Include the same message in error (and/or adjust the success/data contract) so downstream callers preserve the intended "do NOT retry" guidance.
| `TOOL TEMPORARILY UNAVAILABLE: "${toolName}" has been disabled after ` + | ||
| `${error.failureCount} consecutive failures. ` + | ||
| `This is a circuit breaker protection — do NOT retry this tool. ` + | ||
| `It will become available again after ${Math.ceil(error.retryAfterMs / 1000)} seconds ` + | ||
| `(at ${error.retryAfter}). ` + | ||
| `Instead, inform the user that the operation failed and suggest trying again later.`, | ||
| }, |
There was a problem hiding this comment.
This user-facing/tool-facing text says the tool was disabled after "consecutive failures", but CircuitBreakerOpenError.failureCount (as currently set by MCPCircuitBreaker) is not necessarily consecutive. To avoid giving incorrect guidance, either ensure failureCount truly reflects consecutive failures, or change this message to a neutral phrasing like "after N failures" / "after exceeding the failure threshold".
| // Circuit breaker open errors: return a structured isError result with | ||
| // actionable details so AI models understand the tool is temporarily | ||
| // unavailable and should NOT retry until the cooldown expires. | ||
| if (error instanceof CircuitBreakerOpenError) { | ||
| mcpLogger.warn( | ||
| `[ToolDiscoveryService] Tool blocked by circuit breaker: ${toolName} on ${serverId}`, | ||
| { | ||
| breakerState: error.breakerState, | ||
| retryAfter: error.retryAfter, | ||
| retryAfterMs: error.retryAfterMs, | ||
| failureCount: error.failureCount, | ||
| }, | ||
| ); | ||
|
|
||
| return { | ||
| success: false, | ||
| data: { | ||
| isError: true, | ||
| content: [ | ||
| { | ||
| type: "text" as const, | ||
| text: | ||
| `TOOL TEMPORARILY UNAVAILABLE: "${toolName}" has been disabled after ` + | ||
| `${error.failureCount} consecutive failures. ` + | ||
| `This is a circuit breaker protection — do NOT retry this tool. ` + | ||
| `It will become available again after ${Math.ceil(error.retryAfterMs / 1000)} seconds ` + | ||
| `(at ${error.retryAfter}). ` + | ||
| `Instead, inform the user that the operation failed and suggest trying again later.`, | ||
| }, | ||
| ], | ||
| }, | ||
| duration, | ||
| metadata: { | ||
| toolName, | ||
| serverId, | ||
| timestamp: Date.now(), | ||
| circuitBreaker: { | ||
| state: error.breakerState, | ||
| retryAfter: error.retryAfter, | ||
| retryAfterMs: error.retryAfterMs, | ||
| failureCount: error.failureCount, | ||
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
New behavior is introduced for the open/half-open circuit-breaker path (typed error + special tool result). There don’t appear to be tests covering circuit-breaker blocking or verifying that the returned message/metadata is preserved through the external tool execution stack. Adding a regression test (e.g., in the MCP continuous test suite) that forces the breaker open and asserts the "TOOL TEMPORARILY UNAVAILABLE" payload (and no retries) would help prevent the retry-storm issue from reappearing.
| import { ErrorCategory, ErrorSeverity } from "../constants/enums.js"; | ||
| import type { StructuredError } from "../types/utilities.js"; | ||
| import { logger } from "./logger.js"; | ||
| import { CircuitBreakerOpenError } from "../mcp/mcpCircuitBreaker.js"; |
There was a problem hiding this comment.
errorHandling.ts now imports CircuitBreakerOpenError from the MCP layer (../mcp/mcpCircuitBreaker.js). This creates a dependency from the general-purpose error utilities into the MCP subsystem and can make reuse/bundling harder (and increases risk of future circular deps since src/lib/mcp/index.ts already imports from ../utils/errorHandling.js). Consider moving CircuitBreakerOpenError to a shared/utils module (or defining a local error type here) and re-exporting it from MCP if needed.
| throw new CircuitBreakerOpenError({ | ||
| breakerName: "tool-execution", | ||
| retryAfter: new Date(this.lastFailureTime + this.resetTimeoutMs), | ||
| retryAfterMs, | ||
| breakerState: "open", | ||
| failureCount: this.failures, | ||
| }); |
There was a problem hiding this comment.
breakerName is hard-coded to "tool-execution" when throwing CircuitBreakerOpenError. Since this CircuitBreaker instance is used per-tool (e.g., keyed by server/tool elsewhere), the metadata/message will be misleading and makes it hard to identify which breaker opened. Consider making the breaker name configurable (constructor param) or deriving it from the call site so the error accurately identifies the blocked operation.
| @@ -84,9 +131,16 @@ 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`, | |||
| ); | |||
| throw new CircuitBreakerOpenError({ | |||
| breakerName: this.name, | |||
| retryAfter: new Date(this.lastFailureTime + this.config.resetTimeout), | |||
| retryAfterMs: Math.max( | |||
| 0, | |||
| this.config.resetTimeout - (Date.now() - this.lastFailureTime), | |||
| ), | |||
| breakerState: "half-open", | |||
| failureCount: this.getStats().failedCalls, | |||
| }); | |||
There was a problem hiding this comment.
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").
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/mcp/mcpCircuitBreaker.ts (1)
130-144:⚠️ Potential issue | 🟡 MinorPotentially stale
retryAfterin half-open state.When the circuit is half-open and the call limit is reached,
retryAfteris computed fromlastFailureTime + resetTimeout. However,lastFailureTimewas set when the circuit originally opened, not when half-open calls started. This could result in:
retryAfterMsbeing 0 or near-0 (due toMath.max(0, ...))retryAftertimestamp already in the pastIf a half-open call fails, the circuit transitions to open and updates
lastFailureTime(line 196). But when the limit is reached without failure, the timing is stale. Consider whether the half-open call limit should trigger a transition back to"open"state with fresh timing, or document that this is expected behavior.🤖 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 130 - 144, When half-open call limit is reached (the block checking this.state === "half-open" && this.halfOpenCalls >= this.config.halfOpenMaxCalls), don't compute retryAfter from the original lastFailureTime (stale); either record a fresh failure time and transition the breaker to "open" or compute retry based on when half-open started. Update the code to set this.lastFailureTime = Date.now() and this.state = "open" (or use a new halfOpenStartTime if you prefer) before throwing CircuitBreakerOpenError so retryAfter and retryAfterMs are derived from the fresh timestamp (use this.config.resetTimeout) and include accurate breakerState and failureCount from getStats().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/mcp/mcpCircuitBreaker.ts`:
- Around line 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.
In `@src/lib/mcp/toolDiscoveryService.ts`:
- Around line 586-616: The CircuitBreakerOpenError return value in
toolDiscoveryService.ts (the block returning success: false with data.isError)
is missing the top-level error field that other error paths include; update that
returned object to include an error property (consistent with the path at the
later return around line 624) containing the error message (e.g., use
error.message or construct errorMessage) so callers like
externalServerManager.ts can reliably read discoveryResult.error; keep the rest
of the payload (data, metadata.circuitBreaker) unchanged.
In `@src/lib/utils/errorHandling.ts`:
- Around line 919-933: The catch blocks around circuitBreaker.execute() calls in
the neurolink and mcpClientFactory call sites need an explicit instanceof check
for CircuitBreakerOpenError: update the catch to first detect if (error
instanceof CircuitBreakerOpenError) and return a structured non-retryable
response that preserves breakerState, retryAfter (or retryAfterMs) and
failureCount (or otherwise surface those fields) so callers can distinguish an
open breaker from other failures; otherwise proceed with the existing error
handling (rethrow or handle as before). Ensure CircuitBreakerOpenError is
imported/available at those call sites and mirror the structured response
pattern used by the toolDiscoveryService example.
---
Outside diff comments:
In `@src/lib/mcp/mcpCircuitBreaker.ts`:
- Around line 130-144: When half-open call limit is reached (the block checking
this.state === "half-open" && this.halfOpenCalls >=
this.config.halfOpenMaxCalls), don't compute retryAfter from the original
lastFailureTime (stale); either record a fresh failure time and transition the
breaker to "open" or compute retry based on when half-open started. Update the
code to set this.lastFailureTime = Date.now() and this.state = "open" (or use a
new halfOpenStartTime if you prefer) before throwing CircuitBreakerOpenError so
retryAfter and retryAfterMs are derived from the fresh timestamp (use
this.config.resetTimeout) and include accurate breakerState and failureCount
from getStats().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9462015-10cc-428e-8cf9-cb8a063eebe9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
package.jsonsrc/lib/mcp/index.tssrc/lib/mcp/mcpCircuitBreaker.tssrc/lib/mcp/toolDiscoveryService.tssrc/lib/utils/errorHandling.ts
| 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, | ||
| }); | ||
| } |
There was a problem hiding this comment.
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:
- Updating the message text to say "failures" instead of "consecutive failures"
- 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.
| return { | ||
| success: false, | ||
| data: { | ||
| isError: true, | ||
| content: [ | ||
| { | ||
| type: "text" as const, | ||
| text: | ||
| `TOOL TEMPORARILY UNAVAILABLE: "${toolName}" has been disabled after ` + | ||
| `${error.failureCount} consecutive failures. ` + | ||
| `This is a circuit breaker protection — do NOT retry this tool. ` + | ||
| `It will become available again after ${Math.ceil(error.retryAfterMs / 1000)} seconds ` + | ||
| `(at ${error.retryAfter}). ` + | ||
| `Instead, inform the user that the operation failed and suggest trying again later.`, | ||
| }, | ||
| ], | ||
| }, | ||
| duration, | ||
| metadata: { | ||
| toolName, | ||
| serverId, | ||
| timestamp: Date.now(), | ||
| circuitBreaker: { | ||
| state: error.breakerState, | ||
| retryAfter: error.retryAfter, | ||
| retryAfterMs: error.retryAfterMs, | ||
| failureCount: error.failureCount, | ||
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ExternalMCPToolResult type definition and workflow usage
echo "=== ExternalMCPToolResult type definition ==="
rg -n "type ExternalMCPToolResult|interface ExternalMCPToolResult" --type=ts -A 15
echo -e "\n=== Workflow handlers checking result.error ==="
rg -n "result\.error" src/lib/workflow/ --type=ts -B2 -A2Repository: juspay/neurolink
Length of output: 3212
🏁 Script executed:
#!/bin/bash
# Check the regular error path in toolDiscoveryService.ts (lines 623-632)
echo "=== Regular error path in toolDiscoveryService.ts (lines 623-632) ==="
sed -n '620,635p' src/lib/mcp/toolDiscoveryService.ts
# Check how ExternalMCPToolResult is used when success is false
echo -e "\n=== Usage of ExternalMCPToolResult in toolDiscoveryService.ts ==="
grep -n "success: false" src/lib/mcp/toolDiscoveryService.ts | head -20
# Check if there are any usages that specifically check for error field
echo -e "\n=== Code checking error field on ExternalMCPToolResult ==="
rg -n "\.error\s*[?]?:" src/lib/mcp/ --type=ts -B2 -A2 | head -40Repository: juspay/neurolink
Length of output: 606
🏁 Script executed:
#!/bin/bash
# Check how toolDiscoveryService results are consumed
echo "=== Callers of toolDiscoveryService methods ==="
rg -n "toolDiscoveryService\." src/lib/ --type=ts | head -20
# Check if callers access the error field
echo -e "\n=== Code accessing result.error from toolDiscoveryService ==="
rg -n "executeToolWithFallback|executeTool" src/lib/mcp/toolDiscoveryService.ts --type=ts | grep "export\|function" | head -10
# Look at other error returns in the same file to confirm pattern
echo -e "\n=== All success: false returns in toolDiscoveryService.ts ==="
grep -n "success: false" src/lib/mcp/toolDiscoveryService.ts -A 10 | grep -E "success|error:" | head -30Repository: juspay/neurolink
Length of output: 917
🏁 Script executed:
#!/bin/bash
# Check how results from toolDiscoveryService are used in externalServerManager
echo "=== Usage of toolDiscoveryService results in externalServerManager.ts ==="
rg -n "executeToolWithFallback|\.error|result\.error" src/lib/mcp/externalServerManager.ts --type=ts -B3 -A3
# Check main execution flow
echo -e "\n=== Check executeTool usage and error handling ==="
rg -n "executeTool\(" src/lib/mcp/ --type=ts -B2 -A5 | head -50Repository: juspay/neurolink
Length of output: 9678
Add error field to CircuitBreakerOpenError response for consistency with other error paths.
The CircuitBreakerOpenError handler (lines 586-616) returns a response without an error field, while all other error paths in this method (lines 75, 149, 624) include it. This breaks consistency and causes undefined values to propagate when callers access result.error directly.
For example, in externalServerManager.ts, line 1688 logs:
`Tool discovery failed for ${serverId}: ${discoveryResult.error}`When discoveryResult comes from a CircuitBreakerOpenError, this logs "undefined", degrading observability.
The regular error path at line 624 includes error: errorMessage. Apply the same pattern here.
🐛 Proposed fix
return {
success: false,
+ error: `TOOL TEMPORARILY UNAVAILABLE: "${toolName}" has been disabled after ${error.failureCount} consecutive failures`,
data: {
isError: true,
content: [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/mcp/toolDiscoveryService.ts` around lines 586 - 616, The
CircuitBreakerOpenError return value in toolDiscoveryService.ts (the block
returning success: false with data.isError) is missing the top-level error field
that other error paths include; update that returned object to include an error
property (consistent with the path at the later return around line 624)
containing the error message (e.g., use error.message or construct errorMessage)
so callers like externalServerManager.ts can reliably read
discoveryResult.error; keep the rest of the payload (data,
metadata.circuitBreaker) unchanged.
| async execute<T>(operation: () => Promise<T>): Promise<T> { | ||
| if (this.state === "open") { | ||
| if (Date.now() - this.lastFailureTime > this.resetTimeoutMs) { | ||
| this.state = "half-open"; | ||
| } else { | ||
| throw new Error("Circuit breaker is open - operation not executed"); | ||
| const retryAfterMs = | ||
| this.resetTimeoutMs - (Date.now() - this.lastFailureTime); | ||
| if (retryAfterMs > 0) { | ||
| throw new CircuitBreakerOpenError({ | ||
| breakerName: "tool-execution", | ||
| retryAfter: new Date(this.lastFailureTime + this.resetTimeoutMs), | ||
| retryAfterMs, | ||
| breakerState: "open", | ||
| failureCount: this.failures, | ||
| }); | ||
| } | ||
| this.state = "half-open"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all CircuitBreaker.execute() call sites and check for instanceof CircuitBreakerOpenError handling
echo "=== Searching for CircuitBreaker usage and error handling ==="
# Find imports of CircuitBreaker from errorHandling
rg -n "import.*CircuitBreaker.*from.*errorHandling" --type=ts
echo -e "\n=== Checking for CircuitBreakerOpenError handling at call sites ==="
# Find files that use CircuitBreaker but don't import CircuitBreakerOpenError
rg -l "circuitBreaker\.execute" --type=ts | xargs -I {} sh -c '
if ! rg -q "CircuitBreakerOpenError" "{}"; then
echo "Missing CircuitBreakerOpenError handling: {}"
fi
'
echo -e "\n=== Checking neurolink.ts for circuit breaker error handling ==="
rg -n -A10 "circuitBreaker\.execute" src/lib/neurolink.ts | head -60Repository: juspay/neurolink
Length of output: 2117
🏁 Script executed:
# Get full context of the circuitBreaker.execute() calls in neurolink.ts
rg -n -B5 -A20 "circuitBreaker\.execute" src/lib/neurolink.tsRepository: juspay/neurolink
Length of output: 3263
🏁 Script executed:
# Check if CircuitBreakerOpenError is imported or used anywhere in neurolink.ts
rg -n "CircuitBreakerOpenError" src/lib/neurolink.tsRepository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check mcpClientFactory.ts for circuitBreaker usage and error handling
rg -n -B5 -A20 "circuitBreaker\.execute" src/lib/mcp/mcpClientFactory.tsRepository: juspay/neurolink
Length of output: 997
🏁 Script executed:
# Check what CircuitBreakerOpenError looks like and if it's being used elsewhere
rg -n "CircuitBreakerOpenError" src/ --type=ts | head -20Repository: juspay/neurolink
Length of output: 785
🏁 Script executed:
# Find the function that contains line 7939 and its try-catch structure
rg -n -B30 "const result: T = await circuitBreaker.execute" src/lib/neurolink.ts | grep -E "(async|try|catch|function|private|public)" | tail -20Repository: juspay/neurolink
Length of output: 316
🏁 Script executed:
# Get the full try-catch block around line 7939 (need to see how errors are ultimately handled)
sed -n '7900,8100p' src/lib/neurolink.ts | head -80Repository: juspay/neurolink
Length of output: 3032
🏁 Script executed:
# Check toolDiscoveryService.ts to see the reference implementation for comparison
rg -n -B10 -A10 "instanceof CircuitBreakerOpenError" src/lib/mcp/toolDiscoveryService.tsRepository: juspay/neurolink
Length of output: 999
🏁 Script executed:
# Get the full catch block after the try at line 7925 to see how errors are handled
sed -n '7925,8150p' src/lib/neurolink.tsRepository: juspay/neurolink
Length of output: 9517
Add explicit CircuitBreakerOpenError handling at call sites.
The circuitBreaker.execute() calls in src/lib/neurolink.ts (line 7939) and src/lib/mcp/mcpClientFactory.ts (line 132) do not explicitly handle CircuitBreakerOpenError. When the breaker is open, the error is caught by the generic catch block and treated like any other error, causing the structured metadata (breakerState, retryAfterMs, failureCount) to be lost.
Compare with src/lib/mcp/toolDiscoveryService.ts (line 575), which explicitly checks if (error instanceof CircuitBreakerOpenError) and returns a structured non-retryable response. This pattern is essential to prevent AI retry storms—without it, the caller cannot distinguish a temporarily unavailable tool from a transient failure.
Add explicit instanceof checks for CircuitBreakerOpenError in the catch blocks of both call sites and return structured responses that communicate the circuit breaker state to the caller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/utils/errorHandling.ts` around lines 919 - 933, The catch blocks
around circuitBreaker.execute() calls in the neurolink and mcpClientFactory call
sites need an explicit instanceof check for CircuitBreakerOpenError: update the
catch to first detect if (error instanceof CircuitBreakerOpenError) and return a
structured non-retryable response that preserves breakerState, retryAfter (or
retryAfterMs) and failureCount (or otherwise surface those fields) so callers
can distinguish an open breaker from other failures; otherwise proceed with the
existing error handling (rethrow or handle as before). Ensure
CircuitBreakerOpenError is imported/available at those call sites and mirror the
structured response pattern used by the toolDiscoveryService example.
af88f9a to
1965269
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
1965269 to
78a145b
Compare
Review Feedback Addressed (Cycle 1)All 9 review comments (6 Copilot + 3 CodeRabbit) have been addressed. Changes MadeIssue 1 — Missing
Issue 2 — "consecutive failures" wording (Copilot #2980836325, #2980836450, CodeRabbit #2980892921)
Issue 3 — Missing tests (Copilot #2980836347)
Issue 4 — Circular dependency risk (Copilot #2980836391)
Issue 5 — Hard-coded
Issue 6 — Missing CB handling at call sites (CodeRabbit #2980892932)
Issue 7 — Stale
Files Modified
Validation
Requesting Re-review@copilot — All feedback addressed. Please re-review. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
All cycle-1 feedback is confirmed addressed. The one deferred item (missing tests) has now been implemented in commit b468e6c:
|
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
b468e6c to
6834e68
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
When a circuit breaker opens due to repeated tool failures, the error message was a raw internal string that AI models could not distinguish from a retryable error. This caused runaway retry loops — in production, one session made 395 consecutive calls against an open circuit breaker over 73 minutes, consuming 74.7M input tokens. Changes: - Add CircuitBreakerOpenError class in shared types with structured metadata (breaker name, state, retry-after timestamp/ms, failure count) - MCPCircuitBreaker throws CircuitBreakerOpenError instead of generic Error when circuit is open or half-open limit reached - toolDiscoveryService catches CircuitBreakerOpenError and returns isError tool result with AI-readable "do NOT retry" message - Add explicit CircuitBreakerOpenError handling at neurolink.ts executeTool() and mcpClientFactory.ts call sites - Fix stale retryAfter in half-open state by resetting lastFailureTime - Make CircuitBreaker name configurable (no more hard-coded identity) - Add circuit breaker blocking regression tests - Fix console.* to logger.* in fileDetector.ts - Fix CI: upgrade Node.js 18/20 → 22, pnpm 8 → 9 across all workflows (Vite 8 requires Node 20.19+, semantic-release requires Node 22.14+)
6834e68 to
5ad54a6
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🎉 This PR is included in version 9.31.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
"Circuit breaker 'X' is open. Next retry at...") that it couldn't distinguish from a retryable erroradd_commentagainst an open circuit breaker over 73 minutes, consuming 74.7M input tokensChanges
src/lib/types/circuitBreakerErrors.ts— New shared typed error classCircuitBreakerOpenErrorwith structured metadata: breaker name, state, retry-after timestamp (ISO), retry-after milliseconds, failure count. Lives intypes/to avoid circular dependencies betweenutils/andmcp/.MCPCircuitBreaker.execute()— ThrowsCircuitBreakerOpenErrorinstead of genericErrorwhen circuit is open or half-open call limit is reached. Half-open reversion now refresheslastFailureTimeto ensure accurate cooldown timestamps.toolDiscoveryService.executeTool()— CatchesCircuitBreakerOpenErrorand returns anisErrortool result with both a top-levelerrorfield and AI-readable message content:"TOOL TEMPORARILY UNAVAILABLE: tool has been disabled after N failures. Do NOT retry. Available again after Xs."Theerrorfield ensures downstream callers (e.g.ExternalServerManager) preserve the actionable message.CircuitBreakerinerrorHandling.ts— Imports from sharedtypes/module; accepts configurablenameconstructor parameter (default:"tool-execution") so thrown errors accurately identify the blocked operation.neurolink.ts— ExplicitCircuitBreakerOpenErrorhandling inexecuteTool()catch block with structuredisErrorresult,warn-level logging, and telemetry span attributes.mcpClientFactory.ts— ExplicitCircuitBreakerOpenErrorhandling increateClient()catch block with structured metadata logging.mcp/index.tsandsrc/lib/index.ts— ExportCircuitBreakerOpenErrorfor downstream consumers.test/continuous-test-suite-mcp.ts— NewtestCircuitBreakerBlocking()test function (no API calls) covering: error construction and all metadata fields, open circuit blocking with correct error metadata, recovery afterreset(), and half-open call limit enforcement.How it works
Before: Circuit breaker throws → raw error propagates → AI sees ambiguous error → retries indefinitely
After: Circuit breaker throws
CircuitBreakerOpenError→toolDiscoveryServicecatches it → returns structuredisErrorresult with actionable message → AI understands tool is temporarily unavailable → informs user instead of retryingTest plan
testCircuitBreakerBlocking()verifiesCircuitBreakerOpenErrormetadata, open circuit blocking, reset recovery, and half-open enforcement (no API calls required)CircuitBreakerOpenErrorwhen call limit is reachedSummary by CodeRabbit
New Features
Chores
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.