Merge pull request #195 from levineuwirth/ci-timeouts-concurrency

ci: put a timeout on every job, cancel superseded PR runs, lint the protocol

.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

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

docs/active-work.md

@@ -953,6 +953,92 @@ has **no branch and no framing yet**.
   at 42 insertions against 88 deletions — merging it would *revert*
   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`.
+
 ## Parked lane: kill-ring browser + persistence
 
 - Portable branch: `githubsucks/kill-ring-browser`