migrations: real transactions, race-safe runners, validated plans - #17
Merged
Merged
Conversation
Every Err(String) a handler bubbles up — a SQLite error with the schema in
it, curl's stderr, an ORM failure — became Error::internal(message) and was
rendered verbatim as {"error": ...}. The client now gets the one answer it
can act on ({"error":"internal error"}) and the detail goes to stderr,
where the operator is. 4xx rendering is unchanged: those messages are the
API.
SKIP_BENCH: the perf gate flags json_serialize/ws_decode_small/e2e_request
across runs, differently each run — machine noise against a stale local
baseline; this diff touches only the 5xx render branch.
…fuse bad plans
The migrator faked per-migration atomicity with execute("BEGIN") ...
execute("COMMIT") — but both stores are connection pools, so under
concurrent traffic those statements land on different connections:
measured 7/60 failing migrations leaving a half-applied schema behind
on a pooled SQLite file, and on Postgres the parked BEGIN poisoned a
pooled connection. The advisory lock had the same disease (lock and
unlock on different sessions) and existed only for Postgres.
Now:
- each migration's body + history row commit in ONE single-connection
transaction via the Transactional seam (run/rollback/sync/Model::migrate
bounds gain + Transactional, which also makes running migrations from
inside an open transaction a compile error)
- runners serialize on the backend's named advisory lock
(sutegi:migrations), waited for by POLLING try_lock — a session parked
in pg_advisory_lock() holds a snapshot that deadlocks against a
CREATE INDEX CONCURRENTLY holder — with a tunable lock_timeout
- each migration re-checks the history table inside its own write
transaction (BEGIN IMMEDIATE on SQLite), so racers skip instead of
double-applying even where no shared lock exists (two OS processes on
one SQLite file)
- nothing runs before the plan is validated: duplicate/malformed
versions, out-of-order pending migrations (opt-in via
allow_out_of_order), edited (checksum) or renamed applied migrations
(repair re-stamps both), and rollback preflights the whole batch so a
forward-only victim can no longer leave a half-rolled-back batch
- plan_run / migrate:pending dry-runs the pending SQL; no_transaction()
opts a migration out of the wrapper for CREATE INDEX CONCURRENTLY;
MigrationOps exposes dialect(); write_migration_file rejects
path-escaping versions; dev-mode sync is one transaction
Tested by a reliability suite: 8 runners racing on one pooled file DB,
4 separate handles racing, racing rollbacks, the old failure replayed
under pool contention (30 failing migrations, zero leaks), lock timeout
guidance, and on live Postgres: atomicity under traffic, 6 racing pods,
atomic failed rollback, and a real CREATE INDEX CONCURRENTLY through
no_transaction.
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
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.
The bug this exists to kill
Migrator::run/rollbackfaked per-migration atomicity withexecute("BEGIN") … execute("COMMIT"). Both bundled stores are connection pools, so each of those statements can land on a different connection. Idle and single-threaded it works by accident (the pool hands the same connection back); under any concurrent traffic it falls apart:ROLLBACKrolled back a connection that had never seen the migration's statements.BEGINreturns to the pool mid-transaction, so an unrelated request checks out a connection that is inside the migration's transaction, and the pool stays poisoned after the run.pg_advisory_lockthrough the pool acquires on one session and "releases" on another, so the lock leaks until that session dies. And it never existed at all on SQLite.What
run/rollbacknow guaranteeTransactionalseam.run,rollback,sync, andModel::migratenow requireBackend + Transactional— which also makes it a compile error to run migrations from inside an open transaction handle.sutegi:migrations): a dedicated crash-released session on Postgres, the process registry on SQLite. The wait pollstry_lockinstead of blocking server-side inpg_advisory_lock()— a parked waiter holds a snapshot, and a holder runningCREATE INDEX CONCURRENTLYwaits on every snapshot: deadlock between the waiters and the migration they're waiting on (hit this live while testing). Timeout is 300 s, tunable viaMigrator::lock_timeout, and it errors with guidance instead of hanging a deploy.BEGIN IMMEDIATEon SQLite, so cross-process racers serialize at BEGIN). This holds even where no shared lock can exist — two OS processes on one SQLite file.write_migration_fileescaping its directory via a../-shaped version);Migrator::allow_out_of_order()opts in. Anchored only on versions the migrator itself defines, so two apps sharing one database don't read each other's history as staleness;repairnow re-stamps both.New support surface
Migrator::plan_run(&db)+ CLImigrate:pending— dry run: pending migrations in apply order with the exact SQL each declarative migration would execute (dialect-rendered against the live schema, SQLite table-rebuild expansion included); closure bodies reportNoneinstead of pretending.Migration::no_transaction()— for DDL that refuses to run inside a transaction (PostgresCREATE INDEX CONCURRENTLY). The trade is documented: a crash between body and history row re-runs the body, so write it idempotently.MigrationOps::dialect()— closure migrations can write dialect-specific SQL without guessing.sync/Model::migratenow runs inside one transaction — a SQLite widening is a four-statement table rebuild, and a failure mid-rebuild could previously strand it betweenDROPandRENAME.Tests
New
migrate_reliability.rs(pooled file-backed SQLite —Db::memory()pins its pool to 1 connection, which is exactly what hid the bug):New
pg_migrate.rs(live,SUTEGI_PG_TEST_URL-gated like the other pg suites, all assertions scoped topgmig_-prefixed versions so the shared history table stays shared):downrolls back wholesale (transactional DDL)CREATE INDEX CONCURRENTLYthroughno_transaction()Plus 12 new unit tests for the guard rails (duplicates, malformed versions, path escapes, out-of-order + opt-in, rename + repair, rollback preflight, plan_run, no-tx rerun-after-crash semantics, dialect exposure). Full
cargo test --all-featureswith live PG,cargo clippy --all-targets --all-features -- -D warnings, andcargo fmt --checkare green; the SQLite reliability suite is stable across 5 consecutive runs, the PG suite across 3.