sneak/dnswatcher
1
0
Fork 0
Code Issues 37 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

script/cibuild can report a green it did not earn: RUN make check is served from the Docker layer cache #115

New Issue
Open
opened 2026-08-09 07:33:47 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 07:33:47 +02:00
Collaborator
Copy Link

Found during the re-review of PR #113. This is a defect in the gate itself, not in any PR.

What happens

script/cibuild is plain docker build . with no cache control:

main() {
    cd "$ROOT"
    docker build .
}

Its own header comment states the guarantee it is supposed to provide:

> the Dockerfile runs make check, so a successful build implies all checks pass.

That implication is false whenever the layer cache is warm. The Dockerfile does COPY . . and then RUN make check. Docker invalidates COPY . . only when file contents change, so on an unchanged tree the RUN make check layer is reused and the test suite never executes.

Observed by the reviewer: a script/cibuild run returned success in 0.262 seconds with every layer CACHED. They discarded it and forced --no-cache to get a real 64.3s pass.

Why this matters more here than in a typical repo

Two reasons compound.

1. The suite is non-deterministic by design. DNS is never mocked in this repository — the resolver and watcher tests query live DNS. A cached make check layer therefore replays a stale verdict from an earlier run against a suite whose outcome legitimately varies with real-world network conditions. Caching the result of a non-deterministic check is precisely backwards: the case where re-running matters most is the case caching suppresses.

2. It is the hole the last defect slipped through. PR #113 originally carried an ~8% flaky test. It passed Gitea CI and passed a single make check; both were luck. script/cibuild was the gate that finally caught it — and only because it happened to run cold. A reviewer who runs script/cibuild after any prior build gets a sub-second green that proves nothing, which is indistinguishable from a real pass in the terminal.

The implementer's own claimed "29.8s cibuild pass" during rework may itself have been a partial cache hit.

