script/cibuild can report a cached green: docker build serves RUN make check layers from cache
#54
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?
Repo-local counterpart to a fleet-wide finding. The same defect was observed concretely in another repo: a
script/cibuildreporting SUCCESS in 0.262 seconds with every layerCACHED, and 64.3s with a genuine pass when forced uncached.The defect
script/cibuildis a plaindocker build --ulimit memlock=-1:-1 .with no cache control. TheDockerfiledoesCOPY . .at line 9 and line 27, each followed by the check steps —RUN make fmt-checkandRUN make lintin the lint stage,RUN make testandRUN make buildin the builder stage.Docker keys those
RUNlayers on the hash of everything before them. On a byte-identical tree theCOPYlayers hit cache, so every check layer hits cache too: the suite never executes, the linter never runs, and the build exits 0. CI reports green having verified nothing.The trigger is any re-run on an unchanged tree: re-running CI on the same commit, a manual re-trigger after an infrastructure blip, a runner with a warm layer cache picking up a commit it has already built, or anyone running
script/cibuildtwice locally.Why this repo is worse than average
It compounds with #32.
script/testcurrently ends withgo test ./... || go test -v ./..., so the verbose rerun's exit status becomes the script's and a test that fails then passes on retry also produces a green.Stacked, the two mean "CI is green" in this repo has been carrying very little information: one path skips execution entirely, the other swallows a real failure. Both need closing, and closing only one leaves a hole.
Not a live incident, for the record
I re-verified PR #29 against this and its green is genuine — CI wall-clock of 2m0s on the head and 1m8s on the pre-rework commit, tree deltas of 60 and 10 files that invalidate
COPY . ., and a reviewer's targeted negative control that produced a specific predicted failure inside the container, which a cached layer cannot do. So this is a latent hazard to close, not a merge to retract. Detail on PR #29.Definition of done
script/cibuildon a byte-identical tree executes the checks again rather than serving them from cache, and demonstrably so.go mod downloadplus base-image re-pull — that is what makes people disable it.ARG CHECK_EPOCHimmediately above the first check step, and havescript/cibuildpass--build-arg CHECK_EPOCH="$(date +%s)". Everything above theARGkeeps caching normally; everything from it down re-runs.ARGmust sit afterCOPY go.mod go.sumandRUN go mod download, and before theCOPY . ./ check sequence. Too early and dependency caching is destroyed; too late and the check layers still cache.Dockerfilehas two stages with check steps — lint (make fmt-check,make lint) and builder (make test,make build).ARGis scoped per stage, so both stages need their own declaration, and--build-argmust reach both. Fixing only the builder stage leaves lint silently cached, which is exactly the kind of half-fix that reads as done.script/cibuildtwice in a row on an unchanged tree and show that the second run still executes the suite. Report both wall-clocks and theCACHEDlayer counts. A build that drops from minutes to under a second is the signature to rule out.--no-cachebuild.TODO.mdupdated in the same commit.Implementation requirements
--no-cache. It works and it is the wrong fix: it discards the base images and the module download too, turning a 2-minute build into a many-minute one for no benefit. The point is to invalidate exactly the check layers and nothing above them.--ulimit memlock=-1:-1. The suite needs it for the memguard test; removing it breaks the build in a way that looks unrelated.script/cibuildmust stay POSIX sh withset -euand the repo-rootcdidiom.Dockerfile.make testdoes, and the verification for this issue involves running it. If #52 lands the build-tag split, make sure the second CI job gets the same treatment, or it will develop the identical cached-green problem on day one..gitea/workflows/check.ymldoes anything that would defeat or duplicate this, such as its own caching layer.Reproduced first-hand in this repo. Two consecutive
script/cibuildruns on a byte-identical tree (the merge-ready head397011a, nothing touched between them):CACHEDlayersRun 2's check layers, verbatim from the build output:
All four check layers cached. Exit 0. Nothing verified. One second, and
script/cibuildreports success.Run 1, by contrast, genuinely executed — real per-package results with real timings:
So the signature to watch for is exactly as reported fleet-wide: a build that drops from over a minute to about a second, with the
CACHEDcount jumping from ~1 to ~19. The 78s-to-1s ratio here is the same phenomenon as the 0.262s false green observed elsewhere.Two things this pins down for whoever implements the fix:
Both stages are affected, confirming the two-
ARGrequirement in the definition of done.make fmt-checkandmake lintcached in the lint stage,make testandmake buildcached in the builder stage. A fix that addsARG CHECK_EPOCHto only one stage leaves the other silently cached and still reports green — and it would look correct in review.The verification target is now concrete. After the fix, run 2 on an unchanged tree must show the four check layers executing rather than
CACHED, while the base-image andgo mod downloadlayers stay cached. Practically: run 2 should land somewhere well under 78s but clearly above 1s. If it comes back at 1s the fix did not work; if it comes back near 78s theARGwas placed too high and dependency caching was destroyed. Both failure modes are visible in the wall-clock alone, so record it.For the record on the timing side, relevant to #52: this run had
internal/cliat 12.388s and the whole suite around 16.6s, without-race. That is a different machine and load than the 18.7s measured in #52, and it lands on the same conclusion from a second direction — the suite is already sitting against the 20-second policy ceiling before the race detector is switched on at all.