diff --git a/internal/config/cache_max_bytes_internal_test.go b/internal/config/cache_max_bytes_internal_test.go index ac7bf72..fe9c9a8 100644 --- a/internal/config/cache_max_bytes_internal_test.go +++ b/internal/config/cache_max_bytes_internal_test.go @@ -2,7 +2,6 @@ package config import ( "errors" - "io" "log/slog" "os" "path/filepath" @@ -13,7 +12,7 @@ import ( // discardLogger returns a logger that swallows all output, for tests // that exercise code paths which log. func discardLogger() *slog.Logger { - return slog.New(slog.NewTextHandler(io.Discard, nil)) + return slog.New(slog.DiscardHandler) } // TestCacheMaxBytesExplicitValueUsedWithoutFloor verifies that an @@ -21,6 +20,8 @@ func discardLogger() *slog.Logger { // given: the 500 MiB floor applies only to the computed default, never // to explicit values. func TestCacheMaxBytesExplicitValueUsedWithoutFloor(t *testing.T) { + t.Parallel() + yamlContent := "signing_key: " + validTestSigningKey + "\ncache_max_bytes: 1024\n" c, err := configFromYAML(t, yamlContent) @@ -38,6 +39,8 @@ func TestCacheMaxBytesExplicitValueUsedWithoutFloor(t *testing.T) { // explicit zero is a valid value (it disables the disk cache), not an // error. func TestCacheMaxBytesZeroIsValidAndDisablesCache(t *testing.T) { + t.Parallel() + yamlContent := "signing_key: " + validTestSigningKey + "\ncache_max_bytes: 0\n" c, err := configFromYAML(t, yamlContent) @@ -53,7 +56,10 @@ func TestCacheMaxBytesZeroIsValidAndDisablesCache(t *testing.T) { // TestCacheMaxBytesLargeExplicitValueParses verifies that values above // 32-bit range parse correctly (the field is an int64 byte count). func TestCacheMaxBytesLargeExplicitValueParses(t *testing.T) { - yamlContent := "signing_key: " + validTestSigningKey + "\ncache_max_bytes: 10737418240\n" + t.Parallel() + + yamlContent := "signing_key: " + validTestSigningKey + + "\ncache_max_bytes: 10737418240\n" c, err := configFromYAML(t, yamlContent) if err != nil { @@ -70,6 +76,8 @@ func TestCacheMaxBytesLargeExplicitValueParses(t *testing.T) { // offending value, per the no-silent-fallback rule: defaults apply // only to omitted keys. func TestCacheMaxBytesInvalidValuesAbortStartup(t *testing.T) { + t.Parallel() + signingKeyLine := "signing_key: " + validTestSigningKey + "\n" cases := []struct { @@ -81,45 +89,48 @@ func TestCacheMaxBytesInvalidValuesAbortStartup(t *testing.T) { { name: "negative", yaml: signingKeyLine + "cache_max_bytes: -1024\n", - wantErrSubstrings: []string{"cache_max_bytes", "-1024"}, + wantErrSubstrings: []string{keyCacheMaxBytes, "-1024"}, }, { name: "float", yaml: signingKeyLine + "cache_max_bytes: 3.5\n", - wantErrSubstrings: []string{"cache_max_bytes", "3.5"}, + wantErrSubstrings: []string{keyCacheMaxBytes, "3.5"}, }, { name: "non-numeric string", yaml: signingKeyLine + "cache_max_bytes: banana\n", - wantErrSubstrings: []string{"cache_max_bytes", "banana"}, + wantErrSubstrings: []string{keyCacheMaxBytes, "banana"}, }, { name: "explicit null", yaml: signingKeyLine + "cache_max_bytes: null\n", - wantErrSubstrings: []string{"cache_max_bytes", "null"}, + wantErrSubstrings: []string{keyCacheMaxBytes, "null"}, }, { name: "bare key no value", yaml: signingKeyLine + "cache_max_bytes:\n", - wantErrSubstrings: []string{"cache_max_bytes", "null"}, + wantErrSubstrings: []string{keyCacheMaxBytes, "null"}, }, { name: "boolean", yaml: signingKeyLine + "cache_max_bytes: true\n", - wantErrSubstrings: []string{"cache_max_bytes", "true"}, + wantErrSubstrings: []string{keyCacheMaxBytes, "true"}, }, { name: "list", yaml: signingKeyLine + "cache_max_bytes:\n - 1\n", - wantErrSubstrings: []string{"cache_max_bytes"}, + wantErrSubstrings: []string{keyCacheMaxBytes}, }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() + c, err := configFromYAML(t, tc.yaml) if err == nil { - t.Fatalf("config with %s cache_max_bytes must abort startup, got config: %+v", + t.Fatalf( + "config with %s cache_max_bytes must abort startup, got config: %+v", tc.name, c) } @@ -138,6 +149,8 @@ func TestCacheMaxBytesInvalidValuesAbortStartup(t *testing.T) { // computed default is 75% of the probed free space when that exceeds // the floor. func TestComputeDefaultCacheMaxBytesUses75PercentOfFreeSpace(t *testing.T) { + t.Parallel() + // 4 GiB free -> 3 GiB default. probe := func(string) (uint64, error) { return 4294967296, nil } @@ -147,7 +160,8 @@ func TestComputeDefaultCacheMaxBytesUses75PercentOfFreeSpace(t *testing.T) { } if got != 3221225472 { - t.Errorf("ComputeDefaultCacheMaxBytes = %d, want 3221225472 (75%% of 4 GiB)", got) + t.Errorf("ComputeDefaultCacheMaxBytes = %d, want 3221225472 (75%% of 4 GiB)", + got) } } @@ -155,6 +169,8 @@ func TestComputeDefaultCacheMaxBytesUses75PercentOfFreeSpace(t *testing.T) { // verifies that when 75% of free space is below 500 MiB, the computed // default is floored at DefaultCacheMaxBytesFloor. func TestComputeDefaultCacheMaxBytesAppliesFloorToComputedDefault(t *testing.T) { + t.Parallel() + cases := []struct { name string freeBytes uint64 @@ -166,6 +182,8 @@ func TestComputeDefaultCacheMaxBytesAppliesFloorToComputedDefault(t *testing.T) for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { + t.Parallel() + probe := func(string) (uint64, error) { return tc.freeBytes, nil } got, err := ComputeDefaultCacheMaxBytes(t.TempDir(), probe) @@ -185,6 +203,8 @@ func TestComputeDefaultCacheMaxBytesAppliesFloorToComputedDefault(t *testing.T) // failing free-space probe produces an error naming the config key, // instead of a silently wrong default. func TestComputeDefaultCacheMaxBytesPropagatesProbeError(t *testing.T) { + t.Parallel() + probe := func(string) (uint64, error) { return 0, errors.New("statfs failed") } _, err := ComputeDefaultCacheMaxBytes(t.TempDir(), probe) @@ -194,7 +214,7 @@ func TestComputeDefaultCacheMaxBytesPropagatesProbeError(t *testing.T) { t.Logf("got expected error: %v", err) - if !strings.Contains(err.Error(), "cache_max_bytes") { + if !strings.Contains(err.Error(), keyCacheMaxBytes) { t.Errorf("error %q does not name the config key cache_max_bytes", err.Error()) } } @@ -205,6 +225,8 @@ func TestComputeDefaultCacheMaxBytesPropagatesProbeError(t *testing.T) { // first so statfs measures the right filesystem), and that the result // lands on the Config. func TestResolveCacheMaxBytesComputesDefaultWhenOmitted(t *testing.T) { + t.Parallel() + c, err := configFromYAML(t, "signing_key: "+validTestSigningKey+"\n") if err != nil { t.Fatalf("minimal config should be valid, got error: %v", err) @@ -227,11 +249,13 @@ func TestResolveCacheMaxBytesComputesDefaultWhenOmitted(t *testing.T) { } if c.CacheMaxBytes != 3221225472 { - t.Errorf("CacheMaxBytes = %d, want computed default 3221225472", c.CacheMaxBytes) + t.Errorf("CacheMaxBytes = %d, want computed default 3221225472", + c.CacheMaxBytes) } if probedPath != wantCacheDir { - t.Errorf("free space probed at %q, want cache directory %q", probedPath, wantCacheDir) + t.Errorf("free space probed at %q, want cache directory %q", + probedPath, wantCacheDir) } info, err := os.Stat(wantCacheDir) @@ -245,6 +269,8 @@ func TestResolveCacheMaxBytesComputesDefaultWhenOmitted(t *testing.T) { // an explicitly configured value survives resolution untouched and // that the free-space probe is never consulted for it. func TestResolveCacheMaxBytesDoesNotOverrideExplicitValue(t *testing.T) { + t.Parallel() + yamlContent := "signing_key: " + validTestSigningKey + "\ncache_max_bytes: 1024\n" c, err := configFromYAML(t, yamlContent) diff --git a/internal/config/cachesize.go b/internal/config/cachesize.go index 2a0f8ad..263035a 100644 --- a/internal/config/cachesize.go +++ b/internal/config/cachesize.go @@ -44,7 +44,7 @@ func defaultFreeSpaceProbe(path string) (uint64, error) { return 0, fmt.Errorf("statfs reported negative block size %d for %q", stat.Bsize, path) } - blockSize := uint64(stat.Bsize) //nolint:gosec // G115: negative Bsize rejected above + blockSize := uint64(stat.Bsize) return stat.Bavail * blockSize, nil } @@ -52,7 +52,9 @@ func defaultFreeSpaceProbe(path string) (uint64, error) { // ComputeDefaultCacheMaxBytes returns the default cache size limit for // the filesystem containing cacheDir: 75% of the free bytes reported // by probe, with a floor of DefaultCacheMaxBytesFloor. -func ComputeDefaultCacheMaxBytes(cacheDir string, probe FreeSpaceProbeFunc) (int64, error) { +func ComputeDefaultCacheMaxBytes( + cacheDir string, probe FreeSpaceProbeFunc, +) (int64, error) { freeBytes, err := probe(cacheDir) if err != nil { return 0, fmt.Errorf("config key %q: cannot determine free space for %q: %w", @@ -60,15 +62,10 @@ func ComputeDefaultCacheMaxBytes(cacheDir string, probe FreeSpaceProbeFunc) (int } computed := freeBytes / freeSpaceFractionDenominator * freeSpaceFractionNumerator - if computed > math.MaxInt64 { - computed = math.MaxInt64 - } + computed = min(computed, math.MaxInt64) - limit := int64(computed) //nolint:gosec // G115: clamped to MaxInt64 above - - if limit < DefaultCacheMaxBytesFloor { - limit = DefaultCacheMaxBytesFloor - } + limit := int64(computed) + limit = max(limit, DefaultCacheMaxBytesFloor) return limit, nil } @@ -79,7 +76,9 @@ func ComputeDefaultCacheMaxBytes(cacheDir string, probe FreeSpaceProbeFunc) (int // on free space in /cache/. The cache directory is created // first so statfs measures the filesystem that will actually hold the // cache. The effective limit is logged either way. -func (c *Config) resolveCacheMaxBytes(log *slog.Logger, probe FreeSpaceProbeFunc) error { +func (c *Config) resolveCacheMaxBytes( + log *slog.Logger, probe FreeSpaceProbeFunc, +) error { if !c.cacheMaxBytesExplicit { cacheDir := filepath.Join(c.StateDir, "cache") diff --git a/internal/config/config.go b/internal/config/config.go index 2c8c8f2..80a089b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -622,7 +622,7 @@ func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, erro key, val) } - return int64(val), nil //nolint:gosec // G115: bounds checked above + return int64(val), nil case float64: if val != math.Trunc(val) { return 0, fmt.Errorf("config key %q: value %v is not an integer", key, val) diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index 078e87f..ffbd145 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -177,32 +177,6 @@ func (c *Cache) Lookup(ctx context.Context, req *ImageRequest) (*LookupResult, e }, nil } -// touchVariant updates the LRU timestamp of a variant, best-effort: -// a failed touch only makes the entry look colder to eviction. -func (c *Cache) touchVariant(ctx context.Context, cacheKey VariantKey) { - _, err := c.db.ExecContext(ctx, ` - UPDATE variant_content SET last_accessed_at = CURRENT_TIMESTAMP - WHERE cache_key = ? - `, string(cacheKey)) - if err != nil { - c.log.Debug("failed to touch variant LRU timestamp", - "cache_key", cacheKey, "error", err) - } -} - -// touchSourceContent updates the LRU timestamp of a source content -// blob, best-effort: a failed touch only makes the blob look colder. -func (c *Cache) touchSourceContent(ctx context.Context, contentHash ContentHash) { - _, err := c.db.ExecContext(ctx, ` - UPDATE source_content SET last_accessed_at = CURRENT_TIMESTAMP - WHERE content_hash = ? - `, string(contentHash)) - if err != nil { - c.log.Debug("failed to touch source content LRU timestamp", - "content_hash", contentHash, "error", err) - } -} - // GetVariant returns a reader, size, and content type for a cached variant. func (c *Cache) GetVariant(cacheKey VariantKey) (io.ReadCloser, int64, string, error) { if c.disabled { @@ -506,6 +480,32 @@ func (c *Cache) IncrementStats(ctx context.Context, hit bool, fetchBytes int64) } } +// touchVariant updates the LRU timestamp of a variant, best-effort: +// a failed touch only makes the entry look colder to eviction. +func (c *Cache) touchVariant(ctx context.Context, cacheKey VariantKey) { + _, err := c.db.ExecContext(ctx, ` + UPDATE variant_content SET last_accessed_at = CURRENT_TIMESTAMP + WHERE cache_key = ? + `, string(cacheKey)) + if err != nil { + c.log.Debug("failed to touch variant LRU timestamp", + "cache_key", cacheKey, "error", err) + } +} + +// touchSourceContent updates the LRU timestamp of a source content +// blob, best-effort: a failed touch only makes the blob look colder. +func (c *Cache) touchSourceContent(ctx context.Context, contentHash ContentHash) { + _, err := c.db.ExecContext(ctx, ` + UPDATE source_content SET last_accessed_at = CURRENT_TIMESTAMP + WHERE content_hash = ? + `, string(contentHash)) + if err != nil { + c.log.Debug("failed to touch source content LRU timestamp", + "content_hash", contentHash, "error", err) + } +} + // checkNegativeCache checks if a request is in the negative cache. func (c *Cache) checkNegativeCache( ctx context.Context, req *ImageRequest, diff --git a/internal/imgcache/cache_internal_test.go b/internal/imgcache/cache_internal_test.go index 410be90..3912bf8 100644 --- a/internal/imgcache/cache_internal_test.go +++ b/internal/imgcache/cache_internal_test.go @@ -160,7 +160,7 @@ func TestCache_StoreAndLookup(t *testing.T) { sourceContent := []byte("fake jpeg data") fetchResult := &httpfetcher.FetchResult{ ContentType: testContentTypeJPEG, - Headers: map[string][]string{"Content-Type": {testContentTypeJPEG}}, + Headers: map[string][]string{testHeaderContentType: {testContentTypeJPEG}}, } contentHash, err := cache.StoreSource( diff --git a/internal/imgcache/contentlock_internal_test.go b/internal/imgcache/contentlock_internal_test.go index 1eb56e5..ff94119 100644 --- a/internal/imgcache/contentlock_internal_test.go +++ b/internal/imgcache/contentlock_internal_test.go @@ -10,10 +10,12 @@ import ( // TestContentLockExcludesSameKey verifies that two goroutines locking // the same key never run their critical sections concurrently. func TestContentLockExcludesSameKey(t *testing.T) { + t.Parallel() + lock := newContentLock() var ( - active int32 + active atomic.Int32 maxSeen int32 wg sync.WaitGroup ) @@ -22,14 +24,14 @@ func TestContentLockExcludesSameKey(t *testing.T) { wg.Add(goroutines) - for i := 0; i < goroutines; i++ { + for range goroutines { go func() { defer wg.Done() unlock := lock.Lock("same-key") defer unlock() - n := atomic.AddInt32(&active, 1) + n := active.Add(1) for { seen := atomic.LoadInt32(&maxSeen) @@ -40,7 +42,7 @@ func TestContentLockExcludesSameKey(t *testing.T) { time.Sleep(time.Millisecond) - atomic.AddInt32(&active, -1) + active.Add(-1) }() } @@ -57,13 +59,15 @@ func TestContentLockExcludesSameKey(t *testing.T) { // them reaching the rendezvous point before any is allowed to // proceed. func TestContentLockAllowsDifferentKeys(t *testing.T) { + t.Parallel() + lock := newContentLock() const goroutines = 20 var ( wg sync.WaitGroup - inside int32 + inside atomic.Int32 reached = make(chan struct{}, goroutines) ) @@ -71,7 +75,7 @@ func TestContentLockAllowsDifferentKeys(t *testing.T) { release := make(chan struct{}) - for i := 0; i < goroutines; i++ { + for i := range goroutines { key := string(rune('a' + i)) go func() { @@ -80,8 +84,10 @@ func TestContentLockAllowsDifferentKeys(t *testing.T) { unlock := lock.Lock(key) defer unlock() - atomic.AddInt32(&inside, 1) + inside.Add(1) + reached <- struct{}{} + <-release }() } @@ -90,7 +96,7 @@ func TestContentLockAllowsDifferentKeys(t *testing.T) { // 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++ { + for i := range goroutines { select { case <-reached: case <-time.After(2 * time.Second): @@ -99,8 +105,9 @@ func TestContentLockAllowsDifferentKeys(t *testing.T) { } } - if n := atomic.LoadInt32(&inside); n != goroutines { - t.Errorf("goroutines inside their critical section = %d, want %d", n, goroutines) + if n := inside.Load(); n != goroutines { + t.Errorf("goroutines inside their critical section = %d, want %d", + n, goroutines) } close(release) @@ -111,6 +118,8 @@ func TestContentLockAllowsDifferentKeys(t *testing.T) { // entries map does not grow without bound: once no goroutine holds or // awaits a key, its entry is removed. func TestContentLockRemovesEntryAfterUnlock(t *testing.T) { + t.Parallel() + lock := newContentLock() unlock := lock.Lock("k") diff --git a/internal/imgcache/eviction.go b/internal/imgcache/eviction.go index cd2d6fe..f359364 100644 --- a/internal/imgcache/eviction.go +++ b/internal/imgcache/eviction.go @@ -516,25 +516,28 @@ func (c *Cache) reconcileAccounting(ctx context.Context) error { // reconcileVariantFiles walks the variant storage directory, adopting // files without accounting rows and sweeping stale temp files. func (c *Cache) reconcileVariantFiles(ctx context.Context) error { - return filepath.WalkDir(c.variants.baseDir, func(path string, entry fs.DirEntry, err error) error { - if err != nil || entry.IsDir() { - return err - } + return filepath.WalkDir( + c.variants.baseDir, + func(path string, entry fs.DirEntry, err error) error { + if err != nil || entry.IsDir() { + return err + } - name := entry.Name() + name := entry.Name() - if strings.HasPrefix(name, tempFilePrefix) { - c.sweepStaleTempFile(path, entry) + if strings.HasPrefix(name, tempFilePrefix) { + c.sweepStaleTempFile(path, entry) - return nil - } + return nil + } - if strings.HasSuffix(name, variantMetaSuffix) { - return nil - } + if strings.HasSuffix(name, variantMetaSuffix) { + return nil + } - return c.adoptVariantFile(ctx, path, entry, VariantKey(name)) - }) + return c.adoptVariantFile(ctx, path, entry, VariantKey(name)) + }, + ) } // adoptVariantFile inserts an accounting row for a variant file that @@ -581,7 +584,8 @@ func (c *Cache) adoptVariantFile( // variantContentTypeFromSidecar reads the content type from a variant // .meta sidecar, falling back to application/octet-stream. func (c *Cache) variantContentTypeFromSidecar(variantPath string) string { - metaData, err := os.ReadFile(variantPath + variantMetaSuffix) //nolint:gosec // path from cache walk + //nolint:gosec // path from cache walk + metaData, err := os.ReadFile(variantPath + variantMetaSuffix) if err != nil { return fallbackContentType } @@ -650,21 +654,24 @@ func (c *Cache) allVariantKeys(ctx context.Context) ([]VariantKey, error) { // lookups always go through source_metadata) and sweeping stale temp // files. func (c *Cache) reconcileSourceFiles(ctx context.Context) error { - return filepath.WalkDir(c.srcContent.baseDir, func(path string, entry fs.DirEntry, err error) error { - if err != nil || entry.IsDir() { - return err - } + return filepath.WalkDir( + c.srcContent.baseDir, + func(path string, entry fs.DirEntry, err error) error { + if err != nil || entry.IsDir() { + return err + } - name := entry.Name() + name := entry.Name() - if strings.HasPrefix(name, tempFilePrefix) { - c.sweepStaleTempFile(path, entry) + if strings.HasPrefix(name, tempFilePrefix) { + c.sweepStaleTempFile(path, entry) - return nil - } + return nil + } - return c.removeUntrackedSourceFile(ctx, path, ContentHash(name)) - }) + return c.removeUntrackedSourceFile(ctx, path, ContentHash(name)) + }, + ) } // removeUntrackedSourceFile deletes a source blob file that has no @@ -691,7 +698,6 @@ func (c *Cache) removeUntrackedSourceFile( return fmt.Errorf("failed to delete metadata rows for untracked blob: %w", err) } - //nolint:gosec // G703: path comes from walking our own cache directory if err := os.Remove(path); err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to remove untracked source file: %w", err) } @@ -765,7 +771,6 @@ func (c *Cache) sweepStaleTempFile(path string, entry fs.DirEntry) { return } - //nolint:gosec // G703: path comes from walking our own cache directory if err := os.Remove(path); err != nil && !os.IsNotExist(err) { c.log.Warn("failed to remove stale temp file", "path", path, "error", err) diff --git a/internal/imgcache/eviction_internal_test.go b/internal/imgcache/eviction_internal_test.go index 77c9bec..dd52741 100644 --- a/internal/imgcache/eviction_internal_test.go +++ b/internal/imgcache/eviction_internal_test.go @@ -20,6 +20,10 @@ import ( // implementation writes. const sqliteTimestampFormat = "2006-01-02 15:04:05" +// testVariantKeyOne is the variant cache key reused across the eviction +// tests as the first stored variant. +const testVariantKeyOne VariantKey = "aabbccdd0001" + // evictionTestDB creates an in-memory SQLite database with the real // production schema, limited to a single connection so the background // eviction goroutine shares the same in-memory database as the test. @@ -85,12 +89,16 @@ func storeEvictionTestSource( result := &httpfetcher.FetchResult{ StatusCode: 200, - ContentType: "image/jpeg", + ContentType: testContentTypeJPEG, ContentLength: int64(len(content)), - Headers: map[string][]string{"Content-Type": {"image/jpeg"}}, + Headers: map[string][]string{ + testHeaderContentType: {testContentTypeJPEG}, + }, } - hash, err := cache.StoreSource(context.Background(), req, bytes.NewReader(content), result) + hash, err := cache.StoreSource( + context.Background(), req, bytes.NewReader(content), result, + ) if err != nil { t.Fatalf("StoreSource(%s%s) failed: %v", host, path, err) } @@ -100,20 +108,25 @@ func storeEvictionTestSource( // storeEvictionTestVariant stores content as a processed variant under // the given cache key. -func storeEvictionTestVariant(t *testing.T, cache *Cache, key VariantKey, content []byte) { +func storeEvictionTestVariant( + t *testing.T, cache *Cache, key VariantKey, content []byte, +) { t.Helper() - if err := cache.StoreVariant(key, bytes.NewReader(content), "image/webp"); err != nil { + err := cache.StoreVariant(key, bytes.NewReader(content), "image/webp") + if err != nil { t.Fatalf("StoreVariant(%s) failed: %v", key, err) } } // setVariantLastAccessed backdates the last access time of a tracked // variant, to make LRU ordering deterministic in tests. -func setVariantLastAccessed(t *testing.T, cache *Cache, key VariantKey, when time.Time) { +func setVariantLastAccessed( + t *testing.T, cache *Cache, key VariantKey, when time.Time, +) { t.Helper() - res, err := cache.db.Exec( + res, err := cache.db.ExecContext(t.Context(), `UPDATE variant_content SET last_accessed_at = ? WHERE cache_key = ?`, when.UTC().Format(sqliteTimestampFormat), string(key), ) @@ -134,10 +147,12 @@ func setVariantLastAccessed(t *testing.T, cache *Cache, key VariantKey, when tim // setSourceLastAccessed backdates the last access time of a tracked // source content blob. -func setSourceLastAccessed(t *testing.T, cache *Cache, hash ContentHash, when time.Time) { +func setSourceLastAccessed( + t *testing.T, cache *Cache, hash ContentHash, when time.Time, +) { t.Helper() - res, err := cache.db.Exec( + res, err := cache.db.ExecContext(t.Context(), `UPDATE source_content SET last_accessed_at = ? WHERE content_hash = ?`, when.UTC().Format(sqliteTimestampFormat), string(hash), ) @@ -156,11 +171,13 @@ func setSourceLastAccessed(t *testing.T, cache *Cache, hash ContentHash, when ti } // countRows returns the number of rows the given query yields. -func countRows(t *testing.T, cache *Cache, query string, args ...interface{}) int { +func countRows(t *testing.T, cache *Cache, query string, args ...any) int { t.Helper() var n int - if err := cache.db.QueryRow(query, args...).Scan(&n); err != nil { + + err := cache.db.QueryRowContext(t.Context(), query, args...).Scan(&n) + if err != nil { t.Fatalf("count query %q failed: %v", query, err) } @@ -173,7 +190,7 @@ func countRows(t *testing.T, cache *Cache, query string, args ...interface{}) in func assertNoDanglingReferences(t *testing.T, cache *Cache) { t.Helper() - rows, err := cache.db.Query( + rows, err := cache.db.QueryContext(t.Context(), `SELECT content_hash FROM source_metadata WHERE content_hash IS NOT NULL AND content_hash != ''`, ) @@ -185,20 +202,25 @@ func assertNoDanglingReferences(t *testing.T, cache *Cache) { for rows.Next() { var hash string - if err := rows.Scan(&hash); err != nil { + + err := rows.Scan(&hash) + if err != nil { t.Fatalf("failed to scan content_hash: %v", err) } if !cache.srcContent.Exists(ContentHash(hash)) { - t.Errorf("source_metadata references content %s but the file is missing", hash) + t.Errorf("source_metadata references content %s but the file is missing", + hash) } } - if err := rows.Err(); err != nil { + err = rows.Err() + if err != nil { t.Fatalf("source_metadata iteration failed: %v", err) } - variantRows, err := cache.db.Query(`SELECT cache_key FROM variant_content`) + variantRows, err := cache.db.QueryContext(t.Context(), + `SELECT cache_key FROM variant_content`) if err != nil { t.Fatalf("failed to query variant_content: %v", err) } @@ -207,7 +229,9 @@ func assertNoDanglingReferences(t *testing.T, cache *Cache) { for variantRows.Next() { var key string - if err := variantRows.Scan(&key); err != nil { + + err := variantRows.Scan(&key) + if err != nil { t.Fatalf("failed to scan cache_key: %v", err) } @@ -216,14 +240,17 @@ func assertNoDanglingReferences(t *testing.T, cache *Cache) { } } - if err := variantRows.Err(); err != nil { + err = variantRows.Err() + if err != nil { t.Fatalf("variant_content iteration failed: %v", err) } } // waitForUsageAtOrBelow polls UsageBytes until it reaches limit or the // timeout expires, returning the last observed usage. -func waitForUsageAtOrBelow(t *testing.T, cache *Cache, limit int64, timeout time.Duration) int64 { +func waitForUsageAtOrBelow( + t *testing.T, cache *Cache, limit int64, timeout time.Duration, +) int64 { t.Helper() deadline := time.Now().Add(timeout) @@ -249,14 +276,18 @@ func waitForUsageAtOrBelow(t *testing.T, cache *Cache, limit int64, timeout time } func TestUsageBytesAccountsSourceAndVariantBytes(t *testing.T) { + t.Parallel() + cache, _ := newEvictionTestCache(t, 1<<30) storeEvictionTestSource(t, cache, "src.example.com", "/a.jpg", bytes.Repeat([]byte{0xAA}, 1000)) storeEvictionTestSource(t, cache, "src.example.com", "/b.jpg", bytes.Repeat([]byte{0xAB}, 2000)) - storeEvictionTestVariant(t, cache, "aabbccdd0001", bytes.Repeat([]byte{0xAC}, 500)) - storeEvictionTestVariant(t, cache, "aabbccdd0002", bytes.Repeat([]byte{0xAD}, 250)) + storeEvictionTestVariant(t, cache, testVariantKeyOne, + bytes.Repeat([]byte{0xAC}, 500)) + storeEvictionTestVariant(t, cache, "aabbccdd0002", + bytes.Repeat([]byte{0xAD}, 250)) usage, err := cache.UsageBytes(context.Background()) if err != nil { @@ -269,15 +300,20 @@ func TestUsageBytesAccountsSourceAndVariantBytes(t *testing.T) { } func TestUsageBytesCountsMultiReferencedBlobOnce(t *testing.T) { + t.Parallel() + cache, _ := newEvictionTestCache(t, 1<<30) content := bytes.Repeat([]byte{0xCC}, 1200) - hashOne := storeEvictionTestSource(t, cache, "src.example.com", "/one.jpg", content) - hashTwo := storeEvictionTestSource(t, cache, "src.example.com", "/two.jpg", content) + hashOne := storeEvictionTestSource(t, cache, + "src.example.com", "/one.jpg", content) + hashTwo := storeEvictionTestSource(t, cache, + "src.example.com", "/two.jpg", content) if hashOne != hashTwo { - t.Fatalf("identical content produced different hashes: %s vs %s", hashOne, hashTwo) + t.Fatalf("identical content produced different hashes: %s vs %s", + hashOne, hashTwo) } usage, err := cache.UsageBytes(context.Background()) @@ -291,12 +327,16 @@ func TestUsageBytesCountsMultiReferencedBlobOnce(t *testing.T) { } func TestEvictToLimitEvictsLeastRecentlyUsedFirst(t *testing.T) { + t.Parallel() + const limit = 3000 cache, _ := newEvictionTestCache(t, limit) now := time.Now() - keys := []VariantKey{"aabbccdd0001", "aabbccdd0002", "aabbccdd0003", "aabbccdd0004"} + keys := []VariantKey{ + testVariantKeyOne, "aabbccdd0002", "aabbccdd0003", "aabbccdd0004", + } fills := []byte{0x01, 0x02, 0x03, 0x04} ages := []time.Duration{4 * time.Hour, 3 * time.Hour, 2 * time.Hour, 1 * time.Hour} @@ -338,6 +378,8 @@ func TestEvictToLimitEvictsLeastRecentlyUsedFirst(t *testing.T) { } func TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences(t *testing.T) { + t.Parallel() + const limit = 1000 cache, _ := newEvictionTestCache(t, limit) @@ -346,10 +388,14 @@ func TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences(t *testing. // One 800-byte blob referenced by two source paths. sharedContent := bytes.Repeat([]byte{0xDD}, 800) - sharedHash := storeEvictionTestSource(t, cache, "src.example.com", "/a.jpg", sharedContent) + sharedHash := storeEvictionTestSource(t, cache, + "src.example.com", "/a.jpg", sharedContent) - if h := storeEvictionTestSource(t, cache, "src.example.com", "/b.jpg", sharedContent); h != sharedHash { - t.Fatalf("identical content produced different hashes: %s vs %s", h, sharedHash) + h := storeEvictionTestSource(t, cache, + "src.example.com", "/b.jpg", sharedContent) + if h != sharedHash { + t.Fatalf("identical content produced different hashes: %s vs %s", + h, sharedHash) } // A newer 600-byte blob referenced by one source path. @@ -408,23 +454,28 @@ func TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences(t *testing. if n := countRows(t, cache, `SELECT COUNT(*) FROM source_metadata WHERE content_hash = ?`, string(recentHash), ); n != 1 { - t.Errorf("recently used blob %s has %d source_metadata rows, want 1", recentHash, n) + t.Errorf("recently used blob %s has %d source_metadata rows, want 1", + recentHash, n) } assertNoDanglingReferences(t, cache) } func TestEvictionKeepsEverythingWhenUnderLimit(t *testing.T) { + t.Parallel() + cache, _ := newEvictionTestCache(t, 1<<30) content := bytes.Repeat([]byte{0xDF}, 800) hash := storeEvictionTestSource(t, cache, "src.example.com", "/a.jpg", content) - if h := storeEvictionTestSource(t, cache, "src.example.com", "/b.jpg", content); h != hash { + h := storeEvictionTestSource(t, cache, "src.example.com", "/b.jpg", content) + if h != hash { t.Fatalf("identical content produced different hashes: %s vs %s", h, hash) } - storeEvictionTestVariant(t, cache, "aabbccdd0001", bytes.Repeat([]byte{0xE0}, 500)) + storeEvictionTestVariant(t, cache, testVariantKeyOne, + bytes.Repeat([]byte{0xE0}, 500)) if err := cache.EvictToLimit(context.Background()); err != nil { t.Fatalf("EvictToLimit failed: %v", err) @@ -440,7 +491,7 @@ func TestEvictionKeepsEverythingWhenUnderLimit(t *testing.T) { t.Errorf("blob %s has %d source_metadata rows, want 2", hash, n) } - if !cache.variants.Exists("aabbccdd0001") { + if !cache.variants.Exists(testVariantKeyOne) { t.Error("variant must not be evicted while usage is under the limit") } @@ -448,8 +499,9 @@ func TestEvictionKeepsEverythingWhenUnderLimit(t *testing.T) { } func TestZeroMaxBytesDisablesDiskCache(t *testing.T) { + t.Parallel() + cache, tmpDir := newEvictionTestCache(t, 0) - ctx := context.Background() req := &ImageRequest{ SourceHost: "src.example.com", @@ -459,14 +511,31 @@ func TestZeroMaxBytesDisablesDiskCache(t *testing.T) { FitMode: FitCover, } - // Writes are no-ops that report success. - if err := cache.StoreVariant(CacheKey(req), bytes.NewReader([]byte("data")), "image/webp"); err != nil { + assertDisabledCacheWritesAreNoOps(t, cache, req) + assertDisabledCacheReadsAlwaysMiss(t, cache, req) + assertDisabledCacheTracksNothing(t, cache) + assertDisabledCacheWroteNothingToDisk(t, tmpDir) +} + +// assertDisabledCacheWritesAreNoOps verifies that stores against a +// disabled cache report success without recording anything. +func assertDisabledCacheWritesAreNoOps( + t *testing.T, cache *Cache, req *ImageRequest, +) { + t.Helper() + + ctx := t.Context() + + err := cache.StoreVariant( + CacheKey(req), bytes.NewReader([]byte("data")), "image/webp", + ) + if err != nil { t.Fatalf("StoreVariant on disabled cache must be a no-op, got error: %v", err) } result := &httpfetcher.FetchResult{ StatusCode: 200, - ContentType: "image/jpeg", + ContentType: testContentTypeJPEG, ContentLength: 4, Headers: map[string][]string{}, } @@ -479,8 +548,17 @@ func TestZeroMaxBytesDisablesDiskCache(t *testing.T) { if hash != "" { t.Errorf("StoreSource on disabled cache returned hash %q, want empty", hash) } +} + +// assertDisabledCacheReadsAlwaysMiss verifies that lookups against a +// disabled cache never report a hit. +func assertDisabledCacheReadsAlwaysMiss( + t *testing.T, cache *Cache, req *ImageRequest, +) { + t.Helper() + + ctx := t.Context() - // Reads always miss. lookup, err := cache.Lookup(ctx, req) if err != nil { t.Fatalf("Lookup on disabled cache failed: %v", err) @@ -496,11 +574,17 @@ func TestZeroMaxBytesDisablesDiskCache(t *testing.T) { } if srcHash != "" || srcType != "" { - t.Errorf("LookupSource on disabled cache = (%q, %q), want empty", srcHash, srcType) + t.Errorf("LookupSource on disabled cache = (%q, %q), want empty", + srcHash, srcType) } +} - // Nothing is tracked and nothing is written to disk. - usage, err := cache.UsageBytes(ctx) +// assertDisabledCacheTracksNothing verifies that a disabled cache +// records no usage and writes no accounting rows. +func assertDisabledCacheTracksNothing(t *testing.T, cache *Cache) { + t.Helper() + + usage, err := cache.UsageBytes(t.Context()) if err != nil { t.Fatalf("UsageBytes failed: %v", err) } @@ -516,24 +600,33 @@ func TestZeroMaxBytesDisablesDiskCache(t *testing.T) { if n := countRows(t, cache, `SELECT COUNT(*) FROM source_metadata`); n != 0 { t.Errorf("disabled cache wrote %d source_metadata rows, want 0", n) } +} - if _, err := os.Stat(filepath.Join(tmpDir, "cache")); !os.IsNotExist(err) { - t.Errorf("disabled cache must not create the cache directory tree (stat err=%v)", err) +// assertDisabledCacheWroteNothingToDisk verifies that a disabled cache +// creates neither the cache directory tree nor any file under stateDir. +func assertDisabledCacheWroteNothingToDisk(t *testing.T, stateDir string) { + t.Helper() + + _, err := os.Stat(filepath.Join(stateDir, "cache")) + if !os.IsNotExist(err) { + t.Errorf("disabled cache must not create the cache directory tree "+ + "(stat err=%v)", err) } var foundFiles []string - walkErr := filepath.WalkDir(tmpDir, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err - } + walkErr := filepath.WalkDir(stateDir, + func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } - if !d.IsDir() { - foundFiles = append(foundFiles, path) - } + if !d.IsDir() { + foundFiles = append(foundFiles, path) + } - return nil - }) + return nil + }) if walkErr != nil { t.Fatalf("failed to walk state dir: %v", walkErr) } @@ -544,6 +637,8 @@ func TestZeroMaxBytesDisablesDiskCache(t *testing.T) { } func TestEvictionRunsUnderWritePressure(t *testing.T) { + t.Parallel() + const limit = 1500 cache, _ := newEvictionTestCache(t, limit) @@ -553,11 +648,12 @@ func TestEvictionRunsUnderWritePressure(t *testing.T) { cache.StartEviction(time.Hour) defer cache.StopEviction() - keys := []VariantKey{"aabbccdd0001", "aabbccdd0002", "aabbccdd0003"} + keys := []VariantKey{testVariantKeyOne, "aabbccdd0002", "aabbccdd0003"} fills := []byte{0x11, 0x12, 0x13} for i, key := range keys { - storeEvictionTestVariant(t, cache, key, bytes.Repeat([]byte{fills[i]}, 1000)) + storeEvictionTestVariant(t, cache, key, + bytes.Repeat([]byte{fills[i]}, 1000)) } usage := waitForUsageAtOrBelow(t, cache, limit, 5*time.Second) @@ -570,6 +666,8 @@ func TestEvictionRunsUnderWritePressure(t *testing.T) { } func TestEvictionRunsOnPeriodicSchedule(t *testing.T) { + t.Parallel() + const limit = 1500 cache, _ := newEvictionTestCache(t, limit) @@ -581,21 +679,23 @@ func TestEvictionRunsOnPeriodicSchedule(t *testing.T) { cache.StartEviction(100 * time.Millisecond) defer cache.StopEviction() - keys := []VariantKey{"aabbccdd0001", "aabbccdd0002", "aabbccdd0003"} + keys := []VariantKey{testVariantKeyOne, "aabbccdd0002", "aabbccdd0003"} fills := []byte{0x21, 0x22, 0x23} for i, key := range keys { content := bytes.Repeat([]byte{fills[i]}, 1000) - if _, err := cache.variants.Store(key, bytes.NewReader(content), "image/webp"); err != nil { + _, err := cache.variants.Store(key, bytes.NewReader(content), "image/webp") + if err != nil { t.Fatalf("failed to store variant file: %v", err) } - if _, err := cache.db.Exec( + _, err = cache.db.ExecContext(t.Context(), `INSERT INTO variant_content (cache_key, size_bytes, content_type) VALUES (?, ?, ?)`, string(key), len(content), "image/webp", - ); err != nil { + ) + if err != nil { t.Fatalf("failed to insert variant accounting row: %v", err) } } @@ -610,21 +710,28 @@ func TestEvictionRunsOnPeriodicSchedule(t *testing.T) { } func TestStartEvictionReconcilesAccountingWithDisk(t *testing.T) { + t.Parallel() + cache, _ := newEvictionTestCache(t, 1<<30) // An untracked variant file on disk (e.g. written before this // feature existed) must be adopted into the accounting. untracked := bytes.Repeat([]byte{0x31}, 1000) - if _, err := cache.variants.Store("aabbccdd0001", bytes.NewReader(untracked), "image/webp"); err != nil { + + _, err := cache.variants.Store( + testVariantKeyOne, bytes.NewReader(untracked), "image/webp", + ) + if err != nil { t.Fatalf("failed to store untracked variant file: %v", err) } // An accounting row whose file is missing must be dropped. - if _, err := cache.db.Exec( + _, err = cache.db.ExecContext(t.Context(), `INSERT INTO variant_content (cache_key, size_bytes, content_type) VALUES (?, ?, ?)`, "deadbeef0001", 700, "image/webp", - ); err != nil { + ) + if err != nil { t.Fatalf("failed to insert stale variant accounting row: %v", err) } @@ -656,7 +763,8 @@ func TestStartEvictionReconcilesAccountingWithDisk(t *testing.T) { } if n := countRows(t, cache, - `SELECT COUNT(*) FROM variant_content WHERE cache_key = ?`, "aabbccdd0001", + `SELECT COUNT(*) FROM variant_content WHERE cache_key = ?`, + string(testVariantKeyOne), ); n != 1 { t.Errorf("untracked variant file was not adopted into accounting (rows=%d)", n) } @@ -678,6 +786,8 @@ func TestStartEvictionReconcilesAccountingWithDisk(t *testing.T) { // staying invisible to UsageBytes/EvictToLimit until the next process // restart. func TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup(t *testing.T) { + t.Parallel() + cache, _ := newEvictionTestCache(t, 1<<30) const interval = 100 * time.Millisecond @@ -696,7 +806,11 @@ func TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup(t *testing.T) { // 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 { + + _, err := cache.variants.Store( + "aabbccdd0099", bytes.NewReader(untracked), "image/webp", + ) + if err != nil { t.Fatalf("failed to store untracked variant file: %v", err) } @@ -747,7 +861,8 @@ func TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent(t *testing.T) content := bytes.Repeat([]byte{0x55}, 400) - hash := storeEvictionTestSource(t, cache, "race.example.com", "/first.jpg", content) + hash := storeEvictionTestSource(t, cache, + "race.example.com", "/first.jpg", content) proceed := make(chan struct{}) storeAttempted := make(chan struct{}) @@ -772,26 +887,7 @@ func TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent(t *testing.T) // 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 - }() + storeDone := storeIdenticalContentConcurrently(ctx, cache, content) // The concurrent store must not be able to complete while eviction // still holds the content hash (i.e. before the file is unlinked): @@ -832,6 +928,39 @@ func TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent(t *testing.T) } if !cache.srcContent.Exists(dupHash) { - t.Errorf("source_content/source_metadata references %s but its file is missing", dupHash) + t.Errorf("source_content/source_metadata references %s but its file is missing", + dupHash) } } + +// storeIdenticalContentConcurrently starts a StoreSource for a second +// source path whose body hashes to the same content hash, returning the +// channel its error is delivered on. +func storeIdenticalContentConcurrently( + ctx context.Context, cache *Cache, content []byte, +) chan error { + 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: testContentTypeJPEG, + ContentLength: int64(len(content)), + Headers: map[string][]string{ + testHeaderContentType: {testContentTypeJPEG}, + }, + } + + _, err := cache.StoreSource(ctx, req, bytes.NewReader(content), result) + storeDone <- err + }() + + return storeDone +} diff --git a/internal/imgcache/storage.go b/internal/imgcache/storage.go index cb315cb..0a64bcb 100644 --- a/internal/imgcache/storage.go +++ b/internal/imgcache/storage.go @@ -79,7 +79,9 @@ func (s *ContentStorage) Store(r io.Reader) (ContentHash, int64, error) { // 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) { +func (s *ContentStorage) StoreHashed( + hash ContentHash, data []byte, +) (size int64, err error) { if err := s.writeIfAbsent(hash, data); err != nil { return 0, err } @@ -87,57 +89,6 @@ func (s *ContentStorage) StoreHashed(hash ContentHash, data []byte) (size int64, 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: /// - path := s.hashToPath(hash) - - // Check if already exists - 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 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 fmt.Errorf("failed to create temp file: %w", err) - } - - tmpPath := tmpFile.Name() - - defer func() { - if err != nil { - _ = os.Remove(tmpPath) - } - }() - - if _, err := tmpFile.Write(data); err != nil { - _ = tmpFile.Close() - - return fmt.Errorf("failed to write content: %w", err) - } - - if err := tmpFile.Close(); err != nil { - 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 fmt.Errorf("failed to rename temp file: %w", err) - } - - return nil -} - // Load returns a reader for the content with the given hash. func (s *ContentStorage) Load(hash ContentHash) (io.ReadCloser, error) { path := s.hashToPath(hash) @@ -197,6 +148,56 @@ func (s *ContentStorage) Exists(hash ContentHash) bool { return err == 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: /// + path := s.hashToPath(hash) + + // Check if already exists + 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 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 fmt.Errorf("failed to create temp file: %w", err) + } + + tmpPath := tmpFile.Name() + + defer func() { + if err != nil { + _ = os.Remove(tmpPath) + } + }() + + if _, err := tmpFile.Write(data); err != nil { + _ = tmpFile.Close() + + return fmt.Errorf("failed to write content: %w", err) + } + + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("failed to close temp file: %w", err) + } + + // Atomic rename + if err := os.Rename(filepath.Clean(tmpPath), filepath.Clean(path)); err != nil { + return fmt.Errorf("failed to rename temp file: %w", err) + } + + return nil +} + // hashToPath converts a hash to a file path: /// func (s *ContentStorage) hashToPath(hash ContentHash) string { h := string(hash) @@ -552,7 +553,6 @@ func (s *VariantStorage) DeleteWithMeta(key VariantKey) error { metaPath := s.keyToPath(key) + ".meta" - //nolint:gosec // G703: path derived from cache key err := os.Remove(metaPath) if err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to delete variant metadata: %w", err) diff --git a/internal/imgcache/testutil_internal_test.go b/internal/imgcache/testutil_internal_test.go index e8d53f7..1ca7c11 100644 --- a/internal/imgcache/testutil_internal_test.go +++ b/internal/imgcache/testutil_internal_test.go @@ -20,10 +20,11 @@ import ( // Shared test data literals, extracted as constants for goconst. const ( - testHostCDN = "cdn.example.com" - testHostExample = "example.com" - testPathCat = "/photos/cat.jpg" - testContentTypeJPEG = "image/jpeg" + testHostCDN = "cdn.example.com" + testHostExample = "example.com" + testPathCat = "/photos/cat.jpg" + testContentTypeJPEG = "image/jpeg" + testHeaderContentType = "Content-Type" ) // TestFixtures contains paths to test files in the mock filesystem.