1 Commits

Author SHA1 Message Date
clawbot
6ddf46e894 Give golangci-lint per-checkout cache and lock state (closes #30)
All checks were successful
check / check (push) Successful in 7s
A golangci-lint result on a host running many concurrent workers does not
reliably belong to the tree that asked for it. Two independent mechanisms,
which have repeatedly been mistaken for one:

The result cache is keyed on file content, not location, so two checkouts
of the same commit hold byte-identical files, share cache entries, and one
tree's findings are served for the other under the other tree's path. This
produced a confirmed false green as well as the loud false reds. Moving
workers from shared worktrees to their own clones does not address it —
two clones collide exactly as two worktrees did — and removes only the
foreign-path artefact that made the defect noticeable.

The concurrency lock is $TMPDIR/golangci-lint.lock (pkg/commands/run.go,
acquireFileLock), host-global and independent of GOLANGCI_LINT_CACHE, with
a five-second acquire timeout, so it fails when the host is busiest. A
private cache directory does not isolate it. Setting only the cache closes
the contamination half and leaves runs failing red on a condition that is
not a result at all.

REPO_POLICIES.md now carries the canonical Go script/lint: both variables
scoped into a .lint-cache/ directory inside the checkout, above any
container-versus-host branch so every path reaching the linter gets them;
--allow-serial-runners, which keeps the mutual-exclusion guard and queues
rather than aborting, for the same-checkout overlap TMPDIR scoping cannot
cover, with --allow-parallel-runners rejected because it deletes the
guard; and a bounded retry that treats the lock error as VOID rather than
as findings, exiting 75 on exhaustion so it is neither a pass nor a
failure. Detection is on the stderr stream and never on exit status:
findings go to stdout, so a finding quoting the lock message in source
cannot be retried away, and the exit status is not a stable discriminator
anyway. The interim void rule is recorded with the ../ clause that the
original filter missed, and with its limit stated — it catches
contamination that names foreign files, not contamination that suppresses
findings. Both checklists gained the corresponding items, since a half-fix
that sets only the cache reads as complete.

GOCACHE was measured rather than assumed and does not need isolating: with
the two variables scoped per checkout and GOCACHE shared at the host
default, each checkout reported its own paths.

Verified with the snippet extracted from the committed document and
executed as a consuming repo would adopt it, each control paired against
the pre-fix form: contamination reproduced on the pre-fix script and
absent on the adopted one; a stub linter colliding twice then clearing,
with the retry engaging and succeeding; exhaustion exiting 75 with a VOID
message; a genuine finding whose text quotes the lock message reported as
findings with no retry; and a real held lock failing the pre-fix script
with exit 3 while the adopted script, inheriting the same environment,
completed in one second.
2026-08-09 17:35:48 +00:00
2 changed files with 18 additions and 52 deletions

20
TODO.md
View File

@@ -38,18 +38,14 @@ fmt-check, and commit.
exits 75 with a VOID message rather than passing or failing quietly.
`--allow-serial-runners` (which keeps the guard and queues) covers the
same-checkout overlap that `TMPDIR` scoping cannot; `--allow-parallel-runners`
is rejected outright. The stdout and stderr capture files are per invocation
rather than per checkout, because serialising the linter does not serialise
the shell's redirections: two runs in one checkout — the overlap the flag
exists to support — would otherwise truncate and read each other's output,
which is the same defect one layer above where it was fixed. Both checklists
gained the corresponding items, since a half-fix that sets only the cache
reads as complete. `GOCACHE` was measured and does not need isolating.
Verified with the snippet extracted from the committed document and executed
as a consuming repo would adopt it, against paired controls: contamination
reproduced on the pre-fix form and absent on the adopted one, retry engaged,
exhaustion loud, a genuine finding still reported, and a held host lock
failing the pre-fix script while leaving the adopted one untouched.
is rejected outright. Both checklists gained the corresponding items, since a
half-fix that sets only the cache reads as complete. `GOCACHE` was measured
and does not need isolating. Verified with the snippet extracted from the
committed document and executed as a consuming repo would adopt it, against
paired controls: contamination reproduced on the pre-fix form and absent on
the adopted one, retry engaged, exhaustion loud, a genuine finding still
reported, and a held host lock failing the pre-fix script while leaving the
adopted one untouched.
- 2026-08-09: Kept in-repo agent scratch out of the Docker build context and out
of version control. `.claude/` holds one worktree — an entire additional
checkout of the repo — per in-flight agent, and under `COPY . .` all of it was

View File

@@ -766,20 +766,6 @@ style conventions are in separate documents:
export GOLANGCI_LINT_CACHE TMPDIR
mkdir -p "$GOLANGCI_LINT_CACHE" "$TMPDIR"
# Capture files, per INVOCATION and not per checkout. Two runs in the same
# checkout would otherwise redirect into one pair of fixed paths, opened
# O_TRUNC before the linter even starts, and each would print and scan the
# other's output — a run reporting a result that is not its own, which is
# the whole defect this bullet exists to close, one layer up from where it
# was closed. That case is not hypothetical here: two runs in the same
# checkout is exactly what --allow-serial-runners below exists to support,
# and serialising the linter does not serialise the shell's redirections
# or the grep and cat that read them. $$ is the same idiom CHECK_EPOCH
# uses, for the same reason.
LINT_OUT="$LINT_STATE/run.$$.stdout"
LINT_ERR="$LINT_STATE/run.$$.stderr"
trap 'rm -f "$LINT_OUT" "$LINT_ERR"' EXIT HUP INT TERM
# Backstop for a caller that reached the linter without the environment
# above. With it set, this should never fire.
LINT_MAX_ATTEMPTS=5
@@ -788,6 +774,8 @@ style conventions are in separate documents:
LINT_VOID_EXIT=75
golangci_lint_run() {
lint_out="$LINT_STATE/run.stdout"
lint_err="$LINT_STATE/run.stderr"
attempt=1
delay=2
while :; do
@@ -799,7 +787,7 @@ style conventions are in separate documents:
# the SAME checkout, which TMPDIR scoping cannot — script/precommit
# overlapping a make check is the realistic trigger.
golangci-lint run --allow-serial-runners "$@" \
>"$LINT_OUT" 2>"$LINT_ERR" || rc=$?
>"$lint_out" 2>"$lint_err" || rc=$?
# Detect the lock collision on the STDERR STREAM, never on the exit
# status. Findings are written to stdout and golangci-lint reports
@@ -807,17 +795,16 @@ style conventions are in separate documents:
# from source cannot be mistaken for a collision and retried away —
# that direction would be a false green. The exit status is not a
# usable discriminator: the collision exits 3 (exitcodes.Failure)
# while findings exit 1, and the other codes in pkg/exitcodes
# carry meanings of their own, so no exit status tells a collision
# apart from a result.
if ! grep -q 'parallel golangci-lint is running' "$LINT_ERR"; then
cat "$LINT_ERR" >&2
cat "$LINT_OUT"
# while findings exit 1, and field reports of 2 mean the value is
# not stable across versions.
if ! grep -q 'parallel golangci-lint is running' "$lint_err"; then
cat "$lint_err" >&2
cat "$lint_out"
return "$rc"
fi
if [ "$attempt" -ge "$LINT_MAX_ATTEMPTS" ]; then
cat "$LINT_ERR" >&2
cat "$lint_err" >&2
echo "lint: VOID after $LINT_MAX_ATTEMPTS attempts:" \
"golangci-lint never acquired its lock, so nothing was" \
"analyzed. This is NOT a lint result and no verdict may" \
@@ -871,21 +858,6 @@ style conventions are in separate documents:
the lock hangs the run instead of failing it; the contending set is
bounded to the same checkout, and an eventual result is preferable to a
fabricated one.
- **Capture stdout and stderr to per-INVOCATION paths, and clean them up.**
Two fixed paths under the checkout are one pair for every run in it, and
the redirections truncate them before the linter starts, so two
overlapping runs print and scan each other's output.
`--allow-serial-runners` does not prevent this — it serialises the linter,
not the shell — and the overlap it exists to support is precisely
`script/precommit` against a `make check` in one checkout. The observed
shapes are a run printing the other's `0 issues.` while its own linter
found something, and a lock error erased before `grep` reads it, so the
retry never fires and the void run returns as a result. Both are a run
reporting a result that is not its own, which is this bullet's entire
subject reintroduced one layer above where it was fixed. `$$` is
sufficient and is the same idiom the `CHECK_EPOCH` rule uses; `mktemp`
under the state directory is equally fine. The `trap` matters as much as
the paths, or the directory accumulates a pair per run forever.
Adopting repos must add `.lint-cache/` to both `.gitignore` and
`.dockerignore`. The second matters as much as the first: the directory
@@ -949,9 +921,7 @@ style conventions are in separate documents:
**State the limit of these tests rather than treating them as a guarantee.**
They catch contamination that **names** foreign files. They cannot catch
contamination that **suppresses** findings through a poisoned entry for
colliding content, which has no wall-clock tell either — **no evidence of
that mode has been observed, and nobody should go chasing it**; the point is
the reach of the tests, not a claim that the mode exists. They are a filter
colliding content, which has no wall-clock tell either. They are a filter
for the loud mode, not a proof of soundness — which is the whole argument
for fixing this in the tooling instead of documenting a discipline that
depends on every agent remembering to apply it.