Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical migration-context handling and repeated no-op job issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR hardens multitenant migration handling during mixed-version deployments by clamping unknown tenant migrations and logging warnings.
Changes:
- Detects unrecognized migration names.
- Adds coverage for unknown, behind, and current migrations.
- Documents migration-resolution fallback behavior.
Review findings:
- Critical (2 votes): The clamp must also apply the local-latest fallback to
request.latestMigration. - Moderate (1 vote): Full-fleet listing should suppress repeated no-op jobs for unknown migration versions.
File summaries
| File | Summary |
|---|---|
src/internal/database/migrations/migrate.ts |
Adds unknown-migration handling and warning. |
src/internal/database/migrations/migrate.test.ts |
Tests migration status scenarios. |
src/http/plugins/db.ts |
Documents migration fallback behavior. |
Review details
Suppressed comments (2)
src/internal/database/migrations/migrate.ts:194
DBMigrationis a normal object, so this lookup also finds inherited properties such asconstructorandtoString. If an invalid/unknown recorded migration has one of those names, this branch is skipped and the final ordinal comparison returns false, causing the pod to attempt migrations instead of clamping. Check for an own key explicitly so every unrecognized value follows the safety path.
if (tenant.migrationVersion && DBMigration[tenant.migrationVersion] === undefined) {
src/internal/database/migrations/migrate.ts:210
- The clamp only prevents work after a migration job has been queued.
listTenantsToMigratestill selects every completed tenant whosemigrations_versiondiffers from the local latest, so a tenant recorded with a future/unknown name remains eligible and each full-fleet run enqueues a job that immediately returns here. Exclude unrecognized versions from the listing (or otherwise suppress these no-op jobs) to avoid repeated queue and worker load during mixed-version rollouts.
return true
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Coverage Report for CI Build 37543182058Coverage increased (+0.3%) to 84.853%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches multitenant migration-version bookkeeping used during rolling deploys — a path where getting the fallback wrong could silently skip or misapply a tenant migration — a human look would still be worthwhile.
What was reviewed: the new unrecognized-migration clamp in areMigrationsUpToDate (src/internal/database/migrations/migrate.ts), the accompanying comment on resolveLatestMigration (src/http/plugins/db.ts), and the three new unit tests covering the clamp, a genuinely-behind tenant, and an already-caught-up tenant. Also traced how the clamp interacts with the ON_REQUEST migration hook's tenant-config cache writes in db.ts — the unrecognized case still gets cached locally as localLatest, but only in this pod's in-memory cache with no DB write, so it does not corrupt the persisted tenant migration state.
Extended reasoning...
Overview
The PR adds a defensive clamp to areMigrationsUpToDate in src/internal/database/migrations/migrate.ts: when a tenant's recorded migrationVersion is not a key in this binary's DBMigration map, it now logs a warning and returns true (treat as up to date) instead of falling through to DBMigration[latestMigrationVersion] <= DBMigration[undefined], which evaluates false and would previously misclassify the tenant as behind. src/http/plugins/db.ts gets a comment-only clarification on the pre-existing resolveLatestMigration fallback (no functional change). migrate.test.ts adds three focused unit tests.
Security risks
None directly — no auth, crypto, or user input handling involved. The risk class here is data-integrity/availability: an incorrect fallback direction could cause an older binary to either skip migrations it should run, or (pre-fix) attempt migrations it can't reason about and clobber a tenant's recorded migration state with a stale value during a rolling deploy.
Level of scrutiny
I verified the logic manually: the new guard only fires when tenant.migrationVersion is truthy and absent from DBMigration, correctly short-circuiting before the ordinal comparison, and the tests exercise exactly the three relevant branches (unrecognized/clamp+warn, known-but-behind/false+no-warn, known-and-current/true+no-warn) with mocks consistent with the rest of the file. I also checked the interaction with the ON_REQUEST strategy in db.ts: when the clamp fires, resolveLatestMigration on the still-unrecognized version falls back to localLatest, and that value gets written into the in-process tenant-config cache (not the DB, since updateTenantMigrationsState is skipped when migrationsUpToDate is true) — self-consistent for this pod and not a source of persisted corruption.
Other factors
This is a small, single-purpose, well-tested change, but it sits in production-critical multitenant migration bookkeeping that the author explicitly frames as deploy-safety hardening meant to precede a follow-up migration-numbering PR — the kind of change where a second pair of eyes on the rollout reasoning is worth the low marginal cost, even though the code itself is correct as far as I can tell.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
4905114 to
50ce5e2
Compare
2b63ff8 to
067e890
Compare
|
@claude review always |
067e890 to
9054c87
Compare
areMigrationsUpToDate compares the tenant's recorded migration name against this binary's own DBMigration ordinals. When two branches developed in parallel each add a migration and one merges first, the other has to renumber (this already happened: 0073 was independently claimed by revoke-grants-to-unused-operations on master and unlock-object-versioning on another branch, which had to move to 0074). During a rolling deploy, an older binary can see a tenant already migrated to a name it doesn't recognize at all. DBMigration[name] is then undefined, the ordinal comparison silently reads as "behind", and the older binary attempts a migration run it can't reason about and overwrites the tenant's recorded migration with its own, older one. Detect the unrecognized-name case explicitly and clamp to "nothing left for me to run" instead of comparing against undefined, logging a warning so the condition is observable. resolveLatestMigration already falls back to its own local latest migration in this situation; documented that as intentional.
Three gaps in the previous commit, all found in review: tenantHasMigrations re-derived the tenant's raw migration name and compared it against DBMigration directly, with no clamp. request.latestMigration is only resolved through resolveLatestMigration under the ON_REQUEST strategy; PROGRESSIVE (and any request that doesn't go through that resolution) hands an unrecognized name straight to StoragePgDB.hasMigration, which falls back to tenantHasMigrations. DBMigration[unrecognized] is undefined there too, so every hasMigration check - not just the new migration - read as false, reintroducing the dropped-index failure this clamp exists to prevent, for whichever strategy isn't ON_REQUEST. tenantHasMigrations now clamps to this binary's own local latest the same way, so the fallback path is protected regardless of strategy. DBMigration is a plain object, so `DBMigration[x] === undefined` also finds inherited properties like `constructor` and `toString`. Switched both checks to Object.hasOwn so a value colliding with one of those can't skip the clamp and fall through to the original broken comparison. listTenantsToMigrateBatch selected every tenant whose recorded migration differs from this binary's local latest, which includes tenants already moved to a name it doesn't recognize (ahead, not behind). A full-fleet run kept re-enqueuing a no-op migration job for such a tenant every pass until the fleet caught up. It now also requires the recorded version to be one this binary knows about, so an unrecognized (ahead) tenant is left alone.
The clamp returned true unconditionally, unlike the known-migration branch right below it which requires migrationStatus === COMPLETED. The only writer of migrations_status leaves migrations_version untouched on a FAILED/FAILED_STALE write, so an unrecognized version can only be paired with a failed status as a leftover from an earlier, separate successful write - but when that pairing occurs, the failure is real and the clamp should not paper over it. Mirror the same status check.
…e in the unrecognized clamp Two gaps found in review: request.latestMigration reaches the database adapter directly (StoragePgDB's constructor derives supportsCustomMetadataColumns, supportsMultipartMetadataColumn and bucketColumnPolicy from it inline, without going through hasMigration at all). Only the ON_REQUEST strategy re-resolved this value through resolveLatestMigration; PROGRESSIVE and FULL_FLEET left the raw tenant-reported name in place. An unrecognized name reaching the adapter unresolved made every one of those capability checks read as unsupported, not just the new migration - the same failure class this clamp exists to prevent, reached through a path that never calls hasMigration. The shared preHandler hook that first populates request.latestMigration now resolves it the same way for every strategy. lastLocalMigrationName respects dbMigrationFreezeAt, which governs which migrations this binary will run - it says nothing about what its own code can already interpret. Using it as the clamp target for an unrecognized tenant migration could under-report a tenant a newer, unfrozen binary already advanced past the freeze point (deploying new code after a freeze, before lifting it). Added highestLocalMigrationName, which ignores the freeze, and use it for both this clamp and the new preHandler resolution above.
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
9054c87 to
690f159
Compare
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
There was a problem hiding this comment.
Beyond the inline finding, I also checked the failed-ledger-read path in observeTenantMigration/areMigrationsUpToDate (observed.ts:148-153) for an ahead-but-not-COMPLETED tenant: a failed read still makes areMigrationsUpToDate report "not up to date," but that matches the pre-existing conservative behavior for non-COMPLETED status — only the COMPLETED-ahead case was the regression this PR targets — so it doesn't revive the bug.
Extended reasoning...
The PR extracts tenant-migration connection/observation logic into new modules and fixes a real bug where an "ahead" tenant with an unrecognized migration name was treated as behind. A CONFIRMED finding about progressive.ts opening up to 200 concurrent DB connections per reconciliation cycle (a new DB I/O cost areMigrationsUpToDate never had before) is being posted inline, which is reason enough for a human to look. I independently traced the observed.ts failed-read fallback for a non-COMPLETED ahead tenant and found it preserves prior conservative semantics rather than reintroducing the original bug.
This pull request has been reviewed before and this review found new issues. Where they share a root cause, one fix may close them together.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
src/internal/database/migrations/progressive.ts— Operators get a connection storm: progressive-migration batch reconciliation can now open up to 200 concurrent live connections to tenant databases per cycle, something the base branch never did (areMigrationsUpToDate was a pure cached-metadata check with no DB I/O). createJobsBatch (progressive.ts:99-106) calls the new areMigrationsUpToDate for every tenant in tenantsBatch via Promise.allSettled with no concurrency limit. For tenants queued cold, needsObservation is true and each call opens a fresh pg.Client via observeTenantMigration/connectMigrationDatabase. Fix: cap concurrent live ledger reads per batch (or keep batch reconciliation metadata-only).Why this was flagged
Trigger: many tenants land in ProgressiveMigrations.tenants without a cached observation, e.g. via progressiveMigrations.addTenant at src/http/routes/admin/tenants.ts:206 right after runTenantMigrations's finally calls onTenantConfigChange(tenantId) (tenants.ts:203), which deletes the tenant config cache entry (tenant.ts:486-488). createJobsBatch (progressive.ts:99-106) then calls areMigrationsUpToDate(tenant) for up to maxSize=200 tenants (migrate.ts:64-67) concurrently via Promise.allSettled. areMigrationsUpToDate (observed.ts:135-161) calls needsObservation/observeTenantMigration (observed.ts:25-33, 72-114), which opens a brand-new pg.Client via connectMigrationDatabase (connection.ts:9-33) per tenant, no pooling or throttle. On base, areMigrationsUpToDate only compared tenant.migrationVersion/migrationStatus from cache and never touched the tenant database, so this reconciliation path was free of DB connections.
Verification: Base areMigrationsUpToDate (migrate.ts@ 3c50b7c:190-200) did only lastLocalMigrationName + cached getTenantConfig — zero tenant-DB I/O. The new areMigrationsUpToDate (observed.ts:135-161) calls observeTenantMigration when needsObservation is true, opening a fresh unpooled pg.Client (connection.ts:36-44).
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
## What kind of change does this PR introduce? Bug fix. Replaces #1403 with a smaller change. ## What is the current behavior? When a tenant's `migrations_version` is a name this binary doesn't know, `DBMigration[name]` is `undefined` and the tenant looks behind. This happens when a newer release has already migrated the tenant, for example during a rolling deploy or after a rollback. The old pod re-runs migrations, writes its own older version over the newer one, and turns off features for that tenant. In PROGRESSIVE mode both releases share the migrations queue. An old worker that picks up a job whose `upToMigration` it doesn't have throws, then marks the tenant FAILED and later FAILED_STALE. ## What is the new behavior? A version the binary doesn't know is treated as ahead. - `getTenantConfig` reports it as the newest migration this binary knows and sets `migrationVersionAhead`. `areMigrationsUpToDate` returns `true` for these tenants. - Migration state writes only apply when the stored version is null, empty or known, so an old pod can't rewind a newer one. If the write matches no row, the pod drops its cached config and logs it. `resetMigration` still writes unguarded. - The fleet listing skips tenants with an unknown version. - Migration jobs with an unknown `upToMigration`, and reset jobs with an unknown `untilMigration` or `markCompletedTillMigration`, fail without touching tenant state. pg-boss retries them so a worker on the newer release can pick them up. Before, an old worker would record a reset it never applied. Nothing new runs on the request path, and tenants with a null version behave as before. This should ship before any PR that adds a migration. ## Additional context Not covered: restored databases whose control row is ahead of the schema, a compare-and-swap on `database_url`, and two branches using the same migration number. During a rollout an old pod can still flip a FAILED row at a version it knows back to COMPLETED, as it already does on master. Unit tests cover the write guard, the listing filter, the up-to-date check, the cache eviction and the skipped job. I also ran the guard SQL by hand against Postgres 15.
What kind of change does this PR introduce?
Bug fix / deploy-safety hardening for the multitenant migration bookkeeping.
What is the current behavior?
areMigrationsUpToDatecompares a tenant's recorded migration name against this binary's ownDBMigrationordinal map (DBMigration[tenant.migrationVersion]). If the tenant's recorded migration name isn't a key this binary knows about at all, that lookup isundefined, and the ordinal comparison silently evaluates as "the tenant is behind."This can happen whenever two branches are developed in parallel and each adds a new migration: whichever merges first claims the next number, and during a rolling deploy an older pod can see a tenant a newer pod already migrated past, using a migration name the older pod has never heard of. It isn't hypothetical — it already happened once during development:
0073was independently claimed byrevoke-grants-to-unused-operationsonmasterand by another migration on a parallel branch, which had to renumber to0074to avoid colliding.When an older pod concludes (incorrectly) that a tenant is "behind", it attempts a migration run it can't reason about, and can overwrite the tenant's recorded migration state with its own, older value.
What is the new behavior?
areMigrationsUpToDateexplicitly detects when the tenant's recorded migration name isn't recognized by this binary and clamps to "nothing left for me to run" instead of comparing againstundefined, logging a warning so the condition is observable.resolveLatestMigration's existing fallback to its own local latest migration in this situation is documented as intentional.This is meant to ship ahead of any PR introducing a new migration ordinal, so the "old" side of that rollout's mixed-version window already tolerates a migration name it doesn't recognize.
Additional context
Three new unit tests cover: the unrecognized-name clamp + warning, a tenant genuinely behind a known migration, and a tenant already caught up.