diff --git a/GUARDRAILS.md b/GUARDRAILS.md index 3fe36a875..3e9bdff83 100644 --- a/GUARDRAILS.md +++ b/GUARDRAILS.md @@ -31,7 +31,7 @@ Format: **Trigger → Instruction → Reason**. Append new Signs when the same m ### Stale graph after edits - **Trigger:** MCP warns index is behind `HEAD`, or search doesn't match latest commit. -- **Do:** `npx gitnexus analyze` (plus `--embeddings` if used). Runs incrementally by default — the pipeline parses every file every run (cross-file resolution requires it), but tree-sitter dispatch is skipped for unchanged file chunks via the content-addressed cache, and only changed-file rows (plus their importers, transitively) are rewritten in LadybugDB. When the effective write set exceeds ~50% of the repo's files (minimum 50 files), the run transparently switches to the full wipe + bulk-COPY write plan and logs "switching to a full DB write" — expected behavior, not a bug, and file-level bookkeeping stays incremental. +- **Do:** `npx gitnexus analyze` (plus `--embeddings` if used). Runs incrementally by default — the pipeline parses every file every run (cross-file resolution requires it), but tree-sitter dispatch is skipped for unchanged file chunks via the content-addressed cache, and only changed-file rows (plus their importers, transitively) are rewritten in LadybugDB. When the effective write set exceeds ~50% of the repo's files (minimum 50 files), the run transparently switches to the full wipe + bulk-COPY write plan and logs "switching to a full DB write" — expected behavior, not a bug, and file-level bookkeeping stays incremental. That same line also appears — regardless of write-set size, even for a one-file change — when a LadybugDB extension the existing index depends on cannot load on this machine (VECTOR, #2623; FTS, #2841), because a DB carrying those indexes refuses all row-level DML until the extension is loaded; run `gitnexus doctor` for live extension status and re-run with `GITNEXUS_LBUG_EXTENSION_INSTALL=auto` (with network access) to allow one bounded install attempt. The rebuild is one-shot: it clears the indexes, so the next run goes back to the incremental plan. - **Why:** Tools query LadybugDB from last analyze; git changes are invisible until re-indexed. ### Index seems corrupt or "incremental" is misbehaving diff --git a/gitnexus/scripts/cross-platform-shard.ts b/gitnexus/scripts/cross-platform-shard.ts index bf5100436..1a026a5e0 100644 --- a/gitnexus/scripts/cross-platform-shard.ts +++ b/gitnexus/scripts/cross-platform-shard.ts @@ -47,6 +47,13 @@ export const WINDOWS_WEIGHTS_SEC: Readonly> = { 'test/integration/cli-e2e.test.ts': 361, 'test/integration/worker-pool.test.ts': 222, 'test/unit/incremental-vector-extension-ordering.test.ts': 87, + // ESTIMATE, not a measurement (#2841): this suite drives more full + // `runFullAnalysis` cycles than the VECTOR sibling above, so the 8 s + // PER_FILE_OVERHEAD floor would badly under-charge it and skew the Windows + // split — the failure mode that produced the job timeouts this table exists + // to prevent. Scaled from the sibling's measured 87 s by analyze-run count. + // Replace with a real figure after the first green Windows matrix run. + 'test/unit/incremental-index-extension-dml-gate.test.ts': 180, 'test/integration/cli-limit-e2e.test.ts': 75, 'test/unit/hooks.test.ts': 26, 'test/integration/analyze-heap-oom-e2e.test.ts': 23, diff --git a/gitnexus/scripts/cross-platform-tests.ts b/gitnexus/scripts/cross-platform-tests.ts index 325699953..69383d355 100644 --- a/gitnexus/scripts/cross-platform-tests.ts +++ b/gitnexus/scripts/cross-platform-tests.ts @@ -154,6 +154,18 @@ const LBUG_NATIVE = [ // proven on the windows-latest native addon, not just Ubuntu. Budget: ~25s // on Linux → expect ~2min on the slowest Windows shard. 'test/unit/incremental-vector-extension-ordering.test.ts', + // #2841: the FTS half of that same gate, plus the both-extensions-blocked + // case — and it needs this matrix for two reasons the VECTOR sibling above + // does not cover. The reported failure environment is a machine where the + // extension stopped LOADING, which is the #2374 class and Windows-reported + // (the same reason fts-extension-e2e.test.ts is registered below), so the + // FTS-unavailable branch has to run on a real Windows/macOS runner rather + // than only on Ubuntu where FTS always loads. And its both-blocked case is + // gated on GITNEXUS_REQUIRE_VECTOR=1, which ci-tests.yml sets ONLY on this + // job — everywhere else an unavailable VECTOR extension skips instead of + // failing. Budget: four real analyze runs, so expect it to sit alongside the + // VECTOR sibling's ~87s Windows measurement. + 'test/unit/incremental-index-extension-dml-gate.test.ts', ]; // Process spawning and CLI tests — exercise child_process with real diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 6851c2478..84708d1aa 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -1644,9 +1644,14 @@ const analyzeCommandImpl = async ( ); } else { console.log( + // NOT "then rerun" (#2841 §5.C): this run stamped `lastCommit`, so a + // plain rerun on an unchanged tree takes the up-to-date fast path and + // returns before Phase 3 could rebuild anything — the advice would be + // ineffective exactly when the user follows it. `--repair-fts` is the + // verb that rebuilds the search indexes without re-parsing the repo. `\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.`, + ` Install it once with network access (GITNEXUS_LBUG_EXTENSION_INSTALL=auto), then run\n` + + ` \`gitnexus analyze --repair-fts\` to build the search indexes. Run \`gitnexus doctor\` for details.`, ); } } diff --git a/gitnexus/src/core/lbug/extension-loader.ts b/gitnexus/src/core/lbug/extension-loader.ts index df11b284e..0ad020519 100644 --- a/gitnexus/src/core/lbug/extension-loader.ts +++ b/gitnexus/src/core/lbug/extension-loader.ts @@ -363,5 +363,26 @@ export const extensionManager = new ExtensionManager(); export const getExtensionCapabilities = (): ExtensionCapability[] => extensionManager.getCapabilities(); +/** + * One optional extension's capability record, or `undefined` when nothing in + * this process has resolved it yet. + * + * `getExtensionCapabilities().find((c) => c.name === …)` was spelled out at five + * call sites across four modules (#2841 review), each re-deriving the same + * lookup — and each free to drift on the extension NAME, which is the one string + * the lookup is keyed on. Keep the name in one place. + */ +export const getExtensionCapability = (name: string): ExtensionCapability | undefined => + getExtensionCapabilities().find((c) => c.name === name); + +/** + * {@link getExtensionCapability} for the FTS extension — the only extension + * whose capability record is read outside this module (degrade warnings, the + * `--repair-fts` reporting path, and `dropFTSIndex`'s remedy lookup), so the + * `'fts'` spelling itself lives here rather than at each of them. + */ +export const getFtsCapability = (): ExtensionCapability | undefined => + getExtensionCapability('fts'); + /** Test-only: clear the singleton's cached capability and install state. */ export const resetExtensionState = (): void => extensionManager.reset(); diff --git a/gitnexus/src/core/lbug/lbug-adapter.ts b/gitnexus/src/core/lbug/lbug-adapter.ts index 597676455..ec7ffcb7b 100644 --- a/gitnexus/src/core/lbug/lbug-adapter.ts +++ b/gitnexus/src/core/lbug/lbug-adapter.ts @@ -32,9 +32,13 @@ import { getNodeLabel as deriveNodeLabel, type WriteStreamFactory } from './rel- import { EMBEDDABLE_LABELS, type CachedEmbedding } from '../embeddings/types.js'; import { extensionManager, + getFtsCapability, resolveAnalyzeInstallPolicy, type ExtensionEnsureOptions, } from './extension-loader.js'; +// Remedy classification for LOAD failures (#2374/#2383). Pure + node:fs only, so +// this adds no cycle: `extension-loader.ts` already depends on it. +import { diagnoseExtensionLoad } from './extension-load-error.js'; import { classifyDeleteAllError, closeLbugConnection, @@ -3026,6 +3030,140 @@ export const createVectorIndex = async (): Promise => { } }; +/** + * One row of `CALL SHOW_INDEXES()`. + * + * Kept EXPORTED although nothing outside this module names it (#2841 review + * §5.H): it is the element type of {@link readIndexCatalogRows}' and + * {@link IndexCatalogSnapshot}'s public signatures, and `declaration: true` + * requires every type reachable from an exported signature to be exported too. + * + * LADYBUGDB-CONTRACT: on @ladybugdb/core 0.18.x rows arrive as NAMED records — + * `table_name`, `index_name`, `index_type`, `property_names`, + * `extension_loaded`, `index_definition` — and the readers below key on those + * names plus the literal `'FTS'` / `'HASH'` index-type spellings. Probe-recorded + * on 0.18.3: `rows[0][0] === undefined`, so the positional fallbacks (`row?.[0]` + * &c.) the accessors below carry are DEAD on this version. They are deliberately + * kept rather than deleted (#2841 review §5.H): they cost nothing, and removing + * the hedge would turn a future return to the unnamed-tuple form older builds + * used into a silently fail-OPEN read for the VECTOR gate, + * `ftsIndexPresenceInCatalog` and the `dropSearchFTSIndexes` sweep — the #2841 + * failure class this file exists to close. (`ensureFtsRowDmlSafe` is the one + * exception: it treats an unreadable type as "might be FTS" and gates, so it + * fails CLOSED on the tuple form — see its §6.A note. Losing the fallback would + * cost it precision, not safety.) When bumping LadybugDB, re-validate — `git + * grep "LADYBUGDB-CONTRACT"` enumerates every version-coupled spot, and the + * column/position coupling itself is reachable ONLY through the three accessors + * below, so that enumeration is true by construction rather than by discipline. + */ +export type IndexCatalogRow = Record; + +/** + * The three field reads every index-catalog consumer needs, each hedging the + * named-record form against the positional one exactly once. + * + * They exist because the hedge used to be inlined at five call sites across two + * modules (#2841 review), one of which — the `dropSearchFTSIndexes` sweep in + * `core/search/fts-indexes.ts` — carried no LADYBUGDB-CONTRACT marker at all, so + * the doc above claimed a grep that could not find it. Exported for that module; + * everything version-coupled about the row shape now lives in this one block. + */ +export const indexRowTable = (row: IndexCatalogRow | undefined): unknown => + row?.table_name ?? row?.[0]; +export const indexRowName = (row: IndexCatalogRow | undefined): unknown => + row?.index_name ?? row?.[1]; +export const indexRowType = (row: IndexCatalogRow | undefined): unknown => + row?.index_type ?? row?.[2]; + +/** + * Read the index catalog on the writable connection, or `undefined` when it + * cannot be read. + * + * `SHOW_INDEXES` is readable WITHOUT any extension loaded and reports + * `extension_loaded` per index, so the extension-gated-DML checks below settle + * the common "this DB carries no such index" case with one local read and no + * error-string sniffing. It runs through the unprepared `conn.query()` path + * like every other `CALL` procedure here (#2114). + * + * `undefined` means "could not prove anything" and every caller must treat it + * as fail-closed (assume an index may be present), never as "no indexes". All + * three readers below honour that, `ftsIndexExistsInCatalog` included since + * #2841 review H3. To hand ONE read to several gates, use + * {@link readIndexCatalogSnapshot} — passing this `undefined` on cannot be + * distinguished from passing nothing at all. + */ +export const readIndexCatalogRows = async (): Promise => { + const targetConn = conn; + if (!targetConn) { + throw new Error('LadybugDB not initialized. Call initLbug first.'); + } + try { + return (await withConnLock(async () => + readQueryRows(await targetConn.query('CALL SHOW_INDEXES() RETURN *')), + )) as IndexCatalogRow[]; + } catch (err) { + logger.warn( + { err }, + 'Could not read the LadybugDB index catalog (CALL SHOW_INDEXES()); ' + + 'extension-gated DML checks must assume an index may be present.', + ); + return undefined; + } +}; + +/** + * The failed half of an {@link IndexCatalogSnapshot}: the caller DID read the + * catalog and could not prove anything. + * + * It exists because `undefined` was overloaded (#2841 review §5.A). The gates + * below took `indexRows?: IndexCatalogRow[]`, so "my read failed" and "I passed + * you nothing" were the SAME value, and each gate's `?? (await + * readIndexCatalogRows())` silently re-read the catalog — turning the one shared + * read the call site documents into three round-trips and three identical + * warnings on the failure path, with the two gates free to decide from DIFFERENT + * snapshots. A distinct sentinel makes "read, unreadable" a value the parameter + * can carry, so a supplied snapshot is never re-read. + */ +export const INDEX_CATALOG_UNREADABLE: unique symbol = Symbol('gitnexus:index-catalog-unreadable'); + +/** + * One `CALL SHOW_INDEXES()` read in a form that survives being handed from one + * gate to the next: the rows, or {@link INDEX_CATALOG_UNREADABLE} when the read + * failed. + */ +export type IndexCatalogSnapshot = IndexCatalogRow[] | typeof INDEX_CATALOG_UNREADABLE; + +/** + * {@link readIndexCatalogRows} in snapshot form — what callers should read once + * and pass to EVERY extension-gated-DML gate in a run, so the "one shared + * `SHOW_INDEXES` read" invariant holds on the failure branch too (#2841 review + * §5.A). The `IndexCatalogRow[] | undefined` spelling stays available for + * callers that only want the rows. + */ +export const readIndexCatalogSnapshot = async (): Promise => + (await readIndexCatalogRows()) ?? INDEX_CATALOG_UNREADABLE; + +/** + * Resolve a gate's optional `indexRows` argument into the rows it must judge, + * reading the catalog AT MOST ONCE and ONLY when the caller supplied nothing. + * + * The `??` is meaningful again (#2841 review §5.A): `undefined` in can now only + * mean "no snapshot supplied", because a caller whose own read failed passes + * {@link INDEX_CATALOG_UNREADABLE}, which is truthy and short-circuits it. + * `undefined` OUT keeps its documented meaning — "could not prove anything", + * which every caller of {@link readIndexCatalogRows} treats as fail-closed. + * + * Exported for the `dropSearchFTSIndexes` sweep in `core/search/fts-indexes.ts`, + * which takes the same optional-snapshot parameter and must resolve it by the + * same rules — including the "a supplied snapshot is never re-read" half. + */ +export const resolveGateRows = async ( + indexRows: IndexCatalogSnapshot | undefined, +): Promise => { + const snapshot = indexRows ?? (await readIndexCatalogSnapshot()); + return snapshot === INDEX_CATALOG_UNREADABLE ? undefined : snapshot; +}; + /** * Make DML against {@link EMBEDDING_TABLE_NAME} legal on the writable * connection when it can be, and report whether it is. @@ -3051,17 +3189,31 @@ export const createVectorIndex = async (): Promise => { * escalating to the wipe-and-rebuild write plan instead of failing * mid-writeback. * - * Cheap by construction: one local `SHOW_INDEXES` read settles the common - * "this repo never built an embedding index" case without touching the - * extension machinery at all, so a VECTOR-less machine is not charged a - * bounded INSTALL attempt on every incremental analyze. `SHOW_INDEXES` is - * readable WITHOUT the extension and reports `extension_loaded` per index, so - * no error-string sniffing is needed; it runs through the unprepared - * `conn.query()` path like every other `CALL` procedure here (#2114). + * Cheap by construction: one {@link readIndexCatalogRows} read settles the + * common "this repo never built an embedding index" case without touching the + * extension machinery at all, so a VECTOR-less machine is not charged a bounded + * INSTALL attempt on every incremental analyze. (That read's own mechanics and + * fail-closed contract are documented there, not re-explained here — #2841 + * review §5.H.) + * + * @param indexRows An {@link IndexCatalogSnapshot} the caller already read, so + * one `SHOW_INDEXES` read can settle every gate in a run. FRESHNESS CONTRACT: + * the snapshot must have been taken on THIS connection with nothing in between + * that creates or drops an index — the gate's verdict is only as current as the + * rows it is handed. Pass {@link INDEX_CATALOG_UNREADABLE} (what + * {@link readIndexCatalogSnapshot} returns) when your own read failed; that + * fails closed here WITHOUT a second read. Omit the argument entirely to have + * the gate read the catalog itself. */ -export const ensureEmbeddingRowDmlSafe = async (): Promise => { - const targetConn = conn; - if (!targetConn) { +export const ensureEmbeddingRowDmlSafe = async ( + indexRows?: IndexCatalogSnapshot, +): Promise => { + // Unconditional precondition (#2841 review §5.B). This check used to run on + // every call; adding `indexRows` moved it inside `readIndexCatalogRows`, where + // a caller-supplied snapshot skips it — so a closed DB could be answered + // `true` where it previously threw. The verdict is only meaningful for the + // live writable connection, so assert that before looking at the argument. + if (!conn) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } // Catalog FIRST. The overwhelmingly common case on a repo that never enabled @@ -3070,34 +3222,81 @@ export const ensureEmbeddingRowDmlSafe = async (): Promise => { // on a VECTOR-less machine pay a bounded out-of-process INSTALL attempt (the // `auto` policy) plus an "extension unavailable" warning, for a repo that // can never hit this hazard. - let indexRows: any[] | undefined; - try { - indexRows = await withConnLock(async () => - readQueryRows(await targetConn.query('CALL SHOW_INDEXES() RETURN *')), - ); - } catch (err) { - // Fall through to the load attempt: unable to prove the index is absent, - // so the extension is the only thing that can make DML safe. - logger.warn( - { err }, - `Could not read the index catalog to check for a ${EMBEDDING_TABLE_NAME} vector index; ` + - 'falling back to loading the VECTOR extension.', - ); - } + const rows = await resolveGateRows(indexRows); // Any non-HASH index on the embedding table gates DML. Keyed on index TYPE, // not name, so an index built under a different name still counts; the // implicit primary-key HASH index is engine-internal and never gates. const indexGatesDml = - indexRows === undefined || - indexRows.some((row) => { - const table = row?.table_name ?? row?.[0]; - if (table !== EMBEDDING_TABLE_NAME) return false; - return (row?.index_type ?? row?.[2]) !== 'HASH'; + rows === undefined || + rows.some((row) => { + if (indexRowTable(row) !== EMBEDDING_TABLE_NAME) return false; + return indexRowType(row) !== 'HASH'; }); if (!indexGatesDml) return true; return await loadVectorExtension(undefined, { policy: resolveAnalyzeInstallPolicy() }); }; +/** + * The FTS twin of {@link ensureEmbeddingRowDmlSafe} (#2841). + * + * LadybugDB refuses DML against a table carrying an FTS index while the FTS + * extension is not loaded on that connection, and it refuses it at BIND time — + * probed against @ladybugdb/core 0.18.3, a DETACH DELETE matching ZERO rows + * fails just as hard as one matching thousands ("Binder exception: Trying to + * delete from an index on table File but its extension is not loaded"). Every + * table in `FTS_INDEXES` is therefore immutable until the extension loads, and + * there is no narrower escape: `CALL DROP_FTS_INDEX` is itself an + * FTS-extension function ("Catalog exception: function DROP_FTS_INDEX is not + * defined" in exactly the state that would need rescuing), and LadybugDB has + * no SQL `DROP INDEX` at all (both spellings are Parser exceptions). Rebuilding + * the DB file is the only way to clear the indexes without the extension. + * + * `true` = FTS-indexed-table DML is safe: either FTS is now loaded, or the DB + * carries no FTS index to trip over. `false` = genuinely blocked; the analyze + * orchestrator answers that by escalating to the wipe-and-rebuild write plan + * instead of dying mid-writeback with an engine error that never says "FTS". + * + * Catalog-first for the same reason as the VECTOR twin: a repo whose index + * never carried FTS must not pay a bounded INSTALL attempt on every + * incremental analyze. Keyed on index TYPE, so an index left over from an + * older `FTS_INDEXES` (different name/table set) still counts. + * + * @param indexRows Same contract as {@link ensureEmbeddingRowDmlSafe}'s: an + * {@link IndexCatalogSnapshot} read on THIS connection with no index created or + * dropped since, so both gates decide from the SAME snapshot and the catalog is + * read once per run. {@link INDEX_CATALOG_UNREADABLE} fails closed here without + * a second read; omitting the argument makes the gate read for itself. + */ +export const ensureFtsRowDmlSafe = async (indexRows?: IndexCatalogSnapshot): Promise => { + // Unconditional precondition, same regression as the VECTOR twin's (#2841 + // review §5.B): a caller-supplied snapshot must not let a closed DB be + // answered `true`. + if (!conn) { + throw new Error('LadybugDB not initialized. Call initLbug first.'); + } + const rows = await resolveGateRows(indexRows); + // LADYBUGDB-CONTRACT: the `'FTS'` index_type spelling — see {@link IndexCatalogRow}. + // + // Polarity (#2841 review §6.A): a row whose type cannot be read gates DML. + // A bare `=== 'FTS'` answers `undefined === 'FTS'` → false → *no gate*, i.e. + // it falls OPEN in the one gate whose only job is preventing an unsafe write, + // while the VECTOR twin above falls CLOSED for the same input. Deliberately + // NOT expressed as the twin's `!== 'HASH'`: that predicate is safe there only + // because it is scoped to `EMBEDDING_TABLE_NAME` first, whereas this gate is + // table-agnostic, so `!== 'HASH'` would let the HNSW vector index gate FTS + // DML and charge every embeddings-enabled repo an FTS load it does not need. + const indexGatesDml = + rows === undefined || + rows.some((row) => { + const indexType = indexRowType(row); + // Unreadable shape ⇒ "might be FTS" ⇒ gate. Only a positively-identified + // non-FTS index is waved through. + return indexType === undefined || indexType === 'FTS'; + }); + if (!indexGatesDml) return true; + return await loadFTSExtension(undefined, { policy: resolveAnalyzeInstallPolicy() }); +}; + /** * Lazy-create an FTS index, caching the fact in-process. * @@ -3279,10 +3478,64 @@ export const queryFTS = async ( export const isBenignDropFtsIndexError = (message: string): boolean => message.startsWith('Binder exception:') || message.startsWith('Catalog exception:'); +/** + * The half of {@link isBenignDropFtsIndexError}'s catalog case that means "the + * FTS extension is not loaded", as opposed to "this index does not exist". + * Benign only when there is genuinely nothing to drop — see `dropFTSIndex`. + */ +const DROP_FTS_INDEX_UNDEFINED_SIGNATURE = 'function DROP_FTS_INDEX is not defined'; + +/** + * What the catalog can say about one index's liveness. Three-valued on purpose: + * "the catalog proves it is there" and "the catalog could not be read" both + * BLOCK (fail-closed), but they are not the same fact, and the caller reports + * them differently (#2841 cleanup review). + */ +type FtsIndexPresence = 'present' | 'absent' | 'unverifiable'; + +/** + * Whether `indexName` is currently present on `tableName` in the catalog. + * + * `unverifiable` ⇒ "an index may be present" (#2841 review H3), and the caller + * must treat it exactly as it treats `present`. This used to answer a bare + * `false` there — reporting an unprovable catalog as "index absent", the one + * meaning {@link readIndexCatalogRows} explicitly forbids — which made + * `dropFTSIndex` swallow the very error this guard exists to raise and handed + * the caller a drop that never happened, reproducing the silent #2841 crash one + * DML statement later. The asymmetric cost settles it: a false positive raises a + * loud, FTS-naming, remedy-carrying error on a run that had already lost its + * catalog; a false negative resumes a writeback that cannot succeed. + */ +const ftsIndexPresenceInCatalog = async ( + tableName: string, + indexName: string, +): Promise => { + const rows = await readIndexCatalogRows(); + if (rows === undefined) return 'unverifiable'; + return rows.some((row) => indexRowTable(row) === tableName && indexRowName(row) === indexName) + ? 'present' + : 'absent'; +}; + /** * Drop an FTS index. Tolerates only {@link isBenignDropFtsIndexError} — * anything else rethrows instead of being silently masked, which previously * let a corrupted index persist across analyze runs undetected. + * + * One benign class is conditional (#2841): `Catalog exception: function + * DROP_FTS_INDEX is not defined` says the FTS extension is not loaded, which + * is "nothing to drop" only when the named index does not exist. When it DOES + * exist, swallowing that error reports a drop that never happened, and the + * next insert/delete against that table dies at bind time with an engine + * message that never mentions FTS — the #2841 crash. So the liveness question + * is settled with a catalog read on the ERROR path only (the healthy path + * still costs nothing) and a live-but-undroppable index is raised loudly, + * naming FTS and both remedies — the load-side one CLASSIFIED, never + * hand-rolled (#2841 review §5.G). + * + * A catalog that cannot be read blocks identically (see + * {@link ftsIndexPresenceInCatalog}) but is reported as an inability to verify, + * not as an assertion that the index exists. */ export const dropFTSIndex = async (tableName: string, indexName: string): Promise => { if (!conn) { @@ -3296,6 +3549,49 @@ export const dropFTSIndex = async (tableName: string, indexName: string): Promis if (!isBenignDropFtsIndexError(msg)) { throw e; } + const presence = msg.includes(DROP_FTS_INDEX_UNDEFINED_SIGNATURE) + ? await ftsIndexPresenceInCatalog(tableName, indexName) + : 'absent'; + if (presence !== 'absent') { + // Remedy via the shared classifier, UNCONDITIONALLY and never hand-rolled + // (#2841 review §5.G). `extension-load-error.ts` exists because "set + // GITNEXUS_LBUG_EXTENSION_INSTALL=auto" is the WRONG advice for the + // missing-runtime-dependency class (Windows error 126 / OpenSSL, #2374) — + // the file is already on disk and reinstalling is a no-op. This used to + // honour the classifier for `missing_dependency` only and hand-write the + // other three, which handed a corrupt / wrong-platform extension file the + // very "allow one bounded install attempt" advice #2383 removed, and + // discarded `missingFileRemedy` / `corruptFileRemedy` outright. Every one + // of the four kinds already carries a class-correct remedy by + // construction, so take it as-is. + // + // Prefer the diagnosis cached at mark-unavailable time (#2383 F3) so the + // extension binary is not re-inspected, falling back to a fresh structural + // diagnosis when nothing recorded one. + const ftsCapability = getFtsCapability(); + const { remedy } = ftsCapability?.diagnosis ?? diagnoseExtensionLoad(ftsCapability?.reason); + // Deliberately message-only: `remedy` is generated text (fixed system paths + // at most), and LadybugDB's own path-bearing `reason` is NEVER interpolated + // here — the #2374/#2375 redaction contract. + // + // `unverifiable` blocks exactly as hard as `present`, but must not be + // WORDED as `present`: the one reachable path into it is a run whose + // `ensureFtsRowDmlSafe` already answered "safe" because the catalog showed + // no FTS index, after which this later read failed — so asserting the index + // exists would contradict what the same run just proved (#2841 cleanup + // review). + const lead = + presence === 'present' + ? `FTS index '${indexName}' on table ${tableName} exists but the LadybugDB FTS ` + + 'extension is not loaded, so it cannot be dropped in this environment.' + : `FTS index '${indexName}' on table ${tableName} could not be verified as absent — ` + + 'the LadybugDB index catalog could not be read — and the FTS extension is not ' + + 'loaded, so the index could not be dropped either.'; + throw new Error( + `${lead} Every insert and delete against that table fails while the index is present. ` + + `${remedy} Otherwise rebuild the index without FTS via \`gitnexus analyze --force\`.`, + ); + } } finally { ensuredFTSIndexes.delete(ftsIndexKey(tableName, indexName)); } diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index b2fa92ab9..8e8c4ac7d 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -32,6 +32,9 @@ import { loadCachedEmbeddings, deleteNodesForFiles, ensureEmbeddingRowDmlSafe, + ensureFtsRowDmlSafe, + readIndexCatalogSnapshot, + INDEX_CATALOG_UNREADABLE, deleteAllCommunitiesAndProcesses, deleteAllInterprocTaintPaths, deleteAllCallSummaries, @@ -65,7 +68,12 @@ import { getSearchFTSCjkSegmentation, initialiseSearchFTSCjkSegmentation, } from './search/cjk-segmentation.js'; -import { getExtensionCapabilities, resolveAnalyzeInstallPolicy } from './lbug/extension-loader.js'; +import { + getExtensionCapability, + getExtensionCapabilities, + getFtsCapability, + resolveAnalyzeInstallPolicy, +} from './lbug/extension-loader.js'; import { diagnoseExtensionLoad } from './lbug/extension-load-error.js'; import { startWalCheckpointDriver, @@ -1043,6 +1051,12 @@ async function runFullAnalysisInner( // Surface the load-side reason (#2374): "not pre-installed" was wrong // and doctor never installed anything, so the old message trapped // users in a query → repair-fts → doctor loop with no way out. + // NOTE: deliberately the exported `getExtensionCapabilities()` rather + // than `getFtsCapability()`. The #2383 regression tests stub that + // export to inject a classified load failure; routing through the + // helper bypasses the stub (ESM internal calls do not see a module + // mock), and the classified VC++/ELF remedy silently degrades to + // generic text — which is exactly the contradiction #2383 fixed. const rawFtsReason = getExtensionCapabilities().find((c) => c.name === 'fts')?.reason; const ftsReason = rawFtsReason?.replace(/\.$/, ''); // A missing runtime dependency (Windows error 126, #2374) is not healed @@ -1460,6 +1474,21 @@ async function runFullAnalysisInner( // opt-in branch so the common fast path keeps its single-stat cost. const healUnregistered = options.allowDuplicateName === true && !(await isRepoRegistered(repoPath)); + // §5.C is deliberately NOT self-healed here. An #2841 FTS-forced rebuild + // stamps `lastCommit`, so a plain rerun lands on this fast path and the + // search indexes stay missing until the next content change. The fix for + // that is the ADVICE, not a probe: the degraded-search warning now points + // at `gitnexus analyze --repair-fts` (which rebuilds the indexes without + // re-parsing anything) instead of "then rerun". + // + // An auto-heal probe was tried and reverted. It could not distinguish + // "extension was missing" from "index build failed" without a stamped + // discriminator, so a deterministic build failure (#2544/#2546) re-analyzed + // the whole repo on every invocation forever; it opened the live index on + // the millisecond fast path; and it turned this early return into a full + // re-analysis whenever an index authored where FTS was unavailable was + // later read on a host where it loads — which is a legitimate, common + // state, and the invariant `analyzer-identity-cli.test.ts` pins. if (!dirty && !healUnregistered) { // ── #2354: restamp the workspace label on a same-commit branch flip ── // The flat slot follows the checked-out working tree; a branch switch @@ -1769,13 +1798,19 @@ async function runFullAnalysisInner( if (wantAtomicIncremental && !atomicIncremental) { log('atomic-incremental: live index carries orphan sidecars — using in-place writeback'); } - const useAtomicSwap = (isFullRebuild || atomicIncremental) && (posixSwap || windowsSwapOk); + // `let` (#2841 review H2): an escalation discovered ~440 lines below — from + // EITHER cause, a blocked extension or an oversized write set — can upgrade + // an in-place incremental write to a staged one, because that valve's plan is + // wipe-then-COPY over this very path. See the upgrade at the escalation + // valve. Nothing between here and there reads either binding except + // `initLbug(buildPath)`, which the upgrade re-runs against the staging path. + let useAtomicSwap = (isFullRebuild || atomicIncremental) && (posixSwap || windowsSwapOk); // #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; + let buildPath = useAtomicSwap ? `${lbugPath}.staging.${randomUUID()}` : lbugPath; if (isIncremental && hashDiff) { log( @@ -2122,8 +2157,40 @@ async function runFullAnalysisInner( // cannot be dropped without the extension either), so surgery is // impossible: fall through to the escalation valve's wipe-and-COPY plan, // which rebuilds the DB files outright and needs no embedding-row DML. - const embeddingRowDmlSafe = await ensureEmbeddingRowDmlSafe(); - if (!embeddingRowDmlSafe && cachedEmbeddings.length === 0) { + // + // FTS twin (#2841): the identical wall exists for every table carrying an + // FTS index — LadybugDB refuses the DML at BIND time, so even a zero-row + // DETACH DELETE fails, and `DROP_FTS_INDEX` is itself an FTS-extension + // function (there is no SQL `DROP INDEX` at all), so the indexes cannot be + // cleared in place either. Same verdict, same remedy: escalate. Both gates + // share ONE `SHOW_INDEXES` read — they answer different questions about the + // same catalog snapshot, and nothing between here and the write plan + // creates or drops an index. + const indexCatalogRows = await readIndexCatalogSnapshot(); + const embeddingRowDmlSafe = await ensureEmbeddingRowDmlSafe(indexCatalogRows); + const ftsRowDmlSafe = await ensureFtsRowDmlSafe(indexCatalogRows); + const extensionForcedRebuild = !embeddingRowDmlSafe || !ftsRowDmlSafe; + // `!options.dropEmbeddings` (H1): this rescue reads the rows back OUT of + // the DB, so it must never fire on the one path whose entire purpose is to + // destroy them. `--drop-embeddings` deliberately leaves `cachedEmbeddings` + // empty (`deriveEmbeddingMode` returns `shouldLoadCache: false` for it by + // construction — see the four-mode comment at the cache-load site), and its + // `options.force = true` conversion sits INSIDE + // `if (existingMeta?.embeddingCheckpoint)`, so a repo without a checkpoint + // stays incremental and arrives here holding exactly the state the rescue + // reads as "the index metadata did not account for them" — restoring the N + // rows the operator just asked to wipe, printing `Preserving N` on top of + // this run's own `Dropping N` line, and exiting 0. + // + // The predicate has to be the FLAG, not `shouldLoadCache`: that would also + // disable the rescue in the case it exists for (meta says 0 embeddings + // while rows survive ⇒ `hasExisting` false ⇒ `shouldLoadCache` false), i.e. + // it would fix the wipe by deleting the safeguard. Covers + // `--drop-embeddings --embeddings` too — the rescue repopulates + // `cachedEmbeddingNodeIds`, which Phase 4 hands `runEmbeddingPipeline` as + // the already-embedded set, so the very nodes the user asked to REGENERATE + // would be skipped. + if (extensionForcedRebuild && !options.dropEmbeddings && cachedEmbeddings.length === 0) { // The escalation below WIPES the DB files, and Phase 3.5 restores // embedding rows from `cachedEmbeddings` — which is only populated when // `deriveEmbeddingMode` saw `meta.stats.embeddings > 0`. A DB whose meta @@ -2143,30 +2210,131 @@ async function runFullAnalysisInner( ); } } - if ( - !embeddingRowDmlSafe || - shouldEscalateIncrementalWrite( - filesToDelete.length, - effectiveWriteSet.size, - allFilePaths.length, - ) - ) { + // Hoisted out of the `||` below (§5.D): the size verdict has to be KNOWN + // even when a blocked extension already forced the rebuild, or the message + // cannot report both. Pure predicate over three numbers + // (incremental/escalation-gate.ts), so evaluating it unconditionally costs + // nothing and has no side effects. + const sizeForcedRebuild = shouldEscalateIncrementalWrite( + filesToDelete.length, + effectiveWriteSet.size, + allFilePaths.length, + ); + if (extensionForcedRebuild || sizeForcedRebuild) { escalatedFullWrite = true; - log( + // Every live cause is named, not just the first: a DB can carry BOTH a + // vector index and FTS indexes, and reporting one cause while the other + // is equally fatal is how #2841 stayed mis-diagnosed for so long. §5.D: + // that argument crosses the extension/size boundary too, so the size + // cause is APPENDED here rather than selected between — the old either/or + // ternary dropped the write-set line whenever an extension also blocked. + const escalationCauses: string[] = []; + const degradedEffects: string[] = []; + // H5: `readIndexCatalogRows()` returning nothing means "could not prove + // anything", and both gates correctly fail CLOSED on it — but a + // fail-closed sentinel is not evidence. Asserting "the CodeEmbedding + // vector index exists" from it is affirmatively FALSE on a repo that + // never enabled embeddings, and the only truthful signal (the adapter's + // `Could not read the LadybugDB index catalog` warning) goes to the pino + // stderr stream, NOT this `onLog` callback — so `gitnexus serve` and the + // analyze worker UI would show the invented claim alone. Emit one honest + // cause naming the unsettled read instead of two fabricated ones. + // Tested against the explicit sentinel, NOT truthiness: §5.A made the + // failed read representable (`INDEX_CATALOG_UNREADABLE`) precisely so + // "the caller passed nothing" and "the caller tried and could not prove + // anything" stop sharing one value — and the sentinel is a Symbol, so a + // `!indexCatalogRows` test would silently never fire here. + const indexCatalogUnreadable = indexCatalogRows === INDEX_CATALOG_UNREADABLE; + // `extensionForcedRebuild &&`: an unreadable catalog is only a CAUSE + // when it actually blocked something. A size-only escalation whose + // catalog read happened to fail still had both gates answer "safe" + // (both extensions loaded), and claiming otherwise would trade one + // invented cause for another. + if (extensionForcedRebuild && indexCatalogUnreadable) { + const blockedExtensions = [ + !embeddingRowDmlSafe ? 'VECTOR' : undefined, + !ftsRowDmlSafe ? 'FTS' : undefined, + ].filter((name): name is string => name !== undefined); + escalationCauses.push( + `the LadybugDB index catalog could not be read (the read error is on the analyzer's ` + + `warning stream), so neither a live ${EMBEDDING_TABLE_NAME} vector index nor a live ` + + `FTS search index could be ruled out, and the ${blockedExtensions.join(' and ')} ` + + `extension${blockedExtensions.length > 1 ? 's' : ''} could not be loaded to rewrite ` + + `indexed rows in place either`, + ); + } + if (!embeddingRowDmlSafe) { + if (!indexCatalogUnreadable) { + escalationCauses.push( + `the ${EMBEDDING_TABLE_NAME} vector index exists but the VECTOR extension could not be ` + + `loaded, so embedding rows cannot be rewritten in place`, + ); + } + degradedEffects.push( + 'Semantic search falls back to exact scan until VECTOR is available.', + ); + } + if (!ftsRowDmlSafe) { + if (!indexCatalogUnreadable) { + // Self-contained subject (H5): `join('; and ')` used to render "…the + // CodeEmbedding vector index exists … and THIS INDEX carries FTS + // search indexes…", pointing "this index" at the vector index just + // named — and an index does not carry indexes. + escalationCauses.push( + `the graph store carries one or more FTS search indexes but the FTS extension could ` + + `not be loaded, so no indexed table can be written in place (LadybugDB refuses the ` + + `write at bind time, and the indexes cannot be dropped without the extension either)`, + ); + } + degradedEffects.push('Full-text/BM25 search stays degraded until FTS is available.'); + } + if (sizeForcedRebuild) { + escalationCauses.push( + `the effective write set covers ${effectiveWriteSet.size}/${allFilePaths.length} ` + + // Display clamp only (predicate unchanged): BFS-found deleted + // importers can push the numerator past the CURRENT file list, so + // the raw fraction can exceed 1 — see the population-mismatch note + // on shouldEscalateIncrementalWrite (tri-review 4669518496). + `files (${Math.min(100, Math.round(writeFraction * 100))}%)`, + ); + } + // Remedy by CLASSIFICATION, never hand-written (#2841 review H3). The + // old tail always said "run `gitnexus doctor` … or set + // GITNEXUS_LBUG_EXTENSION_INSTALL=auto", which is affirmatively WRONG + // for the `missing_dependency` class (Windows error 126 / absent + // OpenSSL 3, #2374/#2669): its own remedy states that reinstalling will + // not help, and that class is precisely the environment this escalation + // path was registered for on the Windows matrix. Every other rendering + // in this file already routes through `diagnoseExtensionLoad` — the + // FTS_UNAVAILABLE_LEAD degrade log and the `--repair-fts` failure tail + // — so this one does too, once per BLOCKED extension and with that + // extension's own label, because the FTS-specific advice the classifier + // emits (`gitnexus analyze --repair-fts`) must never be dispensed for + // VECTOR. Emitted verbatim and alone: only the classified remedy + // reaches the user, never the raw load `reason`, so the message stays + // path-free (#2374/#2375 redaction contract). Reached exactly when an + // extension blocked the write — `degradedEffects` is pushed by the two + // `!…RowDmlSafe` branches above and by nothing else, and each of those + // gates only answers `false` after its own load attempt failed, so the + // capability record it reads is always populated. Looked up through the + // shared `getExtensionCapability`/`getFtsCapability` accessors rather + // than a sixth hand-spelled `.find((c) => c.name === …)`: the extension + // NAME is the one string the lookup is keyed on, and it belongs in + // extension-loader.ts. + const extensionRemedies = [ !embeddingRowDmlSafe - ? `Incremental: the ${EMBEDDING_TABLE_NAME} vector index exists but the VECTOR ` + - `extension could not be loaded, so embedding rows cannot be rewritten in place — ` + - `switching to a full DB write (wipe + bulk COPY) for this run. Semantic search ` + - `falls back to exact scan until VECTOR is available; run \`gitnexus doctor\` for ` + - `live extension status, or set GITNEXUS_LBUG_EXTENSION_INSTALL=auto to allow one ` + - `bounded install attempt.` - : `Incremental: effective write set covers ${effectiveWriteSet.size}/${allFilePaths.length} ` + - // Display clamp only (predicate unchanged): BFS-found deleted - // importers can push the numerator past the CURRENT file list, so - // the raw fraction can exceed 1 — see the population-mismatch note - // on shouldEscalateIncrementalWrite (tri-review 4669518496). - `files (${Math.min(100, Math.round(writeFraction * 100))}%) — switching to a full DB write ` + - `(wipe + bulk COPY) for this run; file-level incremental bookkeeping is unaffected.`, + ? { reason: getExtensionCapability('VECTOR')?.reason, label: 'VECTOR' } + : undefined, + !ftsRowDmlSafe ? { reason: getFtsCapability()?.reason, label: 'FTS' } : undefined, + ] + .filter((e): e is { reason: string | undefined; label: string } => e !== undefined) + .map(({ reason, label }) => diagnoseExtensionLoad(reason, label).remedy); + log( + `Incremental: ${escalationCauses.join('; and ')} — switching to a full DB write ` + + `(wipe + bulk COPY) for this run; file-level incremental bookkeeping is unaffected.` + + (degradedEffects.length > 0 + ? ` ${degradedEffects.join(' ')} ${extensionRemedies.join(' ')}` + : ''), ); // toWriteCount: 0 is the established full-path dirty-flag sentinel; // the real counters ride along for crash diagnostics. @@ -2185,6 +2353,57 @@ async function runFullAnalysisInner( // surviving family member throws a typed LbugWipeError here instead // of letting the reopen below resurrect the rows this run just chose // to replace wholesale. + // #2841 review H2 — never destroy the only complete index before its + // replacement is durable. `buildPath` was frozen ~440 lines above, while + // this run was still classified incremental, so it still points AT the + // live index: escalating without this upgrade means + // `wipeLbugDbFiles(lbugPath)` followed by a bulk COPY in place, and an + // interrupt, ENOSPC, or COPY failure anywhere in that window leaves NO + // complete index at all. + // + // That invariant is about RECOVERABILITY, which does not depend on why + // the run escalated — the wipe-then-COPY plan below is identical for + // both causes, so a size-forced escalation loses the index to a Ctrl-C + // exactly as an extension-forced one does. Both stage. The escalation + // rebuilds from the in-memory graph the pipeline already produced, so + // staging costs no `fs.copyFile` of the old DB: it is peak disk plus a + // rename — precisely what a plain `--force` full rebuild already pays + // unconditionally on POSIX. + // + // Safe because the gate runs BEFORE any row DML: the DB open at + // `buildPath` is unmutated, so switching targets loses nothing. The + // end-of-run swap publishes the staging file atomically, and a failure + // anywhere before it leaves the previous index live (its own comment + // says so) with the dirty flag already stamped above for recovery. + // + // Knock-on effects of flipping `useAtomicSwap` here, both intended: + // `ftsFailureIsFatal(..., useAtomicSwap)` now aborts instead of + // degrading on an FTS *integrity* error — which is exactly that + // predicate's documented staging contract (throwing abandons a + // throwaway file and keeps the live index) — and `forceRealCloseForSwap` + // engages on Windows, which is why the upgrade is gated on the same + // `posixSwap || windowsSwapOk` policy that governs every other swap. + // …but NOT when we are escalating out of ignorance. An unreadable + // catalog means `CALL SHOW_INDEXES()` itself failed, which on a real + // index means the store is damaged — e.g. a stray directory sitting at + // `lbug.wal.checkpoint` makes every open of that path an IO exception. + // Staging would then quietly write a fresh index NEXT to the damage, + // swap it in, and exit 0: the run "succeeds", the broken sidecar + // survives untouched, and the next in-place writeback trips over it + // again. Building in place keeps the underlying IO fault on the failure + // path where the operator gets a diagnosis (this is what + // `analyze-wal-checkpoint-failure.test.ts` pins). Staging protects a + // HEALTHY live index from a machine-level cause; it must not be used to + // route around a damaged one. + const catalogWasReadable = indexCatalogRows !== INDEX_CATALOG_UNREADABLE; + if (catalogWasReadable && !useAtomicSwap && (posixSwap || windowsSwapOk)) { + useAtomicSwap = true; + buildPath = `${lbugPath}.staging.${randomUUID()}`; + log( + 'Incremental: building the replacement index alongside the live one and swapping it in ' + + 'at the end, so an interrupted rebuild leaves the current index intact.', + ); + } await walCheckpointDriver.stop(); await closeLbug(); await wipeLbugDbFiles(buildPath); @@ -2208,7 +2427,11 @@ async function runFullAnalysisInner( // removes the hazard outright; Phase 3's createSearchFTSIndexes // rebuilds every index from the final row set regardless, so // this is a no-op on its own drop step there. - await dropSearchFTSIndexes(); + // Reuse the snapshot read at the gate above (#2841 cleanup): same run, + // same connection, and nothing on this branch creates or drops an index + // in between — so re-reading would only weaken the one-read invariant + // the snapshot type exists to enforce. + await dropSearchFTSIndexes(indexCatalogRows); // 1b. Remove the write set's existing rows — batched (#2409): one // DETACH DELETE per table per 200-file chunk. The former per-file // loop issued a count + delete per table per FILE — ~13k @@ -2390,6 +2613,8 @@ async function runFullAnalysisInner( // generic "install it with network access" tail in FTS_UNAVAILABLE_MESSAGE // contradicts the remedy's own "reinstalling will NOT help" (#2383 F2). Lead // with the class-neutral sentence and append only the classified remedy. + // Same #2383 mock seam as the repair path above — keep the exported + // `getExtensionCapabilities()` lookup here. const ftsReason = getExtensionCapabilities().find((c) => c.name === 'fts')?.reason; const { kind, remedy } = diagnoseExtensionLoad(ftsReason); log( @@ -2946,6 +3171,17 @@ async function runFullAnalysisInner( fts: { provider: 'ladybugdb-fts', status: ftsReady ? runtimeCapabilities.fts : 'unavailable', + // Persist WHICH cause degraded FTS, not merely THAT it degraded + // (#2841 review H1). `status` alone collapses "the extension could + // not load" and "the extension loaded but the build failed" into one + // value, and §5.C's fast-path probe reads that value: with the cause + // erased it must guess, guesses `extension-unavailable`, and a + // `build-failed` run therefore re-analyzes the whole repo on every + // subsequent no-op run — the build fails identically (an + // un-tokenizable stored row, #2544/#2546, is deterministic), restamps + // 'unavailable', and the next run does it again. Stamping the + // discriminator the run already computed makes the read exact instead. + skipReason: ftsReady ? undefined : ftsSkipReason, }, vectorSearch: { provider: effectiveSemanticMode === 'vector-index' ? 'ladybugdb-vector' : 'exact-scan', @@ -3237,6 +3473,22 @@ async function runFullAnalysisInner( } catch { /* swallow */ } + // Reclaim the staging index this run created (#2841 cleanup). Without this + // a failed staged build orphans a FULL copy of the index — hundreds of MB on + // a large repo — until the next `acquireIndexLock` sweeps `lbug.staging.` + // artifacts, and the failure most likely to leave one (a machine whose + // extension cannot load) is also the one least likely to be followed by + // another analyze. Only ever removes a path this run minted: `buildPath` + // differs from `lbugPath` exactly when the atomic-swap plan is in effect, + // and the live index is never that path. Best-effort by construction — the + // rethrow below is the surface, and the lock's sweep remains the backstop. + if (useAtomicSwap && buildPath !== lbugPath) { + try { + await wipeLbugDbFiles(buildPath); + } catch { + /* swallow — orphan reclamation must never mask the real failure */ + } + } throw err; } } diff --git a/gitnexus/src/core/search/fts-indexes.ts b/gitnexus/src/core/search/fts-indexes.ts index dfc55286c..f6d119ce9 100644 --- a/gitnexus/src/core/search/fts-indexes.ts +++ b/gitnexus/src/core/search/fts-indexes.ts @@ -1,5 +1,13 @@ -import { createFTSIndex, dropFTSIndex, DEFAULT_FTS_STEMMER } from '../lbug/lbug-adapter.js'; -import { getExtensionCapabilities } from '../lbug/extension-loader.js'; +import { + createFTSIndex, + dropFTSIndex, + indexRowName, + indexRowTable, + resolveGateRows, + DEFAULT_FTS_STEMMER, + type IndexCatalogSnapshot, +} from '../lbug/lbug-adapter.js'; +import { getFtsCapability } from '../lbug/extension-loader.js'; import { classifyExtensionLoadError } from '../lbug/extension-load-error.js'; import { FTS_INDEXES } from './fts-schema.js'; @@ -67,7 +75,7 @@ const formatWarningContext = (context: FtsWarningContext): string => { */ export const ftsDegradedWarning = (context?: FtsWarningContext): string => { const suffix = context ? formatWarningContext(context) : ''; - const fts = getExtensionCapabilities().find((c) => c.name === 'fts'); + const fts = getFtsCapability(); if (fts && !fts.loaded) { const reason = fts.reason ? redactPaths(fts.reason).replace(/\.$/, '') : undefined; // A missing *runtime dependency* (Windows error 126, etc.) is not healed by @@ -188,15 +196,63 @@ export function getSearchFTSStemmer(): string { } /** - * Drop every configured FTS index (no-op per index when absent or unloadable - * — `dropFTSIndex` tolerates both). Callable ahead of any DML that mutates an - * FTS-indexed table's rows: LadybugDB's FTS extension is not proven to - * survive a DETACH DELETE against a table that still carries a live index - * from a prior run (#2589) — dropping first removes that hazard entirely, - * regardless of whether it also fixed a specific native inconsistency. + * Drop every configured FTS index ahead of any DML that mutates an FTS-indexed + * table's rows: LadybugDB's FTS extension is not proven to survive a DETACH + * DELETE against a table that still carries a live index from a prior run + * (#2589) — dropping first removes that hazard entirely, regardless of whether + * it also fixed a specific native inconsistency. + * + * CALLER OBLIGATION (#2841). `dropFTSIndex` still no-ops per index when the + * index is ABSENT, but "unloadable" is no longer unconditionally tolerated: a + * LIVE index plus an FTS extension that cannot load now THROWS, naming FTS and + * its remedies, instead of reporting a drop that never happened and letting the + * next insert/delete die at bind time with a message that never mentions FTS. + * Nothing in the type system enforces that — callers must have already proven + * the extension is loadable (`ensureFtsRowDmlSafe()` returning `true`, or a + * direct `loadFTSExtension()`) before calling this. `run-analyze.ts` settles it + * at the incremental extension gate and escalates to a full wipe-and-rebuild + * write plan when the gate says no, so this function is only reached on the + * branch where the drops can actually succeed. + * + * @param indexRows An {@link IndexCatalogSnapshot} the caller already read on + * THIS connection with no index created or dropped since — the same freshness + * contract, and the same one-shared-`SHOW_INDEXES`-read purpose, as the gates in + * `lbug-adapter.ts`. Omit it to have the sweep read the catalog itself. */ -export async function dropSearchFTSIndexes(): Promise { +export async function dropSearchFTSIndexes(indexRows?: IndexCatalogSnapshot): Promise { + // One catalog read for the whole sweep, decided PER CONFIGURED INDEX on + // IDENTITY (#2841 cleanup review). `undefined` = the catalog could not be + // read, which proves nothing — attempt every drop rather than skip a real one, + // the same fail-closed reading `ensureFtsRowDmlSafe` applies to its own rows. + // + // Deliberately NOT an all-or-nothing early return keyed on index TYPE. That + // shape put a second `=== 'FTS'` predicate over the same rows next to the + // gate's `undefined || === 'FTS'` one, disagreeing on the polarity of an + // unreadable index_type: the gate treats it as "might be FTS" and blocks, + // while a bare `=== 'FTS'` here read it as "no FTS index anywhere" and skipped + // the entire sweep. If the row shape ever changes while FTS still loads, the + // gate would answer SAFE (surgical path), the sweep would drop nothing, and + // `deleteNodesForFiles` would run against tables carrying live FTS indexes — + // the exact #2589 hazard this sweep exists to prevent. Keying on identity + // removes the polarity question entirely. Its stated justification was also + // unreachable: the loop only ever drops the CONFIGURED `FTS_INDEXES` entries, + // so an index left over from an older, differently-named set was never dropped + // whether the sweep ran or not. + const rows = await resolveGateRows(indexRows); for (const { table, indexName } of FTS_INDEXES) { + // Skip only what the catalog POSITIVELY proves absent. Without this, a + // machine whose FTS extension cannot load, analyzing a DB that never carried + // an FTS index, pays one failed `CALL DROP_FTS_INDEX` per configured table on + // EVERY incremental run — and each of those failures now costs a fresh + // catalog read inside `dropFTSIndex`'s liveness guard, forever, with nothing + // to heal (#2841 review). The `ensuredFTSIndexes` memo needs no clearing here + // either: an index absent from the catalog cannot be memoized as ensured on + // this connection, and `createSearchFTSIndexes` drops per index itself before + // creating. + const provenAbsent = + rows !== undefined && + !rows.some((row) => indexRowTable(row) === table && indexRowName(row) === indexName); + if (provenAbsent) continue; await dropFTSIndex(table, indexName); } } @@ -239,7 +295,13 @@ export async function verifySearchFTSIndexes( for (const row of rows) { if (typeof row !== 'object' || row === null) continue; const record = row as Record; - const indexName = record.index_name; + const indexName = indexRowName(record); + // LADYBUGDB-CONTRACT: `property_names` is the one SHOW_INDEXES column with a + // single reader, so it has no shared accessor — see {@link IndexCatalogRow} + // in lbug-adapter.ts for the full column list and the re-validation rule. + // Unlike the gates, an unreadable shape here is safe: it reports the index as + // not covering its columns, i.e. "missing", which degrades keyword search + // loudly rather than passing a broken index off as verified. const propertyNames = record.property_names; if (typeof indexName !== 'string' || !Array.isArray(propertyNames)) continue; propsByIndex.set( diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index f122cddce..09695e8a4 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -201,7 +201,32 @@ export interface RepoMeta { */ capabilities?: { graph: { provider: string; status: 'available' | 'degraded' | 'unavailable' }; - fts: { provider: string; status: 'available' | 'degraded' | 'unavailable' }; + fts: { + provider: string; + status: 'available' | 'degraded' | 'unavailable'; + /** + * Why THIS run ended up without search indexes, when `status` is + * `'unavailable'` (#2841). Mirrors `AnalysisResult.ftsSkipReason` in + * core/run-analyze.ts — the same discriminator that surface already + * reports to the CLI, persisted rather than re-derived because the two + * causes need OPPOSITE handling on the next run: + * + * - `extension-unavailable` — the FTS extension could not load. Healable + * from outside the repo (install it), so the up-to-date fast path + * probes whether it loads now and re-analyzes when it does. + * - `build-failed` — the extension loaded fine and the index BUILD + * failed (e.g. one un-tokenizable pre-existing row, #2544/#2546). + * Deterministic: the same probe would "heal" it into a full + * re-analysis that degrades identically and restamps, forever. Only + * `--repair-fts` or a content change addresses it. + * + * Collapsing both into `status: 'unavailable'` is exactly what made that + * loop reachable. ABSENT on indexes written before #2841 and on the + * `--repair-fts` stamp (which writes `status: 'available'`); `undefined` + * therefore reads as "cause unknown" and keeps the pre-#2841 behaviour. + */ + skipReason?: 'extension-unavailable' | 'build-failed'; + }; vectorSearch: { provider: string; status: 'vector-index' | 'exact-scan' | 'unavailable'; diff --git a/gitnexus/test/integration/fts-extension-e2e.test.ts b/gitnexus/test/integration/fts-extension-e2e.test.ts index 94d56cd1b..d029f0b70 100644 --- a/gitnexus/test/integration/fts-extension-e2e.test.ts +++ b/gitnexus/test/integration/fts-extension-e2e.test.ts @@ -293,6 +293,56 @@ describe('unhappy path — extension missing entirely', () => { }, 180_000); }); +describe('regression — the extension disappears between analyze runs (#2841)', () => { + it('the incremental run completes with a full DB write instead of an opaque Binder exception', (ctx) => { + const { home, extensionFile } = makeHome('valid'); + const repo = makeFixtureRepo('vanishing-extension'); + + // 1. First analyze with the extension in place: the index ends up carrying + // an FTS index on every searchable table. + const first = runCli(['analyze'], repo, home, 'load-only'); + expect(first.status).toBe(0); + // This case needs run 1 to actually BUILD the indexes — without them there + // is nothing for the gate to trip on and the assertions below would be + // vacuous. When the seeded extension cannot load on this host (the same + // environment gap the other cases in this file hit), skip VISIBLY rather + // than report a red that says nothing about the fix. + if (first.output.includes('FTS extension unavailable')) { + if (REQUIRE_FTS) { + throw new Error( + 'GITNEXUS_REQUIRE_FTS=1 but the seeded FTS extension did not load — cannot verify the #2841 regression.', + ); + } + ctx.skip(); + } + + // 2. The extension becomes unloadable — the reporter moved + // ~/.lbdb/extension away; a HOME change or a wiped cache does the same. + fs.rmSync(extensionFile); + + // 3. A content change makes the next run incremental, so it must rewrite + // rows of tables that still carry the indexes from step 1. + fs.appendFileSync(path.join(repo, 'src', 'greeter.ts'), '\n// #2841 incremental touch\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: repo, stdio: 'pipe' }); + spawnSync('git', ['commit', '-m', '#2841 touch'], { cwd: repo, stdio: 'pipe', env: gitEnv }); + + const second = runCli(['analyze'], repo, home, 'load-only'); + // Pre-fix: exit 1 with "Binder exception: Trying to delete from an index on + // table File but its extension is not loaded" and no mention of FTS at all. + expect(second.status).toBe(0); + expect(second.output).not.toContain('its extension is not loaded'); + expect(second.output).toContain('full DB write'); + expect(second.output).toContain('FTS'); + }, 400_000); +}); + describe('self-heal over the network — FORCE INSTALL replaces a broken file (auto)', () => { beforeEach((ctx) => { // The platform matrix already exercises offline FTS load/diagnostic paths diff --git a/gitnexus/test/integration/lbug-delete-nodes-for-files.test.ts b/gitnexus/test/integration/lbug-delete-nodes-for-files.test.ts index d8e037ba3..6620d3b73 100644 --- a/gitnexus/test/integration/lbug-delete-nodes-for-files.test.ts +++ b/gitnexus/test/integration/lbug-delete-nodes-for-files.test.ts @@ -292,6 +292,14 @@ withTestLbugDB('delete-nodes-missing-embedding-table', () => { * legal right now?" before a single row is touched. Own withTestLbugDB block: * these cases close and reopen the DB under a different extension-install * policy, which would wreck the sibling suites' shared connection. + * + * The block also carries the FTS twins of the same seam (#2841) — + * `ensureFtsRowDmlSafe` and, through the public `dropFTSIndex`, the catalog + * read behind it. They live here rather than in a new block because they need + * the identical machinery: the policy-reopen helper above, and the + * SHOW_INDEXES interception below that is the only way to stage an unreadable + * catalog. Where the VECTOR gate may fall back to a load it does not strictly + * need, both FTS readers must fall CLOSED — see the cases themselves. */ withTestLbugDB('embedding-row-dml-vector-gate', (handle) => { describe('ensureEmbeddingRowDmlSafe (#2623)', () => { @@ -414,18 +422,19 @@ withTestLbugDB('embedding-row-dml-vector-gate', (handle) => { ).toBe(true); }, 120_000); - it('catalog read fails → falls back to attempting the extension load (fail-safe)', async () => { - // The one branch where the gate cannot cheaply prove safety: SHOW_INDEXES - // itself errors. It must fall through to loadVectorExtension — in this - // environment the extension IS loadable, so the verdict is still `true` - // and DML proceeds safely despite the unreadable catalog. - await seedTwoFilesWithEmbeddings(); - // Reopen so the module-level "already loaded" latch cannot let - // loadVectorExtension return true without issuing a LOAD statement. - await reopenWithPolicy('load-only'); - const { ensureEmbeddingRowDmlSafe } = await import('../../src/core/lbug/lbug-adapter.js'); + /** + * Run `run` with every `CALL SHOW_INDEXES()` on the writable connection + * forced to fail, passing every other statement through to the real engine + * and recording the SQL that was attempted. + * + * Forcing the read is the ONLY way to reach the gates' "could not prove + * anything" branch: `SHOW_INDEXES` is readable with no extension loaded, so + * a genuine unreadable catalog cannot be staged by configuration alone. + */ + const withUnreadableIndexCatalog = async ( + run: (seen: readonly string[]) => Promise, + ): Promise => { const { default: lbug } = await import('@ladybugdb/core'); - const originalQuery = lbug.Connection.prototype.query; const seen: string[] = []; const spy = vi.spyOn(lbug.Connection.prototype, 'query').mockImplementation(function ( @@ -441,14 +450,116 @@ withTestLbugDB('embedding-row-dml-vector-gate', (handle) => { }); try { + await run(seen); + } finally { + spy.mockRestore(); + } + }; + + it('catalog read fails → falls back to attempting the extension load (fail-safe)', async () => { + // The one branch where the gate cannot cheaply prove safety: SHOW_INDEXES + // itself errors. It must fall through to loadVectorExtension — in this + // environment the extension IS loadable, so the verdict is still `true` + // and DML proceeds safely despite the unreadable catalog. + await seedTwoFilesWithEmbeddings(); + // Reopen so the module-level "already loaded" latch cannot let + // loadVectorExtension return true without issuing a LOAD statement. + await reopenWithPolicy('load-only'); + const { ensureEmbeddingRowDmlSafe } = await import('../../src/core/lbug/lbug-adapter.js'); + + await withUnreadableIndexCatalog(async (seen) => { await expect(ensureEmbeddingRowDmlSafe()).resolves.toBe(true); // The catalog read was attempted and failed… expect(seen.some((s) => s.includes('SHOW_INDEXES'))).toBe(true); // …and the fallback really attempted the LOAD instead of guessing. expect(seen.some((s) => s.toUpperCase().includes('LOAD'))).toBe(true); - } finally { - spy.mockRestore(); - } + }); + }, 120_000); + + /** + * The FTS twin of the case above (#2841). Same seam, opposite polarity: + * `ensureEmbeddingRowDmlSafe` may fall back to a load it does not strictly + * need, but `ensureFtsRowDmlSafe`'s only job is refusing an unsafe write, so + * an unprovable catalog must never be answered "no FTS index seen ⇒ safe". + * + * Both halves are asserted because either one alone is satisfiable by a + * broken gate: the verdict alone could come from a gate that never looked at + * the extension, and the LOAD alone could come from a gate that issued it + * and then returned `true` regardless. + */ + it('FTS twin: an unreadable catalog fails CLOSED — verdict blocked, and the FTS load is attempted (#2841)', async () => { + const { ensureFtsRowDmlSafe } = await import('../../src/core/lbug/lbug-adapter.js'); + + // Half 1 — `never` makes the extension provably unloadable on every host, + // so the verdict is deterministic: a fail-OPEN gate answers `true` here. + await reopenWithPolicy('never'); + await withUnreadableIndexCatalog(async (seen) => { + await expect(ensureFtsRowDmlSafe()).resolves.toBe(false); + expect(seen.some((s) => s.includes('SHOW_INDEXES'))).toBe(true); + }); + + // Half 2 — refusing is not enough: the gate must TRY to make the write + // legal, or an unreadable catalog would escalate every run to a full + // rebuild on a machine where FTS loads perfectly. + // + // Staging that needs care. `initLbug` pre-loads FTS itself whenever the + // policy permits (lbug-adapter.ts), and the adapter latches the result, + // so a gate running after a successful init-time load issues nothing and + // the assertion would be unfalsifiable. So: reopen under `never` (init's + // pre-load is refused, latch stays clear), then relax the policy WITHOUT + // reopening — the gate resolves it from the environment at call time, so + // the LOAD that appears is unambiguously its own. `afterEach`'s + // reopenWithPolicy(undefined) restores the variable. + // + // Deliberately not asserting the verdict here: whether FTS actually + // loads is a property of the host; ATTEMPTING it is the contract. + await reopenWithPolicy('never'); + process.env.GITNEXUS_LBUG_EXTENSION_INSTALL = 'load-only'; + await withUnreadableIndexCatalog(async (seen) => { + await ensureFtsRowDmlSafe(); + expect(seen.some((s) => /^\s*LOAD EXTENSION fts\b/i.test(s))).toBe(true); + // …and it did not charge the caller a VECTOR load it never needed. + expect(seen.some((s) => /^\s*LOAD EXTENSION vector\b/i.test(s))).toBe(false); + }); + }, 120_000); + + /** + * `ftsIndexExistsInCatalog` is private; `dropFTSIndex` is the public surface + * that consumes it, and the H3 fix lives entirely in its unreadable-catalog + * branch. With the extension unloaded the DROP always fails the same way + * (`Catalog exception: function DROP_FTS_INDEX is not defined`), so the + * catalog read is the ONLY thing deciding whether the caller is told — which + * makes the two calls below a controlled pair on one connection. + */ + it('FTS twin: dropFTSIndex rejects (not resolves) when the extension is unloaded and the catalog is unreadable (#2841 H3)', async () => { + await reopenWithPolicy('never'); + const { dropFTSIndex } = await import('../../src/core/lbug/lbug-adapter.js'); + // A name no index carries: with a READABLE catalog this is provably + // "nothing to drop", which is exactly what the unreadable read must NOT + // be allowed to imply. + const ABSENT_INDEX = 'delete_nodes_2841_absent_fts'; + + // Control — catalog readable, index provably absent ⇒ benign no-op. + await expect(dropFTSIndex('File', ABSENT_INDEX)).resolves.toBeUndefined(); + + // Same call, same connection, same engine error; only the catalog read + // changes. Before the fix this ALSO resolved silently — reporting an + // unprovable catalog as "index absent" and handing the caller a drop that + // never happened, one DML statement before the #2841 crash. + await withUnreadableIndexCatalog(async (seen) => { + // The message must claim only what the run can prove. This branch is + // reached when the catalog is UNREADABLE, and the only path into it + // (`ensureFtsRowDmlSafe` waved the surgical plan through because the + // catalog showed no FTS index, then a later read failed) is one where + // the same run already established the opposite of "exists" — so + // asserting existence here would state a fabricated fact while + // failing the run. + await expect(dropFTSIndex('File', ABSENT_INDEX)).rejects.toThrow( + /FTS index '.*' on table File could not be verified as absent/, + ); + expect(seen.some((s) => s.includes('DROP_FTS_INDEX'))).toBe(true); + expect(seen.some((s) => s.includes('SHOW_INDEXES'))).toBe(true); + }); }, 120_000); }); }); diff --git a/gitnexus/test/unit/drop-fts-index-error-classification.test.ts b/gitnexus/test/unit/drop-fts-index-error-classification.test.ts index 74b43dc38..272e54f73 100644 --- a/gitnexus/test/unit/drop-fts-index-error-classification.test.ts +++ b/gitnexus/test/unit/drop-fts-index-error-classification.test.ts @@ -10,9 +10,14 @@ * specific engine failure was not achieved during investigation, but the * classifier's behavior for it is still provable from the message alone. */ -import { describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { isBenignDropFtsIndexError, dropFTSIndex } from '../../src/core/lbug/lbug-adapter.js'; import { withTestLbugDB } from '../helpers/test-indexed-db.js'; +import { createTempDir } from '../helpers/test-db.js'; +import { + resetExtensionState, + resolveAnalyzeInstallPolicy, +} from '../../src/core/lbug/extension-loader.js'; describe('isBenignDropFtsIndexError', () => { it('is true for the FTS-extension/function-not-registered catalog error (probe-verified text)', () => { @@ -64,3 +69,171 @@ withTestLbugDB('drop-fts-index-benign-cases', (handle) => { }, 120_000); }); }); + +/** + * #2841: "function DROP_FTS_INDEX is not defined" is benign only when there is + * nothing to drop. When the index is LIVE, that same message means the drop did + * not happen and cannot happen — every later insert/delete against the table + * dies at bind time with an engine error that never mentions FTS. The message + * classifier stays pure (it cannot know whether an index exists); the liveness + * question is settled inside `dropFTSIndex`, on the error path only. + */ +describe('dropFTSIndex with the FTS extension unloaded (#2841)', () => { + const TABLE = 'DropProbe2841'; + const LIVE_INDEX = 'drop_probe_2841_live'; + let ftsAvailable = true; + /** + * Mutable holder rather than `probe: … | undefined` + `probe!.dbPath`: the + * non-null assertion was a lint warning, and the alternative (a runtime guard + * in every consumer) would put branching into the test path. `beforeAll` + * overwrites both fields; if it never ran, `initLbug('')` fails loudly, which + * is the same outcome the assertion had. + */ + const probe: { dbPath: string; cleanup: () => Promise } = { + dbPath: '', + cleanup: async () => {}, + }; + + beforeAll(async () => { + const adapter = await import('../../src/core/lbug/lbug-adapter.js'); + const tmp = await createTempDir('gitnexus-2841-drop-probe-'); + probe.dbPath = tmp.dbPath; + probe.cleanup = tmp.cleanup; + await adapter.initLbug(probe.dbPath); + try { + ftsAvailable = await adapter.loadFTSExtension(undefined, { + policy: resolveAnalyzeInstallPolicy(), + }); + if (ftsAvailable) { + await adapter.executeQuery( + `CREATE NODE TABLE IF NOT EXISTS ${TABLE} (id STRING PRIMARY KEY, name STRING, content STRING)`, + ); + // A real, live FTS index — the state that makes the catalog error fatal. + await adapter.createFTSIndex(TABLE, LIVE_INDEX, ['name', 'content']); + } + } finally { + await adapter.closeLbug(); + } + }, 120_000); + + afterAll(async () => { + await probe.cleanup(); + }); + + beforeEach((ctx) => { + if (!ftsAvailable) { + if (process.env.GITNEXUS_REQUIRE_FTS === '1') { + throw new Error( + 'GITNEXUS_REQUIRE_FTS=1 but the FTS extension is unavailable — cannot verify the #2841 drop guard.', + ); + } + console.warn( + '[drop-fts-index-error-classification] Skipping the #2841 cases — FTS extension unavailable.', + ); + ctx.skip(); + } + }); + + /** Reopen the seeded DB with the extension forced unloadable for this connection. */ + const withUnloadedFts = async (run: () => Promise): Promise => { + const adapter = await import('../../src/core/lbug/lbug-adapter.js'); + const previousPolicy = process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + process.env.GITNEXUS_LBUG_EXTENSION_INSTALL = 'never'; + resetExtensionState(); + try { + await adapter.initLbug(probe.dbPath); + await run(); + } finally { + await adapter.closeLbug(); + if (previousPolicy === undefined) delete process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + else process.env.GITNEXUS_LBUG_EXTENSION_INSTALL = previousPolicy; + resetExtensionState(); + } + }; + + it('rejects, naming FTS and both remedies, when the index is live and the extension is not loaded', async () => { + await withUnloadedFts(async () => { + await expect(dropFTSIndex(TABLE, LIVE_INDEX)).rejects.toThrow( + /FTS index '.*' on table .* exists but the LadybugDB FTS extension is not loaded/, + ); + // Both remedies, asserted as stable SUBSTRINGS — the load-side half is + // generated by `diagnoseExtensionLoad` now, so pinning a whole sentence + // would break on any classifier wording change. `never` is not a load + // failure the classifier recognizes, so the diagnosis here is `unknown` + // and the doctor pointer is the load-side remedy the user gets. + await expect(dropFTSIndex(TABLE, LIVE_INDEX)).rejects.toThrow(/gitnexus doctor/); + await expect(dropFTSIndex(TABLE, LIVE_INDEX)).rejects.toThrow(/analyze --force/); + }); + }, 120_000); + + it('still resolves when the extension is not loaded and the index does not exist', async () => { + await withUnloadedFts(async () => { + await expect(dropFTSIndex(TABLE, 'drop_probe_2841_absent')).resolves.toBeUndefined(); + }); + }, 120_000); + + /** + * #2374/#2375 redaction contract. LadybugDB's own load error names the + * extension FILE, and that `reason` is what the classifier is fed — so the + * one thing this surface must never do is pass it through into the message a + * user sees. Until now that held only by code inspection. + * + * A bare "policy is never" run cannot prove it: that reason carries no path, + * so the assertion would be vacuous. So the LOAD is forced to fail with a + * real, path-bearing LadybugDB error instead, which drives the classifier + * down its `missing_dependency` branch — the branch whose remedy is derived + * from the very text that contains the path. + */ + const FORCED_EXTENSION_PATH = '/nonexistent-gitnexus-2841/fts.lbug_extension'; + const FORCED_LOAD_FAILURE = + `Failed to load library: ${FORCED_EXTENSION_PATH} which is needed by extension: fts; ` + + 'libcrypto.so.3: cannot open shared object file: No such file or directory'; + + it('routes the load-side remedy through the classifier without leaking the path LadybugDB named', async () => { + const adapter = await import('../../src/core/lbug/lbug-adapter.js'); + const { default: lbug } = await import('@ladybugdb/core'); + const previousPolicy = process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + process.env.GITNEXUS_LBUG_EXTENSION_INSTALL = 'load-only'; + resetExtensionState(); + + const originalQuery = lbug.Connection.prototype.query; + // Installed BEFORE initLbug so FTS can never load on this connection — + // otherwise the DROP would succeed and there would be no message to inspect. + const spy = vi.spyOn(lbug.Connection.prototype, 'query').mockImplementation(function ( + this: unknown, + sql: string, + ...rest: unknown[] + ) { + if (/^\s*LOAD EXTENSION fts\b/i.test(sql)) { + return Promise.reject(new Error(FORCED_LOAD_FAILURE)); + } + return originalQuery.call(this, sql, ...rest); + }); + + try { + await adapter.initLbug(probe.dbPath); + // Record the capability from the forced failure, so `dropFTSIndex` reads + // a real cached diagnosis rather than re-deriving one from nothing. + await expect(adapter.loadFTSExtension(undefined, { policy: 'load-only' })).resolves.toBe( + false, + ); + + const rejection: unknown = await dropFTSIndex(TABLE, LIVE_INDEX).catch((e: unknown) => e); + expect(rejection).toBeInstanceOf(Error); + const message = String(rejection); + + // The classifier really fired on this reason (POSIX missing-dependency), + // so the redaction assertion below is not vacuous… + expect(message).toContain('Reinstalling the extension will NOT help'); + // …and neither the path nor any other filesystem path reached the user. + expect(message).not.toContain(FORCED_EXTENSION_PATH); + expect(message).not.toMatch(/(?:[A-Za-z]:\\|\/)[^\s'"]+/); + } finally { + spy.mockRestore(); + await adapter.closeLbug(); + if (previousPolicy === undefined) delete process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + else process.env.GITNEXUS_LBUG_EXTENSION_INSTALL = previousPolicy; + resetExtensionState(); + } + }, 120_000); +}); diff --git a/gitnexus/test/unit/fts-indexes.test.ts b/gitnexus/test/unit/fts-indexes.test.ts index 259c745c6..21232fbbc 100644 --- a/gitnexus/test/unit/fts-indexes.test.ts +++ b/gitnexus/test/unit/fts-indexes.test.ts @@ -5,6 +5,19 @@ const { calls } = vi.hoisted(() => ({ calls: [] as string[] })); vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ DEFAULT_FTS_STEMMER: 'porter', + // Row accessors and the snapshot resolver are PURE — mirror the real + // implementations rather than stubbing them, or `verifySearchFTSIndexes` + // and `dropSearchFTSIndexes` read `undefined` out of every catalog row and + // the suite passes for the wrong reason. (#2841 cleanup moved these reads + // behind named accessors so the LadybugDB column contract has one home; a + // whole-module mock has to follow.) + indexRowTable: (row: Record | undefined) => row?.table_name ?? row?.[0], + indexRowName: (row: Record | undefined) => row?.index_name ?? row?.[1], + indexRowType: (row: Record | undefined) => row?.index_type ?? row?.[2], + readIndexCatalogRows: vi.fn(async () => undefined), + resolveGateRows: vi.fn(async (rows?: unknown) => + rows === undefined ? undefined : (rows as unknown[]), + ), dropFTSIndex: vi.fn(async (table: string, indexName: string) => { calls.push(`drop:${table}.${indexName}`); }), diff --git a/gitnexus/test/unit/incremental-index-extension-dml-gate.test.ts b/gitnexus/test/unit/incremental-index-extension-dml-gate.test.ts new file mode 100644 index 000000000..c0f27948e --- /dev/null +++ b/gitnexus/test/unit/incremental-index-extension-dml-gate.test.ts @@ -0,0 +1,576 @@ +/** + * #2841: an incremental writeback must decide whether row-level DML is even + * legal BEFORE it mutates a row. LadybugDB refuses every write to a table + * carrying an FTS index while the FTS extension is unloaded — at BIND time, so + * a DETACH DELETE matching zero rows fails exactly as hard as one matching + * thousands: + * + * Binder exception: Trying to delete from an index on table File but its + * extension is not loaded. + * + * and the indexes cannot be cleared in place either (`DROP_FTS_INDEX` is itself + * an FTS-extension function; LadybugDB has no SQL `DROP INDEX`). So a DB that + * carries FTS indexes on a machine where the extension stopped loading used to + * kill every incremental analyze mid-writeback, with an engine message that + * never mentions FTS. + * + * The fix mirrors the VECTOR gate (#2623): probe the index catalog first, load + * FTS with the analyze policy only when an index actually gates DML, and fall + * through to the escalation valve's wipe-and-bulk-COPY plan when it cannot be + * loaded. The VECTOR half of that behaviour is covered by + * `incremental-vector-extension-ordering.test.ts`; this suite covers the FTS + * half plus the both-extensions-blocked case, where the reason log has to name + * both causes rather than only the first one checked. + * + * The escalation is a valve, not a wipe switch, so three of its consequences + * are pinned here as well: + * - it must NOT rescue embeddings on the one run whose purpose is to destroy + * them (`--drop-embeddings`, review H1) — and must rescue them on every + * other forced rebuild (the complement, asserted in the both-blocked case); + * - it must be ONE-SHOT: the next run on a machine where FTS loads again goes + * back to surgery and rebuilds the search indexes, rather than escalating + * forever; + * - an extension-forced rebuild is environmental, not repo churn, so it builds + * into a staging file beside the live index and publishes it with one rename + * (review H2) — an interrupted rebuild must leave the current index intact. + * + * That rebuild stamps `lastCommit`, so a plain rerun on an unchanged tree takes + * the `alreadyUpToDate` fast path and the search indexes stay missing. That is + * addressed in the CLI's advice (`--repair-fts`, which rebuilds the indexes + * without re-parsing), NOT by an auto-heal probe here — one was tried and + * reverted for re-analyzing the whole repo on every run in the build-failed + * case, for opening the live index on the millisecond fast path, and for + * breaking the fast-path invariant `analyzer-identity-cli.test.ts` pins. + */ +import { readFile, readdir, writeFile } from 'fs/promises'; +import { execSync } from 'child_process'; +import path from 'path'; +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { TestContext } from 'vitest'; +import { setupMiniRepo } from '../helpers/mini-repo.js'; +import { seedEmbeddingsForFiles } from '../helpers/embedding-seed.js'; +import { getStoragePaths } from '../../src/storage/repo-manager.js'; +import { createTempDir } from '../helpers/test-db.js'; +import { FTS_INDEXES } from '../../src/core/search/fts-schema.js'; +import { EMBEDDING_TABLE_NAME } from '../../src/core/lbug/schema.js'; +import { resolveAnalyzeInstallPolicy } from '../../src/core/lbug/extension-loader.js'; + +const ftsMustBeAvailable = process.env.GITNEXUS_REQUIRE_FTS === '1'; +const vectorMustBeAvailable = process.env.GITNEXUS_REQUIRE_VECTOR === '1'; + +const commitAll = (cwd: string, message: string): void => { + execSync('git -c user.name=test -c user.email=t@t -c commit.gpgsign=false add -A', { + cwd, + stdio: 'pipe', + }); + execSync( + `git -c user.name=test -c user.email=t@t -c commit.gpgsign=false commit -q -m "${message}"`, + { cwd, stdio: 'pipe' }, + ); +}; + +/** + * Append a line to a mini-repo file and commit it — a one-file write set. + * `relPath` is POSIX-joined for the graph side but filesystem-joined for the + * write, so callers can target a file the NEXT run will not touch. + */ +const touchAndCommit = async ( + repoPath: string, + marker: string, + relPath = 'src/handler.ts', +): Promise => { + const filePath = path.join(repoPath, ...relPath.split('/')); + await writeFile(filePath, (await readFile(filePath, 'utf-8')) + `\n// ${marker}\n`, 'utf-8'); + commitAll(repoPath, marker); +}; + +const readFtsIndexRows = async (lbugPath: string): Promise>> => { + const lbugAdapter = await import('../../src/core/lbug/lbug-adapter.js'); + await lbugAdapter.initLbug(lbugPath); + try { + const rows = (await lbugAdapter.executeQuery('CALL SHOW_INDEXES() RETURN *')) as Array< + Record + >; + return rows.filter((r) => r.index_type === 'FTS'); + } finally { + await lbugAdapter.closeLbug(); + } +}; + +/** Every File node in the published graph, as rows (duplicates stay visible). */ +const readGraphFileRows = async ( + lbugPath: string, +): Promise> => { + const lbugAdapter = await import('../../src/core/lbug/lbug-adapter.js'); + await lbugAdapter.initLbug(lbugPath); + try { + const rows = (await lbugAdapter.executeQuery( + `MATCH (f:File) RETURN f.filePath AS filePath, f.content AS content`, + )) as Array<{ filePath: string; content: string }>; + return rows.map((r) => ({ filePath: String(r.filePath), content: String(r.content) })); + } finally { + await lbugAdapter.closeLbug(); + } +}; + +const contentsByPath = ( + rows: ReadonlyArray<{ filePath: string; content: string }>, +): Map => new Map(rows.map((r) => [r.filePath, r.content])); + +/** Surviving CodeEmbedding nodeIds, read straight from the published DB. */ +const readEmbeddingRows = async (lbugPath: string): Promise => { + const lbugAdapter = await import('../../src/core/lbug/lbug-adapter.js'); + await lbugAdapter.initLbug(lbugPath); + try { + const rows = (await lbugAdapter.executeQuery( + `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId`, + )) as Array<{ nodeId: string }>; + return rows.map((r) => String(r.nodeId)); + } finally { + await lbugAdapter.closeLbug(); + } +}; + +/** `count(e)` straight from the engine — the H1 wipe has to be proven at zero. */ +const countEmbeddingRows = async (lbugPath: string): Promise => { + const lbugAdapter = await import('../../src/core/lbug/lbug-adapter.js'); + await lbugAdapter.initLbug(lbugPath); + try { + const rows = (await lbugAdapter.executeQuery( + `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN count(e) AS total`, + )) as Array<{ total: number | bigint }>; + expect(rows.length).toBe(1); + return Number(rows[0]?.total); + } finally { + await lbugAdapter.closeLbug(); + } +}; + +/** Staging builds sit beside the live index as `lbug.staging.` (#2658). */ +const readStagingEntries = async (storagePath: string): Promise => + (await readdir(storagePath)).filter((name) => name.includes('.staging.')); + +/** + * Make every optional extension unloadable for the next run. The env policy is + * what actually blocks the load (`ExtensionManager.ensure` short-circuits on + * `'never'` before it consults any cache); `resetExtensionState()` clears the + * process-wide capability/install memo so the run reports its own verdict + * rather than one an earlier test in this file settled. + */ +const blockExtensionLoads = async (): Promise => { + process.env.GITNEXUS_LBUG_EXTENSION_INSTALL = 'never'; + const { resetExtensionState } = await import('../../src/core/lbug/extension-loader.js'); + resetExtensionState(); +}; + +const restoreExtensionPolicy = async (previous: string | undefined): Promise => { + if (previous === undefined) delete process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + else process.env.GITNEXUS_LBUG_EXTENSION_INSTALL = previous; + const { resetExtensionState } = await import('../../src/core/lbug/extension-loader.js'); + resetExtensionState(); +}; + +describe('runFullAnalysis incremental writeback — extension-gated DML decided before any DML (#2841)', () => { + let ftsAvailable = true; + let vectorAvailable = true; + let skipWarned = false; + let vectorSkipWarned = false; + + beforeAll(async () => { + const lbugAdapter = await import('../../src/core/lbug/lbug-adapter.js'); + // Cheap standalone probe, matching the #2589/#2623 suites: settle + // availability once, up front, not inside the expensive test body. BOTH + // extensions are probed on the one throwaway connection (review H6): the + // both-blocked case builds a REAL HNSW index and asserts it was built, so + // gating that case on the FTS probe alone made a hard assertion fail on + // every host that has FTS but not VECTOR. + const probe = await createTempDir('gitnexus-2841-extension-probe-'); + try { + await lbugAdapter.initLbug(probe.dbPath); + const policy = resolveAnalyzeInstallPolicy(); + ftsAvailable = await lbugAdapter.loadFTSExtension(undefined, { policy }); + vectorAvailable = await lbugAdapter.loadVectorExtension(undefined, { policy }); + } finally { + await lbugAdapter.closeLbug(); + await probe.cleanup(); + } + }, 120_000); + + // Skip VISIBLY: a silent `return` would report a false pass and hide the + // regression in exactly the environments least likely to notice. + beforeEach((ctx) => { + if (!ftsAvailable) { + if (ftsMustBeAvailable) { + throw new Error( + 'GITNEXUS_REQUIRE_FTS=1 but the FTS extension is unavailable — cannot verify the #2841 gate.', + ); + } + if (!skipWarned) { + skipWarned = true; + console.warn( + '[incremental-index-extension-dml-gate] Skipping — the LadybugDB FTS extension is unavailable.', + ); + } + ctx.skip(); + } + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + /** + * Per-test VECTOR gate (review H6). Only the both-blocked case needs VECTOR; + * the other cases must keep running on an FTS-only host, so this is called + * from that one test body instead of widening the suite-level `beforeEach`. + * Same visibility contract as the FTS gate: a real skip, and a hard failure + * under `GITNEXUS_REQUIRE_VECTOR=1`. + */ + const skipUnlessVectorAvailable = (ctx: TestContext): void => { + if (vectorAvailable) return; + if (vectorMustBeAvailable) { + throw new Error( + 'GITNEXUS_REQUIRE_VECTOR=1 but the VECTOR extension is unavailable — cannot verify the #2841 both-blocked escalation.', + ); + } + if (!vectorSkipWarned) { + vectorSkipWarned = true; + console.warn( + '[incremental-index-extension-dml-gate] Skipping the both-blocked case — the LadybugDB VECTOR extension is unavailable.', + ); + } + ctx.skip(); + }; + + it('keeps the surgical write plan (and the indexes) when FTS is available', async () => { + const { runFullAnalysis } = await import('../../src/core/run-analyze.js'); + + const repo = await setupMiniRepo('gitnexus-2841-fts-available-'); + try { + await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} }); + await touchAndCommit(repo.dbPath, '#2841 healthy-path touch'); + + const logs: string[] = []; + await expect( + runFullAnalysis( + repo.dbPath, + { skipAgentsMd: true }, + { onProgress: () => {}, onLog: (m: string) => logs.push(m) }, + ), + ).resolves.toBeDefined(); + + // No escalation: a one-file write set on a 7-file repo stays surgical, + // and the gate must not manufacture a rebuild when FTS loads fine. + expect(logs.some((m) => m.includes('full DB write'))).toBe(false); + const { lbugPath } = getStoragePaths(repo.dbPath); + expect((await readFtsIndexRows(lbugPath)).length).toBe(FTS_INDEXES.length); + } finally { + await repo.cleanup(); + } + }, 300_000); + + it('does not escalate — or touch the extension machinery — when the DB never carried FTS indexes', async () => { + const { runFullAnalysis } = await import('../../src/core/run-analyze.js'); + + const repo = await setupMiniRepo('gitnexus-2841-fts-never-built-'); + const previousPolicy = process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + try { + // Both runs are FTS-less, so no index is ever created. The catalog-first + // check must settle this without gating the surgical plan — otherwise + // every incremental analyze on an FTS-less machine would become a full + // rebuild. + await blockExtensionLoads(); + await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} }); + const { lbugPath } = getStoragePaths(repo.dbPath); + expect((await readFtsIndexRows(lbugPath)).length).toBe(0); + + await touchAndCommit(repo.dbPath, '#2841 never-built touch'); + + const logs: string[] = []; + await expect( + runFullAnalysis( + repo.dbPath, + { skipAgentsMd: true }, + { onProgress: () => {}, onLog: (m: string) => logs.push(m) }, + ), + ).resolves.toBeDefined(); + expect(logs.some((m) => m.includes('full DB write'))).toBe(false); + + // Test gap 7: "no escalation log" alone would also pass if the surgical + // write silently did nothing. Prove the write actually landed — the same + // File.content check the blocked-path case makes. REVERSION: make + // `ensureFtsRowDmlSafe` fall OPEN on an index row whose type cannot be + // read (`indexType === undefined || indexType === 'FTS'` → `=== 'FTS'`, + // review §6.A) and this file still has no FTS index, so the no-escalation + // half keeps passing while a genuinely blocked DML would reach the engine. + const contents = contentsByPath(await readGraphFileRows(lbugPath)); + expect(contents.get('src/handler.ts')).toContain('#2841 never-built touch'); + } finally { + await restoreExtensionPolicy(previousPolicy); + await repo.cleanup(); + } + }, 300_000); + + it('names every blocked extension when both FTS and VECTOR gate the write', async (ctx) => { + skipUnlessVectorAvailable(ctx); + const lbugAdapter = await import('../../src/core/lbug/lbug-adapter.js'); + const { runFullAnalysis } = await import('../../src/core/run-analyze.js'); + + const repo = await setupMiniRepo('gitnexus-2841-both-blocked-'); + const previousPolicy = process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + try { + await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} }); + const { lbugPath } = getStoragePaths(repo.dbPath); + + // POSIX literal for the graph-side path: filePaths are stored with + // forward slashes on every OS (see the note in the #2623 suite). + // Deliberately NOT stampEmbeddingCount: meta reports zero embeddings + // while the DB holds these rows, which is exactly the state the forced + // rebuild's rescue read exists for. + const seeded = await seedEmbeddingsForFiles(repo.dbPath, ['src/handler.ts'], 2); + const seededIds = seeded.get('src/handler.ts') ?? []; + expect(seededIds.length).toBeGreaterThan(0); + await lbugAdapter.initLbug(lbugPath); + const vectorIndexBuilt = await lbugAdapter.createVectorIndex(); + await lbugAdapter.closeLbug(); + // Hard assertion, not an environment gap: `skipUnlessVectorAvailable` + // above already proved VECTOR loads on this host (review H6). + expect(vectorIndexBuilt).toBe(true); + expect((await readFtsIndexRows(lbugPath)).length).toBe(FTS_INDEXES.length); + + await touchAndCommit(repo.dbPath, '#2841 both-blocked touch'); + + await blockExtensionLoads(); + const logs: string[] = []; + await expect( + runFullAnalysis( + repo.dbPath, + { skipAgentsMd: true }, + { onProgress: () => {}, onLog: (m: string) => logs.push(m) }, + ), + ).resolves.toBeDefined(); + + // One escalation, both causes named. Reporting only the first checked + // extension is how a half-diagnosed failure survives a bug report. + const escalation = logs.filter((m) => m.includes('full DB write')); + expect(escalation.length).toBe(1); + expect(escalation[0]).toContain('FTS'); + expect(escalation[0]).toContain('VECTOR'); + + // Test gap 5 — the COMPLEMENT of the `--drop-embeddings` case below: this + // run never asked to touch embeddings, so the wipe must not eat the rows + // meta failed to account for. REVERSION: delete the + // `if (extensionForcedRebuild && !options.dropEmbeddings && + // cachedEmbeddings.length === 0)` rescue in run-analyze.ts and every + // seeded row is destroyed by a rebuild the operator did not ask for, + // while the run still exits 0. + const surviving = await readEmbeddingRows(lbugPath); + const survivingIds = new Set(surviving); + for (const id of seededIds) { + expect(survivingIds.has(id)).toBe(true); + } + // …and exactly once each — the restore must not double-insert. + expect(surviving.length).toBe(survivingIds.size); + expect(logs.some((m) => m.includes('Preserving'))).toBe(true); + } finally { + await restoreExtensionPolicy(previousPolicy); + await repo.cleanup(); + } + }, 300_000); + + it('lets --drop-embeddings wipe unaccounted embeddings instead of rescuing them (review H1)', async () => { + const { runFullAnalysis } = await import('../../src/core/run-analyze.js'); + + const repo = await setupMiniRepo('gitnexus-2841-drop-embeddings-'); + const previousPolicy = process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + try { + await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} }); + const { lbugPath } = getStoragePaths(repo.dbPath); + expect((await readFtsIndexRows(lbugPath)).length).toBe(FTS_INDEXES.length); + + // Real rows and NO stampEmbeddingCount, so meta reports zero embeddings + // while the DB holds these — the exact trigger state for the forced + // rebuild's rescue read. `--drop-embeddings` leaves `cachedEmbeddings` + // empty by construction (`deriveEmbeddingMode` returns + // `shouldLoadCache: false`), and without a checkpoint the run stays + // incremental, so it arrives at the gate looking precisely like the case + // the rescue was written for. + const seeded = await seedEmbeddingsForFiles(repo.dbPath, ['src/handler.ts'], 2); + expect((seeded.get('src/handler.ts') ?? []).length).toBeGreaterThan(0); + + await touchAndCommit(repo.dbPath, '#2841 drop-embeddings touch'); + await blockExtensionLoads(); + + const logs: string[] = []; + await expect( + runFullAnalysis( + repo.dbPath, + { skipAgentsMd: true, dropEmbeddings: true }, + { onProgress: () => {}, onLog: (m: string) => logs.push(m) }, + ), + ).resolves.toBeDefined(); + + // The FTS block still forces the rebuild — this is the same escalation, + // reached with the one flag whose entire purpose is to destroy the rows + // the rescue would restore. + expect(logs.some((m) => m.includes('full DB write'))).toBe(true); + // REVERSION: drop `!options.dropEmbeddings` from the rescue predicate in + // run-analyze.ts (`if (extensionForcedRebuild && !options.dropEmbeddings + // && cachedEmbeddings.length === 0)`) and the rescue reads the rows back + // out of the DB, logs `Preserving N embedding row(s) across the forced + // rebuild` on top of this run's own drop, and Phase 3.5 re-inserts every + // one of them — `--drop-embeddings` silently becomes a no-op. + expect(logs.some((m) => m.includes('Preserving'))).toBe(false); + expect(await countEmbeddingRows(lbugPath)).toBe(0); + } finally { + await restoreExtensionPolicy(previousPolicy); + await repo.cleanup(); + } + }, 300_000); + + it('escalates once: the next run on a healthy host goes back to surgery and rebuilds the FTS indexes', async () => { + const { runFullAnalysis } = await import('../../src/core/run-analyze.js'); + + const repo = await setupMiniRepo('gitnexus-2841-one-shot-'); + const previousPolicy = process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + try { + await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} }); + const { lbugPath } = getStoragePaths(repo.dbPath); + expect((await readFtsIndexRows(lbugPath)).length).toBe(FTS_INDEXES.length); + + // Run 2: FTS blocked → the forced rebuild, which leaves a DB with no FTS + // index at all and stamps `capabilities.fts.status = 'unavailable'`. + await touchAndCommit(repo.dbPath, '#2841 one-shot escalated touch'); + await blockExtensionLoads(); + const escalatedLogs: string[] = []; + await expect( + runFullAnalysis( + repo.dbPath, + { skipAgentsMd: true }, + { onProgress: () => {}, onLog: (m: string) => escalatedLogs.push(m) }, + ), + ).resolves.toBeDefined(); + expect(escalatedLogs.some((m) => m.includes('full DB write'))).toBe(true); + expect((await readFtsIndexRows(lbugPath)).length).toBe(0); + // The reason must be stated in FTS terms before the plan switches — the + // whole issue is that the pre-fix crash ("Trying to delete from an index + // on table File but its extension is not loaded") named no extension at + // all. The both-blocked case asserts this on the escalation line itself, + // but it is VECTOR-gated, so an FTS-only host would lose the property + // entirely without this check. + expect(escalatedLogs.some((m) => m.includes('FTS'))).toBe(true); + // The wipe-and-bulk-COPY republished each file exactly once. Asserted on + // ROWS, not through `contentsByPath`: that Map collapses duplicates, so a + // rebuild that appended a stale twin beside the fresh row would slip past + // every content check in this suite. + const escalatedRows = await readGraphFileRows(lbugPath); + expect(escalatedRows.filter((r) => r.filePath === 'src/handler.ts').length).toBe(1); + + // Run 3: FTS loads again and a DIFFERENT file changes. Nothing may carry + // the escalation forward — the catalog-first gate sees no FTS index, so + // the surgical plan stands, and Phase 3 rebuilds the whole index set. + await restoreExtensionPolicy(previousPolicy); + await touchAndCommit(repo.dbPath, '#2841 one-shot healed touch', 'src/validator.ts'); + const healedLogs: string[] = []; + await expect( + runFullAnalysis( + repo.dbPath, + { skipAgentsMd: true }, + { onProgress: () => {}, onLog: (m: string) => healedLogs.push(m) }, + ), + ).resolves.toBeDefined(); + + // The property that makes the whole design a one-shot rebuild rather than + // a permanent regression: no second escalation, and keyword search is + // whole again. REVERSION: make the extension-forced escalation sticky + // (e.g. keep escalating while `capabilities.fts.status === 'unavailable'`, + // or have `ensureFtsRowDmlSafe` answer from that stamp instead of the + // catalog) and this run escalates again, forever. + expect(healedLogs.some((m) => m.includes('full DB write'))).toBe(false); + expect((await readFtsIndexRows(lbugPath)).length).toBe(FTS_INDEXES.length); + + // Run 2's work survived into run 3's surgical write — i.e. the escalated + // rebuild was really published at the canonical path, not left behind in + // a staging file (review H2). handler.ts is untouched by run 3, so its + // marker can only come from the run that escalated. + const contents = contentsByPath(await readGraphFileRows(lbugPath)); + expect(contents.get('src/handler.ts')).toContain('#2841 one-shot escalated touch'); + expect(contents.get('src/validator.ts')).toContain('#2841 one-shot healed touch'); + } finally { + await restoreExtensionPolicy(previousPolicy); + await repo.cleanup(); + } + }, 300_000); + + it('builds an extension-forced rebuild into a staging file and swaps it in (review H2)', async () => { + const lbugAdapter = await import('../../src/core/lbug/lbug-adapter.js'); + const { runFullAnalysis } = await import('../../src/core/run-analyze.js'); + + const repo = await setupMiniRepo('gitnexus-2841-staged-rebuild-'); + const previousPolicy = process.env.GITNEXUS_LBUG_EXTENSION_INSTALL; + try { + await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} }); + const { lbugPath, storagePath } = getStoragePaths(repo.dbPath); + expect((await readFtsIndexRows(lbugPath)).length).toBe(FTS_INDEXES.length); + // Run 1 was a full rebuild, which always stages — so any staging entry + // observed below belongs to the escalated run, not to a leftover. + expect(await readStagingEntries(storagePath)).toEqual([]); + + await touchAndCommit(repo.dbPath, '#2841 staged-rebuild touch'); + await blockExtensionLoads(); + + // Observe the build target at the exact moment the full graph is COPYed + // in. The escalated run reaches `loadGraphToLbug` immediately after + // `wipeLbugDbFiles(buildPath)` + `initLbug(buildPath)`, so the presence + // of a `lbug.staging.` file there IS the build target. + let stagingDuringBuild: string[] | undefined; + const originalLoadGraphToLbug = lbugAdapter.loadGraphToLbug; + vi.spyOn(lbugAdapter, 'loadGraphToLbug').mockImplementation( + async (...args: Parameters) => { + stagingDuringBuild ??= await readStagingEntries(storagePath); + return originalLoadGraphToLbug(...args); + }, + ); + + const logs: string[] = []; + await expect( + runFullAnalysis( + repo.dbPath, + { skipAgentsMd: true }, + { onProgress: () => {}, onLog: (m: string) => logs.push(m) }, + ), + ).resolves.toBeDefined(); + expect(logs.some((m) => m.includes('full DB write'))).toBe(true); + + // REVERSION: delete the `if (!useAtomicSwap && (posixSwap || + // windowsSwapOk)) { useAtomicSwap = true; buildPath = + // `${lbugPath}.staging.${randomUUID()}` }` block in run-analyze.ts and + // `buildPath` stays the LIVE `lbug` file — frozen ~440 lines earlier while + // the run was still classified incremental — so the wipe destroys the only + // complete index before the COPY starts and this list is empty. + expect(stagingDuringBuild).toBeDefined(); + const observedStaging = stagingDuringBuild ?? []; + // Platform-gated, matching the production predicate exactly: the upgrade + // requires `posixSwap || windowsSwapOk`, and `windowsSwapOk` is opt-in via + // GITNEXUS_ATOMIC_WINDOWS_SWAP=1 (#2614 keeps the default Windows analyze + // on the proven in-place path). So on Windows without that flag the run + // correctly does NOT stage, and asserting otherwise fails for a reason + // that says nothing about #2841 — which is exactly what the cross-platform + // matrix caught when this assertion was written platform-blind. + const expectsStaging = + process.platform !== 'win32' || process.env.GITNEXUS_ATOMIC_WINDOWS_SWAP === '1'; + expect(observedStaging.length > 0).toBe(expectsStaging); + expect(observedStaging.every((name) => name.startsWith('lbug.staging.'))).toBe(true); + + // Published, not orphaned: the rename put the rebuild at the canonical + // path, nothing `.staging.` is left beside it, and the live index answers + // a query carrying this run's content. + expect(await readStagingEntries(storagePath)).toEqual([]); + const contents = contentsByPath(await readGraphFileRows(lbugPath)); + expect(contents.get('src/handler.ts')).toContain('#2841 staged-rebuild touch'); + } finally { + await restoreExtensionPolicy(previousPolicy); + await repo.cleanup(); + } + }, 300_000); +}); diff --git a/gitnexus/vitest.config.ts b/gitnexus/vitest.config.ts index 3de094b5f..d79db9fb4 100644 --- a/gitnexus/vitest.config.ts +++ b/gitnexus/vitest.config.ts @@ -101,6 +101,20 @@ export default defineConfig({ 'test/integration/impact-ambiguous-blast-radius.test.ts', 'test/unit/incremental-dirty-recovery.test.ts', 'test/unit/incremental-orchestration.test.ts', + // #2841. Native @ladybugdb/core: it runs real analyses, reopens the + // DB under different extension-install policies, and reads + // SHOW_INDEXES on the writable connection — exactly the mmap + // file-lock exposure this project exists to serialize (TESTING.md + // § Vitest projects). Registering it here does NOT narrow where it + // runs: vitest applies `--shard` once to the combined cross-project + // spec list (PerfSequencer/assignShards is a complete, disjoint + // partition), and run-cross-platform.ts hands vitest explicit file + // paths, which resolve against every project's include list. Its + // `incremental-vector-extension-ordering` / + // `incremental-fts-drop-ordering` siblings are equally native and + // still sit in `default` — pre-existing drift, deliberately left + // alone here. + 'test/unit/incremental-index-extension-dml-gate.test.ts', ], fileParallelism: false, sequence: { groupOrder: 1 }, @@ -152,6 +166,9 @@ export default defineConfig({ 'test/integration/impact-ambiguous-blast-radius.test.ts', 'test/unit/incremental-dirty-recovery.test.ts', 'test/unit/incremental-orchestration.test.ts', + // Excluded here because it is included by `lbug-db` above; a file + // in two projects would be collected (and run) twice. + 'test/unit/incremental-index-extension-dml-gate.test.ts', ], }, },