Repository navigation
fix(mcp): prevent memory leak from uncleared interval timer in MCPCir… - #77
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds explicit cleanup lifecycle to circuit breakers: stores cleanup timer, introduces MCPCircuitBreaker.destroy() for timers/listeners/history cleanup, updates CircuitBreakerManager.removeBreaker() to destroy before removal, and adds CircuitBreakerManager.destroyAll() to destroy all breakers and clear the map. Logging updated to reflect cleanup/destruction. Changes
Sequence Diagram(s)sequenceDiagram
actor App
participant Manager as CircuitBreakerManager
participant Breaker as MCPCircuitBreaker
participant Timer as Cleanup Timer
App->>Manager: removeBreaker(name)
alt breaker exists
Manager->>Breaker: destroy()
Breaker-->>Timer: clearInterval(cleanupTimer)
Breaker->>Breaker: removeAllListeners(), clearHistory()
Manager->>Manager: delete from map
Manager-->>App: true
else
Manager-->>App: false
end
sequenceDiagram
actor App
participant Manager as CircuitBreakerManager
participant Breaker* as All MCPCircuitBreaker instances
participant Timer as Cleanup Timers
App->>Manager: destroyAll()
loop for each breaker
Manager->>Breaker*: destroy()
Breaker*-->>Timer: clearInterval()
Breaker*->>Breaker*: removeAllListeners(), clearHistory()
end
Manager->>Manager: clear map
Manager-->>App: done
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/lib/mcp/mcpCircuitBreaker.ts (5)
128-130: Use a cross-runtime-safe timer type for better compatibilityTyping Node timers as NodeJS.Timeout ties you to Node’s ambient types. Prefer ReturnType to work across Node/browser TS configs and avoid type friction.
Apply this diff:
- // Store the cleanup timer reference for proper cleanup - private cleanupTimer?: NodeJS.Timeout; + // Store the cleanup timer reference for proper cleanup + private cleanupTimer?: ReturnType<typeof setInterval>; + + // Optional: mark lifecycle state to prevent use-after-destroy + private destroyed = false;
147-149: Unref the maintenance interval so it doesn’t keep the process aliveStoring the reference is great. In Node, calling unref() prevents this interval from keeping the event loop open if the rest of the app is done. This complements destroy() and further reduces shutdown friction.
Apply this diff:
- // Clean up old call records periodically - now storing the timer reference - this.cleanupTimer = setInterval(() => this.cleanupCallHistory(), 60000); + // Clean up old call records periodically - now storing the timer reference + this.cleanupTimer = setInterval(() => this.cleanupCallHistory(), 60000); + // Do not keep the event loop alive only because of this maintenance timer (Node.js) + if (typeof (this.cleanupTimer as any).unref === "function") { + (this.cleanupTimer as any).unref(); + }Separately: your timeoutPromise used in Promise.race cannot be canceled and may still reject after the race settles, risking unhandled rejection warnings. Consider a cancelable timeout or AbortController. I can provide a small helper if desired.
434-449: Solid destroy() implementation; make it idempotent and guard subsequent usageThis is the right fix for the leak. Two small hardening tweaks:
- Mark the breaker as destroyed to avoid use-after-destroy calls.
- Optionally, throw if execute() is called after destroy().
Apply this diff to mark destruction:
destroy(): void { // Clear the interval timer to prevent memory leaks if (this.cleanupTimer) { clearInterval(this.cleanupTimer); this.cleanupTimer = undefined; mcpLogger.debug(`[CircuitBreaker:${this.name}] Cleanup timer cleared`); } // Clear any remaining event listeners this.removeAllListeners(); // Clear call history to free memory this.callHistory = []; + // Mark as destroyed to prevent use-after-destroy + this.destroyed = true; + mcpLogger.debug(`[CircuitBreaker:${this.name}] Destroyed and cleaned up`); }If you want to prevent calls post-destroy, add a guard in execute (outside changed lines):
if (this.destroyed) { throw new Error(`Circuit breaker '${this.name}' has been destroyed`); }Optional test idea (Jest/Vi with fake timers): assert that destroy() clears the interval and no further cleanup ticks occur.
571-578: destroyAll: continue on errors to ensure full shutdownIf a single breaker’s destroy throws, the loop aborts and the map may not be cleared. Catch per-breaker, continue, then clear the map.
Apply this diff:
destroyAll(): void { - for (const breaker of this.breakers.values()) { - breaker.destroy(); - } - this.breakers.clear(); + for (const [name, breaker] of this.breakers) { + try { + breaker.destroy(); + } catch (err) { + mcpLogger.error( + `[CircuitBreakerManager] Failed to destroy circuit breaker ${name}: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } + } + this.breakers.clear(); mcpLogger.info("[CircuitBreakerManager] Destroyed all circuit breakers"); }
478-493: Harden removeBreaker: isolate destroy errors and confirm no external callersWrap breaker.destroy() in a try/catch so that a failing destroy can’t block removal, and always delete the entry in a finally block. We searched the repo for removeBreaker(…) and found no call sites outside of mcpCircuitBreaker.ts—please verify there are no downstream integrations that depend on a thrown error or a void return.
Proposed diff in src/lib/mcp/mcpCircuitBreaker.ts (lines 478–493):
removeBreaker(name: string): boolean { - const breaker = this.breakers.get(name); - if (breaker) { - // Destroy the breaker to clean up its timer and resources - breaker.destroy(); - this.breakers.delete(name); + const breaker = this.breakers.get(name); + if (breaker) { + // Destroy the breaker to clean up its timer and resources + try { + breaker.destroy(); + } catch (err) { + mcpLogger.error( + `[CircuitBreakerManager] Error destroying circuit breaker ${name}: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } finally { + this.breakers.delete(name); + } mcpLogger.debug( `[CircuitBreakerManager] Removed and cleaned up circuit breaker: ${name}`, ); return true; } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib/mcp/mcpCircuitBreaker.ts(5 hunks)
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a memory leak in the MCPCircuitBreaker class by implementing proper cleanup of interval timers. The interval timer created in the constructor was never cleared, preventing Node.js processes from exiting cleanly and causing memory leaks.
- Added timer reference storage and cleanup mechanisms to MCPCircuitBreaker
- Implemented proper resource destruction methods for individual and bulk cleanup
- Updated the CircuitBreakerManager to call cleanup methods when removing breakers
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| private lastFailureTime = 0; | ||
| private halfOpenCalls = 0; | ||
| private lastStateChange = new Date(); | ||
| // Store the cleanup timer reference for proper cleanup |
There was a problem hiding this comment.
Consider adding a JSDoc comment to document the purpose of the cleanupTimer property, explaining that it holds the reference to the interval timer for proper cleanup.
| // Store the cleanup timer reference for proper cleanup | |
| /** | |
| * Holds the reference to the interval timer used for periodic cleanup of old call records. | |
| * This allows proper cleanup of the timer when the circuit breaker is disposed. | |
| */ |
| /** | ||
| * Destroy the circuit breaker and clean up resources | ||
| * This method should be called when the circuit breaker is no longer needed | ||
| * to prevent memory leaks from the cleanup timer |
There was a problem hiding this comment.
The destroy() method should have a JSDoc comment explaining when and why it should be called, its side effects (clearing timers, listeners, and memory), and that the circuit breaker becomes unusable after calling this method.
| * to prevent memory leaks from the cleanup timer | |
| * Destroys the circuit breaker and cleans up all associated resources. | |
| * | |
| * When and why to call: | |
| * - Call this method when the circuit breaker is no longer needed, such as during application shutdown or when the breaker will not be used again. | |
| * | |
| * Side effects: | |
| * - Clears the internal cleanup timer to prevent memory leaks. | |
| * - Removes all event listeners attached to this circuit breaker instance. | |
| * - Clears the call history to free up memory. | |
| * | |
| * After calling this method: | |
| * - The circuit breaker instance becomes unusable and should not be used for further operations. |
|
|
||
| /** | ||
| * Destroy all circuit breakers and clean up their resources | ||
| * This should be called during application shutdown to prevent memory leaks |
There was a problem hiding this comment.
The destroyAll() method should have a JSDoc comment documenting its intended use during application shutdown and that it renders all managed circuit breakers unusable.
| * This should be called during application shutdown to prevent memory leaks | |
| * Destroys all managed circuit breakers and cleans up their resources. | |
| * Intended to be called during application shutdown to prevent memory leaks. | |
| * After calling this method, all managed circuit breakers become unusable. |
…cuitBreaker Added timer cleanup to MCPCircuitBreaker to prevent memory leaks: - Store interval timer reference in cleanupTimer property - Implement destroy() method to clear timer and resources - Update removeBreaker() to call destroy() on removal - Add destroyAll() method for bulk cleanup Issue: The interval timer created in constructor was never cleared, causing memory leaks and preventing Node.js processes from exiting cleanly.
dfd6a0a to
ff6bbf4
Compare
Added timer cleanup to MCPCircuitBreaker to prevent memory leaks:
Issue:
The interval timer created in constructor was never cleared, causing memory leaks and preventing Node.js processes from exiting cleanly.
Pull Request
Description
Type of Change
Related Issues
Changes Made
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
Screenshots/Demo
Checklist
Additional Notes
Summary by CodeRabbit