mirror of
https://github.com/czlonkowski/n8n-mcp.git
synced 2026-01-30 06:22:04 +00:00
fix: critical memory leak from per-session database connections (#554)
* fix: critical memory leak from per-session database connections (#542) Each MCP session was creating its own database connection (~900MB), causing OOM kills every ~20 minutes with 3-4 concurrent sessions. Changes: - Add SharedDatabase singleton pattern - all sessions share ONE connection - Reduce session timeout from 30 min to 5 min (configurable) - Add eager cleanup for reconnecting instances - Fix telemetry event listener leak Memory impact: ~900MB/session → ~68MB shared + ~5MB/session overhead Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Conceived by Romuald Czlonkowski - https://www.aiadvisors.pl/en * fix: resolve test failures from shared database race conditions - Fix `shutdown()` to respect shared database pattern (was directly closing) - Add `await this.initialized` in both `close()` and `shutdown()` to prevent race condition where cleanup runs while initialization is in progress - Add double-shutdown protection with `isShutdown` flag - Export `SharedDatabaseState` type for proper typing - Include error details in debug logs - Add MCP server close to `shutdown()` for consistency with `close()` - Null out `earlyLogger` in `shutdown()` for consistency The CI test failure "The database connection is not open" was caused by: 1. `shutdown()` directly calling `this.db.close()` which closed the SHARED database connection, breaking subsequent tests 2. Race condition where `shutdown()` ran before initialization completed Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * test: add unit tests for shared-database module Add comprehensive unit tests covering: - getSharedDatabase: initialization, reuse, different path error, concurrent requests - releaseSharedDatabase: refCount decrement, double-release guard - closeSharedDatabase: state clearing, error handling, re-initialization - Helper functions: isSharedDatabaseInitialized, getSharedDatabaseRefCount 21 tests covering the singleton database connection pattern used to prevent ~900MB memory leaks per session. Conceived by Romuald Członkowski - www.aiadvisors.pl/en Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
committed by
GitHub
parent
fad3437977
commit
c8c76e435d
302
tests/unit/database/shared-database.test.ts
Normal file
302
tests/unit/database/shared-database.test.ts
Normal file
@@ -0,0 +1,302 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
|
||||
|
||||
// Mock dependencies at module level
|
||||
const mockDb = {
|
||||
prepare: vi.fn().mockReturnValue({
|
||||
get: vi.fn(),
|
||||
all: vi.fn(),
|
||||
run: vi.fn()
|
||||
}),
|
||||
exec: vi.fn(),
|
||||
close: vi.fn(),
|
||||
pragma: vi.fn(),
|
||||
inTransaction: false,
|
||||
transaction: vi.fn(),
|
||||
checkFTS5Support: vi.fn()
|
||||
};
|
||||
|
||||
vi.mock('../../../src/database/database-adapter', () => ({
|
||||
createDatabaseAdapter: vi.fn().mockResolvedValue(mockDb)
|
||||
}));
|
||||
|
||||
vi.mock('../../../src/database/node-repository', () => ({
|
||||
NodeRepository: vi.fn().mockImplementation(() => ({
|
||||
getNodeTypes: vi.fn().mockReturnValue([])
|
||||
}))
|
||||
}));
|
||||
|
||||
vi.mock('../../../src/templates/template-service', () => ({
|
||||
TemplateService: vi.fn().mockImplementation(() => ({}))
|
||||
}));
|
||||
|
||||
vi.mock('../../../src/services/enhanced-config-validator', () => ({
|
||||
EnhancedConfigValidator: {
|
||||
initializeSimilarityServices: vi.fn()
|
||||
}
|
||||
}));
|
||||
|
||||
vi.mock('../../../src/utils/logger', () => ({
|
||||
logger: {
|
||||
debug: vi.fn(),
|
||||
info: vi.fn(),
|
||||
warn: vi.fn(),
|
||||
error: vi.fn()
|
||||
}
|
||||
}));
|
||||
|
||||
describe('Shared Database Module', () => {
|
||||
let sharedDbModule: typeof import('../../../src/database/shared-database');
|
||||
let createDatabaseAdapter: ReturnType<typeof vi.fn>;
|
||||
|
||||
beforeEach(async () => {
|
||||
// Reset all mocks
|
||||
vi.clearAllMocks();
|
||||
mockDb.close.mockReset();
|
||||
|
||||
// Reset modules to get fresh state
|
||||
vi.resetModules();
|
||||
|
||||
// Import fresh module
|
||||
sharedDbModule = await import('../../../src/database/shared-database');
|
||||
|
||||
// Get the mocked function
|
||||
const adapterModule = await import('../../../src/database/database-adapter');
|
||||
createDatabaseAdapter = adapterModule.createDatabaseAdapter as ReturnType<typeof vi.fn>;
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
// Clean up any shared state by closing
|
||||
try {
|
||||
await sharedDbModule.closeSharedDatabase();
|
||||
} catch {
|
||||
// Ignore errors during cleanup
|
||||
}
|
||||
});
|
||||
|
||||
describe('getSharedDatabase', () => {
|
||||
it('should initialize database on first call', async () => {
|
||||
const state = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
|
||||
expect(state).toBeDefined();
|
||||
expect(state.db).toBe(mockDb);
|
||||
expect(state.dbPath).toBe('/path/to/db');
|
||||
expect(state.refCount).toBe(1);
|
||||
expect(state.initialized).toBe(true);
|
||||
expect(createDatabaseAdapter).toHaveBeenCalledWith('/path/to/db');
|
||||
});
|
||||
|
||||
it('should reuse existing connection and increment refCount', async () => {
|
||||
// First call initializes
|
||||
const state1 = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(state1.refCount).toBe(1);
|
||||
|
||||
// Second call reuses
|
||||
const state2 = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(state2.refCount).toBe(2);
|
||||
|
||||
// Same object
|
||||
expect(state1).toBe(state2);
|
||||
|
||||
// Only initialized once
|
||||
expect(createDatabaseAdapter).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('should throw error when called with different path', async () => {
|
||||
await sharedDbModule.getSharedDatabase('/path/to/db1');
|
||||
|
||||
await expect(sharedDbModule.getSharedDatabase('/path/to/db2'))
|
||||
.rejects.toThrow('Shared database already initialized with different path');
|
||||
});
|
||||
|
||||
it('should handle concurrent initialization requests', async () => {
|
||||
// Start two requests concurrently
|
||||
const [state1, state2] = await Promise.all([
|
||||
sharedDbModule.getSharedDatabase('/path/to/db'),
|
||||
sharedDbModule.getSharedDatabase('/path/to/db')
|
||||
]);
|
||||
|
||||
// Both should get the same state
|
||||
expect(state1).toBe(state2);
|
||||
|
||||
// RefCount should be 2 (one for each call)
|
||||
expect(state1.refCount).toBe(2);
|
||||
|
||||
// Only one actual initialization
|
||||
expect(createDatabaseAdapter).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('should handle initialization failure', async () => {
|
||||
createDatabaseAdapter.mockRejectedValueOnce(new Error('DB error'));
|
||||
|
||||
await expect(sharedDbModule.getSharedDatabase('/path/to/db'))
|
||||
.rejects.toThrow('DB error');
|
||||
|
||||
// After failure, should not be initialized
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(false);
|
||||
});
|
||||
|
||||
it('should allow retry after initialization failure', async () => {
|
||||
// First call fails
|
||||
createDatabaseAdapter.mockRejectedValueOnce(new Error('DB error'));
|
||||
await expect(sharedDbModule.getSharedDatabase('/path/to/db'))
|
||||
.rejects.toThrow('DB error');
|
||||
|
||||
// Reset mock for successful call
|
||||
createDatabaseAdapter.mockResolvedValueOnce(mockDb);
|
||||
|
||||
// Second call succeeds
|
||||
const state = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
|
||||
expect(state).toBeDefined();
|
||||
expect(state.initialized).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('releaseSharedDatabase', () => {
|
||||
it('should decrement refCount', async () => {
|
||||
const state = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(state.refCount).toBe(1);
|
||||
|
||||
sharedDbModule.releaseSharedDatabase(state);
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(0);
|
||||
});
|
||||
|
||||
it('should not decrement below 0', async () => {
|
||||
const state = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
|
||||
// Release once (refCount: 1 -> 0)
|
||||
sharedDbModule.releaseSharedDatabase(state);
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(0);
|
||||
|
||||
// Release again (should stay at 0, not go negative)
|
||||
sharedDbModule.releaseSharedDatabase(state);
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(0);
|
||||
});
|
||||
|
||||
it('should handle null state gracefully', () => {
|
||||
// Should not throw
|
||||
sharedDbModule.releaseSharedDatabase(null as any);
|
||||
});
|
||||
|
||||
it('should not close database when refCount hits 0', async () => {
|
||||
const state = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
sharedDbModule.releaseSharedDatabase(state);
|
||||
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(0);
|
||||
expect(mockDb.close).not.toHaveBeenCalled();
|
||||
|
||||
// Database should still be accessible
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('closeSharedDatabase', () => {
|
||||
it('should close database and clear state', async () => {
|
||||
// Get state
|
||||
await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(true);
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(1);
|
||||
|
||||
await sharedDbModule.closeSharedDatabase();
|
||||
|
||||
// State should be cleared
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(false);
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(0);
|
||||
});
|
||||
|
||||
it('should handle close error gracefully', async () => {
|
||||
await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
mockDb.close.mockImplementationOnce(() => {
|
||||
throw new Error('Close error');
|
||||
});
|
||||
|
||||
// Should not throw
|
||||
await sharedDbModule.closeSharedDatabase();
|
||||
|
||||
// State should still be cleared
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(false);
|
||||
});
|
||||
|
||||
it('should be idempotent when already closed', async () => {
|
||||
// Close without ever initializing
|
||||
await sharedDbModule.closeSharedDatabase();
|
||||
|
||||
// Should not throw
|
||||
await sharedDbModule.closeSharedDatabase();
|
||||
});
|
||||
|
||||
it('should allow re-initialization after close', async () => {
|
||||
// Initialize
|
||||
const state1 = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(state1.refCount).toBe(1);
|
||||
|
||||
// Close
|
||||
await sharedDbModule.closeSharedDatabase();
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(false);
|
||||
|
||||
// Re-initialize
|
||||
const state2 = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(state2.refCount).toBe(1);
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(true);
|
||||
|
||||
// Should be a new state object
|
||||
expect(state1).not.toBe(state2);
|
||||
});
|
||||
});
|
||||
|
||||
describe('isSharedDatabaseInitialized', () => {
|
||||
it('should return false before initialization', () => {
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(false);
|
||||
});
|
||||
|
||||
it('should return true after initialization', async () => {
|
||||
await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(true);
|
||||
});
|
||||
|
||||
it('should return false after close', async () => {
|
||||
await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
await sharedDbModule.closeSharedDatabase();
|
||||
expect(sharedDbModule.isSharedDatabaseInitialized()).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getSharedDatabaseRefCount', () => {
|
||||
it('should return 0 before initialization', () => {
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(0);
|
||||
});
|
||||
|
||||
it('should return correct refCount after multiple operations', async () => {
|
||||
const state = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(1);
|
||||
|
||||
await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(2);
|
||||
|
||||
await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(3);
|
||||
|
||||
sharedDbModule.releaseSharedDatabase(state);
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(2);
|
||||
});
|
||||
|
||||
it('should return 0 after close', async () => {
|
||||
await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
await sharedDbModule.closeSharedDatabase();
|
||||
expect(sharedDbModule.getSharedDatabaseRefCount()).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('SharedDatabaseState interface', () => {
|
||||
it('should expose correct properties', async () => {
|
||||
const state = await sharedDbModule.getSharedDatabase('/path/to/db');
|
||||
|
||||
expect(state).toHaveProperty('db');
|
||||
expect(state).toHaveProperty('repository');
|
||||
expect(state).toHaveProperty('templateService');
|
||||
expect(state).toHaveProperty('dbPath');
|
||||
expect(state).toHaveProperty('refCount');
|
||||
expect(state).toHaveProperty('initialized');
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -333,13 +333,14 @@ describe('HTTP Server Session Management', () => {
|
||||
server = new SingleSessionHTTPServer();
|
||||
|
||||
// Mock expired sessions
|
||||
// Note: Default session timeout is 5 minutes (configurable via SESSION_TIMEOUT_MINUTES)
|
||||
const mockSessionMetadata = {
|
||||
'session-1': {
|
||||
lastAccess: new Date(Date.now() - 40 * 60 * 1000), // 40 minutes ago (expired)
|
||||
'session-1': {
|
||||
lastAccess: new Date(Date.now() - 10 * 60 * 1000), // 10 minutes ago (expired with 5 min timeout)
|
||||
createdAt: new Date(Date.now() - 60 * 60 * 1000)
|
||||
},
|
||||
'session-2': {
|
||||
lastAccess: new Date(Date.now() - 10 * 60 * 1000), // 10 minutes ago (not expired)
|
||||
'session-2': {
|
||||
lastAccess: new Date(Date.now() - 2 * 60 * 1000), // 2 minutes ago (not expired with 5 min timeout)
|
||||
createdAt: new Date(Date.now() - 20 * 60 * 1000)
|
||||
}
|
||||
};
|
||||
@@ -514,15 +515,16 @@ describe('HTTP Server Session Management', () => {
|
||||
|
||||
it('should get session metrics correctly', async () => {
|
||||
server = new SingleSessionHTTPServer();
|
||||
|
||||
|
||||
// Note: Default session timeout is 5 minutes (configurable via SESSION_TIMEOUT_MINUTES)
|
||||
const now = Date.now();
|
||||
(server as any).sessionMetadata = {
|
||||
'active-session': {
|
||||
lastAccess: new Date(now - 10 * 60 * 1000), // 10 minutes ago
|
||||
lastAccess: new Date(now - 2 * 60 * 1000), // 2 minutes ago (not expired with 5 min timeout)
|
||||
createdAt: new Date(now - 20 * 60 * 1000)
|
||||
},
|
||||
'expired-session': {
|
||||
lastAccess: new Date(now - 40 * 60 * 1000), // 40 minutes ago (expired)
|
||||
lastAccess: new Date(now - 10 * 60 * 1000), // 10 minutes ago (expired with 5 min timeout)
|
||||
createdAt: new Date(now - 60 * 60 * 1000)
|
||||
}
|
||||
};
|
||||
@@ -532,7 +534,7 @@ describe('HTTP Server Session Management', () => {
|
||||
};
|
||||
|
||||
const metrics = (server as any).getSessionMetrics();
|
||||
|
||||
|
||||
expect(metrics.totalSessions).toBe(2);
|
||||
expect(metrics.activeSessions).toBe(2);
|
||||
expect(metrics.expiredSessions).toBe(1);
|
||||
|
||||
Reference in New Issue
Block a user