allVariantKeys and allSourceContentHashes were byte-for-byte the same
query-scan-collect loop over a single string column, differing only in
the SQL, the result type and the noun in their error messages. Both now
delegate to a generic queryStringColumn helper.
Extracted rather than suppressed with a nolint: the duplication was
real, and the two call sites are the only places this shape appears.
Every error message is preserved verbatim, which is why the helper takes
both a plural and a singular noun.
- goconst: extracted testVariantKeyTwo alongside testVariantKeyOne.
- paralleltest: t.Parallel() on
TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent. The test
asserts that a concurrent store stays blocked for 200ms while eviction
holds the content lock; parallel load can only make it more blocked,
never less, so the assertion does not become timing-fragile. Verified
over repeated full -race runs.
- gosec G115: a live, justified suppression on the int64 conversion in
ComputeDefaultCacheMaxBytes. The preceding clamp was an explicit
if-statement that gosec could follow; the modernize linter requires it
to be min(), which gosec's range analysis cannot see through. The
clamp is still there and still correct, so the conversion cannot
overflow. Unlike the directives removed earlier in this branch, this
one is live: nolintlint confirms it suppresses a finding that is
actually raised.
The size-accounting INSERT in StoreVariant used db.Exec, and
StoreVariant took no context at all, so the write was uncancellable and
untraceable. Rather than paper over that with context.Background() at
the call site, StoreVariant now takes a context.Context and uses
ExecContext.
The plumbing is small: the only production caller is
Service.processAndStore, which already has the request context in
scope, so the accounting insert now shares the lifetime of the request
that produced the variant. Test callers pass t.Context().
The insert remains best-effort: a failure is logged and the startup and
periodic reconciliation passes still adopt any variant file whose
accounting row is missing.
Hand-reviewed rather than mechanical, because two of these functions
carried cleanup that depended on the shape being replaced.
internal/imgcache/storage.go, writeIfAbsent: dropped the named result
and the deferred temp-file cleanup that read it (nonamedreturns), in
favour of an explicit os.Remove(tmpPath) on each failing path. The
deferred form removed tmpPath whenever the function returned a non-nil
error, which is reachable on exactly three paths once the temp file
exists: Write, Close and Rename. Each of those now unlinks explicitly,
in the same order relative to tmpFile.Close(). The paths that must NOT
unlink are unchanged and still cannot: the content-already-present
early return, a MkdirAll failure and a CreateTemp failure all happen
before tmpPath exists, and the success path renames the temp file away.
This is the same cleanup shape MetadataStorage.Store and
VariantStorage.Store already use in this file. StoreHashed likewise
loses its named results.
internal/imgcache/eviction.go: converted 26 inline assignments. In
evictSourceBlob the conversions reuse the function-scope err that the
transaction already used; the rollback defer does not read it, and the
ordering of the delete transaction, its commit, the sidecar deletes and
the blob unlink is untouched. In the rows.Next() loops the scan error
is declared inside the loop body and rows.Err() is checked after it, as
before.
internal/config: the remaining conversions are in straight-line code
with no defer or named result.
No behavior changes. make test (with -race, per script/test) is green,
including TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent,
which exercises the commit-to-unlink window this cleanup protects.
No behavior changes. Covers the purely mechanical findings the canonical
v2.12.2 config raises on the cache-size/eviction work:
- nolintlint: deleted 7 dead //nolint:gosec directives (cachesize.go x2,
config.go, eviction.go x2, storage.go x2). gosec never raises G115/G703
on those lines under the pinned toolchain, exactly the defect class
that failed round 2 of this PR. The 6 live gosec suppressions are
untouched.
- funcorder: moved writeIfAbsent after Exists (storage.go) and
touchVariant/touchSourceContent after IncrementStats (cache.go).
- lll: wrapped over-length signatures, calls and messages at 88 columns.
- paralleltest: t.Parallel() on the new eviction, contentlock and
cache_max_bytes tests and their subtests. configFromYAML uses only
t.TempDir, so the config cases are parallel-safe.
- noctx: test helper DB calls now use ExecContext/QueryContext/
QueryRowContext with t.Context().
- goconst: extracted testHeaderContentType into the shared test constant
block and testVariantKeyOne into the eviction tests; reused the
existing testContentTypeJPEG and keyCacheMaxBytes constants.
- modernize: interface{} to any, atomic.Int32 for the contentlock
counters, min/max in ComputeDefaultCacheMaxBytes.
- intrange: integer range loops in the contentlock tests.
- wsl_v5: whitespace before the contentlock rendezvous statements.
- sloglint: slog.DiscardHandler in the config test logger.
- cyclop/funlen: split TestZeroMaxBytesDisablesDiskCache into four
assertion helpers and extracted the concurrent store goroutine from
TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent. Every
assertion is preserved verbatim; only their grouping changed.
Absorbs the cache size management and LRU eviction work (#55). All four
textual conflicts resolved in favor of main's implementation, with this
branch's mechanical lint conformance re-applied on top:
- internal/config/config.go: took main's cache_max_bytes wiring
(CacheMaxBytes, cacheMaxBytesExplicit) verbatim and expressed the key
through this branch's constant convention as keyCacheMaxBytes.
- internal/imgcache/cache.go: took main's disabled-cache guards, LRU
touch on lookup, and variant_content size accounting verbatim;
re-applied this branch's signature wrapping for lll.
- internal/imgcache/storage.go: took main's new writeIfAbsent helper and
its temp-file cleanup defer verbatim. The auto-merge had silently
dropped that defer in favor of this branch's inline cleanup form;
restored so the cleanup semantics that arrived from main are intact.
- TODO.md: kept both sides' Completed Steps entries.
.golangci.yml resolves to this branch's canonical version
(sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb).
make build and make test are green; #55's code is not yet conformant
with the canonical lint config, which the following commits address.
The 15 //nolint:gosec G703/G704 directives added in 13e9f2c suppressed
findings that only a stale local golangci-lint (v2.10.1) reports; the
repo's pinned golangci-lint:v2.12.2-alpine never raises gosec G703/G704
on these lines, so nolintlint correctly flagged every directive as an
unused suppression and failed the pinned lint build.
Verified with the authoritative docker build --target lint . using the
pinned image, and separately with a locally installed pinned v2.12.2
binary: both report 0 issues after removal.
The v2.12.2 gosec ruleset's new path-traversal (G703) and SSRF (G704)
taint checks flag os.Stat/os.Remove/os.Rename calls on paths that are
never attacker-controlled: our own temp files created immediately
before in the same function, content-hash- or cache-key-derived
storage paths, the operator-supplied config search path, and the
already SSRF-guarded upstream fetch (protected by ssrfSafeDialer at
the transport layer). Each suppression carries the rule ID and a
one-line justification, matching this repo's existing gosec nolint
convention in internal/imgcache/storage.go. No behavior change.
Insert the blank line wsl_v5 requires above four defer statements
whose deferred closure does not share a variable with the immediately
preceding line, across internal/handlers, internal/httpfetcher, and
internal/imgcache. No behavior change.
The stricter canonical .golangci.yml surfaced 81 findings in the
config validation code merged from main (#53). Fix them all with no
behavior change: static sentinel errors wrapped with %w preserving the
existing messages (err113), config key name constants (goconst),
t.Parallel() throughout except the Setenv/Chdir test (paralleltest),
white-box test renamed to config_validation_internal_test.go
(testpackage), case tables extracted into builder functions plus a
shared runAbortCases helper (funlen/dupl/gochecknoglobals), plain
error assignments (noinlineerr), any instead of interface{} and
strings.SplitSeq (modernize), slog.DiscardHandler (sloglint), 88-col
wrapping (lll), and removal of two stale nolint:gosec directives
(nolintlint).
Resolves conflicts with the startup config validation from #53:
internal/config/config.go takes main's validation implementation
wholesale, with getStringSlice mechanically adapted to this branch's
keyless signature; TODO.md keeps both Completed Steps entries.
Replace .golangci.yml with the canonical v2-schema config
(default: all minus six disabled linters, lll 88, tests included)
and bump every golangci-lint pin to v2.12.2:
- Dockerfile: golangci/golangci-lint:v2.12.2-alpine (hash-pinned)
- script/bootstrap: GOLANGCI_LINT_VERSION 2.12.2 with new
linux-amd64/arm64 release-archive sha256 pins
Fix all 747 findings the stricter config surfaces, with no behavior
changes: t.Parallel() throughout the test suite, static sentinel
errors and errors.Is comparisons, checked error returns, context
propagation (contextcheck/noctx), 88-column wrapping, extracted
constants and helpers for goconst/dupl/funlen/cyclop, exhaustive
switch cases replicating existing defaults, and white-box test files
renamed to *_internal_test.go for testpackage. Three
nolint:tagliatelle directives preserve the existing snake_case JSON
wire and on-disk metadata formats.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
