refactor: plain error assignment in eviction, storage and config (noinlineerr)

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.
This commit is contained in:
2026-08-09 13:34:27 +00:00
parent 3ef9715054
commit 888c3a409a
4 changed files with 104 additions and 53 deletions

View File

@@ -36,7 +36,9 @@ type FreeSpaceProbeFunc func(path string) (uint64, error)
// the given path, as available to unprivileged processes. // the given path, as available to unprivileged processes.
func defaultFreeSpaceProbe(path string) (uint64, error) { func defaultFreeSpaceProbe(path string) (uint64, error) {
var stat syscall.Statfs_t var stat syscall.Statfs_t
if err := syscall.Statfs(path, &stat); err != nil {
err := syscall.Statfs(path, &stat)
if err != nil {
return 0, err return 0, err
} }
@@ -82,9 +84,10 @@ func (c *Config) resolveCacheMaxBytes(
if !c.cacheMaxBytesExplicit { if !c.cacheMaxBytesExplicit {
cacheDir := filepath.Join(c.StateDir, "cache") cacheDir := filepath.Join(c.StateDir, "cache")
if err := os.MkdirAll(cacheDir, cacheDirPerms); err != nil { err := os.MkdirAll(cacheDir, cacheDirPerms)
if err != nil {
return fmt.Errorf("config key %q: cannot create cache directory %q: %w", return fmt.Errorf("config key %q: cannot create cache directory %q: %w",
"cache_max_bytes", cacheDir, err) keyCacheMaxBytes, cacheDir, err)
} }
limit, err := ComputeDefaultCacheMaxBytes(cacheDir, probe) limit, err := ComputeDefaultCacheMaxBytes(cacheDir, probe)

View File

@@ -134,7 +134,8 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) {
return nil, err return nil, err
} }
if err := c.resolveCacheMaxBytes(log, defaultFreeSpaceProbe); err != nil { err = c.resolveCacheMaxBytes(log, defaultFreeSpaceProbe)
if err != nil {
return nil, err return nil, err
} }

View File

@@ -130,7 +130,8 @@ func (c *Cache) evictBatch(ctx context.Context, excessBytes int64) (int64, error
break break
} }
if err := c.evictCandidate(ctx, candidate); err != nil { err := c.evictCandidate(ctx, candidate)
if err != nil {
c.log.Warn("failed to evict cache entry", c.log.Warn("failed to evict cache entry",
"cache_key", candidate.cacheKey, "cache_key", candidate.cacheKey,
"content_hash", candidate.contentHash, "content_hash", candidate.contentHash,
@@ -209,7 +210,9 @@ func (c *Cache) variantCandidates(ctx context.Context) ([]evictionCandidate, err
candidate := evictionCandidate{isVariant: true} candidate := evictionCandidate{isVariant: true}
var key string var key string
if err := rows.Scan(&key, &candidate.sizeBytes, &candidate.lastAccessedAt); err != nil {
err := rows.Scan(&key, &candidate.sizeBytes, &candidate.lastAccessedAt)
if err != nil {
return nil, fmt.Errorf("failed to scan variant candidate: %w", err) return nil, fmt.Errorf("failed to scan variant candidate: %w", err)
} }
@@ -217,7 +220,8 @@ func (c *Cache) variantCandidates(ctx context.Context) ([]evictionCandidate, err
candidates = append(candidates, candidate) candidates = append(candidates, candidate)
} }
if err := rows.Err(); err != nil { err = rows.Err()
if err != nil {
return nil, fmt.Errorf("variant candidate iteration failed: %w", err) return nil, fmt.Errorf("variant candidate iteration failed: %w", err)
} }
@@ -246,7 +250,9 @@ func (c *Cache) sourceCandidates(ctx context.Context) ([]evictionCandidate, erro
var candidate evictionCandidate var candidate evictionCandidate
var hash string var hash string
if err := rows.Scan(&hash, &candidate.sizeBytes, &candidate.lastAccessedAt); err != nil {
err := rows.Scan(&hash, &candidate.sizeBytes, &candidate.lastAccessedAt)
if err != nil {
return nil, fmt.Errorf("failed to scan source candidate: %w", err) return nil, fmt.Errorf("failed to scan source candidate: %w", err)
} }
@@ -254,7 +260,8 @@ func (c *Cache) sourceCandidates(ctx context.Context) ([]evictionCandidate, erro
candidates = append(candidates, candidate) candidates = append(candidates, candidate)
} }
if err := rows.Err(); err != nil { err = rows.Err()
if err != nil {
return nil, fmt.Errorf("source candidate iteration failed: %w", err) return nil, fmt.Errorf("source candidate iteration failed: %w", err)
} }
@@ -271,7 +278,8 @@ func (c *Cache) evictVariant(ctx context.Context, cacheKey VariantKey) error {
return fmt.Errorf("failed to delete variant accounting row: %w", err) return fmt.Errorf("failed to delete variant accounting row: %w", err)
} }
if err := c.variants.DeleteWithMeta(cacheKey); err != nil { err = c.variants.DeleteWithMeta(cacheKey)
if err != nil {
return err return err
} }
@@ -315,17 +323,20 @@ func (c *Cache) evictSourceBlob(ctx context.Context, contentHash ContentHash) er
defer func() { _ = tx.Rollback() }() defer func() { _ = tx.Rollback() }()
if _, err := tx.ExecContext(ctx, _, err = tx.ExecContext(ctx,
`DELETE FROM source_metadata WHERE content_hash = ?`, string(contentHash)); err != nil { `DELETE FROM source_metadata WHERE content_hash = ?`, string(contentHash))
if err != nil {
return fmt.Errorf("failed to delete source metadata rows: %w", err) return fmt.Errorf("failed to delete source metadata rows: %w", err)
} }
if _, err := tx.ExecContext(ctx, _, err = tx.ExecContext(ctx,
`DELETE FROM source_content WHERE content_hash = ?`, string(contentHash)); err != nil { `DELETE FROM source_content WHERE content_hash = ?`, string(contentHash))
if err != nil {
return fmt.Errorf("failed to delete source content row: %w", err) return fmt.Errorf("failed to delete source content row: %w", err)
} }
if err := tx.Commit(); err != nil { err = tx.Commit()
if err != nil {
return fmt.Errorf("failed to commit eviction transaction: %w", err) return fmt.Errorf("failed to commit eviction transaction: %w", err)
} }
@@ -335,13 +346,15 @@ func (c *Cache) evictSourceBlob(ctx context.Context, contentHash ContentHash) er
// Only after the rows are gone may the files be removed. // Only after the rows are gone may the files be removed.
for _, reference := range references { for _, reference := range references {
if err := c.srcMetadata.Delete(reference.host, reference.pathHash); err != nil { err := c.srcMetadata.Delete(reference.host, reference.pathHash)
if err != nil {
c.log.Warn("failed to delete metadata sidecar", c.log.Warn("failed to delete metadata sidecar",
"host", reference.host, "path_hash", reference.pathHash, "error", err) "host", reference.host, "path_hash", reference.pathHash, "error", err)
} }
} }
if err := c.srcContent.Delete(contentHash); err != nil { err = c.srcContent.Delete(contentHash)
if err != nil {
return err return err
} }
@@ -368,7 +381,9 @@ func (c *Cache) sourceReferences(
var reference sourceReference var reference sourceReference
var pathHash string var pathHash string
if err := rows.Scan(&reference.host, &pathHash); err != nil {
err := rows.Scan(&reference.host, &pathHash)
if err != nil {
return nil, fmt.Errorf("failed to scan source reference: %w", err) return nil, fmt.Errorf("failed to scan source reference: %w", err)
} }
@@ -376,7 +391,8 @@ func (c *Cache) sourceReferences(
references = append(references, reference) references = append(references, reference)
} }
if err := rows.Err(); err != nil { err = rows.Err()
if err != nil {
return nil, fmt.Errorf("source reference iteration failed: %w", err) return nil, fmt.Errorf("source reference iteration failed: %w", err)
} }
@@ -465,7 +481,8 @@ func (c *Cache) evictionLoop(interval time.Duration) {
// runEvictionPass runs one eviction pass, logging failures instead of // runEvictionPass runs one eviction pass, logging failures instead of
// propagating them (the loop must keep running). // propagating them (the loop must keep running).
func (c *Cache) runEvictionPass(ctx context.Context) { func (c *Cache) runEvictionPass(ctx context.Context) {
if err := c.EvictToLimit(ctx); err != nil { err := c.EvictToLimit(ctx)
if err != nil {
c.log.Warn("cache eviction pass failed", "error", err) c.log.Warn("cache eviction pass failed", "error", err)
} }
} }
@@ -473,7 +490,8 @@ func (c *Cache) runEvictionPass(ctx context.Context) {
// runReconciliationPass runs one reconciliation pass, logging failures // runReconciliationPass runs one reconciliation pass, logging failures
// instead of propagating them (the loop must keep running). // instead of propagating them (the loop must keep running).
func (c *Cache) runReconciliationPass(ctx context.Context) { func (c *Cache) runReconciliationPass(ctx context.Context) {
if err := c.reconcileAccounting(ctx); err != nil { err := c.reconcileAccounting(ctx)
if err != nil {
c.log.Warn("cache accounting reconciliation failed", "error", err) c.log.Warn("cache accounting reconciliation failed", "error", err)
} }
} }
@@ -494,19 +512,23 @@ func (c *Cache) reconcileAccounting(ctx context.Context) error {
return nil return nil
} }
if err := c.reconcileVariantFiles(ctx); err != nil { err := c.reconcileVariantFiles(ctx)
if err != nil {
return err return err
} }
if err := c.reconcileVariantRows(ctx); err != nil { err = c.reconcileVariantRows(ctx)
if err != nil {
return err return err
} }
if err := c.reconcileSourceFiles(ctx); err != nil { err = c.reconcileSourceFiles(ctx)
if err != nil {
return err return err
} }
if err := c.reconcileSourceRows(ctx); err != nil { err = c.reconcileSourceRows(ctx)
if err != nil {
return err return err
} }
@@ -611,8 +633,9 @@ func (c *Cache) reconcileVariantRows(ctx context.Context) error {
continue continue
} }
if _, err := c.db.ExecContext(ctx, _, err := c.db.ExecContext(ctx,
`DELETE FROM variant_content WHERE cache_key = ?`, string(key)); err != nil { `DELETE FROM variant_content WHERE cache_key = ?`, string(key))
if err != nil {
return fmt.Errorf("failed to drop stale variant accounting row: %w", err) return fmt.Errorf("failed to drop stale variant accounting row: %w", err)
} }
@@ -635,14 +658,17 @@ func (c *Cache) allVariantKeys(ctx context.Context) ([]VariantKey, error) {
for rows.Next() { for rows.Next() {
var key string var key string
if err := rows.Scan(&key); err != nil {
err := rows.Scan(&key)
if err != nil {
return nil, fmt.Errorf("failed to scan variant key: %w", err) return nil, fmt.Errorf("failed to scan variant key: %w", err)
} }
keys = append(keys, VariantKey(key)) keys = append(keys, VariantKey(key))
} }
if err := rows.Err(); err != nil { err = rows.Err()
if err != nil {
return nil, fmt.Errorf("variant key iteration failed: %w", err) return nil, fmt.Errorf("variant key iteration failed: %w", err)
} }
@@ -693,12 +719,14 @@ func (c *Cache) removeUntrackedSourceFile(
return nil return nil
} }
if _, err := c.db.ExecContext(ctx, _, err = c.db.ExecContext(ctx,
`DELETE FROM source_metadata WHERE content_hash = ?`, string(contentHash)); err != nil { `DELETE FROM source_metadata WHERE content_hash = ?`, string(contentHash))
if err != nil {
return fmt.Errorf("failed to delete metadata rows for untracked blob: %w", err) return fmt.Errorf("failed to delete metadata rows for untracked blob: %w", err)
} }
if err := os.Remove(path); err != nil && !os.IsNotExist(err) { err = os.Remove(path)
if err != nil && !os.IsNotExist(err) {
return fmt.Errorf("failed to remove untracked source file: %w", err) return fmt.Errorf("failed to remove untracked source file: %w", err)
} }
@@ -722,7 +750,8 @@ func (c *Cache) reconcileSourceRows(ctx context.Context) error {
// The blob file is already gone; evictSourceBlob removes the // The blob file is already gone; evictSourceBlob removes the
// rows and sidecars and tolerates the missing file. // rows and sidecars and tolerates the missing file.
if err := c.evictSourceBlob(ctx, hash); err != nil { err := c.evictSourceBlob(ctx, hash)
if err != nil {
return err return err
} }
@@ -745,14 +774,17 @@ func (c *Cache) allSourceContentHashes(ctx context.Context) ([]ContentHash, erro
for rows.Next() { for rows.Next() {
var hash string var hash string
if err := rows.Scan(&hash); err != nil {
err := rows.Scan(&hash)
if err != nil {
return nil, fmt.Errorf("failed to scan content hash: %w", err) return nil, fmt.Errorf("failed to scan content hash: %w", err)
} }
hashes = append(hashes, ContentHash(hash)) hashes = append(hashes, ContentHash(hash))
} }
if err := rows.Err(); err != nil { err = rows.Err()
if err != nil {
return nil, fmt.Errorf("content hash iteration failed: %w", err) return nil, fmt.Errorf("content hash iteration failed: %w", err)
} }
@@ -771,7 +803,8 @@ func (c *Cache) sweepStaleTempFile(path string, entry fs.DirEntry) {
return return
} }
if err := os.Remove(path); err != nil && !os.IsNotExist(err) { err = os.Remove(path)
if err != nil && !os.IsNotExist(err) {
c.log.Warn("failed to remove stale temp file", "path", path, "error", err) c.log.Warn("failed to remove stale temp file", "path", path, "error", err)
return return

View File

@@ -66,7 +66,8 @@ func (s *ContentStorage) Store(r io.Reader) (ContentHash, int64, error) {
hash := ContentHash(hex.EncodeToString(h[:])) hash := ContentHash(hex.EncodeToString(h[:]))
size := int64(len(data)) size := int64(len(data))
if err := s.writeIfAbsent(hash, data); err != nil { err = s.writeIfAbsent(hash, data)
if err != nil {
return "", 0, err return "", 0, err
} }
@@ -81,8 +82,9 @@ func (s *ContentStorage) Store(r io.Reader) (ContentHash, int64, error) {
// left untouched. // left untouched.
func (s *ContentStorage) StoreHashed( func (s *ContentStorage) StoreHashed(
hash ContentHash, data []byte, hash ContentHash, data []byte,
) (size int64, err error) { ) (int64, error) {
if err := s.writeIfAbsent(hash, data); err != nil { err := s.writeIfAbsent(hash, data)
if err != nil {
return 0, err return 0, err
} }
@@ -151,18 +153,21 @@ func (s *ContentStorage) Exists(hash ContentHash) bool {
// writeIfAbsent writes data to the path derived from hash, unless // writeIfAbsent writes data to the path derived from hash, unless
// content already exists there, via a temp-file-plus-rename so // content already exists there, via a temp-file-plus-rename so
// concurrent readers never observe a partial file. // concurrent readers never observe a partial file.
func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) (err error) { func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) error {
// Build path: <basedir>/<ab>/<cd>/<hash> // Build path: <basedir>/<ab>/<cd>/<hash>
path := s.hashToPath(hash) path := s.hashToPath(hash)
// Check if already exists // Check if already exists
if _, statErr := os.Stat(path); statErr == nil { _, statErr := os.Stat(path)
if statErr == nil {
return nil return nil
} }
// Create directory structure // Create directory structure
dir := filepath.Dir(path) dir := filepath.Dir(path)
if err := os.MkdirAll(dir, StorageDirPerm); err != nil {
err := os.MkdirAll(dir, StorageDirPerm)
if err != nil {
return fmt.Errorf("failed to create directory: %w", err) return fmt.Errorf("failed to create directory: %w", err)
} }
@@ -174,24 +179,32 @@ func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) (err error
tmpPath := tmpFile.Name() tmpPath := tmpFile.Name()
defer func() { // Each failure path below unlinks the temp file explicitly. This
// replaces a deferred cleanup that read a named result, which the
// canonical config does not permit; the set of paths that remove
// tmpPath, and the order relative to Close, is unchanged. This
// mirrors how MetadataStorage.Store and VariantStorage.Store below
// already express the same cleanup.
_, err = tmpFile.Write(data)
if err != nil { if err != nil {
_ = os.Remove(tmpPath)
}
}()
if _, err := tmpFile.Write(data); err != nil {
_ = tmpFile.Close() _ = tmpFile.Close()
_ = os.Remove(tmpPath)
return fmt.Errorf("failed to write content: %w", err) return fmt.Errorf("failed to write content: %w", err)
} }
if err := tmpFile.Close(); err != nil { err = tmpFile.Close()
if err != nil {
_ = os.Remove(tmpPath)
return fmt.Errorf("failed to close temp file: %w", err) return fmt.Errorf("failed to close temp file: %w", err)
} }
// Atomic rename // Atomic rename
if err := os.Rename(filepath.Clean(tmpPath), filepath.Clean(path)); err != nil { err = os.Rename(filepath.Clean(tmpPath), filepath.Clean(path))
if err != nil {
_ = os.Remove(tmpPath)
return fmt.Errorf("failed to rename temp file: %w", err) return fmt.Errorf("failed to rename temp file: %w", err)
} }
@@ -547,13 +560,14 @@ func (s *VariantStorage) Delete(key VariantKey) error {
// DeleteWithMeta removes the content at the given key together with // DeleteWithMeta removes the content at the given key together with
// its .meta sidecar file. A missing file is not an error. // its .meta sidecar file. A missing file is not an error.
func (s *VariantStorage) DeleteWithMeta(key VariantKey) error { func (s *VariantStorage) DeleteWithMeta(key VariantKey) error {
if err := s.Delete(key); err != nil { err := s.Delete(key)
if err != nil {
return err return err
} }
metaPath := s.keyToPath(key) + ".meta" metaPath := s.keyToPath(key) + ".meta"
err := os.Remove(metaPath) err = os.Remove(metaPath)
if err != nil && !os.IsNotExist(err) { if err != nil && !os.IsNotExist(err) {
return fmt.Errorf("failed to delete variant metadata: %w", err) return fmt.Errorf("failed to delete variant metadata: %w", err)
} }