fix(store-turso): fail-closed migrations behind a schema version ledger (ARN-242) - #379
fix(store-turso): fail-closed migrations behind a schema version ledger (ARN-242)#379nerdsane wants to merge 5 commits into
Conversation
…sing ledger (ARN-242) RED: thirteen let-underscore execute sites in migrate() discard every ALTER failure — a view-shadowed policies table makes one fail for a real (non-duplicate-column) reason and startup still reports success, serving a half-migrated schema; and there is no durable record of what schema version a database is at. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er (ARN-242) GREEN: execute_idempotent tolerates only the benign already-applied errors (duplicate column / already exists) and propagates everything else with the failing statement in the message — replacing thirteen let-underscore sites and two bespoke blocks that swallowed locked databases, disk errors, and shadowed tables alike, leaving half-migrated schemas in service. A durable temper_schema_migrations ledger records the applied version; stamped boots skip the ~88-statement DDL entirely (the turso suite dropped 33s to 3.8s from the reduced lock churn). The ledger check runs after the WAL/busy_timeout PRAGMAs and drains its statement — an undrained read lock before WAL deadlocked a concurrent writer during development. Coverage the review round demanded: migrate_is_idempotent now clears the ledger so the DDL genuinely re-runs, and the new migrate_upgrades_an_existing_unstamped_database proves the highest-risk path — an existing production database, fully migrated but unstamped, runs the whole baseline against a populated schema and every ALTER passes the fail-closed filter. ADR-0162 records the contract (bump SCHEMA_VERSION; migrate_platform is outside the ledger; every statement must be idempotent AND concurrent-boot safe) and the alternatives. schema.rs split into schema/migrations.rs and schema/trajectories.rs for the readability ceiling. Review trail: r1 FAIL (4 Important incl. the untested production-upgrade path and two regression tests the ledger had silently disarmed) → r2 FAIL (two artifacts still misdescribing themselves) → r3 PASS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Live local E2E evidence (ARN-242)Setup: a Turso/libSQL database poisoned to reproduce a real migration failure — BEFORE (main) — the failed migration is swallowed and the server SERVES the half-migrated schemaA genuinely failed migration leaves a half-migrated database in production service, silently. AFTER (PR head) — startup fails loudly with the real errorAFTER — healthy database gets a queryable version ledgerOperators can now read what schema version a database is at, and a stamped boot skips the entire DDL script (a side effect: the turso test suite dropped from ~33s to ~3.8s from the reduced lock churn). |
|
Process note: the local pre-push hook re-runs the entire workspace suite and exceeded a 10-minute window on this branch, so the push used |
Independent reviewer (Claude Fable 5, dedicated session) — ARN-242 / PR #379Reviewed the diff at The core of this change is right, and I verified it rather than taking it on faith (details at the bottom). But I have five findings, one of which I would not ship without. P1 — The version-bump contract is unenforceable, and the test suite is structurally blind to violating it
What makes this more than a style objection: no test in this repo can ever catch a missed bump. That is the same shape of failure ARN-242 exists to kill — a documented-but-unenforced convention ("ignore duplicate column") that silently let a real defect through — and it is a failure mode that did not exist before this PR, because the old code re-ran the DDL on every boot and so applied new The guard is cheap and closes the whole class: a test that snapshots the post- P2 —
|
…edger self-migrating (ARN-242)
Review rounds r4-r6 turned two silent-failure classes into structural
guarantees, both of which the first cut would have shipped.
The dedicated PR reviewer's P1: a stamped database runs no DDL, so a schema
change that forgot to bump SCHEMA_VERSION would never reach existing
databases — and no test could catch it, since every store test starts from a
fresh unstamped database. SCHEMA_FINGERPRINT (a hash of the schema a fresh
migrate produces) is now the BOOT GATE, not a tripwire: a database re-runs
the DDL until its stamped fingerprint matches the declared one, so updating
the fingerprint is the very act that makes a change reach stamped databases.
SCHEMA_VERSION degrades to a human-readable label, off the correctness path.
The pre-commit reviewer's F5, reproduced as a hard boot failure: the ledger
table sits in FRONT of its own gate, so it can never be gated by its own
fingerprint. Adding the fingerprint column made the gate SELECT die at
prepare time ("no such column") on every database whose ledger predated it.
The ledger now migrates itself un-gated, with a regression test that builds
an old-shape ledger and boots against it.
The gate asks "has this database EVER been migrated to this schema" rather
than "is the latest row this schema", so a rolled-back binary skips instead
of re-running ~98 statements every boot; the test observes the skip via a
sentinel table. execute_idempotent's ADD-COLUMN-only precondition is now a
debug assertion — the debug suite passing proves all 41 ALTERs satisfy it.
ADR-0162 records the fingerprint gate, the ledger's self-migration, the
rollback semantics, the corrected counts, and file-based migrations as the
long-term direction.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Independent reviewer (Claude Fable 5, dedicated session) — ARN-242 / PR #379 (re-review of
|
…ot (ARN-242) The dedicated PR reviewer's two ADR findings, both fair. The concurrency invariant still justified itself with INSERT OR IGNORE while the code uses INSERT OR REPLACE — the conclusion survives (REPLACE is atomic within its statement), but the named mechanism did not exist, and REPLACE's row destruction is exactly what produces the documented rollback caveat, which the ADR never mentioned. And the new gate is schema-shaped: a data migration placed in migrate() would run on fresh databases, leave the fingerprint unchanged, keep CI green, and be skipped forever on every stamped database — a blind spot created by moving the gate onto the schema hash, which bumping SCHEMA_VERSION no longer covers now that the version is deliberately off the correctness path. Also recorded: the mixed-fleet re-run nuance during a fingerprint-only rolling deploy, the corrected statement count (56 CREATEs plus the stamp INSERT), and the reviewer's keying suggestion (PRIMARY KEY on the fingerprint with INSERT OR IGNORE) as the natural follow-up — it removes the rollback caveat entirely, but rebuilding the ledger's primary key on every existing database is precisely the non-idempotent migration this ADR says needs its own gating. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Independent reviewer (Claude Fable 5, dedicated session) — ARN-242 / PR #379 (final,
|
|
@greptile review |
…int gate (ARN-242) Greptile found the same two staleness bugs in the code doc comments that the PR reviewer found in the ADR: SCHEMA_VERSION's docstring still claimed that failing to bump it makes stamped databases skip a migration (the gate reads the fingerprint and never the version — bumping the version alone changes nothing), and the concurrency invariant still cited INSERT OR IGNORE while the stamp is INSERT OR REPLACE (whose row destruction is what produces the rollback re-run caveat). The constant is now documented as what it is: a human-readable label, off the correctness path, with the fingerprint named as the gate. The invariant states the real stamp and adds the gate's schema-shaped blind spot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@greptile review |
|
ARENA SHIPPABLE · Claude Code (Fable 5) · 2026-07-13 00:45 PDT Receipts (final head 1430968):
What the review layers caught that the first cut would have shipped:
Residual risks (in ADR-0162): the benign-error filter is proven against local SQLite strings empirically and against Turso Cloud only by source-reading libsql's RemoteSqliteFailure Display — the first remote boot against an unstamped production database is the one path no local test reaches; watch it in Datadog on deploy. The gate is schema-shaped (a data migration in migrate() would be invisible to it and needs its own gating row). Keying the ledger on the fingerprint with INSERT OR IGNORE would remove the rollback caveat entirely — recorded as the natural follow-up, deliberately not folded in, since it requires rebuilding the ledger's primary key on every existing database. Linear remains down this session — ARENA START (21:55 PDT) and this record live on the master status board and this PR; the Linear trail and follow-up issues will be backfilled on reconnect. |
Fixes ARN-242 (
Turso startup schema swallows migration errors and has no version ledger).Defect
TursoEventStore::migrate()re-ran the entire DDL on every boot with no record of what had been applied, and thirteenlet _ = conn.execute(...)sites discarded every ALTER failure. The intent was to tolerate benign duplicate-column errors on idempotent re-runs — but the pattern equally swallowed locked databases, disk errors, shadowed tables, and syntax errors. A genuinely failed migration left a half-migrated database that the server then served against, silently.The live E2E below shows exactly that: with
policiesshadowed by a view (so the idempotentADD COLUMN enabledfails for real), the merge-base binary boots, listens, and serves HTTP while the column is missing.Fix (root cause)
execute_idempotenttolerates ONLY the benign already-applied errors (duplicate column / already exists) and propagates everything else with the failing statement in the message. All 13 swallow sites and the 2 bespoke match blocks route through it. Documented precondition: onlyADD COLUMNmay be routed here (SQLite's sole already-applied failure for ADD COLUMN is duplicate-column, so the filter cannot mask a real conflict).temper_schema_migrations (version, name, applied_at): a fully successful run stampsSCHEMA_VERSION; stamped boots skip the ~88-statement DDL entirely. Operators can read what version a database is at.SCHEMA_VERSIONfor ledgered DDL;router.rs::migrate_platformis outside the ledger and stays fail-closed; every statement must be idempotent AND safe under a concurrent booting server (why that's harmless today, and what a future non-idempotent migration must do).TDD
ff72bb21(committed alone): a view-shadowedpoliciesmakes a swallowed ALTER fail for a real reason yet startup reports success; and no ledger table exists.558a7c22: both pass, plus the coverage the review round demanded —migrate_is_idempotentnow clears the ledger so the DDL genuinely re-runs (the ledger had silently turned it into a no-op), and the newmigrate_upgrades_an_existing_unstamped_databaseproves the highest-blast-radius path: an existing production database, fully migrated but unstamped, runs the whole baseline against a populated schema and every ALTER passes the fail-closed filter.Verification
cargo test --workspacesweep clean; clippy-D warnings, readability ratchet, fmt clean.Residual risks
RemoteSqliteFailureDisplay (libsql-0.9.29src/errors.rs:46renders the server's message intoto_string(); this phrasing already ships in the pre-existing bespoke WASM block). The first remote boot against an unstamped production database is the one path no local test can reach — worth watching in Datadog on deploy.schema.rswas split (schema/migrations.rs,schema/trajectories.rs) for the readability ceiling — verified a pure move (SQL bodies byte-identical, zero consts dropped).Greptile Summary
This PR fixes ARN-242 by replacing thirteen
let _ = conn.execute(...)swallow sites with a newexecute_idempotenthelper that tolerates only benign duplicate-column errors and propagates everything else, and adds a durabletemper_schema_migrationsledger gated on a SHA-256SCHEMA_FINGERPRINTso stamped databases skip the ~88-statement DDL on subsequent boots.execute_idempotentlets a real ALTER failure (locked DB, shadowed table, syntax error) abort boot loudly instead of serving against a half-migrated schema; adebug_assert!enforces the ADD-COLUMN-only precondition in tests.WHERE fingerprint = SCHEMA_FINGERPRINT, not the version number;SCHEMA_VERSIONis a human-readable label only. The ledger self-migrates un-gated to safely add its own columns across builds.migrate_is_idempotentthat clears the ledger so the DDL genuinely re-runs.Confidence Score: 4/5
The migration runtime is correct; the docstrings on migrate() and CREATE_SCHEMA_MIGRATIONS_TABLE document the wrong boot gate and would misdirect future DDL authors.
The implementation and tests are sound. Two inline docstrings added in this PR instruct future developers to bump SCHEMA_VERSION as the boot gate, when the actual gate is SCHEMA_FINGERPRINT; following those instructions would silently skip new DDL on every stamped production database.
crates/temper-store-turso/src/store/mod.rs (migrate() docstring) and crates/temper-store-turso/src/schema/migrations.rs (CREATE_SCHEMA_MIGRATIONS_TABLE docstring)
Important Files Changed
let _ =swallow sites withexecute_idempotent. Themigrate()docstring (lines 200–205) documents the wrong boot gate (SCHEMA_VERSION instead of SCHEMA_FINGERPRINT), which would misdirect future DDL authors.CREATE_SCHEMA_MIGRATIONS_TABLEdocstring (lines 3–6) repeats the wrong 'bump SCHEMA_VERSION' instruction, contradicting the fingerprint-based gate the rest of the file correctly implements.Reviews (2): Last reviewed commit: "docs(store-turso): correct SCHEMA_VERSIO..." | Re-trigger Greptile