internal/handlers runs at 22s against script/test's 30s per-package timeout, and has already reded a build under load #194
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Milestoned 1.0.0, on the same reasoning #186 was milestoned: a test budget that can red a package when nothing is wrong makes the gate untrustworthy from the other direction, and
nextis required to stay green and mergeable tomainwithout notice. This repo has spent the entire cycle making the gate mean what it says — #119, #152, #109, #186 — and tagging 1.0 with a known, already-breached margin contradicts all of it.This has been observed, not theorised. During the rebase of #182 a cache-defeated
docker build --no-cache-filter=lint --no-cache-filter=builder .failed:internal/handlershitscript/test's-timeout 30sper-package budget, withTestFailedLogin_LogLineDoesNotTrackUsernameSize/{json,text}/astraltaking 16s each, at host load average 46 on 48 cores. A re-run of the same head passed at 21.940s.Measured, by the round-3 review of #182
Cache-defeated builder stage at ambient load 12-18/48: 17.415 s, 17.948 s, 16.488 s against the 30 s budget.
GOMAXPROCSsweep, plainnext(563e834) versus that PR's head (65148ab), as a proxy for effective cores:GOMAXPROCSnextThe key finding:
nextalone already times out atGOMAXPROCS=4, before 182 landed. This is not a problem that any one PR introduced. 182 moves the cliff from 4 effective cores to 5, at a flat +4-5 s attributable to its non-parallelTestFlood_NoWriterGrowsWithTheInput(5.06 s / 5.67 s measured), taking budget consumption from 40 % to 57 %. #189 will consume more.Spurious red is unlikely at ambient load and likely below roughly 6 effective cores. Since the box is shared and routinely loaded, "unlikely" is not the same as "won't happen" — and it already did once.
Preferred fix, and why
Raise the per-package timeout in
script/test. One file, conflicts with nothing in flight, and 30 s was never a considered figure — it is a default the suite outgrew. The 182 reviewer suggests 120 s; that is a reasonable starting point but justify whatever you pick against the measurements above rather than adopting it because it was suggested.Do not move tests between packages in this issue. Splitting
internal/handlerswould collide with work that touches it. If you conclude a split is genuinely right, say so here and stop rather than doing it.Also state a view on whether a single per-package
-timeoutis the right instrument at all, givengo testapplies it per package and this suite's packages are very unevenly sized.Definition of done
script/test's per-package timeout is raised, with the chosen value justified in a comment against measured durations rather than asserted.GOMAXPROCS=4cache-defeated builder run ofinternal/handlerspasses, since that is wherenextcurrently fails. Report the duration.Implementation requirements
next, PR based onnext, single commit, title ending(closes #N).TODO.md(see #112).make bootstrapin a fresh clone before gating. Gate onmake checkplus the cache-defeated Docker lint path; all linting in Docker, never the host. Clean up every container and image; never run any prune.#195 —
script/test's-timeout 30sbecomes-timeout 180s, one line plus the justification as a comment in the script. No test touched, nothing moved between packages.Chose 180 s, not the suggested 120 s. Re-measured on the final tree in a cache-defeated builder-equivalent stage;
internal/handlersatGOMAXPROCS48/16/8/6/5/4/3/2/1 was 16.961 / 20.107 / 27.660 / 30.575 / 32.940 / 39.666 / 49.361 / 67.320 / 97.510 s. 120 s is only 1.23x that worst figure — a number the suite sits under, not a margin. 180 s is 1.85x it.The old budget really fails: plain
nextat33e4fa4, unmodified,GOMAXPROCS=4→FAIL internal/handlers 30.165s,panic: test timed out after 30s. Same run on this branch:ok internal/handlers 41.262s.Under real load (36 spinners, load 31-73/48), three cache-defeated runs: 45.881 / 47.274 / 49.000 s — all three would have breached 30 s. Compound
GOMAXPROCS=4plus load 52-68: 67.270 s. Spinners were killed anddocker ps -averified empty before the proof and gate runs.No new cliff: 180 s is not breached anywhere in the sweep, down to the instrument's floor at
GOMAXPROCS=1(54 % of budget).Slowest package:
internal/handlers, in all 16 measured runs — 16.689 s inmake check, 17.144 s in the gate build, 49.000 s at its worst under load. Second place isinternal/ciscript(7.1-7.2 s) with cores to spare,internal/delivery(up to 18.306 s) without; the gap is 4-5x either way.Verified with
make check(GOFLAGS=-count=1, lint in Docker,0 issues.) anddocker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain ., both rc=0 with the lint and builder stages shown executing rather than replaying. CI green on7f27362.Two things for you in the PR body: my view on whether a single per-package
-timeoutis the right instrument (it is the wrong shape but the right choice today), and a deliberate divergence fromREPO_POLICIES.md, which still mandates 30 s.