ReviewOS

also looking at this

stacks/bun-query-builder

fix(migrate): create the ledger inside the lock, and stop the two bots racing

#1079
Merged glennmichael123 wants to merge fix/migrate-lock-race into main
4 files +81 -13
10 days ago

Two things, both surfaced by the same intermittent CI failure.

1. The "flaky test" was reporting a real hole in the lock

migration locking (#1067) failed twice in CI while passing 10/10 locally. That looked like flakiness. It wasn't.

createMigrationsTable() ran before withMigrationLock() — so the one operation guaranteed to happen on every concurrent boot was the one left unprotected. CREATE TABLE IF NOT EXISTS is not atomic in Postgres: two sessions running it at the same instant collide in the system catalogue, and one gets

duplicate key value violates unique constraint "pg_type_typname_nsp_index"

createMigrationsTable rethrows, so that process exits non-zero. The precise scenario the lock exists to prevent still had a way to fail — #1076 closed the ledger-read race but left the ledger-creation race open.

Moved inside the lock. Reproduced with five concurrent boots against a dropped ledger:

result
beforecodes: [0,0,0,1,0]
aftercodes: [0,0,0,0,0], recorded exactly once, repeatedly

2. The test was too weak to catch what it was reporting

Two failures, both mine:

  • Two workers made the collision window narrow enough to pass locally a dozen times running.
  • The reset only DELETEd this file's ledger row, leaving the table in place — so the workers never raced to create it, and the half that actually broke was never exercised at all.

Now five workers against a dropped ledger. Measured both ways:

with fix:     5/5 pass
without fix:  3/5 fail

resetDatabase() drops that table routinely elsewhere in the suite, so dropping it here is consistent with how the shared database is already treated.

3. Renovate disabled, dependabot added for Actions

CLAUDE.md already said buddy-bot owns dependency updates, but renovate was still live on the same manifests. The result is worse than duplicated effort: both rewrite bun.lock, so whichever lands first invalidates every open PR from the other. That's why all six renovate PRs were mergeable and all six buddy-bot PRs were conflicting at the same moment, and how the queue reached 12 open PRs covering 3 distinct upgrades.

It also cost a red main — renovate's #999 grouped kysely 0.28 → 0.29 as "non-major" (for a 0.x package a minor is breaking) and ignored kysely-bun-sqlite's declared peer ^0.28.2.

Disabled rather than deleted: renovate treats a config-less repo as un-onboarded and opens a fresh onboarding PR. Worth also deselecting the repo in the Renovate app settings.

That leaves a gap buddy-bot doesn't fill — it generates workflow files but doesn't bump uses: pins, and every actions bump this repo has had came from renovate. .github/dependabot.yml now covers github-actions only, deliberately not npm: a third bot on bun.lock would recreate the exact problem being fixed.

Verification

Typecheck 0, lint 0, suite 1719 pass / 4 skip / 0 fail.

Note the concurrency test remains probabilistic by nature — it reproduces a race 3 times in 5 without the fix. What matters for CI is the other direction: with the fix it passed 5/5 locally and the underlying race is gone, so it should stop producing false reds.

🤖 Generated with Claude Code

4 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.