From 63c5545979a52a7b983344796e2301af4e3313bb Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 14:12:47 -0400 Subject: [PATCH] =?UTF-8?q?review=20round=201:=20anchor=20the=20ceilings?= =?UTF-8?q?=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.