diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index ef4cacf..02a8edd 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -193,25 +193,18 @@ func (c *Cache) GetVariant(cacheKey VariantKey) (io.ReadCloser, int64, string, e } contentType, known := c.metaCache.Get(cacheKey) - if known { - reader, size, err := c.variants.LoadWithSize(cacheKey) - if err != nil { - // The file is gone, e.g. deleted outside pixa - c.metaCache.Remove(cacheKey) - - return nil, 0, "", err - } - - return reader, size, contentType, nil + if !known { + return c.loadVariantWithMeta(cacheKey) } - reader, size, contentType, err := c.variants.LoadWithMeta(cacheKey) + reader, size, err := c.variants.LoadWithSize(cacheKey) if err != nil { + // The file is gone, e.g. deleted outside pixa + c.metaCache.Remove(cacheKey) + return nil, 0, "", err } - c.metaCache.Add(cacheKey, contentType) - return reader, size, contentType, nil } @@ -527,6 +520,22 @@ func (c *Cache) IncrementTransformCount(ctx context.Context) { } } +// loadVariantWithMeta is GetVariant for a variant metaCache does not +// hold: it reads the content type from the variant's .meta file and +// keeps it in metaCache. +func (c *Cache) loadVariantWithMeta( + cacheKey VariantKey, +) (io.ReadCloser, int64, string, error) { + reader, size, contentType, err := c.variants.LoadWithMeta(cacheKey) + if err != nil { + return nil, 0, "", err + } + + c.metaCache.Add(cacheKey, contentType) + + return reader, size, contentType, nil +} + // writeMetadataSidecar writes the JSON metadata sidecar of a stored source. // A failure is logged and is otherwise non-fatal; the metadata is in the // database. diff --git a/internal/imgcache/metacache_internal_test.go b/internal/imgcache/metacache_internal_test.go index 0a2d368..c327249 100644 --- a/internal/imgcache/metacache_internal_test.go +++ b/internal/imgcache/metacache_internal_test.go @@ -114,6 +114,37 @@ func TestSecondHitDoesNotReadMetaFile(t *testing.T) { assertVariantServed(t, restarted, testVariantKeyOne, content) } +// TestReadDuringStoreKeepsStoredContentType checks that a GetVariant +// which began before StoreVariant finished cannot replace the content +// type the store kept in memory. Such a read can find the variant file +// but not yet its .meta file, and so gets application/octet-stream. The +// test deletes the .meta file after the store, then runs the part of +// GetVariant that comes after its check of memory. +func TestReadDuringStoreKeepsStoredContentType(t *testing.T) { + t.Parallel() + + cache, _ := newEvictionTestCache(t, 1<<20) + content := []byte("webp variant bytes") + + storeEvictionTestVariant(t, cache, testVariantKeyOne, content) + + err := os.Remove(cache.variants.keyToPath(testVariantKeyOne) + ".meta") + if err != nil { + t.Fatalf("removing .meta file: %v", err) + } + + reader, _, contentType, err := cache.loadVariantWithMeta(testVariantKeyOne) + if err != nil { + t.Fatalf("loadVariantWithMeta(%s) error = %v", testVariantKeyOne, err) + } + + _ = reader.Close() + + t.Logf("the read without a .meta file got content type %q", contentType) + + assertVariantServed(t, cache, testVariantKeyOne, content) +} + // TestEvictedVariantIsNotServed checks that a variant the evictor // removed is a miss and cannot be read, although it had been stored // and served before.