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