From 1e764cd475045df2e981084f41f4876825c5210e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Sat, 25 Jul 2026 05:08:13 +0100 Subject: [PATCH] fix(analyze): single-writer lock for the index write path (#2658) (#2677) --- gitnexus/scripts/cross-platform-tests.ts | 15 + gitnexus/src/cli/analyze.ts | 37 +- gitnexus/src/cli/cli-message.ts | 3 +- gitnexus/src/core/run-analyze.ts | 254 ++++-- gitnexus/src/core/search/fts-indexes.ts | 81 +- gitnexus/src/server/analyze-worker-core.ts | 11 +- gitnexus/src/server/analyze-worker.ts | 10 + gitnexus/src/storage/index-lock.ts | 722 ++++++++++++++++++ gitnexus/test/fixtures/index-lock-child.mjs | 58 ++ .../integration/analyze-atomic-swap.test.ts | 12 +- .../analyze-index-lock-concurrency.test.ts | 173 +++++ .../analyze-wal-checkpoint-failure.test.ts | 92 ++- .../test/unit/analyze-worker-core.test.ts | 32 + gitnexus/test/unit/index-lock.test.ts | 381 +++++++++ .../test/unit/run-analyze-fts-repair.test.ts | 197 +++++ 15 files changed, 1977 insertions(+), 101 deletions(-) create mode 100644 gitnexus/src/storage/index-lock.ts create mode 100644 gitnexus/test/fixtures/index-lock-child.mjs create mode 100644 gitnexus/test/integration/analyze-index-lock-concurrency.test.ts create mode 100644 gitnexus/test/unit/index-lock.test.ts diff --git a/gitnexus/scripts/cross-platform-tests.ts b/gitnexus/scripts/cross-platform-tests.ts index 0ccd6beb8..e88096cc5 100644 --- a/gitnexus/scripts/cross-platform-tests.ts +++ b/gitnexus/scripts/cross-platform-tests.ts @@ -79,6 +79,13 @@ const PLATFORM_LOGIC = [ // POSIX and Windows — the fail-closed path-claim semantics must hold on the // real windows-latest path implementation (#2419/#2420). 'test/unit/server-api-repo-resolution.test.ts', + // The index write-lock (#2658) selects its backend by process.platform — the + // OS socket lock (Windows named pipe / Linux abstract socket) vs the file + // fallback — and its socket-backend describe block is gated to linux/win32. + // The Ubuntu suite only proves the Linux abstract-socket path, so run it here + // to exercise the Windows named-pipe backend and the macOS file fallback on + // their real platforms (#2658 review H3). + 'test/unit/index-lock.test.ts', ]; // Native LadybugDB integration tests — exercise the @ladybugdb/core @@ -147,6 +154,14 @@ const SPAWN_CLI = [ 'test/integration/antigravity-hook-e2e.test.ts', 'test/unit/local-cli-subprocess.test.ts', 'test/unit/runner-exec-tail.test.ts', + // Real cross-process single-writer lock coordination (#2658): child processes + // contend for the lock and race to reclaim a dead holder. Process spawning, + // kernel socket auto-release (Win named pipe / Linux abstract socket), and the + // FILE-backend rename-steal reclaim (macOS/BSD default) all vary across OSes — + // the exact behaviors the Windows/macOS matrix must prove. macOS timing first + // exposed a file-backend double-admit race here (#2658 review); the reclaim is + // now judgment-verified so a live holder is never displaced. + 'test/integration/analyze-index-lock-concurrency.test.ts', ]; // Worker threads tests — exercise real worker_threads which have diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 3d4505b6e..d4b42cf74 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -34,6 +34,7 @@ import { type AnalyzerRunnerIdentity, } from '../storage/repo-manager.js'; import { getGitRoot, hasGitDir, getDefaultBranch } from '../storage/git.js'; +import { IndexLockTimeoutError } from '../storage/index-lock.js'; import { loadAnalyzeConfig, mergeAnalyzeOptions, @@ -1553,11 +1554,21 @@ const analyzeCommandImpl = async ( // progress-bar log() that fired mid-run has already scrolled away, so the // degraded-search state must also appear in the final summary (#1161). if (result.ftsSkipped) { - console.log( - `\n Warning: full-text/BM25 search is disabled — the LadybugDB FTS extension was unavailable.\n` + - ` Install it once with network access (GITNEXUS_LBUG_EXTENSION_INSTALL=auto) then rerun, or\n` + - ` run \`gitnexus analyze --repair-fts\` when connected. Run \`gitnexus doctor\` for details.`, - ); + // #2658 review L2: a build/verify failure is NOT an extension-unavailable + // problem — sending the user to install the extension is the wrong remedy. + if (result.ftsSkipReason === 'build-failed') { + console.log( + `\n Warning: full-text/BM25 search is disabled — the search index build failed this run.\n` + + ` The FTS extension is available; rerun \`gitnexus analyze --repair-fts\`. If it persists,\n` + + ` check the disk for space or corruption. Run \`gitnexus doctor\` for details.`, + ); + } else { + console.log( + `\n Warning: full-text/BM25 search is disabled — the LadybugDB FTS extension was unavailable.\n` + + ` Install it once with network access (GITNEXUS_LBUG_EXTENSION_INSTALL=auto) then rerun, or\n` + + ` run \`gitnexus analyze --repair-fts\` when connected. Run \`gitnexus doctor\` for details.`, + ); + } } try { @@ -1594,6 +1605,22 @@ const analyzeCommandImpl = async ( return; } + // Another analyze held the index lock past the configured wait ceiling + // (#2658, GITNEXUS_INDEX_LOCK_TIMEOUT_MS). The on-disk index is being + // refreshed by the holder — this is a clean, expected condition, not a + // crash, so render the message without a stack trace. + if (err instanceof IndexLockTimeoutError) { + cliError( + ` Another gitnexus analyze (pid ${err.holder.pid} on ${err.holder.hostname}) is ` + + `already refreshing this index and did not finish within the wait window.\n` + + ` The on-disk index is being updated by that run. Retry later, or raise\n` + + ` GITNEXUS_INDEX_LOCK_TIMEOUT_MS to wait longer.\n`, + { recoveryHint: 'index-lock-timeout', holderPid: err.holder.pid }, + ); + process.exitCode = 1; + return; + } + // Finalize invariant failure (#1169) — keep the rich actionable // message intact and write through realStderrWrite so it can't be // erased by a leftover bar refresh on slow terminals. diff --git a/gitnexus/src/cli/cli-message.ts b/gitnexus/src/cli/cli-message.ts index 9d27b6686..61b65dd00 100644 --- a/gitnexus/src/cli/cli-message.ts +++ b/gitnexus/src/cli/cli-message.ts @@ -59,7 +59,8 @@ export type RecoveryHint = | 'npm-resolution' | 'module-not-found' | 'gitnexusrc-invalid' - | 'default-branch-invalid'; + | 'default-branch-invalid' + | 'index-lock-timeout'; /** * Common shape for the optional structured-field bag passed to diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index b404866a5..cf535cf9f 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -11,7 +11,9 @@ import path from 'path'; import fs from 'fs/promises'; +import { randomUUID } from 'node:crypto'; import { retryRename } from '../storage/fs-atomic.js'; +import { acquireIndexLock } from '../storage/index-lock.js'; import { runPipelineFromRepo } from './ingestion/pipeline.js'; import type { KnowledgeGraph } from './graph/types.js'; import { resetDegradedParseCounter } from './tree-sitter/safe-parse.js'; @@ -40,6 +42,7 @@ import { estimateBufferPool, setBufferPoolSizeHint } from './lbug/lbug-config.js import { escapeCypherString } from './lbug/cypher-escape.js'; import { buildSearchIndexesOrDegrade, + ftsFailureIsFatal, createSearchFTSIndexes, dropSearchFTSIndexes, initialiseSearchFTSStemmer, @@ -358,6 +361,15 @@ export interface AnalyzeResult { * the persisted meta surface the degraded state instead of reporting healthy. */ ftsSkipped?: boolean; + /** + * Why FTS was skipped, when `ftsSkipped` is true (#2658 review L2): + * `extension-unavailable` (the LadybugDB FTS extension could not load — the + * offline-first case, remedied by installing it) vs `build-failed` (the + * extension loaded but the index build/verify failed non-fatally — remedied by + * `--repair-fts`, not by installing the extension). Lets the CLI show the + * correct recovery hint instead of always blaming a missing extension. + */ + ftsSkipReason?: 'extension-unavailable' | 'build-failed'; /** * True when the index this run produced/validated is the flat workspace * slot (#2106 R2, inverted by #2354 to follow the checked-out branch). @@ -625,34 +637,175 @@ export const pdgModeMismatch = (recorded: RepoMeta['pdg'], options: PdgOptions): return false; }; +/** + * The storage paths + resolved branch placement a run will write to. Computed + * once, up front, so the `runFullAnalysis` wrapper can lock the ACTUAL write + * directory (#2658). `metaDir` — not `getStoragePaths(repoPath, options.branch)` + * — is the lock scope: a `--branch X` that owns the flat slot resolves to the + * flat `.gitnexus`, so scoping off the raw option would lock the wrong dir. + */ +interface WriteTarget { + storagePath: string; + repoHasGit: boolean; + currentCommit: string; + checkedOutBranch: string | null; + branchLabel: string | null; + placement: { branch?: string }; + lbugPath: string; + metaPath: string; + metaDir: string; +} + +/** + * Resolve which storage slot this analyze writes to, including branch + * placement (#2106/#2354). Extracted from the top of the pipeline so the lock + * scope (`metaDir`) is known before the lock is acquired. Throws the same + * `--branch` / checked-out mismatch error the pipeline used to throw inline, so + * that failure still surfaces before any lock is taken. + */ +async function resolveWriteTarget(repoPath: string, options: AnalyzeOptions): Promise { + // `storagePath` is ALWAYS the flat `.gitnexus` — content-addressed caches + // (parse-cache, parsedfile-store) and kuzu-migration cleanup live there and + // are shared across branches (#2106 KTD7). + const { storagePath } = getStoragePaths(repoPath); + const repoHasGit = hasGitDir(repoPath); + const currentCommit = repoHasGit ? getCurrentCommit(repoPath) : ''; + // Normalize the auto-detected branch the same way an explicit `--branch` is + // validated (#2106 R1): a git ref the branch-name rules forbid becomes `null` + // → the flat slot, matching that a later `--branch ` query would + // also be rejected. A normal ref round-trips index-time/query-time labels. + const checkedOutBranch = repoHasGit + ? (sanitizeDetectedBranch(getCurrentBranch(repoPath)) ?? null) + : null; + // Analyze indexes the working tree, not an arbitrary ref. An explicit + // `--branch X` while a DIFFERENT branch Y is checked out would write Y's + // content into X's slot, corrupting X (#2106). Refuse the mismatch. Detached + // HEAD / non-git (checkedOutBranch === null) still allow an explicit label. + if (options.branch && checkedOutBranch && options.branch !== checkedOutBranch) { + throw new Error( + `--branch "${options.branch}" does not match the checked-out branch "${checkedOutBranch}". ` + + `Check out "${options.branch}" before indexing it, or omit --branch to index the current branch.`, + ); + } + const branchLabel = options.branch ?? checkedOutBranch; + const placement = options.branch ? await resolveBranchPlacement(repoPath, branchLabel) : {}; + const { lbugPath, metaPath } = getStoragePaths(repoPath, placement.branch); + return { + storagePath, + repoHasGit, + currentCommit, + checkedOutBranch, + branchLabel, + placement, + lbugPath, + metaPath, + metaDir: path.dirname(metaPath), + }; +} + +/** + * Run the full analysis under an exclusive, index-directory-scoped write lock + * (#2658). A second concurrent `analyze` on the same slot waits here for the + * first to finish, then falls through to the normal freshness check inside — + * so a run whose work the holder already did returns `alreadyUpToDate` in + * seconds instead of rebuilding (single-flight coalescing), while a run for a + * genuinely-changed tree does one follow-up incremental. No new flag: waiting + * is the default, which is what hook-driven re-index wants. + * + * The lock is held by whichever process runs the pipeline (the heap-respawn + * child, or the original) — see index-lock.ts for why ownership lives with the + * writer, not a supervising parent. Released as soon as the write completes or + * throws; the post-analysis steps in the CLI (skills, registry) run lock-free. + */ export async function runFullAnalysis( repoPath: string, options: AnalyzeOptions, callbacks: AnalyzeCallbacks, runnerIdentityAtBootstrap?: AnalyzerRunnerIdentity, +): Promise { + // Validate operator-provided FTS config before anything else — a typo fails + // here in ms, without taking the lock. (createSearchFTSIndexes reuses the + // cached value via getSearchFTSStemmer.) + initialiseSearchFTSStemmer(); + initialiseSearchFTSCjkSegmentation(); + // Scope the degraded-parse log throttle to this run (module-level counter + // would otherwise stay saturated on a reused process). + resetDegradedParseCounter(); + + const log = (msg: string) => callbacks.onLog?.(msg); + const acquireOpts = { + log, + onWaitStart: () => + callbacks.onProgress('lock', 0, 'Waiting for another analyze to finish on this index…'), + }; + + let writeTarget = await resolveWriteTarget(repoPath, options); + let lock = await acquireIndexLock(writeTarget.metaDir, acquireOpts); + try { + // #2658 review H2: acquireIndexLock can wait up to the timeout ceiling, + // during which git HEAD/branch — and thus the resolved write slot — may + // change (a commit lands, a branch is switched, or another writer adopts the + // flat slot). The pre-wait snapshot must NOT be reused: re-resolve UNDER the + // lock so the freshness check (`existingMeta.lastCommit === currentCommit`) + // and the meta stamps see current git state, honoring the module's "re-check + // freshness after acquiring" contract. If the slot itself moved we hold the + // WRONG lock — release and re-acquire the correct one. Bounded so a + // pathologically churning checkout can't loop forever; after the cap we + // proceed on the current lock. The loop is INSIDE the try so a re-resolve + // that throws (e.g. a `--branch` that stopped matching the now-switched + // checkout) still releases the held lock via `finally` (no leak). + const MAX_RELOCK = 3; + for (let attempt = 0; attempt < MAX_RELOCK; attempt++) { + const fresh = await resolveWriteTarget(repoPath, options); + if (fresh.metaDir === writeTarget.metaDir) { + writeTarget = fresh; // same slot — adopt the freshly-read commit/branch/placement + break; + } + log( + `Index write target moved while waiting for the lock ` + + `(${writeTarget.metaDir} → ${fresh.metaDir}); re-acquiring the correct slot.`, + ); + lock.release(); + writeTarget = fresh; + lock = await acquireIndexLock(fresh.metaDir, acquireOpts); + if (attempt === MAX_RELOCK - 1) { + log('Index write target still moving after repeated re-acquire; proceeding on this lock.'); + } + } + return await runFullAnalysisInner( + repoPath, + options, + callbacks, + writeTarget, + runnerIdentityAtBootstrap, + ); + } finally { + lock.release(); + } +} + +async function runFullAnalysisInner( + repoPath: string, + options: AnalyzeOptions, + callbacks: AnalyzeCallbacks, + writeTarget: WriteTarget, + runnerIdentityAtBootstrap?: AnalyzerRunnerIdentity, ): Promise { const log = (msg: string) => callbacks.onLog?.(msg); const progress = (phase: string, percent: number, message: string) => callbacks.onProgress(phase, percent, message); - // Resolve + validate operator-provided FTS config once, before the expensive - // parse/load phases. A typo fails here in ms; createSearchFTSIndexes reuses - // the cached value via getSearchFTSStemmer. - initialiseSearchFTSStemmer(); - initialiseSearchFTSCjkSegmentation(); + // FTS-config validation and the degraded-parse counter reset happen in the + // `runFullAnalysis` wrapper (before the lock is taken). - // Scope the degraded-parse log throttle to this run. On a reused process - // (e.g. tests, or any host that calls runFullAnalysis more than once) the - // module-level counter would otherwise stay saturated and suppress every - // degraded-parse log after the first run. The per-parse worker holds its own - // counter in its own module instance and is process-scoped, so no separate - // worker-side reset is needed (see safe-parse.ts ParseTimeoutError contract). - resetDegradedParseCounter(); - - // `storagePath` is ALWAYS the flat `.gitnexus` — content-addressed caches - // (parse-cache, parsedfile-store) and the kuzu-migration cleanup live there - // and are shared across branches (#2106 KTD7). - const { storagePath } = getStoragePaths(repoPath); + // Write target (storage paths + resolved branch placement) was computed by + // the `runFullAnalysis` wrapper — which needs `metaDir` up front to acquire + // the exclusive index lock BEFORE any of the freshness/write work below + // (#2658). `storagePath` is ALWAYS the flat `.gitnexus`; `placement.branch` + // selects a `branches//` sub-slot only for an explicit `--branch` that + // does not own the flat slot. See resolveWriteTarget for the full contract. + const { storagePath, repoHasGit, currentCommit, branchLabel, placement, lbugPath, metaDir } = + writeTarget; // Start each analyze with a clean buffer-pool hint: any pre-pipeline DB open // (e.g. the embeddings-cache open) falls back to the default until the hint is @@ -665,44 +818,6 @@ export async function runFullAnalysis( log('Migrating from KuzuDB to LadybugDB — rebuilding index...'); } - const repoHasGit = hasGitDir(repoPath); - const currentCommit = repoHasGit ? getCurrentCommit(repoPath) : ''; - - // ── #2106/#2354: resolve which branch slot this run writes to ───────── - // `branchLabel` is the branch identity recorded in meta.json (incl. the - // flat workspace slot). `placement.branch` is undefined for the flat slot - // (the lbug/meta paths stay byte-identical to single-branch behavior) and - // set for a `branches//` sub-directory. Only an explicit `--branch` - // can route to a sub-directory; a plain analyze ALWAYS targets the flat - // slot, which follows the checked-out working tree (#2354) — the - // auto-detected branch (null for detached HEAD / non-git) is recorded as - // the slot's informational label only. - // Normalize the auto-detected branch the same way an explicit `--branch` is - // validated (#2106 R1): a git ref the branch-name rules forbid (backtick, - // `~ ^ : ? *`, leading `-`, `..`) becomes `null` → the flat slot, matching - // that a later `--branch ` query would also be rejected. A normal - // ref passes through unchanged so index-time and query-time labels round-trip. - const checkedOutBranch = repoHasGit - ? (sanitizeDetectedBranch(getCurrentBranch(repoPath)) ?? null) - : null; - // Analyze indexes the working tree, not an arbitrary ref. An explicit - // `--branch X` while a DIFFERENT branch Y is checked out would write Y's - // content (and Y's commit) into X's index slot, corrupting X (#2106). Refuse - // the mismatch. Detached HEAD / non-git (checkedOutBranch === null) still - // allow an explicit label so CI checkouts can name their snapshot. - if (options.branch && checkedOutBranch && options.branch !== checkedOutBranch) { - throw new Error( - `--branch "${options.branch}" does not match the checked-out branch "${checkedOutBranch}". ` + - `Check out "${options.branch}" before indexing it, or omit --branch to index the current branch.`, - ); - } - const branchLabel = options.branch ?? checkedOutBranch; - const placement = options.branch ? await resolveBranchPlacement(repoPath, branchLabel) : {}; - const { lbugPath, metaPath } = getStoragePaths(repoPath, placement.branch); - // metaPath now points to the metadata file (gitnexus.json) in a branch-specific directory. - // metaDir is the directory containing the metadata file (and branch-specific DBs). - const metaDir = path.dirname(metaPath); - // Keep gitnexus.json and the legacy meta.json mirror in sync (fresher // indexedAt wins; nothing is deleted). Best-effort: loadMeta has its own // legacy fallback, so a reconciliation failure (read-only mount, full disk) @@ -1380,7 +1495,12 @@ export async function runFullAnalysis( log('atomic-incremental: live index carries orphan sidecars — using in-place writeback'); } const useAtomicSwap = (isFullRebuild || atomicIncremental) && (posixSwap || windowsSwapOk); - const buildPath = useAtomicSwap ? `${lbugPath}.new` : lbugPath; + // #2658: a per-run staging name (was the fixed `lbug.new`). Even under the + // single-writer lock, a unique name means a crashed run's half-built staging + // file can never be mistaken for — or clobber — a live run's; the lock's + // orphan sweep (sweepStagingArtifacts) reclaims stragglers on the next + // acquire. The `.staging.` prefix is what that sweep matches. + const buildPath = useAtomicSwap ? `${lbugPath}.staging.${randomUUID()}` : lbugPath; if (isIncremental && hashDiff) { log( @@ -1906,6 +2026,11 @@ export async function runFullAnalysis( // build/verify step itself fails, so capabilities.fts.status / ftsSkipped // stay honest even though that failure no longer aborts the whole analyze. let ftsReady = ftsAvailable; + // Why FTS ended up skipped (#2658 review L2): extension-unavailable up front, + // or build-failed in the degrade branch below. + let ftsSkipReason: 'extension-unavailable' | 'build-failed' | undefined = ftsAvailable + ? undefined + : 'extension-unavailable'; if (ftsAvailable) { // Degrade rather than throw: createSearchFTSIndexes re-tokenizes every // stored row on every run, so a native tokenizer error on a single @@ -1921,8 +2046,24 @@ export async function runFullAnalysis( }); if (ftsResult.ok) { progress('fts', 90, 'Search indexes ready'); + } else if (ftsFailureIsFatal(ftsResult.failureClass, useAtomicSwap)) { + // #2658: an IO/rename/checkpoint/corruption failure while building FTS + // is a genuinely broken build on this disk — not a concurrent writer + // (the single-writer lock rules that out). ONLY fatal on the atomic-swap + // path: the graph was built into a throwaway staging DB, so throwing + // before the swap abandons the staging file and leaves the previous live + // index intact. On an in-place build the live DB is already mutated and + // cannot be rolled back by throwing (see ftsFailureIsFatal) — those + // degrade in the branch below instead. + throw new Error( + `Search index build failed with an integrity error and the analysis was aborted ` + + `to avoid publishing a broken index: ${ftsResult.error}. The previous index is ` + + `left intact. Re-run \`gitnexus analyze\`; if it persists, check the disk for space ` + + `or corruption.`, + ); } else { ftsReady = false; + ftsSkipReason = 'build-failed'; log( `FTS index build failed (${ftsResult.error}) — keyword search degraded this run. ` + 'Graph and embeddings analysis completed successfully. Run `gitnexus analyze --repair-fts` to retry.', @@ -2573,6 +2714,7 @@ export async function runFullAnalysis( stats: meta.stats, pipelineResult, ftsSkipped: !ftsReady, + ftsSkipReason: ftsReady ? undefined : ftsSkipReason, isPrimaryBranch: !placement.branch, }; } catch (err) { diff --git a/gitnexus/src/core/search/fts-indexes.ts b/gitnexus/src/core/search/fts-indexes.ts index 1207861b9..ab098a68c 100644 --- a/gitnexus/src/core/search/fts-indexes.ts +++ b/gitnexus/src/core/search/fts-indexes.ts @@ -193,9 +193,81 @@ export async function verifySearchFTSIndexes( return missing; } +/** + * Why an FTS build failed, so the caller can react correctly (#2658): + * + * - `capability`: the environment can't support FTS this run, or a single + * pre-existing row can't be tokenized (#2544/#2546 "Invalid UTF-8"). The + * graph/embeddings work is sound — degrade keyword search and keep exit 0. + * - `integrity`: an IO / rename / checkpoint / corruption failure while + * writing the index. With the single-writer lock (#2658) this is no longer + * "some other analyze racing us" — it's a genuinely broken build on this + * disk, so the run must fail loudly rather than publish a clean-looking + * index whose search silently never worked. + */ +export type FtsBuildFailureClass = 'capability' | 'integrity'; + +// Checked before integrity signatures: a row-level tokenizer error that happens +// to mention an integrity word still degrades (it isn't a broken build). +const FTS_CAPABILITY_SIGNATURES = ['invalid utf-8', 'failed calling lower', 'tokeniz'] as const; +// IO / durability / corruption signatures that mean the build itself broke. +// Deliberately SPECIFIC (#2658 review L1): generic OS errors a capability/config +// failure can also carry — bare 'no such file or directory' (ENOENT, e.g. a +// missing FTS extension asset) and 'bad file descriptor'/'ebadf' — are NOT here, +// so an ambiguous failure degrades (the pre-#2658 safe behavior) instead of +// newly aborting the whole analyze. A genuine write/rename/checkpoint integrity +// failure still matches via 'error renaming' / 'io exception' / 'checkpoint' +// (the #2658 repro message "Error renaming … : No such file or directory" hits +// both 'io exception' and 'error renaming'). +const FTS_INTEGRITY_SIGNATURES = [ + 'io exception', + 'i/o error', + 'io error', + 'error renaming', + 'checkpoint', + 'corrupt', + 'no space', + 'enospc', + 'double free', + 'segmentation', +] as const; + +/** + * Classify an FTS build failure message. Defaults to `capability` (degrade) — + * only clearly-integrity failures escalate, so the long-standing resilience to + * row-level tokenizer errors is preserved and we never newly fail a run on an + * unrecognised message. + */ +export const classifyFtsBuildError = (message: string): FtsBuildFailureClass => { + const m = message.toLowerCase(); + if (FTS_CAPABILITY_SIGNATURES.some((s) => m.includes(s))) return 'capability'; + if (FTS_INTEGRITY_SIGNATURES.some((s) => m.includes(s))) return 'integrity'; + return 'capability'; +}; + +/** + * Whether an FTS build failure should ABORT the analyze (throw before publish) + * rather than degrade to a search-less-but-queryable index (#2658). + * + * Only an `integrity` failure on the atomic-swap path is fatal: there the graph + * was built into a throwaway staging DB, so throwing abandons the staging file + * and leaves the previous live index intact. On an in-place build + * (`useAtomicSwap === false`: incremental, Windows default) the graph DML + * already mutated the LIVE database, so there is nothing to roll back by + * throwing — degrading to a queryable index with FTS marked unavailable is + * strictly better than exiting mid-finalization over a dirty, partially-indexed + * live DB. `capability` failures always degrade. + */ +export const ftsFailureIsFatal = ( + failureClass: FtsBuildFailureClass | undefined, + useAtomicSwap: boolean, +): boolean => failureClass === 'integrity' && useAtomicSwap; + export interface BuildSearchIndexesResult { ok: boolean; error?: string; + /** Present only when `ok` is false. See {@link FtsBuildFailureClass}. */ + failureClass?: FtsBuildFailureClass; } /** @@ -216,10 +288,15 @@ export async function buildSearchIndexesOrDegrade( await createSearchFTSIndexes(options); const missing = await verifySearchFTSIndexes(executeQuery); if (missing.length > 0) { - return { ok: false, error: `missing indexes after build: ${missing.join(', ')}` }; + // Structural incompleteness with no thrown error — treat as capability + // (degrade), matching prior behavior; a broken *write* surfaces as a + // thrown IO/checkpoint error below and is classified integrity there. + const error = `missing indexes after build: ${missing.join(', ')}`; + return { ok: false, error, failureClass: classifyFtsBuildError(error) }; } return { ok: true }; } catch (e) { - return { ok: false, error: e instanceof Error ? e.message : String(e) }; + const error = e instanceof Error ? e.message : String(e); + return { ok: false, error, failureClass: classifyFtsBuildError(error) }; } } diff --git a/gitnexus/src/server/analyze-worker-core.ts b/gitnexus/src/server/analyze-worker-core.ts index 5c35d69a3..547b29dbd 100644 --- a/gitnexus/src/server/analyze-worker-core.ts +++ b/gitnexus/src/server/analyze-worker-core.ts @@ -16,6 +16,9 @@ import type { AnalyzeOptions } from '../core/run-analyze.js'; import type { WorkerMessage } from './analyze-worker.js'; import type { AnalyzerRunnerIdentity } from '../storage/repo-manager.js'; import { projectAnalyzeResultForIpc } from './analyze-worker-ipc.js'; +// Value import (instanceof): index-lock is a lightweight storage primitive +// (node:fs/net/crypto only), so this does NOT pull in run-analyze/repo-manager. +import { IndexLockTimeoutError } from '../storage/index-lock.js'; export interface WorkerAnalysisDeps { runFullAnalysis: typeof import('../core/run-analyze.js').runFullAnalysis; @@ -74,7 +77,13 @@ export async function runWorkerAnalysis( } catch (err: unknown) { // Report the failure to the parent over IPC (the parent surfaces the message). const message = err instanceof Error ? err.message : 'Analysis failed'; - terminal = { type: 'error', message }; + // #2658 review M2: a lock-wait timeout is transient contention (another + // analyze held the single-writer lock), not a broken build — tag it so the + // parent can surface a retry signal instead of an opaque hard failure. + terminal = + err instanceof IndexLockTimeoutError + ? { type: 'error', message, code: 'index-lock-timeout', retryable: true } + : { type: 'error', message }; } // P3 (#2264): only report if a SIGTERM cancellation hasn't already claimed the diff --git a/gitnexus/src/server/analyze-worker.ts b/gitnexus/src/server/analyze-worker.ts index 37ae1713b..08b32b9d6 100644 --- a/gitnexus/src/server/analyze-worker.ts +++ b/gitnexus/src/server/analyze-worker.ts @@ -40,6 +40,16 @@ export interface CompleteMessage { export interface ErrorMessage { type: 'error'; message: string; + /** + * Machine-readable failure code for a parent that wants to branch instead of + * only surfacing the string. `index-lock-timeout` (#2658 review M2) means + * another analyze held the single-writer lock past the wait ceiling — a + * transient, retryable condition, not a broken build. Absent for a generic + * failure. + */ + code?: 'index-lock-timeout'; + /** True when the failure is expected to clear on retry (e.g. lock contention). */ + retryable?: boolean; } /** Child → parent IPC messages. Shared with the parent-side launcher. */ diff --git a/gitnexus/src/storage/index-lock.ts b/gitnexus/src/storage/index-lock.ts new file mode 100644 index 000000000..c67750266 --- /dev/null +++ b/gitnexus/src/storage/index-lock.ts @@ -0,0 +1,722 @@ +/** + * Cross-process single-writer lock for a GitNexus index directory (#2658). + * + * `analyze` is the only writer of a `.gitnexus/` (or `branches//`) slot, + * but nothing stopped two `analyze` runs — e.g. two editor/agent SessionStart + * hooks firing on the same repo at once — from wiping and rebuilding the same + * store concurrently. They raced on `lbug` and its sidecars, wasted N× CPU + * producing one index, and left orphaned WAL fragments (#2637). This module + * gives the write path an exclusive, index-directory-scoped lock so a second + * writer waits for the first instead of colliding; after acquiring, the caller + * re-runs its normal freshness check, so a run whose work the holder already + * did exits up-to-date rather than rebuilding (single-flight coalescing). + * + * Ownership lives with the process that runs the pipeline (the heap-respawn + * child when a respawn happens, the original otherwise) — NOT a supervising + * parent — so the entity the OS tracks for liveness is always the real writer. + * See run-analyze.ts for the acquire site. + * + * TWO BACKENDS behind the {@link acquireIndexLock} seam: + * + * - **socket** (Windows named pipe / Linux abstract socket, via `net`) — the + * preferred, KERNEL-OWNED lock. Holding it = holding a listening endpoint the + * kernel binds to this process; `EADDRINUSE` therefore means a *live* holder, + * and the kernel drops the binding the instant the holder exits for ANY reason + * (clean exit, crash, OOM, SIGKILL). That makes it provably race-free: no + * stale detection, no pid-reuse guess, no takeover, and — since the endpoint + * lives outside the index dir — no filesystem write, so it works unchanged on + * a read-only index mount. This is the same class of kernel object as the + * Windows named mutex the issue's reporter used as an external workaround, but + * built from Node's stdlib `net`, so it adds NO native dependency and cannot + * break `npx gitnexus` install anywhere. + * + * - **file** (`O_EXCL` pidfile) — the portable fallback for macOS/BSD (no + * abstract sockets; filesystem sockets don't release cleanly on death) and + * for any environment where the socket backend can't bind. It uses pid- + * liveness staleness, an atomic rename-steal reclaim, bounded malformed-file + * handling, read-only tolerance, and a finite wait timeout (a reused pid can + * masquerade as live where process start-time isn't verifiable, so waiting is + * bounded rather than a hang). Its stale-takeover has an irreducible narrow + * race — inherent to file-based advisory locks — which is precisely why the + * socket backend is preferred; only a kernel primitive closes it. + * + * Scope: cross-process, same logical index dir. The file backend never steals a + * foreign-host lock (pid liveness is meaningless across hosts); the socket + * backend is single-host by nature. The motivating case (local hook-driven + * re-index) is single-host. See AcquireOptions.timeoutMs for the wait ceiling. + */ +import { + openSync, + writeSync, + closeSync, + readFileSync, + unlinkSync, + renameSync, + existsSync, + mkdirSync, + readdirSync, + realpathSync, +} from 'node:fs'; +import net from 'node:net'; +import path from 'node:path'; +import os from 'node:os'; +import { randomBytes, randomUUID, createHash } from 'node:crypto'; + +const LOCK_FILENAME = 'analyze.lock'; +const LOCK_RECORD_VERSION = 1 as const; + +/** Base poll interval while waiting for a live holder; jittered per attempt. */ +const DEFAULT_POLL_MS = 250; +/** How often to re-emit the "still waiting for pid N" diagnostic. */ +const DIAGNOSTIC_INTERVAL_MS = 15_000; +/** + * Default wait ceiling (10 min). Generous enough to sit behind a normal + * analyze, finite so a pid-reuse ghost on a platform without start-time + * verification can't wedge acquisition forever (see AcquireOptions.timeoutMs). + * A repo whose analyze legitimately runs longer can raise + * GITNEXUS_INDEX_LOCK_TIMEOUT_MS (or set it ≤ 0 for unbounded). + */ +const DEFAULT_TIMEOUT_MS = 600_000; +/** + * How long a lock file must stay unreadable (empty/partial JSON) before we + * treat it as a crash orphan and reclaim it. Tolerates the microsecond + * create→write→close window of a *live* owner (see acquireIndexLock), so we + * never steal a lock that is a poll-interval away from being written. Scaled + * off the poll interval, floored at 1s. + */ +const malformedGraceMs = (pollMs: number): number => Math.max(1000, pollMs * 2); + +/** + * On-disk lock record. `token` proves ownership on release/steal; `startTime` + * (Linux only) defends against pid reuse; `invocationId` is a human-traceable + * id distinct from the security-irrelevant `token`. + */ +export interface LockRecord { + v: typeof LOCK_RECORD_VERSION; + pid: number; + hostname: string; + /** /proc//stat starttime (clock ticks) on Linux; null where unavailable. */ + startTime: string | null; + token: string; + invocationId: string; + acquiredAt: string; +} + +export interface IndexLockHandle { + /** Our own record — `invocationId` is shown to waiters as the holder id. */ + readonly record: LockRecord; + /** Idempotent; only removes the lock file if it still carries our token. */ + release(): void; +} + +export interface AcquireOptions { + log?: (msg: string) => void; + /** + * Give up waiting after this long (ms), throwing {@link IndexLockTimeoutError}. + * Default: {@link DEFAULT_TIMEOUT_MS} ({@link resolveTimeoutMs}). A finite + * default is deliberate: on platforms without process start-time verification + * (anything but Linux — see {@link readProcStartTime}) a crashed holder whose + * pid was reused by an unrelated long-lived process reads as a live holder and + * would otherwise block acquisition forever. Timing out is safe — it stops + * *waiting*, never *steals* a possibly-live holder — and names the holder so + * the caller can retry. Override (including to unbounded, value ≤ 0) via + * GITNEXUS_INDEX_LOCK_TIMEOUT_MS. + */ + timeoutMs?: number; + /** Base poll interval (ms); jittered. Default 250. */ + pollMs?: number; + /** Called once when we start waiting on a live holder. */ + onWaitStart?: (holder: LockRecord) => void; +} + +export class IndexLockTimeoutError extends Error { + readonly holder: LockRecord; + /** + * Whether `holder` carries a real, identifiable owner. False on the socket + * backend (and the file backend's malformed/vanished-lock timeouts), where the + * holder is a placeholder (`pid -1`) — the OS socket lock exposes no owner + * metadata (#2658 review M3). Consumers must not present `holder.pid` as a real + * pid when this is false. + */ + readonly holderKnown: boolean; + constructor(holder: LockRecord, waitedMs: number, holderKnown = true) { + super( + holderKnown + ? `Timed out after ${waitedMs}ms waiting for another gitnexus analyze ` + + `(pid ${holder.pid} on ${holder.hostname}, invocation ${holder.invocationId}) ` + + `to release the index lock.` + : `Timed out after ${waitedMs}ms waiting for another gitnexus analyze ` + + `(holder identity unknown) to release the index lock.`, + ); + this.name = 'IndexLockTimeoutError'; + this.holder = holder; + this.holderKnown = holderKnown; + } +} + +const HOSTNAME = os.hostname(); + +/** Linux: field 22 of /proc//stat (starttime). null elsewhere / on error. */ +const readProcStartTime = (pid: number): string | null => { + if (process.platform !== 'linux') return null; + try { + const stat = readFileSync(`/proc/${pid}/stat`, 'utf8'); + // comm (field 2) is parenthesized and may contain spaces/')' — split after + // the last ')' so the remaining fields align to their documented numbers. + const afterComm = stat + .slice(stat.lastIndexOf(') ') + 2) + .trim() + .split(' '); + // afterComm[0] is field 3 (state); starttime is field 22 → index 19. + return afterComm[19] ?? null; + } catch { + return null; + } +}; + +/** true if the pid exists (signal 0). EPERM means it exists but isn't ours. */ +const pidAlive = (pid: number): boolean => { + try { + process.kill(pid, 0); + return true; + } catch (err) { + return (err as NodeJS.ErrnoException).code === 'EPERM'; + } +}; + +const buildRecord = (): LockRecord => ({ + v: LOCK_RECORD_VERSION, + pid: process.pid, + hostname: HOSTNAME, + startTime: readProcStartTime(process.pid), + token: randomBytes(16).toString('hex'), + invocationId: randomUUID(), + acquiredAt: new Date().toISOString(), +}); + +const readRecord = (lockPath: string): LockRecord | null => { + try { + const raw = readFileSync(lockPath, 'utf8'); + const parsed = JSON.parse(raw) as Partial; + // `typeof NaN === 'number'`, so a bare number check lets NaN/0/-1/Infinity/ + // fractional pids reach process.kill (#2658 review L4): a garbled or crafted + // lock file with `{"pid":0}` reads as a live holder and wedges a real analyze + // for the full wait timeout. A real pid is a positive integer. + if (!Number.isInteger(parsed.pid) || (parsed.pid as number) <= 0) return null; + if (typeof parsed.token !== 'string') return null; + return parsed as LockRecord; + } catch { + // Missing (won the race, file gone) or malformed/half-written → treat as + // "no readable holder"; the caller retries the O_EXCL create. + return null; + } +}; + +/** + * A same-host holder is stale iff its process is gone, or (Linux) its pid is + * alive but was reused — a different start time. A live holder is never stolen + * on age alone (a large repo legitimately analyzes for many minutes), and a + * foreign-host holder is never stale (its liveness is unknowable here). Where + * start-time verification is unavailable (non-Linux), a reused pid cannot be + * distinguished from a genuine live holder, so it is NOT stolen — the finite + * acquire timeout is what bounds that case instead (see AcquireOptions). + */ +const isStale = (holder: LockRecord): boolean => { + if (holder.hostname !== HOSTNAME) return false; + if (!pidAlive(holder.pid)) return true; + const now = readProcStartTime(holder.pid); + if (holder.startTime && now && holder.startTime !== now) return true; // pid reused + return false; +}; + +/** + * Reclaim a lock file we judged reclaimable — a dead holder (`expected` = its + * record) or a malformed/unreadable crash-orphan (`expected` = null) — moving + * the exact inode aside in ONE `rename` syscall to a token-unique name so two + * waiters reclaiming the same orphan can't both win (the loser's rename ENOENTs). + * + * CRITICAL (#2658 review): the reclaim must not act on a STALE judgment. The + * staleness decision (`isStale` / malformed-grace) happened a few syscalls ago; + * a live writer may have O_EXCL-created its own lock at `lockPath` since. Blindly + * renaming that live lock aside would delete it and admit a SECOND writer — the + * exact double-writer this lock exists to prevent (reproduced: ~18%/round under + * 4-way reclaim contention on the file backend). So: + * 1. re-read `lockPath` immediately BEFORE the rename and confirm it still holds + * exactly what we judged (same token, or still-unreadable) — shrinking the + * window to the single gap between this read and the rename; + * 2. after the rename, confirm what we ACTUALLY moved matches the judgment; if a + * live lock slipped into that residual gap, RESTORE it (rename back) so its + * holder is never displaced, and lose the reclaim. + * A concurrent creator whose fresh lock the restore overwrites is caught by the + * acquire loop's post-write read-back verify (see acquireViaFile), so it backs + * off rather than proceeding as a second writer. + * + * Returns true if we won the reclaim (caller retries the create), false if we + * lost the race or the judgment went stale (caller re-loops and re-reads). + */ +const matchesJudgment = (record: LockRecord | null, expected: LockRecord | null): boolean => + expected === null ? record === null : record?.token === expected.token; + +const stealLock = (lockPath: string, me: LockRecord, expected: LockRecord | null): boolean => { + // (1) Re-verify the judgment still holds right before we move anything. + if (!matchesJudgment(readRecord(lockPath), expected)) return false; + if (expected === null && !existsSync(lockPath)) return false; // malformed → but now vanished + + const aside = `${lockPath}.dead.${me.token}`; + try { + renameSync(lockPath, aside); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return false; // another stealer won + throw err; + } + // (2) Confirm what we moved is what we judged; if a live lock slipped into the + // read→rename gap, put it back — a live holder must never be displaced. + if (!matchesJudgment(readRecord(aside), expected)) { + try { + renameSync(aside, lockPath); // restore; an overwritten concurrent creator's read-back backs it off + } catch { + /* slot re-taken between our move and restore — leave it; we lost the reclaim */ + } + return false; + } + try { + unlinkSync(aside); // uniquely ours by token → safe; best-effort + } catch { + /* leftover .dead. is inert (not analyze.lock, not swept) — harmless */ + } + return true; +}; + +/** + * Placeholder holder for an {@link IndexLockTimeoutError} thrown while the lock + * file exists but no valid record can be read (malformed/partial), or it keeps + * vanishing — there is no real holder to name, but the error still needs one so + * the CLI's `err.holder.pid` stays defined. This path is a rare backstop: + * malformed files are reclaimed within {@link MALFORMED_GRACE_MS}. + */ +const unknownHolder = (): LockRecord => ({ + v: LOCK_RECORD_VERSION, + pid: -1, + hostname: HOSTNAME, + startTime: null, + token: '', + invocationId: '', + acquiredAt: '', +}); + +/** + * Filesystem-create error codes we tolerate by proceeding lock-free: a + * read-only mount (EROFS) or a denied create (EACCES/EPERM). Such a filesystem + * rejects every index WRITE in the same directory too, so no concurrent writer + * can exist and the lock is moot — an already-indexed repo on a `:ro` mount + * must still reach its `alreadyUpToDate` fast path (#2658). A genuinely-needed + * write fails later exactly as it would have without the lock. + */ +export const LOCK_UNWRITABLE_CODES: ReadonlySet = new Set(['EROFS', 'EACCES', 'EPERM']); +export const isLockUnwritableCode = (code: string | undefined): boolean => + code !== undefined && LOCK_UNWRITABLE_CODES.has(code); + +/** A lock handle that owns nothing — returned when the filesystem refuses to + * create the lock file (see {@link LOCK_UNWRITABLE_CODES}). Release is a no-op. */ +const noopHandle = (record: LockRecord): IndexLockHandle => ({ record, release: () => {} }); + +/** + * Delete orphaned build/staging artifacts left in the lock directory by a + * crashed prior writer. Safe precisely because we hold the exclusive lock: no + * other writer can be creating these here right now, so anything present is a + * crash orphan. Matches this slot's staging files ONLY — never `lbug` itself, + * never `lbug.wal`/`lbug.shadow` (the LIVE index's own sidecars), and never a + * `branches//` sub-slot (which owns its own lock + sweep). Non-recursive. + */ +export const sweepStagingArtifacts = (lockDir: string, log?: (msg: string) => void): void => { + // Matches `lbug.new`, `lbug.new.wal`, `lbug.staging.`, `lbug.staging..wal`, … + // Does NOT match `lbug`, `lbug.wal`, `lbug.shadow`. + const stagingRe = /^lbug\.(staging\..+|new(\..+)?)$/; + let removed = 0; + let entries: string[]; + try { + entries = readdirSync(lockDir); + } catch { + return; + } + for (const name of entries) { + if (!stagingRe.test(name)) continue; + try { + unlinkSync(path.join(lockDir, name)); + removed++; + } catch { + /* best-effort */ + } + } + if (removed > 0) { + log?.(`Cleared ${removed} orphaned index-staging file(s) from a prior interrupted analyze.`); + } +}; + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +/** Poll delay with jitter (avoids two waiters lock-stepping), clamped so it + * never overshoots the remaining timeout budget. Callers guarantee + * `waited < timeoutMs`, so the result is ≥ 1. */ +const jitteredDelay = (pollMs: number, timeoutMs: number, waited: number): number => { + const jitter = Math.floor(Math.random() * pollMs); + const remaining = timeoutMs - waited; + return Math.max(1, Math.min(pollMs + jitter, remaining)); +}; + +/** + * Resolve the wait ceiling. Explicit `opt` wins; else + * GITNEXUS_INDEX_LOCK_TIMEOUT_MS; else {@link DEFAULT_TIMEOUT_MS}. A value ≤ 0 + * (from either source) means unbounded. + */ +const resolveTimeoutMs = (opt?: number): number => { + const raw = + typeof opt === 'number' + ? opt + : (() => { + const env = process.env.GITNEXUS_INDEX_LOCK_TIMEOUT_MS; + if (env === undefined || env === '') return DEFAULT_TIMEOUT_MS; + const n = Number(env); + return Number.isFinite(n) ? n : DEFAULT_TIMEOUT_MS; + })(); + return raw <= 0 ? Number.POSITIVE_INFINITY : raw; +}; + +/** + * Acquire the exclusive write lock for `lockDir` (the resolved index slot + * directory, e.g. `/.gitnexus` or `/.gitnexus/branches/`). + * + * Blocks until the lock is held (waiting only on live holders, stealing dead + * ones immediately), then sweeps orphaned staging files under the lock and + * returns a handle. Rejects with `IndexLockTimeoutError` if `timeoutMs` is + * exceeded while a live holder still holds the lock. + */ +/** + * File-based (O_EXCL pidfile) backend. The portable fallback used on platforms + * without the socket backend (macOS/BSD) or when the OS socket lock is + * unavailable. Carries the pid-liveness staleness, atomic rename-steal reclaim, + * bounded malformed-file handling, and read-only tolerance. Its stale-takeover + * has an irreducible (narrow) race — see the module header — which is why the + * socket backend is preferred where available. + */ +const acquireViaFile = async ( + lockDir: string, + me: LockRecord, + opts: AcquireOptions, +): Promise => { + try { + mkdirSync(lockDir, { recursive: true }); + } catch (err) { + // Read-only / denied filesystem → proceed lock-free (see LOCK_UNWRITABLE_CODES). + if (isLockUnwritableCode((err as NodeJS.ErrnoException).code)) return noopHandle(me); + throw err; + } + const lockPath = path.join(lockDir, LOCK_FILENAME); + const pollMs = opts.pollMs ?? DEFAULT_POLL_MS; + const timeoutMs = resolveTimeoutMs(opts.timeoutMs); + const startedAt = Date.now(); + let announcedWait = false; + let lastDiagnosticAt = 0; + // When the lock file exists but has no readable record, the timestamp we + // first observed it unreadable — used to reclaim a crash-orphan after a grace. + let malformedSince: number | null = null; + + for (;;) { + try { + // O_WRONLY | O_CREAT | O_EXCL — the atomic arbiter of ownership. + const fd = openSync(lockPath, 'wx'); + try { + writeSync(fd, JSON.stringify(me)); + } finally { + closeSync(fd); + } + // Read-back verify (#2658 review L5): if this process stalled (a >graceMs + // GC pause) between the O_EXCL create of the *empty* file and the write + // above, a waiter could have reclaimed the empty file (renamed it aside) + // and O_EXCL-created its own lock at `lockPath`. Our write then landed on + // the renamed-aside inode, not `lockPath`. Confirm `lockPath` still carries + // our token before claiming ownership; if it was stolen, contend normally. + const confirmed = readRecord(lockPath); + if (!confirmed || confirmed.token !== me.token) continue; + return { + record: me, + release: () => { + const current = readRecord(lockPath); + if (current && current.token !== me.token) return; // no longer ours + try { + unlinkSync(lockPath); + } catch { + /* already gone */ + } + }, + }; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code === 'EEXIST') { + // fall through to holder inspection / wait / reclaim below + } else if (isLockUnwritableCode(code)) { + return noopHandle(me); // read-only / denied → proceed lock-free + } else { + throw err; + } + } + + const holder = readRecord(lockPath); + const waited = Date.now() - startedAt; + + if (holder) { + malformedSince = null; + if (isStale(holder)) { + opts.log?.( + `Reclaiming stale index lock from dead analyze (pid ${holder.pid}, ` + + `invocation ${holder.invocationId}).`, + ); + stealLock(lockPath, me, holder); // reclaim ONLY this dead record; live locks are never stolen + continue; + } + // Live holder → wait. + if (!announcedWait) { + announcedWait = true; + opts.onWaitStart?.(holder); + opts.log?.( + `Another gitnexus analyze (pid ${holder.pid} on ${holder.hostname}) is ` + + `refreshing this index — waiting for it to finish.`, + ); + } + if (waited >= timeoutMs) throw new IndexLockTimeoutError(holder, waited); + if (Date.now() - lastDiagnosticAt >= DIAGNOSTIC_INTERVAL_MS) { + lastDiagnosticAt = Date.now(); + if (waited >= DIAGNOSTIC_INTERVAL_MS) { + opts.log?.( + `Still waiting for analyze pid ${holder.pid} (${Math.round(waited / 1000)}s elapsed).`, + ); + } + } + await sleep(jitteredDelay(pollMs, timeoutMs, waited)); + continue; + } + + // holder === null: the lock file is either gone (vanished between the failed + // create and our read) or present-but-unreadable (a crash between the + // O_EXCL create and the record write, or a partial write). NEVER hot-loop + // here — both branches are bounded by sleep + timeout. + if (!existsSync(lockPath)) { + malformedSince = null; // genuinely vanished → the next create likely wins + if (waited >= timeoutMs) throw new IndexLockTimeoutError(unknownHolder(), waited, false); + await sleep(jitteredDelay(pollMs, timeoutMs, waited)); + continue; + } + // Malformed orphan present. Reclaim only after a grace, so a live owner's + // microsecond create→write window is never mistaken for a crash. + if (malformedSince === null) malformedSince = Date.now(); + if (Date.now() - malformedSince >= malformedGraceMs(pollMs)) { + opts.log?.('Reclaiming a malformed/partial index lock file (no readable owner record).'); + stealLock(lockPath, me, null); // reclaim ONLY while still unreadable; a live lock written since is left + malformedSince = null; + continue; + } + if (waited >= timeoutMs) throw new IndexLockTimeoutError(unknownHolder(), waited, false); + await sleep(jitteredDelay(pollMs, timeoutMs, waited)); + } +}; + +/** Signals that the OS socket backend can't be used here (e.g. abstract + * namespace disabled, sandbox, or an unexpected bind error) so the caller + * should fall back to the file backend. NOT thrown for EADDRINUSE (that is a + * live holder → wait) or timeouts (those propagate as IndexLockTimeoutError). */ +class SocketLockUnavailable extends Error { + constructor(readonly cause: NodeJS.ErrnoException) { + super(`OS socket lock unavailable: ${cause.code ?? cause.message}`); + this.name = 'SocketLockUnavailable'; + } +} + +/** + * Canonicalize a path to its real filesystem identity so lexical aliases of the + * same directory (a symlink, a bind-mount path, a Windows junction, a `\\?\` + * prefix) map to ONE name (#2658 review H1). `lockDir` (the index slot) often + * does not exist yet, so `realpathSync` the deepest existing ancestor and + * re-append the not-yet-created remainder. A path with no symlink components + * realpaths to itself, so the common (non-aliased) case is unchanged — a holder + * that used the old resolved name is never orphaned. + */ +const canonicalizeDir = (p: string): string => { + const resolved = path.resolve(p); + const tail: string[] = []; + let dir = resolved; + for (;;) { + try { + const real = realpathSync(dir); + return tail.length ? path.join(real, ...tail.reverse()) : real; + } catch { + const parent = path.dirname(dir); + if (parent === dir) return resolved; // reached the root with nothing to resolve + tail.push(path.basename(dir)); + dir = parent; + } + } +}; + +/** + * Stable OS-IPC endpoint name for an index directory. The name is derived from + * the REAL path (case-folded on Windows), so two processes targeting the same + * physical slot — even via different lexical aliases — collide, and separate + * worktrees/branches never do. The endpoint lives OUTSIDE the index directory + * (abstract namespace / pipe namespace), so the lock needs no filesystem write + * and is unaffected by a read-only index mount. + */ +const socketLockName = (lockDir: string): string => { + const resolved = canonicalizeDir(lockDir); + const key = createHash('sha256') + .update(process.platform === 'win32' ? resolved.toLowerCase() : resolved) + .digest('hex') + .slice(0, 32); + return process.platform === 'win32' + ? `\\\\.\\pipe\\gitnexus-idx-${key}` + : `\0gitnexus-idx-${key}`; // Linux abstract socket (no filesystem entry) +}; + +/** Attempt to listen; resolve to null on success or the error on failure. */ +const tryListen = (server: net.Server, name: string): Promise => + new Promise((resolve) => { + const onError = (err: NodeJS.ErrnoException): void => { + server.removeListener('listening', onListening); + resolve(err); + }; + const onListening = (): void => { + server.removeListener('error', onError); + resolve(null); + }; + server.once('error', onError); + server.once('listening', onListening); + server.listen(name); + }); + +/** + * OS-owned socket/pipe backend (Windows named pipe, Linux abstract socket). + * Holding the lock = holding a listening endpoint the kernel binds to this + * process; `EADDRINUSE` therefore means a *live* holder, and the kernel drops + * the binding the instant the holder exits (clean exit, crash, OOM, SIGKILL) — + * so there is no stale detection, no reclaim, and no takeover race. See the + * module header for why this is preferred over the file backend. + */ +const acquireViaSocket = async ( + lockDir: string, + me: LockRecord, + opts: AcquireOptions, +): Promise => { + const name = socketLockName(lockDir); + const pollMs = opts.pollMs ?? DEFAULT_POLL_MS; + const timeoutMs = resolveTimeoutMs(opts.timeoutMs); + const startedAt = Date.now(); + let announcedWait = false; + let lastDiagnosticAt = 0; + + for (;;) { + const server = net.createServer(); + // Never keep the process alive on the lock's account, and never hold an + // incoming connection (nothing should connect; drop any stray peer). + server.unref(); + server.on('connection', (sock) => sock.destroy()); + const listenErr = await tryListen(server, name); + + if (!listenErr) { + return { + record: me, + release: () => { + try { + server.close(); + } catch { + /* already closed / releasing on exit */ + } + }, + }; + } + + // This server never bound (listen failed); release its handle before the + // next poll or the fallback, so a long contended wait doesn't churn one + // unclosed net.Server per iteration (#2658 review L3). + try { + server.close(); + } catch { + /* never listened */ + } + + // Only EADDRINUSE means "held by a live holder → wait". Anything else means + // this environment can't use the socket backend → fall back to the file one. + if (listenErr.code !== 'EADDRINUSE') throw new SocketLockUnavailable(listenErr); + + if (!announcedWait) { + announcedWait = true; + opts.onWaitStart?.(me); + opts.log?.('Another gitnexus analyze is refreshing this index — waiting for it to finish.'); + } + const waited = Date.now() - startedAt; + // Socket backend exposes no owner metadata → holder identity is unknown (M3). + if (waited >= timeoutMs) throw new IndexLockTimeoutError(unknownHolder(), waited, false); + if (Date.now() - lastDiagnosticAt >= DIAGNOSTIC_INTERVAL_MS) { + lastDiagnosticAt = Date.now(); + if (waited >= DIAGNOSTIC_INTERVAL_MS) { + opts.log?.(`Still waiting for another analyze (${Math.round(waited / 1000)}s elapsed).`); + } + } + await sleep(jitteredDelay(pollMs, timeoutMs, waited)); + } +}; + +/** Platforms whose OS IPC namespace gives a clean, auto-releasing lock via + * `net`: Windows named pipes and Linux abstract sockets. Elsewhere (macOS/BSD) + * the file backend is used (no abstract namespace; filesystem sockets don't + * release cleanly on death). Override for tests via GITNEXUS_INDEX_LOCK_BACKEND + * = 'socket' | 'file'. + * + * Scope caveat: the socket backend's mutual-exclusion domain is NOT uniform. + * Windows `\\.\pipe\` names are machine-wide (all sessions); Linux abstract + * sockets are network-namespace-scoped (network_namespaces(7)). So two writers + * that share a bind-mounted index dir but sit in separate netns (e.g. two + * containers, Docker's default) do NOT collide on Linux — "single-host" is + * really "single-netns" here. That cross-netns-shared-mount case is the one + * the file backend (shared-filesystem O_EXCL) would cover; set + * GITNEXUS_INDEX_LOCK_BACKEND=file there. The motivating case (local hook- + * driven re-index) is single-netns, so the default socket backend covers it. */ +const selectBackend = (): 'socket' | 'file' => { + const override = process.env.GITNEXUS_INDEX_LOCK_BACKEND; + if (override === 'socket' || override === 'file') return override; + return process.platform === 'win32' || process.platform === 'linux' ? 'socket' : 'file'; +}; + +/** + * Acquire the exclusive write lock for `lockDir` (the resolved index slot + * directory). Uses the OS socket/pipe backend where available (Windows/Linux), + * falling back to the file backend otherwise or if the socket backend is + * unusable in this environment. After acquiring, sweeps orphaned staging files + * under the lock (best-effort; a no-op on a read-only mount). Rejects with + * `IndexLockTimeoutError` if `timeoutMs` elapses while another live holder holds + * the lock. + */ +export const acquireIndexLock = async ( + lockDir: string, + opts: AcquireOptions = {}, +): Promise => { + const me = buildRecord(); + let handle: IndexLockHandle; + if (selectBackend() === 'socket') { + try { + handle = await acquireViaSocket(lockDir, me, opts); + } catch (err) { + if (!(err instanceof SocketLockUnavailable)) throw err; // timeout etc. propagate + opts.log?.('Index lock: OS socket lock unavailable here — using the file lock.'); + handle = await acquireViaFile(lockDir, me, opts); + } + } else { + handle = await acquireViaFile(lockDir, me, opts); + } + // Reclaim crashed-build staging orphans while we hold the lock. Best-effort: + // a read-only mount (no orphans reachable) just no-ops. + try { + sweepStagingArtifacts(lockDir, opts.log); + } catch { + /* best-effort */ + } + return handle; +}; diff --git a/gitnexus/test/fixtures/index-lock-child.mjs b/gitnexus/test/fixtures/index-lock-child.mjs new file mode 100644 index 000000000..9e64cca46 --- /dev/null +++ b/gitnexus/test/fixtures/index-lock-child.mjs @@ -0,0 +1,58 @@ +/** + * Child process for the cross-process index-lock tests (#2658), using the BUILT + * module (LOCK_MODULE). Two modes: + * + * - default (HOLD): acquire the lock on LOCK_DIR, write MARKER once held, then + * hold until killed. Proves real cross-process exclusion and SIGKILL + * kill-recovery against a parent that uses the source module. + * + * - MODE=EXCLUSIVE (SENTINEL set): acquire, then enter a critical section + * guarded by an O_EXCL sentinel create — if the sentinel already exists, + * another process holds the lock at the same time, which is the exact + * single-writer violation the test hunts. Hold briefly, remove the sentinel, + * release, exit 0. Exit 3 if the sentinel was already present (overlap). + * Used by the multi-reclaimer test where ≥2 children reclaim one dead holder. + */ +import { writeFileSync, openSync, closeSync, unlinkSync } from 'node:fs'; +import { pathToFileURL } from 'node:url'; + +// LOCK_MODULE is an absolute path. On Windows `import('C:\\…')` throws +// ERR_UNSUPPORTED_ESM_URL_SCHEME (a bare drive path is read as a URL scheme), so +// convert to a file:// URL — required on Windows, harmless on POSIX. +const { acquireIndexLock } = await import(pathToFileURL(process.env.LOCK_MODULE).href); + +if (process.env.MODE === 'EXCLUSIVE') { + const lock = await acquireIndexLock(process.env.LOCK_DIR, { timeoutMs: 30_000, pollMs: 25 }); + try { + // O_EXCL create fails if any other process is simultaneously in its own + // critical section — that is a broken single-writer invariant. + let fd; + try { + fd = openSync(process.env.SENTINEL, 'wx'); + } catch { + process.exit(3); // overlap detected: two holders at once + } + closeSync(fd); + // Hold the section briefly so concurrent reclaimers would collide here. + await new Promise((r) => setTimeout(r, 150)); + unlinkSync(process.env.SENTINEL); + } finally { + lock.release(); + } + process.exit(0); +} else { + const lock = await acquireIndexLock(process.env.LOCK_DIR, { timeoutMs: 30_000, pollMs: 25 }); + writeFileSync(process.env.MARKER, String(process.pid)); + // Hold the lock until the parent kills us. + setInterval(() => {}, 1000); + // Release on a graceful signal (the SIGKILL path in the test never reaches this). + const release = () => { + try { + lock.release(); + } finally { + process.exit(0); + } + }; + process.on('SIGTERM', release); + process.on('SIGINT', release); +} diff --git a/gitnexus/test/integration/analyze-atomic-swap.test.ts b/gitnexus/test/integration/analyze-atomic-swap.test.ts index 871acc428..8bee4aba0 100644 --- a/gitnexus/test/integration/analyze-atomic-swap.test.ts +++ b/gitnexus/test/integration/analyze-atomic-swap.test.ts @@ -1,9 +1,10 @@ /** * 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, + * A full rebuild builds the fresh index at a per-run `.staging.` + * (#2658) and swaps it over the live index in one atomic rename (POSIX). Two + * invariants: + * - success publishes a single valid `lbug` with no staging 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 @@ -49,7 +50,10 @@ const identity = async (p: string): Promise => { 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`)); + // Staging temps are the legacy fixed `${base}.new*` and the current per-run + // `${base}.staging.*` (#2658). Match both so this leftover-temp guard + // still catches a failed swap under the new naming. + return entries.filter((e) => e.startsWith(`${base}.new`) || e.startsWith(`${base}.staging.`)); }; describe.skipIf(isWin)('atomic full-rebuild swap (#2)', () => { diff --git a/gitnexus/test/integration/analyze-index-lock-concurrency.test.ts b/gitnexus/test/integration/analyze-index-lock-concurrency.test.ts new file mode 100644 index 000000000..d85744674 --- /dev/null +++ b/gitnexus/test/integration/analyze-index-lock-concurrency.test.ts @@ -0,0 +1,173 @@ +/** + * Real cross-process tests for the index write lock (#2658): child processes + * contend for the lock on the same directory as this process. + * + * - Test 1 exercises the DEFAULT backend (the OS socket/pipe lock on + * Linux/Windows): while the child holds it, our acquire blocks and times out; + * after the child is SIGKILLed the kernel drops the binding and our next + * acquire succeeds — the kernel-auto-release guarantee, no stale handling. + * - Test 2 pins the FILE backend and races several children reclaiming one dead + * holder, asserting the atomic rename-steal never lets two into the critical + * section at once. + * + * The child imports the BUILT module (dist/) and this process imports the + * source, proving the guarantee is a genuine cross-process one (and, for the + * socket backend, that both derive the same endpoint name for a given dir). + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { spawn, type ChildProcess } from 'node:child_process'; +import { mkdtempSync, rmSync, existsSync, readFileSync, writeFileSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { acquireIndexLock, IndexLockTimeoutError } from '../../src/storage/index-lock.js'; + +const testDir = path.dirname(fileURLToPath(import.meta.url)); +const repoRoot = path.resolve(testDir, '../..'); +const lockModule = path.join(repoRoot, 'dist', 'storage', 'index-lock.js'); +const childScript = path.resolve(testDir, '..', 'fixtures', 'index-lock-child.mjs'); + +let dir: string; +let marker: string; +let child: ChildProcess | undefined; + +const waitFor = async (predicate: () => boolean, timeoutMs: number): Promise => { + const start = Date.now(); + for (;;) { + if (predicate()) return; + if (Date.now() - start > timeoutMs) throw new Error('condition not met within timeout'); + await new Promise((r) => setTimeout(r, 25)); + } +}; + +const waitForExit = (proc: ChildProcess, timeoutMs: number): Promise => + new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error('child did not exit')), timeoutMs); + proc.once('exit', () => { + clearTimeout(timer); + resolve(); + }); + }); + +beforeEach(() => { + dir = mkdtempSync(path.join(os.tmpdir(), 'gnx-lock-xp-')); + marker = path.join(dir, 'held.marker'); +}); +afterEach(() => { + if (child && child.exitCode === null && child.signalCode === null) child.kill('SIGKILL'); + rmSync(dir, { recursive: true, force: true }); +}); + +describe('index lock across processes (#2658)', () => { + it('excludes a second writer while held, then recovers after the holder is killed', async () => { + if (!existsSync(lockModule)) { + throw new Error( + `dist/storage/index-lock.js missing — run \`npm run build\` first ` + + `(or use \`npm run test:integration\`, which builds via pretest:integration).`, + ); + } + + child = spawn(process.execPath, [childScript], { + env: { ...process.env, LOCK_MODULE: lockModule, LOCK_DIR: dir, MARKER: marker }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + // Generous marker wait: Windows process startup is ~5x slower and the + // platform-sensitive shard runs heavy suites in parallel, so a child spawn + // can be badly delayed under load — the wait must tolerate that, not race it. + await waitFor(() => existsSync(marker), 40_000); + const holderPid = Number(readFileSync(marker, 'utf8')); + expect(holderPid).toBeGreaterThan(0); + + // Mutual exclusion: the live holder is waited on, then we time out. + await expect(acquireIndexLock(dir, { timeoutMs: 500, pollMs: 25 })).rejects.toBeInstanceOf( + IndexLockTimeoutError, + ); + + // Kill recovery: with the holder gone, its lock becomes reclaimable. + child.kill('SIGKILL'); + await waitForExit(child, 30_000); + const lock = await acquireIndexLock(dir, { timeoutMs: 15_000, pollMs: 25 }); + expect(lock.record.pid).toBe(process.pid); + lock.release(); + }, 90_000); + + // The FILE backend is the DEFAULT only on macOS/BSD; Windows and Linux default + // to the race-free kernel lock (named pipe / abstract socket). This case FORCES + // the file backend to stress its rename-steal reclaim, so it runs where that + // backend is actually production (macOS — where the double-admit bug this + // guards lived and is now fixed) plus Linux. It is skipped on Windows, where + // the file backend is never the default; Windows' real lock (the named pipe) is + // covered by index-lock.test.ts on the Windows matrix and by the + // default-backend cross-process case above. + it.skipIf(process.platform === 'win32')( + 'lets multiple waiters reclaim one dead holder without ever admitting two writers', + async () => { + if (!existsSync(lockModule)) { + throw new Error( + `dist/storage/index-lock.js missing — run \`npm run build\` first ` + + `(or use \`npm run test:integration\`, which builds via pretest:integration).`, + ); + } + // This case targets the FILE backend's reclaim path specifically (the socket + // backend has no stale file to reclaim). Seed a stale lock owned by a dead, + // same-host holder — every child must reclaim it, and the reclaim must let + // exactly one at a time win so no two children are ever in their O_EXCL + // sentinel section together. + // + // The reclaim's rename-steal must NOT act on a stale staleness judgment: a + // waiter that judged the dead record must re-verify the file still holds it + // before renaming, or it will rename a live winner's freshly-created lock + // aside and admit a second writer (#2658 review — this reproduced at ~18% per + // round of 4-way contention before the judgment-verified steal). One round + // catches that regression only ~1-in-6 of the time, so loop several rounds to + // make it a reliable guard; with the fix every round is clean. + const sentinel = path.join(dir, 'critical.sentinel'); + const seedDeadHolder = (): void => { + writeFileSync( + path.join(dir, 'analyze.lock'), + JSON.stringify({ + v: 1, + pid: 999_999_999, + hostname: os.hostname(), + startTime: null, + token: 'dead-holder-token', + invocationId: 'dead-holder', + acquiredAt: new Date(0).toISOString(), + }), + ); + }; + + const runChild = (): Promise<{ code: number | null; signal: NodeJS.Signals | null }> => + new Promise((resolve) => { + const c = spawn(process.execPath, [childScript], { + env: { + ...process.env, + LOCK_MODULE: lockModule, + LOCK_DIR: dir, + SENTINEL: sentinel, + MODE: 'EXCLUSIVE', + GITNEXUS_INDEX_LOCK_BACKEND: 'file', + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + c.once('exit', (code, signal) => resolve({ code, signal })); + }); + + const ROUNDS = 8; + const KIDS = 5; + for (let round = 0; round < ROUNDS; round++) { + seedDeadHolder(); // the previous round's winner released (unlinked) the lock + const results = await Promise.all(Array.from({ length: KIDS }, () => runChild())); + // Every child acquired, ran its exclusive section, and exited cleanly (0). + // Exit 3 = it found the sentinel already present = two holders at once. + for (const r of results) { + expect(r.signal).toBeNull(); + expect(r.code).toBe(0); + } + // No leftover sentinel — the last holder cleaned up. + expect(existsSync(sentinel)).toBe(false); + } + }, + 60_000, + ); +}); diff --git a/gitnexus/test/integration/analyze-wal-checkpoint-failure.test.ts b/gitnexus/test/integration/analyze-wal-checkpoint-failure.test.ts index 1bae0bb4e..fdcd6ecae 100644 --- a/gitnexus/test/integration/analyze-wal-checkpoint-failure.test.ts +++ b/gitnexus/test/integration/analyze-wal-checkpoint-failure.test.ts @@ -72,45 +72,73 @@ afterAll(() => { if (suiteGitnexusHome) cleanupTempDirSync(suiteGitnexusHome); }); +const runAnalyze = () => + spawnSync(process.execPath, [...CLI_SPAWN_PREFIX, 'analyze', '--skip-skills'], { + cwd: repoPath, + encoding: 'utf8', + // Generous timeout: the test does real CSV/COPY work before the + // first failing checkpoint, and CI runners are slow. + timeout: process.env.CI ? 120_000 : 60_000, + stdio: ['pipe', 'pipe', 'pipe'], + env: { + ...process.env, + GITNEXUS_HOME: suiteGitnexusHome, + // Skip ensureHeap re-exec (which drops the tsx loader). + NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), + // Tiny threshold forces auto-checkpoint on every write so the + // first write into the WAL trips the planted rename blocker. + GITNEXUS_WAL_CHECKPOINT_THRESHOLD: '1', + CI: '1', + }, + }); + describe('analyze WAL auto-checkpoint rename failure (real lbug, no mocks)', () => { it('surfaces the --wal-checkpoint-threshold recovery hint when the rename target is blocked', () => { - // Plant a non-empty directory at the path Ladybug's auto-checkpoint - // will try to rename `.wal` over. `fs.rename` cannot overwrite a - // non-empty directory, and the adapter's orphan-sidecar cleanup uses - // `fs.unlink` (which fails on directories) — so the blocker persists - // through `doInitLbug` and trips the very first auto-checkpoint that - // a `GITNEXUS_WAL_CHECKPOINT_THRESHOLD=1` setting forces. + // The checkpoint rename target must be a PREDICTABLE path so the blocker + // can be pre-planted. A full rebuild builds into a per-run + // `lbug.staging.` and checkpoints `lbug.staging..wal.checkpoint` + // (#2658) — an unknowable name. An INCREMENTAL run instead writes the live + // index in place, so its auto-checkpoint targets the fixed + // `lbug.wal.checkpoint`. So: first do a clean full analyze to create the + // index, then plant the blocker and drive an incremental analyze into it. const storageDir = path.join(repoPath, '.gitnexus'); - fs.mkdirSync(storageDir, { recursive: true }); - // 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'], { + // 1) Clean full analyze (no blocker) — builds the index into staging and + // swaps it in. Must succeed; the staging checkpoint name is unblocked. + const first = runAnalyze(); + expect(first.status === null ? 'timeout' : first.status).toBe(0); + + // 2) Change a tracked source file and commit, so the next analyze is an + // incremental writeback (in-place), not a full rebuild. + const churnFile = path.join(repoPath, 'src', 'logger.ts'); + fs.appendFileSync(churnFile, `\nexport const walChurnMarker = ${Date.now()};\n`); + const gitEnv = { + ...process.env, + GIT_AUTHOR_NAME: 'test', + GIT_AUTHOR_EMAIL: 'test@test', + GIT_COMMITTER_NAME: 'test', + GIT_COMMITTER_EMAIL: 'test@test', + }; + spawnSync('git', ['add', '-A'], { cwd: repoPath, stdio: 'pipe' }); + spawnSync('git', ['commit', '-m', 'churn for incremental'], { cwd: repoPath, - encoding: 'utf8', - // Generous timeout: the test does real CSV/COPY work before the - // first failing checkpoint, and CI runners are slow. - timeout: process.env.CI ? 120_000 : 60_000, - stdio: ['pipe', 'pipe', 'pipe'], - env: { - ...process.env, - GITNEXUS_HOME: suiteGitnexusHome, - // Skip ensureHeap re-exec (which drops the tsx loader). - NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), - // Tiny threshold forces auto-checkpoint on every write so the - // first write into the WAL trips the planted rename blocker. - GITNEXUS_WAL_CHECKPOINT_THRESHOLD: '1', - CI: '1', - }, + stdio: 'pipe', + env: gitEnv, }); + // 3) Plant a non-empty directory at `lbug.wal.checkpoint`, the fixed rename + // target of the in-place checkpoint. `fs.rename` cannot overwrite a + // non-empty directory, and the adapter's orphan-sidecar cleanup uses + // `fs.unlink` (which fails on a directory) — so the blocker persists through + // `doInitLbug` and trips the auto-checkpoint the incremental writeback + // forces at `GITNEXUS_WAL_CHECKPOINT_THRESHOLD=1`. + const blockerDir = path.join(storageDir, 'lbug.wal.checkpoint'); + fs.rmSync(blockerDir, { recursive: true, force: true }); + fs.mkdirSync(blockerDir, { recursive: true }); + fs.writeFileSync(path.join(blockerDir, 'blocker'), 'cannot-be-renamed-over'); + + // 4) Incremental analyze into the blocked checkpoint target. + const result = runAnalyze(); const combined = `${result.stderr}\n${result.stdout}`; // The CLI must exit non-zero. status === null means the timeout fired diff --git a/gitnexus/test/unit/analyze-worker-core.test.ts b/gitnexus/test/unit/analyze-worker-core.test.ts index f63ad08da..06e3acd35 100644 --- a/gitnexus/test/unit/analyze-worker-core.test.ts +++ b/gitnexus/test/unit/analyze-worker-core.test.ts @@ -20,6 +20,7 @@ import { import type { AnalyzeResult } from '../../src/core/run-analyze.js'; import type { WorkerMessage } from '../../src/server/analyze-worker.js'; import type { AnalyzerRunnerIdentity } from '../../src/storage/repo-manager.js'; +import { IndexLockTimeoutError, type LockRecord } from '../../src/storage/index-lock.js'; const baseResult: AnalyzeResult = { repoName: 'repo', @@ -121,6 +122,37 @@ describe('runWorkerAnalysis — finalize guard (#2264 P2)', () => { expect(send).toHaveBeenCalledWith({ type: 'error', message: 'boom' }); expect(finalize).not.toHaveBeenCalled(); }); + + it('tags an index-lock timeout as a retryable index-lock-timeout error (#2658 review M2)', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const holder: LockRecord = { + v: 1, + pid: -1, + hostname: 'host', + startTime: null, + token: '', + invocationId: 'unknown', + acquiredAt: '', + }; + const lockContended: WorkerAnalysisDeps['runFullAnalysis'] = vi.fn(async () => { + throw new IndexLockTimeoutError(holder, 600_000, false); + }); + + await runWorkerAnalysis( + '/repo', + {}, + { + runFullAnalysis: lockContended, + assertAnalysisFinalized: okFinalize, + send, + claimTerminal: alwaysClaim, + }, + ); + + expect(send).toHaveBeenCalledWith( + expect.objectContaining({ type: 'error', code: 'index-lock-timeout', retryable: true }), + ); + }); }); describe('runWorkerAnalysis — terminal-claim coordination (#2264 P3)', () => { diff --git a/gitnexus/test/unit/index-lock.test.ts b/gitnexus/test/unit/index-lock.test.ts new file mode 100644 index 000000000..1a6846cc0 --- /dev/null +++ b/gitnexus/test/unit/index-lock.test.ts @@ -0,0 +1,381 @@ +/** + * Unit tests for the cross-process index write lock (#2658). + * + * These exercise the lock's decision logic deterministically by pre-seeding + * `analyze.lock` records and asserting acquire/steal/release/sweep behavior — + * including the kill-recovery mechanism (a dead holder's lock is reclaimed) and + * mutual exclusion (a live holder is waited on, never stolen). A real + * two-process exclusion + SIGKILL-recovery test lives in + * test/integration/analyze-index-lock-concurrency.test.ts. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { + mkdtempSync, + rmSync, + writeFileSync, + readFileSync, + existsSync, + chmodSync, + symlinkSync, +} from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { + acquireIndexLock, + sweepStagingArtifacts, + isLockUnwritableCode, + IndexLockTimeoutError, + type LockRecord, +} from '../../src/storage/index-lock.js'; +import { classifyFtsBuildError, ftsFailureIsFatal } from '../../src/core/search/fts-indexes.js'; + +let dir: string; +const lockPath = () => path.join(dir, 'analyze.lock'); + +const seedLock = (overrides: Partial): void => { + const record: LockRecord = { + v: 1, + pid: 999999999, // implausible pid → dead by default + hostname: os.hostname(), + startTime: null, + token: 'seed-token', + invocationId: 'seed-invocation', + acquiredAt: new Date().toISOString(), + ...overrides, + }; + writeFileSync(lockPath(), JSON.stringify(record)); +}; + +beforeEach(() => { + dir = mkdtempSync(path.join(os.tmpdir(), 'gnx-lock-')); + // These suites exercise the file (O_EXCL pidfile) backend directly. On Linux + // the default is the socket backend, so pin the file backend explicitly. + process.env.GITNEXUS_INDEX_LOCK_BACKEND = 'file'; +}); +afterEach(() => { + delete process.env.GITNEXUS_INDEX_LOCK_BACKEND; + rmSync(dir, { recursive: true, force: true }); +}); + +describe('acquireIndexLock', () => { + it('acquires a free directory and writes a record carrying our pid', async () => { + const lock = await acquireIndexLock(dir); + expect(existsSync(lockPath())).toBe(true); + const onDisk = JSON.parse(readFileSync(lockPath(), 'utf8')) as LockRecord; + expect(onDisk).toMatchObject({ v: 1, pid: process.pid, hostname: os.hostname() }); + expect(lock.record.token).toBe(onDisk.token); + lock.release(); + expect(existsSync(lockPath())).toBe(false); + }); + + it('reclaims a stale lock left by a dead process (kill recovery)', async () => { + seedLock({ pid: 999999999, token: 'dead-holder' }); + const lock = await acquireIndexLock(dir, { timeoutMs: 2000 }); + const onDisk = JSON.parse(readFileSync(lockPath(), 'utf8')) as LockRecord; + expect(onDisk.pid).toBe(process.pid); + expect(onDisk.token).not.toBe('dead-holder'); + lock.release(); + }); + + it('waits on a live holder and times out instead of stealing (mutual exclusion)', async () => { + // A live pid (our own) with a different token — never stale, so acquire + // must block and then time out rather than clobber the holder. + seedLock({ pid: process.pid, startTime: null, token: 'live-holder' }); + await expect(acquireIndexLock(dir, { timeoutMs: 300, pollMs: 20 })).rejects.toBeInstanceOf( + IndexLockTimeoutError, + ); + // The live holder's record is untouched. + const onDisk = JSON.parse(readFileSync(lockPath(), 'utf8')) as LockRecord; + expect(onDisk.token).toBe('live-holder'); + }); + + it('surfaces the holder identity on timeout', async () => { + seedLock({ pid: process.pid, startTime: null, token: 'live-holder', invocationId: 'held-run' }); + await expect(acquireIndexLock(dir, { timeoutMs: 200, pollMs: 20 })).rejects.toMatchObject({ + holder: { invocationId: 'held-run', pid: process.pid }, + }); + }); + + it.skipIf(process.platform !== 'linux')( + 'treats a reused pid (live pid, different start time) as stale', + async () => { + // Our pid is alive but the seeded start time cannot match it → reused. + seedLock({ pid: process.pid, startTime: '1', token: 'reused-pid' }); + const lock = await acquireIndexLock(dir, { timeoutMs: 2000 }); + const onDisk = JSON.parse(readFileSync(lockPath(), 'utf8')) as LockRecord; + expect(onDisk.token).not.toBe('reused-pid'); + lock.release(); + }, + ); + + it('reclaims an empty lock file (crash between O_EXCL create and record write) without hanging', async () => { + // Pre-fix, readRecord→null hot-looped forever here. Post-fix it reclaims + // the malformed orphan after the grace and acquires. + writeFileSync(lockPath(), ''); + const lock = await acquireIndexLock(dir, { timeoutMs: 5000, pollMs: 20 }); + const onDisk = JSON.parse(readFileSync(lockPath(), 'utf8')) as LockRecord; + expect(onDisk.pid).toBe(process.pid); + lock.release(); + }); + + it('reclaims a partial/malformed record (valid JSON, missing token) without hanging', async () => { + writeFileSync(lockPath(), '{"pid":123}'); + const lock = await acquireIndexLock(dir, { timeoutMs: 5000, pollMs: 20 }); + const onDisk = JSON.parse(readFileSync(lockPath(), 'utf8')) as LockRecord; + expect(onDisk.pid).toBe(process.pid); + expect(onDisk.token.length).toBeGreaterThan(0); + lock.release(); + }); + + it('treats a non-positive/NaN pid as no readable holder and reclaims (never wedges on process.kill) (#2658 review L4)', async () => { + // `{"pid":0}` pre-fix: typeof 0 === 'number' passed readRecord, then + // process.kill(0,0) reported the process group "alive" → treated as a live + // holder → the acquire wedged until the full timeout. Post-fix a pid that is + // not a positive integer makes readRecord return null, so the file is a + // malformed orphan that is reclaimed after the grace. + seedLock({ pid: 0, token: 'zero-pid' }); + const lock = await acquireIndexLock(dir, { timeoutMs: 5000, pollMs: 20 }); + const onDisk = JSON.parse(readFileSync(lockPath(), 'utf8')) as LockRecord; + expect(onDisk.pid).toBe(process.pid); + expect(onDisk.token).not.toBe('zero-pid'); + lock.release(); + }); + + it('honors GITNEXUS_INDEX_LOCK_TIMEOUT_MS as the wait ceiling (bounds pid-reuse hangs)', async () => { + // A live holder we cannot steal (own pid, no start-time recorded). Without a + // finite ceiling this would hang; the env var must bound it (#2658). + seedLock({ pid: process.pid, startTime: null, token: 'live-holder' }); + const prev = process.env.GITNEXUS_INDEX_LOCK_TIMEOUT_MS; + process.env.GITNEXUS_INDEX_LOCK_TIMEOUT_MS = '150'; + try { + // No explicit timeoutMs → the env ceiling applies (not the 10-min default). + await expect(acquireIndexLock(dir, { pollMs: 20 })).rejects.toBeInstanceOf( + IndexLockTimeoutError, + ); + } finally { + if (prev === undefined) delete process.env.GITNEXUS_INDEX_LOCK_TIMEOUT_MS; + else process.env.GITNEXUS_INDEX_LOCK_TIMEOUT_MS = prev; + } + }); +}); + +describe('release', () => { + it('does not remove a lock that has been re-taken by another owner', async () => { + const lock = await acquireIndexLock(dir); + // Simulate the file being replaced by a different owner after we acquired. + seedLock({ pid: process.pid, token: 'someone-else' }); + lock.release(); + expect(existsSync(lockPath())).toBe(true); // not ours → left intact + const onDisk = JSON.parse(readFileSync(lockPath(), 'utf8')) as LockRecord; + expect(onDisk.token).toBe('someone-else'); + }); + + it('is idempotent', async () => { + const lock = await acquireIndexLock(dir); + lock.release(); + expect(() => lock.release()).not.toThrow(); + }); +}); + +describe('sweepStagingArtifacts', () => { + it('removes only staging files, never the live index or its sidecars', () => { + const files = [ + 'lbug', + 'lbug.wal', + 'lbug.shadow', + 'lbug.new', + 'lbug.new.wal', + 'lbug.new.wal.checkpoint', + 'lbug.staging.abc-123', + 'lbug.staging.abc-123.wal', + 'lbug.staging.abc-123.shadow', + 'gitnexus.json', + ]; + for (const f of files) writeFileSync(path.join(dir, f), 'x'); + + sweepStagingArtifacts(dir); + + const survives = (f: string) => existsSync(path.join(dir, f)); + expect(survives('lbug')).toBe(true); + expect(survives('lbug.wal')).toBe(true); + expect(survives('lbug.shadow')).toBe(true); + expect(survives('gitnexus.json')).toBe(true); + expect(survives('lbug.new')).toBe(false); + expect(survives('lbug.new.wal')).toBe(false); + expect(survives('lbug.new.wal.checkpoint')).toBe(false); + expect(survives('lbug.staging.abc-123')).toBe(false); + expect(survives('lbug.staging.abc-123.wal')).toBe(false); + expect(survives('lbug.staging.abc-123.shadow')).toBe(false); + }); + + it('runs the sweep automatically on acquire', async () => { + writeFileSync(path.join(dir, 'lbug.staging.orphan'), 'x'); + writeFileSync(path.join(dir, 'lbug'), 'x'); + const lock = await acquireIndexLock(dir); + expect(existsSync(path.join(dir, 'lbug.staging.orphan'))).toBe(false); + expect(existsSync(path.join(dir, 'lbug'))).toBe(true); + lock.release(); + }); +}); + +describe('classifyFtsBuildError', () => { + it('classifies IO/rename/checkpoint/corruption failures as integrity', () => { + expect( + classifyFtsBuildError( + 'IO exception: Error renaming file lbug.new.wal to lbug.new.wal.checkpoint. ErrorMessage: No such file or directory', + ), + ).toBe('integrity'); + expect(classifyFtsBuildError('checkpoint failed')).toBe('integrity'); + expect(classifyFtsBuildError('database file is corrupt')).toBe('integrity'); + expect(classifyFtsBuildError('write failed: no space left on device (ENOSPC)')).toBe( + 'integrity', + ); + }); + + it('classifies row-level tokenizer failures as capability (degrade)', () => { + expect(classifyFtsBuildError('Failed calling LOWER: Invalid UTF-8')).toBe('capability'); + expect(classifyFtsBuildError('tokenizer error on row 5')).toBe('capability'); + }); + + it('defaults unknown failures to capability so runs are not newly failed', () => { + expect(classifyFtsBuildError('some unrecognised message')).toBe('capability'); + expect(classifyFtsBuildError('missing indexes after build: File.name_fts')).toBe('capability'); + }); + + it('keeps a bare ENOENT / bad-fd as capability so it degrades, not aborts (#2658 review L1)', () => { + // A missing extension asset / closed handle reports a generic OS error; those + // must NOT escalate to an abort on the atomic-swap path. Only a specific + // write/rename/checkpoint failure is integrity. + expect(classifyFtsBuildError('ENOENT: no such file or directory, open fts.ext')).toBe( + 'capability', + ); + expect(classifyFtsBuildError('read failed: bad file descriptor (EBADF)')).toBe('capability'); + // The genuine build-broke rename race is still integrity via 'error renaming'. + expect( + classifyFtsBuildError('Error renaming lbug.new.wal to checkpoint: No such file or directory'), + ).toBe('integrity'); + }); + + it('lets a row-level tokenizer error win even if it mentions an integrity word', () => { + // A tokenizer error is a bad row, not a broken build — must still degrade. + expect(classifyFtsBuildError('Invalid UTF-8 during io exception path')).toBe('capability'); + }); +}); + +describe('ftsFailureIsFatal (#2658)', () => { + it('is fatal ONLY for an integrity failure on the atomic-swap path', () => { + // Atomic swap: staging DB, previous index intact → integrity may abort. + expect(ftsFailureIsFatal('integrity', true)).toBe(true); + // In-place: live DB already mutated, nothing to roll back → degrade. + expect(ftsFailureIsFatal('integrity', false)).toBe(false); + // Capability never aborts, either path. + expect(ftsFailureIsFatal('capability', true)).toBe(false); + expect(ftsFailureIsFatal('capability', false)).toBe(false); + // Missing class (ok result, or no classification) never aborts. + expect(ftsFailureIsFatal(undefined, true)).toBe(false); + }); +}); + +// The OS socket/pipe backend is only meaningful where `net` gives a clean, +// auto-releasing namespace: Linux abstract sockets and Windows named pipes. +describe.skipIf(process.platform !== 'linux' && process.platform !== 'win32')( + 'OS socket lock backend (#2658)', + () => { + // Override the file-backend pin from the outer beforeEach. + beforeEach(() => { + process.env.GITNEXUS_INDEX_LOCK_BACKEND = 'socket'; + }); + + it('holds no filesystem lock file (works on a read-only index dir)', async () => { + const lock = await acquireIndexLock(dir, { timeoutMs: 2000 }); + expect(existsSync(lockPath())).toBe(false); // endpoint is outside the dir + lock.release(); + }); + + it('excludes a second acquire on the same dir, then frees it on release', async () => { + const first = await acquireIndexLock(dir, { timeoutMs: 2000 }); + // A second acquire on the SAME slot is refused by the kernel (EADDRINUSE) + // and waits, then times out — the live holder is never displaced. + await expect(acquireIndexLock(dir, { timeoutMs: 300, pollMs: 20 })).rejects.toBeInstanceOf( + IndexLockTimeoutError, + ); + first.release(); + // Once released, the endpoint is free again. + const second = await acquireIndexLock(dir, { timeoutMs: 2000 }); + second.release(); + }); + + it('reports the holder as unknown on timeout — never a bogus "pid -1" (#2658 review M3)', async () => { + // The OS socket lock exposes no owner metadata, so a contended-wait timeout + // must not surface the unknownHolder() placeholder pid (-1) as if it were a + // real process the operator can look up. + const first = await acquireIndexLock(dir, { timeoutMs: 2000 }); + try { + const err = await acquireIndexLock(dir, { timeoutMs: 200, pollMs: 20 }).catch((e) => e); + expect(err).toBeInstanceOf(IndexLockTimeoutError); + expect((err as IndexLockTimeoutError).holderKnown).toBe(false); + expect((err as IndexLockTimeoutError).message).not.toContain('pid -1'); + } finally { + first.release(); + } + }); + + it('excludes an acquire reaching the same physical dir via a symlink alias (#2658 review H1)', async () => { + // Pre-fix the endpoint name hashed the LEXICAL path, so `alias` (a symlink + // to `dir`) produced a different name and BOTH acquired — a double-writer. + // Post-fix both canonicalize to `dir`'s real path → one name → excluded. + const alias = mkdtempSync(path.join(os.tmpdir(), 'gnx-lock-aliasparent-')); + const aliasLink = path.join(alias, 'link'); + symlinkSync(dir, aliasLink); + try { + const first = await acquireIndexLock(dir, { timeoutMs: 2000 }); + await expect( + acquireIndexLock(aliasLink, { timeoutMs: 300, pollMs: 20 }), + ).rejects.toBeInstanceOf(IndexLockTimeoutError); + first.release(); + } finally { + rmSync(alias, { recursive: true, force: true }); + } + }); + + it('gives independent locks to different index dirs', async () => { + const other = mkdtempSync(path.join(os.tmpdir(), 'gnx-lock-other-')); + try { + const a = await acquireIndexLock(dir, { timeoutMs: 2000 }); + const b = await acquireIndexLock(other, { timeoutMs: 2000 }); // distinct name → no contention + a.release(); + b.release(); + } finally { + rmSync(other, { recursive: true, force: true }); + } + }); + }, +); + +describe('read-only / permission-denied filesystem (#2658)', () => { + it('classifies EROFS/EACCES/EPERM as tolerable, others not', () => { + expect(isLockUnwritableCode('EROFS')).toBe(true); + expect(isLockUnwritableCode('EACCES')).toBe(true); + expect(isLockUnwritableCode('EPERM')).toBe(true); + expect(isLockUnwritableCode('EEXIST')).toBe(false); + expect(isLockUnwritableCode('ENOENT')).toBe(false); + expect(isLockUnwritableCode(undefined)).toBe(false); + }); + + // Mode bits are bypassed for uid 0, so the denied-create path only reproduces + // as non-root. The predicate test above is the always-on guard. + it.skipIf(!process.getuid || process.getuid() === 0)( + 'returns a no-op handle instead of throwing when the lock dir cannot be written', + async () => { + chmodSync(dir, 0o555); + try { + const lock = await acquireIndexLock(dir, { timeoutMs: 2000 }); + expect(typeof lock.release).toBe('function'); + expect(() => lock.release()).not.toThrow(); + expect(existsSync(lockPath())).toBe(false); // lock file was never created + } finally { + chmodSync(dir, 0o755); + } + }, + ); +}); diff --git a/gitnexus/test/unit/run-analyze-fts-repair.test.ts b/gitnexus/test/unit/run-analyze-fts-repair.test.ts index eb524305f..3dc0be797 100644 --- a/gitnexus/test/unit/run-analyze-fts-repair.test.ts +++ b/gitnexus/test/unit/run-analyze-fts-repair.test.ts @@ -493,6 +493,8 @@ describe('runFullAnalysis FTS repair and verification failure paths', () => { ok: false, error: 'missing indexes after build: Function.function_fts', })), + ftsFailureIsFatal: (fc: 'capability' | 'integrity' | undefined, swap: boolean) => + fc === 'integrity' && swap, })); vi.doMock('../../src/core/ingestion/pipeline.js', () => ({ runPipelineFromRepo: vi.fn(async (repoPath: string) => ({ @@ -513,6 +515,7 @@ describe('runFullAnalysis FTS repair and verification failure paths', () => { ); expect(result.ftsSkipped).toBe(true); + expect(result.ftsSkipReason).toBe('build-failed'); // #2658 review L2 expect(logs.join('\n')).toMatch( /FTS index build failed.*missing indexes after build.*keyword search degraded this run/i, ); @@ -525,6 +528,77 @@ describe('runFullAnalysis FTS repair and verification failure paths', () => { } }); + it('ABORTS (throws before publish, leaves the previous index intact) on an FTS integrity failure on the atomic-swap path (#2658 review M1)', async () => { + // The single-writer lock rules out a concurrent-writer race, so an + // integrity-class FTS failure on the atomic-swap (--force) path is a real + // broken build: run-analyze must throw BEFORE swapping the staging DB in, + // leaving the previous live index untouched — not silently publish a + // search-less index as success. This end-to-end throw path was previously + // untested (only the ftsFailureIsFatal truth table was). + vi.doMock('../../src/core/lbug/lbug-adapter.js', () => ({ + initLbug: vi.fn(async () => undefined), + loadGraphToLbug: vi.fn(async () => undefined), + getLbugStats: vi.fn(async () => ({ nodes: 0, edges: 0, communities: 0, processes: 0 })), + executeQuery: vi.fn(async () => []), + executeWithReusedStatement: vi.fn(async () => []), + closeLbug: vi.fn(async () => undefined), + wipeLbugDbFiles: vi.fn(async () => undefined), + loadCachedEmbeddings: vi.fn(async () => ({ embeddingNodeIds: new Set(), embeddings: [] })), + deleteNodesForFile: vi.fn(async () => undefined), + deleteNodesForFiles: vi.fn(async () => undefined), + deleteAllCommunitiesAndProcesses: vi.fn(async () => undefined), + queryImporters: vi.fn(async () => []), + queryImportersBatch: vi.fn(async () => []), + loadFTSExtension: vi.fn(async () => true), + })); + // Import the REAL classifier/predicate (not a re-stub) so the test pins the + // actual fatal-decision logic, per the #2658 review. + vi.doMock('../../src/core/search/fts-indexes.js', async () => { + const actual = await vi.importActual( + '../../src/core/search/fts-indexes.js', + ); + return { + ...actual, + initialiseSearchFTSStemmer: vi.fn(() => 'porter'), + buildSearchIndexesOrDegrade: vi.fn(async () => ({ + ok: false, + failureClass: 'integrity' as const, + error: 'IO exception: Error renaming lbug.staging.wal to checkpoint', + })), + }; + }); + vi.doMock('../../src/core/ingestion/pipeline.js', () => ({ + runPipelineFromRepo: vi.fn(async (repoPath: string) => ({ + repoPath, + graph: { forEachNode: () => undefined }, + })), + })); + + const tmpRepo = await createTempDir('gitnexus-run-analyze-integrity-abort-'); + try { + const { storagePath, lbugPath } = getStoragePaths(tmpRepo.dbPath); + await fs.mkdir(storagePath, { recursive: true }); + // A pre-existing "previous index" that must survive the aborted rebuild. + await createPlaceholderGraphStore(lbugPath); + const before = await fs.readFile(lbugPath); + + const { runFullAnalysis } = await import('../../src/core/run-analyze.js'); + const message = await runFullAnalysis( + tmpRepo.dbPath, + { force: true }, + { onProgress: () => {}, onLog: () => {} }, + ).catch((e: unknown) => (e instanceof Error ? e.message : String(e))); + + expect(message).toMatch(/integrity error/i); + expect(message).toMatch(/aborted|previous index is\s+left intact/i); + // The previous index bytes are untouched (throw happened before the swap). + const after = await fs.readFile(lbugPath); + expect(after.equals(before)).toBe(true); + } finally { + await tmpRepo.cleanup(); + } + }); + it('full analyze degrades gracefully (no throw, warns, skips index creation) when FTS extension is unavailable', async () => { // Offline-first degradation: when loadFTSExtension() returns false, the // analyze path must NOT call createSearchFTSIndexes / verifySearchFTSIndexes @@ -584,6 +658,7 @@ describe('runFullAnalysis FTS repair and verification failure paths', () => { ); expect(result.ftsSkipped).toBe(true); + expect(result.ftsSkipReason).toBe('extension-unavailable'); // #2658 review L2 expect(createSearchFTSIndexes).not.toHaveBeenCalled(); expect(verifySearchFTSIndexes).not.toHaveBeenCalled(); expect(logs.join('\n')).toMatch(/FTS extension unavailable; skipping search-index creation/i); @@ -665,6 +740,7 @@ describe('runFullAnalysis FTS repair and verification failure paths', () => { ); expect(result.ftsSkipped).toBe(true); + expect(result.ftsSkipReason).toBe('extension-unavailable'); // #2658 review L2 const degradeLine = logs .filter((l) => l.includes('skipping search-index creation')) .join('\n'); @@ -1005,3 +1081,124 @@ describe('runFullAnalysis dirty-recovery parking failure fails fast (this shippi } }); }); + +describe('runFullAnalysis re-resolves git state under the lock (#2658 review H2)', () => { + afterEach(() => { + vi.doUnmock('../../src/storage/git.js'); + vi.doUnmock('../../src/core/ingestion/pipeline.js'); + vi.resetModules(); + vi.clearAllMocks(); + }); + + it('re-reads HEAD after acquiring the lock, so a commit that lands during the wait is not missed', async () => { + // acquireIndexLock can wait up to the timeout ceiling; HEAD may advance + // during that wait. Pre-fix, resolveWriteTarget was called ONCE (before the + // lock) and its stale snapshot fed the freshness check — a waiter could + // return alreadyUpToDate against the OLD commit. Post-fix the wrapper + // re-resolves UNDER the lock, so getCurrentCommit is called again and the + // post-wait commit is what the pipeline uses. Simulate the advance by making + // getCurrentCommit return a new value on each call. + const commits = ['commit-before-wait', 'commit-after-wait']; + let call = 0; + const getCurrentCommit = vi.fn( + () => commits[call < commits.length ? call++ : commits.length - 1], + ); + vi.doMock('../../src/storage/git.js', async () => { + const actual = await vi.importActual( + '../../src/storage/git.js', + ); + return { + ...actual, + getCurrentCommit, + hasGitDir: () => true, + getCurrentBranch: () => 'main', + isWorkingTreeDirty: () => false, + }; + }); + // Stop the run right after the wrapper's two resolveWriteTarget calls so the + // test pins the re-resolve, not the full pipeline. + vi.doMock('../../src/core/ingestion/pipeline.js', () => ({ + runPipelineFromRepo: vi.fn(async () => { + throw new Error('stop-after-resolve'); + }), + })); + + const tmpRepo = await createTempDir('gitnexus-h2-relock-'); + try { + const { runFullAnalysis } = await import('../../src/core/run-analyze.js'); + await runFullAnalysis( + tmpRepo.dbPath, + { force: true }, + { onProgress: () => {}, onLog: () => {} }, + ).catch(() => undefined); + + // Pre-fix: exactly 1 (single pre-lock resolve). Post-fix: >= 2 (re-resolve + // under the lock), and the second call observed the post-wait commit. + expect(getCurrentCommit.mock.calls.length).toBeGreaterThanOrEqual(2); + expect(getCurrentCommit.mock.results[1]?.value).toBe('commit-after-wait'); + } finally { + await tmpRepo.cleanup(); + } + }); + + it('releases the lock when the under-lock re-resolve throws (no leak) (#2658 review H2 self-review)', async () => { + // The re-resolve runs UNDER the held lock and can throw (e.g. a `--branch` + // that no longer matches a checkout switched during the wait). That throw + // must still release the lock — the loop lives inside the try/finally. + vi.doUnmock('../../src/storage/git.js'); + const release = vi.fn(); + vi.doMock('../../src/storage/index-lock.js', async () => { + const actual = await vi.importActual( + '../../src/storage/index-lock.js', + ); + return { + ...actual, + acquireIndexLock: vi.fn(async () => ({ + record: { + v: 1, + pid: 1, + hostname: 'h', + startTime: null, + token: 't', + invocationId: 'i', + acquiredAt: '', + }, + release, + })), + }; + }); + // getCurrentCommit succeeds on the pre-lock resolve, then throws on the + // under-lock re-resolve — the exact shape a mid-wait git change produces. + let call = 0; + vi.doMock('../../src/storage/git.js', async () => { + const actual = await vi.importActual( + '../../src/storage/git.js', + ); + return { + ...actual, + hasGitDir: () => true, + getCurrentBranch: () => 'main', + isWorkingTreeDirty: () => false, + getCurrentCommit: () => { + if (call++ === 0) return 'c1'; + throw new Error('git HEAD read failed mid-wait'); + }, + }; + }); + + const tmpRepo = await createTempDir('gitnexus-h2-leak-'); + try { + const { runFullAnalysis } = await import('../../src/core/run-analyze.js'); + const err = await runFullAnalysis( + tmpRepo.dbPath, + { force: true }, + { onProgress: () => {}, onLog: () => {} }, + ).catch((e: unknown) => e); + expect(err).toBeInstanceOf(Error); + expect(release).toHaveBeenCalledTimes(1); // lock freed despite the throw + } finally { + vi.doUnmock('../../src/storage/index-lock.js'); + await tmpRepo.cleanup(); + } + }); +});