diff --git a/CHANGELOG.md b/CHANGELOG.md index bae88d3..fe2cd3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,10 +57,11 @@ Day 3: 2250 MB (extrapolated - OOM kill) - After: `fsSync.writeFileSync(path, data);` (direct Uint8Array write) - **Impact:** 50% reduction in temporary allocations per save -✅ **Added explicit cleanup** -- Wrapped saveToFile() in try-finally with `data = null` -- Helps garbage collector reclaim memory faster -- **Impact:** Prevents reference retention +✅ **Optimized memory allocation** +- Removed Buffer.from() copy, write Uint8Array directly to disk +- Local variable automatically cleared when function exits +- V8 garbage collector can reclaim memory immediately after save +- **Impact:** 50% reduction in temporary allocations per save ✅ **Made save interval configurable** - New env var: `SQLJS_SAVE_INTERVAL_MS` (default: 5000) diff --git a/Dockerfile.railway b/Dockerfile.railway index 20096e5..5f51442 100644 --- a/Dockerfile.railway +++ b/Dockerfile.railway @@ -25,17 +25,20 @@ RUN npm run build FROM node:22-alpine AS runtime WORKDIR /app -# Install system dependencies including build tools for better-sqlite3 compilation -# Build tools (python3, make, g++) enable native SQLite instead of sql.js, preventing memory leaks -RUN apk add --no-cache curl python3 make g++ && \ +# Install runtime dependencies +RUN apk add --no-cache curl && \ rm -rf /var/cache/apk/* # Copy runtime-only package.json COPY package.runtime.json package.json -# Install only production dependencies -RUN npm install --production --no-audit --no-fund && \ - npm cache clean --force +# Install production dependencies with temporary build tools +# Build tools (python3, make, g++) enable better-sqlite3 compilation (native SQLite) +# They are removed after installation to reduce image size and attack surface +RUN apk add --no-cache python3 make g++ && \ + npm install --production --no-audit --no-fund && \ + npm cache clean --force && \ + apk del python3 make g++ # Copy built application from builder stage COPY --from=builder /app/dist ./dist diff --git a/src/database/database-adapter.ts b/src/database/database-adapter.ts index 544d006..f952d0e 100644 --- a/src/database/database-adapter.ts +++ b/src/database/database-adapter.ts @@ -233,9 +233,17 @@ class BetterSQLiteAdapter implements DatabaseAdapter { class SQLJSAdapter implements DatabaseAdapter { private saveTimer: NodeJS.Timeout | null = null; private saveIntervalMs: number; + private closed = false; // Prevent multiple close() calls // Default save interval: 5 seconds (balance between data safety and performance) // Configurable via SQLJS_SAVE_INTERVAL_MS environment variable + // + // DATA LOSS WINDOW: Up to 5 seconds of database changes may be lost if process + // crashes before scheduleSave() timer fires. This is acceptable because: + // 1. close() calls saveToFile() immediately on graceful shutdown + // 2. Docker/Kubernetes SIGTERM provides 30s for cleanup (more than enough) + // 3. The alternative (100ms interval) caused 2.2GB memory leaks in production + // 4. MCP server is primarily read-heavy (writes are rare) private static readonly DEFAULT_SAVE_INTERVAL_MS = 5000; constructor(private db: any, private dbPath: string) { @@ -243,13 +251,21 @@ class SQLJSAdapter implements DatabaseAdapter { const envInterval = process.env.SQLJS_SAVE_INTERVAL_MS; this.saveIntervalMs = envInterval ? parseInt(envInterval, 10) : SQLJSAdapter.DEFAULT_SAVE_INTERVAL_MS; - // Validate interval - if (isNaN(this.saveIntervalMs) || this.saveIntervalMs < 100) { - logger.warn(`Invalid SQLJS_SAVE_INTERVAL_MS value: ${envInterval}, using default ${SQLJSAdapter.DEFAULT_SAVE_INTERVAL_MS}ms`); + // Validate interval (minimum 100ms, maximum 60000ms = 1 minute) + if (isNaN(this.saveIntervalMs) || this.saveIntervalMs < 100 || this.saveIntervalMs > 60000) { + logger.warn( + `Invalid SQLJS_SAVE_INTERVAL_MS value: ${envInterval} (must be 100-60000ms), ` + + `using default ${SQLJSAdapter.DEFAULT_SAVE_INTERVAL_MS}ms` + ); this.saveIntervalMs = SQLJSAdapter.DEFAULT_SAVE_INTERVAL_MS; } logger.debug(`SQLJSAdapter initialized with save interval: ${this.saveIntervalMs}ms`); + + // NOTE: No initial save scheduled here (optimization) + // Database is either: + // 1. Loaded from existing file (already persisted), or + // 2. New database (will be saved on first write operation) } prepare(sql: string): PreparedStatement { @@ -264,11 +280,18 @@ class SQLJSAdapter implements DatabaseAdapter { } close(): void { + if (this.closed) { + logger.debug('SQLJSAdapter already closed, skipping'); + return; + } + this.saveToFile(); if (this.saveTimer) { clearTimeout(this.saveTimer); + this.saveTimer = null; } this.db.close(); + this.closed = true; } pragma(key: string, value?: any): any { @@ -318,6 +341,12 @@ class SQLJSAdapter implements DatabaseAdapter { // Save after configured interval of inactivity (default: 5000ms) // This debouncing reduces memory churn from frequent buffer allocations + // + // NOTE: Under constant write load, saves may be delayed until writes stop. + // This is acceptable because: + // 1. MCP server is primarily read-heavy (node lookups, searches) + // 2. Writes are rare (only during database rebuilds) + // 3. close() saves immediately on shutdown, flushing any pending changes this.saveTimer = setTimeout(() => { this.saveToFile(); }, this.saveIntervalMs); diff --git a/tests/integration/database/sqljs-memory-leak.test.ts b/tests/integration/database/sqljs-memory-leak.test.ts index 2d52fb7..5beee16 100644 --- a/tests/integration/database/sqljs-memory-leak.test.ts +++ b/tests/integration/database/sqljs-memory-leak.test.ts @@ -289,7 +289,7 @@ describe('SQLJSAdapter Memory Leak Prevention (Issue #330)', () => { // - Total temporary memory: ~20-50MB (90-95% reduction) // Should have saved much fewer times than operations (batching works) - expect(saveCallback).toBeLessThan(10); + expect(saveCallback.mock.calls.length).toBeLessThan(10); // Cleanup if (timer) clearTimeout(timer);