Repository navigation
fix: treat an unrecognized tenant migration as ahead - #1483
Conversation
When a tenant's recorded migrations_version is a name this binary does not know (a newer release already migrated it, e.g. mid rolling deploy or after a rollback), its ordinal lookup was undefined and comparisons concluded the tenant was behind. An older pod would then re-run migrations and overwrite the newer version with its own. - getTenantConfig presents an unknown version as the highest migration this binary knows and flags it as ahead, so capability checks and request latestMigration see a valid name - areMigrationsUpToDate returns true for ahead tenants - migration state writes are guarded in SQL so they never touch a row whose version is unknown; explicit resets stay unguarded - the fleet migration listing skips rows with an unknown version
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The core read-side normalization lacks direct regression coverage.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Hardens multitenant migration bookkeeping so older releases treat unknown tenant migration versions as ahead.
Changes:
- Normalizes unknown versions for migration checks and feature detection.
- Guards state updates and fleet selection while preserving explicit resets.
- Adds guarded-write, listing, and migration-status tests.
| File | Description |
|---|---|
src/internal/database/tenant.ts |
Normalizes unknown versions and logs warnings. |
src/internal/database/tenant-store-pg.ts |
Guards writes and excludes ahead tenants from migration batches. |
src/internal/database/tenant-store-pg.test.ts |
Tests guarded SQL and listing filters. |
src/internal/database/migrations/migrate.ts |
Handles ahead tenants and preserves reset bypass. |
src/internal/database/migrations/migrate.test.ts |
Tests ahead checks and guarded completion writes. |
src/internal/database/migrations/guards.ts |
Adds migration classification and highest-known lookup helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… a write A pod with a cached pre-migration config cannot tell a newer release already moved the tenant ahead. When its guarded state write matches no row, drop the local config so the next read sees the ahead version instead of waiting for the cache TTL, and log that the guard fired.
…t know Old and new releases share the migrations queue during a rollout. A worker on the older release picking up a job whose upToMigration it does not have used to throw, mark the tenant FAILED and eventually FAILED_STALE. Skip the job instead and leave the tenant for the newer release.
Coverage Report for CI Build 37604413389Coverage increased (+0.03%) to 84.939%Details
Uncovered ChangesNo uncovered changes found. 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's a deploy-safety fix touching the shared multitenant migration read/write path (version comparison, SQL guard conditions, and the fleet-listing filter), a human look would still be worthwhile.
What was reviewed: the new guard helpers in guards.ts (unrecognized-version detection and highest-known-migration lookup), the migrationVersionAhead short-circuit in areMigrationsUpToDate and its propagation through getTenantConfig, the new guarded updateMigrationsState SQL (null/empty/known-version WHERE clause) versus the intentionally unguarded resetMigration rewind path, and the added known-version filter in listTenantsToMigrateBatch. Checked that downstream consumers of migrationVersion (getTenantCapabilities, tenantHasMigrations, progressive.ts's scheduling logic) see a valid, known name and behave consistently for ahead tenants.
Extended reasoning...
The change spans guards.ts, migrate.ts, tenant.ts, and tenant-store-pg.ts to fix a cross-version migration race in multitenant Postgres bookkeeping: it treats an unrecognized migrations_version as "ahead," short-circuits migration runs for such tenants, and adds a SQL guard so older binaries can't clobber a newer release's recorded version. It touches direct SQL (parameterized, no injection concern) governing production tenant migration state, a security/correctness-sensitive surface for data integrity across rolling deploys. The logic traced through consistently (read point sets migrationVersionAhead and substitutes the highest known version; write point and fleet listing share the same known-version guard; reset path intentionally bypasses the guard for explicit rewinds), and three candidate edge cases around unrecognized-version semantics were already investigated and ruled out by the bug hunt, but the production blast radius of a migration-state bug (stuck tenants, silently skipped migrations) warrants a human pass before merge.
This review covers commit 240b17c, which is no longer the latest commit on this pull request; later commits are not covered by it.
… dropping them Fail the job without touching tenant state so pg-boss retries it, giving a worker on the newer release the chance to pick it up. Jobs that exhaust their retries go to the dead-letter queue rather than completing as a no-op.
…d end to end Read raw stored versions through getTenantConfig: an unknown name becomes the highest known version with the ahead flag and a warning, while known, null and empty values pass through. Against the real control database, an older release's completion and failure writes leave an ahead row untouched and evict the cached config, while a known row still accepts both writes.
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
An older worker picking up a reset from a newer release resolved the unknown untilMigration to undefined, deleted nothing, and still recorded the target as COMPLETED, so the requested rewind was silently lost. An unknown markCompletedTillMigration was ignored instead, re-running migrations the operator meant to skip. Fail the job before touching any state so pg-boss retries it on a worker that knows the migration.
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_versionis a name this binary doesn't know,DBMigration[name]isundefinedand 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
upToMigrationit 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.
getTenantConfigreports it as the newest migration this binary knows and setsmigrationVersionAhead.areMigrationsUpToDatereturnstruefor these tenants.resetMigrationstill writes unguarded.upToMigration, and reset jobs with an unknownuntilMigrationormarkCompletedTillMigration, 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.