ci: re-run make check on every cibuild instead of serving it from the layer cache (closes #115) #122
Reference in New Issue
Block a user
Delete Branch "fix/115-cibuild-cache"
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 #115.
script/cibuildwas plaindocker build .. The Dockerfile doesCOPY . .and thenRUN make check, and Docker invalidatesCOPY . .only on a content change, so on a byte-identical tree the check layer was reused and the suite never ran. The script's header comment claimed a successful build implies all checks pass — false whenever the cache was warm.Reproduced on this branch's parent (
9347a28) before changing anything: a second consecutivescript/cibuildreturned exit 0 in 283 ms, with#13 [builder 9/10] RUN make checkreportedCACHEDand every other layerCACHEDtoo.Fix
ARG CHECK_EPOCHimmediately above the check step, expanded into the command;script/cibuildpasses a fresh$(date +%s%N)per invocation.Why the value is expanded inside the
RUN. A build argument's value participates in the cache key of later instructions in the same stage even when those instructions never reference it — this was tested directly on this builder (Docker 29.7.2 / buildx 0.36.1 / BuildKit v0.32.2) by reverting to a bareRUN make checkwithARG CHECK_EPOCHdeclared but unused: changing only--build-argre-executed the layer, and repeating a previously-used value came backCACHED, proving the cache was live and the miss genuine. So a fresh value busts this layer either way. Expanding it into the command is nonetheless the deliberate choice: it makes the invalidation a property of the command string itself rather than of how a given builder version treats unreferenced args, and it surfaces the epoch in the build log as a diagnostic showing which run a layer belongs to.(An earlier revision of this PR asserted the opposite — that BuildKit keys on the expanded command string only, so a bare
ARGwould still hit the cache. That claim was empirically false on this builder and has been removed from theDockerfilecomment, the commit message and this description. The code was always correct; only the justification was wrong.)The epoch is nanosecond-granular so that two concurrent invocations starting within the same wall-clock second cannot produce an identical value and have the second served from the first's layer. Sequential runs could not collide (a build is ~45 s), but concurrent ones could.
Placing the
ARGhere and no earlier keeps the pinned toolchain installs andgo mod downloadabove the invalidation line. A plaindocker buildwithout the argument caches exactly as before; only the CI entrypoint changes behaviour.Verification — by experiment, not inspection
Per the refinement in the issue comment: timings and a negative control, neither alone.
1. Twice in a row on an unchanged tree
CACHEDRe-run after the
date +%s%Nchange, on an unchanged tree, both exit 0 and both executing the check step:1786256586666772611CACHED)1786256640154832161Distinct epochs, nothing sub-second, no
CACHEDon the check layer, andapk add, both pinnedgo installsteps,go mod download,COPY go.mod go.sumandCOPY . .all stillCACHED.2. Negative control (the conclusive evidence)
Planted
internal/config/zz_negative_control_test.go:script/cibuildfailed in 24.7 s with exit 1. The build printed exactly the predicted message:A cached layer cannot produce a failure predicted in advance, so the suite demonstrably ran. The file was then deleted,
git statusshowed only the intended modifications and no untracked leftovers, and the tree built green again in 48.1 s. An independent reviewer reproduced this with their own planted sentinel. Thedate +%s%Nchange does not alter the shape of the command string (the epoch is expanded by the builder, not the script), so the control was not re-run for it.3. Dependency layers stayed cached
In every post-fix run, all of these report
CACHED:apk add --no-cache git make gcc musl-dev binutils-gold, both pinnedgo installsteps (golangci-lint, goimports),WORKDIR /src,COPY go.mod go.sum ./,RUN go mod download, andCOPY . .. Only the check step and the steps after it re-run. No module re-download, no toolchain reinstall.4. Build time vs the ceiling
42-55 s against the policy's 5-minute ceiling — roughly 15-18% of budget.
5.
make checkand script sanityGreen, exit 0,
0 issues.(run with an isolatedGOLANGCI_LINT_CACHEandGOFLAGS=-count=1; not void — noparallel golangci-lint is running, no paths outside the worktree).sh -n script/cibuildclean; the script stays POSIXshwithset -euand no bashisms.date +%s%Nwas verified to work in this environment (uutils coreutils 0.8.0) and returns a full nanosecond value, not a literal%N. It is a GNU/uutils extension rather than POSIX; if a future CI host lacks it, the fallback would be appending$$to a POSIXdate +%s.Constraints honoured
golang/alpinesha256digests, golangci-lintc0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports009367f5c17a8d4c45a961a3a509277190a9a6f0all unchanged — theDockerfilediff adds only the comment block, theARG, and theechoprefix..golangci.ymluntouched; stillsha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.make checkstill runs in full. No-short, no skip flags, no narrowing to lint-only.script/cibuildstays POSIXshand keeps theROOT="$(cd "$(dirname "$0")/.." && pwd -P)"idiom.TODO.mdupdated in the same commit. One commit; files staged by name.Scope notes for the reviewer
Dockerfilefor a fail-fastlintstage andARG VERSION. This PR adds only theARGand theechoprefix at the existing check step, so whichever lands second rebases trivially.README.mdEntrypoints entry updated: it describedscript/cibuildas "plaindocker build .", which this change falsifies. One line, no other README edits.script/dockerand plaindocker build .still serving a cached check (filed as #124); andREPO_POLICIES.mdlines 62 and 170-172 still carrying the now-false guarantee, which belongs upstream withprompts#26 since that file is the org-canonical copy.[implementer] Summary and how to reproduce the evidence yourself.
One commit, four files, +38/-6.
Dockerfilegets a comment block,ARG CHECK_EPOCH, and anechoprefix on the existing check step;script/cibuildgains--build-arg CHECK_EPOCH="$(date +%s)"and a corrected header comment;README.mdgets one Entrypoints line;TODO.mdgets a Completed Steps entry in the same commit.The single thing worth checking closely is why
${CHECK_EPOCH}is expanded inside theRUNrather than just declared above it. Under BuildKit — the builder here, Docker 29.7.2 — a bareARGis not a layer and does not change the cache key of the instruction below it. WritingARG CHECK_EPOCHabove an unmodifiedRUN make checkwould look like the recommended fix from the issue while leaving the layerCACHEDand the bug fully intact. That failure mode is invisible to code review, which is exactly why the negative control matters here rather than being a formality.Reproducing the before state: check out
9347a28, runscript/cibuildtwice. The second returns exit 0 in about a quarter second withRUN make checkmarkedCACHED. I measured 283 ms.Reproducing the after state: on this branch, run
script/cibuildtwice. Both execute the suite (55.2 s and 42.2 s for me), print a differentcheck epoch:value each time, and show 216 passing tests — whileapk add, both pinnedgo installsteps,go mod downloadand bothCOPYlayers all stayCACHED. No modules re-downloaded, no toolchain reinstalled.Reproducing the negative control, which is the part I would re-run if I were reviewing this: drop a file in
internal/configwhose only test callst.Fatalwith a string you choose, runscript/cibuild, and confirm the build dies printing your string. Mine failed in 24.7 s with exit 1, surfacingNEGATIVE-CONTROL-115: planted failure, cache did not serve this layerand--- FAIL: TestNegativeControlIssue115, with the check step exiting 2. Deleting the file returned the tree to clean and the build to green in 48.1 s. The planted file is not in the commit.Build time lands at 42-55 s against the 5-minute ceiling.
make checkis green with0 issues.Two disclosures the reviewer should weigh:
docker buildonly throughscript/cibuild; there was no directdocker buildinvocation, including for the negative control.GOLANGCI_LINT_CACHE="$(mktemp -d)"in front of the make target. One run was void withError: parallel golangci-lint is running— a lock collision, not a lint result. Worth noting for whoever owns that guidance: the isolated cache directory did not prevent the collision, so the lock evidently does not live inGOLANGCI_LINT_CACHE. The retry was clean under both validity conditions.Two things I found and deliberately did not fix, to keep this confined to the issue and separately reviewable from #109:
REPO_POLICIES.mdnow describesscript/cibuildinaccurately in two places. It is the org-canonical copy, so the correction belongs upstream withprompts#26.script/dockerhas the identical caching hole. It is not the CI gate and it is byte-identical across repos, so it also wants a template fix rather than a local one. Say the word and I will file it.Nothing in the definition of done went unsatisfied.
[reviewer] Independent adversarial review of #122 @
f71ed12(basemain@9347a28)Verdict: FAIL —
needs-reworkThe fix works. I reproduced the cache defeat and confirmed it with my own negative control. One blocking finding, and it is not the code: the PR's headline technical justification — repeated verbatim in a permanent
Dockerfilecomment, the commit message, the PR body and the issue plan — is empirically false on this host. I tested it as instructed and it does not hold.Blocking
B1.
Dockerfilelines 23-26: the stated BuildKit mechanism is wrongThis says a bare
ARG CHECK_EPOCHabove an unchangedRUN make checkwould still be servedCACHED. It would not. On this builder (Docker 29.7.2 / buildx 0.36.1) the build-arg value participates in the cache key of subsequent instructions in the stage even when the instruction never references it.Experiment: I temporarily replaced line 33 with a bare
RUN make check, leavingARG CHECK_EPOCHin place, and ran four directdocker buildinvocations against a byte-identical tree, varying only--build-arg:--build-arg CHECK_EPOCHRUN make checkresult11111111112222222222CACHED)2222222222(repeat of B)CACHED1111111111(repeat of A)CACHEDB is the decisive cell: with the same tree, the same Dockerfile and an unreferenced
ARG, changing only the value busted the layer. C and D prove the cache was live and working the whole time, so B was a genuine key miss and not a cold cache. The recommended-in-#115 bare-ARGform would therefore have worked on its own.Why this is blocking rather than a nit:
echofor a reason that is fictitious and may reason from the false premise elsewhere. #109 is about to restructure this exact file.Acceptable looks like: keep the code exactly as it is — expanding the epoch into the
RUNis fine and has a real, honest benefit (it prints the epoch, making it visible in build output which run a layer belongs to). Reword lines 18-31 so the comment states what is true:script/cibuildpasses a freshCHECK_EPOCHper invocation; the build-arg value participates in this instruction's cache key, so the layer is re-executed every run; the value is echoed so the epoch is visible in the log. Drop the "BuildKit expands before keying, so a bare ARG would still hit the cache" claim. Correct the commit message and PR body to match. No functional change is needed.Non-blocking
N1.
script/cibuildline 15:date +%sis second-granularTwo invocations starting inside the same wall-clock second on an unchanged tree produce an identical
CHECK_EPOCH, and the second can be served from the first's layer. Sequential runs cannot collide (a build is ~45 s), so this is not the reported bug, but concurrent invocations (two CI jobs, a wrapper firing twice) can.date +%s%N, or appending$$, closes it at zero cost. Note the fix is only as strong as the freshness of this value — worth a comment either way.N2. Unbounded builder-cache growth
Every
script/cibuildrun mints a new check layer plus theRUN make buildlayer beneath it, none of which is ever reused. Correct by design, but the local builder cache now grows monotonically with cibuild invocations. Operational note only.N3. Plain
docker build .andscript/dockerstill serve a cached checkVerified: two consecutive plain
docker build .runs on the unchanged tree — the second reports#14 [builder 9/10] RUN echo "check epoch: ${CHECK_EPOCH}" && make checkasCACHED, with an emptycheck epoch:line on the first. This matches what the PR claims, and #115 scoped the fix to the CI entrypoint, so it is in-bounds and honestly documented. For the record: Docker emits no warning for the unsetARG, so the only signal is a barecheck epoch:with nothing after it.make docker/script/dockertherefore still produce a green that did not run the suite. The PR flags this rather than fixing it, which is the right call for scope; it should become an issue.N4.
REPO_POLICIES.mdnow contradicts the codeLines 62 and 170-172 still describe
script/cibuildas runningdocker build .and assert "a successful build implies all checks pass" — the exact false statement #115 was filed about. Deliberately deferred upstream by the author; defensible, but the repo carries a false claim in the meantime and it should be tracked, not just mentioned in a PR body.N5. Pre-existing, not this PR
level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0)"surfaces on every check run. Already accepted inTODO.md.Verification performed
All builds via
./script/cibuildexcept the bare-ARGexperiment (B1) and the no-build-arg experiment (N3), which used directdocker buildinvocations — stated explicitly. Nogotool run directly. Nothing was committed or pushed; working tree confirmed clean before and after every experiment.1. Twice in a row, unchanged tree
Four consecutive runs total, all on a byte-identical tree, all exit 0, all executing the check step:
Distinct epoch every run, no
CACHEDon the check layer, nothing close to sub-second. 216 test results across all eight packages,0 issues.from lint inside the container.2. My own negative control (not the author's)
Planted
internal/config/zz_reviewer_negctl_test.gowith a singlet.Fatalcarrying a string I chose in advance, then ranscript/cibuild. Exit 1, verbatim:A cached layer cannot emit a failure predicted in advance. The suite demonstrably ran. File deleted,
git statusclean, tree green again on the next run. Definition-of-done item 1 is satisfied.3. Dependency layers stayed cached
Confirmed
CACHEDon every post-fix run:apk add --no-cache git make gcc musl-dev binutils-gold,go install .../golangci-lint@c0d3ddc9...,go install .../goimports@009367f5...,WORKDIR /src,COPY go.mod go.sum ./,RUN go mod download,COPY . .. No module re-download, no toolchain reinstall. TheARGplacement is correct. Item 2 satisfied.4. Build time
48-53 s against the 5-minute ceiling in
REPO_POLICIES.mdline 231. Roughly 16-18% of budget. Item 6 satisfied.5. Other checks
check / check (push)= success in 58 s onf71ed12. Green. The workflow runsscript/cibuild, so CI now exercises the fixed path.f71ed12is a descendant oforigin/main@9347a28. No rebase needed.make check: exit 0,0 issues., run with an isolatedGOLANGCI_LINT_CACHE. Not void — zero occurrences ofparallel golangci-lint is running, zero file paths outside this worktree. One valid run, no voids hit.golang@sha256:f6751d82...,alpine@sha256:c3f8e73f..., golangci-lintc0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports009367f5c17a8d4c45a961a3a509277190a9a6f0— all unchanged inDockerfileandscript/bootstrap..golangci.yml:sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb. Unmodified.make checkruns in full viascript/check(test, then lint, then fmt-check). No-short, no skip flags, no lint-only narrowing. Strictly stricter.git diff --name-status 9347a28..f71ed12= 4 files,internal/untouched. No negative-control leftovers in the commit (mine or the author's).#!/bin/sh,set -eu,sh -nclean,ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"idiom preserved,$(date +%s)is POSIX, no bashisms.ARG CHECK_EPOCH, and theechoprefix. #109 rebases trivially.README.mdandTODO.mdedits are in-scope and correct.(closes #115),TODO.mdin the same commit, markdown wrapped consistently with surrounding prose. No forbidden attribution trailers or vendor references anywhere in the diff or commit message. Terminology clean.Summary: definition-of-done items 1-7 are all functionally met and independently verified. The single reason this is
needs-reworkrather thanmerge-readyis B1 — a factually incorrect explanation of builder behaviour, committed as a permanent comment and as the PR's central argument, in a repo where exactly that class of false claim is what produced this issue. Correcting prose is all that is required; the code should not change.[manager] Independent adversarial review returned FAIL. Relabeled
needs-rework, staying assigned toclawbot. Dispatching a prose-only rework.The fix is correct and stays. The explanation committed alongside it is false and must go.
Two independent experiments agree, and both contradict the PR
The reviewer reverted the
RUNto the bare form (leavingARG CHECK_EPOCHdeclared but unreferenced) and ran four builds on a byte-identical tree, varying only--build-arg:11111111112222222222CACHED2222222222CACHED1111111111CACHEDB is the decisive one; C and D prove the cache was live throughout, so B was a genuine key miss rather than an absent cache.
I ran my own probe before the review returned, on a minimal two-variant Dockerfile using this repo's pinned alpine digest. Same result: a declared-but-unreferenced
ARGdoes enter the cache key, and a changed value forces re-execution (DONE 0.3s, notCACHED).So the comment at
Dockerfilelines 23-26 — that BuildKit "derives each instruction's cache key from the command string after expansion, so a bare ARG above an unchanged RUN would still hit the cache" — is empirically false on this builder (Docker 29.7.2, buildx v0.36.1, BuildKit v0.32.2,DOCKER_BUILDKITunset, no# syntax=line). The bare form recommended in #115 would have worked on its own.Why this blocks, when the fix demonstrably works
Because the false claim is committed as a permanent
Dockerfilecomment, and because this PR is the reference implementation other repos are copying. Two sibling repos independently measured the opposite behaviour, and this claim was the sole reason propagation of the shared-template fix was halted while three repos waited.A correct fix carrying an incorrect justification is more dangerous than a plain error: the next person to touch this will reason from the comment. Merging it would embed a falsehood at the exact point where someone might later "simplify" the expanded form back to bare — and be misled about why they shouldn't.
A methodological note worth keeping
My first probe attempt returned "both
CACHED" and would have confirmed the PR's claim. It was wrong — an earlier run of my own probe had already populated the cache for those exact command strings. I only caught it because the result was too convenient. The corrected probe embeds a unique per-run nonce so no prior entry can match.That is the same trap as the original bug, one level up: a cache experiment can itself be served from cache. Anyone re-testing this without a nonce is measuring their own history.
What the author got right, and it is most of it
The negative control was rigorous and real, and the reviewer reproduced it independently with their own planted sentinel (
REVIEWER-NEGCTL-122, build exit 1, exact predicted string in the output, tree clean afterwards). Definition-of-done item 1 is satisfied. Four consecutivescript/cibuildruns on an unchanged tree all executed the check step with distinct epochs — 48.36s and 53.36s wall on runs 3 and 4, nothing sub-second, noCACHEDon the check layer. Dependency layers all stayedCACHED, so theARGplacement is right. 48-53s against the 5-minute ceiling.The failure was narrow and specific: the author asserted the mechanism as reasoning while proving only the conclusion. Their negative control tested the expanded form; it never tested the bare form.
Rework scope — prose only
Reword the
Dockerfilecomment block, the commit message, and the PR body to state that the build-arg value participates in the cache key of subsequent instructions even when they do not reference it, and that the expanded form is chosen deliberately — because it is correct regardless of how BuildKit treats unreferenced args, and because it surfaces the epoch in build logs as a diagnostic. No code change; theechostays.Also worth fixing while in there:
date +%sis second-granular, so two concurrent invocations in the same second produce identical epochs and the later one could be served from cache — the original bug, narrowly. Builds take 40s+ so sequential runs cannot collide, but concurrent ones can.date +%s%Nremoves the concern for one character.Deferred to follow-ups, not this PR: unbounded builder-cache growth;
script/dockerand plaindocker build .still serving a cached check (already filed as #124); andREPO_POLICIES.mdlines 62 and 170-172 still carrying the now-false guarantee, which belongs upstream withprompts#26 since that file is the org-canonical copy.A fresh reviewer will re-review the amended head.
[implementer] Rework of
f71ed12->ff66ecc. Prose plus one character. No functional change to the mechanism; theARGplacement and theechoare untouched.B1 (blocking) — false BuildKit claim: fixed, and the finding is accepted without reservation
The reviewer is right and my original justification was wrong. I asserted a mechanism while my negative control only ever established the conclusion — it exercised the expanded form and never once tested the bare form, so it could not have detected the error. Two independent experiments (the reviewer's four-build A/B/C/D matrix, and the manager's nonce-bearing minimal probe) both show that a declared-but-unreferenced
ARGdoes enter the cache key of later instructions in the stage, and that changing its value forces re-execution. Cells C and D settle it: the cache was live throughout, so B was a genuine key miss.Dockerfilelines 23-26 now read (replacing the four lines that carried the false claim):The surrounding comment (the
CHECK_EPOCHpurpose sentence above, theARG-placement paragraph below) is unchanged. The commit message carries the identical correction in itsFix:paragraph, and the PR body has been rewritten — including an explicit note that the earlier revision asserted the opposite and why that was wrong, so the record does not just quietly change.N1 (non-blocking) —
date +%sgranularity: fixedscript/cibuildline 15 now passes$(date +%s%N). Sequential runs never could collide at ~45 s per build; concurrent invocations within one wall-clock second could, and that is the original bug in miniature.Portability, since
%Nis a GNU/uutils extension and not POSIX: verified working here —date (uutils coreutils) 0.8.0returns a full 19-digit value, and it behaves identically under/bin/sh, not just the interactive shell. No literal%Nanywhere. Had it not worked, the fallback would have been$(date +%s)$$, which is POSIX and still collision-free for concurrent invocations.sh -n script/cibuildis clean; still#!/bin/sh,set -eu, no bashisms,ROOTidiom preserved.The change does not alter the shape of the command string — the epoch is substituted by the builder, not the script — so per the rework brief I did not repeat the negative control. Two independent ones already passed against this exact
RUN.Verification of the reworked head
script/cibuildtwice in a row, unchanged tree, after the%Nchange:1786256586666772611CACHED1786256640154832161Distinct nanosecond epochs, both printed in the build log, nothing remotely sub-second, no
CACHEDon the check layer either time. Layers 9-14 (apk add, both pinnedgo installsteps,WORKDIR,COPY go.mod go.sum,go mod download) plusCOPY . .allCACHEDon both runs — no toolchain reinstall, no module re-download. Gate unweakened: fullmake checkinside the container.Host
make check: exit 0,0 issues., run asGOLANGCI_LINT_CACHE=<isolated dir> GOFLAGS=-count=1 make check. Not void — noparallel golangci-lint is running, and no reported path outside this worktree. I hit zero void runs during this rework, one clean run on the first attempt. Thegomodguarddeprecation warning (N5) still surfaces; already tracked, untouched.Untouched, as required: no pin moved (
golang/alpinedigests, golangci-lintc0d3ddc9..., goimports009367f5...);.golangci.ymlstill021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb; nointernal/or test file changed; no negative-control leftovers; DNS still never mocked.git statusclean after both builds. Files staged by name —Dockerfile,TODO.md,script/cibuild.TODO.mdsits in the same commit and its Completed Steps entry was updated to say$(date +%s%N).The commit was amended rather than followed up, so the head is still a single commit whose title ends
(closes #115), force-pushed tofix/115-cibuild-cache. No new PR, no new branch. Diff against base is now 4 files, +41/-6.N2, N3, N4 — acknowledged, deliberately not addressed here
Builder-cache growth is an operational note;
script/dockerand plaindocker build .still serving a cached check is filed as #124;REPO_POLICIES.mdlines 62 and 170-172 belong upstream withprompts#26 since that file is the org-canonical copy. All three are out of #115's scope and none is silently skipped.Label left
needs-rework, assignee leftclawbot, for a fresh reviewer to re-reviewff66ecc.[reviewer] Independent adversarial re-review of #122 @
ff66ecc(basemain@9347a28; prior headf71ed12)Verdict: PASS
I did not author or rework this change and did not see it before this review. I re-tested the corrected claim from scratch rather than accepting the earlier experiments, using a per-run nonce so no result could be served from my own probe history. The new wording is true on this builder. No blocking findings. Four non-blocking items below, one of which I considered blocking and explain why I did not.
1. The corrected
Dockerfileclaim — verified, twice, with liveness controlsThe claim under test (
Dockerfilelines 23-26): "A build argument's value participates in the cache key of later instructions in the stage even when they do not reference it, so a fresh value busts this layer either way."Builder: Docker 29.7.2, buildx v0.36.1. Both experiments used direct
docker build— stated explicitly, as required. Nonce for this review:ba7900b69c439a99, never used by any earlier run.Experiment A — minimal context, nonce embedded in every command string (so no pre-existing cache entry can match). Dockerfile: pinned
alpinedigest, a baseRUN, thenARG CHECK_EPOCH, then a finalRUNthat never references the arg.--build-arg CHECK_EPOCHRUN...-a(fresh)...-b(fresh)CACHED...-b(repeat)...-a(repeat)Experiment B — the repo's real
Dockerfile, with line 36 temporarily reverted to a bareRUN make checkandARG CHECK_EPOCHleft declared-but-unreferenced, byte-identical tree across all four builds:--build-arg CHECK_EPOCHCOPY . .RUN make check...-A(fresh)...-B(fresh)CACHED...-B(repeat)...-A(repeat)Cell B is decisive in both: identical tree, identical command string, identical cached parent, only the unreferenced arg value changed, and the layer re-executed. Cells C and D are the liveness proof — repeating a previously-seen value comes back
CACHED, so the cache was live throughout and B was a genuine key miss, not an empty cache. In Experiment B theCOPY . .above it reportsCACHEDin the same build thatRUN make checkexecutes, which isolates the arg as the only variable.Conclusion: the reworked wording is correct, and the earlier wording it replaced was false. The second sentence — that the expanded form is chosen so invalidation is a property of the command string rather than of how a builder treats unreferenced args — is a hedge, not a mechanism claim, and is sound. B1 is resolved.
Experiment reverted;
git statusclean,HEADstillff66ecc,Dockerfilerestored byte-for-byte (git diff HEADempty).2. The corrected claim propagated everywhere
grepforbare ARG,after expansion,would still hit the cacheacross the tree: no hits. TheFix:paragraph of the commit message (git log 9347a28..ff66ecc --format=%B) carries the corrected claim verbatim. The PR body carries it plus an explicit erratum recording that the earlier revision asserted the opposite. The old wording survives nowhere.3.
script/cibuildtwice in a row on an unchanged tree1786257099551809952DONE 31.6s, notCACHED17862572408130483111786257361506593276Distinct epoch every run, printed in the build log, nothing remotely sub-second, never
CACHED.Run 1's 2 m 12 s is my doing, not the PR's: my Experiment B had left the builder holding a different
Dockerfilecontent, soCOPY . .and everything below it rebuilt. Runs 2 and 3 are the representative figures. Steady-state build time 34-44 s against the 5-minute ceiling (REPO_POLICIES.mdline 231) — roughly 11-15% of budget.4. My own negative control (third independent one)
Planted
internal/config/zz_rereviewer_negctl_test.gowith a singlet.Fatalcarrying a sentinel I chose in advance.script/cibuildoutput, verbatim:Separately confirmed
script/cibuild exit=1. File deleted,git statusclean, tree green again on run 3. A cached layer cannot emit a failure predicted in advance — definition-of-done item 1 is satisfied.5. Dependency layers stayed cached
On run 2, all
CACHED:apk add --no-cache git make gcc musl-dev binutils-gold,go install ...golangci-lint@c0d3ddc9...,go install ...goimports@009367f5...,WORKDIR /src,COPY go.mod go.sum ./,RUN go mod download,COPY . ., plus all four runtime-stage layers. No toolchain reinstall, no module re-download. TheARGplacement is right. Item 2 satisfied.6. Scope discipline
git diff f71ed12..ff66eccis exactly three hunks: theDockerfilecomment block (4 lines out, 7 in), theTODO.mdsentence that quoted$(date +%s), and onescript/cibuildline. NoARGrelocation, no removal of theecho, no restructuring, nointernal/or test-file change, noREADME.mdchange. Prose plus one character, as scoped.7. Hard constraints
golang@sha256:f6751d82...,alpine@sha256:c3f8e73f..., golangci-lintc0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports009367f5c17a8d4c45a961a3a509277190a9a6f0— unchanged in bothDockerfileandscript/bootstrap; neither file's pin lines appear in the diff..golangci.yml:sha256sum=021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb. Untouched.Co-Authored-By, no session trailers.git diff --name-status 9347a28..ff66ecc= 4 files (Dockerfile,README.md,TODO.md,script/cibuild). No test file, nointernal/file. No negative-control leftovers committed by anyone; my own planted file is deleted and the tree is clean.sh:#!/bin/sh,set -eu,sh -n script/cibuildclean,ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"idiom preserved, no bashisms.make checkruns in full inside the container (test, lint, fmt-check). No-short, no skip flags, no lint-only narrowing.script/test,script/check,script/lint,script/fmt-checkare untouched by this PR.(closes #115),TODO.mdupdated in the same commit and consistent with the surrounding entry style.check / check (push)= success onff66ecc. The workflow (.gitea/workflows/check.yml) is a singlerun: script/cibuild, so CI exercises the fixed path.ff66eccis a descendant oforigin/main@9347a28(re-fetched at end of review;mainhas not moved). No rebase needed.make check: exit 0,0 issues.,make fmt-checksilent. Not void — grep of the lint output forparallel golangci-lint is running, any../path, and any absolute path outside this worktree returned nothing. Zero void runs hit during this review.Non-blocking
N1.
%Nis not POSIX, and busybox silently drops it — the "never cached" guarantee is host-conditionalI tested three implementations rather than one:
date (uutils coreutils) 0.8.0, under/bin/sh(dash): 19-digit nanosecond value. Works.alpineimage, busyboxdate:date +%s%Nprints1786257437—%Nis silently dropped, exit 0. Degrades to second granularity.So on a busybox/POSIX-only host the epoch is second-granular and the concurrency guarantee the rework was meant to add does not hold there. That is no worse than
f71ed12, which is why it is not blocking. The catastrophic case — a constant value — requiresdateto exit non-zero, and no implementation I could find fails on an unknownstrftimeconversion; they copy it through. Worth knowing that if it ever did fail,set -euwould not save you: I verified that in dash a failing command substitution inside a command's arguments does not tripset -e(sh -c 'set -eu; echo "[$(false)]"; echo REACHED'prints[], thenREACHED, exit 0). The build would then run withCHECK_EPOCH=constant-empty and serve a cached green, silently. Low likelihood, maximum blast radius.$(date +%s%N)$$costs one more token, keeps nanoseconds where available, and makes concurrent-invocation uniqueness unconditional via the PID even where%Nis dropped or a substitution returns empty. Recommended, not required.Related:
script/cibuild's header comment and the newREADME.mdline both state the guarantee unconditionally ("never served from the Docker layer cache", "always means the checks ran on this invocation"). Those are true on this host and on theubuntu-latestrunner, and the PR body honestly discloses the%Ncaveat, so this does not reach the bar that failedf71ed12— that claim was false on this builder. Flagging it so it is a deliberate acceptance rather than an oversight.N2. An unset
CHECK_EPOCHdefaults silentlyRUN echo "check epoch: ${CHECK_EPOCH}" && make checkwith no--build-argprints an empty value, emits no builder warning, and serves the cached check — the repo's known-rejected "silent defaulting" shape. Failing loudly on an empty value would fixscript/dockerand plaindocker build .at the same time, but it would change behaviour the issue explicitly scoped out ("a plain docker build without the argument caches exactly as before") and is already tracked as #124. Correct call to leave it; noting it belongs in #124's brief rather than being lost.N3. Could not corroborate CI-side behaviour from logs
The commit status on
ff66eccis green and the workflow does invokescript/cibuild, but the job log the API returned for that run is stale and unrelated (dated 2026-02-21, showing a nativego buildand pre-#93 DNS skips rather than a Docker build). So there is no log-level confirmation thatdate +%s%Nexpanded correctly on the runner or that the check layer executed there. Not attributable to this PR, and the green status plus a workflow that is one line is sufficient. Recorded so it is not mistaken for verified.N4. Deferred items, correctly deferred
REPO_POLICIES.mdlines 62 and 170-172 still describescript/cibuildasdocker build .and still assert "a successful build implies all checks pass" — the exact statement #115 falsified. Confirmed still present. Deferred upstream toprompts#26 as the org-canonical copy. Also unbounded builder-cache growth, and #124. All three are out of #115's scope and none is silently skipped.N5. Nit
The commit subject is 95 characters and becomes ~102 after the squash-merge
(#122)suffix. No repo policy sets a limit, so this is preference only.Summary: the rework does exactly what it was scoped to do and nothing else. The claim that failed
f71ed12has been replaced with one I independently verified true under a nonce-protected design with liveness controls, and the correction reached theDockerfile, the commit message and the PR body alike. Definition-of-done items 1-7 all satisfied and independently re-verified, including a third negative control. Constraints intact, CI green, mergeable, no void lint runs.[manager] Fresh re-review — by a reviewer who had neither seen this PR before nor performed the rework — returned PASS, no blocking findings. Labeling
merge-readyand assigning to @sneak.The corrected claim is now verified, not merely asserted
This PR failed the first time because its explanation was asserted as reasoning while only its conclusion was proven. The rework could have repeated that mistake in the opposite direction, so the reviewer was asked to test the new wording experimentally. They did, twice, with a fresh nonce (
ba7900b6) never used by any prior probe:On a minimal context —
ARG CHECK_EPOCHdeclared, finalRUNnever referencing it:CACHEDCACHEDCACHEDOn this repo's real
Dockerfile, line 36 temporarily reverted to bareRUN make check, byte-identical tree throughout:COPY . .RUN make checkCACHEDCACHEDCACHEDCACHEDCACHEDCACHEDRow B of the second table is the strongest single piece of evidence produced in this whole investigation:
COPY . .reportsCACHEDin the very same build where the check executes, which isolates the build argument as the only variable. C and D are the liveness proof that a miss was a genuine key miss rather than an empty cache. Experiment reverted,git diff HEADempty, head stillff66ecc.A third independent negative control also passed, with the reviewer's own predicted sentinel appearing verbatim in a failing build (exit 1), then a clean tree and a green build afterwards.
Grep confirms the old false wording is gone tree-wide — no hits for
bare ARG,after expansion, orwould still hit the cachein theDockerfile, commit message, or PR body.Two findings that outlive this PR
1.
%Nsilently degrades on busybox. The reviewer checked the repo's own pinned alpine image:date +%s%Nthere prints1786257437— it drops%Nsilently and exits 0, degrading to second granularity. Not blocking, because that is exactly wheref71ed12already was and no worse. But it means the nanosecond guarantee is host-conditional, whilescript/cibuild's header comment and the README describe it unconditionally.They also chased the catastrophic path properly rather than assuming: a constant
CHECK_EPOCHwould requiredateto exit non-zero, which no strftime implementation does for an unknown conversion — and they verified in dash that even if it did,set -euwould not catch a failing command substitution inside a command's arguments, so the build would silently getCHECK_EPOCH=empty and constant. That is the false green returning by a different door.$(date +%s%N)$$makes the freshness unconditional for four characters. Filed as #125.2. The CI log for this commit does not correspond to this commit. The reviewer could not corroborate CI-side behaviour: the job log Gitea's API returns for the green run on
ff66eccis dated 2026-02-21 and shows a nativego buildwith pre-#93 DNS skips — a build that predates this branch entirely. So the green check mark on this PR is not evidence about this code.That matters well beyond #122. "CI green" has been cited as corroborating evidence on every PR in this milestone. It now appears to be a fifth check that looks authoritative and is not — after
script/cibuild(#115),script/bootstrap(#117),script/lint(#121), andscript/docker(#124). Filed as #126; it needs your access, since the Actions API returns 403 for this account.None of the merge-ready decisions in this milestone rested on CI. Every one was made on locally-reproduced evidence — negative controls, mutation tests, and repeated cache-bypassed runs — which is fortunate, and in hindsight was the right instinct rather than good luck.
Verified clean
All pins unchanged;
.golangci.ymlsha256 still021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb; no test orinternal/file touched;git diff f71ed12..ff66eccis exactly prose plus one character;sh -n script/cibuildclean and still POSIX; title ends(closes #115);TODO.mdin the same commit; no vendor references or attribution trailers; fast-forwards fromorigin/main9347a28.Zero void lint runs —
0 issues., noparallel golangci-lint is running, no../or foreign absolute paths.ci: re-run make check on every cibuild instead of serving it from the layer cache (closes #115)to WIP: ci: re-run make check on every cibuild instead of serving it from the layer cache (closes #115)WIP: ci: re-run make check on every cibuild instead of serving it from the layer cache (closes #115)to ci: re-run make check on every cibuild instead of serving it from the layer cache (closes #115)`script/cibuild` was plain `docker build .`. The Dockerfile does `COPY . .` and then `RUN make check`, and Docker invalidates `COPY . .` only on a content change, so on a byte-identical tree the check layer was reused and the suite never ran. The script's header comment claimed that a successful build implies all checks pass, which was false whenever the cache was warm. Reproduced on this branch's parent: a second consecutive run returned success in 283 ms with `#13 [builder 9/10] RUN make check` reported `CACHED`. That matters more here than in a typical repo. DNS is never mocked in this repository, so the suite queries live DNS and its outcome varies with real-world conditions; caching the verdict of a non-deterministic check replays a stale result in exactly the case where re-running is most valuable. It is also the gate every PR is verified through. Fix: declare `ARG CHECK_EPOCH` immediately above the check step and expand it into the command, with `script/cibuild` passing a fresh `$(date +%s%N)` per invocation. A build argument's value participates in the cache key of later instructions in the stage even when they do not reference it, so a fresh value busts this layer either way; the value is expanded into the command deliberately, which makes the invalidation a property of the command string itself rather than of how a given builder treats unreferenced args, and surfaces the epoch in the build log as a diagnostic. Placing the ARG here and no earlier keeps the pinned toolchain installs and `go mod download` above the invalidation line, so only the check and the steps after it re-run. The epoch is nanosecond granular so that two concurrent invocations starting in the same second cannot share a value. A plain `docker build` without the argument caches as before; nothing outside the CI entrypoint changes behaviour. Verified by experiment, not inspection: - Two consecutive runs on an unchanged tree: 55.2 s and 42.2 s, both exit 0, with distinct epochs. The second run shows `RUN echo "check epoch: ..." && make check` executing for 36.0 s and 216 passing tests across all eight packages, while `apk add`, both pinned `go install` steps, `go mod download`, `COPY go.mod go.sum` and `COPY . .` all report `CACHED`. - Negative control: planted `internal/config/zz_negative_control_test.go` calling `t.Fatal("NEGATIVE-CONTROL-115: planted failure, cache did not serve this layer")`. The build failed in 24.7 s with exit 1, printing that exact message and `--- FAIL: TestNegativeControlIssue115`, and the check step exited with code 2. A cached layer cannot produce a failure predicted in advance, so this establishes the suite ran. The file was then removed, `git status` confirmed clean, and the tree built green again in 48.1 s. - Total build time 42-55 s against the policy's 5-minute ceiling. - `make check` green. No pin touched: the `golang` and `alpine` sha256 digests, golangci-lint `c0d3ddc9`, and goimports `009367f5` are unchanged, and `.golangci.yml` still hashes to `021cc83f4e6f...`.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.