From e15a6c8176ac7633ef84df53c49fe5e50c16e5a0 Mon Sep 17 00:00:00 2001 From: thedancingdeveloper <306930456+thedancingdeveloper@users.noreply.github.com> Date: Tue, 1 Sep 2026 00:41:39 +0000 Subject: [PATCH] ci: cancel superseded runs instead of banking one per commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Push runs to dev/main/prod previously kept one CI and one build run per commit, because cancelling was unsafe: ci.yml classified a push by before..sha, so a cancelled run's files were checked by nothing, ever (two escapes reached dev that way), and a cancelled build left no sha- image. Make cancellation safe instead of avoiding it: a push now classifies as a change to everything, so a replacing run fully covers a superseded one, and ci.yml, build.yml and codeql.yml switch to newest-wins per ref (cancel-in-progress: true). The ref stays in every group so dev/main/prod — the same commit after a fast-forward release — never evict each other, and PRs group by their own merge ref. e2e.yml keeps queueing: two stacks would race the same loopback port. The cost accepted: a docs-only push now runs the full matrix (docs.yml remains the cheap path-filtered gate), and only the newest commit of a merge burst gets images — which is the commit anything would deploy. tests/test_deploy.py pins the pairing: newest-wins keying and classify-everything-on-push must move together. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_0161etTkUPyRoUXnHQ8xgKUu --- .github/workflows/build.yml | 47 ++++++++-------------- .github/workflows/ci.yml | 72 +++++++++++++++------------------- .github/workflows/codeql.yml | 5 ++- tests/test_deploy.py | 75 ++++++++++++++++++------------------ 4 files changed, 90 insertions(+), 109 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 2cfe498..43e3c13 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -88,37 +88,24 @@ env: CACHE_IMAGE: ${{ vars.VOGT_BUILDKIT_CACHE_REGISTRY }}/vogt-buildcache concurrency: - # Keyed by *commit*, not by ref, since 2026-08-19. + # Newest-wins, by ref: a superseded build — pending or in progress — is + # cancelled when a newer push joins its group. Only the newest commit of a + # burst gets images, and that is the commit anything would deploy; a + # superseded commit's missing `sha-` image costs nothing, because nothing + # pins a digest it was never offered. The cost this accepts is the + # burst-tail gap: while a merge train runs, the branch has no *fresh* + # completed image until the final run lands — the previous per-commit + # keying banked a full build per merge to avoid that, and buried the small + # self-hosted pool instead. # - # "Only the newest commit on a branch is worth an image" is true of a repo - # that merges slowly. This one merged to main 23 times in 24 hours — a - # median gap of 16 minutes — while this workflow took a median of 19 minutes - # and a p90 of 41. Every build was therefore superseded before it finished: - # 12 of the last 13 main builds were cancelled, 36% of the repository's - # entire CI compute was spent on runs that were thrown away, and main - # frequently had no `sha-` image at all. Nothing was left for the estate - # workflow to pin, so shipping a fix meant waiting for a quiet window. - # - # Keying by SHA means no two commits share a group and none can supersede - # another — the same reasoning `ci.yml` already records, and for the same - # reason. It costs runner time on a burst, which is what the capacity freed - # by lifting the runners' four-core cap is for. - # - # The ref is in the key *as well*, and by SHA alone this is wrong now. - # Releases move `dev` → `main` → `prod` by fast-forward, so all three refs - # carry the identical commit and a SHA-only group put three different - # streams in one. It is not hypothetical: pushing `main` during the prod - # cutover cancelled `dev`'s build of the same commit, and `cancel-in-progress` - # did not save it — that setting governs runs already *in progress*, while a - # still-*pending* run is evicted whenever a newer run joins its group, - # always. On a small self-hosted pool almost every run is pending for a - # while. `ci.yml` records finding the same hole the same way. - # - # Each stream builds its own images (`dev-`, `sha-`, `prod-`), so a build - # cancelled by a sibling ref is an image that never exists — which for - # `prod` means nothing for the estate workflow to pin. - group: build-${{ github.ref }}-${{ github.sha }} - cancel-in-progress: false + # The ref stays in the key, and that part is not optional: releases move + # `dev` → `main` → `prod` by fast-forward, so the refs carry the identical + # commit, and a ref-free group let a `main` push during the prod cutover + # evict `dev`'s build of the same commit. Each stream builds its own + # images (`dev-`, `sha-`, `prod-`), so a build cancelled by a sibling ref + # is an image that never exists. + group: build-${{ github.ref }} + cancel-in-progress: true jobs: # Docker Hub is not on the critical path any more (#33): both Dockerfiles diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 792db3e..4fa4547 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,37 +34,26 @@ on: workflow_dispatch: concurrency: - # Keyed by *commit* for a push, and by pull request for a PR. + # Newest-wins, by ref: a superseded run — pending or in progress — is + # cancelled when a newer one joins its group, so a merge burst costs one + # full run instead of a bank of queued ones. # - # The first version of this keyed pushes by ref and set `cancel-in-progress` - # to false for them, on the reasoning below — and that is not sufficient, - # which was found by watching it fail rather than by reading the docs - # again. `cancel-in-progress` governs runs that are *in progress*. A run - # that is still **pending** is cancelled whenever a newer run joins its - # group, always, whatever that setting says. On a single self-hosted runner - # almost every run is pending for a while, so the hole stayed open and the - # workflow claimed it was shut. + # Cancelling is only safe because a run's coverage is fully contained in + # the run that replaces it. A pull request classifies against the merge + # base, so that was always true of PRs. A push is now classified as a + # change to *everything* (the classify step below), which restores the + # same property: the previous design classified a push by `before..sha`, + # and a cancelled incremental run meant its files were checked by nothing, + # ever — a lint error reached `dev` through exactly that gap twice. If + # push classification ever becomes incremental again, this key must go + # back to per-commit in the same change. # - # Keying by SHA means no two pushed commits ever share a group, so none can - # supersede another. It costs runner time on a burst — every commit is - # checked, which is the point. - # The ref is in the key too, for the reason `build.yml` records at length: - # `dev` and `main` carry the same commit once a release fast-forwards, so a - # SHA-only group let one branch's push evict the other's pending run — and a - # push run that never happens is a commit NFR-C1's path gating never checks. - group: ci-${{ github.workflow }}-${{ github.ref }}-${{ github.event.pull_request.number || github.sha }} - # Why any of this matters: NFR-C1's path gating classifies a push by - # `before..sha`, so each commit is checked by exactly one run and no later - # run ever looks at it again. Lose that run and those files are not - # "checked later" — they are checked never. It has happened twice: - # `tests/test_deploy.py` changed in one push whose run was superseded, and - # a lint error reached `dev` through the gap; then a `ruff format` failure - # went the same way while the supposed fix was in place. - # - # A pull request is different — its runs classify against the merge base, - # so a later run covers everything an earlier one would have, and - # cancelling the superseded one loses nothing. - cancel-in-progress: ${{ github.event_name == 'pull_request' }} + # The ref stays in the key: `dev` and `main` carry the same commit once a + # release fast-forwards, and a SHA-free, ref-free group would let one + # branch's push evict the other's run. A PR groups by its own + # `refs/pull/N/merge` ref, so PRs never evict each other or a branch. + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true permissions: contents: read @@ -110,8 +99,6 @@ jobs: env: EVENT_NAME: ${{ github.event_name }} BASE_REF: ${{ github.base_ref }} - BEFORE: ${{ github.event.before }} - SHA: ${{ github.sha }} run: | set -euo pipefail @@ -125,20 +112,23 @@ jobs: } >> "$GITHUB_OUTPUT" } - if [ "$EVENT_NAME" = "pull_request" ]; then - git fetch --no-tags origin "$BASE_REF" - base="$(git merge-base "origin/$BASE_REF" HEAD)" - range="$base..HEAD" - elif [ -z "$BEFORE" ] \ - || [ "$BEFORE" = "0000000000000000000000000000000000000000" ] \ - || ! git cat-file -e "$BEFORE^{commit}" 2>/dev/null; then - echo "no usable base commit; treating as a change to everything" + if [ "$EVENT_NAME" != "pull_request" ]; then + # A push checks everything, always. The concurrency group is + # newest-wins per ref, so a superseded push run is cancelled — + # which is only safe because the replacing run re-checks the + # whole tree. Classifying a push by `before..sha` under that + # policy would mean a cancelled run's files are checked by + # nothing, ever; the two escapes that found that hole are + # recorded in the concurrency comment at the top of this file. + echo "push: newest-wins concurrency, so every push checks everything" everything exit 0 - else - range="$BEFORE..$SHA" fi + git fetch --no-tags origin "$BASE_REF" + base="$(git merge-base "origin/$BASE_REF" HEAD)" + range="$base..HEAD" + files="$(git diff --name-only "$range")" echo "changed files:" echo "$files" diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 47f187a..a3ce656 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -28,8 +28,11 @@ jobs: runs-on: [self-hosted] timeout-minutes: 45 concurrency: + # Newest-wins: an analysis of a superseded commit is fully covered by + # the analysis that replaces it — CodeQL scans the whole tree, so + # cancelling loses nothing on any event. group: codeql-${{ github.ref }}-${{ matrix.language }} - cancel-in-progress: ${{ github.event_name == 'pull_request' }} + cancel-in-progress: true permissions: contents: read security-events: write diff --git a/tests/test_deploy.py b/tests/test_deploy.py index 954c265..d20c8a4 100644 --- a/tests/test_deploy.py +++ b/tests/test_deploy.py @@ -1040,37 +1040,40 @@ def test_the_opencode_registration_does_not_freeze_an_endpoint() -> None: ) -def test_no_two_pushed_commits_share_a_concurrency_group() -> None: - """NFR-C1's path gating makes a lost run lose coverage permanently. - - A push is classified by `before..sha`, so each commit is checked by - exactly one run and no later run looks at it again. Lose that run and - those files are not checked later — they are checked never. - - **`cancel-in-progress: false` is not enough, and believing it was cost a - second escape.** That setting governs runs which are *in progress*; a run - still **pending** is cancelled whenever a newer run joins its group, - unconditionally. On a single self-hosted runner nearly every run is - pending for a while, so a `ruff format` failure slipped through exactly - as the previous failure had, while the workflow carried a comment saying - the hole was shut. - - Keying a push by its commit is what closes it: no two pushed commits - share a group, so none can supersede another. A pull request still - cancels its own superseded runs, which loses nothing — those classify - against the merge base. +def test_newest_wins_ci_is_paired_with_full_classification_on_push() -> None: + """Cancelling a run is safe only when its replacement covers its ground. + + CI is newest-wins per ref: a superseded run — pending or in progress — + is cancelled when a newer one joins its group, so a merge burst costs + one run instead of a bank of queued ones. That is only sound because a + push run classifies as a change to *everything*: under the earlier + incremental `before..sha` classification, each commit was examined by + exactly one run, and a cancelled run's files were checked by nothing, + ever — two failures escaped to `dev` through that gap. These two + properties must move together, so they are asserted together. """ raw = (WORKFLOWS / "ci.yml").read_text(encoding="utf-8") group = re.search(r"^ group: (.+)$", raw, re.MULTILINE) assert group, "ci.yml declares a concurrency group" - assert "github.sha" in group.group(1), ( - f"pushes must be keyed by commit, not by ref: {group.group(1)!r}. Two " - "commits in one group means the older run can be cancelled while " - "pending, and its files are then checked by nothing, ever" + assert "github.ref" in group.group(1), ( + "the ref keeps `dev` and `main` — the same commit after a " + "fast-forward release — from evicting each other's runs" ) - assert "pull_request.number" in group.group(1), ( - "a pull request still groups by PR, so its superseded runs can be " - "cancelled — they classify against the merge base and lose nothing" + assert "cancel-in-progress: true" in raw, ( + "newest-wins is the policy: superseded runs are cancelled, not banked" + ) + assert "github.sha" not in group.group(1), ( + "per-commit keying banks a run per push; if that is being restored, " + "push classification may become incremental again in the same change" + ) + classify = _without_comments(raw) + assert '"$BEFORE' not in classify, ( + "a push must not classify by `before..sha`: newest-wins cancellation " + "would leave a superseded run's files checked by nothing, ever" + ) + assert "everything" in classify, ( + "the classify step's push arm must declare a change to everything, " + "which is what makes cancelling a superseded push run lose nothing" ) @@ -1229,17 +1232,15 @@ def test_the_gate_fails_on_anything_that_is_not_success_or_skipped() -> None: def test_a_workflow_may_cancel_only_when_a_later_run_covers_the_same_ground() -> None: - """The rule that tells `docs.yml` apart from `ci.yml`. - - Both cancel superseded runs on a branch and only one of them can afford - to. `docs.yml` checks the whole tree every time, so a later run covers - everything a superseded one would have. `ci.yml` classifies a push by - `before..sha` — each commit is examined by exactly one run, and a lost - run is coverage lost permanently. - - Asserted because the two files look the same at the point where they - differ, and the difference cost two escaped failures before it was - understood (§6.3 finding 19). + """A workflow that cancels superseded runs must re-cover their ground. + + `docs.yml` checks the whole tree every time, so a later run covers + everything a superseded one would have — which is what makes its + cancellation safe, and the property the comment in the file exists to + protect. `ci.yml` now holds the same property by classifying every push + as a change to everything (asserted in the newest-wins test above); + historically it classified by `before..sha`, and cancelling under that + regime cost two escaped failures (§6.3 finding 19). """ docs = (WORKFLOWS / "docs.yml").read_text(encoding="utf-8") assert "whole tree" in docs, (