1 Commits

Author SHA1 Message Date
clawbot
62b31af5bd Give golangci-lint per-checkout cache and lock state (closes #30)
All checks were successful
check / check (push) Successful in 14s
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 18:15:24 +00:00

View File

@@ -774,11 +774,38 @@ style conventions are in separate documents:
# was closed. That case is not hypothetical here: two runs in the same # was closed. That case is not hypothetical here: two runs in the same
# checkout is exactly what --allow-serial-runners below exists to support, # checkout is exactly what --allow-serial-runners below exists to support,
# and serialising the linter does not serialise the shell's redirections # 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 # or the grep and cat that read them. mktemp rather than $$: two
# uses, for the same reason. # containerised runs over one bind-mounted checkout are in separate PID
LINT_OUT="$LINT_STATE/run.$$.stdout" # namespaces and can both be PID 7, which puts the collision back.
LINT_ERR="$LINT_STATE/run.$$.stderr" LINT_OUT="$(mktemp "$LINT_STATE/run.out.XXXXXX")"
trap 'rm -f "$LINT_OUT" "$LINT_ERR"' EXIT HUP INT TERM LINT_ERR="$(mktemp "$LINT_STATE/run.err.XXXXXX")"
# Print whatever the linter had written before the interruption, then exit
# 128+signal. The exit is what makes this handler TERMINATING, and that is
# the point: a signal-trap handler that returns RESUMES the script. With
# the capture files already deleted, execution would fall into the grep
# below against a missing file, take the not-a-collision branch, and report
# the FINDINGS exit status with empty output for a run that was killed —
# after deleting the findings it was about to print. That is why cleanup
# is on EXIT only. Killing a run is not hypothetical here: it is the stated
# mitigation for the unbounded wait --allow-serial-runners can produce, and
# it is what Ctrl-C on a make check does.
lint_interrupted() {
if [ -f "$LINT_ERR" ]; then cat "$LINT_ERR" >&2; fi
if [ -f "$LINT_OUT" ]; then cat "$LINT_OUT"; fi
echo "lint: interrupted by a signal, so nothing was completed." \
"This is NOT a lint result." >&2
exit "$1"
}
# The EXIT trap still fires on the way out of a terminating handler, so
# cleanup happens exactly once on every path. `|| :` because a failing rm
# — an unwritable state directory is enough — would otherwise change the
# exit status of an otherwise clean run under set -e.
trap 'rm -f "$LINT_OUT" "$LINT_ERR" || :' EXIT
trap 'lint_interrupted 129' HUP
trap 'lint_interrupted 130' INT
trap 'lint_interrupted 143' TERM
# Backstop for a caller that reached the linter without the environment # Backstop for a caller that reached the linter without the environment
# above. With it set, this should never fire. # above. With it set, this should never fire.
@@ -806,10 +833,12 @@ style conventions are in separate documents:
# this failure only on stderr, so a finding that quotes the string # this failure only on stderr, so a finding that quotes the string
# from source cannot be mistaken for a collision and retried away — # from source cannot be mistaken for a collision and retried away —
# that direction would be a false green. The exit status is not a # that direction would be a false green. The exit status is not a
# usable discriminator: the collision exits 3 (exitcodes.Failure) # usable discriminator: the collision exits 3 (exitcodes.Failure),
# while findings exit 1, and the other codes in pkg/exitcodes # which does separate it from findings at 1, but run.go returns it
# carry meanings of their own, so no exit status tells a collision # as a plain error that Execute maps to Failure like every other
# apart from a result. # error at that level, so 3 cannot separate a collision from a
# genuine linter failure — a broken config, an unparseable file.
# Retrying on 3 would retry real failures into a void.
if ! grep -q 'parallel golangci-lint is running' "$LINT_ERR"; then if ! grep -q 'parallel golangci-lint is running' "$LINT_ERR"; then
cat "$LINT_ERR" >&2 cat "$LINT_ERR" >&2
cat "$LINT_OUT" cat "$LINT_OUT"
@@ -882,10 +911,27 @@ style conventions are in separate documents:
found something, and a lock error erased before `grep` reads it, so the 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 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 reporting a result that is not its own, which is this bullet's entire
subject reintroduced one layer above where it was fixed. `$$` is subject reintroduced one layer above where it was fixed. Use `mktemp`
sufficient and is the same idiom the `CHECK_EPOCH` rule uses; `mktemp` under the state directory rather than `$$`: two containerised runs over
under the state directory is equally fine. The `trap` matters as much as one bind-mounted checkout sit in separate PID namespaces and can hold the
the paths, or the directory accumulates a pair per run forever. same low PID, which puts the collision back on exactly the fleet's
arrangement. `$$` is acceptable only where `mktemp` is unavailable and
that arrangement is ruled out.
- **Clean up on `EXIT` only, and give each signal a TERMINATING handler.** A
signal-trap handler that does not exit **resumes** the script: with the
capture files already deleted, the run falls into the `grep` against a
missing file, takes the not-a-collision branch, and reports the
**findings** exit status with empty output for a run that was killed —
having deleted the findings it was about to print. Measured: `TERM`, `INT`
and `HUP` all returning 1 with empty stdout, against 143, 130 and 129 for
the same block without the handler. Exit `128+signal` instead, after
printing what the linter had already written, and let the `EXIT` trap do
the cleanup on the way out. Killing a run is not a corner case here — it
is the stated mitigation for the unbounded wait `--allow-serial-runners`
can produce, and it is what Ctrl-C on a `make check` does. Put `|| :` on
the `rm`: an unwritable state directory makes it fail, and a failing
`EXIT` trap under `set -e` changes the exit status of an otherwise clean
run to 1, which was measured in both `dash` and `bash`.
Adopting repos must add `.lint-cache/` to both `.gitignore` and Adopting repos must add `.lint-cache/` to both `.gitignore` and
`.dockerignore`. The second matters as much as the first: the directory `.dockerignore`. The second matters as much as the first: the directory