refactor(lbug): extract safeClose helper to consolidate WAL flush (#1377)

eslint.config.mjs

@@ -79,6 +79,27 @@ export default [
     },
   },
 
+  // Prevent direct conn.close() / db.close() in the LadybugDB adapter (#1376).
+  // All close operations must go through safeClose() so the WAL is always
+  // flushed before the connection is released. The sole authorised call site
+  // inside safeClose itself uses an eslint-disable-next-line override.
+  {
+    files: ['gitnexus/src/core/lbug/lbug-adapter.ts'],
+    rules: {
+      'no-restricted-syntax': [
+        'error',
+        {
+          selector: "CallExpression[callee.object.name='conn'][callee.property.name='close']",
+          message: 'Use safeClose() instead of calling conn.close() directly (#1376).',
+        },
+        {
+          selector: "CallExpression[callee.object.name='db'][callee.property.name='close']",
+          message: 'Use safeClose() instead of calling db.close() directly (#1376).',
+        },
+      ],
+    },
+  },
+
   // Disable formatting rules (prettier handles those)
   prettierConfig,
 ];

gitnexus/src/core/lbug/lbug-adapter.ts

@@ -257,26 +257,7 @@ export const withLbugDb = async <T>(dbPath: string, operation: () => Promise<T>)
       // Close stale connection inside the session lock to prevent race conditions
       // with concurrent operations that might acquire the lock between cleanup steps
       await runWithSessionLock(async () => {
-        // CHECKPOINT before close to flush WAL contents (same rationale as closeLbug)
-        if (conn) {
-          try {
-            await conn.query('CHECKPOINT');
-          } catch {
-            /* best-effort */
-          }
-        }
-        try {
-          if (conn) await conn.close();
-        } catch {
-          /* best-effort */
-        }
-        try {
-          if (db) await db.close();
-        } catch {
-          /* best-effort */
-        }
-        conn = null;
-        db = null;
+        await safeClose();
         currentDbPath = null;
         ftsLoaded = false;
         vectorExtensionLoaded = false;
@@ -302,22 +283,7 @@ const ensureLbugInitialized = async (dbPath: string) => {
 const doInitLbug = async (dbPath: string) => {
   // Different database requested — close the old one first
   if (conn || db) {
-    // CHECKPOINT before close to flush WAL contents (same rationale as closeLbug)
-    if (conn) {
-      try {
-        await conn.query('CHECKPOINT');
-      } catch {
-        /* ignore — older LadybugDB or schemaless DB may not accept it */
-      }
-    }
-    try {
-      if (conn) await conn.close();
-    } catch {}
-    try {
-      if (db) await db.close();
-    } catch {}
-    conn = null;
-    db = null;
+    await safeClose();
     currentDbPath = null;
     ftsLoaded = false;
     vectorExtensionLoaded = false;
@@ -1063,34 +1029,62 @@ export const fetchExistingEmbeddingHashes = async (
   }
 };
 
-export const closeLbug = async (): Promise<void> => {
-  // CHECKPOINT before close so the WAL/.shadow contents are flushed into
-  // the main database file. Without this, LadybugDB 0.16.0's non-blocking
-  // checkpoint thread can outlive the close call and leave sidecar pages
-  // pending on disk, which makes a subsequent read-side open either race
-  // with the WAL replay or trip the database-id check on the sidecars.
-  // This is especially critical after embedding writes, which generate
-  // large amounts of WAL data. CHECKPOINT is a no-op when there's nothing
-  // pending, so it's cheap on the happy path.
-  if (conn) {
-    try {
-      await conn.query('CHECKPOINT');
-    } catch {
-      /* ignore — older LadybugDB or schemaless DB may not accept it */
-    }
+/**
+ * Flush the WAL so all pending writes are visible to subsequent readers.
+ *
+ * Best-effort: swallows errors from older LadybugDB versions or schemaless
+ * databases that do not support the CHECKPOINT command.  A no-op when there
+ * is nothing pending, so safe (and cheap) to call unconditionally after any
+ * write path.
+ *
+ * Use this instead of safeClose when the connection must stay open
+ * (e.g. the /api/embed handler that keeps serving queries after flushing).
+ *
+ * @see safeClose — CHECKPOINT + connection/database close
+ */
+export const flushWAL = async (): Promise<void> => {
+  if (!conn) return;
+  try {
+    await conn.query('CHECKPOINT');
+  } catch {
+    /* ignore — older LadybugDB or schemaless DB may not accept it */
   }
+};
+
+/**
+ * Flush the WAL and close the connection and database handles.
+ *
+ * Consolidates the CHECKPOINT + close pattern into a single function so
+ * callers never call conn.close() or db.close() directly (#1376).
+ * An ESLint no-restricted-syntax rule enforces this — see eslint.config.mjs.
+ *
+ * @see flushWAL — CHECKPOINT-only (connection stays open)
+ * @see closeLbug — safeClose + module state reset (full teardown)
+ */
+export const safeClose = async (): Promise<void> => {
+  await flushWAL();
   if (conn) {
     try {
+      // eslint-disable-next-line no-restricted-syntax -- sole authorised close site
       await conn.close();
-    } catch {}
+    } catch {
+      /* best-effort */
+    }
     conn = null;
   }
   if (db) {
     try {
+      // eslint-disable-next-line no-restricted-syntax -- sole authorised close site
       await db.close();
-    } catch {}
+    } catch {
+      /* best-effort */
+    }
     db = null;
   }
+};
+
+export const closeLbug = async (): Promise<void> => {
+  await safeClose();
   currentDbPath = null;
   ftsLoaded = false;
   vectorExtensionLoaded = false;

gitnexus/src/server/api.ts

@@ -19,6 +19,7 @@ import {
   executePrepared,
   executeWithReusedStatement,
   streamQuery,
+  flushWAL,
   closeLbug,
   withLbugDb,
 } from '../core/lbug/lbug-adapter.js';
@@ -1706,12 +1707,8 @@ export const createServer = async (port: number, host: string = '127.0.0.1') =>
             // Flush WAL so subsequent /api/search requests see the new
             // embeddings immediately (#1149). In the CLI path closeLbug()
             // handles this during process exit, but the server keeps the
-            // connection open for other routes -- a CHECKPOINT is enough.
-            try {
-              await executeQuery('CHECKPOINT');
-            } catch {
-              /* best-effort -- older LadybugDB may not support it */
-            }
+            // connection open for other routes — a CHECKPOINT is enough.
+            await flushWAL();
           });
 
           clearTimeout(embedTimeout);

gitnexus/test/helpers/test-indexed-db.ts

@@ -115,6 +115,12 @@ export function withTestLbugDB(
       }
     }
 
+    // 5b. Flush WAL so seed data + FTS indexes are visible to the pool
+    //     adapter's read path. Without this, Windows CI intermittently
+    //     fails FTS queries because the WAL hasn't been checkpointed
+    //     before the pool adapter starts reading.
+    await adapter.flushWAL();
+
     // 6. Open pool adapter by injecting the core adapter's writable Database.
     //    LadybugDB enforces file locks — writable + read-only can't coexist
     //    on the same path, and db.close() segfaults on macOS due to N-API

gitnexus/test/unit/lbug-checkpoint.test.ts

@@ -0,0 +1,88 @@
+/**
+ * Structural + behavioural tests for the WAL-flush / close helpers (#1376).
+ *
+ * After the review-driven refactor, the module exposes two layers:
+ *   - flushWAL  — CHECKPOINT only (connection stays open)
+ *   - safeClose — flushWAL + conn.close + db.close
+ *
+ * closeLbug delegates to safeClose for the CHECKPOINT + close step and
+ * then resets module-level state (currentDbPath, ftsLoaded, etc.).
+ *
+ * The structural tests read the adapter source and verify delegation
+ * contracts so a future refactor that inlines close logic is caught.
+ *
+ * The behavioural tests import flushWAL directly and exercise the
+ * runtime null-guard path (conn is null at module load) so a future
+ * refactor that accidentally throws is caught immediately.
+ */
+import { beforeAll, describe, expect, it } from 'vitest';
+import fs from 'node:fs/promises';
+import path from 'node:path';
+import { flushWAL } from '../../src/core/lbug/lbug-adapter.js';
+
+describe('flushWAL / safeClose — consolidation guard (#1376)', () => {
+  let adapterSource: string;
+
+  beforeAll(async () => {
+    adapterSource = await fs.readFile(
+      path.join(__dirname, '..', '..', 'src', 'core', 'lbug', 'lbug-adapter.ts'),
+      'utf-8',
+    );
+  });
+
+  it('exports flushWAL (CHECKPOINT-only helper)', () => {
+    expect(adapterSource).toMatch(/export const flushWAL/);
+  });
+
+  it('exports safeClose (CHECKPOINT + close helper)', () => {
+    expect(adapterSource).toMatch(/export const safeClose/);
+  });
+
+  it('safeClose delegates to flushWAL for the CHECKPOINT step', () => {
+    const safeCloseBody = adapterSource.slice(adapterSource.indexOf('export const safeClose'));
+    expect(safeCloseBody).toMatch(/await flushWAL\(\)/);
+  });
+
+  it('closeLbug delegates to safeClose instead of inlining conn.close/db.close', () => {
+    const closeLbugBody = adapterSource.slice(adapterSource.indexOf('export const closeLbug'));
+    expect(closeLbugBody).toMatch(/await safeClose\(\)/);
+    // closeLbug must NOT contain its own conn.close() or db.close() — those
+    // live exclusively inside safeClose now.
+    const closeLbugBlock = closeLbugBody.slice(0, closeLbugBody.indexOf('export const', 1) >>> 0);
+    expect(closeLbugBlock).not.toMatch(/conn\.close\(\)/);
+    expect(closeLbugBlock).not.toMatch(/db\.close\(\)/);
+  });
+
+  it('flushWAL is the only place that issues conn.query(CHECKPOINT)', () => {
+    const matches = adapterSource.match(/conn\.query\('CHECKPOINT'\)/g) ?? [];
+    expect(matches.length).toBe(1);
+  });
+
+  it('conn.close() only appears inside safeClose (with eslint-disable)', () => {
+    // Every conn.close() in the adapter must live inside safeClose, guarded
+    // by the eslint-disable comment. Count occurrences to catch leaks.
+    const matches = adapterSource.match(/await conn\.close\(\)/g) ?? [];
+    expect(matches.length).toBe(1);
+  });
+
+  it('db.close() only appears inside safeClose (with eslint-disable)', () => {
+    const matches = adapterSource.match(/await db\.close\(\)/g) ?? [];
+    expect(matches.length).toBe(1);
+  });
+});
+
+// Behavioural tests — exercise flushWAL at runtime rather than just
+// grepping source text.  At module load `conn` is null, so these hit
+// the early-return guard without needing a real LadybugDB instance.
+describe('flushWAL — runtime behaviour', () => {
+  it('resolves without error when no connection is open', async () => {
+    // conn is null at module load — flushWAL must not throw.
+    await expect(flushWAL()).resolves.toBeUndefined();
+  });
+
+  it('can be called repeatedly without throwing (idempotent)', async () => {
+    await flushWAL();
+    await flushWAL();
+    // No assertion needed beyond "did not throw".
+  });
+});

gitnexus/test/unit/rate-limit.test.ts

@@ -261,6 +261,13 @@ describe('production routes — rate-limit middleware wiring', () => {
       /app\.set\(\s*'trust proxy'\s*,\s*'loopback,\s*linklocal,\s*uniquelocal'\s*\)/,
     );
   });
+
+  it('embed route flushes WAL via flushWAL, not inline executeQuery (#1376)', () => {
+    // The embed handler must call the consolidated helper, not hand-roll
+    // its own try/catch around executeQuery('CHECKPOINT').
+    expect(apiSource).toMatch(/await flushWAL\(\)/);
+    expect(apiSource).not.toMatch(/executeQuery\('CHECKPOINT'\)/);
+  });
 });
 
 // Structural guard for #1360 — validates that the validation module uses