From 9efc6bfcad56ca8a6a97083b18702a1b1c059fdf Mon Sep 17 00:00:00 2001 From: Abhigyan Patwari <126312502+abhigyanpatwari@users.noreply.github.com> Date: Wed, 22 Jul 2026 02:28:00 +0530 Subject: [PATCH] fix(lbug/analyze): atomic index swap + read-pool staleness invalidation (#2614) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(lbug): re-open the read pool when analyze rebuilds the index under it The MCP read pool's initLbug early-returned on an existing pool entry with no freshness check, so after analyze rebuilt or mutated the on-disk index the pool kept serving the old (POSIX: unlinked-but-open) inode until LRU/idle eviction — a silent stale-read window of up to IDLE_TIMEOUT_MS (5 min). Record the file identity {ino, mtimeMs, size} on each PoolEntry at open, and re-stat in initLbug: unchanged → reuse; changed & idle → closeOne + reopen the new file; changed while a query is in flight → serve the current handle (a later idle initLbug reopens, since closing an in-use connection is a native use-after-free). A stat failure (ENOENT during a full rebuild's unlink window) is treated as unchanged so the reader keeps its valid open inode until the new file appears. Mirrors the bridge cache's mtime-invalidation pattern. Step 1 of docs/plans/2026-07-21-...-analyze-atomic-swap-invalidation. The end-to-end reopen-on-swap path is exercised by the reader-during-rebuild integration test in a later step. * fix(analyze): publish a full rebuild via an atomic swap (POSIX) The full-rebuild path wiped the live index (wipeLbugDbFiles(lbugPath)) and rebuilt it in place, so a concurrent MCP reader that opened mid-build could see an empty/half-loaded DB, and a crash between the wipe and the end-of-run left the index destroyed (recoverable only by --force). Build the fresh index at .new and swap it over the live index in one atomic rename at the end. All DB work flows through the singleton connection, so only initLbug/wipeLbugDbFiles take the temp target; the close already checkpoint-consolidates the build to a single file (verified: no residual .wal/.shadow), so the rename publishes a complete index in one step. A reader opening mid-build only ever sees the previous complete index; a reader holding the old inode keeps a consistent stale snapshot until the pool re-opens onto the new one (the pool staleness invalidation from the prior commit). On failure the swap is skipped, leaving the previous index byte-for-byte intact. POSIX only: the common CLI/serve-worker analyze paths skip the native close (closeLbugBeforeExit, #2264) and leave the build handle open at swap time. POSIX renames an open file cleanly; a same-process open handle blocks the rename on Windows, so Windows keeps the current in-place behavior (buildPath === lbugPath) until that is resolved. The Windows atomic swap and a deterministic concurrent reader-during-rebuild test are deferred follow-ups. Steps 2b + partial 3 of docs/plans/2026-07-21-...-analyze-atomic-swap-invalidation. Integration test asserts the no-temp-leak + inode-swap invariants and the crash-safety guarantee (a load failure leaves the live index untouched). * test(analyze): end-to-end read-pool reopen after an atomic swap Adds the deferred reader-during-rebuild / pool-reopen integration test: analyze v1 -> read pool serves it -> rebuild with a renamed function (atomic swap) -> the same repoId's initLbug detects the swapped inode and re-opens the pool onto the new index. Asserts the pool sees the renamed function and NOT the stale v1 name, exercising #1 (invalidation) and #2 (swap) together end to end. * fix(lbug): bound pooled read queries with setQueryTimeout The read pool relied only on a JS-side Promise.race (QUERY_TIMEOUT_MS) that frees the waiter but leaves the native call running. Set the engine-level setQueryTimeout on every pooled connection so a pathological query is bounded at the source too. * fix(lbug): name the held-open cause for WAL checkpoint failures (#2599) A WAL-checkpoint IO error that also carries a busy/lock signal means another handle (a gitnexus mcp server, or this process's own reader) holds the store open, not a disk fault. Add isLbugCheckpointBusyError (reusing the tested isDbBusyError keyword set) and, when the checkpoint driver exhausts its retry budget on such an error, annotate the surfaced error with the actionable held-open cause instead of a raw IO string. Note: overlaps in-flight work on repro/issue-2599-windows-wal-checkpoint; bundled here at the maintainer's request. * feat(analyze): opt-in atomic incremental + best-effort Windows swap Extends the atomic-swap publish (POSIX full rebuild) to two more cases: - Windows: the swap now applies when a real close is safe to release the build handle before the rename — i.e. non-pdg runs (windowsSwapOk excludes --pdg, the #2264 destructor-crash case), forcing a real close on the swap path. UNVERIFIED on Windows (no Windows runner here); --pdg and any failure fall back to today's in-place behavior, so it can never corrupt. - Incremental (opt-in, GITNEXUS_ATOMIC_INCREMENTAL=1): copies the live index into the temp, applies the incremental delete/writeback to the copy, and swaps at the end. Off by default because the whole-file copy negates incremental's speed premise — kept behind a flag pending a benchmark. The escalation valve also targets the temp so an escalated write stays atomic. Integration test covers the opt-in incremental path end to end (no temp leak, the incremental change is reflected after the swap). * refactor(lbug): centralize the read-pool + bridge open-retry budgets The lbug-config retry registry documented the open/handle-release/query-time budgets but the read pool's LOCK_RETRY_* (pool-adapter) and the bridge's LBUG_OPEN_RETRY_* (group/bridge-db) kept private copies that could drift. Move both into the registry as exported constants (POOL_OPEN_LOCK_RETRY_*, BRIDGE_OPEN_RETRY_*) and alias the local names to them — one tuning surface, no behavior change. * fix: address CI regressions from the bundled follow-ups - setQueryTimeout: guard the call so test doubles that don't model the engine method don't break connection creation. - atomic swap: skip the rename when the build produced no DB at buildPath (an empty repo / mocked pipeline) instead of throwing ENOENT. - #2599: don't wrap the checkpoint error in the driver (it hid the IO signature the CLI's --wal-checkpoint-threshold hint keys on); name the held-open cause at the CLI instead, beside that hint, keeping the original error intact. - retry consolidation: revert to documentation-only — moving the pool/bridge budgets into lbug-config broke every explicit lbug-config test mock. The registry now catalogues all budgets with their in-file locations. - analyze-wal-checkpoint-failure test: block both lbug.wal.checkpoint and lbug.new.wal.checkpoint, since a full rebuild now checkpoints the temp. * fix(analyze): publish the swap before stamping meta; identity-gate the reader (#2614 F1) Review found a HIGH regression: the full-rebuild wrote the freshness stamp (saveMeta, indexedAt=T_new) BEFORE the atomic swap, so a concurrent MCP reader that reinited in the saveMeta->swap window opened the OLD inode, recorded observed=T_new, and then never reinited again (ensureInitialized returns early on 'current') — serving the pre-rebuild graph indefinitely. The build-into-temp change inverted the pre-PR invariant that 'meta shows T_new' implied 'lbugPath holds T_new data'. Two coordinated fixes: - run-analyze: move the final saveMeta AFTER the swap, so meta.indexedAt only becomes visible once lbugPath resolves to the new inode. Verified nothing in the span reads on-disk meta and registerRepo writes only the registry. Leaving the dirty flag set across the swap also improves crash-safety. - local-backend: the reader staleness gate now also compares the lbug file IDENTITY (ino/mtime/size), reiniting on an inode change even when meta.indexedAt is unchanged. This closes the swap-window latch and covers the in-place incremental case — and is what actually makes the pool's dbIdentity net reachable for the MCP reader (the indexedAt gate otherwise bypassed it). * fix(lbug/analyze): WAL-aware incremental, residual-sidecar reconcile, Windows opt-in, #2599 anchor (#2614 F2-F4) Review remediations: - F3: gate atomic incremental on a CLEAN live index (inspectLbugSidecars) — the main-file-only copy would drop an orphan .wal's delta; fall back to in-place. - F4: on the swap, MOVE a residual .wal/.shadow beside the published index (not orphan it) so a swallowed final checkpoint's delta is replayed. - F2: record identity on the shared read-only Database and warn when a cached handle is reused after its on-disk index was rebuilt while another consumer holds it (unreachable via MCP — one consumer per lbugPath; a complete fix needs per-inode handles, documented). - Windows swap: opt-in (GITNEXUS_ATOMIC_WINDOWS_SWAP=1), default off — the forced real close re-bets an unproven #2264 assumption and can't be verified without a Windows runner, so the default Windows path stays in-place. - #2599: anchor isLbugCheckpointBusyError to real held-open wording instead of isDbBusyError's bare .includes('lock') over a message that embeds the DB path (a repo under blockchain-app misclassified a disk fault as held-open). - Docs: corrected retry-catalogue budgets (linear, not exp) and the checkedOut>0 bound comment (load-bounded, not IDLE_TIMEOUT_MS). * test(analyze): cover the production close path in the atomic swap (#2614 F5) Adds a full-rebuild swap test with skipNativeCloseOnExit:true — the close path the CLI and serve-worker actually ship (build handle left open at swap time), distinct from the default real-close the other swap tests exercise. Asserts the POSIX swap still publishes a single consolidated lbug with no .new temp and no orphan sidecar. * test(analyze): give the follow-up git commits an inline identity (CI fix) The end-to-end reopen and atomic-incremental tests' second commits used a bare `git commit`, which fails on CI runners with no global git identity (empty ident name). makeRepo's initial commit already passes -c user.name/-c user.email inline; apply the same to the rename/change commits. No code change. * fix(mcp): route reader reinit through initLbug's active-query guard (#2614 review) Review found an active-query retirement race: LocalBackend.ensureInitialized detected an identity/stamp change and called closeLbug(poolKey) DIRECTLY, but closeOne closes the shared Database at refCount 0 regardless of checked-out connections. So a reader detecting the new generation could close the Database a concurrent query is still executing on — a native use-after-free. This bypassed the checkedOut>0 guard that initLbug itself has. Fix (delegate, not close directly): initLbug now returns whether it actually rolled the pool over; ensureInitialized calls initLbug (which serves the current handle while a query is in flight and reopens only when idle) instead of closeLbug. The observed IDENTITY is advanced only when the pool actually reopened — if a query was in flight, the identity stays divergent and the reopen retries on a later idle check rather than latching on the old handle. The observed STAMP advances regardless so a same-file stamp change can't loop. Old generation now stays alive until its in-flight queries drain (lazy rollover); new requests during the busy window share the old handle until the pool goes idle, then reopen. No parallel open-both-generations, but no UAF and no stale latch. --------- Co-authored-by: Gergő Magyar --- gitnexus/src/cli/analyze.ts | 9 + gitnexus/src/core/group/bridge-db.ts | 2 + gitnexus/src/core/lbug/lbug-config.ts | 36 +++ gitnexus/src/core/lbug/pool-adapter.ts | 117 +++++++++- .../src/core/lbug/wal-checkpoint-driver.ts | 3 + gitnexus/src/core/run-analyze.ts | 137 ++++++++++- gitnexus/src/mcp/local/local-backend.ts | 48 +++- .../integration/analyze-atomic-swap.test.ts | 220 ++++++++++++++++++ .../analyze-wal-checkpoint-failure.test.ts | 12 +- .../test/unit/checkpoint-busy-2599.test.ts | 34 +++ .../unit/pool-freshness-invalidation.test.ts | 91 ++++++++ 11 files changed, 683 insertions(+), 26 deletions(-) create mode 100644 gitnexus/test/integration/analyze-atomic-swap.test.ts create mode 100644 gitnexus/test/unit/checkpoint-busy-2599.test.ts create mode 100644 gitnexus/test/unit/pool-freshness-invalidation.test.ts diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index ab4b8e00d..3d4505b6e 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -18,6 +18,7 @@ import { boundedCheckpointBeforeExit } from '../core/lbug/shutdown-helpers.js'; import { getOsPageSize, isLbugCheckpointIoError, + isLbugCheckpointBusyError, isLbugPageSizeFrameError, isPageSizeAwareLadybug, isWalCorruptionError, @@ -1624,8 +1625,16 @@ const analyzeCommandImpl = async ( } if (isLbugCheckpointIoError(err)) { + // #2599: when the checkpoint IO error also looks busy/locked, another + // handle holds the store open — name that actionable cause alongside the + // threshold hint (the original error is preserved so the hint still fires). + const heldOpen = isLbugCheckpointBusyError(err) + ? ` Another process may hold the store open (a running \`gitnexus mcp\` server, or a\n` + + ` stale reader) — close other GitNexus processes on this repo, then retry.\n` + : ''; cliError( ` LadybugDB failed while rotating/removing WAL checkpoint files.\n` + + heldOpen + ` This can happen when auto-checkpoint runs at the default threshold (~16MB).\n` + ` Retry with a larger checkpoint threshold to reduce checkpoint frequency:\n` + ` gitnexus analyze --wal-checkpoint-threshold ${RECOMMENDED_WAL_CHECKPOINT_THRESHOLD}\n` + diff --git a/gitnexus/src/core/group/bridge-db.ts b/gitnexus/src/core/group/bridge-db.ts index 6d99174a2..15bb1fb15 100644 --- a/gitnexus/src/core/group/bridge-db.ts +++ b/gitnexus/src/core/group/bridge-db.ts @@ -1031,6 +1031,8 @@ const LBUG_OPEN_RETRY_PATTERNS = [ 'lock held by another process', ]; +// Cross-repo bridge RO open retry. Catalogued as entry 5 of the lbug-config +// retry-budget registry; caps back-off so total wait ~3s. const LBUG_OPEN_RETRY_ATTEMPTS = 10; const LBUG_OPEN_RETRY_BASE_MS = 100; /** Cap individual back-off delays so the total wait is bounded (~3s). */ diff --git a/gitnexus/src/core/lbug/lbug-config.ts b/gitnexus/src/core/lbug/lbug-config.ts index 7b4ba4c65..d5de2161e 100644 --- a/gitnexus/src/core/lbug/lbug-config.ts +++ b/gitnexus/src/core/lbug/lbug-config.ts @@ -628,6 +628,29 @@ export const isDbBusyError = (err: unknown): boolean => { ); }; +/** + * True when a WAL-checkpoint IO error ALSO carries a busy/lock signal — the + * rotation failed because another handle (a `gitnexus mcp` server, or this + * process's own reader) holds the store's WAL open, rather than a permanent + * disk error. Reuses `isDbBusyError`'s already-tested keyword set instead of a + * fresh regex, so an unmatched message degrades to "IO error" rather than + * silently claiming a held-open cause. (#2599) + */ +export const isLbugCheckpointBusyError = (err: unknown): boolean => { + if (!isLbugCheckpointIoError(err)) return false; + // Anchor to real held-open wording rather than isDbBusyError's broad + // `.includes('lock')`, which matches the DB PATH embedded in the checkpoint + // error message (e.g. a repo under `blockchain-app`) and would misclassify a + // pure disk fault as held-open (#2614 LOW). + const msg = (err instanceof Error ? err.message : String(err)).toLowerCase(); + return ( + msg.includes('could not set lock') || + msg.includes('lock is held') || + msg.includes('being used by another process') || + msg.includes('is busy') + ); +}; + /** See {@link classifyDeleteAllError}. */ export type DeleteAllErrorClass = 'benign-missing-table' | 'rethrow'; @@ -715,6 +738,19 @@ export const HANDLE_RELEASE_PROBE_ATTEMPTS = 5; export const HANDLE_RELEASE_PROBE_DELAY_MS = 50; const HANDLE_RELEASE_LOCK_CODES = new Set(['EBUSY', 'EPERM', 'EACCES']); +// Retry-budget registry, part 2 (retry-budget consolidation): the remaining +// open-time lock retries live next to their call sites but are catalogued here +// so all lbug retry budgets surface in one grep. They retry the same lock class +// as 1–3 ("Could not set lock" while a writer rebuilds the index): +// 4. LOCK_RETRY_ATTEMPTS / LOCK_RETRY_DELAY_MS (pool-adapter.ts) +// → read pool's read-only open while `gitnexus analyze` is writing +// (3 attempts, linear 2s·n back-off ≈ 6s total) +// 5. LBUG_OPEN_RETRY_ATTEMPTS / _BASE_MS / _MAX_MS (group/bridge-db.ts) +// → cross-repo bridge RO open race (10 attempts, linear 100ms·n capped +// at 500ms ≈ 3.5s total) +// Kept in-file (not moved here) so explicit `lbug-config` test mocks don't have +// to enumerate them; change a budget in its call site and update this catalogue. + /** * Test-fixture directory prefixes recognized by `isTestFixturePath`. * diff --git a/gitnexus/src/core/lbug/pool-adapter.ts b/gitnexus/src/core/lbug/pool-adapter.ts index 307328860..a53ca0c92 100644 --- a/gitnexus/src/core/lbug/pool-adapter.ts +++ b/gitnexus/src/core/lbug/pool-adapter.ts @@ -53,10 +53,44 @@ interface PoolEntry { }>; lastUsed: number; dbPath: string; + /** Filesystem identity of the on-disk DB at open time. When `analyze` + * rebuilds or mutates the index, this diverges from the current file and + * initLbug re-opens the pool onto the new file instead of serving the + * stale open inode. Null for injected/external databases (initLbugWithDb), + * which are never invalidated this way. */ + dbIdentity: DbIdentity | null; /** Set to true when the pool entry is closed — checkin will close orphaned connections */ closed: boolean; } +/** Filesystem identity used to detect an index rebuilt/mutated under a live + * read pool. `ino` catches a full-rebuild unlink+recreate or an atomic-rename + * swap; `mtimeMs`+`size` catch an in-place incremental writeback. */ +interface DbIdentity { + ino: number; + mtimeMs: number; + size: number; +} + +export async function statDbIdentity(dbPath: string): Promise { + try { + const s = await fs.stat(dbPath); + return { ino: s.ino, mtimeMs: s.mtimeMs, size: s.size }; + } catch { + return null; + } +} + +/** True only when both identities are known AND differ. A stat failure + * (ENOENT during the brief unlink window of a full rebuild) yields false, so + * the reader keeps serving its still-valid open inode until the NEW file + * appears with a different identity — avoiding a churn into a failed reopen + * mid-rebuild. */ +export function dbIdentityChanged(prev: DbIdentity | null, next: DbIdentity | null): boolean { + if (!prev || !next) return false; + return prev.ino !== next.ino || prev.mtimeMs !== next.mtimeMs || prev.size !== next.size; +} + const pool = new Map(); /** @@ -92,6 +126,10 @@ interface SharedDB { db: lbug.Database; refCount: number; ftsLoaded: boolean; + /** File identity at open — used to detect reuse of a shared read-only handle + * whose on-disk index was rebuilt/swapped since it opened (only reachable + * when a second pool consumer shares this dbPath; #2614 F2). */ + dbIdentity?: DbIdentity | null; /** When true, closeOne skips db.close() — the Database is owned externally. */ external?: boolean; } @@ -389,7 +427,16 @@ setInterval(() => { function createConnection(db: lbug.Database): lbug.Connection { silenceStdout(); try { - return new lbug.Connection(db); + const conn = new lbug.Connection(db); + // Bound a single query at the engine level so a pathological query cannot + // hang a pooled connection past the JS-side Promise.race guard (which frees + // the waiter but not the native call). Matches QUERY_TIMEOUT_MS. Guarded so + // test doubles that don't model the engine method don't break connection + // creation. + if (typeof conn.setQueryTimeout === 'function') { + conn.setQueryTimeout(QUERY_TIMEOUT_MS); + } + return conn; } finally { restoreStdout(); } @@ -400,6 +447,8 @@ const QUERY_TIMEOUT_MS = 30_000; /** Waiter queue timeout in milliseconds */ const WAITER_TIMEOUT_MS = 15_000; +// Read-only open retry while `gitnexus analyze` writes. Catalogued as entry 4 +// of the lbug-config retry-budget registry. const LOCK_RETRY_ATTEMPTS = 3; const LOCK_RETRY_DELAY_MS = 2000; const SHADOW_REPLAY_PROBE_QUERY = 'MATCH (n) RETURN n LIMIT 1'; @@ -593,18 +642,45 @@ const initPromises = new Map>(); * Concurrent calls for the same repoId are deduplicated — the second caller * awaits the first's in-progress init rather than starting a redundant one. */ -export const initLbug = async (repoId: string, dbPath: string): Promise => { +/** + * Returns `true` when this call (re)opened a fresh handle onto the current + * on-disk file, `false` when it reused/served the existing handle (unchanged, + * or changed-but-a-query-is-in-flight). Callers that gate their own freshness + * bookkeeping on "did the pool actually roll over" (LocalBackend) use the + * return value; callers that only need the pool ready can ignore it. + */ +export const initLbug = async (repoId: string, dbPath: string): Promise => { const existing = pool.get(repoId); if (existing) { existing.lastUsed = Date.now(); - return; + // Detect an index that `analyze` rebuilt or mutated under this live read + // pool. Without this, the pool keeps serving the old (POSIX: + // unlinked-but-open) inode until LRU/idle eviction — a stale-read window + // of up to IDLE_TIMEOUT_MS after analyze finishes. + const current = await statDbIdentity(dbPath); + if (!dbIdentityChanged(existing.dbIdentity, current)) return false; // unchanged → reuse + // A query is in flight on this entry; closing its connection (and the + // shared Database at refCount 0) mid-use is a native use-after-free. Serve + // the current handle for this dispatch — the next initLbug that finds the + // entry idle (checkedOut === 0) reopens, since the identity stays divergent + // until then. Under sustained overlapping queries `checkedOut` may never + // reach 0 and `lastUsed` keeps the idle timer from evicting, so this window + // is bounded by the load, not IDLE_TIMEOUT_MS — the data stays consistent + // (a complete older snapshot), just not the newest. Callers that route + // freshness THROUGH initLbug (rather than calling closeLbug directly) get + // this guard for free; that is why LocalBackend delegates here (#2614). + if (existing.checkedOut > 0) return false; + closeOne(repoId); // idle & changed → evict, then fall through to reopen the new file } // Deduplicate concurrent init calls for the same repoId — // prevents double-init race when multiple parallel tool calls // trigger initialization for the same repo simultaneously. const pending = initPromises.get(repoId); - if (pending) return pending; + if (pending) { + await pending; + return true; + } const promise = doInitLbug(repoId, dbPath); initPromises.set(repoId, promise); @@ -613,6 +689,7 @@ export const initLbug = async (repoId: string, dbPath: string): Promise => } finally { initPromises.delete(repoId); } + return true; }; /** @@ -633,6 +710,23 @@ async function doInitLbug(repoId: string, dbPath: string): Promise { // Reuse an existing native Database if another repoId already opened this path. // This prevents buffer manager exhaustion from multiple mmap regions on the same file. let shared = dbCache.get(dbPath); + if (shared && !shared.external && shared.dbIdentity) { + // #2614 F2: a cached read-only Database is keyed by dbPath and shared across + // pool consumers. If the on-disk index was rebuilt/swapped (new inode) while + // ANOTHER consumer still holds this handle (refCount kept it alive), reusing + // it serves a superseded index. Unreachable via the MCP backend (one + // consumer per lbugPath ⇒ refCount hits 0 ⇒ closeOne reopens fresh); a + // complete fix needs per-inode handles rather than a dbPath-keyed cache. + // Surface it so the corner is observable instead of silently stale. + const current = await statDbIdentity(dbPath); + if (dbIdentityChanged(shared.dbIdentity, current)) { + realStderrWrite( + `GitNexus: reusing a shared read-only handle for ${dbPath} whose on-disk ` + + `index was rebuilt while another consumer holds it — results may be stale ` + + `until that consumer releases it.\n`, + ); + } + } if (!shared) { // Open in read-only mode — MCP server never writes to the database. // This allows multiple MCP server instances to read concurrently, and @@ -641,7 +735,7 @@ async function doInitLbug(repoId: string, dbPath: string): Promise { for (let attempt = 1; attempt <= LOCK_RETRY_ATTEMPTS; attempt++) { try { const db = await openReadOnlyDatabase(dbPath); - shared = { db, refCount: 0, ftsLoaded: false }; + shared = { db, refCount: 0, ftsLoaded: false, dbIdentity: await statDbIdentity(dbPath) }; dbCache.set(dbPath, shared); break; } catch (err: any) { @@ -650,7 +744,12 @@ async function doInitLbug(repoId: string, dbPath: string): Promise { if (isWalCorruptionError(lastError)) { try { const db = await tryQuarantineAndReopen(dbPath, repoId); - shared = { db, refCount: 0, ftsLoaded: false }; + shared = { + db, + refCount: 0, + ftsLoaded: false, + dbIdentity: await statDbIdentity(dbPath), + }; dbCache.set(dbPath, shared); break; } catch (retryErr) { @@ -715,6 +814,9 @@ async function doInitLbug(repoId: string, dbPath: string): Promise { // Register pool entry only after all connections are pre-warmed and FTS is // loaded. Concurrent executeQuery calls see either "not initialized" // (and throw cleanly) or a fully ready pool — never a half-built one. + // Record the on-disk identity so a later initLbug can detect an analyze + // rebuild/mutation and re-open onto the new file (pool staleness invalidation). + const dbIdentity = await statDbIdentity(dbPath); pool.set(repoId, { db, available, @@ -722,6 +824,7 @@ async function doInitLbug(repoId: string, dbPath: string): Promise { waiters: [], lastUsed: Date.now(), dbPath, + dbIdentity, closed: false, }); ensureIdleTimer(); @@ -785,6 +888,8 @@ export async function initLbugWithDb( waiters: [], lastUsed: Date.now(), dbPath, + // Injected/external DB (tests) — not tracked for rebuild invalidation. + dbIdentity: null, closed: false, }); ensureIdleTimer(); diff --git a/gitnexus/src/core/lbug/wal-checkpoint-driver.ts b/gitnexus/src/core/lbug/wal-checkpoint-driver.ts index 458c63947..09665127f 100644 --- a/gitnexus/src/core/lbug/wal-checkpoint-driver.ts +++ b/gitnexus/src/core/lbug/wal-checkpoint-driver.ts @@ -118,6 +118,9 @@ export const runCheckpointWithRetry = async ( { attempts: CHECKPOINT_RETRY_ATTEMPTS }, 'GitNexus: manual WAL checkpoint exhausted retry budget — surfacing IO error to caller', ); + // The held-open cause (#2599) is named at the CLI layer (analyze.ts) where the + // --wal-checkpoint-threshold recovery hint already renders, so the original IO + // error is preserved intact for that classifier rather than re-wrapped here. throw lastError; }; diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 9d5e7a811..cd3c9a458 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -11,6 +11,7 @@ import path from 'path'; import fs from 'fs/promises'; +import { retryRename } from '../storage/fs-atomic.js'; import { runPipelineFromRepo } from './ingestion/pipeline.js'; import type { KnowledgeGraph } from './graph/types.js'; import { resetDegradedParseCounter } from './tree-sitter/safe-parse.js'; @@ -55,7 +56,10 @@ import { checkpointOnce, type WalCheckpointDriver, } from './lbug/wal-checkpoint-driver.js'; -import { quarantineSidecarsForDirtyRecovery } from './lbug/sidecar-recovery.js'; +import { + quarantineSidecarsForDirtyRecovery, + inspectLbugSidecars, +} from './lbug/sidecar-recovery.js'; import type { EmbeddingIdentity } from './embeddings/embedding-identity.js'; import { getStoragePaths, @@ -1329,6 +1333,54 @@ export async function runFullAnalysis( ? diffFileHashes(newFileHashes, existingMeta!.fileHashes) : undefined; + // #2 atomic index publish: on a full rebuild, build the fresh DB at a temp + // path and swap it over the live index in one rename at the very end, so a + // concurrent MCP reader opening mid-build only ever sees the previous + // complete index (never a wiped/half-built file) and a crash leaves the old + // index intact. The whole build flows through the singleton connection, so + // only initLbug/wipeLbugDbFiles below take the temp target. + // + // POSIX only: the common CLI/serve-worker analyze paths skip the native close + // (closeLbugBeforeExit, #2264) and leave the build handle open at swap time. + // POSIX renames an open file cleanly; a same-process open handle blocks the + // rename on Windows. Windows keeps the current in-place behavior + // (buildPath === lbugPath, no swap) until that is resolved (see §12/follow-up). + const isFullRebuild = !(isIncremental && hashDiff); + // Where the swap is allowed: + // - POSIX renames an open file, so the usual skip-native-close (#2264) is + // fine and the swap always applies. + // - Windows can swap only when a real close is safe to release the build + // handle before the rename — i.e. NOT a --pdg run (the #2264 destructor + // crash). Unverified on Windows CI; falls back to in-place otherwise. + const posixSwap = process.platform !== 'win32'; + // #2614 Windows: the forced real-close before the rename re-bets that #2264 is + // --pdg-only, which is unproven (the CLI/worker skip the native close + // UNCONDITIONALLY) and unverifiable without a Windows runner. Keep it opt-in + // (GITNEXUS_ATOMIC_WINDOWS_SWAP=1) so the default Windows analyze stays on the + // proven in-place path; enable it only to test the Windows swap. + const windowsSwapOk = + process.platform === 'win32' && + options.pdg !== true && + process.env.GITNEXUS_ATOMIC_WINDOWS_SWAP === '1'; + // Incremental atomicity copies the whole index into the temp before mutating + // it, which negates incremental's speed premise — so it is opt-in + // (GITNEXUS_ATOMIC_INCREMENTAL=1) pending a benchmark. Full rebuilds always + // swap where the platform allows. + const wantAtomicIncremental = + isIncremental && !!hashDiff && process.env.GITNEXUS_ATOMIC_INCREMENTAL === '1'; + // #2614 F3: the copy-then-swap stages ONLY the main lbug file, so a live index + // carrying an orphan .wal/.shadow (a silently-failed prior checkpoint) would + // be copied incompletely and lose that delta. Only take the atomic path when + // the live index is a consolidated single file; otherwise fall back to the + // in-place writeback, which the next open replays correctly. + const atomicIncremental = + wantAtomicIncremental && (await inspectLbugSidecars(lbugPath)).kind === 'clean'; + if (wantAtomicIncremental && !atomicIncremental) { + log('atomic-incremental: live index carries orphan sidecars — using in-place writeback'); + } + const useAtomicSwap = (isFullRebuild || atomicIncremental) && (posixSwap || windowsSwapOk); + const buildPath = useAtomicSwap ? `${lbugPath}.new` : lbugPath; + if (isIncremental && hashDiff) { log( `Incremental: changed=${hashDiff.changed.length}, ` + @@ -1351,6 +1403,14 @@ export async function runFullAnalysis( directWriteCount: hashDiff.toWrite.length, }, }); + if (atomicIncremental) { + // Stage the live index into the temp so the in-place delete/writeback + // below mutates the COPY, and the end-of-run swap publishes it atomically. + // Clear any stale temp first (a crashed run), then copy the (consolidated, + // single-file) live index. Whole-file copy — hence opt-in. + await wipeLbugDbFiles(buildPath); + await fs.copyFile(lbugPath, buildPath); + } } else { // Full rebuild path: wipe DB files first. // Set the dirty flag BEFORE the wipe whenever a prior meta exists, @@ -1380,7 +1440,12 @@ export async function runFullAnalysis( // valve below can never drift. Failures now throw a typed LbugWipeError // (ENOENT-verified removal) instead of silently letting initLbug reopen // a still-populated DB this run believes it wiped. - await wipeLbugDbFiles(lbugPath); + // + // With the atomic swap (POSIX), this wipes the TEMP build target + // (`buildPath` = `.new`, clearing any stragglers from a crashed + // run) and leaves the live index untouched until the end-of-run swap. On + // Windows buildPath === lbugPath, so this is the original in-place wipe. + await wipeLbugDbFiles(buildPath); } // Size the buffer pool to the graph just built by the pipeline (a page cache @@ -1393,7 +1458,9 @@ export async function runFullAnalysis( estimateBufferPool(pipelineResult.graph.nodeCount + pipelineResult.graph.relationshipCount), ); - await initLbug(lbugPath); + // Full rebuild (POSIX) builds into the temp `buildPath`; incremental and + // Windows use `buildPath === lbugPath` in place. + await initLbug(buildPath); // Manual WAL checkpoint driver (#1741): periodically drain the WAL // from JS so the un-retriable native auto-checkpoint almost never @@ -1655,8 +1722,8 @@ export async function runFullAnalysis( // to replace wholesale. await walCheckpointDriver.stop(); await closeLbug(); - await wipeLbugDbFiles(lbugPath); - await initLbug(lbugPath); + await wipeLbugDbFiles(buildPath); + await initLbug(buildPath); walCheckpointDriver = startWalCheckpointDriver(); await loadGraphToLbug(pipelineResult.graph, pipelineResult.repoPath, storagePath, (msg) => { lbugMsgCount++; @@ -2257,7 +2324,11 @@ export async function runFullAnalysis( // inside the resolver, and a mismatch leaves the dirty flag intact so the // next run takes the established full-recovery path. meta.runnerIdentity = finalizeAnalyzerRunnerIdentity(import.meta.url, runnerIdentity); - await saveMeta(metaDir, meta); + // #2614 F1: the freshness stamp (saveMeta) is written AFTER the atomic swap + // below — never here — so a concurrent MCP reader can't observe + // meta.indexedAt = T_new while lbugPath still resolves to the pre-swap + // inode (which latched the reader on the stale index permanently). The meta + // object is fully computed at this point; only its write is deferred. // Persist the incremental parse cache for the next run. Wraps in // try/catch so a cache-write failure never breaks an otherwise @@ -2395,7 +2466,59 @@ export async function runFullAnalysis( // 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()); + // + // On Windows a swap must release the build handle before the rename (a + // same-process open file can't be renamed), so it forces a real close — + // safe because windowsSwapOk excludes --pdg (the #2264 case). POSIX renames + // an open file, so it keeps the skip-native-close there. + const forceRealCloseForSwap = useAtomicSwap && process.platform === 'win32'; + await (options.skipNativeCloseOnExit && !forceRealCloseForSwap + ? closeLbugBeforeExit() + : closeLbug()); + + // #2 atomic publish: the fresh index was built at buildPath (a full rebuild, + // or an opt-in atomic incremental that copied the live index in first). Swap + // it over the live lbugPath in one rename so an MCP reader that opened + // mid-build only ever saw the previous complete index — never a wiped/ + // half-built file. The close above checkpoint-consolidated buildPath to a + // single file (no .wal), so the rename publishes a complete index; a reader + // holding the old inode keeps a consistent stale snapshot until the pool + // re-opens onto the new one (the pool staleness invalidation). Runs only on + // success — a thrown error skips this, leaving the live index intact and the + // temp build to be cleared by the next run's wipe. + // Only publish if the build actually produced a DB at buildPath. A + // degenerate run (empty repo, or a mocked pipeline that never opened the + // store) leaves nothing to swap — skip rather than throw ENOENT. + const builtDbExists = useAtomicSwap + ? await fs.stat(buildPath).then( + () => true, + () => false, + ) + : false; + if (useAtomicSwap && builtDbExists) { + await retryRename(buildPath, lbugPath); + // Clear any sidecars orphaned beside the replaced file. A cleanly-closed + // prior index has none; a crashed one could, and it would be replay + // poison next to the freshly published index. Best-effort. + for (const suffix of ['.wal', '.shadow', '.wal.checkpoint'] as const) { + await fs.rm(`${lbugPath}${suffix}`, { force: true }).catch(() => {}); + } + // #2614 F4: if the final checkpoint silently failed, the build may still + // carry a residual .wal/.shadow under the temp name. MOVE it beside the + // published index (not orphan/delete it) so the next open replays the + // delta, rather than leaving it under a name LadybugDB never reconciles. + for (const suffix of ['.wal', '.shadow'] as const) { + await fs.rename(`${buildPath}${suffix}`, `${lbugPath}${suffix}`).catch(() => {}); + } + } + + // #2614 F1: stamp the freshness metadata now that the index is published. + // When meta.indexedAt becomes visible, lbugPath already resolves to the new + // inode, so a reader reiniting on the stamp opens the fresh graph rather + // than latching on the old one. Leaving the dirty flag set across the swap + // is a crash-safety improvement: a failed swap leaves the previous index + // live and the next run recovers via the full-rebuild path. + await saveMeta(metaDir, meta); progress('done', 100, 'Done'); diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 5b104dd32..2ed048fb9 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -15,6 +15,8 @@ import { executeParameterized, closeLbug, isLbugReady, + statDbIdentity, + dbIdentityChanged, } from '../../core/lbug/pool-adapter.js'; import { queryClassBeanMetadata } from './bean-metadata.js'; import { isValidQueryParams } from '../../core/lbug/query-params.js'; @@ -725,6 +727,12 @@ export class LocalBackend { // not persist across calls and the staleness check would reinit forever // (#2106). private lastObservedIndexedAt: Map = new Map(); + // #2614 F1: file identity of the lbug the pool last opened. An atomic swap or + // an in-place incremental changes the inode; reiniting on that reinit-covers + // the window where meta.indexedAt hasn't caught up (and the incremental case), + // so a rebuilt index is never served stale even when the stamp looks current. + private lastObservedDbIdentity: Map>> = + new Map(); private groupToolSvc: GroupService | null = null; /** * One-shot stderr warnings for sibling-clone drift, keyed by @@ -1060,6 +1068,7 @@ export class LocalBackend { this.initializedRepos.delete(key); this.lastStalenessCheck.delete(key); this.lastObservedIndexedAt.delete(key); + this.lastObservedDbIdentity.delete(key); this.reinitPromises.delete(key); closeLbug(key).catch(() => {}); } @@ -1463,22 +1472,40 @@ export class LocalBackend { // Reading the flat meta for a branch handle would compare the branch // index's indexedAt against the primary's and thrash the pool (#2106). const meta = await loadMeta(path.dirname(repo.lbugPath)); - if (!meta) return; // Compare against the last indexedAt OBSERVED for this pool (keyed by // lbugPath), not the handle's — branch handles are fresh spreads so a // handle mutation would not persist and would reinit on every check. const observed = this.lastObservedIndexedAt.get(poolKey) ?? repo.indexedAt; - if (meta.indexedAt && meta.indexedAt !== observed) { - // Index was rebuilt — close stale connection and re-init. - // Wrap in reinitPromises to prevent TOCTOU race where concurrent - // callers both detect staleness and double-close the pool. + const stampChanged = !!meta?.indexedAt && meta.indexedAt !== observed; + // #2614 F1: also reinit on a file-identity change. An atomic swap (or an + // in-place incremental) changes the lbug inode; keying only on + // meta.indexedAt let a reader that reinited inside the pre-swap window + // latch on the old inode forever (its stamp already == meta.indexedAt). + const currentIdentity = await statDbIdentity(repo.lbugPath); + const identityChanged = dbIdentityChanged( + this.lastObservedDbIdentity.get(poolKey) ?? null, + currentIdentity, + ); + if (stampChanged || identityChanged) { + // Index was rebuilt/swapped — DELEGATE the close/reopen to the pool's + // initLbug, which refuses to evict (and close the shared Database) + // while a query is in flight (its checkedOut>0 guard). Calling + // closeLbug directly here bypassed that guard and could close a + // Database mid-query — a native use-after-free (#2614). Wrap in + // reinitPromises to serialize concurrent detectors. const reinit = (async () => { try { - await closeLbug(poolKey); - this.initializedRepos.delete(poolKey); - this.lastObservedIndexedAt.set(poolKey, meta.indexedAt); - await initLbug(poolKey, repo.lbugPath); - this.initializedRepos.add(poolKey); + // Advance the observed stamp regardless: a stamp change with an + // unchanged file must not re-trigger on every check. + if (meta?.indexedAt) this.lastObservedIndexedAt.set(poolKey, meta.indexedAt); + const reopened = await initLbug(poolKey, repo.lbugPath); + // Advance the observed IDENTITY only when the pool actually rolled + // over. If a query was in flight, initLbug served the current + // handle and returned false; leaving the identity divergent + // re-triggers the reopen on a later idle check instead of latching. + if (reopened) { + this.lastObservedDbIdentity.set(poolKey, await statDbIdentity(repo.lbugPath)); + } } finally { this.reinitPromises.delete(poolKey); } @@ -1497,6 +1524,7 @@ export class LocalBackend { await initLbug(poolKey, repo.lbugPath); this.initializedRepos.add(poolKey); this.lastObservedIndexedAt.set(poolKey, repo.indexedAt); + this.lastObservedDbIdentity.set(poolKey, await statDbIdentity(repo.lbugPath)); } catch (err: any) { // If lock error, mark as not initialized so next call retries this.initializedRepos.delete(poolKey); diff --git a/gitnexus/test/integration/analyze-atomic-swap.test.ts b/gitnexus/test/integration/analyze-atomic-swap.test.ts new file mode 100644 index 000000000..871acc428 --- /dev/null +++ b/gitnexus/test/integration/analyze-atomic-swap.test.ts @@ -0,0 +1,220 @@ +/** + * Integration test for the #2 atomic full-rebuild swap. + * + * A full rebuild builds the fresh index at `.new` and swaps it over + * the live index in one atomic rename (POSIX). Two invariants: + * - success publishes a single valid `lbug` with no `.new` temp left behind, + * and a repeat rebuild replaces the inode (proving the swap, not an in-place + * edit); and + * - a failure BEFORE the swap leaves the previous index byte-for-byte intact + * (the crash-safety win — the live index is never wiped mid-rebuild). + * + * POSIX only: on Windows the build stays in place (buildPath === lbugPath), so + * these swap invariants do not apply — see run-analyze's platform guard. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { execSync } from 'child_process'; +import { promises as fs } from 'node:fs'; +import path from 'node:path'; + +type LbugAdapter = typeof import('../../src/core/lbug/lbug-adapter.js'); +const ctx = vi.hoisted(() => ({ + loadMock: vi.fn(), + realLoad: null as LbugAdapter['loadGraphToLbug'] | null, +})); +// Delegating mock: overrides only loadGraphToLbug so a rebuild can be made to +// fail on demand (mirrors run-analyze-adopt-failure.test.ts). +vi.mock('../../src/core/lbug/lbug-adapter.js', async (importOriginal) => { + const actual = await importOriginal(); + ctx.realLoad = actual.loadGraphToLbug; + ctx.loadMock.mockImplementation(actual.loadGraphToLbug); + return { ...actual, loadGraphToLbug: ctx.loadMock }; +}); + +import { runFullAnalysis } from '../../src/core/run-analyze.js'; +import { getStoragePaths } from '../../src/storage/repo-manager.js'; +import { + initLbug as poolInit, + executeQuery as poolQuery, + closeLbug as poolClose, +} from '../../src/core/lbug/pool-adapter.js'; +import { createTempDir } from '../helpers/test-db.js'; + +const isWin = process.platform === 'win32'; + +const identity = async (p: string): Promise => { + const s = await fs.stat(p); + return `${s.ino}:${s.mtimeMs}:${s.size}`; +}; +const lingeringTemp = async (lbugPath: string): Promise => { + const base = path.basename(lbugPath); + const entries = await fs.readdir(path.dirname(lbugPath)); + return entries.filter((e) => e.startsWith(`${base}.new`)); +}; + +describe.skipIf(isWin)('atomic full-rebuild swap (#2)', () => { + let tmpHome: Awaited>; + let savedHome: string | undefined; + + beforeEach(async () => { + tmpHome = await createTempDir('gn-atomic-swap-home-'); + savedHome = process.env.GITNEXUS_HOME; + process.env.GITNEXUS_HOME = tmpHome.dbPath; + ctx.loadMock.mockReset(); + ctx.loadMock.mockImplementation((...a: Parameters) => + ctx.realLoad!(...a), + ); + }); + + afterEach(async () => { + if (savedHome === undefined) delete process.env.GITNEXUS_HOME; + else process.env.GITNEXUS_HOME = savedHome; + await tmpHome.cleanup(); + }); + + const makeRepo = async () => { + const tmp = await createTempDir('gn-atomic-swap-repo-'); + const repo = tmp.dbPath; + execSync('git init', { cwd: repo, stdio: 'pipe' }); + await fs.writeFile( + path.join(repo, 'a.ts'), + 'export function greet(n: string) { return `hi ${n}`; }\nexport function caller() { return greet("x"); }\n', + ); + execSync('git add -A && git -c user.name=t -c user.email=t@t commit -m init', { + cwd: repo, + stdio: 'pipe', + }); + return { repo, cleanup: tmp.cleanup }; + }; + + it('publishes one lbug with no temp leak; a repeat rebuild swaps the inode', async () => { + const { repo, cleanup } = await makeRepo(); + try { + await runFullAnalysis(repo, {}, { onProgress: () => {} }); + const { lbugPath } = getStoragePaths(repo); + await expect(fs.stat(lbugPath)).resolves.toBeTruthy(); + expect(await lingeringTemp(lbugPath)).toEqual([]); + const first = await identity(lbugPath); + + await runFullAnalysis(repo, { force: true }, { onProgress: () => {} }); + expect(await lingeringTemp(lbugPath)).toEqual([]); + // The atomic rename replaced the file — a new inode, not an in-place edit. + expect(await identity(lbugPath)).not.toBe(first); + } finally { + await cleanup(); + } + }, 180_000); + + it('leaves the previous index intact when a rebuild fails before the swap', async () => { + const { repo, cleanup } = await makeRepo(); + try { + await runFullAnalysis(repo, {}, { onProgress: () => {} }); // v1 + const { lbugPath } = getStoragePaths(repo); + const before = await identity(lbugPath); + + ctx.loadMock.mockRejectedValueOnce(new Error('injected mid-rebuild failure')); + await expect( + runFullAnalysis(repo, { force: true }, { onProgress: () => {} }), + ).rejects.toThrow('injected mid-rebuild failure'); + + // The build failed in the temp; the swap (skipped on failure) never + // published it, so the live index is byte-for-byte untouched. + expect(await identity(lbugPath)).toBe(before); + } finally { + await cleanup(); + } + }, 180_000); + + it('the read pool serves the freshly-swapped index after a rebuild (#1 + #2 end-to-end)', async () => { + const { repo, cleanup } = await makeRepo(); + const repoId = 'atomic-swap-e2e'; + const names = async (): Promise => + (await poolQuery(repoId, 'MATCH (f:Function) RETURN f.name AS n')).flatMap((r) => + Object.values(r as Record).map(String), + ); + try { + await runFullAnalysis(repo, {}, { onProgress: () => {} }); // v1: greet + const { lbugPath } = getStoragePaths(repo); + + await poolInit(repoId, lbugPath); + expect(await names()).toContain('greet'); + + // Rebuild with a renamed function so v1 and v2 differ observably. + await fs.writeFile( + path.join(repo, 'a.ts'), + 'export function renamedGreet(n: string) { return `hi ${n}`; }\nexport function caller() { return renamedGreet("x"); }\n', + ); + execSync('git -c user.name=t -c user.email=t@t commit -am rename', { + cwd: repo, + stdio: 'pipe', + }); + await runFullAnalysis(repo, { force: true }, { onProgress: () => {} }); // v2 → atomic swap + + // Same repoId: initLbug detects the swapped inode and re-opens the pool + // onto the new index instead of serving the stale (unlinked) one. + await poolInit(repoId, lbugPath); + const v2 = await names(); + expect(v2).toContain('renamedGreet'); + // Proves the pool actually re-opened — a stale handle would still see v1. + expect(v2).not.toContain('greet'); + } finally { + await poolClose(repoId); + await cleanup(); + } + }, 180_000); + + it('opt-in atomic incremental copies then swaps, no temp leak, change reflected', async () => { + const { repo, cleanup } = await makeRepo(); + const prev = process.env.GITNEXUS_ATOMIC_INCREMENTAL; + process.env.GITNEXUS_ATOMIC_INCREMENTAL = '1'; + const repoId = 'atomic-incr-e2e'; + try { + await runFullAnalysis(repo, {}, { onProgress: () => {} }); // v1 + const { lbugPath } = getStoragePaths(repo); + + // Change a single file so the next run is incremental, adding a function. + await fs.writeFile( + path.join(repo, 'a.ts'), + 'export function greet(n: string) { return `hi ${n}`; }\nexport function caller() { return greet("x"); }\nexport function addedFn() { return 1; }\n', + ); + execSync('git -c user.name=t -c user.email=t@t commit -am change', { + cwd: repo, + stdio: 'pipe', + }); + + await runFullAnalysis(repo, {}, { onProgress: () => {} }); // incremental + atomic swap + expect(await lingeringTemp(lbugPath)).toEqual([]); + + await poolInit(repoId, lbugPath); + const names = (await poolQuery(repoId, 'MATCH (f:Function) RETURN f.name AS n')).flatMap( + (r) => Object.values(r as Record).map(String), + ); + expect(names).toContain('addedFn'); // the incremental change landed via the swap + } finally { + if (prev === undefined) delete process.env.GITNEXUS_ATOMIC_INCREMENTAL; + else process.env.GITNEXUS_ATOMIC_INCREMENTAL = prev; + await poolClose(repoId); + await cleanup(); + } + }, 180_000); + + it('publishes cleanly on the production close path (skipNativeCloseOnExit) (#2614 F5)', async () => { + const { repo, cleanup } = await makeRepo(); + try { + // The CLI and serve-worker set skipNativeCloseOnExit (dodges #2264), so the + // build handle is still open at swap time — the path production actually + // ships, distinct from the default real-close the other tests exercise. + // Prove the POSIX swap still publishes a single consolidated file with no + // .new temp and no orphan sidecar. + await runFullAnalysis(repo, { skipNativeCloseOnExit: true }, { onProgress: () => {} }); + const { lbugPath } = getStoragePaths(repo); + await expect(fs.stat(lbugPath)).resolves.toBeTruthy(); + expect(await lingeringTemp(lbugPath)).toEqual([]); + for (const s of ['.wal', '.shadow', '.wal.checkpoint'] as const) { + await expect(fs.stat(`${lbugPath}${s}`)).rejects.toThrow(); // no orphan sidecar + } + } finally { + await cleanup(); + } + }, 180_000); +}); diff --git a/gitnexus/test/integration/analyze-wal-checkpoint-failure.test.ts b/gitnexus/test/integration/analyze-wal-checkpoint-failure.test.ts index 1be20fd54..1bae0bb4e 100644 --- a/gitnexus/test/integration/analyze-wal-checkpoint-failure.test.ts +++ b/gitnexus/test/integration/analyze-wal-checkpoint-failure.test.ts @@ -82,9 +82,15 @@ describe('analyze WAL auto-checkpoint rename failure (real lbug, no mocks)', () // a `GITNEXUS_WAL_CHECKPOINT_THRESHOLD=1` setting forces. const storageDir = path.join(repoPath, '.gitnexus'); fs.mkdirSync(storageDir, { recursive: true }); - const blockerDir = path.join(storageDir, 'lbug.wal.checkpoint'); - fs.mkdirSync(blockerDir, { recursive: true }); - fs.writeFileSync(path.join(blockerDir, 'blocker'), 'cannot-be-renamed-over'); + // A full rebuild now builds into `lbug.new` and swaps atomically (POSIX), so + // its auto-checkpoint targets `lbug.new.wal.checkpoint`; on the in-place / + // Windows path it targets `lbug.wal.checkpoint`. Block BOTH so the planted + // rename blocker trips the first checkpoint whichever path analyze takes. + for (const name of ['lbug.wal.checkpoint', 'lbug.new.wal.checkpoint']) { + const blockerDir = path.join(storageDir, name); + fs.mkdirSync(blockerDir, { recursive: true }); + fs.writeFileSync(path.join(blockerDir, 'blocker'), 'cannot-be-renamed-over'); + } const result = spawnSync(process.execPath, [...CLI_SPAWN_PREFIX, 'analyze', '--skip-skills'], { cwd: repoPath, diff --git a/gitnexus/test/unit/checkpoint-busy-2599.test.ts b/gitnexus/test/unit/checkpoint-busy-2599.test.ts new file mode 100644 index 000000000..a64ee815e --- /dev/null +++ b/gitnexus/test/unit/checkpoint-busy-2599.test.ts @@ -0,0 +1,34 @@ +/** + * #2599: a WAL-checkpoint IO error that also carries a busy/lock signal means + * another handle holds the store open (a `gitnexus mcp` server, or this + * process's own reader) — not a disk fault. `isLbugCheckpointBusyError` + * classifies it; the CLI (analyze.ts) names that held-open cause alongside the + * existing --wal-checkpoint-threshold recovery hint, leaving the original IO + * error intact. + */ +import { describe, it, expect } from 'vitest'; +import { isLbugCheckpointBusyError } from '../../src/core/lbug/lbug-config.js'; + +const IO_BUSY = + 'runtime exception: io exception: error renaming file /x/lbug.wal to /x/lbug.wal.checkpoint: could not set lock on file'; +const IO_DISK = + 'runtime exception: io exception: error removing directory or file /x/lbug.wal.checkpoint: disk full'; + +describe('#2599 checkpoint-busy classification', () => { + it('classifies a checkpoint IO error carrying a lock signal as busy', () => { + expect(isLbugCheckpointBusyError(new Error(IO_BUSY))).toBe(true); + }); + + it('does not classify a plain checkpoint IO error (disk fault) as busy', () => { + expect(isLbugCheckpointBusyError(new Error(IO_DISK))).toBe(false); + }); + + it('does not classify a non-checkpoint lock error as checkpoint-busy', () => { + expect(isLbugCheckpointBusyError(new Error('could not set lock on file /x/lbug'))).toBe(false); + }); + + it('ignores nullish input', () => { + expect(isLbugCheckpointBusyError(undefined)).toBe(false); + expect(isLbugCheckpointBusyError(null)).toBe(false); + }); +}); diff --git a/gitnexus/test/unit/pool-freshness-invalidation.test.ts b/gitnexus/test/unit/pool-freshness-invalidation.test.ts new file mode 100644 index 000000000..41a375a86 --- /dev/null +++ b/gitnexus/test/unit/pool-freshness-invalidation.test.ts @@ -0,0 +1,91 @@ +/** + * Unit tests for the read-pool staleness identity mechanism (pool invalidation). + * + * When `analyze` rebuilds or mutates the on-disk index under a live MCP read + * pool, `initLbug` must detect the change and re-open onto the new file instead + * of serving the stale (POSIX: unlinked-but-open) inode. Detection rests on the + * filesystem identity `{ino, mtimeMs, size}` diverging. These tests pin that the + * identity actually diverges on the two real rebuild shapes — a whole-file + * replace (new inode) and an in-place grow (size change) — and that a stat + * failure is treated as "unchanged" so a reader keeps its valid open inode + * through the brief unlink window of a full rebuild. + * + * The end-to-end initLbug reopen (native DB open on a swapped file) is covered + * by the reader-during-rebuild integration test. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { promises as fs } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +// Defensive: keep native search/embedding adapters from loading at import time. +vi.mock('../../src/core/search/bm25-index.js', () => ({ + searchFTSFromLbug: vi.fn().mockResolvedValue({ results: [], ftsAvailable: true }), +})); +vi.mock('../../src/mcp/core/embedder.js', () => ({ + embedQuery: vi.fn().mockResolvedValue([]), + getEmbeddingDims: vi.fn().mockReturnValue(384), +})); + +import { statDbIdentity, dbIdentityChanged } from '../../src/core/lbug/pool-adapter.js'; + +describe('pool freshness identity (pool invalidation)', () => { + let dir: string; + let dbPath: string; + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-pool-fresh-')); + dbPath = path.join(dir, 'lbug'); + await fs.writeFile(dbPath, 'v1-index-bytes', 'utf-8'); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + await fs.rm(dir, { recursive: true, force: true }); + }); + + it('reports an unchanged file as not changed (reader reuses the pool)', async () => { + const a = await statDbIdentity(dbPath); + const b = await statDbIdentity(dbPath); + expect(a).not.toBeNull(); + expect(dbIdentityChanged(a, b)).toBe(false); + }); + + it('detects a whole-file replace — the full-rebuild / atomic-swap shape', async () => { + const before = await statDbIdentity(dbPath); + // unlink + recreate at the same path = new inode (what a full rebuild's + // unlink+recreate, or a temp-build atomic rename-over, produces). + await fs.rm(dbPath); + await fs.writeFile(dbPath, 'v2-rebuilt-index-bytes-different-length', 'utf-8'); + const after = await statDbIdentity(dbPath); + expect(dbIdentityChanged(before, after)).toBe(true); + }); + + it('detects an in-place grow — the incremental writeback shape', async () => { + const before = await statDbIdentity(dbPath); + await fs.appendFile(dbPath, '-more-rows-appended', 'utf-8'); // same inode, larger size + const after = await statDbIdentity(dbPath); + expect(dbIdentityChanged(before, after)).toBe(true); + }); + + it('treats a missing file as unchanged (keep the still-valid open inode)', async () => { + const before = await statDbIdentity(dbPath); + await fs.rm(dbPath); // brief unlink window of a full rebuild + const missing = await statDbIdentity(dbPath); + expect(missing).toBeNull(); + // Not "changed": the reader keeps serving its open inode until the NEW file + // appears with a different identity — avoids churning into a failed reopen. + expect(dbIdentityChanged(before, missing)).toBe(false); + }); + + it('compares each identity field (pure decision)', () => { + const base = { ino: 10, mtimeMs: 1000, size: 500 }; + expect(dbIdentityChanged(base, { ...base })).toBe(false); + expect(dbIdentityChanged(base, { ...base, ino: 11 })).toBe(true); + expect(dbIdentityChanged(base, { ...base, mtimeMs: 1001 })).toBe(true); + expect(dbIdentityChanged(base, { ...base, size: 501 })).toBe(true); + // Unknown identity on either side is never "changed". + expect(dbIdentityChanged(null, base)).toBe(false); + expect(dbIdentityChanged(base, null)).toBe(false); + }); +});