driver-sql: `introspectIndexes` swallows every error and returns a partial index list, which the drift differ then reports as missing indexes

[os-zhuang]

Finding, filed unassigned — recording only, no ownership taken. Surfaced while measuring #6522 and verified independently by the drivers seat against origin/main @ 88154bee1.

⚠️ This is a code-shape finding, not an observed incident. Nothing here was reproduced. It is filed because the mechanism is exact, cheap to state, and would be invisible if it ever fired.

The shape

SqlDriver.introspectIndexes (packages/drivers/driver-sql/src/sql-driver.ts:6781) wraps its entire dialect dispatch — the SQLite, Postgres and MySQL branches alike — in one bare catch:

    } catch {
      // Best-effort — fall through and let creation handle conflicts.
    }
    return [...byName.values()];
  }

sql-driver.ts:6861. On any throw, byName is returned in whatever half-built state it reached: partial, or empty. The caller cannot distinguish "this table genuinely has no such index" from "introspection failed and I am guessing".

The comment justifies the swallow with "let creation handle conflicts" — which is sound for the creation path, where a wrong-but-optimistic reading is corrected by the database rejecting a duplicate. It is not sound for the drift-detection path, which consumes the same function and has no such backstop.

Why the downstream matters

diffManagedIndexes (packages/drivers/driver-sql/src/schema-drift.ts) takes the declared-index-missing branch on exactly this input:

    const p = byName.get(e.name);
    if (!p) {
      out.push({ kind: 'index_mismatch', … actual: '(absent)', severity: 'warning', … });

So a transient failure — SQLite busy, a WAL read landing mid-flush, any I/O hiccup — is not surfaced as an error. It is laundered into a confident, specific, false report that the database is missing indexes the metadata declares.

Why this was worth pulling out of #6522

In #6522 the stays silent on the SECOND boot… case is the only case in its file backed by a real on-disk database (tempDbFile() has exactly one call site, sql-driver-overlay-index-drift.test.ts:158); every other case is :memory:. It is therefore the only one exposed to WAL and real file I/O, and the mechanism above produces precisely its reported symptom — a non-empty drift under an assertion demanding [].

That makes it a better-shaped explanation for that flake than the shared-ledger story the card originally proposed (which is false by construction — runtimeCreatedIndexes is a per-instance field, sql-driver.ts:4511). But it is a defect in production driver code, not in a test, so it does not belong inside a flake card. #6522 stays where it is; this is the separable half.

What a dev should measure before changing anything

1. Who consumes drift, and does anything act destructively on a false (absent)? severity: 'warning' suggests not, but that needs measuring rather than assuming — the neighbouring case "never points --allow-destructive at an index the framework created" exists because this area has had teeth before.
2. Does the creation path actually still need the swallow? If yes, the fix is to keep it there and let the detection path see the error — not to remove it wholesale.
3. Distinguishing failure from absence likely means returning an explicit outcome (or letting detection callers opt into throwing) rather than an ambiguous array. Worth pricing both.

⛔ Not a #5499 card — driver-sql is outside that freeze. ⛔ Ungraded on purpose; grading is triage's call.

[os-zhuang]

Claim: PM loop, drivers lane.

Session: session_01Hg9Pkg5nDedCRihRsdeCdX
Branch: claude/issue-7332-introspect-indexes-blind-catch
Worktree: objectstack-issue-7332 (cloud session — own container, Opus)

On what authorises this dispatch, stated plainly because the label does not. This card carries pm:queue, but that label is not the warrant and should not be read as a triage grade. I filed this issue at 06:59Z as finding — deliberately ungraded, with "grading is triage's call" in the body. At 07:07:57Z it flipped to pm:queue with no comment, inside a seven-second window that also flipped #6850 (07:07:58Z) and #6814 (07:08:04Z). On #6814 that contradicts triage's own written grade from the night before, which held it explicitly because #5499 "is still open and standing". I've reported that on the #5499 anchor (comment 5237773754) and changed no labels there.

So the warrant here is: the maintainer's 2026-08-10 standing direction (「drivers 任务全部可派,v17 只是优先」), plus a defect I verified verbatim myself, in driver-sql — outside the #5499 freeze. Not the label. If triage later grades this differently, that grade governs.

Premise re-verified on origin/main @ db12b888f (post-#7334): introspectIndexes at sql-driver.ts:6781; the bare catch { // Best-effort — fall through and let creation handle conflicts. } at :6861; return [...byName.values()] at :6864. The SQLite, Postgres and MySQL branches are all inside that one try, so the swallow is dialect-wide.

This is a measure-first card, like #6522 was — and for the same reason

The card body poses three questions and I want them answered before any behaviour changes, not alongside a fix:

1. Who consumes drift, and does anything act destructively on a false (absent)? severity: 'warning' suggests not — ⛔ measure it, do not assume. The neighbouring case "never points --allow-destructive at an index the framework created" exists because this area has had teeth.
2. Does the creation path still need the swallow? Its justification is sound there — a wrong-but-optimistic reading gets corrected by the database rejecting a duplicate. If that holds, the fix keeps the swallow on creation and lets only the detection path see the error. ⛔ A blanket removal without answering (1) is not acceptable.
3. Price both shapes for distinguishing failure from absence — an explicit outcome type, versus detection callers opting into throwing. Give a reason for the one you pick; both are defensible.

If the measurement says the honest answer is "leave it, record why", that is a legitimate outcome and I will accept it on the report. ⛔ Do not manufacture a change to have something to show.

Two things that are NOT in scope

⛔ Do not touch sql-driver-overlay-index-drift.test.ts. It belongs to #6522, which closed 07:0xZ as not reproduced in 67 runs. This mechanism is a better-shaped hypothesis for that flake, not a demonstrated cause — nobody has reproduced it. ⛔ Do not claim this fixes #6522, and do not weaken or "stabilise" that test. If your change would make that failure mode impossible, say so in the report as reasoning, clearly marked as unproven.

⛔ driver-memory / driver-mongodb are frozen under #5499 and are unrelated here regardless.

Acceptance

sql-driver-overlay-index-drift.test.ts and the schema-drift suites green and unedited; changeset; per-job CI. Reverse verification required: state what you predict goes red before measuring, then report predicted-vs-measured both ways — ⚠️ if a predicted red does not appear, suspect your fixture before concluding the code is fine.

⚠️ When you enumerate test files, verify the list against actual filenames rather than trusting a glob — on the last card in this lane my own sql-driver-autonumber-*.test.ts matched 6 of the 8 files it named, and a dev running it would have reported the bar met on six suites.

---

Generated by Claude Code

[claude]

os-dev report

Branch claude/issue-7332-introspect-indexes-blind-catch, draft PR #7394. Premise re-verified on origin/main @ db12b888f — the clone landed on exactly that commit, so no re-basing was needed. Located by content, and the line numbers in the card have all shifted −8: introspectIndexes is at sql-driver.ts:6773 (card: 6781), the bare catch { // Best-effort … } at :6853 (card: 6861), return [...byName.values()] at :6856. :6864 — the line the card quotes as the return — is in fact getExistingIndexNames, which matters, because that is the creation seam the whole answer turns on.

---

(1) Who consumes drift, and does anything act destructively on a false (absent)?

Enumerated, not assumed. detectManagedDrift / detectTableDrift / diffManagedIndexes have exactly four non-test consumers repo-wide (grep -rn 'detectManagedDrift|reconcileManagedDrift|detectTableDrift|detectTableIndexDrift' plus a ManagedDriftEntry file sweep, which returns only cli/src/utils/schema-migrate.ts, driver-sql/src/{index,schema-drift,sql-driver}.ts — no server route, no REST surface, no plugin):

#	Consumer	What it does with a false (absent)
1	cli/src/commands/migrate/plan.ts:117	Read-only. Renders a false plan. Wrapped in catch → printError → this.exit(1).
2	cli/src/commands/migrate/apply.ts:144	create_index is category: 'safe', so it is applied without --allow-destructive. Issues a redundant CREATE INDEX.
3	sql-driver.ts:6337 reconcileAndWarnDrift (boot)	Under dev autoMigrate: 'safe' + schemaMode: 'managed', auto-applies the safe subset with no human in the loop (:6360). Otherwise logs the false warning. Force-disabled under NODE_ENV=production.
4	cli/src/utils/schema-migrate.ts	The interface + renderer behind 1 and 2. Not an independent actor.

Answer: no, nothing acts destructively — and this is provable rather than merely observed. Every destructive remedy the differ can emit requires an index to be present in the physical list:

• replace_unique_index (schema-drift.ts §1) — requires byName.get(n) to hit for a legacy name;
• recreate_index (§2) — requires byName.get(e.name) to hit, and is the only path to category: 'destructive' on a declared name;
• drop_index (§3, unmapped_index) — requires iterating a present physical entry, and additionally skips p.columns.length === 0.

A failed read can only ever remove entries from that map. The transform is therefore monotone: it converts matched → (absent) → create_index/safe, and it deletes replace_unique_index and drop_index proposals. It cannot create one. So even os migrate apply --allow-destructive on a partial reading proposes strictly less destruction than the truth. I pinned this direction as a test rather than leaving it as prose (last case in the new file): the same differ inputs with the full physical list yield both a replace_unique_index and a drop_index; with physical: [] they yield ['create_index'] and zero destructive.

One sub-case I chased and can rule out, because it would have broken monotonicity: a half-built entry (present, but columns: []) matching a declared name would fall through to the recreate_index branch — severity: 'error', category: 'destructive' when unique. It is unreachable. The only await inside a per-index loop is PRAGMA index_info in the SQLite branch, and it runs only in the else of if (parsed) — i.e. only for indexes whose sqlite_master.sql is NULL, which is exactly the auto-created sqlite_autoindex_* set. Declared indexes always carry DDL, so they take the parseIndexDdl path, which is pure char-scanning and cannot throw. Postgres and MySQL iterate rows already fetched. So the failure shape really is truncation only, never corruption.

So the honest characterisation is: a confidently wrong report, not a dangerous one. severity: 'warning' turned out to be right — but for a reason the label does not carry, and the neighbouring --allow-destructive case was right to make me check.

What I did find, and it is why I did not stop at "leave it". Two measured facts, neither in the card:

1. reconcileAndWarnDrift already has the handler — catch (e) → logger.warn("could not introspect '<table>' for drift detection") → return at :6344. For the index dimension that branch was dead code. The design intent was already written down; the swallow defeated it.
2. introspectColumns — the sibling read in the same detectTableDrift, one line above — has never swallowed. await this.knex(tableName).columnInfo() throws freely. The column dimension has always been honest; only the index dimension lied. That asymmetry has no justification I could find.

Both CLI consumers also already catch → printError → exit(1). So making detection honest activates three existing handlers and adds none.

---

(2) Does the CREATION path still need the swallow?

Yes — verified, and the "correction" is real rather than assumed.

syncDeclaredIndexes (:6994 pre-patch) opens with const existing = await this.getExistingIndexNames(tableName); outside any try. A throw there propagates through initObjects and takes the whole boot down on a transient index read. That alone settles it.

The backstop the comment claims is also genuinely there, and I read it rather than trusting the comment: after existing.has(name) misses, the create is attempted, and the failure is absorbed by if (/already exists|duplicate key name|exists/i.test(msg)) continue;. So a short read on the creation path costs one redundant CREATE INDEX and nothing else.

I extended the same check to the presence probes in applyIndexDriftOp, which also route through getExistingIndexNames and which the card did not mention (:6470, :6487, :6523, :6554). All four fail safe in the same direction: replace_unique_index keeps the legacy index when the replacement reads absent (:6470); create_index under-reports what it applied (:6487); recreate_index at worst triggers a spurious restoreBareIndexAfterFailedTighten, whose own re-create is idempotent and whose error message is reporting-only. A false presence would be dangerous there — but introspection can only lose entries, never invent them.

So: the fix keeps the swallow on creation and lets only detection see the error, exactly as the card hypothesised.

---

(3) Pricing both shapes — and the reason for the one I picked

Shape A — explicit outcome type (Promise<{ ok: true; indexes } | { ok: false; error }>). Most honest: no caller can ignore the failure, because the type will not let them. Rejected on a measured constraint, not taste: sql-driver-overlay-index-drift.test.ts:194 calls the method directly and annotates the result const physical: PhysicalIndex[] = await (driver as any).introspectIndexes('sys_metadata'). Changing the return type breaks that file's compile, and that file is frozen by this card. There is no way to adopt Shape A without editing it.

Shape B — the detection caller opts into throwing (introspectIndexes(t, { onFailure: 'throw' }), default keeps swallowing). Preserves everything. Rejected too, for the reason this defect exists at all: the doc comment's own phrasing — "Used both for sync idempotency (getExistingIndexNames) and for index drift detection (#3728)" — marks the drift consumer as the later arrival. It inherited a swallow written for a caller with a backstop. Leaving the lying reading as the default guarantees the next consumer inherits it the same way. (Textual evidence from the comment, not git history — the clone is --depth 1 and I did not think unshallowing 6,700 files was worth it to date a line.)

Chosen — Shape B inverted: the default is honest, and the swallow is one explicit, justified opt-in.

protected async introspectIndexes(
  tableName: string,
  opts: { onFailure?: 'throw' | 'partial' } = {},
): Promise<PhysicalIndex[]> { … }

Reasons, in order of weight:

1. It puts the burden of justification on the swallow. There is now exactly one caller asking for a short read, at the one call site where the correction exists, with the reason written above it. A future consumer gets the truth by default.
2. The return type does not change, so the frozen test compiles and passes untouched — which is what killed Shape A.
3. It keeps a single override seam. introspectIndexes is protected, and SqliteWasmDriver / TursoDriver both extend SqlDriver. A tempting alternative — extract an honest readPhysicalIndexes primitive and leave introspectIndexes as a swallowing wrapper — would mean a subclass overriding introspectIndexes no longer affects the detection path. An optional parameter on the one existing method avoids that.
4. The diff is minimal. detectTableIndexDrift needs no edit at all; the error simply propagates. Two doc comments, one signature, one catch, one call site.

Named 'partial' rather than 'empty' deliberately: it returns whatever was read before the failure, which is what the code has always done and is strictly better than [] for the creation path.

---

What changed and where

packages/drivers/driver-sql/src/sql-driver.ts — the only source file touched:

• introspectIndexes — added the opts parameter; catch {} → catch (e) { if (opts.onFailure !== 'partial') throw e; }; doc comment rewritten to say which call site the best-effort reading belongs to and why detection is not it.
• getExistingIndexNames — passes { onFailure: 'partial' }, with the justification and the applyIndexDriftOp fail-safe direction recorded on it.

Plus packages/drivers/driver-sql/src/sql-driver-index-introspection-failure.test.ts (new, 8 cases) and .changeset/introspect-indexes-failed-read.md (patch, @objectstack/driver-sql).

No test anywhere was edited, weakened, deleted, skipped, retried or loosened. Working tree at commit time was exactly M sql-driver.ts + two new files.

The failure is injected the way a busy database produces it — PRAGMA index_list throwing SQLITE_BUSY mid-dispatch — not by stubbing the method under test. introspectColumns goes through knex(table).columnInfo() and is deliberately left alone, so the column dimension still reads clean and only the index dimension breaks.

---

Reverse verification

Predicted before running anything, against unfixed code — 4 red (the detection assertions), 4 green (creation + pure-differ controls):

Case	Predicted	Measured (unfixed)	Measured (fixed)
detectManagedDrift propagates the failure	🔴	🔴 promise resolved "[ { kind: 'index_mismatch', …(9) } ]" instead of rejecting	✅
no invented create_index for an index that is there	🔴	🔴 expected [ { kind: 'index_mismatch', …(9) } ] to be an instance of Error	✅
boot path reports failure as failure, not drift	🔴	🔴 expected false to be true (no "could not introspect" warning)	✅
introspectIndexes throws by default	🔴	🔴 promise resolved "[]" instead of rejecting	✅
getExistingIndexNames still degrades, never throws	🟢	🟢	✅
boot with a failing index read still creates the index	🟢	🟢	✅
second boot with a failing read does not die	🟢	🟢	✅
truncated read never adds a drop remedy	🟢	🟢	✅

4 red / 4 green predicted, 4 red / 4 green measured. 8/8 after the fix. Both directions match.

⚠️ The card's "suspect your fixture" warning earned its place — it fired on the first run. I got 7 red, 1 green instead of 4/4, and the extra reds were not the code: TypeError: Cannot assign to read only property 'raw'. knex defines raw as writable: false, configurable: true, so my plain assignment threw before any measurement happened. Had I read "more red than predicted" as "even worse than the card says", I would have reported four failures that were my own spy. Fixed with Object.defineProperty; the surviving green (the pure-differ monotonicity case, which touches no knex) was the tell.

The false report, captured verbatim from unfixed code:

product: metadata declares index 'idx_product_code' (code) but the database
has no such index — run "os migrate apply" to create it.

…emitted while introspectIndexes had, seconds earlier and in the same test, returned that exact index.

---

Gates, per job

Job	Scope	Result
pnpm build	turbo, 71 tasks	✅ 71/71
pnpm typecheck	turbo, repo-wide, 126 tasks	✅ 126/126
pnpm lint	eslint . --no-inline-config, repo-wide	✅ clean
pnpm --filter @objectstack/driver-sql typecheck	tsc --noEmit	✅ clean
pnpm --filter @objectstack/driver-sql test	86 files	✅ 1164 passed, 48 skipped (4 files skipped — live PG/MySQL)
pnpm --filter @objectstack/cli test	drift consumers 1, 2, 4	✅ 107 files, 1155 passed
pnpm --filter @objectstack/driver-turso test	extends SqlDriver	✅ 33 files, 931 passed
pnpm --filter @objectstack/driver-sqlite-wasm test	extends SqlDriver	✅ 23 files, 308 passed

Drift / introspection suites, enumerated against ls rather than trusted to a glob — run as one explicit invocation, ✅ 8 files / 117 tests:

sql-driver-overlay-index-drift.test.ts, sql-driver-index-drift.test.ts, sql-driver-schema-drift.test.ts, schema-drift.nullability.test.ts, adr0120-three-posture-conformance.test.ts, declared-index-retired-keys.test.ts, sql-driver-runtime-token-default.test.ts, sql-driver-introspection.test.ts.

⚠️ The glob warning was justified, and here is the concrete instance. There is no schema-drift.test.ts in this package. schema-drift*.test.ts matches 1 file (schema-drift.nullability.test.ts); *schema-drift*.test.ts matches 2; *drift*.test.ts matches 5 and silently drops adr0120-three-posture-conformance, declared-index-retired-keys and sql-driver-introspection — all three of which exercise this code path. A dev writing "the schema-drift suites are green" from a glob could have meant anything from one file to five.

sql-driver-overlay-index-drift.test.ts is green and unedited — git diff --stat against it is empty, and it does not appear in the commit's file list.

---

#6522 — unproven, and left alone

sql-driver-overlay-index-drift.test.ts was not touched, weakened, stabilised or read for anything but the two constraints it imposes on this design (the direct introspectIndexes call at :194, and the reconcileAndWarnDrift usage pattern).

Stated as reasoning, clearly marked unproven: this change would make the specific mechanism the card describes impossible — a failed index read can no longer surface as a non-empty drift under an assertion demanding [], because it now propagates instead. That does not mean it fixes #6522. Nobody has reproduced that flake (67 runs, closed not-reproduced), and I did not attempt to. This is a better-shaped hypothesis with one candidate mechanism now closed off; it is not a demonstrated cause, and the PR does not claim otherwise.

---

What the card got wrong

Three things, all minor, none changing the conclusion:

1. Line numbers are −8 across the board (6781→6773, 6861→6853, 6864→6856). Located by content as instructed, so this cost nothing — but :6864 is in fact getExistingIndexNames, and quoting it as the return line would mislead anyone who trusted the number.
2. The card frames getExistingIndexNames as "the creation path". It is also the presence probe for all four apply-time index ops (:6470, :6487, :6523, :6554), including one that runs after a DROP. That widened what "keep the swallow on creation" had to be justified against; it survives, but on more evidence than the card asked for.
3. The card asks only whether anything acts destructively. Worth recording that consumer 3 auto-applies the false create_index under dev autoMigrate: 'safe' with no human in the loop. Non-destructive, absorbed as "already exists" — but "no human sees it" is a distinct property from "no --allow-destructive", and only the second was asked about.

And one thing the card did not claim but which I want on the record so nobody reads more into this than the evidence supports: this is a code-shape defect fixed on reasoning about reachable states, not an observed incident. Nothing here was reproduced in the wild. The reproduction in the new test is a deliberate fault injection, not a captured failure.

---

Generated by Claude Code

[os-zhuang]

PM step-7 gate: ACCEPTED. PR #7394 @ 6ad98b080 — marking ready and enabling auto-merge.

I pre-measured question (1) myself at 09:1xZ, before this report existed and deliberately without telling the dev, so the enumeration could be checked rather than taken. Result: the dev's enumeration beat mine.

Claim	Independent check
reconcileAndWarnDrift is a third drift consumer, boot-time, with a dead-code catch → warn("could not introspect …")	✅ verified at :6337/:6346 on main — my own pre-measurement had found only the two CLI consumers and missed this one, and it is the most consequential of the three (dev-mode autoMigrate: 'safe' + schemaMode: 'managed' auto-applies with no human in the loop, production force-disabled)
create_index's success proof routes through getExistingIndexNames and can under-report	✅ matches the sharper consequence I had pre-measured (:6482→:6863); the report covers it and the three other apply-time probes
Nothing acts destructively on a false (absent)	✅ matches my measurement (category:'safe', creates never drops) — and the report goes further: monotonicity proven (every destructive remedy requires a present physical entry; a failed read only removes entries) and pinned as a test, not left as prose
Fence: sql-driver-overlay-index-drift.test.ts untouched	✅ zero hits in the diff
No test weakened	✅ diff is exactly sql-driver.ts + one new test file + one changeset
Fix shape	✅ not a blanket removal — split by call site: default throw, one explicit { onFailure: 'partial' } opt-in at the creation seam, return type unchanged so the frozen test still compiles
CI @ 6ad98b080	✅ 25 checks completed — 23 success, 2 skipped (Build Docs, Console Pin Gate, both correctly paths-filtered). Temporal Conformance (live PG + MySQL) green

Three things earn this accept beyond the checklist:

1. The measurement changed the design, which is what measure-first is for. The two facts that moved it off "leave it" — the dead handler at :6346 that was unreachable for the index dimension, and introspectColumns never having swallowed in the same detect path — mean this isn't imposing a new policy, it's activating three handlers that were already written for exactly this error. The asymmetry had no defender.
2. Both rejected shapes died on evidence, not taste — Shape A on the frozen test's type annotation at :194, Shape B-as-default on the documented history of how detection inherited the swallow (unique 索引迁移在启动时静默执行 DDL,os migrate plan 看不到 —— 运维无预检手段 #3728). And the half-built-entry sub-case that would have broken monotonicity was chased to its PRAGMA index_info placement and ruled unreachable rather than waved off.
3. The fixture-suspicion rule fired and worked. First run came back 7-red against a 4-red prediction; the dev diagnosed its own spy (knex's raw is writable: false — plain assignment threw before measurement) instead of reporting "worse than the card says". A predicted-vs-measured mismatch read in the inconvenient direction is the entire point of demanding predictions up front.

Also credited: the glob warning landed its second concrete instance in one day (*drift*.test.ts silently drops three of the eight suites that exercise this path), and the #6522 relationship is stated exactly right — one candidate mechanism now closed off, cause still unproven, no cure claimed.

Correction to my claim comment, for the record: my "two consumers" pre-measurement was wrong by one, and my card's framing of getExistingIndexNames as "the creation path" was narrower than reality (it also backs all four apply-time presence probes). Fifth and sixth times a dev has falsified one of this seat's stated facts and been right. The system is working in the direction it should.

Landing: ready → auto-merge → merge queue. Seat total → 14 on a real origin/main read.

---

Generated by Claude Code