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