ReviewOS

also looking at this

stacks/bun-query-builder

fix(migrate): apply each migration atomically, and one run at a time

#1076
Merged glennmichael123 wants to merge fix/migrate-transaction-lock into main
3 files +364 -54
10 days ago

Closes #1067.

The runner applied each .sql file with no transaction and no lock, recording it afterwards. Two holes.

Partial application

A file that fails partway leaves the statements before the failure committed. On SQLite the wrapper splits a file and runs it statement by statement, so a two-statement file whose second statement is invalid left the first table behind — neither applied nor cleanly re-runnable:

unfixed:  tables left behind: ["migrations", "mig_a"]
fixed:    tables left behind: ["migrations"]

Each file and its ledger row now go in as one transaction, which also closes the matching gap at the other end: schema changed, process died before recording.

Postgres does not reproduce the statement-level hole through this path — Bun sends the file as one multi-statement query and the server already wraps it. Worth saying plainly, because it means my first attempt at this test passed against unfixed code and proved nothing. The wrap still closes the apply/record gap there.

Not every file can be wrapped

Pretending otherwise would be worse than not wrapping:

  • MySQL commits DDL implicitly. A transaction there commits mid-file and reports a success it can't deliver. Those files stay unwrapped and log why.
  • Postgres rejects CREATE INDEX CONCURRENTLY and VACUUM inside a transaction block — and building an index concurrently is exactly what a migration on a live table should do. Files containing one keep the old behaviour rather than failing on something they were right to write.

Concurrent runs

Two processes booting together — a rolling deploy — both read the same pending set and both applied it. Now serialised on an advisory lock, with the ledger read inside the lock, since computing pending before locking is precisely what lets both agree on the same work.

That needed advisoryUnlock(), which didn't exist. Its absence made the two existing lock helpers a trap rather than a tool: a Postgres session lock lives until released or the session ends, and a pooled connection does not end when it returns to the pool — so advisoryLock() stranded the lock on whatever connection served it, and the next caller waited forever. The lock is taken on a reserved connection and released on the same one. SQLite has no primitive and a single-writer file, so it runs unlocked rather than throwing.

The Postgres key hash was transcribed separately in lock, try-lock, and now unlock. Any drift means unlock computes a different key than lock did and releases nothing — so it's one function now.

Tests

Atomicity on SQLite (where the hole is real), locking on a live Postgres in spawned subprocesses. The concurrency test uses pg_sleep to guarantee the two runs overlap — without it they may simply not contend, and a concurrency test that only sometimes exercises concurrency is worse than none.

Verified both discriminate: against the unfixed runner, both fail; with it, both pass.

One thing worth knowing about the locking test: it shares the suite's Postgres database, so it asserts on its own ledger row rather than a total count. My first version counted all rows and passed alone but failed in the full suite — other files leave rows behind.

Typecheck 0, lint 0 errors, suite 1701 pass / 4 skip / 0 fail.

🤖 Generated with Claude Code

3 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.