diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index a302c6989..429482a34 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/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((resolve) => setTimeout(resolve, SIGINT_CLEANUP_TIMEOUT_MS)), ]).finally(async () => { const { flushLoggerSync } = await import('../core/logger.js'); diff --git a/gitnexus/src/core/lbug/lbug-adapter.ts b/gitnexus/src/core/lbug/lbug-adapter.ts index 72b6bdd13..cf5f972ea 100644 --- a/gitnexus/src/core/lbug/lbug-adapter.ts +++ b/gitnexus/src/core/lbug/lbug-adapter.ts @@ -1913,26 +1913,31 @@ export const safeClose = async (): Promise => { } }; -export const closeLbug = async (options: { skipNativeClose?: boolean } = {}): Promise => { - 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 => { + await flushWAL(); +}; + +export const closeLbug = async (): Promise => { await safeClose(); currentDbPath = null; ftsLoaded = false; diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 4043075d9..0b5d9c9ba 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/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 */ } diff --git a/gitnexus/src/server/analyze-worker.ts b/gitnexus/src/server/analyze-worker.ts index 90575f22e..33faa986d 100644 --- a/gitnexus/src/server/analyze-worker.ts +++ b/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 }); diff --git a/gitnexus/test/integration/lbug-conn-serialization.test.ts b/gitnexus/test/integration/lbug-conn-serialization.test.ts index 3f0d68364..57a1ec32e 100644 --- a/gitnexus/test/integration/lbug-conn-serialization.test.ts +++ b/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 diff --git a/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts b/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts index ee56e80b7..ee1a8b22c 100644 --- a/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-embeddings-limit.test.ts b/gitnexus/test/unit/analyze-embeddings-limit.test.ts index cfd3ac0fd..c7f12fe7f 100644 --- a/gitnexus/test/unit/analyze-embeddings-limit.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-finalize-failure-exits.test.ts b/gitnexus/test/unit/analyze-finalize-failure-exits.test.ts index 013aa30a4..c87008726 100644 --- a/gitnexus/test/unit/analyze-finalize-failure-exits.test.ts +++ b/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', () => ({ diff --git a/gitnexus/test/unit/analyze-gitnexusrc.test.ts b/gitnexus/test/unit/analyze-gitnexusrc.test.ts index 807f3fce0..fa1b3f9fb 100644 --- a/gitnexus/test/unit/analyze-gitnexusrc.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-heap-respawn.test.ts b/gitnexus/test/unit/analyze-heap-respawn.test.ts index 1dea8b5ea..e384e460c 100644 --- a/gitnexus/test/unit/analyze-heap-respawn.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts b/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts index a8cdc082b..5e7f5190c 100644 --- a/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-local-embedding-error.test.ts b/gitnexus/test/unit/analyze-local-embedding-error.test.ts index cbdc3dc3c..c90896be2 100644 --- a/gitnexus/test/unit/analyze-local-embedding-error.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-no-stats-bridge.test.ts b/gitnexus/test/unit/analyze-no-stats-bridge.test.ts index dc87ae04e..e08098ff6 100644 --- a/gitnexus/test/unit/analyze-no-stats-bridge.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts b/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts index 0532db50b..a5bfb16ee 100644 --- a/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-wal-error.test.ts b/gitnexus/test/unit/analyze-wal-error.test.ts index a267ff538..0cc768043 100644 --- a/gitnexus/test/unit/analyze-wal-error.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-worker-pool-size.test.ts b/gitnexus/test/unit/analyze-worker-pool-size.test.ts index 9e8a5830b..8af0f213f 100644 --- a/gitnexus/test/unit/analyze-worker-pool-size.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/analyze-worker-timeout.test.ts b/gitnexus/test/unit/analyze-worker-timeout.test.ts index 830eda2cb..8a2dc04ef 100644 --- a/gitnexus/test/unit/analyze-worker-timeout.test.ts +++ b/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), })); diff --git a/gitnexus/test/unit/lbug-checkpoint.test.ts b/gitnexus/test/unit/lbug-checkpoint.test.ts index d4b7c36f2..f898030e2 100644 --- a/gitnexus/test/unit/lbug-checkpoint.test.ts +++ b/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')`