Decide against a non-transactional migration runner, and record the trigger - #166
Conversation
…d the trigger Closes the open item in docs/design-sweep/SECOND-PASS.md section 4b. CodeRabbit raised it on #163, it was answered there, and the reviewer withdrew the finding — but the reasoning lived only in a PR thread and a "why deferred" table cell. Decision: keep drizzle's stock migrate(). Reading the migrator source makes the case stronger than the deferral recorded: - it wraps the ENTIRE pending batch in one transaction, not one per migration (pg-core/dialect.cjs: session.transaction sits outside the for-await loop) - __drizzle_migrations is read as a high-water mark (order by created_at desc limit 1), not a set of applied hashes So a custom runner's real risk is not the INVALID index, it is decoupling the DDL from the bookkeeping insert: a mid-batch failure commits statements the high-water mark still sits behind, the retry re-runs them and dies on "already exists", and the deploy wedges inside the gate. A failed CONCURRENTLY build is precisely the case that triggers it. Against that, the avoided cost is a sub-second SHARE lock. The COMMIT;-in-the-file hack is worse than inelegant: because the transaction spans the batch, it also commits every pending migration before it and runs every one after it untransacted. If the trigger ever fires, apply such an index out-of-band, watched, with a documented manual bookkeeping insert — failure mode stated in the doc. Also corrects two prose claims found while writing this up: sync_run is not purged (so it is unbounded too, just slower), and 0002/0003 build indexes against tables created in 0000, so those were not necessarily empty either. No change to how migrations are applied. src/db/migrate.ts gains a comment only.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Comment |
1 participant
Answers the open item in
docs/design-sweep/SECOND-PASS.md§4b: does authGD need a migration runner that can apply statements outside a transaction (soCREATE INDEX CONCURRENTLYbecomes expressible)?No — and the reasoning now lives somewhere durable instead of in a PR thread.
CodeRabbit raised this on #163, it was answered there, and the reviewer withdrew the finding. This wasn't an open disagreement; it was a settled call recorded only as a "why deferred" table cell.
What reading the migrator changed
Two facts from
drizzle-orm/pg-core/dialect.cjsmake the case stronger than the deferral recorded, and correct it:session.transactionsits outside thefor awaitloop. SECOND-PASS.md said "wraps every migration in one," which understated it.__drizzle_migrationsis a high-water mark (order by created_at desc limit 1, compared with<), not a set of applied hashes. Anything that advances it out of order is not self-correcting.So a custom runner's real risk isn't the INVALID index everyone focuses on — it's decoupling the DDL from the bookkeeping insert. A mid-batch failure then commits statements the high-water mark still sits behind; the retry re-runs them and dies on "already exists", wedging the deploy inside the gate that
fly.tomlruns on every release. A failedCONCURRENTLYbuild is precisely the case that triggers this. The one feature the runner would exist for is the one that detonates it.Against that, the avoided cost is a sub-second
SHARElock — writes block, reads don't.The
COMMIT;-in-the-file hack is worse than inelegant: because the transaction spans the batch, aCOMMIT;in one file also commits every pending migration before it and runs every one after it untransacted — a non-local effect that only appears on a deploy carrying more than one pending migration. It also requires hand-editing a generated migration, which this project forbids.What's recorded
A new
docs/ops.mdsection, Migrations run in one transaction — deliberately:audit_logINSERT, and getting it wrong in either direction is bad (skip it → deploy fails on "already exists"; wrongcreated_at→ silently masks other pending migrations)Two prose corrections found while writing this up
code-reviewercaught both, and they were mine:sync_runis never purged.purge.tscovers expired sessions, spent OAuth transactions, and dispatched outbox rows only. I'd called it "bounded" — it is unbounded too, just ~an order of magnitude slower thanaudit_log. Flagged in the doc, deliberately not fixed here: it wants its own retention policy and that's a separate change.0002/0003build indexes against tables created in0000, so those weren't necessarily empty either. The real distinguishing argument is table size, not emptiness.Scope
No change to how migrations are applied — that's a red line and I didn't cross it.
src/db/migrate.tsgains a comment only, pointing at the runbook from the file where someone will actually ask the question.Verification
80 test files vs #163's 77 baseline, so nothing silently dropped.
npm run buildrun locally since CI is currently broken.The documented bookkeeping command was proven end-to-end rather than by code-reading: extracted verbatim from
ops.md, it emits327e0000…74a8, 1786046498461— byte-identical to the real__drizzle_migrationsrow in a live database with0010applied. (First draft of that command embedded a'via${"'"}inside a shell single-quoted string, which would have broken on paste; it now builds the quote withString.fromCharCode(39).)Reviewer focus
docs/ops.md's claims about drizzle internals — they're the load-bearing part and they're all checkable againstnode_modules/drizzle-orm/pg-core/dialect.cjsandmigrator.cjs. If the batch-transaction or high-water-mark reading is wrong, the recommendation changes.