make check/CI never exercised the race detector over this PR's
concurrency (evictor goroutine, write-pressure channel, the new
per-hash contentLock, concurrent store/evict). Minimal, scoped change:
add -race to the existing go test invocation. Full suite is clean
under it (make check passes; go test -race ./... completes in under
7s, well inside the 30s timeout).
reconcileAccounting previously ran exactly once, when the evictor
goroutine started. Combined with StoreVariant's best-effort accounting
insert (warns and continues on failure), a long-running process could
accumulate untracked disk usage past cache_max_bytes indefinitely --
the disk-exhaustion failure mode issue #51 exists to close -- with
recovery gated on a process restart.
evictionLoop now also runs a reconciliation pass on every periodic
ticker tick (the same interval eviction itself uses; reconciliation
walks the cache directories so it deliberately does not run on every
write-pressure wakeup, to stay off the per-store hot path). This
bounds unaccounted drift to at most one eviction interval regardless
of how long the process has been running.
TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup proves it:
introduces an untracked variant file only after startup reconciliation
has already completed and asserts a later periodic pass adopts it.
reconcileAccounting currently runs exactly once, at evictor startup.
Combined with StoreVariant's best-effort accounting insert, a variant
file that lands on disk untracked during steady-state operation (e.g.
insert failed under transient DB contention) stays invisible to
UsageBytes/EvictToLimit until the next process restart -- the drift
window the review flagged. Currently red: a file introduced after
startup reconciliation has already run is never adopted.
StoreSource now hashes content itself and holds the per-hash
contentLock across the whole store (file write plus accounting row
inserts); evictSourceBlob holds the same lock across its whole
operation (row deletion transaction through file unlink). A concurrent
store and eviction of identical content bytes can no longer
interleave: either runs to completion before the other starts, so a
fresh row can never be left pointing at a file the other side is
mid-unlink on.
ContentStorage gains StoreHashed for callers that need the hash before
writing; Store is refactored to share the write-if-absent logic with
it, with no change to its existing behavior or signature.
internal/imgcache/eviction_test.go:
TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent proves it:
pauses eviction (via evictSourceBlobTestHook) in the exact window
between commit and unlink, asserts a concurrent StoreSource for
identical content blocks rather than completing, then verifies no
dangling reference and that the store's data survives once eviction
releases the hash.
Adds an instrumentation seam (evictSourceBlobTestHook, fired after the
row-deletion transaction commits and before the content file is
unlinked) and a test that pauses eviction there while a concurrent
StoreSource for identical content bytes races it. Currently red: the
store completes immediately instead of being excluded, which is
exactly the window the review flagged between evictSourceBlob's commit
and its unlink.
Introduces the keyed exclusion primitive that StoreSource and
evictSourceBlob will hold across their full operation, so a store and
an eviction racing on identical content bytes cannot interleave.
Covered here in isolation: same-key exclusion, independence across
distinct keys, and that the entry map does not grow unbounded.
Pre-1.0 with no installed base to migrate: REPO_POLICIES.md forbids
adding numbered migration files beyond 001 before a tagged release.
Fold the variant_content table and source_content.last_accessed_at
column (previously 002_cache_eviction.sql) directly into
001_initial_schema.sql and delete the 002 file. The migration runner
is generic over whatever *.sql files exist in schema/, so no runner
code changes are needed.
Add cache_max_bytes to config.example.yml and the README key settings
list. TODO.md: move cache size management and eviction to Completed
Steps, promote P1 blocked networks configuration into Next Step, and
note in Status that the unbounded disk growth DoS vector is closed.
Migration 002 adds a variant_content table (processed variants were
untracked on disk) and an LRU timestamp on source_content. Total usage
is two SUMs, never a directory scan on the hot path; hits touch LRU
timestamps best-effort. A background goroutine evicts globally
least-recently-used entries (variants and source blobs merged) until
usage is under MaxBytes, woken by a periodic ticker and by non-blocking
write-pressure notifications from stores. Evicting a source blob
deletes all source_metadata rows referencing it plus its
source_content row in one transaction before the file is unlinked, so
multi-referenced blobs are removed only with all their references and
rows never point at deleted files; JSON sidecars are cleaned up too. A
one-time startup reconciliation walk adopts untracked variant files,
drops rows whose files are missing, removes unreachable source blobs,
and sweeps stale temp files. CacheConfig.DisableDiskCache turns the
disk cache off entirely (config maps cache_max_bytes: 0 to it): no
directories, lookups miss, stores no-op, no evictor. Handlers wire the
limit, start eviction on startup, and stop it on shutdown.
Strict int64 parsing via the startup validation framework: a SET but
invalid value (negative, float, null, non-numeric) aborts startup
naming the key and value. An omitted key resolves after state_dir
validation to max(75% of free bytes on the filesystem containing
<state_dir>/cache/, 500 MiB), measured via an injectable statfs probe;
the floor never applies to explicit values. Zero is valid and means
the disk cache is disabled. The effective limit is logged at startup.
Red phase for #51: covers strict cache_max_bytes parsing (invalid
explicit values abort naming key and value), the computed default of
max(75% of free space, 500 MiB) via an injectable free-space probe,
explicit-value-no-floor, zero-disables-cache, size accounting over
source blobs and variants, LRU eviction under the limit, the
multi-referenced blob case, write-pressure and periodic eviction
triggers, and startup reconciliation. Minimal API skeletons keep the
tree compiling and lint-clean; only the new tests fail.
