diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3c1bc59..b2bb834 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,10 +9,63 @@ env: CARGO_TERM_COLOR: always RUSTFLAGS: "-D warnings" +# Cancel a pull request's superseded runs instead of letting them burn +# to completion. This project rebases heavily — the ledger re-conflicts +# on nearly every merge — so a branch routinely takes several pushes +# while an earlier run is still going, and each of those runs is +# obsolete the moment the next push lands. macOS minutes are the +# expensive ones and the macOS leg is the critical path, so superseded +# runs are exactly where the waste concentrates. +# +# Scoped to pull requests deliberately. `github.event.pull_request.number` +# is empty for a push to `main`, so the fallback keys those runs by SHA: +# every `main` commit gets its own group and none can cancel another. +# Cancelling a `main` run would leave the branch-protection record +# ambiguous about a commit that has already landed — the one place this +# saving is not worth having. +concurrency: + group: ci-${{ github.event.pull_request.number || github.sha }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +# Every job carries `timeout-minutes`. Without one a job inherits +# GitHub's 360-minute default, so a single hung test burns six hours — +# times four on the test matrix — and reports nothing useful at the end +# of it. +# +# The ceilings are justified against OBSERVED EXECUTION, and the +# numbers are a reading rather than a constant, so re-measure before +# trusting them: +# +# * observed max, 25-run window: 17 min (macOS/luajit) +# * observed max, 12-run window: 15.8 min (same job) +# * every other job: under 4 min +# +# `timeout-minutes` counts EXECUTION, not queue time — a 33-minute +# wall-clock run in that window executed its longest job in 17 — so no +# run in the observed history would have been killed by these values. +# +# The exposure is the case the window does NOT contain: a COLD CACHE. +# A stable-toolchain bump invalidates Swatinem's key on every leg at +# once, and a cold macOS debug build of this workspace plus the suite is +# the plausible way a HEALTHY run exceeds its ceiling. The test job +# therefore gets 35 rather than 25 — roughly 2x its observed max — while +# everything else keeps 25 against a sub-4-minute observed max, except +# `m6-perf-gates`, which keeps its own tighter 15. +# +# DIAGNOSIS, WRITTEN BEFORE IT HAPPENS: four test legs timing out +# simultaneously, shortly after a Rust release, is a cold cache and not +# a hang. Rerun, or raise this number. A single leg timing out while its +# siblings pass is the hang case these ceilings exist to catch. +# +# This is also the gate that has to exist before the basedpyright-class +# hang can ever be armed — see `PMACS_REQUIRE_PYRIGHT`, deliberately +# never set, in the test job below. 35 still beats the 360-minute +# default by an order of magnitude. jobs: fmt: name: Format runs-on: ubuntu-latest + timeout-minutes: 25 steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable @@ -23,6 +76,7 @@ jobs: clippy: name: Lint (${{ matrix.lua }}) runs-on: ubuntu-latest + timeout-minutes: 25 strategy: fail-fast: false matrix: @@ -40,10 +94,17 @@ jobs: # not pmacs), so the root-package clippy above never lints it. Lint # it explicitly or its warnings slip through CI (audit F-001). - run: cargo clippy -p pmacs-gpu --all-targets -- -D warnings + # pmacs-protocol is likewise never linted by the root-package + # clippy above: the workspace default member is only `pmacs`. The + # local `--workspace` gate covers it, so it passes today — CI has + # simply never checked, and a warning introduced through a + # protocol-only PR would reach `main` unseen. + - run: cargo clippy -p pmacs-protocol --all-targets -- -D warnings gpu-render: name: GPU Render (headless) runs-on: ubuntu-latest + timeout-minutes: 25 steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable @@ -76,6 +137,12 @@ jobs: test: name: Test (${{ matrix.os }} / ${{ matrix.lua }}) runs-on: ${{ matrix.os }} + # 35, not 25: this is the only job whose observed max is minutes + # rather than seconds, and the only one a cold cache can plausibly + # push past a 25-minute ceiling on all four legs at once. See the + # note above `jobs:` for the measurements and the cold-cache + # diagnosis. + timeout-minutes: 35 strategy: fail-fast: false matrix: @@ -160,6 +227,7 @@ jobs: acceptance: name: M1 Acceptance Gates runs-on: ubuntu-latest + timeout-minutes: 25 steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable @@ -170,6 +238,7 @@ jobs: m4-perf-gates: name: M4 Perf Gates runs-on: ubuntu-latest + timeout-minutes: 25 steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable @@ -188,6 +257,7 @@ jobs: m5-perf-gates: name: M5 Perf Gates runs-on: ubuntu-latest + timeout-minutes: 25 steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable diff --git a/TEST_IMPROVEMENT.md b/TEST_IMPROVEMENT.md index cd8e8a1..3c332e9 100644 --- a/TEST_IMPROVEMENT.md +++ b/TEST_IMPROVEMENT.md @@ -42,6 +42,9 @@ hand before inclusion. on `push:main` + `pull_request` only. No coverage measurement, no scheduled runs, no branch protection on `main` (verified via API: 404, so every job is advisory). + **~~No branch protection~~ — CLOSED. Protection was enabled during + the arc; the API now reports 12 required contexts, `strict` off, + `enforce_admins` off. The 404 above was a reading at audit time.** The suite is unusually thoughtful in places — the daemon harness's connect-based readiness probe, the `PMACS_REQUIRE_GPU` hard-fail @@ -358,9 +361,12 @@ test) pass in CI and flake for whoever runs the documented local gate. (Findings that change what CI *certifies*; speedups are §6.) -1. **Branch protection is off** — every job is advisory; a red run - merges as easily as a green one. Turn on required checks for the - cheap deterministic jobs at minimum (fmt, clippy, ubuntu test legs). +1. ~~**Branch protection is off**~~ — **DONE.** Every job was + advisory; a red run merged as easily as a green one. All 12 contexts + are now required, rather than the cheap-jobs-only starter suggested + here. `strict` is off (a PR need not rebase every time `main` moves, + which this repository's ledger contention makes expensive) and + `enforce_admins` is off (the maintainer retains an override). 2. **No job timeouts except m6** (15 min). Everything else inherits 360 min. The day a runner image ships any of the PATH-gated tools (§1.2), the basedpyright-class hang burns 6 h × 4 matrix legs with diff --git a/docs/active-work.md b/docs/active-work.md index a195d9f..5b95e5a 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -954,12 +954,98 @@ has **no branch and no framing yet**. current documentation. The section said "whoever confirms the branch carries nothing unique removes the section"; this is that. +## Test-improvement arc, lane 3a — CI timeouts and concurrency + +- Portable branch: `githubsucks/ci-timeouts-concurrency`, worktree + `../pmacs-ci3`. Workflow only — **no product code, no tests changed.** +- **Base, measured at write time:** + + ``` + $ git log --oneline -1 githubsucks/main + b7bf2c6 Merge pull request #194 from levineuwirth/silent-skip-arming + ``` + +- Ships the three cheap, deterministic items of `TEST_IMPROVEMENT.md` + §5-6. The larger ones — nextest (§6.3), the serial/parallel split + (§6.2), the parallel canary leg (§5.6), the nightly cron (§5.5), and + the macOS matrix trim (§6.4) — are **deliberately not here**: each + changes what CI certifies or how it runs, and each wants its own + decision rather than riding a timeout patch. +- **`timeout-minutes` on every job (§5.2).** Measured before changing: + **7 of 8 jobs had none** and inherited GitHub's 360-minute default; + only `m6-perf-gates` had one (15). A single hung test therefore burnt + six hours, times four on the test matrix. + **This is the gate that must land before `PMACS_REQUIRE_PYRIGHT` can + ever be set** — lane 2 left basedpyright unarmed precisely because + this did not exist. +- **The ceilings are 25, and 35 for the test job — anchored on observed + execution, corrected in review.** Revision 1 cited "~14.6 min, ample + headroom", which was one reading quoted as a property. Re-measured + over two windows: **17 min** max over 25 runs and **15.8 min** over + 12, both macOS/luajit; every other job under 4 min. Against 17, a + flat 25 is ~1.5x, not "ample". + - `timeout-minutes` counts **execution, not queue** — a 33-minute + wall-clock run in that window executed its longest job in 17 — so + **no run in observed history would have been killed** by either + value. + - The real exposure is what the window does *not* contain: a **cold + cache**. A stable-toolchain bump invalidates Swatinem's key on + every leg simultaneously, and a cold macOS debug build plus suite + is the plausible way a *healthy* run overruns. It would present as + four legs timing out at once, the day after a Rust release. + - So the test job takes 35 (~2x its observed max) and the rest keep + 25 (~6x theirs), and **the diagnosis is written into the workflow + before the event**: simultaneous four-leg timeouts after a + toolchain release are a cold cache, not a hang; a single leg + timing out beside passing siblings is the hang case. +- **`concurrency` with `cancel-in-progress` (§6.1)**, scoped to pull + requests. `github.event.pull_request.number` is empty on a push to + `main`, so the fallback keys those by SHA and no `main` run can + cancel another — cancelling one would leave the branch-protection + record ambiguous about a commit that already landed. +- **`-p pmacs-protocol` clippy (§5.7).** Verified passing locally + *before* proposing it, so adding it cannot turn CI red on arrival. + The root-package clippy never covered it: the workspace default + member is only `pmacs`. +- **§5.1 branch protection is DONE, not deferred** — it belongs in + neither this lane's shipped list nor its deferrals, and review was + right that its absence from both was an omission. It was enabled + earlier in this session; verified against the API at review time: + + ``` + $ gh api repos/levineuwirth/pmacs/branches/main/protection + {"enforce_admins":false,"force_push":false,"required_checks":12,"strict":false} + ``` + + All 12 checks required; `strict` off deliberately, so a PR need not + rebase every time `main` moves (this repository's ledger contention + makes strict expensive); `enforce_admins` off so the user retains an + override. **This matters to the concurrency comment**, which + justifies exempting `main` pushes by appeal to "the + branch-protection record" — that record now exists, so the + justification is real rather than aspirational. +- **Required status checks are NAME-COUPLED to job names, and this + lane's own deferrals will break them.** A required context that no + longer exists does not fail — it leaves every PR pinned on + "Expected — waiting for status", indefinitely, which is + `main` becoming unmergeable by policy rather than by a red run. + Three deferrals above change job names or the matrix: the macOS trim + (§6.4) removes two contexts outright, and nextest (§6.3) or the + serial/parallel split (§6.2) rename or add them. + **Rule: any job rename, removal, or matrix change updates the + branch-protection required-checks list in the same motion.** Recorded + here because this is the entry that both enabled protection and named + the lanes that will invalidate it. +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-ci3 + -b ci-timeouts-concurrency githubsucks/ci-timeouts-concurrency`. + ## Test-improvement arc, lane 4 — process teardown stdin deadlock - Portable branch: `githubsucks/process-teardown-stdin-deadlock`, worktree `../pmacs-hang`. Implements - `docs/process-teardown-stdin-deadlock-framing.md` (rev 2, one review - round). + `docs/process-teardown-stdin-deadlock-framing.md` (rev 3: one review + round, then a CI round that falsified the reproduction). - **Base, measured rather than quoted:** ```