From 40c870de22b290f87a8a6f7d5ad616bcd9802e7c Mon Sep 17 00:00:00 2001 From: Shannon Atkinson Date: Fri, 14 Aug 2026 13:28:22 -0700 Subject: [PATCH 1/2] ci: a skipped matrix job creates no per-leg contexts, so #351 made docs PRs unmergeable #351 gated the acceptance matrix with a job-level `if:` on the reasoning that a skipped job still reports and a skip satisfies a required check. That is true of an ordinary job and false of a matrix one: a skipped matrix never expands, so the per-leg contexts are not skipped, they are never created. #349 is the proof. Fifteen checks green, zero failures, one entry in the checks list named literally `acceptance: ${{ matrix.suite }}`, and all twelve required `acceptance: acceptance-*` contexts reported MISSING. A documentation-only pull request -- the exact case #351 was built for -- could not be merged at all. The #351 PR body named this trap ("if a required check is skipped by a path filter, GitHub can leave a PR unmergeable forever") and then walked into it, because the mitigation it chose was the same bug in a different costume. The matrix now always expands and every leg reports. What is conditional is the work: each step carries the gate, a documentation-only run costs a runner allocation instead of a suite, and an explicit notice step says why a green check did nothing rather than leaving a reader to guess. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX --- .github/workflows/ci.yml | 52 ++++++++++++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f7a1f38..41b6a3f2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,12 +59,23 @@ jobs: # three-OS builds, Docker and browser suites -- to validate files no job reads. # One of them was a single file. See #350. # - # WHY A JOB-LEVEL `if:` AND NOT A WORKFLOW-LEVEL `paths:` FILTER. Twelve of the - # acceptance suites are REQUIRED status checks. A workflow-level filter stops - # the workflow running at all, so a required check never reports, and the pull - # request stays pending forever with no way to merge it. A skipped job still - # reports, and a skip satisfies the requirement. The naive fix is worse than - # the problem it solves. + # WHY THIS GATES STEPS AND NOT JOBS. Twelve of the acceptance suites are + # REQUIRED status checks. A workflow-level `paths:` filter stops the workflow + # running at all, so a required check never reports and the pull request stays + # pending forever with no way to merge it. + # + # A JOB-LEVEL `if:` ON A MATRIX IS THE SAME BUG WEARING A DISGUISE, and #351 + # shipped it. A skipped ordinary job does report, and a skip does satisfy the + # requirement -- but a skipped MATRIX job never expands its matrix, so the + # per-leg contexts are never created at all. The checks list showed one entry + # named literally `acceptance: ${{ matrix.suite }}`, the twelve required + # contexts were absent rather than skipped, and #349 -- a documentation-only + # pull request, the exact case this was built for -- was unmergeable. Fifteen + # green checks and no way in. + # + # So the matrix always expands and every leg reports. What is conditional is + # the WORK: each step carries the `if:`, the job costs a runner allocation and + # nothing else, and the required context reports success either way. # # IT FAILS TOWARD RUNNING. `code` is false only when EVERY changed path is # documentation; anything unrecognised makes it true. A new top-level directory @@ -1148,10 +1159,13 @@ jobs: acceptance: name: "acceptance: ${{ matrix.suite }}" runs-on: ubuntu-latest - # Skipped, not filtered out: these are required checks and a skip satisfies - # them where an absent report does not. See the changes job. + # NO JOB-LEVEL `if:` HERE, DELIBERATELY, and #351 is why. All twelve of + # these are required checks. A skipped matrix job never expands its matrix, + # so the per-leg contexts are not skipped -- they do not exist, and the + # pull request can never satisfy branch protection. The `if:` is on every + # step below instead: the matrix expands, all twelve report, and a + # documentation-only run costs a runner allocation rather than a suite. needs: changes - if: needs.changes.outputs.code == 'true' env: # See the deadline note on the suite step below. The library's 900s # default is for a laptop, where nothing outranks it; here it has to fire @@ -1190,18 +1204,32 @@ jobs: # apt step of its own -- Docker is already present on ubuntu-latest. - acceptance-mqtt steps: + # EVERY STEP CARRIES THE GATE. There is no way to end a job early from a + # step without failing it, so "documentation only" has to be spelled out + # on each one rather than checked once at the top. - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + if: needs.changes.outputs.code == 'true' - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 + if: needs.changes.outputs.code == 'true' with: go-version-file: go.mod cache: true - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7 + if: needs.changes.outputs.code == 'true' with: node-version: 24 cache: npm cache-dependency-path: ui/package-lock.json + # Says out loud why a green check did no work, so a reader of the checks + # list is never left guessing whether the suite ran or silently no-opped. + - name: Documentation-only change, so this suite did not run + if: needs.changes.outputs.code != 'true' + run: | + echo "::notice title=${{ matrix.suite }} skipped::every changed path was documentation, so this suite could not be affected by it. See the 'which changes' job." + - name: Install FFmpeg + if: needs.changes.outputs.code == 'true' # See the note on the go job's install step: a hung apt must fail with its # own name attached rather than eating this job's whole 20 minutes. timeout-minutes: 6 @@ -1235,6 +1263,7 @@ jobs: # The suites run the real binary with the UI embedded, so it has to be # the real build rather than `go build ./cmd/...`. - run: make build + if: needs.changes.outputs.code == 'true' # THE THREE DEADLINES, AND WHY THEY ARE IN THIS ORDER. # @@ -1268,6 +1297,7 @@ jobs: # watchdog that itself wedges, and it still leaves the always() upload # below its chance to run, which a job-level cancel does not. - name: ./scripts/${{ matrix.suite }}.sh + if: needs.changes.outputs.code == 'true' timeout-minutes: 12 run: ./scripts/${{ matrix.suite }}.sh @@ -1366,7 +1396,9 @@ jobs: # for the next occurrence. Only that suite writes one; the others match # nothing here and if-no-files-found already covers it. - name: Upload logs - if: always() + # always(), but not when the suite never ran: a documentation-only job + # has no artifact to collect, only a warning about its absence. + if: always() && needs.changes.outputs.code == 'true' uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: ${{ matrix.suite }}-logs From cb62d494bff608503e52bc8bd6a0e0930700318f Mon Sep 17 00:00:00 2001 From: Shannon Atkinson Date: Fri, 14 Aug 2026 13:57:11 -0700 Subject: [PATCH 2/2] ci: the coverage guard could not fire before the ceiling above it, and today it hung for 24 minutes `go build, vet, test` on #357 sat 24 minutes in "internal/api coverage measures the tests, not the preflight" against a step measured at 98-114s across the last five runs on main, until the job's own timeout-minutes: 25 cancelled it and named nothing. TWO GAPS, AND THE SECOND IS BROKEN ARITHMETIC WITH OR WITHOUT A HANG. The probes carried no -timeout, so Go's default of TEN MINUTES PER PROBE applied to three of them: 30 minutes of worst case behind a job ceiling of 25. The guard could never have reported; it could only ever have been cancelled. This is the error the acceptance job already documents about its watchdog, where the arithmetic "quietly assumes the suite starts when the job does". And the step was the one exception to a discipline this file states out loud at :340 -- "THE FOUR SCRIPT STEPS IN THIS JOB CARRY STEP TIMEOUTS ... a job timeout names nothing, a step timeout names the step". Four got one. The fifth is the one that hung. WHY GO'S TIMEOUT CANNOT BE THE BACKSTOP HERE, though it is still worth having. The guard captures each probe with `out="$(go test ...)"`, and command substitution blocks until the pipe has no writers left rather than until go test exits. A test leaking a child that holds inherited stdout hangs the capture after the binary is gone and after Go's timeout has already fired -- the mechanism the note at :360 already gives for the watchdog, one step below the step this happened to. So: -timeout 4m per probe (~3x the slowest real one, 12m worst case) and timeout-minutes: 14 on the step, above that 12 so Go still wins the race and prints the goroutine dump naming the test, and below the job's 25 either way. Verified: passes in 103s locally, matching CI's 98-114s. Mutating the flag to 1ms makes the first probe fail with `panic: test timed out after 1ms` naming TestLedgerPreflight, so the bound is wired to the probes rather than decorative. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX --- .github/workflows/ci.yml | 19 +++++++++++++++++++ scripts/coverage-instrument-guard.sh | 13 ++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 41b6a3f2..d01146cf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -303,7 +303,26 @@ jobs: # package under three different selections and requires the numbers to # differ. ~60s, dominated by the unfiltered probe, which is the number # anybody would actually quote. + # THE STEP TIMEOUT THE FOUR BELOW GOT AND THIS ONE DID NOT, and on + # 2026-08-14 this is the step that hung: 24 minutes against a measured + # 98-114s, until the job's own ceiling cancelled it and named nothing. + # + # It can hang for a reason the note at :360 already gives about the + # watchdog -- "a background process holding the suite's stdout ... does + # not merely fail to report, it becomes the hang". The guard captures each + # probe with `out="$(go test ...)"`, and command substitution blocks until + # the pipe has no writers left, NOT until go test exits. A test that + # leaks a child holding inherited stdout hangs the capture after the test + # binary is gone and after Go's own timeout has already fired, so Go's + # diagnostic cannot be the backstop here. Only this can. + # + # 14 rather than something tighter, so the ordering the file argues for + # everywhere else holds: three probes at -timeout 4m is 12m of worst case, + # and Go has to be able to win that race and print the goroutine dump + # before this fires. Below the job's 25 either way, which is the property + # being claimed. - name: internal/api coverage measures the tests, not the preflight + timeout-minutes: 14 run: make coverage-instrument-guard # -race because the engine reconciles from several goroutines and a data diff --git a/scripts/coverage-instrument-guard.sh b/scripts/coverage-instrument-guard.sh index 60353033..f8550a6b 100755 --- a/scripts/coverage-instrument-guard.sh +++ b/scripts/coverage-instrument-guard.sh @@ -43,7 +43,18 @@ pct() { local label="$1" shift local out - if ! out="$(go test -count=1 -covermode=set "$@" ./internal/api 2>&1)"; then + # -timeout, because the default is 10 MINUTES PER PROBE and there are three + # of them. 30 minutes of worst case sat behind ci.yml's timeout-minutes: 25, + # so this guard could not fire before the ceiling above it -- the job died + # saying "cancelled" and naming nothing. Measured at 0.9s + 0.8s + 57.2s + # locally and 98-114s for the whole step on ubuntu-latest, so 4m per probe + # is ~3x the slowest real one and 12m worst case, which fits under the step + # timeout ci.yml now carries. + # + # Go's own timeout is the one worth having: it panics with a goroutine dump + # naming the test that was running, where a step timeout only names the + # step. That is why it is set here and set BELOW the step's. + if ! out="$(go test -count=1 -timeout 4m -covermode=set "$@" ./internal/api 2>&1)"; then echo "coverage-instrument-guard: the probe '$label' did not pass:" >&2 echo "$out" >&2 exit 1