fix(server): reject WITHOUT ROWID tables in assertSyncCompatible#23
Merged
Conversation
…-0015) The cold-snapshot/fetch reader defaults to `ORDER BY rowid` when the client sends no orderBy (ADR-0015). A WITHOUT ROWID table has no rowid, so the read threw `no such column: rowid` and hung the subscriber — after the table had already passed registration. assertSyncCompatible now probes for a usable rowid and rejects such a table loudly at registerSync, alongside the existing INTEGER PRIMARY KEY guard. Probes the exact property the reader depends on rather than parsing the DDL; an unrelated exec failure rethrows unchanged. Corrects the ADR-0015 / sql-compiler claim that the rowid is "always intact" under D9 (D9 forbade only the INTEGER-alias pk, not WITHOUT ROWID). Ordinary rowid tables (id TEXT PRIMARY KEY) are unaffected. Adds a reason-pinning test; full suite 192 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adversarial review surfaced a shadow the SELECT-rowid probe alone misses: a user column named `rowid` resolves the probe (SQLite binds the name to the column), passes validation, then the compiler's default `ORDER BY rowid` silently sorts by that arbitrary column instead of insertion order. Reject a declared `rowid` column via the introspected pragma_table_info columns before the WITHOUT ROWID probe, which makes the probe unambiguous. Adds a test; docs and CHANGELOG updated to cover all three no-usable-rowid cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Follow-up from the #22 review. #22 made cold-snapshot/fetch reads deterministic by defaulting to
ORDER BY rowidwhen the client sends noorderBy(ADR-0015). That default assumes a realrowid— butassertSyncCompatibleonly rejected anINTEGER PRIMARY KEYpk, not aWITHOUT ROWIDtable.A synced table declared
CREATE TABLE t (id TEXT PRIMARY KEY, …) WITHOUT ROWIDclears every D9 check and registers fine, then the firstorderBy-less subscribe/fetch runsORDER BY rowidand throwsno such column: rowid. That error isn't on the caught path, so it propagates out ofwebSocketMessageand the subscriber just hangs (never receivessnap-end) — a silent failure, not a loud one.Narrow: only a table deliberately declared
WITHOUT ROWIDis affected. The documentedid TEXT PRIMARY KEYpattern is a rowid table and is untouched. Nothing in the wild breaks; this closes a latent gap.What changed
assertSyncCompatiblenow probesSELECT rowid FROM "<tbl>" LIMIT 0and rejects the table with a clear message if there's no rowid. It probes the exact property the reader depends on rather than parsing the DDL; an unrelatedexecfailure rethrows unchanged (guarded on the error message mentioningrowid).sql-compilercomment that the rowid is "always intact" under D9 — D9 forbade only theINTEGER-alias pk.assert-sync-compatible.test.tsstyle.Result
Fail-loud at
registerSyncinstead of a silent hang at read time, matching the repo's "reject; don't degrade" rule and leveraging the existingassertSyncCompatiblestructure.🤖 Generated with Claude Code