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

@@ -790,20 +790,11 @@ style conventions are in separate documents:
# 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.
#
# All three writes are best-effort, and the `|| :` on each is load-bearing
# rather than defensive habit. Under set -e a failed write aborts the
# function BEFORE exit "$1", and the shell then exits 1 — the findings
# status, on a run that analysed nothing. SIGHUP is precisely the case
# where writing fails: once the controlling terminal is gone the writes
# return EIO. Guarding only the two cats is not enough, because with the
# capture files empty the cats write nothing and succeed, and the echo is
# what fails.
lint_interrupted() {
if [ -f "$LINT_ERR" ]; then cat "$LINT_ERR" >&2 || :; fi
if [ -f "$LINT_OUT" ]; then cat "$LINT_OUT" || :; fi
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 || :
"This is NOT a lint result." >&2
exit "$1"
}
@@ -846,9 +837,7 @@ style conventions are in separate documents:
# which does separate it from findings at 1, but run.go returns it
# as a plain error that Execute maps to Failure like every other
# error at that level, so 3 cannot separate a collision from a
# genuine linter failure — an unknown linter name, an unknown flag,
# malformed config YAML all exit 3 too. (An unparseable Go source
# file does not: that is reported as typecheck issues and exits 1.)
# 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
cat "$LINT_ERR" >&2
@@ -935,34 +924,14 @@ style conventions are in separate documents:
**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, and let the
`EXIT` trap do the cleanup on the way out. The handler also makes a
**best-effort** attempt to print what the linter had already written —
best-effort because under `SIGHUP` the terminal is typically gone and
every write returns `EIO`, in which case nothing is printed and only the
status carries the message. Each of those writes needs its own `|| :`:
under `set -e` a failed write aborts the handler before it reaches `exit`,
and the shell then exits 1, which is the findings status on a run that
analysed nothing. Guarding only the `cat`s is not enough — with the
capture files empty they write nothing and succeed, and the `echo` is what
fails. Put `|| :` on the `rm` in the `EXIT` trap for the same reason: an
unwritable state directory makes it fail, and a failing `EXIT` trap under
`set -e` turns an otherwise clean run into exit 1, measured in both `dash`
and `bash`.
- **A signal must reach the LINTER, not just the wrapper.** POSIX defers a
trap until the running foreground command completes, so
`kill -TERM <wrapper pid>` does nothing at all while `golangci-lint` is
running — measured still alive three seconds later, where the same block
without a handler dies immediately at 143. Ctrl-C is unaffected because
the terminal signals the whole process group. This matters exactly where
the handler is supposed to help: the unbounded `--allow-serial-runners`
wait, where the process holding things up is the linter itself. Kill the
group (`kill -- -<pgid>`) or use Ctrl-C.
- **Known, accepted gap:** a signal arriving between the `mktemp` calls and
the `trap ... EXIT` line leaves the two capture files behind. Closing it
needs a trap installed before the files have names and rewritten after,
which is more moving parts than a couple of stray files in a gitignored
directory is worth. Stated rather than silently left.
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
`.dockerignore`. The second matters as much as the first: the directory