fix(driver-sql): a failed index read is an error, not an empty index list (#7332)

.changeset/introspect-indexes-failed-read.md

@@ -0,0 +1,51 @@
+---
+"@objectstack/driver-sql": patch
+---
+
+fix(driver-sql): a failed index read is an error, not an empty index list (#7332)
+
+`SqlDriver.introspectIndexes` wrapped its **entire** dialect dispatch — the
+SQLite, Postgres and MySQL branches alike — in one bare `catch {}` and then
+returned its accumulator in whatever half-built state it had reached. The caller
+could not tell *"this table genuinely has no such index"* from *"the read failed
+and I am guessing"*.
+
+Drift detection consumed that same function. `diffManagedIndexes` takes its
+declared-index-missing branch on exactly that input, so a transient failure —
+SQLITE_BUSY, a WAL read landing mid-flush, any I/O hiccup — was not surfaced as
+an error. It was laundered into a confident, specific and **false** report:
+
+```
+product: metadata declares index 'idx_product_code' (code) but the database
+has no such index — run "os migrate apply" to create it.
+```
+
+…about an index that was there the whole time.
+
+**The swallow is kept where its justification holds, and only there.** That
+justification — *"let creation handle conflicts"* — is sound at
+`getExistingIndexNames`, whose caller `syncDeclaredIndexes` corrects an
+optimistic wrong reading by attempting the create and absorbing the
+"already exists" error; a throw there would take a whole boot down on a
+transient read. Detection has no such backstop, and inherited the swallow only
+because #3728 wired a second consumer onto the same function. `introspectIndexes`
+therefore now **throws by default** and takes an explicit
+`{ onFailure: 'partial' }` opt-in, which the creation seam passes and nothing
+else does.
+
+**What changes for you.** Nothing on the creation path: boot still tolerates a
+failed index read and still converges the schema. On the detection path, a
+failure that was previously invisible is now reported as one — `os migrate plan`
+and `os migrate apply` print it and exit non-zero instead of rendering a plan
+built on a partial reading, and boot-time drift handling logs
+`could not introspect '<table>' for drift detection` (a handler
+`reconcileAndWarnDrift` already carried) instead of a false drift warning. This
+matches the sibling read in the same detect path, `introspectColumns`, which has
+never swallowed.
+
+Measured, and worth stating plainly: no consumer ever acted **destructively** on
+the false reading. Dropping entries from the physical list is monotone — the
+`replace_unique_index`, `drop_index` and `recreate_index` remedies all require an
+index to be *present*, so a short read can only ever remove a destructive
+proposal, never arm one. The defect was a confidently wrong report, not a
+dangerous one.

packages/drivers/driver-sql/src/sql-driver-index-introspection-failure.test.ts

@@ -0,0 +1,217 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+import { describe, it, expect, afterEach, vi } from 'vitest';
+import { SqlDriver, diffManagedIndexes, type PhysicalIndex } from '../src/index.js';
+
+/**
+ * A failed index read is not an empty index list (#7332).
+ *
+ * `introspectIndexes` used to wrap its whole dialect dispatch — SQLite,
+ * Postgres and MySQL alike — in one bare `catch {}` and return `byName` in
+ * whatever half-built state it had reached. The caller could not tell "this
+ * table genuinely has no such index" from "the read failed and I am guessing",
+ * so a transient SQLITE_BUSY / WAL hiccup was laundered into a confident,
+ * specific, FALSE report that the database is missing declared indexes.
+ *
+ * The swallow's own justification — *"let creation handle conflicts"* — is
+ * sound where it was written, {@link SqlDriver.getExistingIndexNames}: a
+ * wrong-but-optimistic reading there costs one redundant `CREATE INDEX`, which
+ * `syncDeclaredIndexes` absorbs as "already exists". Drift DETECTION has no
+ * such backstop, and it inherited the swallow only because it was wired onto
+ * the same function later (#3728). So the split is by call site, not by
+ * removal: the creation seam opts into `{ onFailure: 'partial' }`, everything
+ * else — detection included — sees the error.
+ *
+ * Note the asymmetry this closes: the sibling read in the very same detect
+ * path, `introspectColumns`, has never swallowed. `reconcileAndWarnDrift`
+ * already carries the handler for a throwing `detectTableDrift`
+ * ("could not introspect '<table>' for drift detection"), and both CLI
+ * consumers already `catch → printError → exit(1)`. The index dimension was
+ * the one that could not reach any of them.
+ */
+describe('index introspection failure is not "no indexes" (#7332)', () => {
+  let knexInstance: any;
+
+  const makeDriver = (opts: any = {}) => {
+    const d = new SqlDriver({
+      client: 'better-sqlite3',
+      connection: { filename: ':memory:' },
+      useNullAsDefault: true,
+      ...opts,
+    });
+    knexInstance = (d as any).knex;
+    (d as any).logger = { warn: vi.fn(), info: vi.fn(), error: vi.fn() };
+    return d;
+  };
+
+  afterEach(async () => {
+    await knexInstance?.destroy();
+    knexInstance = undefined;
+  });
+
+  const productMeta = [
+    {
+      name: 'product',
+      fields: {
+        id: { type: 'string' },
+        organization_id: { type: 'string' },
+        code: { type: 'string' },
+      },
+      indexes: [{ name: 'idx_product_code', fields: ['code'] }],
+    },
+  ];
+
+  /**
+   * Fail the index read the way a busy database does — mid-dispatch, on one
+   * statement — rather than by stubbing the method under test. `PRAGMA
+   * index_list` is the SQLite branch's second round-trip; `introspectColumns`
+   * goes through `knex(table).columnInfo()` and is deliberately untouched, so
+   * the column dimension still reads clean and only the index dimension breaks.
+   */
+  const breakIndexRead = (driver: SqlDriver, message = 'SQLITE_BUSY: database is locked') => {
+    const knex: any = (driver as any).knex;
+    const real = knex.raw.bind(knex);
+    // knex defines `raw` as non-writable (but configurable), so a plain
+    // assignment throws — the swap has to go through `defineProperty`.
+    const swap = (fn: any) => Object.defineProperty(knex, 'raw', { value: fn, configurable: true });
+    swap((sql: any, ...rest: any[]) => {
+      if (typeof sql === 'string' && /PRAGMA\s+index_list/i.test(sql)) throw new Error(message);
+      return real(sql, ...rest);
+    });
+    return () => swap(real);
+  };
+
+  // ── Detection sees the failure ────────────────────────────────────────────
+
+  it('detectManagedDrift propagates the read failure instead of reporting "(absent)"', async () => {
+    const driver = makeDriver();
+    await driver.initObjects(productMeta);
+    const restore = breakIndexRead(driver);
+    try {
+      await expect(driver.detectManagedDrift(productMeta as any)).rejects.toThrow(/SQLITE_BUSY/);
+    } finally {
+      restore();
+    }
+  });
+
+  it('does not invent a create_index remedy for an index that is actually there', async () => {
+    const driver = makeDriver();
+    await driver.initObjects(productMeta);
+
+    // Ground truth first: the index the read is about to miss really exists.
+    const physical: PhysicalIndex[] = await (driver as any).introspectIndexes('product');
+    expect(physical.map((p) => p.name)).toContain('idx_product_code');
+
+    const restore = breakIndexRead(driver);
+    try {
+      const drift = await driver.detectManagedDrift(productMeta as any).catch((e) => e);
+      expect(drift).toBeInstanceOf(Error);
+    } finally {
+      restore();
+    }
+  });
+
+  it('the boot path reports the failure as a failure, not as drift', async () => {
+    const driver = makeDriver();
+    await driver.initObjects(productMeta);
+    (driver as any).logger.warn.mockClear();
+
+    const restore = breakIndexRead(driver);
+    try {
+      await (driver as any).reconcileAndWarnDrift('product', productMeta[0].fields, productMeta[0].indexes);
+    } finally {
+      restore();
+    }
+
+    const warnings = (driver as any).logger.warn.mock.calls.map((c: any[]) => String(c[0]));
+    expect(warnings.some((w: string) => /could not introspect 'product' for drift detection/.test(w))).toBe(true);
+    // The false report the swallow used to produce, verbatim from the differ.
+    expect(warnings.some((w: string) => /has no such index/.test(w))).toBe(false);
+  });
+
+  it('introspectIndexes throws by default — the honest contract for a read', async () => {
+    const driver = makeDriver();
+    await driver.initObjects(productMeta);
+    const restore = breakIndexRead(driver);
+    try {
+      await expect((driver as any).introspectIndexes('product')).rejects.toThrow(/SQLITE_BUSY/);
+    } finally {
+      restore();
+    }
+  });
+
+  // ── Creation keeps the swallow, because it keeps the backstop ─────────────
+
+  it('getExistingIndexNames still degrades to a partial read rather than throwing', async () => {
+    const driver = makeDriver();
+    await driver.initObjects(productMeta);
+    const restore = breakIndexRead(driver);
+    try {
+      await expect((driver as any).getExistingIndexNames('product')).resolves.toBeInstanceOf(Set);
+    } finally {
+      restore();
+    }
+  });
+
+  it('a boot whose index read fails throughout still creates the declared index', async () => {
+    // The whole justification for keeping the swallow on the creation path: an
+    // optimistic wrong reading is corrected by the database rejecting the
+    // duplicate, so the boot completes and the schema converges anyway.
+    const driver = makeDriver();
+    const restore = breakIndexRead(driver);
+    try {
+      await driver.initObjects(productMeta);
+    } finally {
+      restore();
+    }
+
+    const physical: PhysicalIndex[] = await (driver as any).introspectIndexes('product');
+    expect(physical.map((p) => p.name)).toContain('idx_product_code');
+  });
+
+  it('an index read that fails on the SECOND boot does not take the boot down', async () => {
+    const driver = makeDriver();
+    await driver.initObjects(productMeta);
+    const restore = breakIndexRead(driver);
+    try {
+      // Everything already exists; the read that would prove it is broken.
+      await expect(driver.initObjects(productMeta)).resolves.not.toThrow();
+    } finally {
+      restore();
+    }
+  });
+
+  // ── A truncated read can never ARM a destructive remedy ───────────────────
+
+  it('losing physical indexes only ever removes drop remedies, never adds one', () => {
+    // The differ's three sections all key off PRESENCE in the physical list:
+    // `replace_unique_index` needs the legacy index present, `drop_index` needs
+    // the orphan present, `recreate_index` needs the declared name present.
+    // Dropping entries is therefore monotone — this pins that direction, which
+    // is why a false read is loud-and-wrong rather than destructive.
+    const full: PhysicalIndex[] = [
+      { name: 'product_code_unique', columns: ['code'], unique: true, primary: false },
+      { name: 'idx_product_stale', columns: ['legacy_col'], unique: false, primary: false },
+    ];
+    const args = {
+      table: 'product',
+      expected: [{ name: 'idx_product_code', columns: ['code'], unique: false }],
+      legacy: [
+        {
+          column: 'code',
+          legacyNames: ['product_code_unique'],
+          replacement: { name: 'uniq_product_organization_id_code', columns: ['organization_id', 'code'], unique: true as const },
+        },
+      ],
+      tenantField: 'organization_id',
+    };
+
+    const complete = diffManagedIndexes({ ...args, physical: full } as any);
+    expect(complete.some((d) => d.op.type === 'replace_unique_index')).toBe(true);
+    expect(complete.some((d) => d.op.type === 'drop_index')).toBe(true);
+
+    const truncated = diffManagedIndexes({ ...args, physical: [] } as any);
+    expect(truncated.some((d) => d.category === 'destructive')).toBe(false);
+    expect(truncated.map((d) => d.op.type)).toEqual(['create_index']);
+  });
+});

packages/drivers/driver-sql/src/sql-driver.ts

@@ -6753,8 +6753,30 @@ export class SqlDriver implements IDataDriver {
    *
    * Used both for sync idempotency ({@link getExistingIndexNames}) and for index
    * drift detection (#3728), which needs the full definition rather than just
-   * the name. Failures are swallowed: at worst we attempt a create and absorb
-   * the "already exists" error in {@link syncDeclaredIndexes}.
+   * the name.
+   *
+   * ⚠️ A failed read is an ERROR, not an empty table (#7332). This used to wrap
+   * the whole dialect dispatch in a bare `catch {}` and return `byName` in
+   * whatever half-built state it had reached, so the caller could not tell "this
+   * table genuinely has no such index" from "the read failed and I am guessing".
+   * Downstream, `diffManagedIndexes` takes its declared-index-missing branch on
+   * exactly that input: a transient SQLITE_BUSY, or a WAL read landing
+   * mid-flush, was laundered into a confident, specific and FALSE
+   * `actual: '(absent)'` — *"metadata declares index 'x' but the database has no
+   * such index"* — about an index that was there the whole time.
+   *
+   * The swallow's justification, *"let creation handle conflicts"*, is sound
+   * only where it was written: {@link getExistingIndexNames}, whose caller
+   * {@link syncDeclaredIndexes} corrects an optimistic wrong reading by
+   * attempting the create and absorbing the "already exists" error. Detection
+   * has no such backstop, and it inherited the swallow only because #3728 wired
+   * a second consumer onto the same function. So the split is by CALL SITE:
+   * `onFailure: 'partial'` is the creation seam's opt-in, and everything else —
+   * detection included — sees the throw. Its callers are already built for it:
+   * {@link reconcileAndWarnDrift} catches and warns "could not introspect
+   * '<table>' for drift detection", and `os migrate plan` / `apply` both catch,
+   * print the error and exit non-zero. The sibling read in the same detect
+   * path, {@link introspectColumns}, has never swallowed.
    *
    * Postgres reads `pg_index` rather than `pg_indexes` so indexes backing a
    * UNIQUE CONSTRAINT (which is exactly what knex's old `col.unique()` produced)
@@ -6770,7 +6792,17 @@ export class SqlDriver implements IDataDriver {
    * what tells the differ this index is none of its business
    * (`isSyncReproducibleIndex`).
    */
-  protected async introspectIndexes(tableName: string): Promise<PhysicalIndex[]> {
+  protected async introspectIndexes(
+    tableName: string,
+    opts: {
+      /**
+       * What a failed read means to THIS caller (#7332). `'throw'` (the
+       * default) surfaces it; `'partial'` returns whatever was read before the
+       * failure — correct only where a short read is self-correcting.
+       */
+      onFailure?: 'throw' | 'partial';
+    } = {},
+  ): Promise<PhysicalIndex[]> {
     const byName = new Map<string, PhysicalIndex>();
     const upsert = (name: string, unique: boolean, primary: boolean): PhysicalIndex => {
       let e = byName.get(name);
@@ -6850,18 +6882,28 @@ export class SqlDriver implements IDataDriver {
           else if (r.EXPRESSION != null) applyIndexKeyParts(entry, [String(r.EXPRESSION)]);
         }
       }
-    } catch {
-      // Best-effort — fall through and let creation handle conflicts.
+    } catch (e) {
+      // Only a caller that can CORRECT a short read may ask for one (#7332).
+      if (opts.onFailure !== 'partial') throw e;
     }
     return [...byName.values()];
   }
 
   /**
    * Names of the indexes that already exist on a table. Used to make
    * declared-index sync idempotent across repeated runs.
+   *
+   * Best-effort by design (#7332), and the one caller entitled to be: every
+   * consumer of this set uses it to decide whether to SKIP a create, so a name
+   * missing from a short read costs at most one redundant `CREATE INDEX` —
+   * which {@link syncDeclaredIndexes} absorbs as "already exists" — and a
+   * throw here would instead take the whole boot down on a transient read.
+   * The presence probes in {@link applyIndexDriftOp} fail the same safe way:
+   * a false absence keeps the legacy index in place and under-reports what was
+   * applied, never the reverse.
    */
   protected async getExistingIndexNames(tableName: string): Promise<Set<string>> {
-    return new Set((await this.introspectIndexes(tableName)).map((i) => i.name));
+    return new Set((await this.introspectIndexes(tableName, { onFailure: 'partial' })).map((i) => i.name));
   }
 
   /**