diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 4db91b6..eb6095a 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **Memory Leak in SimpleCache**: Fixed critical memory leak causing MCP server connection loss after several hours (fixes #118) + - Added proper timer cleanup in `SimpleCache.destroy()` method + - Updated MCP server shutdown to clean up cache timers + - Enhanced HTTP server error handling with transport error handlers + - Fixed event listener cleanup to prevent accumulation + - Added comprehensive test coverage for memory leak prevention + ## [2.10.0] - 2025-08-02 ### Added diff --git a/src/http-server-single-session.ts b/src/http-server-single-session.ts index 59e1566..6c8e771 100644 --- a/src/http-server-single-session.ts +++ b/src/http-server-single-session.ts @@ -369,7 +369,7 @@ export class SingleSessionHTTPServer { } }); - // Set up cleanup handler + // Set up cleanup handlers transport.onclose = () => { const sid = transport.sessionId; if (sid) { @@ -378,6 +378,17 @@ export class SingleSessionHTTPServer { } }; + // Handle transport errors to prevent connection drops + transport.onerror = (error: Error) => { + const sid = transport.sessionId; + logger.error('Transport error', { sessionId: sid, error: error.message }); + if (sid) { + this.removeSession(sid, 'transport_error').catch(err => { + logger.error('Error during transport error cleanup', { error: err }); + }); + } + }; + // Connect the server to the transport BEFORE handling the request logger.info('handleRequest: Connecting server to new transport'); await server.connect(transport); @@ -873,7 +884,7 @@ export class SingleSessionHTTPServer { const sessionId = req.headers['mcp-session-id'] as string | undefined; // Only add event listener if the request object supports it (not in test mocks) if (typeof req.on === 'function') { - req.on('close', () => { + const closeHandler = () => { if (!res.headersSent && sessionId) { logger.info('Connection closed before response sent', { sessionId }); // Schedule immediate cleanup if connection closes unexpectedly @@ -883,11 +894,20 @@ export class SingleSessionHTTPServer { const timeSinceAccess = Date.now() - metadata.lastAccess.getTime(); // Only remove if it's been inactive for a bit to avoid race conditions if (timeSinceAccess > 60000) { // 1 minute - this.removeSession(sessionId, 'connection_closed'); + this.removeSession(sessionId, 'connection_closed').catch(err => { + logger.error('Error during connection close cleanup', { error: err }); + }); } } }); } + }; + + req.on('close', closeHandler); + + // Clean up event listener when response ends to prevent memory leaks + res.on('finish', () => { + req.removeListener('close', closeHandler); }); } diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 472c954..b081592 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -2538,6 +2538,16 @@ Full documentation is being prepared. For now, use get_node_essentials for confi async shutdown(): Promise { logger.info('Shutting down MCP server...'); + // Clean up cache timers to prevent memory leaks + if (this.cache) { + try { + this.cache.destroy(); + logger.info('Cache timers cleaned up'); + } catch (error) { + logger.error('Error cleaning up cache:', error); + } + } + // Close database connection if it exists if (this.db) { try { diff --git a/src/utils/simple-cache.ts b/src/utils/simple-cache.ts index c2d8088..cbbad82 100644 --- a/src/utils/simple-cache.ts +++ b/src/utils/simple-cache.ts @@ -4,10 +4,11 @@ */ export class SimpleCache { private cache = new Map(); + private cleanupTimer: NodeJS.Timeout | null = null; constructor() { // Clean up expired entries every minute - setInterval(() => { + this.cleanupTimer = setInterval(() => { const now = Date.now(); for (const [key, item] of this.cache.entries()) { if (item.expires < now) this.cache.delete(key); @@ -34,4 +35,16 @@ export class SimpleCache { clear(): void { this.cache.clear(); } + + /** + * Clean up the cache and stop the cleanup timer + * Essential for preventing memory leaks in long-running servers + */ + destroy(): void { + if (this.cleanupTimer) { + clearInterval(this.cleanupTimer); + this.cleanupTimer = null; + } + this.cache.clear(); + } } \ No newline at end of file diff --git a/tests/unit/utils/simple-cache-memory-leak-fix.test.ts b/tests/unit/utils/simple-cache-memory-leak-fix.test.ts new file mode 100644 index 0000000..eea96cf --- /dev/null +++ b/tests/unit/utils/simple-cache-memory-leak-fix.test.ts @@ -0,0 +1,123 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { SimpleCache } from '../../../src/utils/simple-cache'; + +describe('SimpleCache Memory Leak Fix', () => { + let cache: SimpleCache; + + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + if (cache && typeof cache.destroy === 'function') { + cache.destroy(); + } + vi.restoreAllMocks(); + }); + + it('should track cleanup timer', () => { + cache = new SimpleCache(); + // Access private property for testing + expect((cache as any).cleanupTimer).toBeDefined(); + expect((cache as any).cleanupTimer).not.toBeNull(); + }); + + it('should clear timer on destroy', () => { + cache = new SimpleCache(); + const timer = (cache as any).cleanupTimer; + + cache.destroy(); + + expect((cache as any).cleanupTimer).toBeNull(); + // Verify timer was cleared + expect(() => clearInterval(timer)).not.toThrow(); + }); + + it('should clear cache on destroy', () => { + cache = new SimpleCache(); + cache.set('test-key', 'test-value', 300); + + expect(cache.get('test-key')).toBe('test-value'); + + cache.destroy(); + + expect(cache.get('test-key')).toBeNull(); + }); + + it('should handle multiple destroy calls safely', () => { + cache = new SimpleCache(); + + expect(() => { + cache.destroy(); + cache.destroy(); + cache.destroy(); + }).not.toThrow(); + + expect((cache as any).cleanupTimer).toBeNull(); + }); + + it('should not create new timers after destroy', () => { + cache = new SimpleCache(); + const originalTimer = (cache as any).cleanupTimer; + + cache.destroy(); + + // Try to use the cache after destroy + cache.set('key', 'value'); + cache.get('key'); + cache.clear(); + + // Timer should still be null + expect((cache as any).cleanupTimer).toBeNull(); + expect((cache as any).cleanupTimer).not.toBe(originalTimer); + }); + + it('should clean up expired entries periodically', () => { + cache = new SimpleCache(); + + // Set items with different TTLs + cache.set('short', 'value1', 1); // 1 second + cache.set('long', 'value2', 300); // 300 seconds + + // Advance time by 2 seconds + vi.advanceTimersByTime(2000); + + // Advance time to trigger cleanup (60 seconds) + vi.advanceTimersByTime(58000); + + // Short-lived item should be gone + expect(cache.get('short')).toBeNull(); + // Long-lived item should still exist + expect(cache.get('long')).toBe('value2'); + }); + + it('should prevent memory leak by clearing timer', () => { + const timers: NodeJS.Timeout[] = []; + const originalSetInterval = global.setInterval; + + // Mock setInterval to track created timers + global.setInterval = vi.fn((callback, delay) => { + const timer = originalSetInterval(callback, delay); + timers.push(timer); + return timer; + }); + + // Create and destroy multiple caches + for (let i = 0; i < 5; i++) { + const tempCache = new SimpleCache(); + tempCache.set(`key${i}`, `value${i}`); + tempCache.destroy(); + } + + // All timers should have been cleared + expect(timers.length).toBe(5); + + // Restore original setInterval + global.setInterval = originalSetInterval; + }); + + it('should have destroy method defined', () => { + cache = new SimpleCache(); + expect(typeof cache.destroy).toBe('function'); + }); +}); \ No newline at end of file