fix(migrations): make deploy-time DDL lose the lock race, not user traffic - #5040
fix(migrations): make deploy-time DDL lose the lock race, not user traffic#5040RSO wants to merge 1 commit into
Conversation
…affic Migration 0204 deadlocked against live traffic on five consecutive production deploys, killing 13 user requests across /api/users/notifications and /api/webhooks/github before it landed. The cause was relation-level lock inversion. DROP TABLE takes ACCESS EXCLUSIVE on the target and on every table it references by foreign key, because dropping the constraint removes the referential-integrity trigger on the parent. 0204 dropped 12 tables holding 21 foreign keys into kilocode_users, organizations, and microdollar_usage, so it needed ACCESS EXCLUSIVE on two of the busiest tables in the schema at once. Ordinary queries touching both in the other order completed the cycle, and the deadlock detector aborts whichever party notices it -- in practice user requests rather than the migration. Production has lock_timeout = 0, so the migration sat in the lock queue indefinitely and stayed exposed to the detector. Measured locally, a conflicting statement waits 2.60s without lock_timeout versus failing at 0.63s with it, against a deadlock_timeout of 1000ms. Apply migrations with a runner that sets lock_timeout below deadlock_timeout, so the migration is the only casualty, and retries lock failures so a transient collision costs seconds instead of a deploy. A custom runner is required because drizzle-kit builds its own pg.Pool and zod-strips unknown dbCredentials keys, so lock_timeout cannot be injected through drizzle.config.ts. It also discarded every error field: the failed deploy printed only "applying migrations...undefined". The runner delegates to the same drizzle-orm migrator, so migration semantics are unchanged. It uses a single pg.Client rather than a Pool because the migrator runs the whole batch in one transaction on one connection, which keeps SET lock_timeout in force with no pool checkout to race against. Behaviour worth knowing: - It refuses to start when lock_timeout is 0 or >= deadlock_timeout, asserted against pg_settings rather than assumed. - Only 40P01, 55P03, and 40001 are retried. Real migration bugs still fail on the first attempt. - 11 existing migrations break out of the transaction with a bare COMMIT; for CREATE INDEX CONCURRENTLY. Replaying those would leave earlier statements applied but unrecorded, so they are detected and run with retries disabled. Only the production and staging deploy jobs are switched over. `pnpm drizzle migrate` is untouched, so local development, test:db, verify-bootstrap, chromatic, setup-smoke, and kiloclaw dev-start behave exactly as before. Documentation records the rules this incident exposed: the FK-parent locking rule and the per-constraint decomposition that avoids it, the single-transaction batching hazard, and the folderMillis skip hazard (the migrator compares each journal entry against max(created_at), so an entry older than the newest applied migration is skipped permanently and silently).
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| await client.connect(); | ||
|
|
||
| try { | ||
| await client.query(`SET lock_timeout = '${lockTimeout}'`); |
There was a problem hiding this comment.
WARNING: MIGRATION_LOCK_TIMEOUT is interpolated directly into SQL
lockTimeout comes straight from the environment and is string-interpolated into the SET lock_timeout = '...' statement. In the deploy workflow the value is operator-controlled, so this is not remotely exploitable — but this runner executes against production with broad privileges, and any value containing a quote breaks or injects SQL. SET cannot take bound parameters, but set_config can, which removes the interpolation entirely and leaves format validation to PostgreSQL:
| await client.query(`SET lock_timeout = '${lockTimeout}'`); | |
| await client.query('SELECT set_config($1, $2, false)', ['lock_timeout', lockTimeout]); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ); | ||
| const row = ledgerRowSchema.safeParse(result.rows[0]); | ||
| appliedThrough = row.success ? Number(row.data.max_created_at ?? 0) : 0; | ||
| } catch { |
There was a problem hiding this comment.
SUGGESTION: Broad catch {} treats any ledger query failure as "fresh database"
If select max(created_at) ... from drizzle.__drizzle_migrations fails for any reason other than a missing ledger (transient connection error, permission problem, lock timeout), this catch silently sets appliedThrough = 0. The log then claims every migration since the beginning is pending, and because the old COMMIT-breaking migrations now appear in pending, retries are silently disabled for the whole run. The failure direction is safe (no incorrect application), but the loss of the retry safety net is invisible. Consider narrowing the catch to the undefined-relation error code (42P01) or at least logging the swallowed error.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe lock-safe migration runner is well-designed and correctly detects all 11 transaction-breaking migrations, but the deploy-time DDL tool interpolates an environment variable directly into a SQL statement in Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Reviewed by kimi-k3 · Input: 156.6K · Output: 26.5K · Cached: 614.4K Review guidance: REVIEW.md from base branch |
1 participant
Why
Migration
0204_drop_cost_insight_tablesdeadlocked against live traffic on five consecutive production deploys (12:37, 13:39, 13:53, 13:58 UTC on Aug 5, plus manual retries), killing 13 user requests — KILOCODE-WEB-27KW. Each merge to main re-attempted the still-pending migration and re-fired, sopromote-appwas blocked for ~1h40m. A failed migration in this setup is not inert.The cause is relation-level lock inversion, not a bug in the migration.
DROP TABLEtakesACCESS EXCLUSIVEon the target and on every table it references by foreign key, because dropping the constraint removes the referential-integrity trigger that lives on the parent. 0204 dropped 12 tables holding 21 foreign keys intokilocode_users(12),organizations(8), andmicrodollar_usage(1), so a single statement neededACCESS EXCLUSIVEon two of the busiest tables in the schema:getUserOrganizationsWithSeatsreadsorganizations→organization_memberships→kilocode_users→organization_invitations, takingAccessShareone relation at a time in query order, which closes the cycle. PostgreSQL's deadlock detector aborts whichever party notices the cycle — in practice user requests, because production haslock_timeout = 0and the migration just sits in the queue.The PR description for #5024 reasoned about incoming FKs and readers of the dropped tables and concluded it was "safe to run at any point relative to the deploy". That is true for correctness but says nothing about locking, and the outgoing FKs are what bit.
Measured locally against
deadlock_timeout = 1000ms, holdingACCESS EXCLUSIVEonkilocode_usersfrom another session:lock_timeout=500msERROR: canceling statement due to lock timeoutat 0.63sdeadlock_timeoutWhat this does
Applies migrations with a runner that sets
lock_timeoutbelowdeadlock_timeout, so the migration surrenders the lock queue before the detector can pick a victim, and retries lock failures so a transient collision costs seconds instead of a deploy.A custom runner is required:
drizzle-kit migratebuilds its ownpg.Pooland zod-strips unknowndbCredentialskeys, solock_timeoutcannot be injected viadrizzle.config.ts. It also discarded every error field — the failed deploy printed onlyapplying migrations...undefined, which is why Sentry was the only way to learn this was a deadlock.The runner delegates to the same
drizzle-ormmigrator, so migration semantics are unchanged. It uses a singlepg.Clientrather than a Pool because the migrator runs the whole batch in one transaction on one connection, keepingSET lock_timeoutin force with no pool checkout to race against. (pool.on('connect')was the first approach and tripped a pg deprecation that becomes an error in pg@9.)Behaviour worth reviewing:
lock_timeoutis 0 or ≥deadlock_timeout, asserted againstpg_settingsrather than assumed.40P01,55P03,40001(10 attempts, linear backoff to 15s). Real migration bugs still fail on attempt one.COMMIT;forCREATE INDEX CONCURRENTLY. Replaying those would leave earlier statements applied but unrecorded, so they are detected and run with retries disabled, with a log line saying so.MIGRATION_LOCK_TIMEOUTandMIGRATION_MAX_ATTEMPTS.Documentation records the rules this incident exposed: the FK-parent locking rule and the per-constraint decomposition that avoids it, the single-transaction batching hazard, and the
folderMillisskip hazard.Blast radius
Only
deploy-production.yml:78anddeploy-staging.yml:74change.pnpm drizzle migrateis untouched, so local development,test:db,verify-bootstrap, chromatic, setup-smoke, and kiloclawdev-startbehave exactly as before.The trade to weigh: deploys can now fail with
55P03where they previously succeeded after a long lock wait. That is intended — the deploy absorbs the failure instead of user requests — but a busy database can cost a deploy retry.Verification
pnpm --filter @kilocode/db typecheckandlint— clean.oxfmtapplied,git diff --checkclean.packages/db/src/migrate.test.ts— pass.900msproceeds.pnpm drizzle:verify-bootstrapstill passes (untouched path).Not full
pnpm validate— the repo-wide suite is unrelated to this diff. The checks above are targeted.Known gap, pre-existing and not fixed here
packages/dbtests do not run anywhere.apps/web/jest.config.ts:38listspackages/db/src/**/*.test.tsintestMatch, butrootsdefaults toapps/web, so jest never crawls that directory —jest --listTestsreturns zero matches there. Five test files are dead, and the new test file is dead with them; it was run via an explicit--rootsoverride.I deliberately did not fix this, because enabling
rootssurfaces two pre-existing failures that need real decisions:packages/db/src/client.test.ts:39expectspg.Poolcalled with{ connectionString, max: 1 }, butcreateDrizzleClientnow spreadsgetDatabaseClientConfig(host/user/password/ssl).packages/db/src/schema.test.ts:1418readsmigrations/0204_brainy_baron_strucker.sql, which does not exist — 0204 is0204_drop_cost_insight_tables.sql. A 0204 numbering collision, the same slot as the incident migration.Happy to do that as a focused follow-up. Until then, treat the new coverage as real but not CI-enforced.
Deliberately not included
App-layer retry of
40P01/40001in the shared web db wrapper. It would have made all 13 events invisible and helps beyond migrations, but it changes behaviour for every query inapps/weband belongs in its own PR.