From 708054e8d50fa95e6b1a15eb430bafb118d157ad Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 13:05:56 -0400 Subject: [PATCH 1/3] ci: put a timeout on every job, cancel superseded PR runs, lint the protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lane 3a of the testing arc --- the three cheap, deterministic items of `TEST_IMPROVEMENT.md` §5-6. The larger ones (nextest, the serial/parallel split, a parallel canary leg, the nightly cron, the macOS matrix trim) are deliberately NOT here: each changes what CI certifies or how it runs, and each deserves its own decision rather than riding in on a timeout patch. `timeout-minutes` on every job (§5.2). Measured before changing rather than assumed: SEVEN of eight jobs had none and inherited GitHub's 360-minute default; only `m6-perf-gates` had one, at 15. So a single hung test burnt six hours --- times four on the test matrix --- and reported nothing useful at the end of it. Set to 25 against a measured ~14.6 min critical path (macOS/luajit), which leaves ample headroom for a slow runner while catching a hang in under half an hour. This is the gate that has to exist before `PMACS_REQUIRE_PYRIGHT` can ever be set. Lane 2 left basedpyright unarmed *because* this did not exist; the two decisions are the same decision, half a lane apart. `concurrency` with `cancel-in-progress` (§6.1), scoped to pull requests. This project rebases heavily --- the ledger re-conflicts on nearly every merge --- so branches take several pushes while earlier runs are still going, and macOS minutes are both the expensive ones and the critical path. Pushes to `main` are deliberately exempt: `github.event.pull_request.number` is empty there, so the fallback keys those runs by SHA and none can cancel another. Cancelling a `main` run would leave the branch-protection record ambiguous about a commit that has already landed, which is the one place the saving is not worth having. `-p pmacs-protocol` clippy (§5.7). The root-package clippy never covered it --- the workspace default member is only `pmacs` --- so a warning introduced through a protocol-only change would reach `main` unseen. Verified passing locally BEFORE proposing it, so it cannot turn CI red on arrival. The timeout rationale is stated once above the job list rather than copied onto each job: the first draft duplicated a seven-line comment across seven jobs, which is the same degraded-copy shape this arc keeps removing elsewhere. Verified: YAML parses; all eight jobs carry a timeout (seven at 25, m6-perf-gates keeping its tighter 15); `cargo fmt --all --check`, `clippy -p pmacs-protocol` and `clippy -p pmacs-gpu` all exit 0; `git diff --check` clean. The diff touches `ci.yml` and the ledger and nothing else, so no code gate is affected. --- .github/workflows/ci.yml | 41 ++++++++++++++++++++++++++++++++++++++++ docs/active-work.md | 39 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3c1bc59..4dedf45 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,10 +9,39 @@ 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 measured critical path is ~14.6 min (macOS/luajit), so 25 +# leaves ample headroom for a slow runner while catching a hang in +# under half an hour. `m6-perf-gates` keeps its tighter 15. +# +# 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. jobs: fmt: name: Format runs-on: ubuntu-latest + timeout-minutes: 25 steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable @@ -23,6 +52,7 @@ jobs: clippy: name: Lint (${{ matrix.lua }}) runs-on: ubuntu-latest + timeout-minutes: 25 strategy: fail-fast: false matrix: @@ -40,10 +70,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 +113,7 @@ jobs: test: name: Test (${{ matrix.os }} / ${{ matrix.lua }}) runs-on: ${{ matrix.os }} + timeout-minutes: 25 strategy: fail-fast: false matrix: @@ -160,6 +198,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 +209,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 +228,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/docs/active-work.md b/docs/active-work.md index dd0016a..3b283d4 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -820,6 +820,45 @@ 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. Set to 25 against a + measured ~14.6 min critical path (macOS/luajit). + **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. +- **`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`. +- 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` From 63c5545979a52a7b983344796e2301af4e3313bb Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 14:12:47 -0400 Subject: [PATCH 2/3] =?UTF-8?q?review=20round=201:=20anchor=20the=20ceilin?= =?UTF-8?q?gs=20on=20observed=20execution,=20record=20=C2=A75.1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 --- the ceiling was justified against the wrong number. Revision 1 cited "~14.6 min, ample headroom", which was one reading quoted as a property, and this ledger's own rule applies to it: a census is a reading, not a constant. Re-measured over two windows --- 17 min max over 25 runs, 15.8 over 12, both macOS/luajit, every other job under 4 --- so a flat 25 was about 1.5x the observed tail, not "ample". Two facts shape the fix. `timeout-minutes` counts EXECUTION, not queue, so the 33-minute wall-clock run in that window executed its longest job in 17 and no run in observed history would have been killed by either value. And the real exposure is the case no window contains: a cold cache. A stable-toolchain bump invalidates Swatinem's key on every leg at once, and a cold macOS debug build plus suite is the plausible way a HEALTHY run overruns --- presenting as four legs timing out simultaneously 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 these ceilings exist to catch. 35 still beats the 360-minute default by an order of magnitude, so the basedpyright arming this gate unblocks is unaffected. P2 --- §5.1 was missing from both lists, and review was right that the omission matters. But its premise had gone stale, which is worth recording rather than quietly working around: branch protection is ON. It was enabled earlier in this session, and I re-verified against the API rather than trusting either the review or my own memory of doing it: {"enforce_admins":false,"force_push":false, "required_checks":12,"strict":false} Recorded in the ledger as DONE with the settings and the reasoning for each --- `strict` off so a PR need not rebase every time `main` moves, `enforce_admins` off so the user keeps an override. This also settles the concurrency comment, which justifies exempting `main` pushes by appeal to "the branch-protection record": that record exists, so the justification is real rather than aspirational, and no softening is needed. P3 --- the double blank line before the parked lane, third PR running. Fixed, and added to the ledger's own update protocol as step 6, since fixing the instance three times has not stopped it: a block ending in a blank line inserted above a heading already preceded by one leaves the seam, and it survives review by sitting beneath the level anyone reads at. The rule now names the check. Verified: YAML parses; ceilings are 25 except test at 35 and m6-perf-gates at its tighter 15; the seam check finds no double blanks anywhere in the ledger; `git diff --check` clean. Workflow and ledger only. --- .github/workflows/ci.yml | 38 +++++++++++++++++++++++++++---- docs/active-work.md | 49 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4dedf45..4ab12a4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,13 +30,36 @@ concurrency: # 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 measured critical path is ~14.6 min (macOS/luajit), so 25 -# leaves ample headroom for a slow runner while catching a hang in -# under half an hour. `m6-perf-gates` keeps its tighter 15. +# 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. +# +# 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. +# never set, in the test job below. 35 still beats the 360-minute +# default by an order of magnitude. jobs: fmt: name: Format @@ -113,7 +136,12 @@ jobs: test: name: Test (${{ matrix.os }} / ${{ matrix.lua }}) runs-on: ${{ matrix.os }} - timeout-minutes: 25 + # 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: diff --git a/docs/active-work.md b/docs/active-work.md index 3b283d4..8245bf6 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -840,11 +840,30 @@ has **no branch and no framing yet**. - **`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. Set to 25 against a - measured ~14.6 min critical path (macOS/luajit). + 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 @@ -854,11 +873,27 @@ has **no branch and no framing yet**. *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. - 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` @@ -1166,4 +1201,10 @@ Whenever a listed lane changes materially: 3. keep durable architecture in `docs/agent-handoff.md`, not here; 4. remove the lane after merge or abandonment; 5. verify every recovery command from a clean worktree before calling - the transfer complete. + the transfer complete; +6. **read the seam back after inserting or removing a lane.** A block + that ends in a blank line, inserted above a heading already preceded + by one, leaves a double blank — three consecutive PRs shipped that + and each was caught in review rather than before it. It survives by + being beneath the level anyone reads at. `grep -n -B2 '^## '` over + the file, or just look at the two lines above the next heading. From 2554bfcbb9e8158db8b3989b07ff0afb61f54b10 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 14:16:27 -0400 Subject: [PATCH 3/3] review round 2: arm the required-checks name-coupling trap, restore m6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P3 --- required status checks are name-coupled to job names, and a required context that no longer exists does NOT fail. It leaves every PR pinned on "Expected --- waiting for status" forever, which is `main` becoming unmergeable by policy rather than by a red run. Three of this lane's own deferrals will do exactly that: the macOS matrix trim removes two contexts outright, and nextest or the serial/parallel split rename or add jobs. The rule is now in the ledger entry --- any job rename, removal, or matrix change updates the branch-protection required-checks list in the same motion --- and it is recorded HERE deliberately, because this is the single entry that both enabled protection and named the lanes that will invalidate it. Arming the warning anywhere else would separate the trap from the thing that sets it. P4 --- the rewritten top comment said "everything else keeps 25 against a sub-4-minute observed max" and dropped the clause noting that `m6-perf-gates` keeps its own tighter 15. Restored. Worth the fixup in a change whose entire subject was comments matching reality. Beyond the PR, and taken here rather than deferred: `TEST_IMPROVEMENT.md` on `main` still said "no branch protection on `main` (verified via API: 404, so every job is advisory)" and listed §5.1 as open. Both went stale during this session, and THIS lane is what made them stale, so it carries the correction rather than leaving it for whoever touches the file next. Struck through in both places rather than rewritten: the 404 was a true reading at audit time, and the document is the arc's scoping record, so what changed is more useful than a clean-looking present tense. Note also that protection shipped wider than §5.1 proposed --- all 12 contexts required, not the cheap-jobs-only starter --- which the correction states. Verified: YAML parses; the seam check from update-protocol rule 6 finds no double blanks; `git diff --check` clean. --- .github/workflows/ci.yml | 3 ++- TEST_IMPROVEMENT.md | 12 +++++++++--- docs/active-work.md | 12 ++++++++++++ 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ab12a4..b2bb834 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,7 +49,8 @@ concurrency: # 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. +# 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 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 8245bf6..5527ba7 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -890,6 +890,18 @@ has **no branch and no framing yet**. 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`.