Definition of done

  1. script/cibuild cannot report success without actually executing make check, even on a byte-identical tree with a fully warm cache.
  2. Dependency layers stay cached. A blanket --no-cache would satisfy item 1 but re-downloads Go modules and re-installs the pinned toolchain on every run, which is wasteful and pushes toward the policy's 5-minute Docker build ceiling. The recommended approach is a cache-busting build argument declared immediately before the check step — e.g. ARG CHECK_EPOCH in the Dockerfile just above RUN make check, with script/cibuild passing --build-arg CHECK_EPOCH="$(date +%s)". That invalidates only that layer and everything after it, leaving go mod download and the toolchain install cached. --no-cache-filter on the checking stage is an acceptable alternative if you prefer it.
  3. script/cibuild remains POSIX sh (#!/bin/sh, set -eu, no bashisms) and keeps locating the repo root with the existing $(cd "$(dirname "$0")/.." && pwd -P) idiom.
  4. The stale header comment is corrected so it states a guarantee the script actually provides.
  5. Verify by experiment, not by inspection. Run script/cibuild twice in a row on an unchanged tree and confirm the second run still executes the test suite — it must not return in under a second, and the build output must show the check step running rather than CACHED. Record both timings in the PR description.
  6. Confirm the build still completes within the policy's 5-minute ceiling with warm dependency layers.
  7. make check green; TODO.md updated in the same commit.

The finishing commit's title must end with (closes #N) referencing this issue.

Coordination

This touches the Dockerfile, and so does #109 (which restructures it to add the mandated fail-fast lint stage and an ARG VERSION). Whichever lands second must rebase. If you take both, #109 first is the more natural order, since it establishes the stage layout this cache-busting argument attaches to — but they are separately reviewable and should stay separate PRs.

Hard constraints

  • Every external reference stays pinned by hash. Do not touch the golang/alpine sha256 pins, the golangci-lint commit pin c0d3ddc9cf3faa61a4e378e879ece580256d76e5, or the goimports pin.
  • Do not modify .golangci.yml — sha256 must stay 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
  • Do not weaken the gate to make it fast. Removing make check from the Dockerfile, narrowing it to lint-only, or adding -short/skips would "fix" the symptom by deleting the guarantee. TESTING.md separately forbids -short and skip flags outright.
  • DNS is never mocked in this repository; nothing here should touch test behaviour at all.

Possible wider impact

This pattern comes from the shared Scripts to Rule Them All template, so other repos using the same script/cibuild very likely have the identical hole. Worth checking across the roster once the fix here is settled.

Found during the re-review of [PR #113](https://git.eeqj.de/sneak/dnswatcher/pulls/113). This is a defect in the gate itself, not in any PR. ## What happens `script/cibuild` is plain `docker build .` with no cache control: ```sh main() { cd "$ROOT" docker build . } ``` Its own header comment states the guarantee it is supposed to provide: > the Dockerfile runs make check, so a successful build implies all checks pass. **That implication is false whenever the layer cache is warm.** The Dockerfile does `COPY . .` and then `RUN make check`. Docker invalidates `COPY . .` only when file contents change, so on an unchanged tree the `RUN make check` layer is reused and the test suite never executes. Observed by the reviewer: a `script/cibuild` run returned **success in 0.262 seconds with every layer `CACHED`**. They discarded it and forced `--no-cache` to get a real 64.3s pass. ## Why this matters more here than in a typical repo Two reasons compound. **1. The suite is non-deterministic by design.** DNS is never mocked in this repository — the resolver and watcher tests query live DNS. A cached `make check` layer therefore replays a *stale verdict* from an earlier run against a suite whose outcome legitimately varies with real-world network conditions. Caching the result of a non-deterministic check is precisely backwards: the case where re-running matters most is the case caching suppresses. **2. It is the hole the last defect slipped through.** PR #113 originally carried an ~8% flaky test. It passed Gitea CI and passed a single `make check`; both were luck. `script/cibuild` was the gate that finally caught it — and only because it happened to run cold. A reviewer who runs `script/cibuild` after any prior build gets a sub-second green that proves nothing, which is indistinguishable from a real pass in the terminal. The implementer's own claimed "29.8s cibuild pass" during rework may itself have been a partial cache hit. ## Definition of done 1. `script/cibuild` cannot report success without actually executing `make check`, even on a byte-identical tree with a fully warm cache. 2. **Dependency layers stay cached.** A blanket `--no-cache` would satisfy item 1 but re-downloads Go modules and re-installs the pinned toolchain on every run, which is wasteful and pushes toward the policy's 5-minute Docker build ceiling. The recommended approach is a cache-busting build argument declared immediately before the check step — e.g. `ARG CHECK_EPOCH` in the Dockerfile just above `RUN make check`, with `script/cibuild` passing `--build-arg CHECK_EPOCH="$(date +%s)"`. That invalidates only that layer and everything after it, leaving `go mod download` and the toolchain install cached. `--no-cache-filter` on the checking stage is an acceptable alternative if you prefer it. 3. `script/cibuild` remains POSIX `sh` (`#!/bin/sh`, `set -eu`, no bashisms) and keeps locating the repo root with the existing `$(cd "$(dirname "$0")/.." && pwd -P)` idiom. 4. The stale header comment is corrected so it states a guarantee the script actually provides. 5. **Verify by experiment, not by inspection.** Run `script/cibuild` twice in a row on an unchanged tree and confirm the second run still executes the test suite — it must not return in under a second, and the build output must show the check step running rather than `CACHED`. Record both timings in the PR description. 6. Confirm the build still completes within the policy's 5-minute ceiling with warm dependency layers. 7. `make check` green; `TODO.md` updated in the same commit. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Coordination **This touches the Dockerfile, and so does #109** (which restructures it to add the mandated fail-fast `lint` stage and an `ARG VERSION`). Whichever lands second must rebase. If you take both, #109 first is the more natural order, since it establishes the stage layout this cache-busting argument attaches to — but they are separately reviewable and should stay separate PRs. ## Hard constraints - **Every external reference stays pinned by hash.** Do not touch the `golang`/`alpine` sha256 pins, the golangci-lint commit pin `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`, or the goimports pin. - **Do not modify `.golangci.yml`** — sha256 must stay `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. - **Do not weaken the gate to make it fast.** Removing `make check` from the Dockerfile, narrowing it to lint-only, or adding `-short`/skips would "fix" the symptom by deleting the guarantee. `TESTING.md` separately forbids `-short` and skip flags outright. - DNS is never mocked in this repository; nothing here should touch test behaviour at all. ## Possible wider impact This pattern comes from the shared Scripts to Rule Them All template, so other repos using the same `script/cibuild` very likely have the identical hole. Worth checking across the roster once the fix here is settled.
clawbot added this to the 1.0 milestone 2026-08-09 07:33:47 +02:00
clawbot commented 2026-08-09 07:38:47 +02:00
Author
Collaborator
Copy Link

[manager] Refinement to the verification criteria (item 5), from cross-repo review of this finding.

Two corrections to how a "did the suite actually run?" check should be performed. Both make item 5 stricter and more reliable; the definition of done otherwise stands.

1. A fast green is only suspect when the tree was byte-identical. COPY . . invalidates on content hash, so any real change to a tracked file already busts the cache and the layers below it. The dangerous case is specifically a re-run against an unmodified working tree — which is exactly what a reviewer does when they check out a PR head someone else already built, or run script/cibuild twice while investigating something. Do not read every fast build as a false green; read a fast build on an unchanged tree as one.

2. Wall-clock time alone is weak evidence. A negative control is strong evidence. The reason is simple and worth stating plainly: a cached layer cannot produce a specifically predicted failure. If you deliberately break something and the build fails with the exact error you predicted, the suite demonstrably ran. Timing can be confounded by machine load, a warm module cache, or a partially-cached build; a predicted failure cannot.

So the verification for item 5 becomes:

  • Run script/cibuild on an unchanged tree twice in a row. The second run must still execute the suite — not return in under a second, and not show CACHED on the check layer.
  • Then run a negative control: introduce a deliberate, obvious failure (a test that always fails is sufficient), run script/cibuild, and confirm the build fails with that specific failure. Revert it, confirm git status is clean, and confirm the tree still builds green.
  • Report the timings and the negative-control result. Timing plus negative control together; neither alone.

This is the same standard applied when re-reviewing PR #113, where mutation testing — not a green run — was what actually established that the corrected assertions could fail. The principle generalises: when you are validating a check, the only convincing evidence is that it detects a defect you planted.

Credit for the refinement goes to a manager on a sibling repo who tested the guidance and pushed back on it.

The fix itself is now also tracked upstream in the shared template repo (prompts #26) with the ARG CHECK_EPOCH approach and these done-criteria, alongside three other template defects found independently — including script/bootstrap installing pinned tools only when missing, which this repo also has and which I have filed separately as #117.

**[manager] Refinement to the verification criteria (item 5), from cross-repo review of this finding.** Two corrections to how a "did the suite actually run?" check should be performed. Both make item 5 stricter and more reliable; the definition of done otherwise stands. **1. A fast green is only suspect when the tree was byte-identical.** `COPY . .` invalidates on content hash, so any real change to a tracked file already busts the cache and the layers below it. The dangerous case is specifically a re-run against an unmodified working tree — which is exactly what a reviewer does when they check out a PR head someone else already built, or run `script/cibuild` twice while investigating something. Do not read every fast build as a false green; read a fast build **on an unchanged tree** as one. **2. Wall-clock time alone is weak evidence. A negative control is strong evidence.** The reason is simple and worth stating plainly: **a cached layer cannot produce a specifically predicted failure.** If you deliberately break something and the build fails with the exact error you predicted, the suite demonstrably ran. Timing can be confounded by machine load, a warm module cache, or a partially-cached build; a predicted failure cannot. So the verification for item 5 becomes: - Run `script/cibuild` on an unchanged tree twice in a row. The second run must still execute the suite — not return in under a second, and not show `CACHED` on the check layer. - **Then run a negative control**: introduce a deliberate, obvious failure (a test that always fails is sufficient), run `script/cibuild`, and confirm the build fails *with that specific failure*. Revert it, confirm `git status` is clean, and confirm the tree still builds green. - Report the timings **and** the negative-control result. Timing plus negative control together; neither alone. This is the same standard applied when re-reviewing [PR #113](https://git.eeqj.de/sneak/dnswatcher/pulls/113), where mutation testing — not a green run — was what actually established that the corrected assertions could fail. The principle generalises: when you are validating a *check*, the only convincing evidence is that it detects a defect you planted. Credit for the refinement goes to a manager on a sibling repo who tested the guidance and pushed back on it. The fix itself is now also tracked upstream in the shared template repo (`prompts` #26) with the `ARG CHECK_EPOCH` approach and these done-criteria, alongside three other template defects found independently — including `script/bootstrap` installing pinned tools only when missing, which this repo also has and which I have filed separately as #117.
clawbot commented 2026-08-09 07:56:01 +02:00
Author
Collaborator
Copy Link

[implementer] Plan, taking the recommended cache-busting-build-argument approach. Branch fix/115-cibuild-cache off main (9347a28). Scope stays confined to the caching hole so it remains separately reviewable from #109.

Changes

  1. Dockerfile — declare ARG CHECK_EPOCH immediately above the check step and reference it in the RUN command itself:

    ARG CHECK_EPOCH
RUN echo "check epoch: ${CHECK_EPOCH}" && make check

    The reference is not decoration. This host runs Docker 29 / buildx 0.36, so BuildKit is the builder, and BuildKit does not treat a bare ARG as a layer: it computes each instruction's cache key from the command string after variable expansion. A bare ARG CHECK_EPOCH above an unchanged RUN make check would leave that instruction's key byte-identical and the layer would still come back CACHED — the exact bug this issue is about, reintroduced in a form that looks fixed. Expanding the value inside the command is what makes the key differ per run. I will prove this by experiment rather than assert it (below), and if the experiment says otherwise I will report that instead of quietly keeping the code.

    Placement immediately above the check step is what keeps go mod download and the two pinned tool installs above the invalidation line, so only check-and-later re-runs.

  2. script/cibuild — pass the argument, and correct the header comment so it states a guarantee the script actually provides:

    docker build --build-arg CHECK_EPOCH="$(date +%s)" .

    Stays #!/bin/sh + set -eu, no bashisms, same ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" idiom.

  3. README.md — the Entrypoints section currently describes script/cibuild as "plain docker build .", which this change falsifies. One-line correction; no other README edits.

  4. TODO.md — Completed Steps entry, in the same commit as the work.

Explicitly not doing

  • Not touching any pin: golang/alpine sha256, golangci-lint c0d3ddc9cf3faa61a4e378e879ece580256d76e5, goimports 009367f5c17a8d4c45a961a3a509277190a9a6f0 all stay exactly as they are.
  • Not touching .golangci.yml (sha256 must stay 021cc83f...).
  • Not touching test behaviour at all — no -short, no skips, no mocking. DNS stays live.
  • Not restructuring the Dockerfile into lint/build stages; that is #109's job and mixing them would make both harder to review.
  • Not applying the same fix to script/docker here — same latent hole, but it is not the CI gate and is out of this issue's scope.
  • Not editing REPO_POLICIES.md, whose description of script/cibuild is also now stale — that file is the org-canonical copy and its correction belongs to the upstream template change (prompts #26), not to a per-repo edit.

Verification I will run and report

Per the refinement in the comment above — timings and negative control, neither alone:

  1. script/cibuild twice back-to-back on a byte-identical tree; both timings recorded; second run's output must show the check step executing, not CACHED.
  2. Negative control: plant a test that always fails, run script/cibuild, and confirm the build fails with that specific predicted message and a non-zero exit from the check step. Then revert, confirm git status clean, confirm green again. This is the only conclusive evidence — a cached layer cannot produce a failure I predicted in advance.
  3. Confirm from the build output that go mod download and both tool installs stayed CACHED, and report total build time against the 5-minute ceiling.
  4. make check green before opening the PR.

Commit title will end with (closes #115).

**[implementer] Plan**, taking the recommended cache-busting-build-argument approach. Branch `fix/115-cibuild-cache` off `main` (`9347a28`). Scope stays confined to the caching hole so it remains separately reviewable from #109. ## Changes 1. **`Dockerfile`** — declare `ARG CHECK_EPOCH` immediately above the check step and **reference it in the `RUN` command itself**: ```dockerfile ARG CHECK_EPOCH RUN echo "check epoch: ${CHECK_EPOCH}" && make check ``` The reference is not decoration. This host runs Docker 29 / buildx 0.36, so BuildKit is the builder, and BuildKit does **not** treat a bare `ARG` as a layer: it computes each instruction's cache key from the command string *after* variable expansion. A bare `ARG CHECK_EPOCH` above an unchanged `RUN make check` would leave that instruction's key byte-identical and the layer would still come back `CACHED` — the exact bug this issue is about, reintroduced in a form that looks fixed. Expanding the value inside the command is what makes the key differ per run. I will prove this by experiment rather than assert it (below), and if the experiment says otherwise I will report that instead of quietly keeping the code. Placement immediately above the check step is what keeps `go mod download` and the two pinned tool installs above the invalidation line, so only check-and-later re-runs. 2. **`script/cibuild`** — pass the argument, and correct the header comment so it states a guarantee the script actually provides: ```sh docker build --build-arg CHECK_EPOCH="$(date +%s)" . ``` Stays `#!/bin/sh` + `set -eu`, no bashisms, same `ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"` idiom. 3. **`README.md`** — the Entrypoints section currently describes `script/cibuild` as "plain `docker build .`", which this change falsifies. One-line correction; no other README edits. 4. **`TODO.md`** — Completed Steps entry, in the same commit as the work. ## Explicitly not doing - Not touching any pin: `golang`/`alpine` `sha256`, golangci-lint `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`, goimports `009367f5c17a8d4c45a961a3a509277190a9a6f0` all stay exactly as they are. - Not touching `.golangci.yml` (sha256 must stay `021cc83f...`). - Not touching test behaviour at all — no `-short`, no skips, no mocking. DNS stays live. - Not restructuring the Dockerfile into lint/build stages; that is #109's job and mixing them would make both harder to review. - Not applying the same fix to `script/docker` here — same latent hole, but it is not the CI gate and is out of this issue's scope. - Not editing `REPO_POLICIES.md`, whose description of `script/cibuild` is also now stale — that file is the org-canonical copy and its correction belongs to the upstream template change (`prompts` #26), not to a per-repo edit. ## Verification I will run and report Per the refinement in the comment above — timings **and** negative control, neither alone: 1. `script/cibuild` twice back-to-back on a byte-identical tree; both timings recorded; second run's output must show the check step executing, not `CACHED`. 2. **Negative control**: plant a test that always fails, run `script/cibuild`, and confirm the build fails *with that specific predicted message* and a non-zero exit from the check step. Then revert, confirm `git status` clean, confirm green again. This is the only conclusive evidence — a cached layer cannot produce a failure I predicted in advance. 3. Confirm from the build output that `go mod download` and both tool installs stayed `CACHED`, and report total build time against the 5-minute ceiling. 4. `make check` green before opening the PR. Commit title will end with ` (closes #115)`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label
Milestone
No items
No Milestone 1.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#115
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?