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

script/lint shares one golangci-lint cache and lock across concurrent worktrees, so results can come from another codebase #121

New Issue
Open
opened 2026-08-09 08:04:02 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-09 08:04:02 +02:00
Collaborator
Copy Link

script/lint invokes golangci-lint without isolating its cache. golangci-lint defaults to a single shared cache directory (~/.cache/golangci-lint) with a single lock, per user account — not per repository and not per worktree.

That is fine on a developer laptop running one thing at a time. It is not fine here, where many concurrent sessions lint different repositories from throwaway worktrees on the same host.

Two distinct failure modes, both confirmed on this host

1. Cross-contamination — a lint verdict from a different codebase. A make lint run on a sibling repo returned 399 issues attributed to files under a worktree path that no longer existed, belonging to another session entirely. The tree being linted genuinely reports 0 issues. Another session's cached findings were served as this repo's own result.

2. Lock collision — a non-result mistaken for a failure. make lint can fail outright with:

Error: parallel golangci-lint is running

That is not a lint verdict. An agent or CI step treating a non-zero exit as "lint failed" will mislabel a perfectly good change as broken.

Why this repo is unusually exposed

Two reasons compound.

This repo's test suite is already deliberately non-deterministic. DNS is never mocked here, so tests query live DNS and outcomes legitimately vary with network conditions. Considerable effort in this milestone has gone into separating real flakiness from noise — see #113, where an ~8% flaky test passed CI and a single make check by luck. A cross-contaminated lint result is a third source of false signal that resembles neither a real failure nor a network flake, and it arrives wearing the costume of a definitive verdict.

It can void a verdict in either direction. Together with #115 (script/cibuild reporting an unearned green from the Docker layer cache) and #117 (script/bootstrap skipping the pinned linter install, so the running linter may not even be the pinned version), the lint gate on this host can currently produce an unearned green, an unearned red, or a result computed from a different codebase. Any one of those undermines every "lint clean" claim recorded in this milestone.

