8 Commits

Author SHA1 Message Date
bdae9cb86b fix: run the test suite with -race
All checks were successful
check / check (push) Successful in 2m43s
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).
2026-08-09 00:48:49 +00:00
e7964fe777 fix: run accounting reconciliation periodically, not just at startup
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.
2026-08-09 00:48:03 +00:00
41347a7e9f test: add failing test for one-shot-only reconciliation
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.
2026-08-09 00:47:18 +00:00
9197b6300a fix: close TOCTOU window between blob eviction commit and unlink
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.
2026-08-09 00:46:45 +00:00
90b2f6fa66 test: add failing test for evictSourceBlob unlink-vs-store TOCTOU
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.
2026-08-09 00:45:39 +00:00
ea7621de29 test: add contentLock, a per-key mutex for content-hash exclusion
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.
2026-08-09 00:44:46 +00:00
314ccbcd9d docs: remove stale migration-002 reference from TODO.md
Follows the schema fold: the variant_content table and last_accessed_at
column are now part of 001_initial_schema.sql, not a separate migration.
2026-08-09 00:43:49 +00:00
6b0870d3c9 fix: fold cache eviction schema into 001_initial_schema.sql
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.
2026-08-09 00:43:21 +00:00
10 changed files with 525 additions and 64 deletions

25
TODO.md
View File

