fix(store-turso): fail-closed migrations with schema ledger (ARN-242) - #394
fix(store-turso): fail-closed migrations with schema ledger (ARN-242)#394rita-aga wants to merge 7 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>
…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>
…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>
…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>
Sibling arena branches claimed 0162 (claude) and 0171 (codex). Use 0174 for the Grok-line ARN-242 record so the ADR number is unique.
Independent Grok code review (ARN-242 / ADR-0174)SummaryCorrect root-cause fix for swallowed Turso migrations: all former What was done well
FindingsImportant
Suggestions
Plan alignmentImplements ADR-0174 decisions 1–7. Tests cover fail-closed ALTER, ledger stamp, fingerprint contract, ledger self-migrate, rollback skip, unstamped upgrade, and re-armed idempotency. Residual risks
No Critical runtime defects in the migration path itself; residual Important is documentation that still contradicts the fingerprint gate on the primary API surface. Verdict: PASS |
|
@greptile review |
ARENA SHIPPABLE · Grok · 2026-07-14 11:57 PDTPR: #394 Checklist
Summaryturso migration ledger |
…N-242) Promote the ADD COLUMN-only guard from debug_assert! to assert! so a CREATE routed through execute_idempotent cannot reintroduce silent swallow of "already exists" in release builds. Document that sqlite_test_url is UUID-unique so the poison-DB test cannot short- circuit on a leftover stamp.
|
@greptile review |
1 similar comment
|
@greptile review |
|
Greptile P2 on poison-DB fingerprint: test creates a fresh file URL never stamped by TursoEventStore, so the ledger gate is not yet active — intentional isolation of the ALTER-poison path. No code change; residual accepted. |
Grok arena ARN-242. ADR-0174. RED→GREEN. CI is full gate. No merge.
Greptile Summary
This PR implements fail-closed schema migrations with a durable fingerprint ledger (ARN-242) for the Turso event store, replacing 13
let _ = conn.execute(...)swallow sites with a typedexecute_idempotenthelper that only tolerates benign duplicate-column errors and propagates everything else as a boot failure.temper_schema_migrationstable records the SHA-256 fingerprint of the applied schema;migrate()short-circuits on a fingerprint match and fails startup on any real error, replacing silent half-migration with loud boot failure.CREATE TABLEandALTER TABLE ADD COLUMN fingerprintrun before the gate query) to survive upgrades from pre-fingerprint ledger schemas.Confidence Score: 5/5
Safe to merge — the fail-closed migration path, ledger bootstrap, and fingerprint gate are all correct and well-tested.
Every former swallow site is correctly converted to execute_idempotent, the fingerprint gate short-circuits cleanly on stamped databases, the ledger self-migration correctly handles the bootstrap problem, and six targeted tests cover the critical upgrade paths. The only finding is a misleading doc comment that contradicts the implementation but is caught by CI.
The doc comment on migrate() in store/mod.rs mis-describes the boot gate; no other file requires special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A([migrate called]) --> B[Set WAL + busy_timeout PRAGMAs local only] B --> C[CREATE TABLE IF NOT EXISTS temper_schema_migrations] C --> D[execute_idempotent ALTER ADD COLUMN fingerprint no-op if column exists] D --> E{SELECT EXISTS WHERE fingerprint = SCHEMA_FINGERPRINT} E -- already applied --> F([return Ok]) E -- not applied --> G[Run full DDL ~98 CREATE IF NOT EXISTS and ADD COLUMN via execute_idempotent] G --> H{Any real error?} H -- duplicate column / already exists / already has --> I[tolerated no-op] I --> G H -- other error --> J([fail boot log statement + error]) G -- all done --> K[INSERT OR REPLACE version name SCHEMA_FINGERPRINT into ledger] K --> L([return Ok ledger stamped])%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A([migrate called]) --> B[Set WAL + busy_timeout PRAGMAs local only] B --> C[CREATE TABLE IF NOT EXISTS temper_schema_migrations] C --> D[execute_idempotent ALTER ADD COLUMN fingerprint no-op if column exists] D --> E{SELECT EXISTS WHERE fingerprint = SCHEMA_FINGERPRINT} E -- already applied --> F([return Ok]) E -- not applied --> G[Run full DDL ~98 CREATE IF NOT EXISTS and ADD COLUMN via execute_idempotent] G --> H{Any real error?} H -- duplicate column / already exists / already has --> I[tolerated no-op] I --> G H -- other error --> J([fail boot log statement + error]) G -- all done --> K[INSERT OR REPLACE version name SCHEMA_FINGERPRINT into ledger] K --> L([return Ok ledger stamped])Comments Outside Diff (1)
crates/temper-store-turso/src/store/mod.rs, line 415-418 (link)CREATE TABLEorCREATE INDEXstatement throughexecute_idempotentwould have the assertion stripped away, and an "already exists" error from a genuine object-name collision would be silently swallowed — exactly the class of silent failure this function was created to eliminate. Changingdebug_assert!toassert!keeps the guard active in all build modes and makes the precondition unconditional.Prompt To Fix With AI
Reviews (3): Last reviewed commit: "fix(store-turso): keep execute_idempoten..." | Re-trigger Greptile