Definition of done

  1. script/lint gives each invocation an isolated golangci-lint cache, so concurrent runs on the same host cannot share cache entries or contend on one lock. Setting GOLANGCI_LINT_CACHE to a path derived from the repo root (or a per-run temporary directory) before invoking the linter is the straightforward fix.
  2. The chosen location must not pollute the repo. If it lives inside the working tree it must be git-ignored and Docker-ignored, and make check must still not modify any tracked file — REPO_POLICIES.md requires that make check leave the repo unmodified.
  3. Repeated runs still benefit from caching (do not simply disable the cache — that trades a correctness bug for a speed regression on every invocation). Report lint wall time before and after.
  4. script/lint remains POSIX sh (#!/bin/sh, set -eu, no bashisms) — it runs in minimal alpine images with no bash — and keeps the $(cd "$(dirname "$0")/.." && pwd -P) root-location idiom.
  5. Works inside the Docker build, where HOME and the cache path may differ from a developer machine. Verify with a real container build, not by reading the script.
  6. Verify empirically under contention. Launch two make lint runs concurrently from two different worktrees of this repo and confirm: neither fails with parallel golangci-lint is running, and neither reports a path belonging to the other. A code-reading argument is not sufficient — the entire defect is that the current script looks correct and is not.
  7. make check green; TODO.md updated in the same commit.

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

Interim rule, until this lands

Anyone recording a lint verdict on this repo — implementer or reviewer — must treat a run as VOID unless both hold:

  • the output does not contain parallel golangci-lint is running; and
  • the output mentions no file paths outside the worktree it was launched from.

Retry until both hold. Never record a verdict from a void run. Prefer sidestepping the shared cache entirely with GOLANGCI_LINT_CACHE="$(mktemp -d)" make lint, which isolates both the cache and the lock and still goes through the make target.

Hard constraints

  • Do NOT modify .golangci.yml. It is org-standardised and must never be touched by an agent. Its sha256 must remain 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
  • Do NOT change the golangci-lint pin c0d3ddc9cf3faa61a4e378e879ece580256d76e5, in either Dockerfile or script/bootstrap.
  • Do not suppress or narrow any lint check to make the gate quieter. This issue is about the cache, not the ruleset.
  • DNS is never mocked in this repository; nothing here touches test behaviour.

Coordination

Touches script/lint. #115 changes script/cibuild and the Dockerfile, #117 changes script/bootstrap, #119 changes script/fmt and script/fmt-check. All four are small and independent; keep them as separate PRs and rebase whichever lands later.

Tracked upstream in the shared template repo as prompts #30, alongside #115 (prompts #26) and #117 (prompts #28). The canonical fix there is a per-worktree GOLANGCI_LINT_CACHE in script/lint; keep this repo consistent with whatever lands upstream.

`script/lint` invokes `golangci-lint` without isolating its cache. golangci-lint defaults to a single shared cache directory (`~/.cache/golangci-lint`) with a single lock, per user account — not per repository and not per worktree. That is fine on a developer laptop running one thing at a time. It is not fine here, where many concurrent sessions lint different repositories from throwaway worktrees on the same host. ## Two distinct failure modes, both confirmed on this host **1. Cross-contamination — a lint verdict from a different codebase.** A `make lint` run on a sibling repo returned **399 issues attributed to files under a worktree path that no longer existed**, belonging to another session entirely. The tree being linted genuinely reports 0 issues. Another session's cached findings were served as this repo's own result. **2. Lock collision — a non-result mistaken for a failure.** `make lint` can fail outright with: ``` Error: parallel golangci-lint is running ``` That is not a lint verdict. An agent or CI step treating a non-zero exit as "lint failed" will mislabel a perfectly good change as broken. ## Why this repo is unusually exposed Two reasons compound. **This repo's test suite is already deliberately non-deterministic.** DNS is never mocked here, so tests query live DNS and outcomes legitimately vary with network conditions. Considerable effort in this milestone has gone into separating real flakiness from noise — see #113, where an ~8% flaky test passed CI and a single `make check` by luck. A cross-contaminated *lint* result is a **third** source of false signal that resembles neither a real failure nor a network flake, and it arrives wearing the costume of a definitive verdict. **It can void a verdict in either direction.** Together with #115 (`script/cibuild` reporting an unearned green from the Docker layer cache) and #117 (`script/bootstrap` skipping the pinned linter install, so the running linter may not even be the pinned version), the lint gate on this host can currently produce an unearned green, an unearned red, **or** a result computed from a different codebase. Any one of those undermines every "lint clean" claim recorded in this milestone. ## Definition of done 1. `script/lint` gives each invocation an isolated golangci-lint cache, so concurrent runs on the same host cannot share cache entries or contend on one lock. Setting `GOLANGCI_LINT_CACHE` to a path derived from the repo root (or a per-run temporary directory) before invoking the linter is the straightforward fix. 2. The chosen location must not pollute the repo. If it lives inside the working tree it **must** be git-ignored and Docker-ignored, and `make check` must still not modify any tracked file — `REPO_POLICIES.md` requires that `make check` leave the repo unmodified. 3. Repeated runs still benefit from caching (do not simply disable the cache — that trades a correctness bug for a speed regression on every invocation). Report lint wall time before and after. 4. `script/lint` remains POSIX `sh` (`#!/bin/sh`, `set -eu`, no bashisms) — it runs in minimal alpine images with no bash — and keeps the `$(cd "$(dirname "$0")/.." && pwd -P)` root-location idiom. 5. Works inside the Docker build, where `HOME` and the cache path may differ from a developer machine. Verify with a real container build, not by reading the script. 6. **Verify empirically under contention.** Launch two `make lint` runs concurrently from two different worktrees of this repo and confirm: neither fails with `parallel golangci-lint is running`, and neither reports a path belonging to the other. A code-reading argument is not sufficient — the entire defect is that the current script looks correct and is not. 7. `make check` green; `TODO.md` updated in the same commit. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Interim rule, until this lands Anyone recording a lint verdict on this repo — implementer or reviewer — must treat a run as **VOID** unless both hold: - the output does **not** contain `parallel golangci-lint is running`; and - the output mentions **no** file paths outside the worktree it was launched from. Retry until both hold. Never record a verdict from a void run. Prefer sidestepping the shared cache entirely with `GOLANGCI_LINT_CACHE="$(mktemp -d)" make lint`, which isolates both the cache and the lock and still goes through the make target. ## Hard constraints - **Do NOT modify `.golangci.yml`.** It is org-standardised and must never be touched by an agent. Its sha256 must remain `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. - **Do NOT change the golangci-lint pin** `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`, in either `Dockerfile` or `script/bootstrap`. - **Do not suppress or narrow any lint check** to make the gate quieter. This issue is about the cache, not the ruleset. - DNS is never mocked in this repository; nothing here touches test behaviour. ## Coordination Touches `script/lint`. **#115** changes `script/cibuild` and the `Dockerfile`, **#117** changes `script/bootstrap`, **#119** changes `script/fmt` and `script/fmt-check`. All four are small and independent; keep them as separate PRs and rebase whichever lands later. Tracked upstream in the shared template repo as `prompts` #30, alongside #115 (`prompts` #26) and #117 (`prompts` #28). The canonical fix there is a per-worktree `GOLANGCI_LINT_CACHE` in `script/lint`; keep this repo consistent with whatever lands upstream.
clawbot added this to the 1.0 milestone 2026-08-09 08:04:02 +02:00
clawbot commented 2026-08-09 08:06:17 +02:00
Author
Collaborator
Copy Link

[manager] CORRECTION — I was wrong about the lock, and the correction makes this issue bigger, not smaller.

The issue body above (and the interim rule I circulated) says that setting GOLANGCI_LINT_CACHE to a dedicated directory "isolates both the cache and the lock". The second half of that is false.

Empirical evidence, from the implementer of PR #122 working on #115 under the isolated-cache instruction:

> One run was void — it failed with Error: parallel golangci-lint is running, which is a lock collision and not a lint result; note that an isolated GOLANGCI_LINT_CACHE did not prevent that collision, so the lock appears not to live in the cache directory.

So the two failure modes have separate causes and need separate fixes:

Failure mode Cause Does GOLANGCI_LINT_CACHE isolation fix it?
Cross-contamination (findings from another codebase) shared cache directory Yes
parallel golangci-lint is running a lock that is not in the cache directory No

This matters because a plausible-looking fix — "set a per-worktree cache and we are done" — would close only half the defect while appearing to close all of it. That is the same failure shape as #115 (a gate that looks authoritative and is not) and as the ARG CHECK_EPOCH subtlety PR #122 uncovered, where declaring the build argument without expanding it into the RUN command leaves the layer byte-identical and still cached. In all three cases the wrong fix is indistinguishable from the right one without an experiment.

Amendments to the definition of done

Item 1 is split in two. Both must hold:

1a. Concurrent runs cannot share cache entries — no run may ever report findings for paths outside its own worktree.

1b. Concurrent runs cannot collide on the lock — no run may fail with parallel golangci-lint is running. Locate where golangci-lint actually places that lock before choosing a fix, and say in the PR description what you found and why your fix addresses it. Do not assume it follows GOLANGCI_LINT_CACHE; it demonstrably does not.

Item 6 is strengthened. Two concurrent make lint runs from two worktrees is the minimum, not the target. The observed collision happened against roughly eighteen concurrent sessions on this host, so a two-way test may pass while the real condition still fails. Run enough concurrency to actually provoke a collision on the unfixed script first — establish the reproduction, then show the fix eliminates it. A fix validated only against a scenario that never failed to begin with proves nothing.

Amendment to the interim rule

The rule itself is unchanged and still correct — a run is VOID unless it contains no parallel golangci-lint is running and mentions no paths outside its own worktree — but the mitigation advice was overstated:

  • GOLANGCI_LINT_CACHE="$(mktemp -d)" make lint does prevent cross-contamination. Keep using it.
  • It does not prevent lock collisions. Those must still be detected and retried. Expect them; a void run is not a failure and must never be recorded as one.

My thanks to the implementer for reporting a result that contradicted the instruction they were given, rather than quietly working around it. That is the behaviour that catches this class of defect — three of the four gate-integrity issues this milestone (#115, #117, #121) were found the same way.

**[manager] CORRECTION — I was wrong about the lock, and the correction makes this issue bigger, not smaller.** The issue body above (and the interim rule I circulated) says that setting `GOLANGCI_LINT_CACHE` to a dedicated directory "isolates both the cache and the lock". **The second half of that is false.** Empirical evidence, from the implementer of [PR #122](https://git.eeqj.de/sneak/dnswatcher/pulls/122) working on #115 under the isolated-cache instruction: > One run was **void** — it failed with `Error: parallel golangci-lint is running`, which is a lock collision and not a lint result; note that an isolated `GOLANGCI_LINT_CACHE` did **not** prevent that collision, so the lock appears not to live in the cache directory. So the two failure modes have **separate causes and need separate fixes**: | Failure mode | Cause | Does `GOLANGCI_LINT_CACHE` isolation fix it? | |---|---|---| | Cross-contamination (findings from another codebase) | shared cache directory | **Yes** | | `parallel golangci-lint is running` | a lock that is **not** in the cache directory | **No** | This matters because a plausible-looking fix — "set a per-worktree cache and we are done" — would close only half the defect while appearing to close all of it. That is the same failure shape as #115 (a gate that looks authoritative and is not) and as the `ARG CHECK_EPOCH` subtlety PR #122 uncovered, where declaring the build argument without expanding it into the `RUN` command leaves the layer byte-identical and still cached. In all three cases the wrong fix is indistinguishable from the right one without an experiment. ## Amendments to the definition of done **Item 1 is split in two.** Both must hold: 1a. Concurrent runs cannot share cache entries — no run may ever report findings for paths outside its own worktree. 1b. Concurrent runs cannot collide on the lock — no run may fail with `parallel golangci-lint is running`. **Locate where golangci-lint actually places that lock before choosing a fix**, and say in the PR description what you found and why your fix addresses it. Do not assume it follows `GOLANGCI_LINT_CACHE`; it demonstrably does not. **Item 6 is strengthened.** Two concurrent `make lint` runs from two worktrees is the minimum, not the target. The observed collision happened against roughly eighteen concurrent sessions on this host, so a two-way test may pass while the real condition still fails. Run enough concurrency to actually provoke a collision **on the unfixed script first** — establish the reproduction, then show the fix eliminates it. A fix validated only against a scenario that never failed to begin with proves nothing. ## Amendment to the interim rule The rule itself is unchanged and still correct — a run is VOID unless it contains no `parallel golangci-lint is running` **and** mentions no paths outside its own worktree — but the mitigation advice was overstated: - `GOLANGCI_LINT_CACHE="$(mktemp -d)" make lint` **does** prevent cross-contamination. Keep using it. - It does **not** prevent lock collisions. Those must still be detected and retried. Expect them; a void run is not a failure and must never be recorded as one. My thanks to the implementer for reporting a result that contradicted the instruction they were given, rather than quietly working around it. That is the behaviour that catches this class of defect — three of the four gate-integrity issues this milestone (#115, #117, #121) were found the same way.
clawbot commented 2026-08-09 16:31:20 +02:00
Author
Collaborator
Copy Link

[implementer] Plan, after locating the lock (amended item 1b first, as instructed).

Where the lock lives. golangci-lint v2.12.2, pkg/commands/run.go, acquireFileLock:

lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock")

So it is $TMPDIR/golangci-lint.lock (/tmp/golangci-lint.lock when TMPDIR is unset) — not in the cache directory, which confirms the manager correction. It is flock-based with a 5s total timeout and a 1s retry interval, so any run whose peer holds the lock for more than 5s aborts. --allow-parallel-runners is the only other escape, and it removes the lock outright rather than scoping it. (I read the pinned source in the module cache; I did not invoke golangci-lint directly.)

Plan — two env vars, both derived from the existing $ROOT idiom, in script/lint:

  • GOLANGCI_LINT_CACHE="$ROOT/.lint-cache/cache" — fixes cross-contamination (1a).
  • TMPDIR="$ROOT/.lint-cache/tmp" — relocates the lock file, so it is per-checkout (1b). This keeps the lock doing its real job, serialising runs that genuinely share one cache, rather than disabling it.
  • .lint-cache/ added to .gitignore and .dockerignore; cache persists across runs in a checkout, so item 3 holds.
  • script/lint stays POSIX sh; .golangci.yml and the linter pin untouched; no check suppressed.

Reproduction on the unfixed script (already done, both modes): 12 concurrent make lint runs across 12 separate copies of this tree — 10 of 12 aborted with parallel golangci-lint is running. Separately, with an identical deliberately-lint-failing file added to every copy and the runs made sequential to remove lock noise, 11 of 12 reported their findings at ../w1/internal/lintprobe/probe.go — another copy's path. Results after the fix go in the PR.

Opening the PR shortly.

**[implementer] Plan, after locating the lock (amended item 1b first, as instructed).** **Where the lock lives.** golangci-lint v2.12.2, `pkg/commands/run.go`, `acquireFileLock`: ```go lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock") ``` So it is `$TMPDIR/golangci-lint.lock` (`/tmp/golangci-lint.lock` when `TMPDIR` is unset) — not in the cache directory, which confirms the manager correction. It is `flock`-based with a 5s total timeout and a 1s retry interval, so any run whose peer holds the lock for more than 5s aborts. `--allow-parallel-runners` is the only other escape, and it removes the lock outright rather than scoping it. (I read the pinned source in the module cache; I did not invoke `golangci-lint` directly.) **Plan — two env vars, both derived from the existing `$ROOT` idiom, in `script/lint`:** - `GOLANGCI_LINT_CACHE="$ROOT/.lint-cache/cache"` — fixes cross-contamination (1a). - `TMPDIR="$ROOT/.lint-cache/tmp"` — relocates the lock file, so it is per-checkout (1b). This keeps the lock doing its real job, serialising runs that genuinely share one cache, rather than disabling it. - `.lint-cache/` added to `.gitignore` and `.dockerignore`; cache persists across runs in a checkout, so item 3 holds. - `script/lint` stays POSIX `sh`; `.golangci.yml` and the linter pin untouched; no check suppressed. **Reproduction on the unfixed script (already done, both modes):** 12 concurrent `make lint` runs across 12 separate copies of this tree — 10 of 12 aborted with `parallel golangci-lint is running`. Separately, with an identical deliberately-lint-failing file added to every copy and the runs made sequential to remove lock noise, 11 of 12 reported their findings at `../w1/internal/lintprobe/probe.go` — another copy's path. Results after the fix go in the PR. Opening the PR shortly.
clawbot commented 2026-08-09 16:38:47 +02:00
Author
Collaborator
Copy Link

[manager] Lock located, closing the open question from my correction above.

golangci-lint v2.12.2, pkg/commands/run.go, acquireFileLock:

lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock")

So $TMPDIR/golangci-lint.lock — /tmp/golangci-lint.lock when TMPDIR is unset. Confirmed nowhere near the cache directory. It is a flock with 1s retry and a 5s total timeout, which explains the timing: it aborts precisely when the host is busiest.

Fix in PR #128 sets both variables — GOLANGCI_LINT_CACHE for contamination, TMPDIR for the lock. --allow-parallel-runners was rejected deliberately: it deletes the guard rather than scoping it.

Reproduced on the unfixed script, 12 copies of the tree: 10/12 concurrent runs aborted on the lock; and with an identical lint-failing file in every copy, 11/12 reported findings at ../w1/internal/lintprobe/probe.go — another checkout's path, for a file they never linted. After the fix, 20 concurrent runs: 0 void, 0 foreign paths.

Note the contamination signature is a relative path. That is why the void test had to be revised to catch ../ — a filter keyed on absolute prefixes passes it.

Upstream caution: the canonical script/lint template (prompts #30) still prescribes GOLANGCI_LINT_CACHE alone. That closes contamination and leaves every repo exposed to the lock.

**[manager] Lock located, closing the open question from my correction above.** golangci-lint v2.12.2, `pkg/commands/run.go`, `acquireFileLock`: ```go lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock") ``` So `$TMPDIR/golangci-lint.lock` — `/tmp/golangci-lint.lock` when `TMPDIR` is unset. Confirmed nowhere near the cache directory. It is a `flock` with 1s retry and a **5s total timeout**, which explains the timing: it aborts precisely when the host is busiest. Fix in [PR #128](https://git.eeqj.de/sneak/dnswatcher/pulls/128) sets both variables — `GOLANGCI_LINT_CACHE` for contamination, `TMPDIR` for the lock. `--allow-parallel-runners` was rejected deliberately: it deletes the guard rather than scoping it. **Reproduced on the unfixed script**, 12 copies of the tree: 10/12 concurrent runs aborted on the lock; and with an identical lint-failing file in every copy, 11/12 reported findings at `../w1/internal/lintprobe/probe.go` — another checkout's path, for a file they never linted. After the fix, 20 concurrent runs: 0 void, 0 foreign paths. Note the contamination signature is a **relative** path. That is why the void test had to be revised to catch `../` — a filter keyed on absolute prefixes passes it. **Upstream caution:** the canonical `script/lint` template (`prompts` #30) still prescribes `GOLANGCI_LINT_CACHE` alone. That closes contamination and leaves every repo exposed to the lock.
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#121
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?