From e0539a860159617f19f6573e797584e733c6060a Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 26 Oct 2026 19:13:04 +0000 Subject: [PATCH] Keep cached sources and variants in one FileStore type (closes #318) imgcache.FileStore is a directory of files named by a hex key, at //. It writes a file under a temp name, syncs it and renames it into place. It opens, reports the size of and deletes the file for a key. The cache keeps its sources in one FileStore and its variants in another, so ContentStorage and VariantStorage are gone. FileStore has no Exists: the reconciliation pass checks a variant's file with Size, and the tests look for the file on disk. Cache.Variant reads the content type from the file's first bytes itself. storage.go is now filestore.go, and the unused testdata/red.avif is gone. Model: opus-5-5 --- TODO.md | 17 + internal/imgcache/cache.go | 66 +++- internal/imgcache/cache_hit_internal_test.go | 2 +- internal/imgcache/eviction.go | 11 +- internal/imgcache/eviction_internal_test.go | 37 +- internal/imgcache/filestore.go | 185 ++++++++++ internal/imgcache/filestore_internal_test.go | 184 ++++++++++ .../partial_cache_files_internal_test.go | 41 ++- .../imgcache/source_content_internal_test.go | 6 +- internal/imgcache/storage.go | 326 ------------------ internal/imgcache/storage_internal_test.go | 156 --------- internal/imgcache/testdata/red.avif | Bin 281 -> 0 bytes 12 files changed, 496 insertions(+), 535 deletions(-) create mode 100644 internal/imgcache/filestore.go create mode 100644 internal/imgcache/filestore_internal_test.go delete mode 100644 internal/imgcache/storage.go delete mode 100644 internal/imgcache/storage_internal_test.go delete mode 100644 internal/imgcache/testdata/red.avif 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 ae4624ba21e6803af856e275967a8231fa1b9a55..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 281 zcmZQzV30{GsVqn=%S>Ycg51nBLl8SRGZDnUmYZ6V2oeVZ#f+4kA_$X#p&&E41jdHZ zdkW|Xd$xjCHTpSn}fe-|KL1_jCc_2?YGcyltDOgk#$Vp`asRF7^EHf|! zF~c$oiVA?VMP@;AK9J@~EHiX&bjSh8iDVWRq=Fbgzyier1(_9@AOU6`9v&bimRXPs zb3f1#%#BRUKvDJFl*AGt1}=^WmfBuMUbcruf)WgT7LQvP7&Zupyfs+zn&%Gyqc|_Q -- 2.54.0