Keep database upgrades within edge request limits - #1845
Conversation
![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: 29411be317
ℹ️ 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".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigration initialization now executes migrations in request-sized batches, records progress and lock release atomically, and preserves partial progress after failures. Lock ownership and release failures are explicitly handled, with tests covering schema restoration, continuation behavior, and lock edge cases. ChangesMigration batching and restoration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant initDbUncached
participant MigrationRunner
participant Database
Request->>initDbUncached: initialize database
initDbUncached->>MigrationRunner: acquire request-specific lock token
MigrationRunner->>Database: run request-sized migration batch
MigrationRunner->>Database: verify token and record markers
Database-->>MigrationRunner: release lock or retain it for continuation
initDbUncached-->>Request: complete or report migration continuation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
stefan-burke
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Around line 699-708: Update the finally block in the migration flow around
releaseMigrationLock so release failures propagate instead of being caught and
reduced to a debug log. Preserve the original migration error when one already
exists, while ensuring a release failure is surfaced when it is the only
failure; remove the catch-and-continue behavior.
- Around line 521-524: Update acquireMigrationLock and its completion cleanup to
propagate an ownership token for the current lease, delete the settings row only
when both key and token match, and verify the token still owns the lease before
recording migration completion. Preserve the existing lock TTL and completion
behavior for the request that retains ownership.
In `@test/shared/db/migration-round-trip-budget.test.ts`:
- Line 73: Update the continuation-count assertion in the migration round-trip
test to derive the expected value from the production batch-size constant used
after LISTINGS_TAG_MIGRATION_ID. Export or reuse that constant from the
production migration code, import it in the test, and replace the hard-coded 3
while preserving the existing assertion behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65cc146f-87e3-4d21-bdcd-4f387d93ec90
📒 Files selected for processing (4)
src/shared/db/migrations.tssrc/shared/db/query-log.tstest/lib/db/migration-restore/helpers.tstest/shared/db/migration-round-trip-budget.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Around line 537-546: Update the catch branch surrounding
executeWhileMigrationLockOwned so failures writing completed migration markers
are captured instead of replacing the original migration error; when both
operations fail, throw an AggregateError containing both errors, while
preserving the original error when marker persistence succeeds. Add a regression
test covering the completed non-empty path where marker writing fails, and
verify both failures are retained.
In `@test/lib/db/migration-runtime.test.ts`:
- Around line 73-83: Update markMigrationPending and the related migration test
queries to use the shared production constants for schema_migrations,
migration_lock, latest_db_update, and db_schema_hash instead of hard-coded
identifiers. Import the existing constants from production code and substitute
them in every affected SQL statement, including the additionally referenced
range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef17395e-f0bb-4d6e-8897-e3b4cb221563
📒 Files selected for processing (2)
src/shared/db/migrations.tstest/lib/db/migration-runtime.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/db/migrations.ts (1)
1-805: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile has grown well past the ~400-line guideline.
migrations.tsnow spans to at least line 805 (per the diff's own line-range annotations forinitDbUncached), several times the guideline size. This PR adds more logic on top (combinedFailures,recordMigrationBatch,runAcquiredMigrations,releaseAfterMigrationFailure, lock/lease handling) to an already large module. Consider splitting lock acquisition/release, batch-marker recording, and the pure migration-runner logic into separate focused modules, keeping I/O (DB calls) separate from pure logic per the same guideline.As per coding guidelines, "Keep files under approximately 400 lines, splitting large modules and tests into focused files and separating pure logic from I/O."
🤖 Prompt for 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. In `@src/shared/db/migrations.ts` around lines 1 - 805, Split the oversized migrations module into focused modules, keeping initDb orchestration and schema migration behavior unchanged. Extract migration lock/lease operations around acquireMigrationLock and releaseMigrationLock, batch marker recording around recordMigrationBatch and related marker helpers, and pure migration execution around runPendingMigrations/applyMigrationWithRetry into separate modules; retain database I/O boundaries and update imports/exports accordingly.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/shared/db/migrations.ts`:
- Around line 1-805: Split the oversized migrations module into focused modules,
keeping initDb orchestration and schema migration behavior unchanged. Extract
migration lock/lease operations around acquireMigrationLock and
releaseMigrationLock, batch marker recording around recordMigrationBatch and
related marker helpers, and pure migration execution around
runPendingMigrations/applyMigrationWithRetry into separate modules; retain
database I/O boundaries and update imports/exports accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff5f44ba-12cc-4bd6-929e-4ea328ccc132
📒 Files selected for processing (3)
src/shared/db/migrations.tssrc/shared/db/migrations/schema/version.tstest/lib/db/migration-runtime.test.ts
|
CodeRabbit’s outside-diff file-size comment is valid, but splitting the full migration subsystem is deliberately separate from this locking/budget safety change: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0f799db48
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/client.ts`:
- Around line 555-563: Update the batch callback to ensure every statement
passed to tx.batch is recorded through the existing query-tracking mechanism,
preserving the single database round trip. Reuse the same tracking behavior and
metadata used by execute, while retaining writtenSql, statementCount, and
enforceTransactionRoundTripGuard handling.
In `@test/shared/db/migration-round-trip-budget.test.ts`:
- Around line 35-40: Remove the uncovered defensive branch after the
MIGRATIONS.find call and use a documented non-null assertion for the migration
lookup, preserving the invariant that FREE_TEXT_MIGRATION_ID always exists.
- Around line 42-51: Update the stale-marker setup in the migration round-trip
test to read back both settings rows after the UPDATE and strongly assert that
each has value "stale" before calling initDb(). Keep the existing migration
deletion and initialization flow unchanged, using the test’s available database
query helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3173d48b-c392-4123-b9f8-c365ad9dccfd
📒 Files selected for processing (5)
src/shared/db/client.tssrc/shared/db/migrations/schema-sync.tstest/shared/db/client/batch.test.tstest/shared/db/link-table.test.tstest/shared/db/migration-round-trip-budget.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 943edb47c5
ℹ️ 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".
1 participant
Summary
Why
Sites with several pending migrations could exceed Bunny’s 50-subrequest request limit while starting. Splitting the work keeps each request within that limit while preserving completed migration progress. Batching table rebuild statements also prevents one heavy migration from exhausting the whole request before its progress can be saved. Lease-scoped completion keeps overlapping or stalled requests from changing each other’s migration state.
Risks
Testing
deno task precommitSummary by CodeRabbit
Bug Fixes
Tests
Documentation