fix: resolve memory leaks, error handling, and edge cases #224
Conversation
- Add destroy() method with process exit handler to CacheManager - Fix TemplateServerManager.cleanup() to clear session maps - Add process signal handlers to FileStorageService - Prevent multiple process handlers in pidFileManager with guard flags - Wrap background retry in try/catch in mcpLoadingManager - Add debug logging and transport cleanup to sseRoutes - Remove unsafe non-null assertions in clientInstancePool - Add minimum length validation for preset names - Create centralized timeout constants file - Create Shutdownable interface for graceful shutdown Co-Authored-By: Claude <[email protected]>
- CacheManager: Add guard flag to prevent duplicate exit handler registration - CacheManager: Fix evictOldest() edge case with empty cache - TemplateServerManager: Add guard flag to prevent duplicate exit handler - TemplateServerManager: Replace unsafe non-null assertion with proper null check - LoadingStateTracker: Fix potential division by zero in averageLoadTime calculation Co-Authored-By: Claude <[email protected]>
- Extract duplicate timeout pattern into reusable createTimeout() function - Replace 4 inline timeout patterns with the helper function - Add tests for the createTimeout helper Co-Authored-By: Claude <[email protected]>
- Extract TEMPLATE_CLEANUP_INTERVAL and TEMPLATE_IDLE_TIMEOUT constants - Replace inline magic numbers in templateServerManager with TIMEOUTS constants - Unify duplicate sendXxxListChangedNotification methods with single sendListChangedNotification Co-Authored-By: Claude <[email protected]>
- filterCache.ts: Use TIMEOUTS.CLEANUP_INTERVAL instead of inline magic number - pidFileManager.ts: Refactor duplicate signal handler registrations into a loop Co-Authored-By: Claude <[email protected]>
|
Thanks again for your contribution. This PR looks solid and addresses several issues. However, please allow me some time to review it thoroughly. |
|
Sure! Highly recommend to enable coderabbit, it's free for opensource projects and provides solid feedback for PRs, useful for contributers :) |
|
Good idea, setting up right now, and could you just take a look about the failing CI test ? |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request introduces centralized timeout configuration, adds graceful shutdown mechanisms with process exit handlers and handler registration guards across multiple services, and improves error handling and defensive programming throughout the application. A new Shutdownable interface is introduced to standardize shutdown implementations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![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 (3)
src/domains/registry/cacheManager.ts (1)
168-179: Exit handler registration logic is misplaced.Registering the process exit handler inside
destroy()creates confusing behavior:
- On first
destroy()call, the timer is cleared, then the exit handler is registered- On process exit,
destroy()is called again (though harmless due to the guard)The exit handler should be registered in the constructor to ensure cleanup happens on process exit even if
destroy()was never explicitly called.🐛 Suggested fix: move exit handler to constructor
constructor(options: Partial<CacheOptions> = {}) { this.options = { defaultTtl: options.defaultTtl || 300, maxSize: options.maxSize || 1000, cleanupInterval: options.cleanupInterval || 60000, }; this.startCleanupTimer(); + + // Register process exit handler for graceful shutdown (only once) + if (typeof process !== 'undefined' && !this.exitHandlerRegistered) { + this.exitHandlerRegistered = true; + process.on('exit', () => this.destroy()); + } } // ... destroy(): void { if (this.cleanupTimer) { clearInterval(this.cleanupTimer); this.cleanupTimer = undefined; } - - // Register process exit handler for graceful shutdown (only once) - if (typeof process !== 'undefined' && !this.exitHandlerRegistered) { - this.exitHandlerRegistered = true; - process.on('exit', () => this.destroy()); - } }src/core/server/templateServerManager.ts (1)
66-69: Fix undefined variablecleanupInterval(pipeline failure).The variable
cleanupIntervalis not defined. This was likely the old hardcoded constant that was replaced. The debug log should referenceTIMEOUTS.TEMPLATE_CLEANUP_INTERVALinstead.🐛 Proposed fix
debugIf(() => ({ message: 'TemplateServerManager cleanup timer started', - meta: { interval: cleanupInterval }, + meta: { interval: TIMEOUTS.TEMPLATE_CLEANUP_INTERVAL }, }));src/core/server/clientInstancePool.ts (1)
504-524: Pass template-level maxInstances to checkInstanceLimits() for proper enforcement.
getTemplateSettings()computestemplateConfig.template.maxInstances, butcheckInstanceLimits()only checks pool-levelthis.options.maxInstances, so template-specific limits are never enforced. The template-level setting is computed at line 134 and available, but not passed to the limit check at line 153.🛠️ Suggested wiring
- private checkInstanceLimits(templateName: string): void { - const maxInstances = this.options.maxInstances ?? DEFAULT_POOL_OPTIONS.maxInstances; + private checkInstanceLimits(templateName: string, maxInstances?: number): void { + const effectiveMaxInstances = + maxInstances ?? this.options.maxInstances ?? DEFAULT_POOL_OPTIONS.maxInstances; // Check per-template limit - if (maxInstances > 0) { + if (effectiveMaxInstances > 0) { const templateInstances = this.getTemplateInstances(templateName); const activeCount = templateInstances.filter((instance) => instance.status !== 'terminating').length; - if (activeCount >= maxInstances) { - throw new Error(`Maximum instances (${maxInstances}) reached for template '${templateName}'`); + if (activeCount >= effectiveMaxInstances) { + throw new Error( + `Maximum instances (${effectiveMaxInstances}) reached for template '${templateName}'`, + ); } }- this.checkInstanceLimits(templateName); + this.checkInstanceLimits(templateName, templateSettings.maxInstances);
🤖 Fix all issues with AI agents
In `@src/core/loading/mcpLoadingManager.ts`:
- Around line 303-308: The interval handler for backgroundRetryTimer currently
calls the async method performBackgroundRetry() without awaiting it, so
rejections escape the try/catch; change the setInterval callback to be async and
await this.performBackgroundRetry() inside the try block (i.e., make the
anonymous function async and use await) so any thrown/rejected errors are caught
by the existing catch that logs via logger.error; keep the same logger usage and
ensure the surrounding reference is to this.performBackgroundRetry().
In `@src/core/server/templateServerManager.ts`:
- Around line 443-444: The exit handler registration currently placed inside
cleanup() must be moved into the class constructor so the handler is always
registered on process start; locate references to exitHandlerRegistered and the
code that attaches process 'exit'/'SIGINT' handlers (also present around lines
464-468) and move that registration into the constructor, removing it from
cleanup(); additionally add a one-time guard inside cleanup() (e.g., a
cleanupRan boolean or reuse exitHandlerRegistered semantics to track execution)
so cleanup() becomes a no-op on subsequent calls to avoid duplicate execution
when the process exit handler invokes it.
In `@src/domains/preset/manager/presetManager.ts`:
- Around line 472-479: The empty-string branch is unreachable because the
initial check uses "!name", which also matches '' — change the presence/type
check to only reject null/undefined (e.g., use name == null || typeof name !==
'string') so the subsequent name.length < 1 branch can run and produce the
distinct message; update the validation logic around the name variable in
presetManager.ts (the block containing the checks for name, name.length, and the
/^[a-zA-Z0-9_-]+$/ test) accordingly.
🧹 Nitpick comments (5)
src/core/notifications/notificationManager.ts (1)
204-226: Consider typing themethodargument as a string literal union
This avoids accidental typos in notification method names while keeping the helper.♻️ Suggested tweak
- private sendListChangedNotification(type: 'tools' | 'resources' | 'prompts', method: string): void { + private sendListChangedNotification( + type: 'tools' | 'resources' | 'prompts', + method: + | 'notifications/tools/list_changed' + | 'notifications/resources/list_changed' + | 'notifications/prompts/list_changed', + ): void { this.sendNotification(type, { method, params: {} }); }src/core/loading/loadingStateTracker.ts (1)
247-251: LGTM! The division-by-zero fix and logic correction are sound.The change correctly guards against division by zero and also fixes a subtle bug where the average was previously computed using
completedStates.lengthas the divisor—even when some of those states lacked a definedduration, which would skew the average downward.One minor nitpick: the variable
durationsholdsServerLoadingInfo[]objects rather than actual duration numbers. Consider renaming tostatesWithDurationfor clarity, or extracting the durations directly:const durations = completedStates .map((s) => s.duration) .filter((d): d is number => d !== undefined); const averageLoadTime = durations.length > 0 ? durations.reduce((sum, d) => sum + d, 0) / durations.length : undefined;This is optional and doesn't affect correctness.
,
src/auth/storage/fileStorageService.ts (1)
344-350: Missing exit-handler guard flag, unlike other managers in this PR.Each
FileStorageServiceinstance registers a newbeforeExithandler without deduplication. If multiple instances are created (e.g., for different storage subdirectories), handlers accumulate. The PR description mentions that CacheManager and TemplateServerManager received guard flags to prevent this—this file should follow the same pattern for consistency.♻️ Suggested fix: Add a static guard flag
export class FileStorageService { private storageDir: string; private cleanupInterval: ReturnType<typeof setInterval> | null = null; + private static exitHandlerRegistered = false; + private static instances: Set<FileStorageService> = new Set(); // ... existing code ... private startPeriodicCleanup(): void { // Clean up expired data every 5 minutes this.cleanupInterval = setInterval( () => { this.cleanupExpiredData(); }, 5 * 60 * 1000, ); + FileStorageService.instances.add(this); + // Ensure cleanup on process exit - if (typeof process !== 'undefined') { + if (typeof process !== 'undefined' && !FileStorageService.exitHandlerRegistered) { + FileStorageService.exitHandlerRegistered = true; process.on('beforeExit', () => { - this.shutdown(); + for (const instance of FileStorageService.instances) { + instance.shutdown(); + } }); } } shutdown(): void { if (this.cleanupInterval) { clearInterval(this.cleanupInterval); this.cleanupInterval = null; logger.info('FileStorageService cleanup interval stopped'); } + FileStorageService.instances.delete(this); }src/commands/mcp/utils/connectionHelper.ts (1)
57-61: Consider usingcreateTimeouthere for consistency.The new
createTimeouthelper is used elsewhere in this file (lines 139, 154, 169, 201), butconnectToServersstill creates a timeout promise inline. Consider refactoring for consistency.♻️ Suggested refactor
- // Create a timeout promise - const timeoutPromise = new Promise<never>((_, reject) => { - setTimeout(() => { - reject(new Error(`Connection timeout after ${timeoutMs}ms`)); - }, timeoutMs); - }); + // Create a timeout promise + const timeoutPromise = createTimeout(timeoutMs, `Connection timeout after ${timeoutMs}ms`);src/core/server/pidFileManager.test.ts (1)
379-382: Make handler-flag resets suite-wide to avoid order dependence.The global flags persist across tests; resetting only in this test can cause false positives if another test already set the flags. Consider resetting in a shared
beforeEach/afterEachfor the describe block (or top-level) instead.🔧 Suggested change
describe('pidFileManager', () => { + beforeEach(() => { + _resetHandlerFlags(); + });
| this.backgroundRetryTimer = setInterval(() => { | ||
| this.performBackgroundRetry(); | ||
| try { | ||
| this.performBackgroundRetry(); | ||
| } catch (error) { | ||
| logger.error('Error during background retry:', error); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine the relevant lines
cat -n src/core/loading/mcpLoadingManager.ts | sed -n '295,320p'Repository: 1mcp-app/agent
Length of output: 986
🏁 Script executed:
# Also search for the performBackgroundRetry method definition
rg "performBackgroundRetry\s*\(" src/core/loading/mcpLoadingManager.ts -A 3Repository: 1mcp-app/agent
Length of output: 374
Async errors still escape the try/catch in the interval handler.
performBackgroundRetry() is declared as async and returns Promise<void> (line 317). Without await, the try/catch block cannot capture rejections from the promise, leaving unhandled promise rejections possible. Add async to the callback and await the call to properly handle errors.
✅ Suggested fix
- this.backgroundRetryTimer = setInterval(() => {
+ this.backgroundRetryTimer = setInterval(async () => {
try {
- this.performBackgroundRetry();
+ await this.performBackgroundRetry();
} catch (error) {
logger.error('Error during background retry:', error);
}
}, this.config.backgroundRetryIntervalMs);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.backgroundRetryTimer = setInterval(() => { | |
| this.performBackgroundRetry(); | |
| try { | |
| this.performBackgroundRetry(); | |
| } catch (error) { | |
| logger.error('Error during background retry:', error); | |
| } | |
| this.backgroundRetryTimer = setInterval(async () => { | |
| try { | |
| await this.performBackgroundRetry(); | |
| } catch (error) { | |
| logger.error('Error during background retry:', error); | |
| } | |
| }, this.config.backgroundRetryIntervalMs); |
🤖 Prompt for AI Agents
In `@src/core/loading/mcpLoadingManager.ts` around lines 303 - 308, The interval
handler for backgroundRetryTimer currently calls the async method
performBackgroundRetry() without awaiting it, so rejections escape the
try/catch; change the setInterval callback to be async and await
this.performBackgroundRetry() inside the try block (i.e., make the anonymous
function async and use await) so any thrown/rejected errors are caught by the
existing catch that logs via logger.error; keep the same logger usage and ensure
the surrounding reference is to this.performBackgroundRetry().
| private exitHandlerRegistered = false; | ||
|
|
There was a problem hiding this comment.
Exit handler registration is misplaced inside cleanup().
The exit handler is registered inside cleanup(), which has two problems:
- Handler may never be registered: If
cleanup()isn't called before process exit, the handler won't exist. - Duplicate cleanup execution: If
cleanup()is called manually, then on process exit it runs again (the guard only prevents re-registration, not re-execution).
The exit handler should be registered in the constructor to ensure it's always active.
♻️ Proposed fix - move registration to constructor
constructor() {
// Initialize the client instance pool
this.clientInstancePool = new ClientInstancePool({
maxInstances: 50, // Configurable limit
idleTimeout: TIMEOUTS.IDLE_TIMEOUT,
cleanupInterval: TIMEOUTS.TEMPLATE_CLEANUP_INTERVAL,
});
// Start cleanup timer for idle template instances
this.startCleanupTimer();
+
+ // Register process exit handler for graceful shutdown (only once)
+ if (typeof process !== 'undefined' && !this.exitHandlerRegistered) {
+ this.exitHandlerRegistered = true;
+ process.on('exit', () => this.cleanup());
+ }
}And remove from cleanup():
public cleanup(): void {
// Clean up cleanup timer
if (this.cleanupTimer) {
clearInterval(this.cleanupTimer);
this.cleanupTimer = undefined;
}
// Clear session-to-rendered-hash mapping
this.sessionToRenderedHash.clear();
// Clear template session map
this.templateSessionMap?.clear();
// Clean up the client instance pool
this.clientInstancePool?.cleanupIdleInstances();
-
- // Register process exit handler for graceful shutdown (only once)
- if (typeof process !== 'undefined' && !this.exitHandlerRegistered) {
- this.exitHandlerRegistered = true;
- process.on('exit', () => this.cleanup());
- }
}Also applies to: 464-468
🤖 Prompt for AI Agents
In `@src/core/server/templateServerManager.ts` around lines 443 - 444, The exit
handler registration currently placed inside cleanup() must be moved into the
class constructor so the handler is always registered on process start; locate
references to exitHandlerRegistered and the code that attaches process
'exit'/'SIGINT' handlers (also present around lines 464-468) and move that
registration into the constructor, removing it from cleanup(); additionally add
a one-time guard inside cleanup() (e.g., a cleanupRan boolean or reuse
exitHandlerRegistered semantics to track execution) so cleanup() becomes a no-op
on subsequent calls to avoid duplicate execution when the process exit handler
invokes it.
| // Validate name | ||
| if (!name || typeof name !== 'string') { | ||
| errors.push('Preset name is required and must be a string'); | ||
| } else if (name.length < 1) { | ||
| errors.push('Preset name must be at least 1 character'); | ||
| } else if (name.length > 50) { | ||
| errors.push('Preset name must be 50 characters or less'); | ||
| } else if (!/^[a-zA-Z0-9_-]+$/.test(name)) { |
There was a problem hiding this comment.
Empty-string check is unreachable
!name already catches '', so the new name.length < 1 branch never runs. If you want a distinct message for empty strings, split the checks.
Suggested fix
- if (!name || typeof name !== 'string') {
+ if (typeof name !== 'string') {
errors.push('Preset name is required and must be a string');
- } else if (name.length < 1) {
+ } else if (name.length === 0) {
errors.push('Preset name must be at least 1 character');
} else if (name.length > 50) {
errors.push('Preset name must be 50 characters or less');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Validate name | |
| if (!name || typeof name !== 'string') { | |
| errors.push('Preset name is required and must be a string'); | |
| } else if (name.length < 1) { | |
| errors.push('Preset name must be at least 1 character'); | |
| } else if (name.length > 50) { | |
| errors.push('Preset name must be 50 characters or less'); | |
| } else if (!/^[a-zA-Z0-9_-]+$/.test(name)) { | |
| // Validate name | |
| if (typeof name !== 'string') { | |
| errors.push('Preset name is required and must be a string'); | |
| } else if (name.length === 0) { | |
| errors.push('Preset name must be at least 1 character'); | |
| } else if (name.length > 50) { | |
| errors.push('Preset name must be 50 characters or less'); | |
| } else if (!/^[a-zA-Z0-9_-]+$/.test(name)) { |
🤖 Prompt for AI Agents
In `@src/domains/preset/manager/presetManager.ts` around lines 472 - 479, The
empty-string branch is unreachable because the initial check uses "!name", which
also matches '' — change the presence/type check to only reject null/undefined
(e.g., use name == null || typeof name !== 'string') so the subsequent
name.length < 1 branch can run and produce the distinct message; update the
validation logic around the name variable in presetManager.ts (the block
containing the checks for name, name.length, and the /^[a-zA-Z0-9_-]+$/ test)
accordingly.
2 participants
Summary
Fixed issues identified in the comprehensive code review report across error handling, memory leaks, type safety, edge cases, security, and code quality.
Memory Leaks & Resource Cleanup
exitHandlerRegisteredguard flag to prevent duplicate exit handlersevictOldest()edge case with empty cacheexitHandlerRegisteredguard flag to prevent duplicate exit handlers!non-null assertion with proper null checkshutdown()method (no change needed)stopPeriodicFlush()method (no change needed)Error Handling
Type Safety
!non-null assertions, using nullish coalescing (??) insteadEdge Cases
averageLoadTimecalculationCode Quality - Refactoring
createTimeout()helper functionsendXxxListChangedNotificationmethods with singlesendListChangedNotificationTEMPLATE_CLEANUP_INTERVAL(30s) andTEMPLATE_IDLE_TIMEOUT(10min) constantsTIMEOUTSconstantsTIMEOUTS.CLEANUP_INTERVALinstead of inline magic numberTests Added
src/commands/mcp/utils/connectionHelper.test.ts- Tests for createTimeout helperFiles Changed
src/domains/registry/cacheManager.tssrc/core/server/templateServerManager.tssrc/core/loading/loadingStateTracker.tssrc/commands/mcp/utils/connectionHelper.tssrc/commands/mcp/utils/connectionHelper.test.tssrc/constants/timeouts.tssrc/core/notifications/notificationManager.tssrc/core/server/clientInstancePool.tssrc/core/filtering/filterCache.tssrc/core/server/pidFileManager.tsTest Results
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.