refactor(lbug): replace skipNativeClose flag with a dedicated closeLbugBeforeExit() (#2264)

The "skip the native close only when a process.exit is guaranteed to follow"
invariant was enforced by convention across ~4 call sites via a boolean option on
closeLbug — the exact foot-gun the review flagged. Encode the contract in the name
instead:

- New closeLbugBeforeExit() (CHECKPOINT via flushWAL, then return without the native
  close); closeLbug() drops the option and is the plain real-close again.
- run-analyze success + error paths: options.skipNativeCloseOnExit ?
  closeLbugBeforeExit() : closeLbug(). CLI SIGINT + worker SIGTERM call
  closeLbugBeforeExit() directly. skipNativeCloseOnExit stays on AnalyzeOptions as the
  caller's "I will exit" signal.
- lbug-checkpoint.test: assert closeLbugBeforeExit exists + has no native close, and
  match `closeLbug =` precisely so it doesn't prefix-match the new function.
- Retarget the conn-serialization integration case to closeLbugBeforeExit(); add the
  new export to the 12 analyze-*.test.ts lbug-adapter mocks.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm

gitnexus/src/cli/analyze.ts

@@ -13,7 +13,7 @@ import os from 'os';
 import { spawn } from 'child_process';
 import v8 from 'v8';
 import cliProgress from 'cli-progress';
-import { closeLbug, isLbugReady } from '../core/lbug/lbug-adapter.js';
+import { closeLbugBeforeExit, isLbugReady } from '../core/lbug/lbug-adapter.js';
 import {
   isLbugCheckpointIoError,
   isWalCorruptionError,
@@ -1180,16 +1180,16 @@ const analyzeCommandImpl = async (
     bar.stop();
     console.log('\n  Interrupted — cleaning up...');
     // process.exit(130) follows, so skip the native close (LadybugDB destructor
-    // can double-free after --pdg writes, #2264); closeLbug's CHECKPOINT still
-    // flushes the WAL. But that CHECKPOINT queues behind the connection lock held
-    // by an in-flight COPY, so a single Ctrl-C during a long --pdg COPY would
+    // can double-free after --pdg writes, #2264); closeLbugBeforeExit's CHECKPOINT
+    // still flushes the WAL. But that CHECKPOINT queues behind the connection lock
+    // held by an in-flight COPY, so a single Ctrl-C during a long --pdg COPY would
     // otherwise appear hung until the COPY releases. Bound it with a short timeout
     // so the interrupt stays responsive; the WAL replays on the next analyze
     // (#2264 review P3). A second Ctrl-C (`if (aborted) process.exit(1)` above)
     // remains the immediate escape hatch.
     const SIGINT_CLEANUP_TIMEOUT_MS = 2000;
     void Promise.race([
-      closeLbug({ skipNativeClose: true }).catch(() => {}),
+      closeLbugBeforeExit().catch(() => {}),
       new Promise<void>((resolve) => setTimeout(resolve, SIGINT_CLEANUP_TIMEOUT_MS)),
     ]).finally(async () => {
       const { flushLoggerSync } = await import('../core/logger.js');

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

@@ -1913,26 +1913,31 @@ export const safeClose = async (): Promise<void> => {
   }
 };
 
-export const closeLbug = async (options: { skipNativeClose?: boolean } = {}): Promise<void> => {
-  if (options.skipNativeClose) {
-    // The caller guarantees a process.exit afterward (CLI analyze success path).
-    // CHECKPOINT for durability via flushWAL, then DELIBERATELY skip the native
-    // connection and database teardown: LadybugDB's ClientContext/Connection
-    // destructor can double-free after large --pdg writes (gdb: `double free or
-    // corruption` in ClientContext::~ClientContext via NodeConnection::Close),
-    // aborting the process AFTER a fully-written, checkpointed index. flushWAL
-    // already persisted the data; process exit reclaims the native handles. We
-    // leave the handles referenced and module state intact so a GC finalizer
-    // cannot run the same destructor before exit, and any post-analyze read
-    // reuses the live connection. Mirrors the pool adapter's fire-and-forget
-    // native teardown (pool-adapter.ts) and the ONNX native-cleanup philosophy.
-    // Workaround for a LadybugDB engine bug (to be reported upstream).
-    // SAFETY: only valid when a process.exit is guaranteed to follow — the
-    // runFullAnalysis ERROR path intentionally does NOT pass this (it closes for
-    // real, so a soft-returning caller still releases handles and terminates).
-    await flushWAL();
-    return;
-  }
+/**
+ * CHECKPOINT for durability, then DELIBERATELY skip the native connection/database
+ * teardown. The name encodes the contract — there is no boolean flag to misuse:
+ * call this ONLY from a path that guarantees a `process.exit` immediately after
+ * (the CLI analyze success/SIGINT paths and the forked worker).
+ *
+ * LadybugDB's ClientContext/Connection destructor can double-free after large
+ * --pdg writes (gdb: `double free or corruption` in ClientContext::~ClientContext
+ * via NodeConnection::Close), aborting the process AFTER a fully-written,
+ * checkpointed index. flushWAL already persisted the data; process exit reclaims
+ * the native handles. We leave the handles referenced and module state intact so a
+ * GC finalizer cannot run the same destructor before exit, and any post-analyze
+ * read reuses the live connection. Mirrors the pool adapter's fire-and-forget
+ * native teardown (pool-adapter.ts) and the ONNX native-cleanup philosophy.
+ * Workaround for a LadybugDB engine bug (to be reported upstream).
+ *
+ * SAFETY: only valid when a process.exit is guaranteed to follow. Long-lived
+ * callers (MCP server, tests) leave `skipNativeCloseOnExit` unset, so
+ * runFullAnalysis closes for real via {@link closeLbug} — never this.
+ */
+export const closeLbugBeforeExit = async (): Promise<void> => {
+  await flushWAL();
+};
+
+export const closeLbug = async (): Promise<void> => {
   await safeClose();
   currentDbPath = null;
   ftsLoaded = false;

gitnexus/src/core/run-analyze.ts

@@ -21,6 +21,7 @@ import {
   executeQuery,
   executeWithReusedStatement,
   closeLbug,
+  closeLbugBeforeExit,
   loadCachedEmbeddings,
   deleteNodesForFile,
   deleteAllCommunitiesAndProcesses,
@@ -1574,9 +1575,10 @@ export async function runFullAnalysis(
     // in-flight CHECKPOINT cannot race the `safeClose` CHECKPOINT.
     await walCheckpointDriver.stop();
     // CLI callers (about to process.exit) skip the native close to dodge a
-    // LadybugDB destructor double-free after --pdg writes — the CHECKPOINT inside
-    // closeLbug keeps the index durable (#2264). Long-lived callers close for real.
-    await closeLbug({ skipNativeClose: options.skipNativeCloseOnExit });
+    // LadybugDB destructor double-free after --pdg writes — closeLbugBeforeExit
+    // CHECKPOINTs for durability then leaves the handles for process exit to
+    // reclaim (#2264). Long-lived callers close for real.
+    await (options.skipNativeCloseOnExit ? closeLbugBeforeExit() : closeLbug());
 
     progress('done', 100, 'Done');
 
@@ -1600,11 +1602,12 @@ export async function runFullAnalysis(
       // Skip the native close on the error path too: a real conn.close() after
       // large --pdg writes can itself abort in LadybugDB's ClientContext
       // destructor (#2264 review P2), turning an actionable exit-1 into a raw
-      // SIGABRT. Skipping leaves the handles open, but the CLI catch now
-      // force-exits when isLbugReady() (analyze.ts, #2264 review P1), so the
-      // process still terminates — no hang, no abort. flushWAL inside closeLbug
-      // keeps the partial index durable; process exit reclaims the handles.
-      await closeLbug({ skipNativeClose: options.skipNativeCloseOnExit });
+      // SIGABRT. closeLbugBeforeExit leaves the handles open, but the CLI catch
+      // now force-exits when isLbugReady() (analyze.ts, #2264 review P1), so the
+      // process still terminates — no hang, no abort. flushWAL keeps the partial
+      // index durable; process exit reclaims the handles. Long-lived callers
+      // (skipNativeCloseOnExit unset) close for real.
+      await (options.skipNativeCloseOnExit ? closeLbugBeforeExit() : closeLbug());
     } catch {
       /* swallow */
     }

gitnexus/src/server/analyze-worker.ts

@@ -15,7 +15,7 @@ import { runFullAnalysis, type AnalyzeOptions } from '../core/run-analyze.js';
 import { type AnalyzeResultIpc } from './analyze-worker-ipc.js';
 import { runWorkerAnalysis, createTerminalClaim } from './analyze-worker-core.js';
 import { assertAnalysisFinalized } from '../storage/repo-manager.js';
-import { closeLbug } from '../core/lbug/lbug-adapter.js';
+import { closeLbugBeforeExit } from '../core/lbug/lbug-adapter.js';
 
 interface StartMessage {
   type: 'start';
@@ -99,7 +99,7 @@ process.on('SIGTERM', () => {
     send({ type: 'error', message: 'Analysis cancelled (worker received SIGTERM)' });
   }
   void Promise.race([
-    closeLbug({ skipNativeClose: true }).catch((err: unknown) => {
+    closeLbugBeforeExit().catch((err: unknown) => {
       const message =
         err instanceof Error ? err.message : 'Worker checkpoint failed during SIGTERM';
       send({ type: 'error', message });

gitnexus/test/integration/lbug-conn-serialization.test.ts

@@ -73,9 +73,9 @@ withTestLbugDB('conn-serialization', () => {
       expect(result).toMatchObject({ deletedNodes: 0 });
     });
 
-    it('closeLbug({ skipNativeClose }) checkpoints but leaves the connection open (#2264 close-crash)', async () => {
+    it('closeLbugBeforeExit() checkpoints but leaves the connection open (#2264 close-crash)', async () => {
       const adapter = await import('../../src/core/lbug/lbug-adapter.js');
-      await adapter.closeLbug({ skipNativeClose: true });
+      await adapter.closeLbugBeforeExit();
       // The native conn/db are deliberately NOT torn down — that avoids LadybugDB's
       // ClientContext destructor double-free after --pdg writes. The connection
       // stays ready and queryable (the CHECKPOINT made the index durable; process

gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts

@@ -8,6 +8,7 @@ vi.mock('../../src/core/run-analyze.js', () => ({
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

gitnexus/test/unit/analyze-embeddings-limit.test.ts

@@ -8,6 +8,7 @@ vi.mock('../../src/core/run-analyze.js', () => ({
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

gitnexus/test/unit/analyze-finalize-failure-exits.test.ts

@@ -58,6 +58,7 @@ vi.mock('../../src/cli/skill-gen.js', () => ({ generateSkillFiles: vi.fn() }));
 vi.mock('../../src/cli/cli-message.js', () => ({ cliError: vi.fn() }));
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: isLbugReadyMock,
 }));
 vi.mock('../../src/storage/repo-manager.js', () => ({

gitnexus/test/unit/analyze-gitnexusrc.test.ts

@@ -41,6 +41,7 @@ vi.mock('../../src/cli/skill-gen.js', () => ({ generateSkillFiles: generateSkill
 vi.mock('../../src/cli/cli-message.js', () => ({ cliError: cliErrorMock }));
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

gitnexus/test/unit/analyze-heap-respawn.test.ts

@@ -25,6 +25,7 @@ vi.mock('os', async () => {
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

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

@@ -8,6 +8,7 @@ vi.mock('../../src/core/run-analyze.js', () => ({
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

gitnexus/test/unit/analyze-local-embedding-error.test.ts

@@ -27,6 +27,7 @@ vi.mock('../../src/core/run-analyze.js', () => ({
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

gitnexus/test/unit/analyze-no-stats-bridge.test.ts

@@ -35,6 +35,7 @@ vi.mock('../../src/cli/cli-message.js', () => ({
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts

@@ -40,6 +40,7 @@ vi.mock('../../src/core/run-analyze.js', () => ({
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

gitnexus/test/unit/analyze-wal-error.test.ts

@@ -20,6 +20,7 @@ vi.mock('../../src/core/run-analyze.js', () => ({
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

gitnexus/test/unit/analyze-worker-pool-size.test.ts

@@ -8,6 +8,7 @@ vi.mock('../../src/core/run-analyze.js', () => ({
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

gitnexus/test/unit/analyze-worker-timeout.test.ts

@@ -8,6 +8,7 @@ vi.mock('../../src/core/run-analyze.js', () => ({
 
 vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
   closeLbug: vi.fn(async () => undefined),
+  closeLbugBeforeExit: vi.fn(async () => undefined),
   isLbugReady: vi.fn(() => false),
 }));
 

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

@@ -54,7 +54,8 @@ describe('flushWAL / safeClose — consolidation guard (#1376)', () => {
   });
 
   it('closeLbug delegates to safeClose instead of inlining conn.close/db.close', () => {
-    const closeLbugBody = adapterSource.slice(adapterSource.indexOf('export const closeLbug'));
+    // Match `closeLbug =` precisely so we don't prefix-match `closeLbugBeforeExit`.
+    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.
@@ -63,6 +64,20 @@ describe('flushWAL / safeClose — consolidation guard (#1376)', () => {
     expect(closeLbugBlock).not.toMatch(/db\.close\(\)/);
   });
 
+  it('exports closeLbugBeforeExit (CHECKPOINT-only, skips native close) (#2264)', () => {
+    expect(adapterSource).toMatch(/export const closeLbugBeforeExit/);
+    // closeLbugBeforeExit is declared immediately before closeLbug; its body must
+    // CHECKPOINT via flushWAL and NEVER do a native conn/db close (that's the
+    // whole point — it relies on a guaranteed process.exit).
+    const body = adapterSource.slice(
+      adapterSource.indexOf('export const closeLbugBeforeExit'),
+      adapterSource.indexOf('export const closeLbug ='),
+    );
+    expect(body).toMatch(/await flushWAL\(\)/);
+    expect(body).not.toMatch(/conn\.close\(\)/);
+    expect(body).not.toMatch(/db\.close\(\)/);
+  });
+
   it('CHECKPOINT is issued only by flushWAL (best-effort) and tryFlushWAL (rethrows for the retry driver)', () => {
     // Receiver-agnostic: since the connection-serialization refactor (#2264)
     // both sites capture `const c = conn` and call `c.query('CHECKPOINT')`