From 6ad98b080e61d2f1d610e38e9ef6a33d7338fdce Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 09:10:14 +0000 Subject: [PATCH] fix(driver-sql): a failed index read is an error, not an empty index list (#7332) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SqlDriver.introspectIndexes` wrapped its entire dialect dispatch — SQLite, Postgres and MySQL alike — in one bare `catch {}` and returned its accumulator half-built. Callers could not tell "this table genuinely has no such index" from "the read failed and I am guessing". `diffManagedIndexes` takes its declared-index-missing branch on exactly that input, so a transient SQLITE_BUSY or a WAL read landing mid-flush became a confident, specific and false `actual: '(absent)'` report about an index that was there the whole time. Split the swallow by call site rather than removing it. Its justification — "let creation handle conflicts" — holds at `getExistingIndexNames`, whose caller `syncDeclaredIndexes` corrects an optimistic wrong reading by attempting the create and absorbing "already exists"; a throw there would take a 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` now throws by default and takes an explicit `{ onFailure: 'partial' }` opt-in that only the creation seam passes. The detection callers were already built for this: `reconcileAndWarnDrift` catches and warns "could not introspect '' for drift detection", and `os migrate plan` / `apply` both catch, print and exit non-zero. The sibling read in the same detect path, `introspectColumns`, has never swallowed. Measured: no consumer ever acted destructively on the false reading. Dropping entries from the physical list is monotone — `replace_unique_index`, `drop_index` and `recreate_index` all require an index to be present — so a short read can only remove a destructive proposal, never arm one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012Wv1i1AwBy8eETqaDCXV6B --- .changeset/introspect-indexes-failed-read.md | 51 ++++ ...driver-index-introspection-failure.test.ts | 217 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 54 ++++- 3 files changed, 316 insertions(+), 6 deletions(-) create mode 100644 .changeset/introspect-indexes-failed-read.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-index-introspection-failure.test.ts diff --git a/.changeset/introspect-indexes-failed-read.md b/.changeset/introspect-indexes-failed-read.md new file mode 100644 index 0000000000..7ccdee9139 --- /dev/null +++ b/.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 '
' 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. diff --git a/packages/drivers/driver-sql/src/sql-driver-index-introspection-failure.test.ts b/packages/drivers/driver-sql/src/sql-driver-index-introspection-failure.test.ts new file mode 100644 index 0000000000..4fbc065a0a --- /dev/null +++ b/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 '
' 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']); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 543f3cc91a..eb3bbe978b 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/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 + * '
' 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 { + 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 { const byName = new Map(); const upsert = (name: string, unique: boolean, primary: boolean): PhysicalIndex => { let e = byName.get(name); @@ -6850,8 +6882,9 @@ 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()]; } @@ -6859,9 +6892,18 @@ export class SqlDriver implements IDataDriver { /** * 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> { - return new Set((await this.introspectIndexes(tableName)).map((i) => i.name)); + return new Set((await this.introspectIndexes(tableName, { onFailure: 'partial' })).map((i) => i.name)); } /**