fix: Address code review findings for memory leak fix (#330)

## Code Review Fixes

1. **Test Assertion Error (line 292)** - CRITICAL
   - Fixed incorrect assertion in sqljs-memory-leak test
   - Changed from `expect(saveCallback).toBeLessThan(10)`
   - To: `expect(saveCallback.mock.calls.length).toBeLessThan(10)`
   - ✅ Test now passes (12/12 tests passing)

2. **Upper Bound Validation**
   - Added maximum value validation for SQLJS_SAVE_INTERVAL_MS
   - Valid range: 100ms - 60000ms (1 minute)
   - Falls back to default 5000ms if out of range
   - Location: database-adapter.ts:255

3. **Railway Dockerfile Optimization**
   - Removed build tools after installing dependencies
   - Reduces image size by ~50-100MB
   - Pattern: install → build native modules → remove tools
   - Location: Dockerfile.railway:38-41

4. **Defensive Programming**
   - Added `closed` flag to prevent double-close issues
   - Early return if already closed
   - Location: database-adapter.ts:236, 283-286

5. **Documentation Improvements**
   - Added comprehensive comments for DEFAULT_SAVE_INTERVAL_MS
   - Documented data loss window trade-off (5 seconds)
   - Explained constructor optimization (no initial save)
   - Clarified scheduleSave() debouncing under load

6. **CHANGELOG Accuracy**
   - Fixed discrepancy about explicit cleanup
   - Updated to reflect automatic cleanup via function scope
   - Removed misleading `data = null` reference

## Verification

- ✅ Build: Success
- ✅ Lint: No errors
- ✅ Critical test: sqljs-memory-leak (12/12 passing)
- ✅ All code review findings addressed

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

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)

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

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);

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);