@@ -28,19 +28,18 @@ P1: implement blocked networks configuration to extend SSRF protection
framework (explicit values used exactly with no floor, `0` disables
the disk cache entirely, omitted defaults to max(75% of free space
on the filesystem containing `<state_dir>/cache/`, 500 MiB), logged
at startup); processed variants are now tracked in the database
(migration 002 adds `variant_content` and an LRU timestamp on
`source_content`) so total usage is two SUMs, never a directory scan
on the hot path; a background goroutine evicts globally
least-recently-used entries (variants and source blobs merged) to
the limit, woken by a periodic ticker and by write-pressure
notifications from stores; a source blob and ALL of its
`source_metadata` references are deleted in one transaction before
the file is unlinked, so multi-referenced blobs are never removed
while referenced and rows never point at deleted files; a startup
reconciliation pass adopts untracked variant files, drops rows for
missing files, removes unreachable source blobs, and sweeps stale
temp files
at startup); processed variants are now tracked in the database (a
new `variant_content` table and an LRU timestamp on `source_content`)
so total usage is two SUMs, never a directory scan on the hot path; a
background goroutine evicts globally least-recently-used entries
(variants and source blobs merged) to the limit, woken by a periodic
ticker and by write-pressure notifications from stores; a source
blob and ALL of its `source_metadata` references are deleted in one
transaction before the file is unlinked, so multi-referenced blobs
are never removed while referenced and rows never point at deleted
files; a startup and periodic reconciliation pass adopts untracked
variant files, drops rows for missing files, removes unreachable
source blobs, and sweeps stale temp files
- 2026-08-07 validate configuration on startup, fail fast on bad
config (closes #52): a config value that is set but unparseable or
invalid aborts startup naming the key and value (defaults apply only

View File

@@ -3,12 +3,17 @@
-- Source content blobs
-- Files stored at: cache/src-content/<ab>/<cd>/<sha256>
-- last_accessed_at is NULL until the first LRU touch; eviction falls
-- back to fetched_at for rows that have never been touched.
CREATE TABLE IF NOT EXISTS source_content (
content_hash TEXT PRIMARY KEY,
content_type TEXT NOT NULL,
size_bytes INTEGER NOT NULL,
fetched_at DATETIME DEFAULT CURRENT_TIMESTAMP
fetched_at DATETIME DEFAULT CURRENT_TIMESTAMP,
last_accessed_at DATETIME
);
CREATE INDEX IF NOT EXISTS idx_source_content_last_accessed
ON source_content(last_accessed_at);
-- Source URL metadata - maps URLs to content hashes
-- JSON stored at: cache/src-metadata/<hostname>/<path_hash>.json
@@ -34,6 +39,22 @@ CREATE INDEX IF NOT EXISTS idx_source_meta_path_hash ON source_metadata(path_has
CREATE INDEX IF NOT EXISTS idx_source_meta_expires ON source_metadata(expires_at);
CREATE INDEX IF NOT EXISTS idx_source_meta_content_hash ON source_metadata(content_hash);
-- Processed variant blobs
-- Files stored at: cache/variants/<ab>/<cd>/<cache_key> (plus a .meta
-- sidecar with the content type). Tracked here (like source content
-- blobs above) so total cache usage can be computed with a SUM query,
-- never a directory scan, and so LRU eviction has a timestamp to order
-- on.
CREATE TABLE IF NOT EXISTS variant_content (
cache_key TEXT PRIMARY KEY,
size_bytes INTEGER NOT NULL,
content_type TEXT NOT NULL,
created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
last_accessed_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP
);
CREATE INDEX IF NOT EXISTS idx_variant_content_last_accessed
ON variant_content(last_accessed_at);
-- Output/transformed content blobs
-- Files stored at: cache/dst-content/<ab>/<cd>/<sha256>
CREATE TABLE IF NOT EXISTS output_content (

View File

@@ -1,25 +0,0 @@
-- Migration 002: cache size accounting and eviction
--
-- Tracks processed variants in the database (source content blobs are
-- already tracked in source_content) so total cache usage can be
-- computed without directory scans, and adds last-access timestamps
-- for LRU eviction ordering.
-- Processed variant blobs
-- Files stored at: cache/variants/<ab>/<cd>/<cache_key> (plus a
-- .meta sidecar with the content type)
CREATE TABLE IF NOT EXISTS variant_content (
cache_key TEXT PRIMARY KEY,
size_bytes INTEGER NOT NULL,
content_type TEXT NOT NULL,
created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
last_accessed_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP
);
CREATE INDEX IF NOT EXISTS idx_variant_content_last_accessed
ON variant_content(last_accessed_at);
-- LRU timestamp for source content blobs. Rows written before this
-- migration have NULL here; eviction falls back to fetched_at.
ALTER TABLE source_content ADD COLUMN last_accessed_at DATETIME;
CREATE INDEX IF NOT EXISTS idx_source_content_last_accessed
ON source_content(last_accessed_at);

View File

@@ -2,7 +2,9 @@ package imgcache
import (
"context"
"crypto/sha256"
"database/sql"
"encoding/hex"
"encoding/json"
"errors"
"fmt"
@@ -76,6 +78,19 @@ type Cache struct {
// In-memory cache of variant metadata (content type, size) to avoid reading .meta files
metaCache map[VariantKey]variantMeta
// contentLocks serializes StoreSource and evictSourceBlob per
// content hash, closing the race window between an eviction's row
// deletion and its file unlink against a concurrent store of
// identical content.
contentLocks *contentLock
// evictSourceBlobTestHook, when set, is invoked by evictSourceBlob
// after its row-deletion transaction commits and before the
// content file is unlinked. It exists solely so tests can
// deterministically pause inside that window to exercise
// concurrent stores against it; production code leaves it nil.
evictSourceBlobTestHook func(ContentHash)
}
// NewCache creates a new cache instance.
@@ -94,6 +109,7 @@ func NewCache(db *sql.DB, config CacheConfig) (*Cache, error) {
evictionStop: make(chan struct{}),
evictionDone: make(chan struct{}),
metaCache: make(map[VariantKey]variantMeta),
contentLocks: newContentLock(),
}
if c.disabled {
@@ -201,8 +217,33 @@ func (c *Cache) StoreSource(
return "", nil
}
// Store content
contentHash, size, err := c.srcContent.Store(content)
// Hash the content ourselves (rather than via srcContent.Store,
// which would hash internally) so the content hash is known before
// any file or database work happens: that lets the entire store be
// serialized, per hash, against a concurrent eviction of the same
// content below.
data, err := io.ReadAll(content)
if err != nil {
return "", fmt.Errorf("failed to read source content: %w", err)
}
sum := sha256.Sum256(data)
contentHash := ContentHash(hex.EncodeToString(sum[:]))
// Hold the content hash's lock for the whole store operation. A
// concurrent eviction of this exact hash (the real SHA-256 dedup
// case: a different source path whose bytes hash identically)
// deletes the accounting rows and unlinks the file inside the same
// lock, so the two can never interleave: either this store
// completes first (and a subsequent eviction removes it together
// with its rows and file, correctly), or eviction completes first
// (and this store finds the file already gone and recreates it
// fresh) — never a fresh row left pointing at a file eviction is
// mid-unlink on.
unlock := c.contentLocks.Lock(string(contentHash))
defer unlock()
size, err := c.srcContent.StoreHashed(contentHash, data)
if err != nil {
return "", fmt.Errorf("failed to store source content: %w", err)
}

View File

@@ -0,0 +1,64 @@
package imgcache
import "sync"
// contentLock provides per-key mutual exclusion for content-hash keyed
// operations. StoreSource and evictSourceBlob each hold a content
// hash's lock for the full duration of their file-plus-accounting-row
// work, so a store and an eviction racing on identical content bytes
// (the real SHA-256 content-addressed dedup case, not a contrived one)
// can never interleave: the unlink of an evicted blob's file can never
// race the creation of a fresh database row for a concurrently
// re-stored copy of the same content. Entries are removed once no
// goroutine holds or is waiting for them, so a long-running process
// does not accumulate memory proportional to the number of distinct
// content hashes it has ever seen.
type contentLock struct {
mu sync.Mutex
entries map[string]*contentLockEntry
}
// contentLockEntry is one key's exclusion lock plus a count of
// goroutines currently holding or waiting to acquire it, used to know
// when it is safe to remove the entry from the map.
type contentLockEntry struct {
mu sync.Mutex
count int
}
// newContentLock creates an empty contentLock.
func newContentLock() *contentLock {
return &contentLock{entries: make(map[string]*contentLockEntry)}
}
// Lock acquires exclusive access for key, blocking until it is
// available, and returns a function that releases it. The caller must
// invoke the returned function exactly once to release the lock.
func (c *contentLock) Lock(key string) func() {
c.mu.Lock()
entry, ok := c.entries[key]
if !ok {
entry = &contentLockEntry{}
c.entries[key] = entry
}
entry.count++
c.mu.Unlock()
entry.mu.Lock()
return func() {
entry.mu.Unlock()
c.mu.Lock()
entry.count--
if entry.count == 0 {
delete(c.entries, key)
}
c.mu.Unlock()
}
}

View File

@@ -0,0 +1,133 @@
package imgcache
import (
"sync"
"sync/atomic"
"testing"
"time"
)
// TestContentLockExcludesSameKey verifies that two goroutines locking
// the same key never run their critical sections concurrently.
func TestContentLockExcludesSameKey(t *testing.T) {
lock := newContentLock()
var (
active int32
maxSeen int32
wg sync.WaitGroup
)
const goroutines = 20
wg.Add(goroutines)
for i := 0; i < goroutines; i++ {
go func() {
defer wg.Done()
unlock := lock.Lock("same-key")
defer unlock()
n := atomic.AddInt32(&active, 1)
for {
seen := atomic.LoadInt32(&maxSeen)
if n <= seen || atomic.CompareAndSwapInt32(&maxSeen, seen, n) {
break
}
}
time.Sleep(time.Millisecond)
atomic.AddInt32(&active, -1)
}()
}
wg.Wait()
if maxSeen != 1 {
t.Errorf("max concurrent holders of the same key = %d, want 1", maxSeen)
}
}
// TestContentLockAllowsDifferentKeys verifies that locking distinct
// keys does not serialize unrelated work: all goroutines must be able
// to enter their critical sections at once, proven by every one of
// them reaching the rendezvous point before any is allowed to
// proceed.
func TestContentLockAllowsDifferentKeys(t *testing.T) {
lock := newContentLock()
const goroutines = 20
var (
wg sync.WaitGroup
inside int32
reached = make(chan struct{}, goroutines)
)
wg.Add(goroutines)
release := make(chan struct{})
for i := 0; i < goroutines; i++ {
key := string(rune('a' + i))
go func() {
defer wg.Done()
unlock := lock.Lock(key)
defer unlock()
atomic.AddInt32(&inside, 1)
reached <- struct{}{}
<-release
}()
}
// Every goroutine must reach the rendezvous point (i.e. acquire its
// own key's lock) without needing any other to release first. If
// keys were incorrectly serialized onto one underlying lock, only
// one would get here and this would time out.
for i := 0; i < goroutines; i++ {
select {
case <-reached:
case <-time.After(2 * time.Second):
t.Fatalf("only %d/%d goroutines locking distinct keys made progress; "+
"keys may be incorrectly serialized", i, goroutines)
}
}
if n := atomic.LoadInt32(&inside); n != goroutines {
t.Errorf("goroutines inside their critical section = %d, want %d", n, goroutines)
}
close(release)
wg.Wait()
}
// TestContentLockRemovesEntryAfterUnlock verifies that the internal
// entries map does not grow without bound: once no goroutine holds or
// awaits a key, its entry is removed.
func TestContentLockRemovesEntryAfterUnlock(t *testing.T) {
lock := newContentLock()
unlock := lock.Lock("k")
lock.mu.Lock()
if _, ok := lock.entries["k"]; !ok {
lock.mu.Unlock()
t.Fatal("entry missing while lock is held")
}
lock.mu.Unlock()
unlock()
lock.mu.Lock()
defer lock.mu.Unlock()
if _, ok := lock.entries["k"]; ok {
t.Error("entry for key still present after the last holder unlocked")
}
}

View File

@@ -291,7 +291,18 @@ type sourceReference struct {
// removed together with all of its references, and database rows never
// point at deleted files. The JSON metadata sidecars for the removed
// rows are deleted afterwards.
//
// The whole operation holds the content hash's lock (the same one
// StoreSource holds for its full store), so a concurrent store of
// identical content bytes can never observe the file gone but a row
// still present, or insert a fresh row between this transaction's
// commit and the file unlink below: it either runs entirely before
// this eviction starts, or is blocked until this eviction (row
// deletion and unlink together) has fully completed.
func (c *Cache) evictSourceBlob(ctx context.Context, contentHash ContentHash) error {
unlock := c.contentLocks.Lock(string(contentHash))
defer unlock()
references, err := c.sourceReferences(ctx, contentHash)
if err != nil {
return err
@@ -318,6 +329,10 @@ func (c *Cache) evictSourceBlob(ctx context.Context, contentHash ContentHash) er
return fmt.Errorf("failed to commit eviction transaction: %w", err)
}
if c.evictSourceBlobTestHook != nil {
c.evictSourceBlobTestHook(contentHash)
}
// Only after the rows are gone may the files be removed.
for _, reference := range references {
if err := c.srcMetadata.Delete(reference.host, reference.pathHash); err != nil {
@@ -384,10 +399,11 @@ func (c *Cache) notifyWritePressure() {
}
// StartEviction launches the background eviction goroutine, which
// reconciles the database accounting with the cache directories once
// at startup and then evicts to the configured limit on the given
// periodic interval and on write-pressure notifications. It is a
// no-op on a disabled cache or when already started.
// reconciles the database accounting with the cache directories at
// startup and again on every periodic tick thereafter, and evicts to
// the configured limit on the given periodic interval and on
// write-pressure notifications. It is a no-op on a disabled cache or
// when already started.
func (c *Cache) StartEviction(interval time.Duration) {
if c.disabled || c.evictionStarted {
return
@@ -418,10 +434,7 @@ func (c *Cache) evictionLoop(interval time.Duration) {
ctx := context.Background()
if err := c.reconcileAccounting(ctx); err != nil {
c.log.Warn("cache accounting reconciliation failed", "error", err)
}
c.runReconciliationPass(ctx)
c.runEvictionPass(ctx)
ticker := time.NewTicker(interval)
@@ -432,6 +445,16 @@ func (c *Cache) evictionLoop(interval time.Duration) {
case <-c.evictionStop:
return
case <-ticker.C:
// Reconciliation walks the cache directories, so it only
// runs on the periodic ticker rather than on every
// write-pressure wakeup, keeping it off the per-store hot
// path. Reusing the eviction interval itself (rather than a
// separate, longer one) is a deliberate choice: it is the
// simplest option that still bounds how long a store's
// best-effort accounting insert can stay silently
// unaccounted for to one interval, on a process that is
// already running this loop regardless.
c.runReconciliationPass(ctx)
case <-c.evictionPressure:
}
@@ -447,13 +470,25 @@ func (c *Cache) runEvictionPass(ctx context.Context) {
}
}
// runReconciliationPass runs one reconciliation pass, logging failures
// instead of propagating them (the loop must keep running).
func (c *Cache) runReconciliationPass(ctx context.Context) {
if err := c.reconcileAccounting(ctx); err != nil {
c.log.Warn("cache accounting reconciliation failed", "error", err)
}
}
// reconcileAccounting synchronizes the database size accounting with
// the actual contents of the cache directories. It runs once when the
// background evictor starts, off the request hot path: it adopts
// variant files that predate the accounting table, drops accounting
// rows whose files are missing, removes source blob files the database
// does not know (and rows whose files are gone), and sweeps stale temp
// files left behind by crashed writes.
// the actual contents of the cache directories. It runs at startup and
// again on every periodic eviction tick thereafter, off the request
// hot path: it adopts variant files that predate the accounting table
// (or whose accounting insert failed, e.g. StoreVariant's best-effort
// insert under transient DB contention), drops accounting rows whose
// files are missing, removes source blob files the database does not
// know (and rows whose files are gone), and sweeps stale temp files
// left behind by crashed writes. Running it periodically, not just
// once, bounds how long such drift can accumulate unaccounted for on a
// long-running process to one eviction interval.
func (c *Cache) reconcileAccounting(ctx context.Context) error {
if c.disabled {
return nil

View File

@@ -667,3 +667,171 @@ func TestStartEvictionReconcilesAccountingWithDisk(t *testing.T) {
t.Errorf("stale accounting row without a file was not dropped (rows=%d)", n)
}
}
// TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup proves
// reconciliation is not a one-shot startup-only pass: it must also run
// on the periodic ticker, so a variant file that lands on disk with no
// accounting row well after startup (e.g. because StoreVariant's
// best-effort accounting insert failed under transient contention, or
// any other cause of an untracked file appearing during steady-state
// operation) is still adopted into accounting eventually, rather than
// staying invisible to UsageBytes/EvictToLimit until the next process
// restart.
func TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup(t *testing.T) {
cache, _ := newEvictionTestCache(t, 1<<30)
const interval = 100 * time.Millisecond
cache.StartEviction(interval)
defer cache.StopEviction()
// Let startup reconciliation run and settle on an empty cache
// before introducing the untracked file, so the adoption we assert
// below can only be the work of a later, periodic pass.
time.Sleep(3 * interval)
// Simulate a variant whose accounting insert failed after the
// process was already running and serving requests: the content
// file is written directly, bypassing StoreVariant's (and thus its
// accounting insert) entirely, exactly as would happen if that
// insert had failed and only the file write had succeeded.
untracked := bytes.Repeat([]byte{0x41}, 900)
if _, err := cache.variants.Store("aabbccdd0099", bytes.NewReader(untracked), "image/webp"); err != nil {
t.Fatalf("failed to store untracked variant file: %v", err)
}
deadline := time.Now().Add(5 * time.Second)
var usage int64
for time.Now().Before(deadline) {
var err error
usage, err = cache.UsageBytes(context.Background())
if err != nil {
t.Fatalf("UsageBytes failed: %v", err)
}
if usage == 900 {
break
}
time.Sleep(25 * time.Millisecond)
}
if usage != 900 {
t.Errorf("usage after periodic reconciliation = %d, want 900 "+
"(a file that appeared after startup reconciliation already ran must still "+
"be adopted by a later periodic pass)", usage)
}
if n := countRows(t, cache,
`SELECT COUNT(*) FROM variant_content WHERE cache_key = ?`, "aabbccdd0099",
); n != 1 {
t.Errorf("file that appeared after startup was not adopted by periodic "+
"reconciliation (rows=%d)", n)
}
}
// TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent exercises
// the exact TOCTOU window between evictSourceBlob's row-deletion
// transaction commit and its content file unlink: a concurrent
// StoreSource for a different source path whose content hashes to the
// same value (real SHA-256 dedup, not a contrived case) must not be
// able to insert a fresh row referencing the file while eviction is
// mid-unlink, and must not lose its own store once eviction has fully
// released the content hash.
func TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent(t *testing.T) {
cache, _ := newEvictionTestCache(t, 1<<30)
ctx := context.Background()
content := bytes.Repeat([]byte{0x55}, 400)
hash := storeEvictionTestSource(t, cache, "race.example.com", "/first.jpg", content)
proceed := make(chan struct{})
storeAttempted := make(chan struct{})
cache.evictSourceBlobTestHook = func(gotHash ContentHash) {
if gotHash != hash {
t.Errorf("test hook invoked for hash %s, want %s", gotHash, hash)
}
close(storeAttempted)
<-proceed
}
evictDone := make(chan error, 1)
go func() {
evictDone <- cache.evictSourceBlob(ctx, hash)
}()
// Wait until eviction has committed its delete transaction and is
// paused (inside the test hook) immediately before unlinking the
// content file: exactly the window the review flagged.
<-storeAttempted
storeDone := make(chan error, 1)
go func() {
req := &ImageRequest{
SourceHost: "race.example.com",
SourcePath: "/dup.jpg",
Format: FormatJPEG,
Quality: 85,
FitMode: FitCover,
}
result := &httpfetcher.FetchResult{
StatusCode: 200,
ContentType: "image/jpeg",
ContentLength: int64(len(content)),
Headers: map[string][]string{"Content-Type": {"image/jpeg"}},
}
_, err := cache.StoreSource(ctx, req, bytes.NewReader(content), result)
storeDone <- err
}()
// The concurrent store must not be able to complete while eviction
// still holds the content hash (i.e. before the file is unlinked):
// if it could, it would insert a row referencing a file about to be
// removed out from under it.
select {
case err := <-storeDone:
t.Fatalf("StoreSource for identical content completed (err=%v) while eviction "+
"still held the content hash open between commit and unlink; the store and "+
"the evict of identical content are not mutually exclusive", err)
case <-time.After(200 * time.Millisecond):
// Expected: the store is blocked behind eviction's exclusion.
}
close(proceed)
if err := <-evictDone; err != nil {
t.Fatalf("evictSourceBlob failed: %v", err)
}
if err := <-storeDone; err != nil {
t.Fatalf("StoreSource failed: %v", err)
}
assertNoDanglingReferences(t, cache)
dupHash, _, err := cache.LookupSource(ctx, &ImageRequest{
SourceHost: "race.example.com",
SourcePath: "/dup.jpg",
})
if err != nil {
t.Fatalf("LookupSource failed: %v", err)
}
if dupHash == "" {
t.Fatal("re-stored blob was lost: the store legitimately ran after eviction " +
"released the content hash and must have recreated the file and row")
}
if !cache.srcContent.Exists(dupHash) {
t.Errorf("source_content/source_metadata references %s but its file is missing", dupHash)
}
}

View File

@@ -65,24 +65,49 @@ func (s *ContentStorage) Store(r io.Reader) (hash ContentHash, size int64, err e
hash = ContentHash(hex.EncodeToString(h[:]))
size = int64(len(data))
if err := s.writeIfAbsent(hash, data); err != nil {
return "", 0, err
}
return hash, size, nil
}
// StoreHashed writes pre-hashed content to storage at the path derived
// from hash, without recomputing it. Callers that already know the
// hash before writing (e.g. because they must hold a hash-keyed lock
// across the whole store operation) use this instead of Store. Like
// Store, it is idempotent: content already on disk at that path is
// left untouched.
func (s *ContentStorage) StoreHashed(hash ContentHash, data []byte) (size int64, err error) {
if err := s.writeIfAbsent(hash, data); err != nil {
return 0, err
}
return int64(len(data)), nil
}
// writeIfAbsent writes data to the path derived from hash, unless
// content already exists there, via a temp-file-plus-rename so
// concurrent readers never observe a partial file.
func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) (err error) {
// Build path: <basedir>/<ab>/<cd>/<hash>
path := s.hashToPath(hash)
// Check if already exists
if _, err := os.Stat(path); err == nil {
return hash, size, nil
if _, statErr := os.Stat(path); statErr == nil {
return nil
}
// Create directory structure
dir := filepath.Dir(path)
if err := os.MkdirAll(dir, StorageDirPerm); err != nil {
return "", 0, fmt.Errorf("failed to create directory: %w", err)
return fmt.Errorf("failed to create directory: %w", err)
}
// Write to temp file first, then rename for atomicity
tmpFile, err := os.CreateTemp(dir, ".tmp-*")
if err != nil {
return "", 0, fmt.Errorf("failed to create temp file: %w", err)
return fmt.Errorf("failed to create temp file: %w", err)
}
tmpPath := tmpFile.Name()
@@ -95,20 +120,20 @@ func (s *ContentStorage) Store(r io.Reader) (hash ContentHash, size int64, err e
if _, err := tmpFile.Write(data); err != nil {
_ = tmpFile.Close()
return "", 0, fmt.Errorf("failed to write content: %w", err)
return fmt.Errorf("failed to write content: %w", err)
}
if err := tmpFile.Close(); err != nil {
return "", 0, fmt.Errorf("failed to close temp file: %w", err)
return fmt.Errorf("failed to close temp file: %w", err)
}
// Atomic rename
//nolint:gosec // G703: paths from internal SHA256 hashes
if err := os.Rename(filepath.Clean(tmpPath), filepath.Clean(path)); err != nil {
return "", 0, fmt.Errorf("failed to rename temp file: %w", err)
return fmt.Errorf("failed to rename temp file: %w", err)
}
return hash, size, nil
return nil
}
// Load returns a reader for the content with the given hash.

View File

@@ -17,7 +17,7 @@ run_with_cgo_deps() {
main() {
cd "$ROOT"
echo "Running tests..."
run_with_cgo_deps "CGO_ENABLED=1 go test -timeout 30s -v ./..."
run_with_cgo_deps "CGO_ENABLED=1 go test -timeout 30s -race -v ./..."
}
main "$@"