diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index 0a54392..d50e858 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -2,7 +2,9 @@ package imgcache import ( "context" + "crypto/sha256" "database/sql" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -215,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) } diff --git a/internal/imgcache/eviction.go b/internal/imgcache/eviction.go index d68708a..7574822 100644 --- a/internal/imgcache/eviction.go +++ b/internal/imgcache/eviction.go @@ -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 diff --git a/internal/imgcache/storage.go b/internal/imgcache/storage.go index 9891074..ed1ea4c 100644 --- a/internal/imgcache/storage.go +++ b/internal/imgcache/storage.go @@ -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: /// 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.