Split the database migration runtime into smaller files - #1965
Conversation
migrations.ts owned the lock, the marker writes, the runner, and the boot routing in one 830-line file. The lock, markers, runner, and migration toolbox now live in their own files, leaving migrations.ts as the boot path that decides what state the database is in and routes to them. No behaviour change: the same statements run in the same order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q9mAQvERz1MNvZxNGwmdHw
The mutation gate requires a mirror-located test per source file. The verify and apply retry tests move out of the integration suite into the runner's own test file, and the lock, markers, and toolbox modules get direct tests for their own contracts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q9mAQvERz1MNvZxNGwmdHw
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 47 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughChangesThe database migration runtime is decomposed into context, error, lock, marker, and runner modules. Migration loading and public helper imports are updated, lock ownership and marker batching are centralized, retry and recovery logic moves into the runner, and tests cover the new module boundaries and workflows. Migration runtime modularization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InitDb
participant MigrationRunner
participant MigrationLock
participant MigrationMarkers
participant Database
InitDb->>MigrationRunner: discover pending migrations
MigrationRunner->>MigrationLock: acquire migration lease
MigrationLock->>Database: store or validate lease
MigrationRunner->>MigrationRunner: apply and verify migrations
MigrationRunner->>MigrationMarkers: record progress and schema markers
MigrationMarkers->>Database: write migration state
MigrationMarkers->>MigrationLock: release migration lease
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a32d66aa8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review: drop the barrel re-exports the split left behind (tests now import from the focused modules), annotate the exported missing-table checkers, and keep the owned schema marker helper private. Each module's own tests now cover what its mutants can change: the boot messages, the four-per-request batch, the half-matching marker pair, the lock's bootstrap tolerance, the rename's repair steps, and the missing-table checkers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q9mAQvERz1MNvZxNGwmdHw
The half-matching marker pair, the 'uninitialized' check that needs both markers missing, the held-lock message, and the unset-vs-blank database name now each have a test that fails when the behaviour changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q9mAQvERz1MNvZxNGwmdHw
# Conflicts: # scripts/mutation/equivalent-mutants.txt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c0f416237
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/db/migrations.ts`:
- Line 30: Remove the wildcard re-export from the boot module by deleting the
export statement in src/shared/db/migrations.ts. Keep the existing direct
imports from the migrations errors module unchanged.
In `@src/shared/db/migrations/lock.ts`:
- Around line 49-55: The tolerated missing-settings path in acquireLock must not
return an unpersisted lock token. In src/shared/db/migrations/lock.ts lines
49-55, initialize the settings table before acquiring the lease or reacquire the
lease after initialization, ensuring a corresponding lock row exists before
returning a token. In test/shared/db/migrations/lock.test.ts lines 184-193,
replace the non-null-token assertion with a regression test that boots through
the tolerated path and verifies the first lock-gated marker or migration write
persists successfully.
In `@test/shared/db/migrations.test.ts`:
- Around line 742-752: Ensure both migration tests restore the shared database
even when assertions fail. In test/shared/db/migrations.test.ts:742-752, call
initDb immediately after rebuildWipedSchema and before asserting debugLines; in
test/shared/db/migrations.test.ts:754-772, wrap the assertions in a finally
block and perform invalidateInitDbCache, markMigrationsForRerun, and initDb
restoration there.
In `@test/shared/db/migrations/markers.test.ts`:
- Around line 80-82: Ensure both cleanup finally blocks in
test/shared/db/migrations/markers.test.ts at lines 80-82 and 96-99 call
releaseMigrationLock(lockToken!) before their existing marker cleanup
operations. Update both sites so the migration lease is always released even
when recordMigrationBatch fails, while preserving the existing
forgetTestMigration and marker-restoration behavior.
In `@test/shared/db/migrations/runner.test.ts`:
- Around line 275-296: Update the test around acquireMigrationLock to explicitly
assert that lockToken is present before passing it to runPendingMigrations and
releaseMigrationLock. Remove the non-null assertions and rely on the expectation
to narrow the type and provide clear failure semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7acec6fa-fc2b-4234-ae2d-26991517f70a
📒 Files selected for processing (29)
TODO.mdscripts/mutation/equivalent-mutants.txtsrc/shared/db/migrations.tssrc/shared/db/migrations/context.tssrc/shared/db/migrations/errors.tssrc/shared/db/migrations/lock.tssrc/shared/db/migrations/markers.tssrc/shared/db/migrations/runner.tstest/integration/code-quality.test.tstest/integration/db/group-listings.test.tstest/integration/db/migration-restore/helpers.tstest/integration/db/migration-runtime.test.tstest/integration/db/migrations/2026-06-14_rename_events_to_listings.test.tstest/integration/db/modifier-aggregates.test.tstest/integration/migration-restore-verify.test.tstest/integration/migration-round-trip-budget.test.tstest/integration/servicing/migration-edge-cases.test.tstest/integration/servicing/migration.test.tstest/shared/db/migrations.test.tstest/shared/db/migrations/2026-06-22_transfers_time_int.test.tstest/shared/db/migrations/context.test.tstest/shared/db/migrations/errors.test.tstest/shared/db/migrations/lock.test.tstest/shared/db/migrations/markers.test.tstest/shared/db/migrations/registry.test.tstest/shared/db/migrations/runner.test.tstest/shared/db/migrations/schema/triggers/listing-aggregates.test.tstest/test-utils/migrations.tstest/test-utils/test-state.ts
💤 Files with no reviewable changes (2)
- test/integration/code-quality.test.ts
- TODO.md
Drop the wildcard error re-export from the boot module, keep the marker-list builder private, and name the nullable settings lookup settingsValueOrNull. The boot-message and batching tests move into their own file so neither test file is oversized, tests restore the shared database in a finally block, and a takeMigrationLock helper replaces the non-null assertions on the lock token. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q9mAQvERz1MNvZxNGwmdHw
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q9mAQvERz1MNvZxNGwmdHw
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q9mAQvERz1MNvZxNGwmdHw
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b6b661d6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q9mAQvERz1MNvZxNGwmdHw
|
@coderabbitai review All five findings from the changes-requested review are addressed on
All four checks are green, but the standing changes-requested review is blocking auto-merge. Could you re-review and clear it if you're satisfied? Generated by Claude Code |
|
✅ Action performedReview finished.
|
2 participants
One file,
migrations.ts, did four separate jobs in 830 lines: it took the lock that stops two servers updating the same database at once, wrote the rows that record what has already run, ran the outstanding updates, and worked out which of those things the database needed in the first place.Each job now has its own file next to it:
migrations/lock.ts— taking, holding, and giving back the lock.migrations/markers.ts— the rows that record what has been applied.migrations/runner.ts— running each outstanding update and re-checking it.migrations/context.ts— the toolbox every dated update is handed.migrations.tsis left with the part that reads the database's current state and decides which of the above to use. It is now 409 lines.Nothing about how updates run has changed: the same statements run in the same order, in the same number of trips to the database, and every existing test still passes untouched.
The new files each gained their own tests. The retry tests for "run the update, then check it landed" moved out of the big integration file into the runner's own test file, and the lock, markers, and toolbox files got tests for their own behaviour — including one that proves a database with nothing outstanding writes nothing at all.
The matching item is removed from
TODO.md.Generated by Claude Code
Summary by CodeRabbit