From c5295175b9b634393684901d597851525c54e450 Mon Sep 17 00:00:00 2001 From: Peyton-Spencer Date: Tue, 28 Jul 2026 13:36:40 -0400 Subject: [PATCH] fix(ci): reject a PR whose merge into main would leave two Alembic heads The Migration order job validated the PR branch in isolation, so a branch that was linear against the main it was cut from stayed green after a second migration landed. That is the 2026-07-28 outage: #505's last check ran 2026-07-27T18:59, #524 landed 85 minutes later, and #505 merged ~21 hours after that on a check that had been true when it ran and false ever since. main ended up with two heads, 1183 DB test errors and a dead deploy. Resolve the merge result -- base plus branch -- rather than the branch, and assert exactly one head. Migrations are immutable and none may be deleted, both asserted against the merge base, so the merge is the union of the two file sets and needs no actual merge to evaluate. That also lets the check run for any PR from any checkout, which the second half of the fix needs. The script alone would not have caught this. GitHub re-runs a PR's checks when the PR moves, not when its base does, and this repo does not require branches to be up to date before merging, so the stale green was mergeable the whole time. Every push to main now re-checks each open PR that adds a migration and posts a commit status, so a PR that has just been made undeployable says so on the PR. Failures name both revisions, the file each lives in, which side each came from, and both remedies. A branch that merely inherits a divergent main is told it is not at fault. Co-Authored-By: Claude Opus 5 --- .github/workflows/migration-order.yml | 31 +++ CLAUDE.md | 21 ++ .../scripts/test_check_migration_order.py | 252 +++++++++++++++++- scripts/check_migration_order.py | 191 +++++++++++-- scripts/recheck_open_pr_migrations.sh | 87 ++++++ 5 files changed, 548 insertions(+), 34 deletions(-) create mode 100755 scripts/recheck_open_pr_migrations.sh diff --git a/.github/workflows/migration-order.yml b/.github/workflows/migration-order.yml index c5312bdf..3ff2c29d 100644 --- a/.github/workflows/migration-order.yml +++ b/.github/workflows/migration-order.yml @@ -4,6 +4,9 @@ on: pull_request: types: [opened, synchronize, reopened] merge_group: + push: + branches: [main] + workflow_dispatch: permissions: contents: read @@ -15,6 +18,7 @@ concurrency: jobs: migration-order: name: Migration order + if: github.event_name != 'push' runs-on: blacksmith-4vcpu-ubuntu-2404 steps: - name: Checkout @@ -22,5 +26,32 @@ jobs: with: fetch-depth: 0 + # Asserts the *merge result* resolves to a single head, not just this + # branch. A branch cut before another migration landed on main is + # perfectly linear on its own and still forks the chain once merged. - name: Check Alembic migration order run: python scripts/check_migration_order.py origin/main + + # The job above is only ever as fresh as the PR's last push. GitHub does not + # re-run a PR's checks when its *base* moves, and this repo does not require + # branches to be up to date before merging, so a green check can quietly go + # stale and stay mergeable -- which is exactly what shipped the 2026-07-28 + # two-head outage. Re-check every open PR whenever main moves. + recheck-open-prs: + name: Re-check open PRs against new main + if: github.event_name == 'push' || github.event_name == 'workflow_dispatch' + runs-on: blacksmith-4vcpu-ubuntu-2404 + permissions: + contents: read + pull-requests: read + statuses: write + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + + - name: Re-check open PRs + env: + GH_TOKEN: ${{ github.token }} + run: ./scripts/recheck_open_pr_migrations.sh diff --git a/CLAUDE.md b/CLAUDE.md index 96046531..0eb35cee 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -50,6 +50,18 @@ async-substrate-interface). Wire shapes are `ditto/api_models` (Pydantic). - **Migrations own the schema.** `ditto/db/models.py` describes it in Python but Alembic under `alembic/versions/` is the source of truth — keep them in sync and add a migration for any schema change. +- **One Alembic head, always — rebase before you add a migration.** Alembic + linears the chain by `down_revision`, *not* by merge date, so two branches + that each extend the same parent stay divergent however git merges them, and + `alembic upgrade head` then refuses to run at all (`Multiple head revisions + are present`) — taking the deploy and every DB test with it. Rebase onto + current `origin/main` and point `down_revision` at its head before opening a + PR, renumbering the `YYYY_MM_DD_` filename if main has moved past your date. + This is enforced, not just advised: `Migration order` resolves the **merge + result** rather than your branch alone, and every push to `main` re-checks + each open PR — so a PR that was green when you pushed it goes red the moment + someone else's migration lands, instead of staying mergeable. Reproduce + either locally with `python scripts/check_migration_order.py origin/main`. - **Adding a column to a hot table? Use `safe_add_column`.** `op.add_column` holds an `AccessExclusiveLock` until the migration commits, so a plain add-then-backfill stalls every writer for the length of the backfill — that is @@ -113,6 +125,15 @@ enforces all four; Python checks run on 3.11 and 3.12. - Put unit tests next to the package they cover under `ditto/tests/`. - `make test-db-reset` forces a template rebuild; `make test-db-clean` reaps every harness-owned database. +- **Hundreds of DB failures after touching migrations? Reset the template + before believing them.** The harness migrates `ditto_test_template` once and + then clones it, so a template built while the chain was broken keeps failing + every DB test long after the chain is fixed — and it fails in a way that + reads as *your* change's fault. During the 2026-07-28 two-head fix the first + run on a clean worktree came back with 513 failures from exactly this; the + same worktree was green after `make test-db-reset`. The tell is breadth: a + real regression fails tests near what you touched, a stale template fails + everything that opens a database. ## Gotchas diff --git a/ditto/tests/scripts/test_check_migration_order.py b/ditto/tests/scripts/test_check_migration_order.py index d279208b..fc2b2c6b 100644 --- a/ditto/tests/scripts/test_check_migration_order.py +++ b/ditto/tests/scripts/test_check_migration_order.py @@ -9,7 +9,9 @@ from scripts.check_migration_order import ( Migration, MigrationError, + merge_result, parse_migration, + require_single_merged_head, resolve_working_tree_head, validate_linear_history, ) @@ -132,12 +134,12 @@ def test_head_mode_prints_the_single_head(tmp_path: Path) -> None: def test_head_mode_names_every_head_and_the_merge_that_fixes_it( tmp_path: Path, ) -> None: - """The 2026-07-25 shape: two PRs extended the same parent and both merged. + """Two migrations extended the same parent and both landed. - Each passed its own migration-order check against `main` at the time, and - the divergence only existed once both had landed -- so the deploy is the - first place it can be caught. The message has to carry the revisions and - the remedy, because alembic's own error carries neither. + By the time the deploy sees this the damage is done -- the pre-merge + guard below is what is supposed to stop it -- but `update.sh` still has + to explain itself. The message has to carry the revisions, the files and + the remedy, because alembic's own error carries none of them. """ _write_history(tmp_path, diverged=True) @@ -147,7 +149,247 @@ def test_head_mode_names_every_head_and_the_merge_that_fixes_it( assert "2 head revisions are present" in result.stderr assert "e5b8c31d47af" in result.stderr assert "e7b4c02a5d18" in result.stderr + assert "2026_07_02_first.py" in result.stderr + assert "2026_07_02_second.py" in result.stderr assert ( 'uv run alembic merge -m "merge heads" e5b8c31d47af e7b4c02a5d18' in result.stderr ) + + +# --- the merge result ------------------------------------------------------- +# +# The 2026-07-28 outage in one sentence: two PRs that were each individually +# linear against the `main` they were cut from produced a second head once +# both had merged, because Alembic linears by down_revision and not by merge +# date. Everything below is about catching that *before* the merge. + + +def test_merge_result_is_the_union_of_both_file_sets() -> None: + base = [migration("root", None), migration("landed", "root")] + head = [migration("root", None), migration("mine", "root")] + + merged = merge_result(base, head) + + assert [m.revision for m in merged] == ["landed", "mine", "root"] + + +def test_merge_result_keeps_the_base_copy_of_a_shared_path() -> None: + """Migrations are immutable, so the base's copy is the one that survives.""" + base = [Migration("shared.py", "base-revision", None)] + head = [Migration("shared.py", "head-revision", None)] + + assert merge_result(base, head) == [Migration("shared.py", "base-revision", None)] + + +def test_single_merged_head_returns_the_head_when_the_branch_extends_base() -> None: + base = [migration("root", None)] + head = [migration("root", None), migration("mine", "root")] + + merged = merge_result(base, head) + + assert require_single_merged_head(merged, "origin/main", {"root"}) == "mine" + + +def test_merged_heads_name_both_revisions_their_files_and_both_remedies() -> None: + """The exact 2026-07-28 fork, with the revisions and files it really had.""" + base = [ + Migration( + "alembic/versions/2026_07_27_record_evicted.py", "b2e9d4a17c60", None + ), + Migration( + "alembic/versions/2026_07_27_add_ticket_failure_detail.py", + "a7c14f8bd260", + "b2e9d4a17c60", + ), + Migration( + "alembic/versions/2026_07_27_reinstate_an_evicted_submission.py", + "c7a4f1e2b903", + "a7c14f8bd260", + ), + ] + head = [ + base[0], + Migration( + "alembic/versions/2026_07_27_add_never_disclose_release_policy.py", + "f4b7d2c91ae5", + "b2e9d4a17c60", + ), + ] + + with pytest.raises(MigrationError) as excinfo: + require_single_merged_head( + merge_result(base, head), "origin/main", {"c7a4f1e2b903"} + ) + + message = str(excinfo.value) + assert "would leave 2 Alembic heads" in message + # Both offending revisions, each named with the file it lives in. + assert "c7a4f1e2b903 alembic/versions/2026_07_27_reinstate_an_evicted" in message + assert "f4b7d2c91ae5 alembic/versions/2026_07_27_add_never_disclose" in message + # And which side each came from, so the author knows what to rebase onto. + assert "already a head on origin/main" in message + assert "added by this branch" in message + # Both remedies. + assert "rebase onto current origin/main" in message + assert 'uv run alembic merge -m "merge heads" c7a4f1e2b903 f4b7d2c91ae5' in message + + +def _git(repo: Path, *args: str) -> str: + return subprocess.run( + ["git", *args], + cwd=repo, + check=True, + capture_output=True, + text=True, + ).stdout + + +def _commit(repo: Path, name: str, revision: str, down: str | None) -> None: + path = repo / "alembic" / "versions" / name + path.write_text(MIGRATION.format(revision=revision, down=down)) + _git(repo, "add", str(path)) + _git(repo, "commit", "-m", f"add {revision}") + + +def _run_check( + repo: Path, base_ref: str, head_ref: str +) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [ + sys.executable, + str(ROOT / "scripts" / "check_migration_order.py"), + base_ref, + head_ref, + ], + cwd=repo, + capture_output=True, + text=True, + check=False, + ) + + +@pytest.fixture +def stale_branch_repo(tmp_path: Path) -> Path: + """The 2026-07-28 fork, rebuilt as two real branches. + + `main` is at b2e9d4a17c60 when `never-disclose` is cut from it. Two more + migrations then land on `main` (#553, then #524). Neither branch is ever + rebased, and both are individually linear. + """ + repo = tmp_path / "repo" + (repo / "alembic" / "versions").mkdir(parents=True) + _git(repo, "init", "-q", "-b", "main") + _git(repo, "config", "user.email", "test@example.com") + _git(repo, "config", "user.name", "test") + + _commit(repo, "2026_07_26_add_artifact_fetch_audit.py", "e8b3c05d7a41", None) + _commit(repo, "2026_07_27_record_evicted_leases.py", "b2e9d4a17c60", "e8b3c05d7a41") + + # PR #505 is cut here, while b2e9d4a17c60 is still the head of main. + _git(repo, "switch", "-q", "-c", "never-disclose") + _commit( + repo, + "2026_07_27_add_never_disclose_release_policy.py", + "f4b7d2c91ae5", + "b2e9d4a17c60", + ) + + # Meanwhile main moves on, twice, and nobody rebases #505 onto it. + _git(repo, "switch", "-q", "main") + _commit( + repo, "2026_07_27_add_ticket_failure_detail.py", "a7c14f8bd260", "b2e9d4a17c60" + ) + _commit( + repo, + "2026_07_27_reinstate_an_evicted_submission.py", + "c7a4f1e2b903", + "a7c14f8bd260", + ) + return repo + + +def test_the_stale_branch_was_genuinely_green_against_the_base_it_was_cut_from( + stale_branch_repo: Path, +) -> None: + """Why the old check passed: at the time it ran, nothing was wrong. + + This is the run that happened on #505 at 2026-07-27T18:59. It is correct, + and it stayed on the PR as a green check for the ~21 hours until #505 + merged -- long after it had stopped being true. + """ + # main as it stood when the branch was cut: b2e9d4a17c60 was the head. + base = _git(stale_branch_repo, "rev-parse", "main~2").strip() + + result = _run_check(stale_branch_repo, base, "never-disclose") + + assert result.returncode == 0, result.stderr + assert "merged head f4b7d2c91ae5" in result.stdout + + +def test_merging_the_stale_branch_into_current_main_is_rejected( + stale_branch_repo: Path, +) -> None: + """The guard. Same branch, same content, re-checked against main as it is now. + + Nothing about the branch changed -- only what it would be merging into. + """ + result = _run_check(stale_branch_repo, "main", "never-disclose") + + assert result.returncode == 1 + assert "would leave 2 Alembic heads" in result.stderr + assert "c7a4f1e2b903" in result.stderr + assert "f4b7d2c91ae5" in result.stderr + assert "2026_07_27_reinstate_an_evicted_submission.py" in result.stderr + assert "2026_07_27_add_never_disclose_release_policy.py" in result.stderr + assert "rebase onto current main" in result.stderr + assert 'alembic merge -m "merge heads" c7a4f1e2b903 f4b7d2c91ae5' in result.stderr + + +def test_a_migration_missing_only_because_main_moved_is_not_a_deletion( + stale_branch_repo: Path, +) -> None: + """Removal is judged against the merge base, not against the base tip. + + The stale branch does not contain #524's migration. It did not delete it + -- it was cut before it existed -- and reporting that as a deletion would + bury the real finding. + """ + result = _run_check(stale_branch_repo, "main", "never-disclose") + + assert "existing migrations were removed" not in result.stderr + + +def test_rebasing_the_branch_onto_current_main_clears_the_failure( + stale_branch_repo: Path, +) -> None: + """The remedy the message recommends actually resolves it.""" + _git(stale_branch_repo, "switch", "-q", "never-disclose") + _git(stale_branch_repo, "rebase", "-q", "main") + path = ( + stale_branch_repo + / "alembic" + / "versions" + / "2026_07_27_add_never_disclose_release_policy.py" + ) + path.write_text(MIGRATION.format(revision="f4b7d2c91ae5", down="c7a4f1e2b903")) + _git(stale_branch_repo, "commit", "-qam", "repoint onto current main head") + + result = _run_check(stale_branch_repo, "main", "never-disclose") + + assert result.returncode == 0, result.stderr + assert "merged head f4b7d2c91ae5" in result.stdout + + +def test_a_branch_that_inherits_a_divergent_base_is_not_blamed_for_it() -> None: + """`main` broken under a PR is `main`'s fault, and the message says so.""" + base = [ + migration("root", None), + migration("one", "root"), + migration("two", "root"), + ] + + with pytest.raises(MigrationError, match="did not cause the divergence"): + require_single_merged_head( + merge_result(base, base), "origin/main", {"one", "two"} + ) diff --git a/scripts/check_migration_order.py b/scripts/check_migration_order.py index affa8949..f2129773 100644 --- a/scripts/check_migration_order.py +++ b/scripts/check_migration_order.py @@ -3,20 +3,30 @@ Two modes: -``check_migration_order.py [base_ref]`` - The CI mode. Validates the PR's migrations against ``base_ref`` - (default ``origin/main``): nothing removed, nothing edited, dated - forward, and resolving to exactly one head. +``check_migration_order.py [base_ref] [head_ref]`` + The CI mode. Validates *head_ref* -- the working tree by default -- + against ``base_ref`` (default ``origin/main``): nothing removed, + nothing edited, dated forward, and the **merge result** resolving to + exactly one head. + + That last one is the whole point, and it is deliberately asserted + against ``base_ref + head_ref`` rather than against the branch alone. + A branch cut before a second migration landed on ``main`` is perfectly + linear on its own; the divergence exists only in the merge, which is + the thing that gets deployed. Passing ``head_ref`` explicitly lets the + check be re-run for an open PR from any checkout -- notably from + ``main`` after it moves, which is when a green PR silently goes stale. ``check_migration_order.py --head`` The deploy mode. Resolves the *working tree's* migrations to a single head and prints it, using only the standard library -- no venv, no alembic import, no database. ``scripts/update.sh`` runs this before it - touches the host, because the CI mode cannot catch the case that - matters at deploy time: two PRs that each extended the single head of - ``main`` independently, passed their own checks, and produced two - heads only once both had merged. ``alembic upgrade head`` then refuses - to run with ``Multiple head revisions are present``. + touches the host: a last line of defence for anything that reached + ``main`` without passing the merge-result check above (a bypass, a + direct push, a check that was never required). + +Both failure paths name every head, the file each lives in, and the two +ways to reconcile them. Alembic's own error carries none of that. """ from __future__ import annotations @@ -181,12 +191,135 @@ def _head_migrations() -> list[Migration]: return [parse_migration(str(path), path.read_text()) for path in paths] -def check(base_ref: str) -> tuple[int, str, str]: - """Validate HEAD against the immutable migration history on *base_ref*.""" +def _merge_base(base_ref: str, head_ref: str) -> str: + return _git("merge-base", base_ref, head_ref).strip() + + +def _paths_for(ref: str | None) -> set[str]: + """Migration paths at *ref*, or in the working tree when it is ``None``.""" + if ref is None: + return {str(path) for path in MIGRATIONS_DIR.glob("*.py")} + return set(_paths_at(ref)) + + +def _migrations_for(ref: str | None) -> list[Migration]: + return _head_migrations() if ref is None else _migrations_at(ref) + + +def merge_result(base: list[Migration], head: list[Migration]) -> list[Migration]: + """The ``alembic/versions`` content that merging *head* into *base* yields. + + Migrations are immutable and none may be deleted -- :func:`check` asserts + both against the merge base first -- so the merge is exactly the union of + the two file sets. That means the merge result can be resolved without + performing the merge, from any checkout, which is what lets this run + against a PR branch that was never rebased. + """ + by_path = {migration.path: migration for migration in head} + by_path.update({migration.path: migration for migration in base}) + return [by_path[path] for path in sorted(by_path)] + + +def _head_table( + heads: list[str], + by_revision: dict[str, Migration], + base_heads: frozenset[str] | set[str] | None = None, + base_ref: str | None = None, +) -> str: + """One line per head: the revision, the file it lives in, and its origin.""" + rows = [] + for revision in heads: + migration = by_revision.get(revision) + row = f" {revision} {migration.path if migration else ''}" + if base_heads is not None: + row += ( + f" (already a head on {base_ref})" + if revision in base_heads + else " (added by this branch)" + ) + rows.append(row) + return "\n".join(rows) + + +def _remedy( + heads: list[str], base_ref: str = "origin/main", base_head: str | None = None +) -> str: + """Why two heads break everything, and the two ways to reconcile them.""" + rebase = ( + f" * rebase onto current {base_ref} and repoint down_revision at its " + f"head {base_head}" + if base_head is not None + else f" * rebase onto current {base_ref} and repoint down_revision at its head" + ) + return "\n".join( + [ + "Alembic linears by down_revision, not by merge date, so two " + "branches that each extend the same parent stay divergent however " + "git merges them. `alembic upgrade head` then refuses to run with " + "\"Multiple head revisions are present for given argument 'head'\", " + "which fails every migration -- the deploy and the whole DB test " + "tier with it.", + "Reconcile on this branch, before merging, either way:", + f"{rebase} (renumbering the YYYY_MM_DD_ filename too if it now " + "precedes the newest migration there), or", + f' * uv run alembic merge -m "merge heads" {" ".join(heads)}', + "Review both branches for conflicting changes to the same table " + "before assuming an empty merge revision is correct.", + ] + ) + + +def require_single_merged_head( + merged: list[Migration], base_ref: str, base_heads: set[str] +) -> str: + """Return the merge result's sole head, or explain the divergence. + + This is the assertion the 2026-07-28 outage needed. Validating a branch on + its own passes a PR that was cut before a second migration landed on + *base_ref*: both PRs are individually linear and the second head exists + only in the merge -- which is the thing that actually gets deployed. + """ + heads = sorted(_history_heads(merged, f"{base_ref} + this branch")) + if len(heads) == 1: + return heads[0] + + by_revision = {migration.revision: migration for migration in merged} + message = [ + f"merging this branch into {base_ref} would leave {len(heads)} " + "Alembic heads, not one:", + _head_table(heads, by_revision, base_heads=base_heads, base_ref=base_ref), + ] + if all(revision in base_heads for revision in heads): + message.append( + f"Every head above is already on {base_ref}, so this branch did " + "not cause the divergence -- but it does not reconcile it either, " + f"and {base_ref} stays undeployable until something does." + ) + message.append( + _remedy( + heads, + base_ref=base_ref, + base_head=next(iter(base_heads)) if len(base_heads) == 1 else None, + ) + ) + raise MigrationError("\n".join(message)) + + +def check(base_ref: str, head_ref: str | None = None) -> tuple[int, str, str]: + """Validate *head_ref* -- the working tree by default -- against *base_ref*. + + Nothing removed, nothing edited, dated forward, and -- the assertion that + matters -- the *merge result* resolves to exactly one head. + """ + label = head_ref or "HEAD" base_paths = set(_paths_at(base_ref)) - head_paths = {str(path) for path in MIGRATIONS_DIR.glob("*.py")} + head_paths = _paths_for(head_ref) - removed = sorted(base_paths - head_paths) + # Against the merge base, not the base tip: a migration that landed on + # `base_ref` after this branch was cut is missing from the branch without + # the branch having removed anything. + ancestor_paths = set(_paths_at(_merge_base(base_ref, label))) + removed = sorted(ancestor_paths - head_paths) if removed: raise MigrationError("existing migrations were removed: " + ", ".join(removed)) @@ -194,7 +327,7 @@ def check(base_ref: str) -> tuple[int, str, str]: "diff", "--name-only", "--diff-filter=M", - f"{base_ref}...HEAD", + f"{base_ref}...{label}", "--", str(MIGRATIONS_DIR), ).splitlines() @@ -202,9 +335,11 @@ def check(base_ref: str) -> tuple[int, str, str]: raise MigrationError("existing migrations are immutable: " + ", ".join(changed)) base_migrations = _migrations_at(base_ref) - head_migrations = _head_migrations() + head_migrations = _migrations_for(head_ref) base_heads = _history_heads(base_migrations, base_ref) - head_revision = validate_linear_history(head_migrations, "HEAD") + merged_head = require_single_merged_head( + merge_result(base_migrations, head_migrations), base_ref, base_heads + ) new_paths = sorted(head_paths - base_paths) base_dates = [MIGRATION_NAME.match(Path(path).name) for path in base_paths] @@ -228,10 +363,10 @@ def check(base_ref: str) -> tuple[int, str, str]: f"{latest_base_date}" ) - if new_paths and head_revision in base_heads: - raise MigrationError("new migrations do not extend the base migration head") - - return len(new_paths), ", ".join(sorted(base_heads)), head_revision + # "new migrations extend the base head" needs no separate assertion: a new + # migration that chains off anything else is a second head, and + # require_single_merged_head has already rejected it by name. + return len(new_paths), ", ".join(sorted(base_heads)), merged_head def resolve_working_tree_head() -> str: @@ -244,15 +379,12 @@ def resolve_working_tree_head() -> str: heads = sorted(_history_heads(migrations, "working tree")) if len(heads) == 1: return heads[0] - joined = " ".join(heads) + by_revision = {migration.revision: migration for migration in migrations} raise MigrationError( f"{len(heads)} head revisions are present: {', '.join(heads)}.\n" - "`alembic upgrade head` cannot choose between them and will refuse to " - "run. Two migrations were merged that each extended the same parent, " - "so a human has to say how they combine. Reconcile on a branch with:\n" - f' uv run alembic merge -m "merge heads" {joined}\n' - "Review the merged branches for conflicting changes to the same table " - "before assuming an empty merge revision is correct." + + _head_table(heads, by_revision) + + "\n" + + _remedy(heads) ) @@ -271,14 +403,15 @@ def main() -> int: if len(sys.argv) > 1 and sys.argv[1] == "--head": return head_mode() base_ref = sys.argv[1] if len(sys.argv) > 1 else "origin/main" + head_ref = sys.argv[2] if len(sys.argv) > 2 else None try: - count, base_head, head = check(base_ref) + count, base_head, head = check(base_ref, head_ref) except (MigrationError, subprocess.CalledProcessError) as exc: print(f"migration-order: {exc}", file=sys.stderr) return 1 print( f"migration-order: ok ({count} new migration(s); " - f"{base_ref} head {base_head}; HEAD head {head})" + f"{base_ref} head {base_head}; merged head {head})" ) return 0 diff --git a/scripts/recheck_open_pr_migrations.sh b/scripts/recheck_open_pr_migrations.sh new file mode 100755 index 00000000..7b67a016 --- /dev/null +++ b/scripts/recheck_open_pr_migrations.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +# Re-check every open PR's merge result against `main` as it is *now*. +# +# The per-PR migration-order check runs when the PR is pushed and never +# again. That is exactly how the 2026-07-28 outage happened: #505's last +# green run was 2026-07-27T18:59, #524 landed 85 minutes later, and nothing +# re-evaluated the pair. #505 then merged ~21 hours later on a check that had +# been true when it ran and false ever since, and `main` had two Alembic +# heads -- 1183 DB test errors and a dead deploy. +# +# GitHub re-runs a PR's checks when the PR moves, not when its base does, and +# "require branches to be up to date before merging" is off on this repo. So +# nothing else closes this window. This runs on every push to `main` and +# posts a commit status on each open PR that adds a migration, which is what +# makes a newly-stale PR say so on the PR itself. +set -euo pipefail + +REPO="${GITHUB_REPOSITORY:-ditto-assistant/ditto-platform}" +BASE_REF="${1:-origin/main}" +CONTEXT="migration-order/merge-result" +RUN_URL="${GITHUB_SERVER_URL:-https://github.com}/${REPO}/actions/runs/${GITHUB_RUN_ID:-0}" + +# If `main` itself is divergent then every PR below inherits that failure and +# none of them caused it. Say so once, fail loudly, and post nothing: red +# statuses on innocent PRs are how a guard gets muted. +if ! head_revision=$(python3 scripts/check_migration_order.py --head 2>&1); then + echo "::error title=main has multiple Alembic heads::${head_revision}" + exit 1 +fi +echo "${BASE_REF} resolves to a single head: ${head_revision}" + +# Every open PR, with a count of the migrations it adds -- not just the ones +# that add migrations. A PR that adds none cannot fork the chain and is +# reported green without being fetched, which keeps the status present on +# every PR (so the context is safe to require) and stops a stale red from +# outliving the migration that caused it. +open_prs=$(gh pr list --repo "${REPO}" --state open --limit 100 \ + --json number,headRefOid,files \ + --jq '.[] | "\(.number)\t\(.headRefOid)\t\([.files[].path + | select(startswith("alembic/versions/"))] | length)"') + +if [[ -z "${open_prs}" ]]; then + echo "no open PRs" + exit 0 +fi + +stale=() +while IFS=$'\t' read -r number sha migrations; do + [[ -n "${number}" ]] || continue + if ((migrations == 0)); then + state=success + description="Adds no migration; cannot fork the Alembic chain." + echo "PR #${number}: no migrations" + elif output=$(git fetch --quiet --force --no-tags origin \ + "pull/${number}/head:refs/pr/${number}" &1 && + python3 scripts/check_migration_order.py \ + "${BASE_REF}" "refs/pr/${number}" 2>&1); then + state=success + description="Merging into main leaves exactly one Alembic head." + echo "PR #${number}: ok" + else + state=failure + description="Merging into main would leave more than one Alembic head." + stale+=("${number}") + echo "::warning title=PR #${number} would now leave multiple Alembic heads::${output}" + fi + gh api --method POST "repos/${REPO}/statuses/${sha}" \ + -f state="${state}" \ + -f context="${CONTEXT}" \ + -f description="${description}" \ + -f target_url="${RUN_URL}" /dev/null +done <<<"${open_prs}" + +# The finding belongs on the PRs, which now carry a red status. Failing this +# run as well would only paint `main` red for something main did not do. +if ((${#stale[@]})); then + { + echo "### Open PRs made undeployable by this push" + echo + echo "Each now resolves to more than one Alembic head when merged." + echo "They carry a failing \`${CONTEXT}\` status until rebased." + echo + for number in "${stale[@]}"; do + echo "- #${number}" + done + } >>"${GITHUB_STEP_SUMMARY:-/dev/stdout}" +fi