Bust the Docker layer cache for the gate steps (closes #32) #37
Reference in New Issue
Block a user
Delete Branch "cibuild-cache-bust"
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?
Closes #32.
script/cibuildandscript/dockerwere baredocker buildinvocations with no cache control, and the
Dockerfilecopies the treebefore running its gates. On an unchanged tree Docker served those
layers from cache: the gates never executed and the build still exited
0.
What changed
Dockerfile—ARG CHECK_EPOCHdeclared in both stages,because
ARGis scoped per stage and this file has three gates acrosstwo of them. One declaration would have left the other stage silently
cacheable, which is the easiest way to ship a fix that does not fix
anything.
COPY . ., aboveRUN make fmt-checkandRUN make lintUSER builder, aboveRUN make checkEach gate
RUNreferences the value (echo "gate ..., epoch ${CHECK_EPOCH}" && make ...). BuildKit hashes the expanded command,not the declaration, so a declared-but-unreferenced
ARGinvalidatesnothing. The echo doubles as evidence in the build log that the layer
really executed, which is what let me verify the rest of this rather
than assert it.
Both declarations sit below the dependency layers on purpose. The
goal is to bust the gates, not to go cold — a build that recompiles the
world every invocation would be a different bug.
The build-stage
ARGis placed afterUSER builder, so the drop tothe unprivileged user still happens before
make check.script/cibuild,script/docker— both pass--build-arg CHECK_EPOCH="$(date +%s)". POSIXsh,set -eu, nobashisms; both verified with
dash -n.script/cibuild's headercomment asserted the guarantee it did not provide, so it now says why
the implication actually holds.
script/dockermatters as much: perthe issue's follow-up comment,
make dockeris the gate a reviewerruns by hand and it is the one that actually fooled someone on PR #31.
Verification
All runs on an unchanged tree with
BUILDKIT_PROGRESS=plain, cachepre-warmed.
1.
script/cibuildtwice in a row — gates ran both timesgate fmt-checkgate lintgate check17862587441786258823Distinct epochs, three gate layers executed each time. No sub-second
run anywhere.
2. Dependency layers still cached
Twelve steps served
CACHEDin the steady state (run 2 above, and bothscript/dockerruns below):The pinned base images resolve from the local store on top of that.
Only the three gates and the
RUN make builddownstream of them gocold, which is the intended blast radius.
One honest wrinkle: the first run after editing the
Dockerfileshowed9
CACHEDrather than 12, because theDockerfileis itself part ofthe build context and BuildKit re-materialised three lint-stage layers.
Every subsequent run on a stable tree settled at 12. That is cache
churn from editing the file, not from
CHECK_EPOCH.3.
script/dockertwice in a rowgate fmt-checkgate lintgate checkCACHED4. The lint stage still gates the build stage
Not asserted — tested. I planted a deliberate
unusedfinding in thetree and ran
script/cibuild:Exit 1 after 36.1s, and
grep -c "gate check, epoch"over the wholebuild log returned 0 — the build-stage
make checknever started.The
COPY --from=lintatDockerfile:29still forces the lint stageto complete first. Worth noting explicitly: that
COPYstep itselfshows as
CACHED, because the copied binary is byte-identical everytime. The lint stage still runs — BuildKit has to build it to resolve
the copy. Probe file removed; it is not in the commit.
5. Non-root build stage preserved, and it is load-bearing
docker run --rm <builder stage> idreports:To confirm the
chmod(0)tests are genuinely exercised rather thanmerely present, I forced the same image to run them as root:
Root reads straight through the
chmod(0), so the test fails. Itpasses in the real build, which is only possible as
builder.6.
make checkGreen in 17.3s:
ok sneak.berlin/go/sfdupes 1.799s coverage: 88.3% of statements,0 issues. Run with a freshGOLANGCI_LINT_CACHEper#36, so no findings are attributable to stale worktree paths. Also
green inside the container on every build above (88.5% there).
Out of scope
Dockerfile/script/cibuildtemplates. I checked upstream first per the issue's closing note:
prompts#26 is still open and the canonicalDockerfilethere isunchanged, so there was nothing to re-vendor. This is the bespoke
local fix, shaped to match what #26 recommends.
addressed here, since it is outside this issue's scope:
The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2. I will file it as its own issue.Notes for the reviewer, since this PR is specifically about not
trusting a build that says it passed.
Reproduce the verification yourself
Do not take the tables in the description on faith — the whole point of
the issue is that a green can be counterfeit. Recipe:
Three
gate ..., epoch Nlines per run means all three gates executed;the epoch differing between the two logs means it was not a replay of
the same cached command. A
CACHEDcount near zero would mean I tradedone bug for a cold build, which the issue explicitly calls a failure.
The counterfactual, if you want it
To see the old behaviour and confirm the fix is actually doing
something, run the same two-run loop against
main. The second runshould return in well under a second with every layer
CACHEDand nogate output at all. That is the 17-layer cache hit PR #31's reviewer
hit.
Where I would attack this if I were reviewing
Four things I deliberately went looking for, so you can check my work
rather than repeat it:
ARGis per-stage. A single declarationleaves the other stage cacheable and the fix looks fine in a diff.
There are two declarations here,
Dockerfilelint stage and buildstage. Confirm both, and confirm both gates in the lint stage sit
below the declaration.
ARGdeclared but not referenced. BuildKit hashes the expandedcommand, so
ARG CHECK_EPOCHwith a plainRUN make checkunder itbusts nothing and still reads as a fix. Every gate
RUNhereinterpolates
${CHECK_EPOCH}.CACHEDcount, not just the gateoutput. If
go mod downloadandapk addare re-running, theARGs are too high in the file.builderat
make checktime. IfARGhad gone aboveUSER, nothing wouldvisibly break — but
TestScanHardlinkRunFailsTogetherand themain_test.gounreadable-file path would stop testing anything,because root reads through
chmod(0). I verified that as root thesuite genuinely fails, so a passing containerised run is itself
proof the drop still happens.
One thing I could not fully close
date +%shas one-second resolution. Two builds starting within thesame wall-clock second would share an epoch and the second could serve
the gates from cache. Not reachable in practice here — the build takes
50 to 80 seconds, so back-to-back invocations cannot collide — and I
kept
$(date +%s)because the issue's definition of done names thatexact form. If you would rather have the guarantee not depend on build
duration,
date +%s%Nor$(date +%s)-$$would close it, and I willmake that change on request rather than argue about it.
Also filed
#38, for the
gomodguarddeprecation warning that appears on everylint run. Noticed here, deliberately not fixed here.
Review: PASS
Independent review of head
964fc29against basemain(b8ebe5f).Every claim in the description was re-executed from a throwaway
worktree; nothing in any checkout was modified and nothing was
committed.
Environment note. A host-wide BuildKit cache prune happened
mid-review, unrelated to this PR. Every cached-step count below comes
from a steady state I re-established myself after the prune (build
once, then count
CACHEDon the next run on an unchanged tree), notfrom the author's numbers. Wall-clock times are therefore not
comparable to the description's and are reported only to show that no
run was a cache hit.
1. Do the gates execute every time — yes
script/cibuild, unchanged tree,BUILDKIT_PROGRESS=plain:CACHED17862602901786260463script/docker, same conditions:CACHED17862612951786261353All three gate steps executed with real wall time in all four runs, no
gate step reported
CACHEDin any of them, and every run carried adistinct epoch. No sub-second run anywhere.
2. Is the invalidation real — yes, confirmed by counterfactual
Held the value constant and built twice with
--build-arg CHECK_EPOCH=CONSTANT:CACHED, all three gateRUNs among them, zerogate ..., epochlines in the log, exit 0 in seconds.That is precisely the 17-layer cache hit recorded on #32. So the
invalidation comes from the interpolated value changing, not from
anything cosmetic, and the
ARGis genuinely referenced rather thanmerely declared. Nothing above the gates is invalidated — see 3.
3. Dependency layers still cached — yes; count is 13 here, not 12
Steady-state
CACHEDsteps, identical in run B and bothscript/dockerruns:
Both
go mod downloads,apk add,adduserand the source copies staycached; the pinned base images resolve from the local store. Only the
three gates and the
RUN make buildbelow them go cold. The blastradius is what the issue asks for; this is not a cold build.
The description and
TODO.md:59say twelve — the thirteenth is thelint stage's
WORKDIR /src. Cosmetic, and possibly BuildKit-versiondependent, but it is wrong in committed content. Non-blocking.
4. Lint stage still gates the build stage — reproduced
Planted a deliberate
unusedfunction in a copy of the tree and ranscript/cibuild: exit 1 after 24.0s, failing atRUN echo "gate lint, epoch ${CHECK_EPOCH}" && make lintwithfunc lintProbeScratch is unused (unused), andgrep -c 'gate check, epoch'over the entire build log returned 0 —the build-stage
make checknever started.The author's reasoning about
COPY --from=lintis correct, and Iverified both halves in a single build: that
COPYreportedCACHEDwhile both lint gates executed in the same run. The copy's key is
content-addressed on the copied binary, which is byte-identical every
time, so the copy result is reused; BuildKit still has to bring the
lint stage to its final state to resolve it, which is why a lint
failure aborts before compilation. Fail-fast is intact.
5. Non-root quirk — intact and load-bearing
--target builderimage reportsuid=1000(builder) gid=1000(builder) groups=1000(builder);ARGafterUSERdoes not reset the user, somake checkstill runs unprivileged.In that same image:
builder:ok sneak.berlin/go/sfdupesforTestScanHardlinkRunFailsTogether;--user 0:0:FAIL ... stats = {added:2 ... skipped:0}, want both hardlink paths skipped.Root reads straight through the
chmod(0), as claimed. The permissiontests are genuinely exercised, and only because the drop is still there.
6.
make checkGreen locally on the head commit with a fresh
GOLANGCI_LINT_CACHEper#36: 13.4s,
coverage: 88.5% of statements,0 issues, andgit statusclean afterwards (the gate modifies nothing). Localgolangci-lintis 2.12.2, matching the pin.7. Scope and hygiene
Dockerfile,TODO.md,script/cibuild,script/docker.No scope creep.
dash -nclean on both scripts;set -eupreserved; no bashisms.git show --check: no whitespace errors. No over-long added lines.(closes #32); body is wrapped andaccurate; no attribution trailers of any kind anywhere in the diff or
the message.
964fc29(check / check (push), 1m50s).main, no conflicts.TODO.mdentry is in the right place and accurate apart from thecount in 3.
Non-blocking findings
Dockerfile:24,Dockerfile:64— an unsetCHECK_EPOCHdegradessilently. The
ARGhas no default and nothing asserts it isnon-empty. Measured on this branch: a bare
docker build .run twicegives 17
CACHED, all three gatesCACHED, no gate output, exit 0 —the exact defect this PR removes, quietly restored, with the log
showing
gate check, epochand an empty value. Not a regression(
docker build .was equally broken before) and both sanctionedentrypoints pass the arg, so the DoD is met. But
REPO_POLICIES.mditself describes CI as "runs
docker build .on push", so the rawcommand is a plausible thing for a human to type. Cheap hardening for
a follow-up: have the first gate assert the value, e.g.
RUN test -n "${CHECK_EPOCH}" || { echo "CHECK_EPOCH unset; use script/docker or script/cibuild" >&2; exit 1; },so the failure is loud instead of a cached green.
Second-resolution epoch — agreed non-blocking, but for a different
reason than the one given. "Builds take 50 to 80 seconds" is an
argument about one process, and it does not cover two concurrent
Gitea Actions jobs sharing a runner — pushing a branch and its merge
commit together is exactly the byte-identical-tree case #32
documents, and those two jobs can plausibly reach
date +%sin thesame second. What makes it acceptable is narrower: a collision needs
the same second and an identical tree, and in that case the reused
result was genuinely computed for that exact tree, so it cannot
certify a tree nobody checked. The residual cost is that one of the
two runs proves nothing on its own. If it is ever tightened, prefer
"$(date +%s)-$$"overdate +%s%N:%Nis a GNU extension, and onmacOS/BSD
dateit emits a literal trailingN, i.e. it wouldsilently stay at one-second resolution on developer machines — a
fix that looks like a fix.
TODO.md:59says twelveCACHEDsteps; measured thirteen here.Noted, not a finding: the
gomodguarddeprecation warning appears onevery lint run, is correctly out of scope here, and is already filed
as #38.
No blocking defects. The gate does what it claims, fails when it should,
and keeps the dependency cache.
Manager note — merged as
a102b8f.maingreen,0 issues., 88.3% coverage,origincarries onlymain.The decisive piece of this review was the counterfactual. Rather than only checking that the gates now run, the reviewer built with
--build-arg CHECK_EPOCH=CONSTANTand watched the second build produce 17CACHEDsteps with all three gateRUNs cached, zero gate output, exit 0 — reproducing precisely the false green #32 was filed for. That establishes the fix is load-bearing rather than incidental, which counting successful runs alone could never show.It also verified the two things most likely to have been quietly broken: the lint stage still gates the build stage (planted an
unusedfinding; build failed at the lint gate in 24s andgrep -c 'gate check, epoch'over the full log returned 0, so the build-stagemake checknever started), and the non-root drop is still load-bearing (TestScanHardlinkRunFailsTogetherpasses asbuilderand fails as--user 0:0withskipped:0, because root reads throughchmod(0)). That second one is the repo quirk we were told to preserve, and it is now demonstrated rather than assumed.Worth recording how the review handled the environment. A host-wide
docker builder prune -afon another repo destroyed roughly 41 GB of shared BuildKit cache mid-review. I warned the reviewer that its steady-state cached-step measurement had become meaningless and that a cold-cache test timeout would look like a bug. It re-established its own baseline — build once, count on the next run — and reported 13 cached steps rather than repeating the author's 12, correctly identifying the extra as the lint stage'sWORKDIR /src. Measurements taken pre- and post-prune are labelled as such in its comment. Without that, this would very likely have been a false FAIL on a PR that is correct.Non-blocking findings, disposed:
CHECK_EPOCHhas no default and nothing asserts it is non-empty, so a baredocker build .silently restores the cached-gate behaviour — 17CACHED, exit 0. Filed as #39. Not a regression, but one habit away from the failure mode this PR exists to prevent.date +%s%Nis GNU-only and degrades silently on macOS.TODO.mdtwelve-versus-thirteen correction, also on #39.gomodguarddeprecation is #26, not #38 — #38 was the third independent rediscovery of it and is closed as a duplicate. #26 is assigned tosneak, since the fix belongs in the canonical config upstream.With this and #24 landed, two of the three ways this repo's gates could lie are closed. #36 (linter cache attributing findings to deleted worktrees) is the remaining one, and it produces noise rather than false confidence.