diff --git a/TODO.md b/TODO.md index 0618377..e76626d 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,23 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-26 the cache keeps its source images in one `imgcache.FileStore` and + its variants in another (closes #318). A `FileStore` is a directory of files, + each named by a hex key, at `//`. + - `ContentStorage` and `VariantStorage` are gone. + - `FileStore` has these methods: + - `Write`, which writes a file under a temp name, syncs it and renames + it into place + - `Open`, which returns the file and its size + - `Size` + - `Delete` + - `FileStore` has no `Exists`. The reconciliation pass checks that a + variant's file is there with `Size`. The tests look for the file on disk. + - `Cache.Variant` reads the content type from the variant file's first + bytes, as `VariantStorage.LoadWithContentType` did. A cache hit still + makes no database call. + - `storage.go` is now `filestore.go`. `internal/imgcache/testdata/red.avif`, + which no test used, is gone. - 2026-10-26 `internal/imgcache` holds the cache only (closes #317). The new package `internal/imageproxy` fetches, transforms, caches and serves images. It imports `imgcache`, and `imgcache` does not import it. diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index ab5cbcf..d1376b2 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -19,6 +19,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "sneak.berlin/go/pixa/internal/imagerequest" + "sneak.berlin/go/pixa/internal/magic" ) // maxPendingAccessTimes is how many access times of variants served from @@ -57,8 +58,8 @@ type Config struct { // Cache implements the caching layer for the image proxy. type Cache struct { db *sql.DB - srcContent *ContentStorage // source images by source key - variants *VariantStorage // processed variants by cache key + srcContent *FileStore // source images by source key + variants *FileStore // processed variants by cache key config Config log *slog.Logger @@ -135,14 +136,14 @@ func NewCache( return c, nil } - srcContent, err := NewContentStorage( + srcContent, err := NewFileStore( filepath.Join(config.StateDir, "cache", "sources"), ) if err != nil { return nil, fmt.Errorf("failed to create source content storage: %w", err) } - variants, err := NewVariantStorage( + variants, err := NewFileStore( filepath.Join(config.StateDir, "cache", "variants"), ) if err != nil { @@ -169,14 +170,21 @@ func (c *Cache) Variant( return nil, 0, "", ErrNotFound } - reader, size, contentType, err := c.variants.LoadWithContentType(cacheKey) + file, size, err := c.variants.Open(string(cacheKey)) if err != nil { return nil, 0, "", err } + contentType, err := outputContentType(file) + if err != nil { + _ = file.Close() + + return nil, 0, "", err + } + c.touchVariant(cacheKey) - return reader, size, contentType, nil + return file, size, contentType, nil } // PutVariant stores a processed variant by its cache key and records @@ -190,11 +198,18 @@ func (c *Cache) PutVariant( return nil } - size, err := c.variants.Store(cacheKey, content) + data, err := io.ReadAll(content) + if err != nil { + return fmt.Errorf("failed to read content: %w", err) + } + + err = c.variants.Write(string(cacheKey), data) if err != nil { return err } + size := int64(len(data)) + _, err = c.db.ExecContext(ctx, ` INSERT INTO variant_content (cache_key, size_bytes) VALUES (?, ?) @@ -238,14 +253,14 @@ func (c *Cache) Source( return nil, 0, ErrNotFound } - reader, size, err := c.srcContent.LoadWithSize(key) + file, size, err := c.srcContent.Open(string(key)) if err != nil { return nil, 0, err } c.touchSourceContent(key) - return reader, size, nil + return file, size, nil } // PutSource stores fetched source content: it writes the file named by @@ -270,11 +285,13 @@ func (c *Cache) PutSource( key := src.Key() - size, err := c.srcContent.Store(key, data) + err = c.srcContent.Write(string(key), data) if err != nil { return "", fmt.Errorf("failed to store source content: %w", err) } + size := int64(len(data)) + _, err = c.db.ExecContext(ctx, ` INSERT INTO source_content (source_key, size_bytes) VALUES (?, ?) @@ -283,7 +300,7 @@ func (c *Cache) PutSource( last_accessed_at = CURRENT_TIMESTAMP `, string(key), size) if err != nil { - deleteErr := c.srcContent.Delete(key) + deleteErr := c.srcContent.Delete(string(key)) if deleteErr != nil { c.log.Warn("failed to remove source file whose row was not written", "source_key", key, "error", deleteErr) @@ -422,3 +439,30 @@ func (c *Cache) cleanExpiredBatch(ctx context.Context) (int64, error) { return deleted, nil } + +// errNotAnOutputFormat is returned for a variant file whose first bytes are +// those of a format pixa reads but never writes, such as SVG. +var errNotAnOutputFormat = errors.New("variant file is not in an output format") + +// outputContentType reads the first bytes of content, without moving its +// offset, and returns the content type of the output format they show. +func outputContentType(content io.ReaderAt) (string, error) { + firstBytes := make([]byte, magic.MinMagicBytes) + + _, err := content.ReadAt(firstBytes, 0) + if err != nil { + return "", fmt.Errorf("failed to read the first bytes: %w", err) + } + + detected, err := magic.DetectFormat(firstBytes) + if err != nil { + return "", err + } + + _, isOutputFormat := magic.MIMEToImageFormat(string(detected)) + if !isOutputFormat { + return "", fmt.Errorf("%w: %s", errNotAnOutputFormat, detected) + } + + return string(detected), nil +} diff --git a/internal/imgcache/cache_hit_internal_test.go b/internal/imgcache/cache_hit_internal_test.go index 068bce0..23e2e07 100644 --- a/internal/imgcache/cache_hit_internal_test.go +++ b/internal/imgcache/cache_hit_internal_test.go @@ -104,7 +104,7 @@ func TestVariant_RefusesFileOfNoOutputFormat(t *testing.T) { for i, tt := range tests { key := imagerequest.VariantKey(fmt.Sprintf("aabbccdd20%02d", i)) - _, err := cache.variants.Store(key, bytes.NewReader(tt.content)) + err := cache.variants.Write(string(key), tt.content) if err != nil { t.Fatalf("%s: storing the variant file: %v", tt.name, err) } diff --git a/internal/imgcache/eviction.go b/internal/imgcache/eviction.go index 8b576ad..ef5f346 100644 --- a/internal/imgcache/eviction.go +++ b/internal/imgcache/eviction.go @@ -48,7 +48,7 @@ const staleTempFileAge = time.Hour const sqliteTimestampLayout = "2006-01-02 15:04:05" // tempFilePrefix is the prefix os.CreateTemp uses for in-flight cache -// writes (".tmp-*" patterns in the storage layer). +// writes (the ".tmp-*" pattern in writeFileSynced). const tempFilePrefix = ".tmp-" // UsageBytes returns the total number of bytes of cache content @@ -306,7 +306,7 @@ func (c *Cache) evictVariant( return fmt.Errorf("failed to delete variant accounting row: %w", err) } - err = c.variants.Delete(cacheKey) + err = c.variants.Delete(string(cacheKey)) if err != nil { return err } @@ -325,7 +325,7 @@ func (c *Cache) evictSourceBlob(ctx context.Context, key imagerequest.SourceKey) return fmt.Errorf("failed to delete source content row: %w", err) } - return c.srcContent.Delete(key) + return c.srcContent.Delete(string(key)) } // notifyWritePressure wakes the background evictor after a store, so @@ -636,11 +636,12 @@ func (c *Cache) reconcileVariantRows(ctx context.Context) error { return ctx.Err() } - if c.variants.Exists(key) { + _, err := c.variants.Size(string(key)) + if err == nil { continue } - _, err := c.db.ExecContext(ctx, + _, err = c.db.ExecContext(ctx, `DELETE FROM variant_content WHERE cache_key = ?`, string(key)) if err != nil { return fmt.Errorf("failed to drop stale variant accounting row: %w", err) diff --git a/internal/imgcache/eviction_internal_test.go b/internal/imgcache/eviction_internal_test.go index 898d990..7be430e 100644 --- a/internal/imgcache/eviction_internal_test.go +++ b/internal/imgcache/eviction_internal_test.go @@ -209,7 +209,8 @@ func assertNoDanglingReferences(t *testing.T, cache *Cache) { t.Fatalf("failed to scan source_key: %v", err) } - if !cache.srcContent.Exists(imagerequest.SourceKey(key)) { + _, err = os.Stat(cache.srcContent.keyToPath(key)) + if err != nil { t.Errorf("source_content references content %s but the file is missing", key) } @@ -236,7 +237,8 @@ func assertNoDanglingReferences(t *testing.T, cache *Cache) { t.Fatalf("failed to scan cache_key: %v", err) } - if !cache.variants.Exists(imagerequest.VariantKey(key)) { + _, err = os.Stat(cache.variants.keyToPath(key)) + if err != nil { t.Errorf("variant_content references key %s but the file is missing", key) } } @@ -334,7 +336,8 @@ func TestEvictToLimitEvictsLeastRecentlyUsedFirst(t *testing.T) { t.Errorf("usage after eviction = %d, want <= %d", usage, limit) } - if cache.variants.Exists(keys[0]) { + _, err = os.Stat(cache.variants.keyToPath(string(keys[0]))) + if err == nil { t.Errorf("least recently used variant %s must be evicted", keys[0]) } @@ -345,7 +348,8 @@ func TestEvictToLimitEvictsLeastRecentlyUsedFirst(t *testing.T) { } for _, key := range keys[1:] { - if !cache.variants.Exists(key) { + _, err = os.Stat(cache.variants.keyToPath(string(key))) + if err != nil { t.Errorf("more recently used variant %s must survive eviction", key) } } @@ -373,7 +377,8 @@ func TestEvictionKeepsEverythingWhenUnderLimit(t *testing.T) { } for _, key := range keys { - if !cache.srcContent.Exists(key) { + _, err = os.Stat(cache.srcContent.keyToPath(string(key))) + if err != nil { t.Errorf("source %s must not be evicted while usage is under the limit", key) } @@ -384,7 +389,8 @@ func TestEvictionKeepsEverythingWhenUnderLimit(t *testing.T) { } } - if !cache.variants.Exists(testVariantKeyOne) { + _, err = os.Stat(cache.variants.keyToPath(string(testVariantKeyOne))) + if err != nil { t.Error("variant must not be evicted while usage is under the limit") } @@ -586,7 +592,7 @@ func TestEvictionRunsOnPeriodicSchedule(t *testing.T) { for i, key := range keys { content := bytes.Repeat([]byte{fills[i]}, 1000) - _, err = cache.variants.Store(key, bytes.NewReader(content)) + err = cache.variants.Write(string(key), content) if err != nil { t.Fatalf("failed to store variant file: %v", err) } @@ -605,7 +611,8 @@ func TestEvictionRunsOnPeriodicSchedule(t *testing.T) { stored = 0 for _, key := range keys { - if cache.variants.Exists(key) { + _, err = os.Stat(cache.variants.keyToPath(string(key))) + if err == nil { stored++ } } @@ -637,9 +644,7 @@ func TestStartEvictionReconcilesAccountingWithDisk(t *testing.T) { // feature existed) must be adopted into the accounting. untracked := bytes.Repeat([]byte{0x31}, 1000) - _, err := cache.variants.Store( - testVariantKeyOne, bytes.NewReader(untracked), - ) + err := cache.variants.Write(string(testVariantKeyOne), untracked) if err != nil { t.Fatalf("failed to store untracked variant file: %v", err) } @@ -738,9 +743,7 @@ func TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup(t *testing.T) { // insert had failed and only the file write had succeeded. untracked := bytes.Repeat([]byte{0x41}, 900) - _, err = cache.variants.Store( - "aabbccdd0099", bytes.NewReader(untracked), - ) + err = cache.variants.Write("aabbccdd0099", untracked) if err != nil { t.Fatalf("failed to store untracked variant file: %v", err) } @@ -1046,13 +1049,15 @@ func assertOnlyOldestSourceEvicted( ) { t.Helper() - if cache.srcContent.Exists(keys[0]) { + _, err := os.Stat(cache.srcContent.keyToPath(string(keys[0]))) + if err == nil { t.Errorf("source %s, evicted when the context was cancelled, "+ "is still on disk", keys[0]) } for _, key := range keys[1:] { - if !cache.srcContent.Exists(key) { + _, err = os.Stat(cache.srcContent.keyToPath(string(key))) + if err != nil { t.Errorf("source %s was evicted after the context was cancelled", key) } diff --git a/internal/imgcache/filestore.go b/internal/imgcache/filestore.go new file mode 100644 index 0000000..779ff65 --- /dev/null +++ b/internal/imgcache/filestore.go @@ -0,0 +1,185 @@ +package imgcache + +import ( + "errors" + "fmt" + "os" + "path/filepath" +) + +// Storage constants. +const ( + // StorageDirPerm is the permission mode for storage directories. + StorageDirPerm = 0750 + // StorageFilePerm is the permission mode for storage files. + StorageFilePerm = 0600 + // MinHashLength is the minimum hash length for path splitting. + MinHashLength = 4 +) + +// Storage errors. +var ( + ErrNotFound = errors.New("content not found") +) + +// FileStore is a directory of files, each named by a hex key. The file for +// a key is at ///, where and are the key's +// first and second pairs of characters. The cache keeps its source images in +// one FileStore and its variants in another. +type FileStore struct { + baseDir string +} + +// NewFileStore creates the directory baseDir if it is missing and returns +// the FileStore of the files in it. +func NewFileStore(baseDir string) (*FileStore, error) { + err := os.MkdirAll(baseDir, StorageDirPerm) + if err != nil { + return nil, fmt.Errorf("failed to create storage directory: %w", err) + } + + return &FileStore{baseDir: baseDir}, nil +} + +// Write writes data with writeFileSynced to the file for key, replacing any +// file already there. +func (s *FileStore) Write(key string, data []byte) error { + path := s.keyToPath(key) + + err := os.MkdirAll(filepath.Dir(path), StorageDirPerm) + if err != nil { + return fmt.Errorf("failed to create directory: %w", err) + } + + return writeFileSynced(path, data) +} + +// Open opens the file for key and returns it with its size in bytes. A +// missing file is ErrNotFound. +func (s *FileStore) Open(key string) (*os.File, int64, error) { + f, err := os.Open(s.keyToPath(key)) + if err != nil { + if os.IsNotExist(err) { + return nil, 0, ErrNotFound + } + + return nil, 0, fmt.Errorf("failed to open content: %w", err) + } + + stat, err := f.Stat() + if err != nil { + _ = f.Close() + + return nil, 0, fmt.Errorf("failed to stat content: %w", err) + } + + return f, stat.Size(), nil +} + +// Size returns the size in bytes of the file for key. A missing file is +// ErrNotFound. +func (s *FileStore) Size(key string) (int64, error) { + stat, err := os.Stat(s.keyToPath(key)) + if err != nil { + if os.IsNotExist(err) { + return 0, ErrNotFound + } + + return 0, fmt.Errorf("failed to stat content: %w", err) + } + + return stat.Size(), nil +} + +// Delete removes the file for key. A missing file is not an error. +func (s *FileStore) Delete(key string) error { + err := os.Remove(s.keyToPath(key)) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to delete content: %w", err) + } + + return nil +} + +// writeFileSynced writes data to path through a temp file in the same +// directory: the temp file is synced to disk, renamed to path, and the +// directory synced after, so that neither a concurrent reader nor a +// crash finds a partial file under path. When the directory sync fails, +// the file is removed from path, so that a failed write leaves no file +// there. +func writeFileSynced(path string, data []byte) error { + dir := filepath.Dir(path) + + tmpFile, err := os.CreateTemp(dir, ".tmp-*") + if err != nil { + return fmt.Errorf("failed to create temp file: %w", err) + } + + tmpPath := tmpFile.Name() + + _, err = tmpFile.Write(data) + if err != nil { + _ = tmpFile.Close() + _ = os.Remove(tmpPath) + + return fmt.Errorf("failed to write temp file: %w", err) + } + + err = tmpFile.Sync() + if err != nil { + _ = tmpFile.Close() + _ = os.Remove(tmpPath) + + return fmt.Errorf("failed to sync temp file: %w", err) + } + + err = tmpFile.Close() + if err != nil { + _ = os.Remove(tmpPath) + + return fmt.Errorf("failed to close temp file: %w", err) + } + + 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) + } + + err = syncDirectory(dir) + if err != nil { + _ = os.Remove(path) + + return err + } + + return nil +} + +// syncDirectory syncs the directory dir to disk, so that a file just +// renamed into it is still there after a crash. +func syncDirectory(dir string) error { + d, err := os.Open(filepath.Clean(dir)) + if err != nil { + return fmt.Errorf("failed to open directory: %w", err) + } + + err = d.Sync() + _ = d.Close() + + if err != nil { + return fmt.Errorf("failed to sync directory: %w", err) + } + + return nil +} + +// keyToPath converts a key to a file path: /// +func (s *FileStore) keyToPath(key string) string { + if len(key) < MinHashLength { + return filepath.Join(s.baseDir, key) + } + + return filepath.Join(s.baseDir, key[0:2], key[2:4], key) +} diff --git a/internal/imgcache/filestore_internal_test.go b/internal/imgcache/filestore_internal_test.go new file mode 100644 index 0000000..06264c9 --- /dev/null +++ b/internal/imgcache/filestore_internal_test.go @@ -0,0 +1,184 @@ +package imgcache + +import ( + "bytes" + "errors" + "io" + "os" + "path/filepath" + "testing" + + "sneak.berlin/go/pixa/internal/imagerequest" +) + +func TestFileStore_WriteAndOpen(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + storage, err := NewFileStore(tmpDir) + if err != nil { + t.Fatalf("NewFileStore() error = %v", err) + } + + content := []byte("hello world") + key := string(imagerequest.Source{Host: testHostCDN, Path: testPathCat}.Key()) + + err = storage.Write(key, content) + if err != nil { + t.Fatalf("Write() error = %v", err) + } + + // Verify file exists at expected path + expectedPath := filepath.Join(tmpDir, key[0:2], key[2:4], key) + + _, err = os.Stat(expectedPath) + if err != nil { + t.Errorf("File not at expected path %s: %v", expectedPath, err) + } + + // Open and verify size and content + r, size, err := storage.Open(key) + if err != nil { + t.Fatalf("Open() error = %v", err) + } + + defer func() { _ = r.Close() }() + + if size != int64(len(content)) { + t.Errorf("Open() size = %d, want %d", size, len(content)) + } + + loaded, err := io.ReadAll(r) + if err != nil { + t.Fatalf("ReadAll() error = %v", err) + } + + if !bytes.Equal(loaded, content) { + t.Errorf("Open() content = %q, want %q", loaded, content) + } +} + +func TestFileStore_OpenNotFound(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + storage, err := NewFileStore(tmpDir) + if err != nil { + t.Fatalf("NewFileStore() error = %v", err) + } + + _, _, err = storage.Open("nonexistent") + if !errors.Is(err, ErrNotFound) { + t.Errorf("Open() error = %v, want ErrNotFound", err) + } +} + +// TestFileStore_Size checks that Size returns the size of a written file, +// and ErrNotFound for a key with no file. +func TestFileStore_Size(t *testing.T) { + t.Parallel() + + storage, err := NewFileStore(t.TempDir()) + if err != nil { + t.Fatalf("NewFileStore() error = %v", err) + } + + content := []byte("the file's content") + key := string(imagerequest.Source{Host: testHostCDN, Path: testPathCat}.Key()) + + err = storage.Write(key, content) + if err != nil { + t.Fatalf("Write() error = %v", err) + } + + size, err := storage.Size(key) + if err != nil || size != int64(len(content)) { + t.Errorf("Size() = %d, error %v; want %d", size, err, len(content)) + } + + _, err = storage.Size("nonexistent") + if !errors.Is(err, ErrNotFound) { + t.Errorf("Size() of a missing file error = %v, want ErrNotFound", err) + } +} + +func TestFileStore_Delete(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + storage, err := NewFileStore(tmpDir) + if err != nil { + t.Fatalf("NewFileStore() error = %v", err) + } + + content := []byte("to be deleted") + key := string(imagerequest.Source{Host: testHostCDN, Path: testPathCat}.Key()) + path := storage.keyToPath(key) + + err = storage.Write(key, content) + if err != nil { + t.Fatalf("Write() error = %v", err) + } + + _, err = os.Stat(path) + if err != nil { + t.Errorf("file missing before Delete(): %v", err) + } + + err = storage.Delete(key) + if err != nil { + t.Fatalf("Delete() error = %v", err) + } + + _, err = os.Stat(path) + if err == nil { + t.Error("file still on disk after Delete()") + } +} + +func TestFileStore_DeleteNonexistent(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + storage, err := NewFileStore(tmpDir) + if err != nil { + t.Fatalf("NewFileStore() error = %v", err) + } + + // Should not error + err = storage.Delete("nonexistent") + if err != nil { + t.Errorf("Delete() error = %v, want nil", err) + } +} + +func TestFileStore_KeyToPath(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + + storage, err := NewFileStore(tmpDir) + if err != nil { + t.Fatalf("NewFileStore() error = %v", err) + } + + // Test by writing and verifying the resulting path structure + content := []byte("test content for path verification") + key := string(imagerequest.Source{Host: testHostCDN, Path: testPathCat}.Key()) + + err = storage.Write(key, content) + if err != nil { + t.Fatalf("Write() error = %v", err) + } + + expectedPath := filepath.Join(tmpDir, key[0:2], key[2:4], key) + + _, err = os.Stat(expectedPath) + if err != nil { + t.Errorf("File not at expected path %s: %v", expectedPath, err) + } +} diff --git a/internal/imgcache/partial_cache_files_internal_test.go b/internal/imgcache/partial_cache_files_internal_test.go index b89b720..bcabf2a 100644 --- a/internal/imgcache/partial_cache_files_internal_test.go +++ b/internal/imgcache/partial_cache_files_internal_test.go @@ -67,8 +67,8 @@ func TestReconciliationRemovesDamagedFiles(t *testing.T) { sourceKey := storeEvictionTestSource(t, cache, testHostCDN, testPathCat, content) storeEvictionTestVariant(t, cache, testVariantKeyOne, content) - sourcePath := cache.srcContent.keyToPath(sourceKey) - variantPath := cache.variants.keyToPath(testVariantKeyOne) + sourcePath := cache.srcContent.keyToPath(string(sourceKey)) + variantPath := cache.variants.keyToPath(string(testVariantKeyOne)) for _, path := range []string{sourcePath, variantPath} { err := os.Truncate(path, size) @@ -82,11 +82,13 @@ func TestReconciliationRemovesDamagedFiles(t *testing.T) { assertWarningNames(t, logs, sourcePath) assertWarningNames(t, logs, variantPath) - if cache.srcContent.Exists(sourceKey) { + _, err := os.Stat(sourcePath) + if err == nil { t.Error("the damaged source file is still on disk") } - if cache.variants.Exists(testVariantKeyOne) { + _, err = os.Stat(variantPath) + if err == nil { t.Error("the damaged variant file is still on disk") } @@ -116,16 +118,19 @@ func TestReconciliationRemovesEmptyVariantFileWithoutRow(t *testing.T) { cache, _ := newEvictionTestCache(t, 1<<30) - _, err := cache.variants.Store(testVariantKeyOne, bytes.NewReader(nil)) + err := cache.variants.Write(string(testVariantKeyOne), nil) if err != nil { t.Fatalf("failed to store an empty variant file: %v", err) } + variantPath := cache.variants.keyToPath(string(testVariantKeyOne)) + logs := reconcileAsAtStartup(t, cache) - assertWarningNames(t, logs, cache.variants.keyToPath(testVariantKeyOne)) + assertWarningNames(t, logs, variantPath) - if cache.variants.Exists(testVariantKeyOne) { + _, err = os.Stat(variantPath) + if err == nil { t.Error("the empty variant file is still on disk") } @@ -137,19 +142,19 @@ func TestReconciliationRemovesEmptyVariantFileWithoutRow(t *testing.T) { } } -// TestContentStorage_StoreReplacesFileOfAnotherSize checks that a -// store over a file of another size at the source's path, such as a -// partial file a crash left, replaces it with the content. -func TestContentStorage_StoreReplacesFileOfAnotherSize(t *testing.T) { +// TestFileStore_WriteReplacesFileOfAnotherSize checks that a write over a +// file of another size at the source's path, such as a partial file a crash +// left, replaces it with the content. +func TestFileStore_WriteReplacesFileOfAnotherSize(t *testing.T) { t.Parallel() - storage, err := NewContentStorage(t.TempDir()) + storage, err := NewFileStore(t.TempDir()) if err != nil { - t.Fatalf("NewContentStorage() error = %v", err) + t.Fatalf("NewFileStore() error = %v", err) } content := []byte("the whole source image") - key := imagerequest.Source{Host: testHostCDN, Path: testPathCat}.Key() + key := string(imagerequest.Source{Host: testHostCDN, Path: testPathCat}.Key()) path := storage.keyToPath(key) err = os.MkdirAll(filepath.Dir(path), StorageDirPerm) @@ -162,14 +167,14 @@ func TestContentStorage_StoreReplacesFileOfAnotherSize(t *testing.T) { t.Fatalf("writing a partial file: %v", err) } - _, err = storage.Store(key, content) + err = storage.Write(key, content) if err != nil { - t.Fatalf("Store() error = %v", err) + t.Fatalf("Write() error = %v", err) } - stored, _, err := storage.LoadWithSize(key) + stored, _, err := storage.Open(key) if err != nil { - t.Fatalf("LoadWithSize() error = %v", err) + t.Fatalf("Open() error = %v", err) } defer func() { _ = stored.Close() }() diff --git a/internal/imgcache/source_content_internal_test.go b/internal/imgcache/source_content_internal_test.go index 3d12835..b9d7adb 100644 --- a/internal/imgcache/source_content_internal_test.go +++ b/internal/imgcache/source_content_internal_test.go @@ -74,7 +74,8 @@ func TestEvictSourceBlob_LeavesSourceWithIdenticalBytesServed(t *testing.T) { t.Fatalf("evictSourceBlob() error = %v", err) } - if cache.srcContent.Exists(evicted) { + _, err = os.Stat(cache.srcContent.keyToPath(string(evicted))) + if err == nil { t.Error("the evicted source's file is still on disk") } @@ -129,7 +130,8 @@ func TestPutSource_FailedRowWriteLeavesNoFile(t *testing.T) { t.Error("PutSource() error = nil, want the failed row write") } - if cache.srcContent.Exists(req.Source.Key()) { + _, err = os.Stat(cache.srcContent.keyToPath(string(req.Source.Key()))) + if err == nil { t.Error("the source's file is on disk although its row was not written") } } diff --git a/internal/imgcache/storage.go b/internal/imgcache/storage.go deleted file mode 100644 index d0da00f..0000000 --- a/internal/imgcache/storage.go +++ /dev/null @@ -1,326 +0,0 @@ -package imgcache - -import ( - "errors" - "fmt" - "io" - "os" - "path/filepath" - - "sneak.berlin/go/pixa/internal/imagerequest" - "sneak.berlin/go/pixa/internal/magic" -) - -// Storage constants. -const ( - // StorageDirPerm is the permission mode for storage directories. - StorageDirPerm = 0750 - // StorageFilePerm is the permission mode for storage files. - StorageFilePerm = 0600 - // MinHashLength is the minimum hash length for path splitting. - MinHashLength = 4 -) - -// Storage errors. -var ( - ErrNotFound = errors.New("content not found") -) - -// ContentStorage handles source image file storage, one file per source -// URL. Files are stored at: /// -type ContentStorage struct { - baseDir string -} - -// NewContentStorage creates a new content storage at the given base directory. -func NewContentStorage(baseDir string) (*ContentStorage, error) { - err := os.MkdirAll(baseDir, StorageDirPerm) - if err != nil { - return nil, fmt.Errorf("failed to create storage directory: %w", err) - } - - return &ContentStorage{baseDir: baseDir}, nil -} - -// Store writes data with writeFileSynced to the path derived from key, -// replacing any file already there, and returns its size. -func (s *ContentStorage) Store( - key imagerequest.SourceKey, data []byte, -) (int64, error) { - path := s.keyToPath(key) - - err := os.MkdirAll(filepath.Dir(path), StorageDirPerm) - if err != nil { - return 0, fmt.Errorf("failed to create directory: %w", err) - } - - err = writeFileSynced(path, data) - if err != nil { - return 0, err - } - - return int64(len(data)), nil -} - -// LoadWithSize returns a reader and file size for the content with the given key. -func (s *ContentStorage) LoadWithSize( - key imagerequest.SourceKey, -) (io.ReadCloser, int64, error) { - path := s.keyToPath(key) - - f, err := os.Open(path) //nolint:gosec // path derived from source key - if err != nil { - if os.IsNotExist(err) { - return nil, 0, ErrNotFound - } - - return nil, 0, fmt.Errorf("failed to open content: %w", err) - } - - stat, err := f.Stat() - if err != nil { - _ = f.Close() - - return nil, 0, fmt.Errorf("failed to stat content: %w", err) - } - - return f, stat.Size(), nil -} - -// Delete removes content with the given key. -func (s *ContentStorage) Delete(key imagerequest.SourceKey) error { - path := s.keyToPath(key) - - err := os.Remove(path) - if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to delete content: %w", err) - } - - return nil -} - -// Exists checks if content with the given key exists. -func (s *ContentStorage) Exists(key imagerequest.SourceKey) bool { - path := s.keyToPath(key) - _, err := os.Stat(path) - - return err == nil -} - -// writeFileSynced writes data to path through a temp file in the same -// directory: the temp file is synced to disk, renamed to path, and the -// directory synced after, so that neither a concurrent reader nor a -// crash finds a partial file under path. When the directory sync fails, -// the file is removed from path, so that a failed write leaves no file -// there. -func writeFileSynced(path string, data []byte) error { - dir := filepath.Dir(path) - - tmpFile, err := os.CreateTemp(dir, ".tmp-*") - if err != nil { - return fmt.Errorf("failed to create temp file: %w", err) - } - - tmpPath := tmpFile.Name() - - _, err = tmpFile.Write(data) - if err != nil { - _ = tmpFile.Close() - _ = os.Remove(tmpPath) - - return fmt.Errorf("failed to write temp file: %w", err) - } - - err = tmpFile.Sync() - if err != nil { - _ = tmpFile.Close() - _ = os.Remove(tmpPath) - - return fmt.Errorf("failed to sync temp file: %w", err) - } - - err = tmpFile.Close() - if err != nil { - _ = os.Remove(tmpPath) - - return fmt.Errorf("failed to close temp file: %w", err) - } - - 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) - } - - err = syncDirectory(dir) - if err != nil { - _ = os.Remove(path) - - return err - } - - return nil -} - -// syncDirectory syncs the directory dir to disk, so that a file just -// renamed into it is still there after a crash. -func syncDirectory(dir string) error { - d, err := os.Open(filepath.Clean(dir)) - if err != nil { - return fmt.Errorf("failed to open directory: %w", err) - } - - err = d.Sync() - _ = d.Close() - - if err != nil { - return fmt.Errorf("failed to sync directory: %w", err) - } - - return nil -} - -// keyToPath converts a key to a file path: /// -func (s *ContentStorage) keyToPath(key imagerequest.SourceKey) string { - k := string(key) - if len(k) < MinHashLength { - return filepath.Clean(filepath.Join(s.baseDir, k)) - } - - return filepath.Clean(filepath.Join(s.baseDir, k[0:2], k[2:4], k)) -} - -// VariantStorage handles key-based file storage for processed image variants. -// Files are stored at: /// -type VariantStorage struct { - baseDir string -} - -// errNotAnOutputFormat is returned for a variant file whose first bytes are -// those of a format pixa reads but never writes, such as SVG. -var errNotAnOutputFormat = errors.New("variant file is not in an output format") - -// NewVariantStorage creates a new variant storage at the given base directory. -func NewVariantStorage(baseDir string) (*VariantStorage, error) { - err := os.MkdirAll(baseDir, StorageDirPerm) - if err != nil { - return nil, fmt.Errorf("failed to create variant storage directory: %w", err) - } - - return &VariantStorage{baseDir: baseDir}, nil -} - -// Store writes content to storage at the given key with writeFileSynced. -func (s *VariantStorage) Store( - key imagerequest.VariantKey, r io.Reader, -) (int64, error) { - data, err := io.ReadAll(r) - if err != nil { - return 0, fmt.Errorf("failed to read content: %w", err) - } - - path := s.keyToPath(key) - - err = os.MkdirAll(filepath.Dir(path), StorageDirPerm) - if err != nil { - return 0, fmt.Errorf("failed to create directory: %w", err) - } - - err = writeFileSynced(path, data) - if err != nil { - return 0, err - } - - return int64(len(data)), nil -} - -// LoadWithContentType opens the content at the given key and returns a -// reader, the file size and the content type, which is read from the -// file's first bytes. A missing file is ErrNotFound. A file whose first -// bytes are not those of an output format, an empty one included, is -// closed again and the reason returned. -func (s *VariantStorage) LoadWithContentType( - key imagerequest.VariantKey, -) (io.ReadCloser, int64, string, error) { - path := s.keyToPath(key) - - f, err := os.Open(path) //nolint:gosec // path derived from cache key - if err != nil { - if os.IsNotExist(err) { - return nil, 0, "", ErrNotFound - } - - return nil, 0, "", fmt.Errorf("failed to open content: %w", err) - } - - stat, err := f.Stat() - if err != nil { - _ = f.Close() - - return nil, 0, "", fmt.Errorf("failed to stat content: %w", err) - } - - contentType, err := outputContentType(f) - if err != nil { - _ = f.Close() - - return nil, 0, "", err - } - - return f, stat.Size(), contentType, nil -} - -// outputContentType reads the first bytes of content, without moving its -// offset, and returns the content type of the output format they show. -func outputContentType(content io.ReaderAt) (string, error) { - firstBytes := make([]byte, magic.MinMagicBytes) - - _, err := content.ReadAt(firstBytes, 0) - if err != nil { - return "", fmt.Errorf("failed to read the first bytes: %w", err) - } - - detected, err := magic.DetectFormat(firstBytes) - if err != nil { - return "", err - } - - _, isOutputFormat := magic.MIMEToImageFormat(string(detected)) - if !isOutputFormat { - return "", fmt.Errorf("%w: %s", errNotAnOutputFormat, detected) - } - - return string(detected), nil -} - -// Exists checks if content exists at the given key. -func (s *VariantStorage) Exists(key imagerequest.VariantKey) bool { - path := s.keyToPath(key) - _, err := os.Stat(path) - - return err == nil -} - -// Delete removes the content at the given key. A missing file is not an -// error. -func (s *VariantStorage) Delete(key imagerequest.VariantKey) error { - path := s.keyToPath(key) - - err := os.Remove(path) - if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to delete content: %w", err) - } - - return nil -} - -// keyToPath converts a key to a file path: /// -func (s *VariantStorage) keyToPath(key imagerequest.VariantKey) string { - k := string(key) - if len(k) < MinHashLength { - return filepath.Join(s.baseDir, k) - } - - return filepath.Join(s.baseDir, k[0:2], k[2:4], k) -} diff --git a/internal/imgcache/storage_internal_test.go b/internal/imgcache/storage_internal_test.go deleted file mode 100644 index a3e0e78..0000000 --- a/internal/imgcache/storage_internal_test.go +++ /dev/null @@ -1,156 +0,0 @@ -package imgcache - -import ( - "bytes" - "errors" - "io" - "os" - "path/filepath" - "testing" - - "sneak.berlin/go/pixa/internal/imagerequest" -) - -func TestContentStorage_StoreAndLoadWithSize(t *testing.T) { - t.Parallel() - - tmpDir := t.TempDir() - - storage, err := NewContentStorage(tmpDir) - if err != nil { - t.Fatalf("NewContentStorage() error = %v", err) - } - - content := []byte("hello world") - key := imagerequest.Source{Host: testHostCDN, Path: testPathCat}.Key() - - size, err := storage.Store(key, content) - if err != nil { - t.Fatalf("Store() error = %v", err) - } - - if size != int64(len(content)) { - t.Errorf("Store() size = %d, want %d", size, len(content)) - } - - // Verify file exists at expected path - keyStr := string(key) - - expectedPath := filepath.Join(tmpDir, keyStr[0:2], keyStr[2:4], keyStr) - - _, err = os.Stat(expectedPath) - if err != nil { - t.Errorf("File not at expected path %s: %v", expectedPath, err) - } - - // Load and verify content - r, _, err := storage.LoadWithSize(key) - if err != nil { - t.Fatalf("LoadWithSize() error = %v", err) - } - - defer func() { _ = r.Close() }() - - loaded, err := io.ReadAll(r) - if err != nil { - t.Fatalf("ReadAll() error = %v", err) - } - - if !bytes.Equal(loaded, content) { - t.Errorf("LoadWithSize() content = %q, want %q", loaded, content) - } -} - -func TestContentStorage_LoadWithSizeNotFound(t *testing.T) { - t.Parallel() - - tmpDir := t.TempDir() - - storage, err := NewContentStorage(tmpDir) - if err != nil { - t.Fatalf("NewContentStorage() error = %v", err) - } - - _, _, err = storage.LoadWithSize(imagerequest.SourceKey("nonexistent")) - if !errors.Is(err, ErrNotFound) { - t.Errorf("LoadWithSize() error = %v, want ErrNotFound", err) - } -} - -func TestContentStorage_Delete(t *testing.T) { - t.Parallel() - - tmpDir := t.TempDir() - - storage, err := NewContentStorage(tmpDir) - if err != nil { - t.Fatalf("NewContentStorage() error = %v", err) - } - - content := []byte("to be deleted") - key := imagerequest.Source{Host: testHostCDN, Path: testPathCat}.Key() - - _, err = storage.Store(key, content) - if err != nil { - t.Fatalf("Store() error = %v", err) - } - - if !storage.Exists(key) { - t.Error("Exists() = false, want true") - } - - err = storage.Delete(key) - if err != nil { - t.Fatalf("Delete() error = %v", err) - } - - if storage.Exists(key) { - t.Error("Exists() = true after delete, want false") - } -} - -func TestContentStorage_DeleteNonexistent(t *testing.T) { - t.Parallel() - - tmpDir := t.TempDir() - - storage, err := NewContentStorage(tmpDir) - if err != nil { - t.Fatalf("NewContentStorage() error = %v", err) - } - - // Should not error - err = storage.Delete(imagerequest.SourceKey("nonexistent")) - if err != nil { - t.Errorf("Delete() error = %v, want nil", err) - } -} - -func TestContentStorage_KeyToPath(t *testing.T) { - t.Parallel() - - tmpDir := t.TempDir() - - storage, err := NewContentStorage(tmpDir) - if err != nil { - t.Fatalf("NewContentStorage() error = %v", err) - } - - // Test by storing and verifying the resulting path structure - content := []byte("test content for path verification") - key := imagerequest.Source{Host: testHostCDN, Path: testPathCat}.Key() - - _, err = storage.Store(key, content) - if err != nil { - t.Fatalf("Store() error = %v", err) - } - - keyStr := string(key) - - expectedPath := filepath.Join(tmpDir, keyStr[0:2], keyStr[2:4], keyStr) - - _, err = os.Stat(expectedPath) - if err != nil { - t.Errorf("File not at expected path %s: %v", expectedPath, err) - } -} diff --git a/internal/imgcache/testdata/red.avif b/internal/imgcache/testdata/red.avif deleted file mode 100644 index ae4624b..0000000 Binary files a/internal/imgcache/testdata/red.avif and /dev/null differ