diff --git a/CHANGELOG.md b/CHANGELOG.md index 036f0bd..8134996 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). While pre-1.0, the public API may change between 0.x releases. +## [Unreleased] + +### Fixed + +- **`registerSync` now rejects tables with no usable internal `rowid` (ADR-0015).** + The cold-snapshot/fetch reader defaults to `ORDER BY rowid` when the client + sends no `orderBy`. A `WITHOUT ROWID` table has no rowid, so that read threw + `no such column: rowid` and hung the subscriber; a table with a declared + `rowid` column shadows the internal one, so the read would silently sort by + that arbitrary column. `assertSyncCompatible` now rejects both loudly at + `registerSync`, alongside the existing `INTEGER PRIMARY KEY` guard. Ordinary + rowid tables (the documented `id TEXT PRIMARY KEY` pattern) are unaffected. + ## [0.5.0] — 2026-07-02 ### Added diff --git a/docs/adr/0015-syncable-mixin.md b/docs/adr/0015-syncable-mixin.md index 3ac74e7..3ab10b1 100644 --- a/docs/adr/0015-syncable-mixin.md +++ b/docs/adr/0015-syncable-mixin.md @@ -194,7 +194,13 @@ tddc design gap; a PR against Actors is possible in principle but out of scope. pk's autoindex over a rowid scan, returning pk-sorted rows instead of insertion order). `rowid` matches insertion order among currently-live rows and needs no schema change, since `assertSyncCompatible` (ADR-0007, D9) - already forbids the `INTEGER PRIMARY KEY` pk that would alias it. + already forbids the `INTEGER PRIMARY KEY` pk that would alias it. It also + forbids the two other ways a table can lack a usable internal rowid: a + `WITHOUT ROWID` table (none at all — the read would throw `no such column: + rowid` and hang the subscriber) and a declared `rowid` column (which shadows + the internal one, so `ORDER BY rowid` would silently sort by that arbitrary + column). All three are rejected at `registerSync`, not left to surface at read + time. - One DO class can be both a framework host and a sync source (`class FeedAgent extends Syncable()(Agent)`), with no framework added to tddc's dependency graph — the app supplies `Base`. diff --git a/src/server/changes.ts b/src/server/changes.ts index b48621a..c1fe98a 100644 --- a/src/server/changes.ts +++ b/src/server/changes.ts @@ -119,10 +119,13 @@ export function ensureTriggers( * Validate that an author-created table is sync-compatible (ADR-0007, D9): the * declared `pk` must be the table's SOLE primary key and have TEXT affinity (a * client-supplied stable key: TEXT/VARCHAR/CHAR/…). Rejects a missing table, a - * composite/wrong pk, and an `INTEGER PRIMARY KEY` (the rowid alias — - * server-assigned, which breaks optimistic id parity). Introspects the real - * table, so it works however the author created it. `pragma_table_info` is the - * table-valued form, so the table name binds as a parameter. + * composite/wrong pk, an `INTEGER PRIMARY KEY` (the rowid alias — server-assigned, + * which breaks optimistic id parity), and any table that lacks a usable internal + * `rowid` — a `WITHOUT ROWID` table (which has none) or one with a declared + * `rowid` column (which shadows it) — because the cold-snapshot/fetch reader + * orders by `rowid` when the client sends no orderBy (ADR-0015). Introspects the + * real table, so it works however the author created it. `pragma_table_info` is + * the table-valued form, so the table name binds as a parameter. */ export function assertSyncCompatible(sql: SqlStorage, tbl: string, pk: string): void { const cols = Array.from( @@ -147,6 +150,38 @@ export function assertSyncCompatible(sql: SqlStorage, tbl: string, pk: string): `key aliases rowid (server-assigned) and breaks optimistic id parity (D9).`, ) } + // The cold-snapshot/fetch reader defaults to `ORDER BY rowid` when the client + // sends no orderBy (ADR-0015), so a synced table must expose SQLite's internal + // rowid — the insertion-order key that default is built on. Two ways an author + // can defeat that, both rejected here at registerSync rather than left to + // surface at read time: + // + // 1. A declared column named `rowid` SHADOWS the internal rowid, so + // `ORDER BY rowid` would silently sort by that arbitrary user column + // (and lose determinism on null/duplicate values). Check the introspected + // columns directly. + if (cols.some((c) => c.name.toLowerCase() === "rowid")) { + throw new Error( + `collection '${tbl}': a column named 'rowid' shadows SQLite's internal rowid, which the ` + + `snapshot/fetch reader orders by when the client sends no orderBy (ADR-0015). Rename that column.`, + ) + } + // 2. A `WITHOUT ROWID` table has NO rowid at all, so `ORDER BY rowid` throws + // `no such column: rowid` and hangs the subscriber. With a shadowing + // column already ruled out, probing the exact property the reader relies + // on is unambiguous — a compile-time "no such column: rowid" is the + // signal; anything else is an unrelated failure and rethrows unchanged. + try { + sql.exec(`SELECT rowid FROM "${tbl}" LIMIT 0`) + } catch (e) { + const msg = e instanceof Error ? e.message : String(e) + if (!/rowid/i.test(msg)) throw e + throw new Error( + `collection '${tbl}': table is WITHOUT ROWID — sync requires a rowid table so snapshot/fetch ` + + `reads have a deterministic default order (ORDER BY rowid, ADR-0015). Recreate '${tbl}' ` + + `without the WITHOUT ROWID clause.`, + ) + } } /** diff --git a/src/server/sql-compiler.ts b/src/server/sql-compiler.ts index 0537d8f..e0a1b34 100644 --- a/src/server/sql-compiler.ts +++ b/src/server/sql-compiler.ts @@ -191,8 +191,10 @@ export function compileSubsetQuery(tbl: string, opts: SubsetQuery): { sql: strin // `rowid` also matches insertion order among currently-live rows: a rowid // table assigns each new row 1 + the current max, which is monotonic // across inserts regardless of intervening deletes. Every synced table - // qualifies — `assertSyncCompatible` (ADR-0007, D9) forbids an `INTEGER - // PRIMARY KEY` pk, so the real rowid is always intact underneath. + // exposes a real internal rowid: `assertSyncCompatible` (changes.ts) forbids + // an `INTEGER PRIMARY KEY` pk (which aliases it), a `WITHOUT ROWID` table + // (which has none), and a declared `rowid` column (which shadows it), so this + // reference always resolves to insertion order. sql += ` ORDER BY rowid` } diff --git a/tests/assert-sync-compatible.test.ts b/tests/assert-sync-compatible.test.ts index 32312ae..23ce890 100644 --- a/tests/assert-sync-compatible.test.ts +++ b/tests/assert-sync-compatible.test.ts @@ -76,4 +76,28 @@ describe("assertSyncCompatible (ADR-0007, D9) — real-table introspection", () expect(() => assertSyncCompatible(sql, "t", "id")).toThrow(/sole PRIMARY KEY/) }) }) + + it("rejects a WITHOUT ROWID table (no rowid for the default snapshot order)", async () => { + // The pk is a valid sole TEXT key, so it clears every D9 check — but a + // WITHOUT ROWID table has no rowid, and the cold-snapshot/fetch reader + // defaults to `ORDER BY rowid` (ADR-0015). Without this guard the table + // registers, then the first orderBy-less sub throws `no such column: rowid` + // at read time and hangs the subscriber. Reject loudly at registerSync. + await withSql((sql) => { + sql.exec("CREATE TABLE t (id TEXT PRIMARY KEY, body TEXT) WITHOUT ROWID") + expect(() => assertSyncCompatible(sql, "t", "id")).toThrow(/WITHOUT ROWID/) + }) + }) + + it("rejects a declared `rowid` column that shadows SQLite's internal rowid", async () => { + // A user column named `rowid` clears every D9 check AND makes `SELECT rowid` + // resolve — but it shadows the internal insertion-order rowid, so the + // reader's default `ORDER BY rowid` (ADR-0015) would sort by this arbitrary + // column and lose determinism on null/duplicate values. Probing alone can't + // see the shadow; reject the declared column explicitly. + await withSql((sql) => { + sql.exec("CREATE TABLE t (id TEXT PRIMARY KEY, rowid TEXT)") + expect(() => assertSyncCompatible(sql, "t", "id")).toThrow(/shadows/) + }) + }) })