also looking at this
fix(migrate): create the ledger inside the lock, and stop the two bots racing
#1079
4 files
+81
-13
| @@ -0,0 +1,25 @@ | ||
| 1 | # GitHub Actions version bumps only. | |
| 2 | # | |
| 3 | # buddy-bot owns npm dependencies (see CLAUDE.md) and renovate is disabled in | |
| 4 | # .github/renovate.json, but neither covers this: buddy-bot GENERATES workflow | |
| 5 | # files, it does not bump the `uses:` pins inside them. Every actions bump this | |
| 6 | # repo has received came from renovate, so switching renovate off without this | |
| 7 | # would leave `actions/checkout`, `actions/cache` and `oven-sh/setup-bun` to | |
| 8 | # drift until something breaks. | |
| 9 | # | |
| 10 | # Scoped deliberately to github-actions. Adding an npm ecosystem here would | |
| 11 | # recreate the exact problem being fixed — two bots rewriting bun.lock, each | |
| 12 | # invalidating the other's open PRs. | |
| 13 | version: 2 | |
| 14 | updates: | |
| 15 | - package-ecosystem: "github-actions" | |
| 16 | directory: "/" | |
| 17 | schedule: | |
| 18 | interval: "weekly" | |
| 19 | commit-message: | |
| 20 | prefix: "chore(deps)" | |
| 21 | labels: | |
| 22 | - "dependencies" | |
| 23 | # One PR per action. These diffs are a single line and are read at a glance; | |
| 24 | # grouping them only makes a bad bump harder to bisect. | |
| 25 | open-pull-requests-limit: 5 | |
| @@ -1,5 +1,26 @@ | ||
| 1 | 1 | { |
| 2 | "extends": [ | |
| 3 | "github>ow3org/renovate-config" | |
| 2 | "$schema": "https://docs.renovatebot.com/renovate-schema.json", | |
| 3 | "enabled": false, | |
| 4 | "_comment": [ | |
| 5 | "Renovate is off: buddy-bot owns dependency updates here (see CLAUDE.md).", | |
| 6 | "", | |
| 7 | "Both bots were live on the same manifests, which does not merely duplicate", | |
| 8 | "work — it deadlocks. They both rewrite bun.lock, so whichever lands first", | |
| 9 | "invalidates every open PR from the other. That is why all six renovate PRs", | |
| 10 | "were mergeable and all six buddy-bot PRs were conflicting at the same", | |
| 11 | "moment, and why the queue reached 12 open PRs for 3 distinct upgrades.", | |
| 12 | "", | |
| 13 | "It also cost a broken main: renovate's #999 grouped kysely 0.28 -> 0.29 as", | |
| 14 | "'non-major', but for a 0.x package a minor IS breaking, and it ignored", | |
| 15 | "kysely-bun-sqlite's declared peer of ^0.28.2. That reddened typecheck on", | |
| 16 | "main and blocked a release.", | |
| 17 | "", | |
| 18 | "This file is kept rather than deleted on purpose: renovate treats a repo", | |
| 19 | "with no config as un-onboarded and opens a fresh 'Configure Renovate' PR.", | |
| 20 | "Disabling in-repo is the only way to stay quiet. For belt and braces,", | |
| 21 | "deselect this repository in the Renovate GitHub App installation settings.", | |
| 22 | "", | |
| 23 | "GitHub Actions bumps are handled by .github/dependabot.yml, because", | |
| 24 | "buddy-bot generates workflow files but does not bump `uses:` pins." | |
| 4 | 25 | ] |
| 5 | 26 | } |
| @@ -496,13 +496,22 @@ export async function executeMigration(dir?: string): Promise<boolean> { | ||
| 496 | 496 | try { |
| 497 | 497 | const qb = createQueryBuilder() |
| 498 | 498 | |
| 499 | // Create migrations table if it doesn't exist | |
| 500 | await createMigrationsTable(qb, dialect) | |
| 501 | ||
| 502 | // Everything from here runs under the migration lock, and the read of the | |
| 503 | // ledger is deliberately INSIDE it: computing `pending` before locking is | |
| 504 | // what lets two processes agree on the same work and then both do it. | |
| 499 | // Everything from here runs under the migration lock, INCLUDING creating | |
| 500 | // the ledger table and reading it. | |
| 501 | // | |
| 502 | // Reading it inside is what stops two processes agreeing on the same | |
| 503 | // pending set and both applying it. Creating it inside is the subtler | |
| 504 | // half: `CREATE TABLE IF NOT EXISTS` is not atomic in Postgres, and two | |
| 505 | // sessions running it at the same instant race in the system catalogue — | |
| 506 | // one of them gets `duplicate key value violates unique constraint | |
| 507 | // "pg_type_typname_nsp_index"` and, since this rethrows, exits non-zero. | |
| 508 | // Leaving it outside the lock meant the very scenario the lock exists for, | |
| 509 | // two concurrent boots, still had a way to fail. Caught by the locking | |
| 510 | // regression test failing intermittently in CI while passing locally. | |
| 505 | 511 | return await withMigrationLock(dialect, async () => { |
| 512 | // Create migrations table if it doesn't exist | |
| 513 | await createMigrationsTable(qb, dialect) | |
| 514 | ||
| 506 | 515 | // Get already executed migrations |
| 507 | 516 | const executedMigrations = await getExecutedMigrations(qb, dialect) |
| 508 | 517 | |
| @@ -117,12 +117,25 @@ import { SQL } from 'bun' | ||
| 117 | 117 | // the suite, so dropping it wholesale would delete other files' bookkeeping. |
| 118 | 118 | const reset = new SQL(${JSON.stringify(PG_URL)}) |
| 119 | 119 | await reset.unsafe('DROP TABLE IF EXISTS mig_concurrent') |
| 120 | await reset.unsafe("DELETE FROM migrations WHERE migration = '0000000001-concurrent.sql'").catch(() => {}) | |
| 120 | // DROP the ledger, not just this file's row. Half the race being guarded here | |
| 121 | // is in creating that table: \`CREATE TABLE IF NOT EXISTS\` is not atomic in | |
| 122 | // Postgres, so concurrent boots collide in the system catalogue. Leaving the | |
| 123 | // table in place means the workers never race to create it and the test cannot | |
| 124 | // see that half at all — which is exactly why an earlier version passed | |
| 125 | // locally while failing in CI. \`resetDatabase()\` drops this table routinely | |
| 126 | // elsewhere in the suite, so this is consistent with how the shared database | |
| 127 | // is already treated. | |
| 128 | await reset.unsafe('DROP TABLE IF EXISTS migrations').catch(() => {}) | |
| 121 | 129 | await reset.end() |
| 122 | 130 | |
| 123 | const a = Bun.spawn({ cmd: ['bun', 'worker.ts'], cwd: process.cwd(), stdout: 'pipe', stderr: 'pipe' }) | |
| 124 | const b = Bun.spawn({ cmd: ['bun', 'worker.ts'], cwd: process.cwd(), stdout: 'pipe', stderr: 'pipe' }) | |
| 125 | const codes = await Promise.all([a.exited, b.exited]) | |
| 131 | // Five, not two. The race this guards lives in the moments before the lock is | |
| 132 | // held, so the odds of two processes colliding there are low enough that a | |
| 133 | // two-worker version passed locally for a dozen runs while failing in CI. Five | |
| 134 | // makes it reliable: with the ledger-creation race present, this reproduces in | |
| 135 | // roughly one run out of three rather than one in twenty. | |
| 136 | const workers = Array.from({ length: 5 }, () => | |
| 137 | Bun.spawn({ cmd: ['bun', 'worker.ts'], cwd: process.cwd(), stdout: 'pipe', stderr: 'pipe' })) | |
| 138 | const codes = await Promise.all(workers.map(w => w.exited)) | |
| 126 | 139 | |
| 127 | 140 | // Count only THIS migration. The suite shares one Postgres database, so other |
| 128 | 141 | // files leave rows in the ledger and a total count is not ours to assert on. |
| @@ -142,7 +155,7 @@ console.log(JSON.stringify({ codes, recorded: count })) | ||
| 142 | 155 | |
| 143 | 156 | const { codes, recorded } = lastJson(result.out) |
| 144 | 157 | // Both succeed: the loser waits for the lock, then finds nothing pending. |
| 145 | expect(codes).toEqual([0, 0]) | |
| 158 | expect(codes).toEqual([0, 0, 0, 0, 0]) | |
| 146 | 159 | // And it is recorded once, not twice. |
| 147 | 160 | expect(recorded).toBe(1) |
| 148 | 161 | }) |