Merge pull request #2598 from magyargergo/repro/issue-2589

fix(analyze): drop FTS indexes before the incremental DETACH DELETE

gitnexus/src/core/lbug/lbug-adapter.ts

@@ -2904,7 +2904,30 @@ export const queryFTS = async (
 };
 
 /**
- * Drop an FTS index
+ * True for the two benign "nothing to drop" `DROP_FTS_INDEX` failures —
+ * both catalog/binder exceptions, LadybugDB's classes for "this name isn't
+ * bound to anything right now" (probe-verified end-to-end through
+ * `dropFTSIndex`'s real `conn.query()` path against @ladybugdb/core
+ * 0.18.x): the named index was never created (`Binder exception: Table <T>
+ * doesn't have an index with name <name>.`), or the FTS extension/function
+ * isn't registered at all (`Catalog exception: function DROP_FTS_INDEX is
+ * not defined...`). A real engine failure — e.g. the `Runtime exception:
+ * FTS index '<name>' is inconsistent: ...` class from #2589 — is a
+ * DIFFERENT exception class (an execution-time failure, not a catalog/bind
+ * lookup miss), so this returns false for it. Anchored to the START of the
+ * message (not a bare substring search): every probed LadybugDB error leads
+ * with its exception class, and anchoring means a future message that merely
+ * mentions "Binder exception" or "Catalog exception" further in in the body
+ * of an otherwise-genuine failure can't be misclassified as benign. Pure
+ * string logic so it is unit-testable without a native LadybugDB connection.
+ */
+export const isBenignDropFtsIndexError = (message: string): boolean =>
+  message.startsWith('Binder exception:') || message.startsWith('Catalog exception:');
+
+/**
+ * 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.
  */
 export const dropFTSIndex = async (tableName: string, indexName: string): Promise<void> => {
   if (!conn) {
@@ -2913,8 +2936,11 @@ export const dropFTSIndex = async (tableName: string, indexName: string): Promis
 
   try {
     await queryAndDrain(conn, `CALL DROP_FTS_INDEX('${tableName}', '${indexName}')`);
-  } catch {
-    // Index may not exist
+  } catch (e: unknown) {
+    const msg = e instanceof Error ? e.message : String(e);
+    if (!isBenignDropFtsIndexError(msg)) {
+      throw e;
+    }
   } finally {
     ensuredFTSIndexes.delete(ftsIndexKey(tableName, indexName));
   }

gitnexus/src/core/run-analyze.ts

@@ -39,6 +39,7 @@ import { escapeCypherString } from './lbug/cypher-escape.js';
 import {
   buildSearchIndexesOrDegrade,
   createSearchFTSIndexes,
+  dropSearchFTSIndexes,
   initialiseSearchFTSStemmer,
   verifySearchFTSIndexes,
 } from './search/fts-indexes.js';
@@ -1661,7 +1662,20 @@ export async function runFullAnalysis(
           progress('lbug', pct, msg);
         });
       } else {
-        // 1a. Remove the write set's existing rows — batched (#2409): one
+        // 1a. Drop every FTS index before touching a single row (#2589).
+        //     `deleteNodesForFiles` below DETACH DELETEs rows out of tables
+        //     that otherwise still carry the FTS index built at the end of
+        //     the PREVIOUS analyze run — Phase 3 doesn't drop+rebuild it
+        //     until well after this delete completes. LadybugDB's FTS
+        //     extension is not proven to survive DML against an indexed
+        //     table (its own docs never demonstrate it), and that ordering
+        //     is exactly what produced "FTS index 'file_fts' is
+        //     inconsistent: term is missing during delete". Dropping first
+        //     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();
+        // 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
         //     single-row write transactions on a ~700-file write set — which

gitnexus/src/core/search/fts-indexes.ts

@@ -121,6 +121,20 @@ export function getSearchFTSStemmer(): string {
   return resolvedStemmer ?? resolveFTSStemmer();
 }
 
+/**
+ * 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.
+ */
+export async function dropSearchFTSIndexes(): Promise<void> {
+  for (const { table, indexName } of FTS_INDEXES) {
+    await dropFTSIndex(table, indexName);
+  }
+}
+
 export async function createSearchFTSIndexes(
   options?: CreateSearchFTSIndexesOptions,
 ): Promise<void> {

gitnexus/test/unit/drop-fts-index-error-classification.test.ts

@@ -0,0 +1,66 @@
+/**
+ * #2589: `dropFTSIndex` must tolerate only benign "nothing to drop"
+ * `DROP_FTS_INDEX` failures and rethrow everything else — previously it
+ * swallowed every error unconditionally, which could mask a genuinely
+ * corrupted FTS index across analyze runs.
+ *
+ * `isBenignDropFtsIndexError` is pure string logic (no native connection
+ * needed), so the classification itself is unit-tested directly, including
+ * against the exact reported #2589 error text — a native repro of that
+ * 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 { isBenignDropFtsIndexError, dropFTSIndex } from '../../src/core/lbug/lbug-adapter.js';
+import { withTestLbugDB } from '../helpers/test-indexed-db.js';
+
+describe('isBenignDropFtsIndexError', () => {
+  it('is true for the FTS-extension/function-not-registered catalog error (probe-verified text)', () => {
+    expect(
+      isBenignDropFtsIndexError(
+        "Catalog exception: function DROP_FTS_INDEX is not defined. This function exists in the FTS extension. You can install and load the extension by running 'INSTALL FTS; LOAD EXTENSION FTS;'.",
+      ),
+    ).toBe(true);
+  });
+
+  it('is true for the index-never-created binder error (probe-verified against the real dropFTSIndex path)', () => {
+    expect(
+      isBenignDropFtsIndexError(
+        "Binder exception: Table File doesn't have an index with name file_fts.",
+      ),
+    ).toBe(true);
+  });
+
+  it('is false for the #2589 runtime inconsistency error (must surface, not be swallowed)', () => {
+    expect(
+      isBenignDropFtsIndexError(
+        "Runtime exception: FTS index 'file_fts' is inconsistent: term 'wiki' is missing during delete.",
+      ),
+    ).toBe(false);
+  });
+
+  it('is false for an unrelated failure', () => {
+    expect(isBenignDropFtsIndexError('Connection Exception: database is closed')).toBe(false);
+  });
+
+  it('is false for a genuine failure that merely mentions "Binder exception" mid-message (anchored, not a bare substring match)', () => {
+    expect(
+      isBenignDropFtsIndexError(
+        'Runtime exception: internal state corrupted while processing Binder exception: recovery failed.',
+      ),
+    ).toBe(false);
+  });
+});
+
+withTestLbugDB('drop-fts-index-benign-cases', (handle) => {
+  describe('dropFTSIndex end-to-end benign cases (#2589)', () => {
+    it('resolves cleanly when the named index was never created', async () => {
+      void handle;
+      const { executeQuery } = await import('../../src/core/lbug/lbug-adapter.js');
+      await executeQuery(
+        `CREATE NODE TABLE IF NOT EXISTS DropProbe (id STRING PRIMARY KEY, content STRING)`,
+      );
+      await expect(dropFTSIndex('DropProbe', 'drop_probe_never_created')).resolves.toBeUndefined();
+    }, 120_000);
+  });
+});

gitnexus/test/unit/incremental-fts-drop-ordering.test.ts

@@ -0,0 +1,138 @@
+/**
+ * #2589: the incremental writeback must drop every FTS index BEFORE
+ * `deleteNodesForFiles` runs its batched DETACH DELETE — not only in
+ * Phase 3, after the delete already ran against a table still carrying the
+ * PREVIOUS run's index. This drives the real `runFullAnalysis` incremental
+ * path (real git repo, real LadybugDB, real FTS extension) and asserts,
+ * at the moment `deleteNodesForFiles` is invoked, that `SHOW_INDEXES()`
+ * already reports every FTS index absent — proving the drop-before-delete
+ * ordering end-to-end rather than only unit-testing the call sequence.
+ */
+import { readFile, writeFile } from 'fs/promises';
+import { execSync } from 'child_process';
+import path from 'path';
+import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';
+import { setupMiniRepo } from '../helpers/mini-repo.js';
+import { getStoragePaths } from '../../src/storage/repo-manager.js';
+import { FTS_INDEXES } from '../../src/core/search/fts-schema.js';
+import { createTempDir } from '../helpers/test-db.js';
+import { resolveAnalyzeInstallPolicy } from '../../src/core/lbug/extension-loader.js';
+
+const ftsMustBeAvailable = process.env.GITNEXUS_REQUIRE_FTS === '1';
+
+describe('runFullAnalysis incremental writeback — FTS drop-before-delete ordering (#2589)', () => {
+  let ftsAvailable = true;
+  let skipWarned = false;
+
+  beforeAll(async () => {
+    const lbugAdapter = await import('../../src/core/lbug/lbug-adapter.js');
+    // Cheap standalone probe — matches the withTestLbugDB/lbug-vector-extension
+    // convention of checking availability once, up front, rather than deep
+    // inside the (expensive) test body.
+    const probe = await createTempDir('gitnexus-2589-fts-probe-');
+    try {
+      await lbugAdapter.initLbug(probe.dbPath);
+      ftsAvailable = await lbugAdapter.loadFTSExtension(undefined, {
+        policy: resolveAnalyzeInstallPolicy(),
+      });
+    } finally {
+      await lbugAdapter.closeLbug();
+      await probe.cleanup();
+    }
+  }, 120_000);
+
+  // Skip VISIBLY (ctx.skip() marks the test as skipped, not passed) when the
+  // extension is unavailable — silently `return`ing from inside `it()` would
+  // report a false pass and hide a regression in the drop-before-delete
+  // ordering in exactly the environments least likely to have a human notice.
+  beforeEach((ctx) => {
+    if (!ftsAvailable) {
+      if (ftsMustBeAvailable) {
+        throw new Error(
+          'GITNEXUS_REQUIRE_FTS=1 but the FTS extension is unavailable — cannot verify the #2589 ordering fix.',
+        );
+      }
+      if (!skipWarned) {
+        skipWarned = true;
+        console.warn(
+          '[incremental-fts-drop-ordering] Skipping — the LadybugDB FTS extension is unavailable.',
+        );
+      }
+      ctx.skip();
+    }
+  });
+
+  afterEach(() => {
+    vi.doUnmock('../../src/core/lbug/lbug-adapter.js');
+    vi.resetModules();
+  });
+
+  it('SHOW_INDEXES() reports every FTS index absent by the time deleteNodesForFiles runs', 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-2589-fts-order-');
+    try {
+      // First run: full rebuild, builds every FTS index for real.
+      await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} });
+
+      // runFullAnalysis closes its own connection on return — open a fresh
+      // one just to probe SHOW_INDEXES(), then close it before the second
+      // run opens its own (LadybugDB is single-writer/single-connection).
+      const { lbugPath } = getStoragePaths(repo.dbPath);
+      await lbugAdapter.initLbug(lbugPath);
+      const showIndexNames = async (): Promise<string[]> => {
+        const rows = (await lbugAdapter.executeQuery('CALL SHOW_INDEXES() RETURN *')) as Array<
+          Record<string, unknown>
+        >;
+        return rows.map((r) => r.index_name).filter((n): n is string => typeof n === 'string');
+      };
+      const beforeChange = await showIndexNames();
+      await lbugAdapter.closeLbug();
+
+      // Hard assertion, not a soft skip: the beforeEach gate already proved
+      // the extension loads, so every index failing to build here is a real
+      // bug in the full-rebuild FTS phase, not an environment gap.
+      for (const { indexName } of FTS_INDEXES) {
+        expect(beforeChange).toContain(indexName);
+      }
+
+      // Spy on the real deleteNodesForFiles, recording the FTS index list at
+      // the exact moment it's invoked (before it does anything), then
+      // delegating to the real implementation so the run completes normally.
+      let indexNamesAtDeleteTime: string[] | undefined;
+      const originalDeleteNodesForFiles = lbugAdapter.deleteNodesForFiles;
+      vi.spyOn(lbugAdapter, 'deleteNodesForFiles').mockImplementation(async (filePaths, opts) => {
+        indexNamesAtDeleteTime = await showIndexNames();
+        return originalDeleteNodesForFiles(filePaths, opts);
+      });
+
+      // Small change to a single file — stays well under the escalation
+      // threshold (50 files) on this 7-file mini-repo, so it takes the
+      // non-escalated (surgical) incremental branch this test targets.
+      const handlerPath = path.join(repo.dbPath, 'src', 'handler.ts');
+      await writeFile(
+        handlerPath,
+        (await readFile(handlerPath, 'utf-8')) + '\n// #2589 ordering-test touch\n',
+        'utf-8',
+      );
+      execSync('git -c user.name=test -c user.email=t@t -c commit.gpgsign=false add -A', {
+        cwd: repo.dbPath,
+        stdio: 'pipe',
+      });
+      execSync(
+        'git -c user.name=test -c user.email=t@t -c commit.gpgsign=false commit -q -m "#2589 ordering touch"',
+        { cwd: repo.dbPath, stdio: 'pipe' },
+      );
+
+      await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} });
+
+      expect(indexNamesAtDeleteTime).toBeDefined();
+      for (const { indexName } of FTS_INDEXES) {
+        expect(indexNamesAtDeleteTime).not.toContain(indexName);
+      }
+    } finally {
+      await repo.cleanup();
+    }
+  }, 300_000);
+});