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

build: isolate golangci-lint cache and lock per checkout (closes #121) #128

Open
clawbot wants to merge 1 commits from fix/121-lint-cache-isolation into next
pull from: fix/121-lint-cache-isolation
merge into: sneak:next
Conversation 3 Commits 1 Files Changed 5 +44 -1
clawbot commented 2026-08-09 16:36:08 +02:00
Collaborator
Copy Link

Where the lock lives

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

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

$TMPDIR/golangci-lint.lock, i.e. /tmp/golangci-lint.lock when TMPDIR is unset. Not in the cache directory, which is why an isolated GOLANGCI_LINT_CACHE did not stop the collision PR #122 hit. It is a flock with a 1s retry and a 5s total timeout, so any run whose peer holds it longer than 5s aborts.

Fix

script/lint sets both, derived from the existing $ROOT idiom:

  • GOLANGCI_LINT_CACHE="$ROOT/.lint-cache/cache" — cache is never shared, so no run can be served another checkout's findings (1a).
  • TMPDIR="$ROOT/.lint-cache/tmp" — moves the lock file per checkout (1b). This scopes the lock rather than removing it, so it still serialises runs that genuinely share one cache. --allow-parallel-runners was the alternative and was rejected: it deletes the guard instead of scoping it.

.lint-cache/ added to .gitignore and .dockerignore. .golangci.yml untouched (sha256 still 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb), linter pin untouched, no check suppressed, script/lint still POSIX sh and sh -n clean.

Verification

Reproduction used 12 (later 20) copies of this tree under /tmp, each a full checkout, running make lint concurrently.

Unfixed script:

  • Lock: 10 of 12 concurrent runs aborted with parallel golangci-lint is running.
  • Contamination: 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 checkout's path, for a file they did not lint. Only w1, which populated the shared cache, reported its own path.

Fixed script: 20 concurrent runs, cold caches — 0 void, 0 foreign paths. Repeating with the lint-failing file present, all 12 copies reported exactly their own internal/lintprobe/probe.go and no ../ path.

Docker: real build, docker build --no-cache-filter=builder ., chosen over script/cibuild because that takes no arguments and can report a false green on a byte-identical tree (#115). The builder stage executed rather than being reused (go install golangci-lint 53.8s, RUN make check 51.8s, not CACHED); lint reported 0 issues inside the container with no lock error. HOME and TMPDIR differ there and the fix does not depend on either — the path comes from the checkout root.

Timing: warm run ~1.7–2.2s, against ~0.8s before; the gap is host scheduling noise, not the change — user CPU time went down (2.0s vs 2.6s). The real cost is that the first run in a fresh checkout is now cold, ~17s, because the cache is no longer shared across checkouts. That is inherent to the fix. Cache is ~80MB per checkout.

make check green (GOFLAGS=-count=1), tree unmodified afterwards, TODO.md updated in the same commit.

Anomalies

  • The gomodguard deprecation warning still appears on every lint run. Pre-existing and already recorded as accepted in TODO.md.
  • The watcher tests log failed to save state error="writing temp state file: open /state.json.tmp: permission denied" repeatedly while passing. Pre-existing, unrelated to this change, and out of scope here — flagging it rather than filing, since I cannot tell whether it is deliberate error-path coverage or tests falling back to the default data directory.
## Where the lock lives golangci-lint v2.12.2 (`pkg/commands/run.go`, `acquireFileLock`): ```go lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock") ``` `$TMPDIR/golangci-lint.lock`, i.e. `/tmp/golangci-lint.lock` when `TMPDIR` is unset. Not in the cache directory, which is why an isolated `GOLANGCI_LINT_CACHE` did not stop the collision PR #122 hit. It is a `flock` with a 1s retry and a 5s total timeout, so any run whose peer holds it longer than 5s aborts. ## Fix `script/lint` sets both, derived from the existing `$ROOT` idiom: - `GOLANGCI_LINT_CACHE="$ROOT/.lint-cache/cache"` — cache is never shared, so no run can be served another checkout's findings (1a). - `TMPDIR="$ROOT/.lint-cache/tmp"` — moves the lock file per checkout (1b). This scopes the lock rather than removing it, so it still serialises runs that genuinely share one cache. `--allow-parallel-runners` was the alternative and was rejected: it deletes the guard instead of scoping it. `.lint-cache/` added to `.gitignore` and `.dockerignore`. `.golangci.yml` untouched (sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`), linter pin untouched, no check suppressed, `script/lint` still POSIX `sh` and `sh -n` clean. ## Verification Reproduction used 12 (later 20) copies of this tree under `/tmp`, each a full checkout, running `make lint` concurrently. **Unfixed script:** - Lock: 10 of 12 concurrent runs aborted with `parallel golangci-lint is running`. - Contamination: 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 checkout's path, for a file they did not lint. Only `w1`, which populated the shared cache, reported its own path. **Fixed script:** 20 concurrent runs, cold caches — 0 void, 0 foreign paths. Repeating with the lint-failing file present, all 12 copies reported exactly their own `internal/lintprobe/probe.go` and no `../` path. **Docker:** real build, `docker build --no-cache-filter=builder .`, chosen over `script/cibuild` because that takes no arguments and can report a false green on a byte-identical tree (#115). The builder stage executed rather than being reused (`go install golangci-lint` 53.8s, `RUN make check` 51.8s, not `CACHED`); lint reported `0 issues` inside the container with no lock error. `HOME` and `TMPDIR` differ there and the fix does not depend on either — the path comes from the checkout root. **Timing:** warm run ~1.7–2.2s, against ~0.8s before; the gap is host scheduling noise, not the change — user CPU time went down (2.0s vs 2.6s). The real cost is that the first run in a fresh checkout is now cold, ~17s, because the cache is no longer shared across checkouts. That is inherent to the fix. Cache is ~80MB per checkout. `make check` green (`GOFLAGS=-count=1`), tree unmodified afterwards, `TODO.md` updated in the same commit. ## Anomalies - The `gomodguard` deprecation warning still appears on every lint run. Pre-existing and already recorded as accepted in `TODO.md`. - The `watcher` tests log `failed to save state error="writing temp state file: open /state.json.tmp: permission denied"` repeatedly while passing. Pre-existing, unrelated to this change, and out of scope here — flagging it rather than filing, since I cannot tell whether it is deliberate error-path coverage or tests falling back to the default data directory.
clawbot added the needs-review label 2026-08-09 16:36:12 +02:00
clawbot self-assigned this 2026-08-09 16:36:13 +02:00
clawbot commented 2026-08-09 16:36:28 +02:00
Author
Collaborator
Copy Link

Built: script/lint now exports GOLANGCI_LINT_CACHE and TMPDIR under a git-ignored, Docker-ignored .lint-cache/ in the checkout root. The cache var closes the cross-contamination half; TMPDIR is the half that actually matters for the lock, because golangci-lint locks os.TempDir()/golangci-lint.lock, not anything under the cache directory.

Verified: reproduced both failure modes on the unfixed script (10 of 12 concurrent runs void with parallel golangci-lint is running; 11 of 12 runs reporting ../w1/... paths for a file they never linted), then 0 of 20 void and 0 foreign paths after. Real Docker build with the builder stage forced to execute, lint 0 issues inside the container. make check green with GOFLAGS=-count=1, tree unmodified after. Full detail and the timing numbers are in the PR description.

Two pre-existing anomalies are called out at the bottom of the PR description; neither is caused by this change.

Built: `script/lint` now exports `GOLANGCI_LINT_CACHE` and `TMPDIR` under a git-ignored, Docker-ignored `.lint-cache/` in the checkout root. The cache var closes the cross-contamination half; `TMPDIR` is the half that actually matters for the lock, because golangci-lint locks `os.TempDir()/golangci-lint.lock`, not anything under the cache directory. Verified: reproduced both failure modes on the unfixed script (10 of 12 concurrent runs void with `parallel golangci-lint is running`; 11 of 12 runs reporting `../w1/...` paths for a file they never linted), then 0 of 20 void and 0 foreign paths after. Real Docker build with the builder stage forced to execute, lint `0 issues` inside the container. `make check` green with `GOFLAGS=-count=1`, tree unmodified after. Full detail and the timing numbers are in the PR description. Two pre-existing anomalies are called out at the bottom of the PR description; neither is caused by this change.
clawbot commented 2026-08-09 16:47:46 +02:00
Author
Collaborator
Copy Link

Review: PASS

Independent review of 520ce79. No blocking findings.

Lock claim verified against the pinned source

The pin c0d3ddc9cf3faa61a4e378e879ece580256d76e5 resolves to v2.12.2 (@v/v2.12.2.info, Origin.Hash), and that source says exactly what the PR says: pkg/commands/run.go:492 lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock"), flock, retryDelay = time.Second (:495), totalTimeout = 5 * time.Second (:499). Path, mechanism and both timings are correct, and the lock is demonstrably not under GOLANGCI_LINT_CACHE.

Concurrency test (my own, 12 fresh copies of this tree)

Condition Result
Unfixed script, 12-way concurrent 11/12 void with parallel golangci-lint is running
Unfixed script, 12 sequential, identical planted lll violation in each copy 11/12 reported ../u4/internal/lintprobe/probe.go — another copy's path
Fixed script, 12-way concurrent, planted violation 12/12 reported only their own internal/lintprobe/probe.go; 0 lock errors, 0 foreign paths
Fixed 12 plus unfixed 12 running simultaneously (24-way) all 12 fixed runs rc=0, 0 issues., no lock error, no foreign path, while 9/12 unfixed peers went void

The reproduction lands before the fix and disappears after it, including with the shared /tmp lock actively thrashed by peers. Both halves of the amended definition of done are met.

Timings here: cold 8.1s, warm 1.3s, cache 80MB — consistent with the PR's numbers.

Everything else checked

make check rc=0 and git status --porcelain empty afterwards; .lint-cache/ git-ignored and Docker-ignored; docker build --progress=plain --no-cache-filter=builder --target builder . (direct docker, stated per policy) ran the builder for real — RUN make check DONE 38.6s, not CACHED, lint 0 issues inside the container with TMPDIR=/src/.lint-cache/tmp; .golangci.yml sha256 matches; pin unchanged in Dockerfile and script/bootstrap; no linter disabled or narrowed; no test file touched; POSIX sh, sh -n clean, no bashisms, $ROOT idiom kept; CI green on head (54s, real); base is current origin/main; commit title ends (closes #121) with TODO.md in the same commit; no Claude/Anthropic references or attribution trailers anywhere in 9347a28..520ce79.

Non-blocking findings

1. The lock still aborts rather than serialises, within a single checkout — and the script comment says otherwise. Two concurrent make lint in the same checkout with a cold cache: one exits rc=0, the other rc=2 with parallel golangci-lint is running (measured). Realistic trigger: script/precommit firing while a make check is already linting the same worktree — a cold run is 8-17s, well past the 5s timeout. This is much narrower than the pre-fix behaviour and is not a regression, but script/lint's comment ("keeps the lock doing its actual job (serialising runs that share one cache)") overstates it: after 5s the loser is aborted, not queued.

The lever that would make that sentence true is --allow-serial-runners, not --allow-parallel-runners: pkg/commands/flagsets.go:59 binds run.allow-serial-runners, and run.go:498 uses it to skip the 5s context.WithTimeout, so the second run blocks on the flock until the first releases instead of aborting. It keeps the guard, unlike --allow-parallel-runners. The PR description and issue comment 49933 both state that --allow-parallel-runners "is the only other escape" — that is incorrect and worth correcting in the record even if the flag is not adopted.

2. make clean does not remove .lint-cache/. Makefile:43 is rm -rf bin/. 80MB per checkout, no automatic trimming, on a host that spawns many throwaway worktrees. rm -rf bin/ .lint-cache/ would close it.

3. Naming nit: .lint-cache/cache stutters, and the directory holds tmp as well as a cache. Something like .lint-state/{cache,tmp} reads better. Not worth a respin on its own.

Anomaly that passes anyway

TMPDIR now points inside the tree being linted. That is safe only because the Go tool skips directories whose names begin with . when expanding ./... — confirmed: no run reported anything under .lint-cache/. It is a load-bearing property of the leading dot, so renaming the directory to lint-cache/ later would silently start feeding temp files to the linter. Worth a word in the comment if anyone touches the name.

The two pre-existing anomalies the author flagged (the gomodguard deprecation warning, and the watcher tests logging permission denied on /state.json.tmp while passing) both reproduce for me on unmodified main and inside the container; neither is caused by this change.

## Review: PASS Independent review of `520ce79`. No blocking findings. ### Lock claim verified against the pinned source The pin `c0d3ddc9cf3faa61a4e378e879ece580256d76e5` resolves to `v2.12.2` (`@v/v2.12.2.info`, `Origin.Hash`), and that source says exactly what the PR says: `pkg/commands/run.go:492` `lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock")`, `flock`, `retryDelay = time.Second` (:495), `totalTimeout = 5 * time.Second` (:499). Path, mechanism and both timings are correct, and the lock is demonstrably not under `GOLANGCI_LINT_CACHE`. ### Concurrency test (my own, 12 fresh copies of this tree) | Condition | Result | |---|---| | Unfixed script, 12-way concurrent | 11/12 void with `parallel golangci-lint is running` | | Unfixed script, 12 sequential, identical planted `lll` violation in each copy | 11/12 reported `../u4/internal/lintprobe/probe.go` — another copy's path | | Fixed script, 12-way concurrent, planted violation | 12/12 reported only their own `internal/lintprobe/probe.go`; 0 lock errors, 0 foreign paths | | Fixed 12 **plus** unfixed 12 running simultaneously (24-way) | all 12 fixed runs `rc=0`, `0 issues.`, no lock error, no foreign path, while 9/12 unfixed peers went void | The reproduction lands before the fix and disappears after it, including with the shared `/tmp` lock actively thrashed by peers. Both halves of the amended definition of done are met. Timings here: cold 8.1s, warm 1.3s, cache 80MB — consistent with the PR's numbers. ### Everything else checked `make check` `rc=0` and `git status --porcelain` empty afterwards; `.lint-cache/` git-ignored and Docker-ignored; `docker build --progress=plain --no-cache-filter=builder --target builder .` (direct docker, stated per policy) ran the builder for real — `RUN make check` `DONE 38.6s`, not `CACHED`, lint `0 issues` inside the container with `TMPDIR=/src/.lint-cache/tmp`; `.golangci.yml` sha256 matches; pin unchanged in `Dockerfile` and `script/bootstrap`; no linter disabled or narrowed; no test file touched; POSIX `sh`, `sh -n` clean, no bashisms, `$ROOT` idiom kept; CI green on head (54s, real); base is current `origin/main`; commit title ends ` (closes #121)` with `TODO.md` in the same commit; no Claude/Anthropic references or attribution trailers anywhere in `9347a28..520ce79`. ### Non-blocking findings **1. The lock still aborts rather than serialises, within a single checkout — and the script comment says otherwise.** Two concurrent `make lint` in the *same* checkout with a cold cache: one exits `rc=0`, the other `rc=2` with `parallel golangci-lint is running` (measured). Realistic trigger: `script/precommit` firing while a `make check` is already linting the same worktree — a cold run is 8-17s, well past the 5s timeout. This is much narrower than the pre-fix behaviour and is not a regression, but `script/lint`'s comment ("keeps the lock doing its actual job (serialising runs that share one cache)") overstates it: after 5s the loser is aborted, not queued. The lever that would make that sentence true is `--allow-serial-runners`, not `--allow-parallel-runners`: `pkg/commands/flagsets.go:59` binds `run.allow-serial-runners`, and `run.go:498` uses it to skip the 5s `context.WithTimeout`, so the second run blocks on the flock until the first releases instead of aborting. It keeps the guard, unlike `--allow-parallel-runners`. The PR description and issue comment 49933 both state that `--allow-parallel-runners` "is the only other escape" — that is incorrect and worth correcting in the record even if the flag is not adopted. **2. `make clean` does not remove `.lint-cache/`.** `Makefile:43` is `rm -rf bin/`. 80MB per checkout, no automatic trimming, on a host that spawns many throwaway worktrees. `rm -rf bin/ .lint-cache/` would close it. **3. Naming nit:** `.lint-cache/cache` stutters, and the directory holds `tmp` as well as a cache. Something like `.lint-state/{cache,tmp}` reads better. Not worth a respin on its own. ### Anomaly that passes anyway `TMPDIR` now points *inside* the tree being linted. That is safe only because the Go tool skips directories whose names begin with `.` when expanding `./...` — confirmed: no run reported anything under `.lint-cache/`. It is a load-bearing property of the leading dot, so renaming the directory to `lint-cache/` later would silently start feeding temp files to the linter. Worth a word in the comment if anyone touches the name. The two pre-existing anomalies the author flagged (the `gomodguard` deprecation warning, and the `watcher` tests logging `permission denied` on `/state.json.tmp` while passing) both reproduce for me on unmodified `main` and inside the container; neither is caused by this change.
clawbot added merge-ready and removed needs-review labels 2026-08-09 16:48:17 +02:00
clawbot removed their assignment 2026-08-09 16:48:17 +02:00
sneak was assigned by clawbot 2026-08-09 16:48:17 +02:00
clawbot commented 2026-08-09 16:48:30 +02:00
Author
Collaborator
Copy Link

[manager] Independent review PASS, no blocking findings. merge-ready, assigned to @sneak.

Lock path independently confirmed against the pinned source (run.go:492, flock, 1s retry, 5s timeout). Reviewer's own 24-way probe — 12 fixed and 12 unfixed runs launched together — had all 12 fixed runs clean while 9/12 unfixed peers went void, so the fix is immune to peers thrashing the shared /tmp lock. Constraints all verified clean.

One correction to record, because I propagated the error upstream. The PR body and my comment on prompts #30 both say --allow-parallel-runners was "the alternative". That is wrong: --allow-serial-runners also exists (flagsets.go:59, consumed at run.go:498 to skip the timeout), and it is the better lever for the residual case — it keeps the guard and makes overlapping runs queue rather than abort. I have corrected the upstream comment, since that repo is actively writing the canonical fix and would otherwise inherit the mistake.

Follow-ups filed as #130, neither blocking: two concurrent make lint within one checkout still abort after 5s rather than queueing (trigger: script/precommit overlapping a make check), and make clean does not remove .lint-cache/, which accumulates ~80MB per throwaway worktree unbounded.

Anomaly worth carrying forward: TMPDIR now lives inside the linted tree, and that is safe only because the Go tool skips dot-prefixed directories when expanding ./.... The leading dot is load-bearing — renaming to lint-cache/ would silently start feeding temp files to the linter.

**[manager]** Independent review **PASS**, no blocking findings. `merge-ready`, assigned to @sneak. Lock path independently confirmed against the pinned source (`run.go:492`, flock, 1s retry, 5s timeout). Reviewer's own 24-way probe — 12 fixed and 12 unfixed runs launched together — had all 12 fixed runs clean while 9/12 unfixed peers went void, so the fix is immune to peers thrashing the shared `/tmp` lock. Constraints all verified clean. **One correction to record, because I propagated the error upstream.** The PR body and my comment on [prompts #30](https://git.eeqj.de/sneak/prompts/issues/30) both say `--allow-parallel-runners` was "the alternative". That is wrong: **`--allow-serial-runners`** also exists (`flagsets.go:59`, consumed at `run.go:498` to skip the timeout), and it is the better lever for the residual case — it keeps the guard and makes overlapping runs *queue* rather than abort. I have corrected the upstream comment, since that repo is actively writing the canonical fix and would otherwise inherit the mistake. **Follow-ups filed as [#130](https://git.eeqj.de/sneak/dnswatcher/issues/130)**, neither blocking: two concurrent `make lint` *within one checkout* still abort after 5s rather than queueing (trigger: `script/precommit` overlapping a `make check`), and `make clean` does not remove `.lint-cache/`, which accumulates ~80MB per throwaway worktree unbounded. **Anomaly worth carrying forward:** `TMPDIR` now lives inside the linted tree, and that is safe *only* because the Go tool skips dot-prefixed directories when expanding `./...`. The leading dot is load-bearing — renaming to `lint-cache/` would silently start feeding temp files to the linter.
clawbot changed title from build: isolate golangci-lint cache and lock per checkout (closes #121) to WIP: build: isolate golangci-lint cache and lock per checkout (closes #121) 2026-08-10 14:39:43 +02:00
clawbot added needs-rebase and removed merge-ready labels 2026-08-10 14:41:18 +02:00
sneak was unassigned by clawbot 2026-08-10 14:41:27 +02:00
clawbot self-assigned this 2026-08-10 14:41:27 +02:00
clawbot changed title from WIP: build: isolate golangci-lint cache and lock per checkout (closes #121) to build: isolate golangci-lint cache and lock per checkout (closes #121) 2026-08-10 15:21:11 +02:00
clawbot changed target branch from main to next 2026-08-10 15:21:11 +02:00
clawbot added 1 commit 2026-08-10 15:21:11 +02:00
build: isolate golangci-lint cache and lock per checkout (closes #121)
All checks were successful
check / check (push) Successful in 54s
Details
520ce79709 
script/lint used golangci-lint's per-user global state, which breaks
when several checkouts on one host lint concurrently. Two independent
failure modes, two causes:

- Cross-contamination. The analysis cache (GOLANGCI_LINT_CACHE,
  default ~/.cache/golangci-lint) is keyed by content, not by
  checkout, so a hit written by another checkout is replayed with
  that checkout's file paths. A run reports findings for files it
  never linted.

- Lock collision. golangci-lint locks os.TempDir()/golangci-lint.lock
  (pkg/commands/run.go, acquireFileLock), which is NOT in the cache
  directory, with a 5s timeout. Peers that hold it longer make the
  run abort with "parallel golangci-lint is running" - a non-result
  that reads as a lint failure. Isolating the cache does not move it.

Point GOLANGCI_LINT_CACHE and TMPDIR at .lint-cache/ under the
checkout root, using the existing $ROOT idiom. TMPDIR is what makes
the lock per-checkout, so the lock keeps serialising the runs that
actually share a cache instead of being disabled. .lint-cache/ is
git-ignored and Docker-ignored, and the cache persists across runs in
a checkout, so caching is not lost.

Reproduced both modes on the unfixed script across 12 copies of this
tree: 10 of 12 concurrent runs void with the lock error, and 11 of 12
sequential runs reported findings at ../w1/... after an identical
lint-failing file was added to every copy. After the fix, 20-way
concurrency gives 0 void and 0 foreign paths, and each copy reports
only its own relative path.
All checks were successful
check / check (push) Successful in 54s
Details
This pull request has changes conflicting with the target branch.
  • README.md
  • TODO.md
  • script/lint
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/121-lint-cache-isolation:fix/121-lint-cache-isolation
git checkout fix/121-lint-cache-isolation
Sign in to join this conversation.
Reviewers
No Reviewers
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 needs-rebase
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#128
Delete Branch "fix/121-lint-cache-isolation"

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?