From fc48e2586d8ab02883ab14e7988f965588e55c0f Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Wed, 22 Apr 2026 00:06:06 -0400 Subject: [PATCH] ci: prevent migration-number collisions slipping past the merge queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses #2815. After the #2813 incident (two PRs reserved migration 419, both passed CI, both merged, broke every fresh boot), close the two gaps that let it happen. 1. check-migration-numbers.yml PR check was inspecting GitHub's speculative merge commit, which goes stale when main moves on in a way that doesn't conflict with the PR's diff. Two PRs reserving the same number each saw a clean local state and passed. Now the PR check takes the UNION of migration filenames on the PR branch and on origin/main, then hunts duplicates in that union. Post- hoc collision with a migration that landed on main after the PR opened is caught on the next PR run. Also drops the paths: 'migrations/**' filter so a rebase triggers a re-check even when the PR itself doesn't touch migrations. 2. deploy.yml has no guard — a broken main push would ship a container that crashloops on migration load. Added a preflight job that runs the duplicate check on the merged main tree; deploy now needs: preflight, so if a collision ever lands it's caught before Fly spins up the image, not after. Simulated locally: echo 'union check with collision → exit 1' → works echo 'union check clean → exit 0' → works Closes #2815. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/ci-migration-collision-guard.md | 12 +++++ .github/workflows/check-migration-numbers.yml | 50 +++++++++++++++---- .github/workflows/deploy.yml | 29 +++++++++++ 3 files changed, 80 insertions(+), 11 deletions(-) create mode 100644 .changeset/ci-migration-collision-guard.md diff --git a/.changeset/ci-migration-collision-guard.md b/.changeset/ci-migration-collision-guard.md new file mode 100644 index 0000000000..127e355849 --- /dev/null +++ b/.changeset/ci-migration-collision-guard.md @@ -0,0 +1,12 @@ +--- +--- + +ci: prevent migration-number collisions from slipping past the merge queue (closes #2815) + +Addresses the class of incident from #2813: #2793 and #2800 both reserved migration 419, passed CI individually, and merged minutes apart — leaving every fresh DB boot broken until a renumber hotfix. Two gaps closed: + +**`check-migration-numbers.yml`** — the PR check now takes the union of migration filenames from the PR branch *and* `origin/main`, then hunts duplicates in that union. Previously it only inspected GitHub's speculative merge commit, which goes stale when main moves on in a way that doesn't conflict with the PR's diff. Two PRs reserving the same number would both see a clean local state and pass. With the union check, the second one to run (after the first lands on main) fails immediately. Also dropped the `paths: server/src/db/migrations/**` filter so a rebase after main adds a migration triggers a re-check even if the PR itself doesn't touch migrations. + +**`deploy.yml`** — added a `preflight` job that runs the duplicate-number check on the merged main tree. `deploy` now `needs: preflight`, so if somehow a collision does land, the Fly deploy is blocked before we ship a container that crashloops on boot instead of after. Cheap sanity check, ~10s runtime. + +Together: pre-merge catches the race, post-merge catches anything pre-merge misses. Both are deterministic shell, no hidden dependencies. diff --git a/.github/workflows/check-migration-numbers.yml b/.github/workflows/check-migration-numbers.yml index 540dfff0fa..9bffce0759 100644 --- a/.github/workflows/check-migration-numbers.yml +++ b/.github/workflows/check-migration-numbers.yml @@ -3,8 +3,9 @@ name: Check Migration Numbers on: pull_request: branches: [main] - paths: - - 'server/src/db/migrations/**' + # Intentionally no `paths` filter — a PR that doesn't touch the + # migrations dir can still collide with one that's been merged since + # it branched, if main added a number the PR reserved upstream. push: branches: [main] paths: @@ -16,21 +17,48 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 + with: + fetch-depth: 0 - - name: Check for duplicate migration number prefixes + - name: Check for duplicate migration numbers against current main run: | - dupes=$(ls server/src/db/migrations/*.sql \ - | xargs -n1 basename \ - | sed 's/_.*//' \ - | sort \ - | uniq -d) + # On pull_request, GitHub checks out the speculative merge of the + # PR into main as computed at PR-open (or at last conflict). That + # merge commit can go stale when main moves forward without + # triggering a re-compute, which is exactly how #2793 and #2800 + # both passed this check with colliding migration 419s. + # + # Guard against the stale-merge case by taking the union of + # migration filenames on the PR branch AND on origin/main, then + # hunting dupes in that union. Same check on push-to-main has no + # merge step — it just sees what's already merged. + git fetch origin main --depth=1 2>/dev/null || true + + # Files on this ref (PR's speculative merge, or main). + local_files=$(ls server/src/db/migrations/*.sql 2>/dev/null | xargs -n1 basename || true) + + # Files on main right now (empty if we're already on main). + main_files="" + if [ "${{ github.event_name }}" = "pull_request" ]; then + main_files=$(git ls-tree --name-only -r origin/main -- server/src/db/migrations/ \ + | sed 's|.*/||' \ + | grep '\.sql$' || true) + fi + + # Union of both file lists, unique by full filename. + all_files=$(printf '%s\n%s\n' "$local_files" "$main_files" | sort -u | sed '/^$/d') + + # Extract the leading number prefix and look for duplicates. + dupes=$(echo "$all_files" | sed 's/_.*//' | sort | uniq -d) + if [ -n "$dupes" ]; then echo "::error::Duplicate migration number prefixes found: $dupes" echo "" - echo "These migration files share a number prefix:" + echo "These migration files share a number prefix (one may be on main," + echo "the other in this PR — rebase + renumber yours if so):" for prefix in $dupes; do - ls server/src/db/migrations/${prefix}_*.sql + echo "$all_files" | awk -v p="$prefix" '$0 ~ "^"p"_" { print " " $0 }' done exit 1 fi - echo "No duplicate migration numbers found." + echo "No duplicate migration numbers found (checked PR + current main)." diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 5a10f7c5e6..e53220f729 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -10,8 +10,37 @@ concurrency: cancel-in-progress: false jobs: + # Preflight: cheap sanity checks that must pass before we spend the + # Fly build/deploy budget. If main has a broken state (e.g. two PRs + # merged minutes apart reserved the same migration number), catch it + # here instead of shipping a container that crashloops on boot. + preflight: + name: Preflight + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: No duplicate migration numbers + run: | + dupes=$(ls server/src/db/migrations/*.sql \ + | xargs -n1 basename \ + | sed 's/_.*//' \ + | sort \ + | uniq -d) + if [ -n "$dupes" ]; then + echo "::error::Duplicate migration number prefixes on main: $dupes" + echo "" + echo "Deploy blocked — main would crashloop on boot. Land a renumber" + echo "hotfix before the next push can deploy. Duplicate files:" + for prefix in $dupes; do + ls server/src/db/migrations/${prefix}_*.sql + done + exit 1 + fi + echo "No duplicate migration numbers on main." + deploy: name: Deploy to Fly.io + needs: preflight runs-on: ubuntu-latest steps: - name: Checkout