From 90e8f132f70c6f1e72eb8de113560af647d96655 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 13:09:26 +0000 Subject: [PATCH] fix(driver-turso): the remote face's not-materialised index skip goes through the durability sink A declared index whose key column never materializes (a misspelt name, a virtual formula field) is skipped by RemoteTransport.buildDeclaredIndexDDL. The skip was reported through the diagnostic sink (logger.warn); it now goes through the existing durability sink (logger.error), stating the consequence and the fix, for unique and plain indexes alike, as the local face's syncDeclaredIndexes does through logDurabilityFailure. Claude-Session: https://claude.ai/code/session_01DEvba2nBuD4tWzfq8r8NFY Co-authored-by: Claude --- .../20537-remote-skipped-index-durability.md | 24 ++ ...ansport-unbuildable-declared-index.test.ts | 210 ++++++++++++++++++ .../driver-turso/src/remote-transport.ts | 42 +++- 3 files changed, 268 insertions(+), 8 deletions(-) create mode 100644 .changeset/20537-remote-skipped-index-durability.md create mode 100644 packages/drivers/driver-turso/src/remote-transport-unbuildable-declared-index.test.ts diff --git a/.changeset/20537-remote-skipped-index-durability.md b/.changeset/20537-remote-skipped-index-durability.md new file mode 100644 index 00000000000..c26b5954b88 --- /dev/null +++ b/.changeset/20537-remote-skipped-index-durability.md @@ -0,0 +1,24 @@ +--- +'@objectstack/driver-turso': patch +--- + +fix(driver-turso): a declared index the remote face skips because a key column never materializes is logged at `error`, not `warn` + +Clause-②: no + +In remote mode (a `libsql://` URL), schema sync skips a declared index whose key column is not +a stored column: a name that is not a field of the object (a misspelling), or a virtual +`formula` field, which is computed on read and has no column. The skip itself is unchanged, since +DDL naming a missing column would fail the whole sync. It used to be reported through the +driver's `warn` diagnostics, so a skipped UNIQUE index left duplicates accepted while the log +said `warn`. + +The skip is now logged at `error`, on the same channel the remote face already uses for a +declared index it could not create, and the local face uses for the same skip. There is one +line per skipped index per sync. It names the object, the index and the missing column, says +whether the index was UNIQUE, states what is not enforced (duplicates for a UNIQUE index, a full +table scan for a plain one), and says how to fix it: make every key column a stored field of +the object, or remove the index. + +No DDL, accept set or refusal changes: the same indexes are created and the same ones are +skipped. diff --git a/packages/drivers/driver-turso/src/remote-transport-unbuildable-declared-index.test.ts b/packages/drivers/driver-turso/src/remote-transport-unbuildable-declared-index.test.ts new file mode 100644 index 00000000000..17cdf1c8dd9 --- /dev/null +++ b/packages/drivers/driver-turso/src/remote-transport-unbuildable-declared-index.test.ts @@ -0,0 +1,210 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20537] The REMOTE face's half of #20432: a declared index the remote + * transport skips, because a key column never materializes, is reported at + * `error` on the durability sink. + * + * ## The defect this pins + * + * `RemoteTransport.buildDeclaredIndexDDL` plans no DDL for a declared index + * whose key column is not a stored column: a misspelt name (`statsu` on a + * table whose column is `status`), or a virtual `formula` field. That skip is + * right, since DDL naming a column that does not exist would fail the whole + * sync. But it was reported through the DIAGNOSTIC sink, which `TursoDriver` + * wires to `logger.warn`. For a UNIQUE index that is the AGENTS.md + * durability-degradation shape: every write keeps succeeding, duplicates are + * accepted, and the only trace was a `warn`. The same transport already has a + * second, durability sink (wired to `logger.error`) for exactly this class, + * and its retrofit arm already reports a unique AND a plain index it could not + * create there. The local face (`SqlDriver.syncDeclaredIndexes`) logs the same + * skip through `logDurabilityFailure`, for unique and plain alike. + * + * ## What is asserted + * + * The CHANNEL (durability sink, never the diagnostic one), the number of lines + * (one per skipped index per sync), the named subjects (table, index name, + * missing column), and the one word that says which kind of index it is + * (`UNIQUE`, or its absence). The prose is not pinned beyond that. The + * consequence the line reports is also checked against the database: the + * index really is absent. + * + * The matrix is {misspelt, formula} x {unique, plain}, as the card asks, over + * all four ways a sync reaches the skip: `syncSchema` and `syncSchemasBatch`, + * each against a new table and against one that already exists (the retrofit + * leg). The four share one builder, and a card that fixes one call site is + * the shape AGENTS.md Prime Directive #10 warns about. + * + * The client is the real `@libsql/client` over `file::memory:`, as the + * declared-index parity suite uses, so "the index is absent" is read off + * `sqlite_master` rather than inferred from a string. The last `describe` + * drives the whole `TursoDriver` in remote mode and reads the LEVEL, because + * the sink-to-level wiring lives in `turso-driver.ts`. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { createClient, type Client } from '@libsql/client'; +import { buildIndexName } from '@objectstack/driver-sql'; +import { RemoteTransport } from './remote-transport.js'; +import { TursoDriver } from './turso-driver.js'; + +type ObjectDef = { + name: string; + fields: Record; + indexes?: Array<{ fields: string[]; unique?: boolean }>; +}; + +interface Cell { + label: string; + table: string; + fields: Record; + /** The key column that never materializes. */ + column: string; + unique: boolean; +} + +const CELLS: Cell[] = [ + { + label: 'a misspelt key column, UNIQUE index', + table: 'os20537_misspelt_unique', + fields: { status: { type: 'text', maxLength: 64 } }, + column: 'statsu', + unique: true, + }, + { + label: 'a misspelt key column, plain index', + table: 'os20537_misspelt_plain', + fields: { status: { type: 'text', maxLength: 64 } }, + column: 'statsu', + unique: false, + }, + { + label: 'a formula key column, UNIQUE index', + table: 'os20537_formula_unique', + fields: { amount: { type: 'number' }, doubled: { type: 'formula', expression: 'amount * 2' } }, + column: 'doubled', + unique: true, + }, + { + label: 'a formula key column, plain index', + table: 'os20537_formula_plain', + fields: { amount: { type: 'number' }, doubled: { type: 'formula', expression: 'amount * 2' } }, + column: 'doubled', + unique: false, + }, +]; + +const withIndex = (cell: Cell): ObjectDef => ({ + name: cell.table, + fields: Object.fromEntries(Object.entries(cell.fields).map(([k, v]) => [k, { ...v }])), + indexes: [{ fields: [cell.column], ...(cell.unique ? { unique: true } : {}) }], +}); + +const withoutIndex = (cell: Cell): ObjectDef => { + const { indexes: _dropped, ...rest } = withIndex(cell); + return rest; +}; + +type SyncPath = 'syncSchema' | 'syncSchemasBatch'; +const PATHS: SyncPath[] = ['syncSchema', 'syncSchemasBatch']; +type TableState = 'new table' | 'existing table'; +const STATES: TableState[] = ['new table', 'existing table']; + +const cleanups: Array<() => Promise | void> = []; +afterEach(async () => { + for (const cleanup of cleanups.splice(0).reverse()) await cleanup(); + vi.restoreAllMocks(); +}); + +/** A transport over an in-memory libsql database, with both sinks captured. */ +function transport(): { t: RemoteTransport; client: Client; durability: string[]; diagnostic: string[] } { + const client = createClient({ url: 'file::memory:' }); + cleanups.push(() => client.close()); + const t = new RemoteTransport(); + t.setClient(client); + const durability: string[] = []; + const diagnostic: string[] = []; + t.setDurabilitySink((m) => durability.push(m)); + t.setDiagnosticSink((m) => diagnostic.push(m)); + return { t, client, durability, diagnostic }; +} + +const sync = (t: RemoteTransport, path: SyncPath, def: ObjectDef): Promise => + path === 'syncSchema' + ? t.syncSchema(def.name, def) + : t.syncSchemasBatch([{ object: def.name, schema: def }]); + +const indexNames = async (client: Client, table: string): Promise => + (await client.execute({ sql: `SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = ?`, args: [table] })) + .rows.map((r) => String(r.name)); + +const tableExists = async (client: Client, table: string): Promise => + (await client.execute({ sql: `SELECT name FROM sqlite_master WHERE type = 'table' AND name = ?`, args: [table] })) + .rows.length === 1; + +describe('[#20537] a declared index the remote face skips is reported on the durability sink', () => { + for (const cell of CELLS) { + const INDEX = buildIndexName(cell.table, [cell.column], cell.unique); + + describe(cell.label, () => { + for (const path of PATHS) { + for (const state of STATES) { + it(`${path}, ${state}: one durability line naming the index, and no diagnostic line`, async () => { + const { t, client, durability, diagnostic } = transport(); + if (state === 'existing table') { + await sync(t, path, withoutIndex(cell)); + expect(await tableExists(client, cell.table)).toBe(true); + durability.length = 0; + diagnostic.length = 0; + } + + // The sync goes on: the skip never fails it. + await expect(sync(t, path, withIndex(cell))).resolves.toBeUndefined(); + expect(await tableExists(client, cell.table)).toBe(true); + + const lines = durability.filter((m) => m.includes(`"${INDEX}"`)); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain(`"${cell.table}"`); + expect(lines[0]).toContain(`'${cell.column}'`); + if (cell.unique) expect(lines[0]).toContain('UNIQUE'); + else expect(lines[0]).not.toContain('UNIQUE'); + // Nothing else reached the durability sink for this object. + expect(durability).toEqual(lines); + + // The level MOVED; it did not gain a second line. Before the fix this + // skip reached ONLY the diagnostic sink, naming the column. + expect(diagnostic.filter((m) => m.includes(cell.column) || m.includes(INDEX))).toEqual([]); + + // The consequence the line reports is real: nothing was built. + expect(await indexNames(client, cell.table)).not.toContain(INDEX); + }); + } + } + }); + } +}); + +describe('[#20537] through TursoDriver in remote mode, the skip lands on logger.error, not logger.warn', () => { + for (const cell of CELLS.filter((c) => c.column === 'statsu')) { + it(cell.label, async () => { + const client = createClient({ url: 'file::memory:' }); + const driver = new TursoDriver({ url: 'libsql://unbuildable-declared-index.turso.io', client }); + await driver.connect(); + cleanups.push(() => driver.disconnect()); + expect(driver.transportMode).toBe('remote'); + const logger = (driver as unknown as { logger: { warn: (m: string) => void; error: (m: string) => void } }) + .logger; + const error = vi.spyOn(logger, 'error').mockImplementation(() => undefined); + const warn = vi.spyOn(logger, 'warn').mockImplementation(() => undefined); + const INDEX = buildIndexName(cell.table, [cell.column], cell.unique); + + await expect(driver.initObjects([withIndex(cell)] as never)).resolves.toBeUndefined(); + + const errors = error.mock.calls.map((c) => String(c[0])).filter((m) => m.includes(`"${INDEX}"`)); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain(`'${cell.column}'`); + expect(warn.mock.calls.some((c) => String(c[0]).includes(cell.column))).toBe(false); + expect(await indexNames(client, cell.table)).not.toContain(INDEX); + }); + } +}); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index 7ddbdb71d3a..1c7cb45f282 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -1248,6 +1248,13 @@ export class RemoteTransport { * each one scans the table — nothing is visibly wrong, and the cost arrives * as read volume. * + * [#20537] A declared index SKIPPED because a key column never materialized + * (a misspelt name, a virtual `formula` field) is the same class again, one + * step earlier: {@link buildDeclaredIndexDDL} plans no DDL for it at all, so + * the constraint (or the access path) is missing for the same reason and with + * the same outward look. The local face logs that skip through + * `SqlDriver.logDurabilityFailure`, so this face reports it here. + * * Absent, the degradation is not lost: {@link retrofitDeclaredIndexes} still * skips the index, and a missing UNIQUE resurfaces as the enveloped refusal * in {@link upsert}. `TursoDriver` wires this to `logger.error` at construction, @@ -2425,11 +2432,13 @@ export class RemoteTransport { * (`COALESCE(, '__global__')`) from the shared helper for the reason * ADR-0120 D3 records. Neither rule is re-decided here. * - * Columns that were never materialized (a virtual `formula` field) are - * skipped rather than emitted — the same choice `SqlDriver.syncDeclaredIndexes` - * makes, and for the same reason: DDL naming a column that does not exist - * fails the whole sync over an index nothing could have used. A name declared - * twice is emitted once, as the local face creates it once. + * Columns that were never materialized (a misspelt key name, a virtual + * `formula` field) are skipped rather than emitted — the same choice + * `SqlDriver.syncDeclaredIndexes` makes, and for the same reason: DDL naming a + * column that does not exist fails the whole sync over an index nothing could + * have used. [#20537] The skip is reported where the local face reports it, + * at `error`: on {@link durabilitySink}, not the diagnostic sink. A name + * declared twice is emitted once, as the local face creates it once. */ private buildDeclaredIndexDDL( tableName: string, @@ -2453,9 +2462,26 @@ export class RemoteTransport { for (const index of expected) { const missing = index.columns.filter((c) => !materializedColumns.has(c)); if (missing.length > 0) { - this.diagnosticSink?.( - `[RemoteTransport] skipping declared index "${index.name}" on "${tableName}" — ` + - `column(s) not materialized: ${missing.join(', ')}`, + // [#20537] Durability, not function — the durability sink, never the + // diagnostic one. The sync goes on and the object serves normally while + // DDL the metadata declares never runs; for a UNIQUE index that means + // duplicate rows are accepted. `SqlDriver.syncDeclaredIndexes` answers + // the same skip on the local face through `logDurabilityFailure`, for + // the same reason, and a PLAIN index takes the same channel on both + // faces (see {@link durabilitySink}). One line per skipped index per + // sync, and the line says which of the two kinds it is. + const columns = missing.map((c) => `'${c}'`).join(', '); + this.durabilitySink?.( + `[RemoteTransport] declared ${index.unique ? 'UNIQUE ' : ''}index "${index.name}" on "${tableName}" ` + + `was NOT created: no column for ${columns} (a key column must be a stored field of the object — a ` + + `name that is not a field, or a virtual formula field computed on read, has no column). ` + + (index.unique + ? `The uniqueness it declares is NOT enforced: duplicate rows are accepted, and nothing looks ` + + `broken from the outside. ` + : `Every query this index exists to serve is answered by scanning the whole table instead: ` + + `nothing looks broken from the outside and results stay correct. `) + + `Fix the metadata so every column in the index's fields is a stored field of the object, or ` + + `remove the index ("os validate" refuses a name that is not a field).`, ); continue; }