diff --git a/TODO.md b/TODO.md index bb3ec91..3fc0dee 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,37 @@ P2: security: per-origin rate limiting # Completed Steps +- 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. + - `imgcache.Service` is `imageproxy.Proxy`, built by `imageproxy.NewProxy` + from an `imageproxy.Config`. + - `ImageResponse`, `CacheStatus`, `ErrNegativeCached` and + `ErrProcessingTimedOut` moved with it, under their names. + - `imgcache.CacheConfig` is `imgcache.Config`. + - The cache has one read and one write for each kind it keeps. Each takes + the place of an earlier method: + - `Variant`, of `GetVariant` + - `PutVariant`, of `StoreVariant` + - `Source`, of `LookupSource` followed by `GetSourceContent` + - `PutSource`, of `StoreSource` + - `Failure`, of `checkNegativeCache` + - `PutFailure`, of `StoreNegative` + - `Variant` and `Source` each open the file and hand its access time to the + background writer. A cache hit still makes no database call. + - `imgcache.go` and `module.go` are gone. + - `NewProxy` makes the counters of upstream fetches and transforms, and + registers them on the registry it is given. The cache keeps only the hit + and miss counters. + - A cached source whose row cannot be read is logged as "failed to load + cached source, fetching". It was logged as "source lookup failed". + - The proxy's tests moved to `internal/imageproxy`. They reach the cache + through its database, its state directory and its metrics registry. + - The tests of an empty or cut cached file are split in two: + - `internal/imgcache` checks that the reconciliation pass removes the + file and its row, with a warning naming the file. + - `internal/imageproxy` removes the file and its row as the pass does, + and checks that `Get` fetches or converts again. - 2026-10-26 both image routes serve an image with one function, `serveImage`, and answer its errors with one, `respondImageError` (closes #316). `HandleImage` parses the path and verifies the signature, and `HandleImageEnc` diff --git a/internal/handlers/cache_hit_internal_test.go b/internal/handlers/cache_hit_internal_test.go index f62a2b0..4cc5081 100644 --- a/internal/handlers/cache_hit_internal_test.go +++ b/internal/handlers/cache_hit_internal_test.go @@ -23,6 +23,7 @@ import ( "sneak.berlin/go/pixa/internal/database" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/seal" @@ -122,7 +123,7 @@ func newCountingDatabaseServer( tb.Fatalf("encoding the photo: %v", err) } - cache, err := imgcache.NewCache(db, imgcache.CacheConfig{ + cache, err := imgcache.NewCache(db, imgcache.Config{ StateDir: tb.TempDir(), NegativeTTL: 5 * time.Minute, MaxBytes: 1 << 30, @@ -131,7 +132,7 @@ func newCountingDatabaseServer( tb.Fatalf("imgcache.NewCache() error = %v", err) } - svc, err := imgcache.NewService(&imgcache.ServiceConfig{ + svc, err := imageproxy.NewProxy(&imageproxy.Config{ Cache: cache, Fetcher: newMockFetcher(fstest.MapFS{ countingHost + photoPath: &fstest.MapFile{Data: photo.Bytes()}, @@ -142,9 +143,9 @@ func newCountingDatabaseServer( }), MaxResponseSize: 50 << 20, Logger: slog.New(slog.DiscardHandler), - }) + }, prometheus.NewRegistry()) if err != nil { - tb.Fatalf("imgcache.NewService() error = %v", err) + tb.Fatalf("imageproxy.NewProxy() error = %v", err) } keys, err := seal.DeriveKeys(testSigningKey) @@ -228,9 +229,9 @@ func TestImageRoutes_CacheHitsMakeNoDatabaseCall(t *testing.T) { rec := serveImage(t, srv, target) cacheStatus := rec.Header().Get("X-Pixa-Cache") - if cacheStatus != string(imgcache.CacheHit) { + if cacheStatus != string(imageproxy.CacheHit) { t.Errorf("GET %s: X-Pixa-Cache = %q, want %s", - target, cacheStatus, imgcache.CacheHit) + target, cacheStatus, imageproxy.CacheHit) } } @@ -260,9 +261,9 @@ func BenchmarkHandleImage_CacheHits(b *testing.B) { rec := serveImage(b, srv, target) cacheStatus := rec.Header().Get("X-Pixa-Cache") - if cacheStatus != string(imgcache.CacheHit) { + if cacheStatus != string(imageproxy.CacheHit) { b.Errorf("GET %s: X-Pixa-Cache = %q, want %s", - target, cacheStatus, imgcache.CacheHit) + target, cacheStatus, imageproxy.CacheHit) return } diff --git a/internal/handlers/first_bytes_internal_test.go b/internal/handlers/first_bytes_internal_test.go index 3a25b79..3f939d4 100644 --- a/internal/handlers/first_bytes_internal_test.go +++ b/internal/handlers/first_bytes_internal_test.go @@ -17,6 +17,7 @@ import ( "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/seal" @@ -38,7 +39,7 @@ func newRoutesFetchingThrough( ) (*Handlers, http.Handler) { t.Helper() - cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{ + cache, err := imgcache.NewCache(setupTestDB(t), imgcache.Config{ StateDir: t.TempDir(), NegativeTTL: 5 * time.Minute, MaxBytes: 1 << 30, @@ -47,7 +48,7 @@ func newRoutesFetchingThrough( t.Fatalf("imgcache.NewCache() error = %v", err) } - svc, err := imgcache.NewService(&imgcache.ServiceConfig{ + svc, err := imageproxy.NewProxy(&imageproxy.Config{ Cache: cache, Fetcher: fetcher, Processor: imageprocessor.New(imageprocessor.Params{ @@ -56,9 +57,9 @@ func newRoutesFetchingThrough( }), MaxResponseSize: 50 << 20, Logger: log, - }) + }, prometheus.NewRegistry()) if err != nil { - t.Fatalf("imgcache.NewService() error = %v", err) + t.Fatalf("imageproxy.NewProxy() error = %v", err) } keys, err := seal.DeriveKeys(testSigningKey) diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 50c4f1e..0f4e134 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -13,7 +13,7 @@ import ( "sneak.berlin/go/pixa/internal/config" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/healthcheck" - "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/logger" "sneak.berlin/go/pixa/internal/seal" "sneak.berlin/go/pixa/internal/session" @@ -29,7 +29,7 @@ type Params struct { Config *config.Config Keys seal.Keys - ImageService *imgcache.Service + ImageService *imageproxy.Proxy SessionManager *session.Manager EncryptedURLGenerator *encurl.Generator } @@ -39,7 +39,7 @@ type Handlers struct { log *slog.Logger hc *healthcheck.Healthcheck config *config.Config - imgSvc *imgcache.Service + imgSvc *imageproxy.Proxy sessMgr *session.Manager encGen *encurl.Generator csrfProtect func(http.Handler) http.Handler diff --git a/internal/handlers/handlers_internal_test.go b/internal/handlers/handlers_internal_test.go index 9568849..b516468 100644 --- a/internal/handlers/handlers_internal_test.go +++ b/internal/handlers/handlers_internal_test.go @@ -23,6 +23,7 @@ import ( "sneak.berlin/go/pixa/internal/database" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/signature" @@ -49,7 +50,7 @@ func setupTestHandler(t *testing.T) *testFixtures { tmpDir := t.TempDir() db := setupTestDB(t) - cache, err := imgcache.NewCache(db, imgcache.CacheConfig{ + cache, err := imgcache.NewCache(db, imgcache.Config{ StateDir: tmpDir, NegativeTTL: 5 * time.Minute, MaxBytes: 1 << 30, @@ -58,7 +59,7 @@ func setupTestHandler(t *testing.T) *testFixtures { t.Fatalf("failed to create cache: %v", err) } - svc, err := imgcache.NewService(&imgcache.ServiceConfig{ + svc, err := imageproxy.NewProxy(&imageproxy.Config{ Cache: cache, Fetcher: newMockFetcher(mockFS), Processor: imageprocessor.New(imageprocessor.Params{ @@ -66,9 +67,9 @@ func setupTestHandler(t *testing.T) *testFixtures { MaxConcurrentProcessing: 2, }), MaxResponseSize: 50 << 20, - }) + }, prometheus.NewRegistry()) if err != nil { - t.Fatalf("failed to create service: %v", err) + t.Fatalf("failed to create proxy: %v", err) } h := &Handlers{ diff --git a/internal/handlers/image.go b/internal/handlers/image.go index d349d5b..68c2fee 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -14,8 +14,8 @@ import ( "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imagerequest" - "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/magic" ) @@ -253,7 +253,7 @@ func (s *Handlers) respondImageError( status, message = http.StatusGatewayTimeout, "upstream timeout" case errors.Is(err, httpfetcher.ErrTooManyConnections), errors.Is(err, imageprocessor.ErrTooManyImages), - errors.Is(err, imgcache.ErrProcessingTimedOut): + errors.Is(err, imageproxy.ErrProcessingTimedOut): status = http.StatusServiceUnavailable message = "server busy, try again later" case errors.Is(err, httpfetcher.ErrInvalidContentType), diff --git a/internal/handlers/image_cache_control_internal_test.go b/internal/handlers/image_cache_control_internal_test.go index a561c8d..4727fd7 100644 --- a/internal/handlers/image_cache_control_internal_test.go +++ b/internal/handlers/image_cache_control_internal_test.go @@ -17,6 +17,7 @@ import ( "sneak.berlin/go/pixa/internal/allowlist" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/seal" @@ -35,7 +36,7 @@ func newSignedHostServer( ) (*Handlers, http.Handler) { t.Helper() - cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{ + cache, err := imgcache.NewCache(setupTestDB(t), imgcache.Config{ StateDir: t.TempDir(), NegativeTTL: 5 * time.Minute, MaxBytes: 1 << 30, @@ -46,7 +47,7 @@ func newSignedHostServer( jpegData := generateTestJPEG(t, 100, 100, color.RGBA{255, 0, 0, 255}) - svc, err := imgcache.NewService(&imgcache.ServiceConfig{ + svc, err := imageproxy.NewProxy(&imageproxy.Config{ Cache: cache, Fetcher: newMockFetcher(fstest.MapFS{ signedHost + photoPath: &fstest.MapFile{Data: jpegData}, @@ -57,9 +58,9 @@ func newSignedHostServer( }), MaxResponseSize: 50 << 20, Logger: log, - }) + }, prometheus.NewRegistry()) if err != nil { - t.Fatalf("imgcache.NewService() error = %v", err) + t.Fatalf("imageproxy.NewProxy() error = %v", err) } keys, err := seal.DeriveKeys(testSigningKey) diff --git a/internal/handlers/image_errors_internal_test.go b/internal/handlers/image_errors_internal_test.go index a7952cb..5f7afa3 100644 --- a/internal/handlers/image_errors_internal_test.go +++ b/internal/handlers/image_errors_internal_test.go @@ -22,6 +22,7 @@ import ( "sneak.berlin/go/pixa/internal/allowlist" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/signature" @@ -36,7 +37,7 @@ const allowlistedHost = "allowed.example.com" func newImageRoute(t *testing.T, fetcher httpfetcher.Fetcher) http.Handler { t.Helper() - cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{ + cache, err := imgcache.NewCache(setupTestDB(t), imgcache.Config{ StateDir: t.TempDir(), NegativeTTL: 5 * time.Minute, MaxBytes: 1 << 30, @@ -45,7 +46,7 @@ func newImageRoute(t *testing.T, fetcher httpfetcher.Fetcher) http.Handler { t.Fatalf("failed to create cache: %v", err) } - svc, err := imgcache.NewService(&imgcache.ServiceConfig{ + svc, err := imageproxy.NewProxy(&imageproxy.Config{ Cache: cache, Fetcher: fetcher, Processor: imageprocessor.New(imageprocessor.Params{ @@ -53,9 +54,9 @@ func newImageRoute(t *testing.T, fetcher httpfetcher.Fetcher) http.Handler { MaxConcurrentProcessing: 2, }), MaxResponseSize: 50 << 20, - }) + }, prometheus.NewRegistry()) if err != nil { - t.Fatalf("failed to create service: %v", err) + t.Fatalf("failed to create proxy: %v", err) } h := &Handlers{ diff --git a/internal/handlers/referer_blocklist_internal_test.go b/internal/handlers/referer_blocklist_internal_test.go index 51fa8ec..68eec02 100644 --- a/internal/handlers/referer_blocklist_internal_test.go +++ b/internal/handlers/referer_blocklist_internal_test.go @@ -15,6 +15,7 @@ import ( "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imagerequest" "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/seal" @@ -52,7 +53,7 @@ func newRefererRoutes(t *testing.T) (http.Handler, *Handlers, *countingFetcher) Fetcher: newPhotoFetcher(t, allowlistedHost, signedHost), } - cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{ + cache, err := imgcache.NewCache(setupTestDB(t), imgcache.Config{ StateDir: t.TempDir(), NegativeTTL: 5 * time.Minute, MaxBytes: 1 << 30, @@ -61,7 +62,7 @@ func newRefererRoutes(t *testing.T) (http.Handler, *Handlers, *countingFetcher) t.Fatalf("imgcache.NewCache() error = %v", err) } - svc, err := imgcache.NewService(&imgcache.ServiceConfig{ + svc, err := imageproxy.NewProxy(&imageproxy.Config{ Cache: cache, Fetcher: fetcher, Processor: imageprocessor.New(imageprocessor.Params{ @@ -69,9 +70,9 @@ func newRefererRoutes(t *testing.T) (http.Handler, *Handlers, *countingFetcher) MaxConcurrentProcessing: 2, }), MaxResponseSize: 50 << 20, - }) + }, prometheus.NewRegistry()) if err != nil { - t.Fatalf("imgcache.NewService() error = %v", err) + t.Fatalf("imageproxy.NewProxy() error = %v", err) } keys, err := seal.DeriveKeys(testSigningKey) diff --git a/internal/handlers/server_busy_internal_test.go b/internal/handlers/server_busy_internal_test.go index 6743959..fa5297c 100644 --- a/internal/handlers/server_busy_internal_test.go +++ b/internal/handlers/server_busy_internal_test.go @@ -11,8 +11,8 @@ import ( "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imagerequest" - "sneak.berlin/go/pixa/internal/imgcache" ) // TestServerBusyAnswers503 checks that respondImageError, which answers the @@ -58,7 +58,7 @@ func TestProcessingTimedOutAnswers503(t *testing.T) { h := &Handlers{log: slog.New(slog.DiscardHandler)} err := fmt.Errorf("%w: failed to encode: %w", - imgcache.ErrProcessingTimedOut, context.DeadlineExceeded) + imageproxy.ErrProcessingTimedOut, context.DeadlineExceeded) rec := httptest.NewRecorder() h.respondImageError(rec, &imagerequest.Request{}, err) diff --git a/internal/imageproxy/cache_hit_internal_test.go b/internal/imageproxy/cache_hit_internal_test.go new file mode 100644 index 0000000..470ef8f --- /dev/null +++ b/internal/imageproxy/cache_hit_internal_test.go @@ -0,0 +1,100 @@ +package imageproxy + +import ( + "bytes" + "os" + "testing" + "time" + + "sneak.berlin/go/pixa/internal/imagerequest" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// TestProxy_Get_MakesAgainVariantOfNoOutputFormat replaces a cached +// variant's file with text, and checks that the next request is a miss +// answered with the variant made again, and the one after a hit. +func TestProxy_Get_MakesAgainVariantOfNoOutputFormat(t *testing.T) { + t.Parallel() + + svc, fixtures := setupTestProxy(t) + req := photoVariant(fixtures, 85, imagerequest.FitCover) + + want, _ := getImage(t, svc, req) + + path := variantFilePath(svc, imagerequest.CacheKey(req)) + + err := os.WriteFile(path, []byte("this is text, not an image"), + imgcache.StorageFilePerm) + if err != nil { + t.Fatalf("replacing the variant file: %v", err) + } + + got, status := getImage(t, svc, req) + t.Logf("after the replacement: %d bytes, %s", len(got), status) + + if status != CacheMiss || !bytes.Equal(got, want) { + t.Errorf("after the replacement: Get() = %d bytes, %s; want the %d bytes "+ + "made again, %s", len(got), status, len(want), CacheMiss) + } + + got, status = getImage(t, svc, req) + + if status != CacheHit || !bytes.Equal(got, want) { + t.Errorf("next request: Get() = %d bytes, %s; want the %d bytes, %s", + len(got), status, len(want), CacheHit) + } +} + +// TestProxy_Get_HitWaitsForNoDatabaseWrite holds the database's one +// connection while a cached variant is requested. The hit must be answered +// while the connection is still held, and its access time must reach the +// database once it is free. +func TestProxy_Get_HitWaitsForNoDatabaseWrite(t *testing.T) { + t.Parallel() + + svc, fixtures := setupTestProxy(t) + req := photoVariant(fixtures, 85, imagerequest.FitCover) + key := imagerequest.CacheKey(req) + + getImage(t, svc, req) + + // Each connection to an in-memory SQLite database opens a new, empty one, + // so the goroutine that writes the access times must share the + // connection that holds the schema. + svc.db.SetMaxOpenConns(1) + svc.cache.StartPendingAccessTimeWrites() + + defer func() { _ = svc.cache.StopPendingAccessTimeWrites(t.Context()) }() + + anHourAgo := time.Now().Add(-time.Hour) + setVariantLastAccessed(t, svc.db, key, anHourAgo) + + releaseDatabase := holdDatabase(t, svc.db) + defer releaseDatabase() + + results := startGet(t.Context(), svc, req) + + select { + case got := <-results: + if got.err != nil { + t.Fatalf("Get() of the cached variant error = %v", got.err) + } + case <-time.After(pendingAccessTimesWait): + t.Fatal("the cache hit waited for the database") + } + + releaseDatabase() + + deadline := time.Now().Add(pendingAccessTimesWait) + + for countRows(t, svc.db, ` + SELECT COUNT(*) FROM variant_content + WHERE cache_key = ? AND last_accessed_at > ? + `, string(key), anHourAgo.UTC().Format(sqliteTimestampFormat)) == 0 { + if time.Now().After(deadline) { + t.Fatal("the hit's access time was not written") + } + + time.Sleep(10 * time.Millisecond) + } +} diff --git a/internal/imgcache/concurrent_misses_internal_test.go b/internal/imageproxy/concurrent_misses_internal_test.go similarity index 85% rename from internal/imgcache/concurrent_misses_internal_test.go rename to internal/imageproxy/concurrent_misses_internal_test.go index 594e4fb..4c7409f 100644 --- a/internal/imgcache/concurrent_misses_internal_test.go +++ b/internal/imageproxy/concurrent_misses_internal_test.go @@ -1,4 +1,4 @@ -package imgcache +package imageproxy import ( "bytes" @@ -56,15 +56,15 @@ func (f *heldFetcher) releaseFetches() { f.releaseOnce.Do(func() { close(f.release) }) } -// setupHeldFetchService returns a test service whose fetches go through a +// setupHeldFetchProxy returns a test proxy whose fetches go through a // heldFetcher. Its database is limited to one connection: each connection to // an in-memory SQLite database opens a new, empty one, so requests running at // once must share the connection that holds the schema. -func setupHeldFetchService(t *testing.T) (*Service, *TestFixtures, *heldFetcher) { +func setupHeldFetchProxy(t *testing.T) (*testProxy, *TestFixtures, *heldFetcher) { t.Helper() - svc, fixtures := SetupTestService(t) - svc.cache.db.SetMaxOpenConns(1) + svc, fixtures := setupTestProxy(t) + svc.db.SetMaxOpenConns(1) fetcher := &heldFetcher{ upstream: svc.fetcher, @@ -107,7 +107,7 @@ type getResult struct { // startGet calls Get in a goroutine of its own and delivers what it returned // on the channel. func startGet( - ctx context.Context, svc *Service, req *imagerequest.Request, + ctx context.Context, svc *testProxy, req *imagerequest.Request, ) <-chan getResult { results := make(chan getResult, 1) @@ -139,14 +139,14 @@ func jpegSize(data []byte) (int, int) { return config.Width, config.Height } -// TestService_Get_ConcurrentMissesShareOneFetch starts several requests for +// TestProxy_Get_ConcurrentMissesShareOneFetch starts several requests for // one uncached variant while the first one's fetch is held. Between them they // must fetch the source once and transcode it once, every one must be answered // with the same 50x25 JPEG, and each must count one miss. -func TestService_Get_ConcurrentMissesShareOneFetch(t *testing.T) { +func TestProxy_Get_ConcurrentMissesShareOneFetch(t *testing.T) { t.Parallel() - svc, fixtures, fetcher := setupHeldFetchService(t) + svc, fixtures, fetcher := setupHeldFetchProxy(t) const requests = 8 @@ -183,7 +183,7 @@ func TestService_Get_ConcurrentMissesShareOneFetch(t *testing.T) { t.Errorf("%d requests made %d upstream fetches, want 1", requests, fetches) } - // NewTestFS builds the same files the test service's fetcher serves. + // NewTestFS builds the same files the test proxy's fetcher serves. testFS, _ := NewTestFS(t) photo, err := fs.ReadFile(testFS, fixtures.GoodHostJPEG) @@ -193,18 +193,18 @@ func TestService_Get_ConcurrentMissesShareOneFetch(t *testing.T) { want := counterValues{0, requests, 1, int64(len(photo)), 1} - if got := readCounters(svc.cache); got != want { + if got := readCounters(t, svc); got != want { t.Errorf("counters = %+v, want %+v", got, want) } } -// TestService_Get_ConcurrentVariantsStayApart requests three variants of the +// TestProxy_Get_ConcurrentVariantsStayApart requests three variants of the // test photo at once that differ only in quality or fit. Each must be made by // a fetch and a transcode of its own, and each answer must be its own variant. -func TestService_Get_ConcurrentVariantsStayApart(t *testing.T) { +func TestProxy_Get_ConcurrentVariantsStayApart(t *testing.T) { t.Parallel() - svc, fixtures, fetcher := setupHeldFetchService(t) + svc, fixtures, fetcher := setupHeldFetchProxy(t) cover := startGet(t.Context(), svc, photoVariant(fixtures, 85, imagerequest.FitCover)) lowQuality := startGet(t.Context(), svc, @@ -253,14 +253,14 @@ func TestService_Get_ConcurrentVariantsStayApart(t *testing.T) { } } -// TestService_Get_WaiterStopsWhenItsContextEnds has a second request for a +// TestProxy_Get_WaiterStopsWhenItsContextEnds has a second request for a // variant join the first one's held fetch, then ends the second request's // context. The second request must return at once with the context's error, // while the fetch is still held, and the first must still be answered. -func TestService_Get_WaiterStopsWhenItsContextEnds(t *testing.T) { +func TestProxy_Get_WaiterStopsWhenItsContextEnds(t *testing.T) { t.Parallel() - svc, fixtures, fetcher := setupHeldFetchService(t) + svc, fixtures, fetcher := setupHeldFetchProxy(t) first := startGet(t.Context(), svc, photoVariant(fixtures, 85, imagerequest.FitCover)) @@ -298,15 +298,15 @@ func TestService_Get_WaiterStopsWhenItsContextEnds(t *testing.T) { } } -// TestService_Get_FirstRequestLeavingKeepsTheWork ends the context of the +// TestProxy_Get_FirstRequestLeavingKeepsTheWork ends the context of the // request whose fetch is held, after a second request has joined it. The fetch // and transcode must go on and answer the second request. The first request // waits for its own work, as every request did before misses were shared, and // is answered too. -func TestService_Get_FirstRequestLeavingKeepsTheWork(t *testing.T) { +func TestProxy_Get_FirstRequestLeavingKeepsTheWork(t *testing.T) { t.Parallel() - svc, fixtures, fetcher := setupHeldFetchService(t) + svc, fixtures, fetcher := setupHeldFetchProxy(t) firstCtx, cancelFirst := context.WithCancel(t.Context()) first := startGet(firstCtx, svc, photoVariant(fixtures, 85, imagerequest.FitCover)) @@ -337,11 +337,11 @@ func TestService_Get_FirstRequestLeavingKeepsTheWork(t *testing.T) { } } -// TestService_Get_ConcurrentMissesShareAFailure has several requests for an +// TestProxy_Get_ConcurrentMissesShareAFailure has several requests for an // image that cannot be served arrive while its fetch is held: the one fetch // answers all of them with its error. The request after them is answered from // the negative cache, where the failure is kept. -func TestService_Get_ConcurrentMissesShareAFailure(t *testing.T) { +func TestProxy_Get_ConcurrentMissesShareAFailure(t *testing.T) { t.Parallel() tests := []struct { @@ -362,7 +362,7 @@ func TestService_Get_ConcurrentMissesShareAFailure(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - svc, fixtures, fetcher := setupHeldFetchService(t) + svc, fixtures, fetcher := setupHeldFetchProxy(t) request := func() *imagerequest.Request { req := photoVariant(fixtures, 85, imagerequest.FitCover) @@ -398,12 +398,12 @@ func TestService_Get_ConcurrentMissesShareAFailure(t *testing.T) { } } -// TestService_Get_EndedRequestFetchesNothing checks that a request whose +// TestProxy_Get_EndedRequestFetchesNothing checks that a request whose // context has already ended when it misses the cache starts no fetch. -func TestService_Get_EndedRequestFetchesNothing(t *testing.T) { +func TestProxy_Get_EndedRequestFetchesNothing(t *testing.T) { t.Parallel() - svc, fixtures, fetcher := setupHeldFetchService(t) + svc, fixtures, fetcher := setupHeldFetchProxy(t) ctx, cancel := context.WithCancel(t.Context()) cancel() @@ -418,14 +418,14 @@ func TestService_Get_EndedRequestFetchesNothing(t *testing.T) { } } -// TestService_Get_ReturnsByItsDeadline gives a request a deadline and holds +// TestProxy_Get_ReturnsByItsDeadline gives a request a deadline and holds // its fetch until the fetch's context ends, as the fetcher does while it waits // for a free connection to the host. The request must return by its deadline // with the deadline's error. -func TestService_Get_ReturnsByItsDeadline(t *testing.T) { +func TestProxy_Get_ReturnsByItsDeadline(t *testing.T) { t.Parallel() - svc, fixtures, _ := setupHeldFetchService(t) + svc, fixtures, _ := setupHeldFetchProxy(t) const timeout = 200 * time.Millisecond @@ -444,16 +444,16 @@ func TestService_Get_ReturnsByItsDeadline(t *testing.T) { } } -// TestService_Get_WaiterGetsProcessingTimedOut has a request with no deadline +// TestProxy_Get_WaiterGetsProcessingTimedOut has a request with no deadline // join the held fetch of a first request with a short one. With the only // processing slot held, the first request's transcode waits for it until the // first request's deadline ends, and stops then. The waiting request, whose // own context has not ended, must get ErrProcessingTimedOut, which the image // routes answer 503 as when pixa is busy. -func TestService_Get_WaiterGetsProcessingTimedOut(t *testing.T) { +func TestProxy_Get_WaiterGetsProcessingTimedOut(t *testing.T) { t.Parallel() - svc, fixtures, fetcher := setupHeldFetchService(t) + svc, fixtures, fetcher := setupHeldFetchProxy(t) svc.processor = imageprocessor.New( imageprocessor.Params{MaxInputBytes: 50 << 20, MaxConcurrentProcessing: 1}, ) @@ -494,13 +494,13 @@ func (panickingFetcher) Fetch( panic("upstream fetcher panicked") } -// TestService_Get_PanicBecomesAnError checks that a panic while a variant is +// TestProxy_Get_PanicBecomesAnError checks that a panic while a variant is // being made reaches its request as an error naming the panic, instead of // being raised again. -func TestService_Get_PanicBecomesAnError(t *testing.T) { +func TestProxy_Get_PanicBecomesAnError(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) svc.fetcher = panickingFetcher{} var err error @@ -520,13 +520,13 @@ func TestService_Get_PanicBecomesAnError(t *testing.T) { } } -// TestService_Get_PanicIsReportedToSentry checks that a panic while a variant +// TestProxy_Get_PanicIsReportedToSentry checks that a panic while a variant // is being made is reported through the Sentry hub on the request's context, // where the Sentry middleware puts one when sentry_dsn is set. -func TestService_Get_PanicIsReportedToSentry(t *testing.T) { +func TestProxy_Get_PanicIsReportedToSentry(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) svc.fetcher = panickingFetcher{} transport := &sentry.MockTransport{} diff --git a/internal/imageproxy/counters_internal_test.go b/internal/imageproxy/counters_internal_test.go new file mode 100644 index 0000000..b89e243 --- /dev/null +++ b/internal/imageproxy/counters_internal_test.go @@ -0,0 +1,78 @@ +package imageproxy + +import ( + "errors" + "testing" + + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/testutil" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// TestNewProxy_RegistersItsCountersOnItsRegistry checks that NewProxy +// registers the proxy's counters on the registry it is given, under the +// names README.md gives them. The cache registers its own counters on +// another registry. +func TestNewProxy_RegistersItsCountersOnItsRegistry(t *testing.T) { + t.Parallel() + + cache, err := imgcache.NewCache(setupTestDB(t), + imgcache.Config{StateDir: t.TempDir(), MaxBytes: 1 << 30}, + prometheus.NewRegistry()) + if err != nil { + t.Fatalf("imgcache.NewCache() error = %v", err) + } + + registry := prometheus.NewRegistry() + + _, err = NewProxy(&Config{Cache: cache}, registry) + if err != nil { + t.Fatalf("NewProxy() error = %v", err) + } + + names := []string{ + "pixa_upstream_fetches_total", + "pixa_upstream_fetch_bytes_total", + "pixa_transforms_total", + } + + count, err := testutil.GatherAndCount(registry, names...) + if err != nil { + t.Fatalf("testutil.GatherAndCount() error = %v", err) + } + + if count != len(names) { + t.Errorf("%d of the counters %v are on the registry, want %d", + count, names, len(names)) + } +} + +// TestNewProxy_FailsWhenItsCountersCannotBeRegistered makes a second proxy +// on the registry of a first. Its counters have the same names, so the +// registry refuses them, and NewProxy must return that error. +func TestNewProxy_FailsWhenItsCountersCannotBeRegistered(t *testing.T) { + t.Parallel() + + cache, err := imgcache.NewCache(setupTestDB(t), + imgcache.Config{StateDir: t.TempDir(), MaxBytes: 1 << 30}, + prometheus.NewRegistry()) + if err != nil { + t.Fatalf("imgcache.NewCache() error = %v", err) + } + + registry := prometheus.NewRegistry() + + _, err = NewProxy(&Config{Cache: cache}, registry) + if err != nil { + t.Fatalf("NewProxy() of the first proxy error = %v", err) + } + + _, err = NewProxy(&Config{Cache: cache}, registry) + t.Logf("NewProxy() of the second proxy error = %v", err) + + var alreadyRegistered prometheus.AlreadyRegisteredError + if !errors.As(err, &alreadyRegistered) { + t.Errorf("NewProxy() of the second proxy error = %v, want %T", + err, alreadyRegistered) + } +} diff --git a/internal/imgcache/divzero_internal_test.go b/internal/imageproxy/divzero_internal_test.go similarity index 98% rename from internal/imgcache/divzero_internal_test.go rename to internal/imageproxy/divzero_internal_test.go index b4117be..66a643c 100644 --- a/internal/imgcache/divzero_internal_test.go +++ b/internal/imageproxy/divzero_internal_test.go @@ -1,4 +1,4 @@ -package imgcache +package imageproxy import ( "fmt" diff --git a/internal/imgcache/service.go b/internal/imageproxy/imageproxy.go similarity index 68% rename from internal/imgcache/service.go rename to internal/imageproxy/imageproxy.go index 35d762a..22a3041 100644 --- a/internal/imgcache/service.go +++ b/internal/imageproxy/imageproxy.go @@ -1,4 +1,7 @@ -package imgcache +// Package imageproxy fetches, transforms, caches and serves images. Its +// Proxy serves an image from the cache when it can, and otherwise makes it +// from the cached or fetched source image. +package imageproxy import ( "bytes" @@ -13,16 +16,18 @@ import ( "github.com/dustin/go-humanize" "github.com/getsentry/sentry-go" "github.com/go-chi/chi/v5/middleware" + "github.com/prometheus/client_golang/prometheus" "golang.org/x/sync/singleflight" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imagerequest" + "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/magic" ) -// Service orchestrates the cache, fetcher, and processor. -type Service struct { - cache *Cache +// Proxy orchestrates the cache, fetcher, and processor. +type Proxy struct { + cache *imgcache.Cache fetcher httpfetcher.Fetcher processor *imageprocessor.ImageProcessor log *slog.Logger @@ -30,12 +35,18 @@ type Service struct { // variantsInProgress lets the requests that miss the same variant at the // same time share one fetch and one transcode. variantsInProgress singleflight.Group + + // The counts of upstream fetches and transforms, made and registered by + // NewProxy. They are kept in memory, so adding to one waits for nothing. + upstreamFetches prometheus.Counter + upstreamFetchBytes prometheus.Counter + transforms prometheus.Counter } -// ServiceConfig holds configuration for the image service. -type ServiceConfig struct { +// Config holds the proxy's configuration. +type Config struct { // Cache is the cache instance - Cache *Cache + Cache *imgcache.Cache // Fetcher fetches source images from upstream hosts Fetcher httpfetcher.Fetcher // Processor converts and resizes the images @@ -47,11 +58,16 @@ type ServiceConfig struct { Logger *slog.Logger } -// errCacheRequired is returned when the service is built without a cache. +// errCacheRequired is returned when the proxy is built without a cache. var errCacheRequired = errors.New("cache is required") -// NewService creates a new image service. -func NewService(cfg *ServiceConfig) (*Service, error) { +// NewProxy creates a new image proxy and registers its counters on +// metricsRegistry. pixad passes Prometheus' default registry, the one +// /metrics serves. It fails when metricsRegistry already holds counters of +// the same names. +func NewProxy( + cfg *Config, metricsRegistry prometheus.Registerer, +) (*Proxy, error) { if cfg.Cache == nil { return nil, errCacheRequired } @@ -68,13 +84,36 @@ func NewService(cfg *ServiceConfig) (*Service, error) { return nil, err } - return &Service{ + p := &Proxy{ cache: cfg.Cache, fetcher: cfg.Fetcher, processor: cfg.Processor, log: log, maxResponseSize: cfg.MaxResponseSize, - }, nil + upstreamFetches: prometheus.NewCounter(prometheus.CounterOpts{ + Name: "pixa_upstream_fetches_total", + Help: "Fetches of a source image from upstream that read any bytes.", + }), + upstreamFetchBytes: prometheus.NewCounter(prometheus.CounterOpts{ + Name: "pixa_upstream_fetch_bytes_total", + Help: "Bytes read from upstream by fetches of source images.", + }), + transforms: prometheus.NewCounter(prometheus.CounterOpts{ + Name: "pixa_transforms_total", + Help: "Images the image processor transformed.", + }), + } + + for _, counter := range []prometheus.Counter{ + p.upstreamFetches, p.upstreamFetchBytes, p.transforms, + } { + err = metricsRegistry.Register(counter) + if err != nil { + return nil, fmt.Errorf("failed to register the proxy's counters: %w", err) + } + } + + return p, nil } // ErrNegativeCached is returned when a URL is in the negative cache (recently failed). @@ -86,25 +125,49 @@ var ErrNegativeCached = errors.New("request is in negative cache (recently faile var ErrProcessingTimedOut = errors.New( "image processing stopped at the deadline of the request doing it") +// ImageResponse represents a processed image ready to serve +type ImageResponse struct { + // Content is the image data reader + Content io.ReadCloser + // ContentLength is the size in bytes (-1 if unknown) + ContentLength int64 + // ContentType is the MIME type of the response + ContentType string + // ETag is the entity tag for caching + ETag string + // CacheStatus indicates HIT or MISS + CacheStatus CacheStatus + // FetchedBytes is the number of bytes fetched from upstream (0 if cache hit) + FetchedBytes int64 +} + +// CacheStatus indicates whether the response was served from cache. +type CacheStatus string + +// Cache status values for response headers. +const ( + CacheHit CacheStatus = "HIT" + CacheMiss CacheStatus = "MISS" +) + // Get retrieves a processed image, fetching and processing if necessary. // While its source URL is in the negative cache, a cached variant is still // served and a variant not in the cache is still made from a cached source; // only a request that needs the source fetched is refused. // -// A cache hit opens the variant file and makes no database call: its access -// time is handed to the goroutine that StartPendingAccessTimeWrites starts, -// without waiting for it, and its hit is counted in memory. A variant file -// that cannot be served, such as one whose first bytes are not those of an -// output format, is a miss, and the variant is made again. -func (s *Service) Get( +// A cache hit opens the variant file and makes no database call: the cache +// hands its access time to the goroutine that writes access times, without +// waiting for it, and its hit is counted in memory. A variant file that +// cannot be served, such as one whose first bytes are not those of an output +// format, is a miss, and the variant is made again. +func (p *Proxy) Get( ctx context.Context, req *imagerequest.Request, ) (*ImageResponse, error) { cacheKey := imagerequest.CacheKey(req) - reader, size, contentType, err := s.cache.GetVariant(cacheKey) + reader, size, contentType, err := p.cache.Variant(cacheKey) if err == nil { - s.cache.touchVariant(cacheKey) - s.cache.IncrementStats(true, 0) + p.cache.IncrementStats(true) return &ImageResponse{ Content: reader, @@ -115,17 +178,17 @@ func (s *Service) Get( }, nil } - if !errors.Is(err, ErrNotFound) { - s.log.Warn("cached variant not served, making it again", + if !errors.Is(err, imgcache.ErrNotFound) { + p.log.Warn("cached variant not served, making it again", "cache_key", cacheKey, "error", err) } // Cache miss - get the variant, processed once for all the requests that // miss it at the same time, then count this request's miss, also when it // failed or the request context has ended meanwhile - response, err := s.processOrWait(ctx, req) + response, err := p.processOrWait(ctx, req) - s.cache.IncrementStats(false, 0) + p.cache.IncrementStats(false) if err != nil { return nil, err @@ -138,8 +201,8 @@ func (s *Service) Get( // WaitForProcessing waits until no image is being processed, or until ctx // ends, and returns how many images were still being processed then. -func (s *Service) WaitForProcessing(ctx context.Context) int { - return s.processor.WaitForProcessing(ctx) +func (p *Proxy) WaitForProcessing(ctx context.Context) int { + return p.processor.WaitForProcessing(ctx) } // errPanicked is returned when processing a variant panicked. @@ -151,7 +214,7 @@ var errPanicked = errors.New("panic while processing image") // and take no upstream connection or processing slot. The processing ignores // the first request's cancellation, so the others are still served if that // client goes away, but keeps its deadline. -func (s *Service) processOrWait( +func (p *Proxy) processOrWait( ctx context.Context, req *imagerequest.Request, ) (*ImageResponse, error) { // A request that has already ended starts no processing @@ -165,7 +228,7 @@ func (s *Service) processOrWait( // only when no other request is processing the variant processing := make(chan struct{}) - results := s.variantsInProgress.DoChan(string(cacheKey), + results := p.variantsInProgress.DoChan(string(cacheKey), func() (_ any, err error) { close(processing) @@ -176,7 +239,7 @@ func (s *Service) processOrWait( defer func() { recovered := recover() if recovered != nil { - s.log.Error("panic while processing image", + p.log.Error("panic while processing image", "host", req.Source.Host, "path", req.Source.Path, "panic", recovered, "stack", string(debug.Stack())) @@ -191,7 +254,7 @@ func (s *Service) processOrWait( processingCtx, cancel := withoutCancelKeepingDeadline(ctx) defer cancel() - return s.processFromSourceOrFetch(processingCtx, req, cacheKey) + return p.processFromSourceOrFetch(processingCtx, req, cacheKey) }) var result singleflight.Result @@ -240,25 +303,31 @@ func withoutCancelKeepingDeadline( return context.WithDeadline(context.WithoutCancel(ctx), deadline) } -// loadCachedSource opens source content from cache, without reading it, and -// returns it with its size; nil if the cached data is unavailable, empty or -// exceeds maxResponseSize. -func (s *Service) loadCachedSource( - sourceKey imagerequest.SourceKey, +// loadCachedSource opens src's cached source, without reading it, and +// returns it with its size; nil if it is not in the cache, cannot be read, +// is empty or exceeds maxResponseSize. +func (p *Proxy) loadCachedSource( + ctx context.Context, src imagerequest.Source, ) (io.ReadCloser, int64) { - reader, size, err := s.cache.GetSourceContent(sourceKey) + reader, size, err := p.cache.Source(ctx, src) + if errors.Is(err, imgcache.ErrNotFound) { + return nil, 0 + } + if err != nil { - s.log.Warn("failed to load cached source, fetching", "error", err) + p.log.Warn("failed to load cached source, fetching", "error", err) return nil, 0 } - if size > s.maxResponseSize { + p.log.Debug("using cached source", "source_key", src.Key()) + + if size > p.maxResponseSize { _ = reader.Close() - s.log.Warn("cached source exceeds max response size, discarding", - "source_key", sourceKey, - "max_bytes", s.maxResponseSize, + p.log.Warn("cached source exceeds max response size, discarding", + "source_key", src.Key(), + "max_bytes", p.maxResponseSize, ) return nil, 0 @@ -275,30 +344,16 @@ func (s *Service) loadCachedSource( // processFromSourceOrFetch processes an image, using cached source content // if available. -func (s *Service) processFromSourceOrFetch( +func (p *Proxy) processFromSourceOrFetch( ctx context.Context, req *imagerequest.Request, cacheKey imagerequest.VariantKey, ) (*processedVariant, error) { - // Check if we have cached source content - sourceKey, err := s.cache.LookupSource(ctx, req) - if err != nil { - s.log.Warn("source lookup failed", "error", err) - } - - var ( - source io.ReadCloser - sourceSize int64 - ) - - if sourceKey != "" { - s.log.Debug("using cached source", "source_key", sourceKey) - source, sourceSize = s.loadCachedSource(sourceKey) - } + source, sourceSize := p.loadCachedSource(ctx, req.Source) // Fetch from upstream if we don't have source data or it's empty if source == nil { - return s.fetchAndProcess(ctx, req, cacheKey) + return p.fetchAndProcess(ctx, req, cacheKey) } defer func() { _ = source.Close() }() @@ -306,26 +361,26 @@ func (s *Service) processFromSourceOrFetch( // Process using cached source; nothing was fetched from upstream. The // image processor reads the source only once it has a processing slot, // so a request waiting for one holds none of it in memory. - return s.processAndStore(ctx, req, cacheKey, source, sourceSize) + return p.processAndStore(ctx, req, cacheKey, source, sourceSize) } // fetchAndProcess fetches from upstream, processes, and caches the result. // It counts the fetch with the bytes read from upstream, including when // reading the response or a later step fails. It fetches nothing while the // source URL is in the negative cache. -func (s *Service) fetchAndProcess( +func (p *Proxy) fetchAndProcess( ctx context.Context, req *imagerequest.Request, cacheKey imagerequest.VariantKey, ) (*processedVariant, error) { // Check negative cache - skip fetching for recently-failed URLs - negHit, err := s.cache.checkNegativeCache(ctx, req) + negHit, err := p.cache.Failure(ctx, req.Source) if err != nil { - s.log.Warn("negative cache check failed", "error", err) + p.log.Warn("negative cache check failed", "error", err) } if negHit { - s.log.Debug("negative cache hit", + p.log.Debug("negative cache hit", "host", req.Source.Host, "path", req.Source.Path, ) @@ -334,14 +389,14 @@ func (s *Service) fetchAndProcess( } // Fetch from upstream - s.log.Debug("fetching from upstream", "host", req.Source.Host, + p.log.Debug("fetching from upstream", "host", req.Source.Host, "path", req.Source.Path, "query", req.Source.Query) - fetchResult, err := s.fetcher.Fetch(ctx, req.Source) + fetchResult, err := p.fetcher.Fetch(ctx, req.Source) if err != nil { // Store negative cache for certain errors if isNegativeCacheable(err) { - s.storeNegative(ctx, req) + p.storeNegative(ctx, req) } return nil, fmt.Errorf("upstream fetch failed: %w", err) @@ -358,7 +413,7 @@ func (s *Service) fetchAndProcess( fetchBytes := int64(len(sourceData)) // Counted also when the request context has ended meanwhile - s.cache.IncrementUpstreamFetch(fetchBytes) + p.incrementUpstreamFetch(fetchBytes) if err != nil { return nil, fmt.Errorf("failed to read upstream response: %w", err) @@ -374,7 +429,7 @@ func (s *Service) fetchAndProcess( } // Log upstream fetch details - s.log.Info("upstream fetched", + p.log.Info("upstream fetched", "request_id", middleware.GetReqID(ctx), "host", req.Source.Host, "path", req.Source.Path, @@ -392,33 +447,44 @@ func (s *Service) fetchAndProcess( // error status does. err = magic.ValidateMagicBytes(sourceData, fetchResult.ContentType) if err != nil { - s.log.Warn("source refused", "request_id", middleware.GetReqID(ctx), + p.log.Warn("source refused", "request_id", middleware.GetReqID(ctx), "host", req.Source.Host, "path", req.Source.Path, "content_type", fetchResult.ContentType, "error", err) - s.storeNegative(ctx, req) + p.storeNegative(ctx, req) return nil, fmt.Errorf("content validation failed: %w", err) } // Store source content - _, err = s.cache.StoreSource(ctx, req, bytes.NewReader(sourceData)) + _, err = p.cache.PutSource(ctx, req.Source, bytes.NewReader(sourceData)) if err != nil { - s.log.Warn("failed to store source content", "error", err) + p.log.Warn("failed to store source content", "error", err) // Continue even if caching fails } - return s.processAndStore( + return p.processAndStore( ctx, req, cacheKey, bytes.NewReader(sourceData), fetchBytes, ) } +// incrementUpstreamFetch counts one upstream fetch that read fetchBytes bytes. +// A fetch that read no bytes is not counted. +func (p *Proxy) incrementUpstreamFetch(fetchBytes int64) { + if fetchBytes <= 0 { + return + } + + p.upstreamFetches.Inc() + p.upstreamFetchBytes.Add(float64(fetchBytes)) +} + // storeNegative stores a negative cache entry for req's source, whose fetch // failed, so the source is not fetched again until the entry expires. A // failure to store the entry is only logged. -func (s *Service) storeNegative(ctx context.Context, req *imagerequest.Request) { - err := s.cache.StoreNegative(ctx, req) +func (p *Proxy) storeNegative(ctx context.Context, req *imagerequest.Request) { + err := p.cache.PutFailure(ctx, req.Source) if err != nil { - s.log.Warn("failed to store negative cache entry", + p.log.Warn("failed to store negative cache entry", "host", req.Source.Host, "path", req.Source.Path, "error", err) } } @@ -433,7 +499,7 @@ type processedVariant struct { // processAndStore processes the image read from source and stores the // result. -func (s *Service) processAndStore( +func (p *Proxy) processAndStore( ctx context.Context, req *imagerequest.Request, cacheKey imagerequest.VariantKey, @@ -445,7 +511,7 @@ func (s *Service) processAndStore( // ctx has the deadline of the request doing the processing, which can // end while a request waiting for its image has time left - processResult, err := s.processor.Process(ctx, source, &req.Transform) + processResult, err := p.processor.Process(ctx, source, &req.Transform) if errors.Is(err, context.DeadlineExceeded) { return nil, fmt.Errorf("%w: %w", ErrProcessingTimedOut, err) } @@ -457,7 +523,7 @@ func (s *Service) processAndStore( processDuration := time.Since(processStart) // Counted also when the request context has ended meanwhile - s.cache.IncrementTransformCount() + p.transforms.Inc() // Read processed content processedData, err := io.ReadAll(processResult.Content) @@ -476,7 +542,7 @@ func (s *Service) processAndStore( sizePercent = float64(outputSize) / float64(fetchBytes) * 100.0 } - s.log.Info("image converted", + p.log.Info("image converted", "request_id", middleware.GetReqID(ctx), "host", req.Source.Host, "path", req.Source.Path, @@ -495,9 +561,9 @@ func (s *Service) processAndStore( ) // Store variant to cache - err = s.cache.StoreVariant(ctx, cacheKey, bytes.NewReader(processedData)) + err = p.cache.PutVariant(ctx, cacheKey, bytes.NewReader(processedData)) if err != nil { - s.log.Warn("failed to store variant", "error", err) + p.log.Warn("failed to store variant", "error", err) // Continue even if caching fails } diff --git a/internal/imgcache/service_internal_test.go b/internal/imageproxy/imageproxy_internal_test.go similarity index 86% rename from internal/imgcache/service_internal_test.go rename to internal/imageproxy/imageproxy_internal_test.go index ed8ae02..5e392ea 100644 --- a/internal/imgcache/service_internal_test.go +++ b/internal/imageproxy/imageproxy_internal_test.go @@ -1,4 +1,4 @@ -package imgcache +package imageproxy import ( "context" @@ -17,10 +17,10 @@ const ( testPathUpload = "/uploads/image.jpg" ) -func TestService_Get_GoodHost(t *testing.T) { +func TestProxy_Get_GoodHost(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() req := &imagerequest.Request{ @@ -58,12 +58,12 @@ func TestService_Get_GoodHost(t *testing.T) { } } -// TestService_Get_OtherHost checks that the image service serves a second host -// as it serves GoodHost. -func TestService_Get_OtherHost(t *testing.T) { +// TestProxy_Get_OtherHost checks that the proxy serves a second host as it +// serves GoodHost. +func TestProxy_Get_OtherHost(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() req := &imagerequest.Request{ @@ -97,10 +97,10 @@ func TestService_Get_OtherHost(t *testing.T) { } } -func TestService_Get_InvalidFile(t *testing.T) { +func TestProxy_Get_InvalidFile(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() req := photoVariant(fixtures, 85, imagerequest.FitCover) @@ -114,10 +114,10 @@ func TestService_Get_InvalidFile(t *testing.T) { } } -func TestService_Get_NotFound(t *testing.T) { +func TestProxy_Get_NotFound(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() req := photoVariant(fixtures, 85, imagerequest.FitCover) @@ -130,10 +130,10 @@ func TestService_Get_NotFound(t *testing.T) { } } -func TestService_Get_FormatConversion(t *testing.T) { +func TestProxy_Get_FormatConversion(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() tests := []struct { @@ -213,10 +213,10 @@ func TestService_Get_FormatConversion(t *testing.T) { } } -func TestService_Get_Caching(t *testing.T) { +func TestProxy_Get_Caching(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() req := &imagerequest.Request{ @@ -272,7 +272,7 @@ func TestService_Get_Caching(t *testing.T) { } } -// TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo caches a source and +// TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgo caches a source and // a variant of it and sets back two days every time pixa stored for them. // For the source: last_accessed_at in source_content, and the access and // modification times of its file. For the variant: last_accessed_at in @@ -283,10 +283,10 @@ func TestService_Get_Caching(t *testing.T) { // Left at the present: the files' status change and birth times, which a // program cannot set, and the times of the directories holding the files, // which other entries share. -func TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo(t *testing.T) { +func TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgo(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) variant := photoVariant(fixtures, 85, imagerequest.FitCover) resp, err := svc.Get(t.Context(), variant) @@ -297,8 +297,8 @@ func TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo(t *testing.T) { _ = resp.Content.Close() twoDaysAgo := time.Now().AddDate(0, 0, -2) - backdateSource(t, svc.cache, variant, twoDaysAgo) - backdateVariant(t, svc.cache, variant, twoDaysAgo) + backdateSource(t, svc, variant, twoDaysAgo) + backdateVariant(t, svc, variant, twoDaysAgo) resp, err = svc.Get(t.Context(), variant) if err != nil { @@ -320,23 +320,23 @@ func TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo(t *testing.T) { _ = resp.Content.Close() - fetches := readCounters(svc.cache).upstreamFetchCount + fetches := readCounters(t, svc).upstreamFetchCount if fetches != 1 { t.Errorf("upstream fetches = %d, want 1", fetches) } } // backdateSource sets every time pixa stored for req's cached source to when, -// as listed on TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo. +// as listed on TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgo. func backdateSource( - t *testing.T, cache *Cache, req *imagerequest.Request, when time.Time, + t *testing.T, svc *testProxy, req *imagerequest.Request, when time.Time, ) { t.Helper() key := req.Source.Key() - setSourceLastAccessed(t, cache, key, when) + setSourceLastAccessed(t, svc.db, key, when) - path := cache.srcContent.keyToPath(key) + path := sourceFilePath(svc, key) err := os.Chtimes(path, when, when) if err != nil { @@ -346,16 +346,16 @@ func backdateSource( // backdateVariant sets every time pixa stored for the cached variant req asks // for to when, as listed on -// TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo. +// TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgo. func backdateVariant( - t *testing.T, cache *Cache, req *imagerequest.Request, when time.Time, + t *testing.T, svc *testProxy, req *imagerequest.Request, when time.Time, ) { t.Helper() key := imagerequest.CacheKey(req) - path := cache.variants.keyToPath(key) + path := variantFilePath(svc, key) - setVariantLastAccessed(t, cache, key, when) + setVariantLastAccessed(t, svc.db, key, when) err := os.Chtimes(path, when, when) if err != nil { @@ -363,10 +363,10 @@ func backdateVariant( } } -func TestService_Get_DifferentSizes(t *testing.T) { +func TestProxy_Get_DifferentSizes(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() // Request same image at different sizes @@ -417,10 +417,10 @@ func TestService_Get_DifferentSizes(t *testing.T) { } } -func TestService_Get_ContextCancellation(t *testing.T) { +func TestProxy_Get_ContextCancellation(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx, cancel := context.WithCancel(context.Background()) cancel() // Cancel immediately @@ -444,10 +444,10 @@ func TestService_Get_ContextCancellation(t *testing.T) { } } -func TestService_Get_ReturnsETag(t *testing.T) { +func TestProxy_Get_ReturnsETag(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() req := &imagerequest.Request{ @@ -481,10 +481,10 @@ func TestService_Get_ReturnsETag(t *testing.T) { } } -func TestService_Get_ETagConsistency(t *testing.T) { +func TestProxy_Get_ETagConsistency(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() req := &imagerequest.Request{ @@ -526,10 +526,10 @@ func TestService_Get_ETagConsistency(t *testing.T) { } } -func TestService_Get_DifferentETagsForDifferentContent(t *testing.T) { +func TestProxy_Get_DifferentETagsForDifferentContent(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx := context.Background() // Request same image at different sizes - should get different ETags diff --git a/internal/imgcache/max_concurrent_processing_internal_test.go b/internal/imageproxy/max_concurrent_processing_internal_test.go similarity index 86% rename from internal/imgcache/max_concurrent_processing_internal_test.go rename to internal/imageproxy/max_concurrent_processing_internal_test.go index 1de6011..5d49d16 100644 --- a/internal/imgcache/max_concurrent_processing_internal_test.go +++ b/internal/imageproxy/max_concurrent_processing_internal_test.go @@ -1,4 +1,4 @@ -package imgcache +package imageproxy import ( "image/color" @@ -55,16 +55,16 @@ func holdProcessingSlot( return release } -// TestService_Get_WaitsForSlotBeforeReadingCachedSource checks that a +// TestProxy_Get_WaitsForSlotBeforeReadingCachedSource checks that a // request whose source is cached holds none of it while it waits for a // processing slot: it reads the cached file only once it has a slot. With // the only slot held, a request for a new width of the cached 100x100 photo // waits; the cached file is then rewritten as a 100x50 image before the slot // is freed, so the request must answer with that image scaled to 40x20. -func TestService_Get_WaitsForSlotBeforeReadingCachedSource(t *testing.T) { +func TestProxy_Get_WaitsForSlotBeforeReadingCachedSource(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) svc.processor = imageprocessor.New( imageprocessor.Params{MaxInputBytes: 50 << 20, MaxConcurrentProcessing: 1}, ) @@ -77,13 +77,15 @@ func TestService_Get_WaitsForSlotBeforeReadingCachedSource(t *testing.T) { _ = resp.Content.Close() - sourceKey, err := svc.cache.LookupSource(t.Context(), - widthOnlyRequest(fixtures, 50)) - if err != nil || sourceKey == "" { - t.Fatalf("LookupSource() = %q, %v; want the cached source", - sourceKey, err) + cachedSource := widthOnlyRequest(fixtures, 50).Source + + reader, _, err := svc.cache.Source(t.Context(), cachedSource) + if err != nil { + t.Fatalf("Source() error = %v; want the cached source", err) } + _ = reader.Close() + release := holdProcessingSlot(t, svc.processor) var ( @@ -103,7 +105,7 @@ func TestService_Get_WaitsForSlotBeforeReadingCachedSource(t *testing.T) { // before waiting, it would have read it by now. time.Sleep(100 * time.Millisecond) - err = os.WriteFile(svc.cache.srcContent.keyToPath(sourceKey), + err = os.WriteFile(sourceFilePath(svc, cachedSource.Key()), generateTestJPEG(t, 100, 50, color.RGBA{0, 0, 255, 255}), 0o600) if err != nil { t.Fatalf("failed to rewrite the cached source: %v", err) diff --git a/internal/imageproxy/negative_cache_internal_test.go b/internal/imageproxy/negative_cache_internal_test.go new file mode 100644 index 0000000..23ca916 --- /dev/null +++ b/internal/imageproxy/negative_cache_internal_test.go @@ -0,0 +1,96 @@ +package imageproxy + +import ( + "errors" + "os" + "testing" + + "sneak.berlin/go/pixa/internal/imagerequest" +) + +// TestProxy_Get_ServesCachedVariantOfNegativeCachedURL caches a variant of +// the test photo, then keeps a failed fetch of the photo's URL in the negative +// cache, as a failed fetch for another variant does once the source has been +// evicted. The cached variant is still served, and a variant not in the cache +// is made from the cached source. Once the source is evicted, a variant not in +// the cache needs a fetch and is refused. +func TestProxy_Get_ServesCachedVariantOfNegativeCachedURL(t *testing.T) { + t.Parallel() + + svc, fixtures := setupTestProxy(t) + + resp, err := svc.Get(t.Context(), photoVariant(fixtures, 85, imagerequest.FitCover)) + if err != nil { + t.Fatalf("Get() first request error = %v", err) + } + + _ = resp.Content.Close() + + err = svc.cache.PutFailure(t.Context(), + photoVariant(fixtures, 85, imagerequest.FitCover).Source) + if err != nil { + t.Fatalf("PutFailure() error = %v", err) + } + + resp, err = svc.Get(t.Context(), photoVariant(fixtures, 85, imagerequest.FitCover)) + if err != nil { + t.Fatalf("Get() of the cached variant error = %v", err) + } + + _ = resp.Content.Close() + + if resp.CacheStatus != CacheHit { + t.Errorf("cached variant CacheStatus = %q, want %q", + resp.CacheStatus, CacheHit) + } + + resp, err = svc.Get(t.Context(), photoVariant(fixtures, 40, imagerequest.FitCover)) + t.Logf("Get() of a variant made from the cached source: error = %v", err) + + if err != nil { + t.Fatalf("Get() of a variant made from the cached source error = %v", err) + } + + _ = resp.Content.Close() + + source := photoVariant(fixtures, 85, imagerequest.FitCover).Source + + reader, _, err := svc.cache.Source(t.Context(), source) + if err != nil { + t.Fatalf("Source() error = %v, want the cached source", err) + } + + _ = reader.Close() + + // Evicts the source as the cache's eviction does: its row, then its file + _, err = svc.db.ExecContext(t.Context(), + `DELETE FROM source_content WHERE source_key = ?`, string(source.Key())) + if err != nil { + t.Fatalf("deleting the source's row: %v", err) + } + + err = os.Remove(sourceFilePath(svc, source.Key())) + if err != nil { + t.Fatalf("removing the source's file: %v", err) + } + + _, err = svc.Get(t.Context(), photoVariant(fixtures, 60, imagerequest.FitCover)) + t.Logf("Get() of a variant whose source was evicted: error = %v", err) + + if !errors.Is(err, ErrNegativeCached) { + t.Errorf("Get() of a variant whose source was evicted error = %v, want %v", + err, ErrNegativeCached) + } +} + +func TestProxy_Get_ReturnsErrorForNegativeCachedURL(t *testing.T) { + t.Parallel() + + // This test verifies that Proxy.Get() checks the negative cache + // We can't easily test the full pipeline without vips, but we can + // verify the error type + err := ErrNegativeCached + if !errors.Is(err, ErrNegativeCached) { + t.Error("ErrNegativeCached should be identifiable with errors.Is") + } +} diff --git a/internal/imageproxy/partial_cache_files_internal_test.go b/internal/imageproxy/partial_cache_files_internal_test.go new file mode 100644 index 0000000..f1b7c4f --- /dev/null +++ b/internal/imageproxy/partial_cache_files_internal_test.go @@ -0,0 +1,149 @@ +package imageproxy + +import ( + "bytes" + "io" + "os" + "testing" + + "sneak.berlin/go/pixa/internal/imagerequest" +) + +// getImage calls Get for req and returns the image it answered with and +// its cache status. +func getImage( + t *testing.T, svc *testProxy, req *imagerequest.Request, +) ([]byte, CacheStatus) { + t.Helper() + + resp, err := svc.Get(t.Context(), req) + if err != nil { + t.Fatalf("Get() of quality %d error = %v", req.Transform.Quality, err) + } + + defer func() { _ = resp.Content.Close() }() + + body, err := io.ReadAll(resp.Content) + if err != nil { + t.Fatalf("reading the image of quality %d: %v", req.Transform.Quality, err) + } + + return body, resp.CacheStatus +} + +// fileSize returns the size of the file at path. +func fileSize(t *testing.T, path string) int64 { + t.Helper() + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s: %v", path, err) + } + + return info.Size() +} + +// cachedSourcePath returns the path of the cached source file req uses. +func cachedSourcePath( + t *testing.T, svc *testProxy, req *imagerequest.Request, +) string { + t.Helper() + + reader, _, err := svc.cache.Source(t.Context(), req.Source) + if err != nil { + t.Fatalf("Source() error = %v; want the cached source", err) + } + + _ = reader.Close() + + return sourceFilePath(svc, req.Source.Key()) +} + +// TestProxy_Get_FetchesAgainForRemovedCachedSource removes the cached +// source's row and file, as the reconciliation pass does when the file is +// empty or cut short. internal/imgcache tests that the pass does so. The +// test then asks for two new sizes of the source. The first must fetch it +// again and cache it whole, so the second is made from the cache without a +// third fetch. +func TestProxy_Get_FetchesAgainForRemovedCachedSource(t *testing.T) { + t.Parallel() + + svc, fixtures := setupTestProxy(t) + + getImage(t, svc, photoVariant(fixtures, 85, imagerequest.FitCover)) + + source := photoVariant(fixtures, 85, imagerequest.FitCover).Source + path := cachedSourcePath(t, svc, photoVariant(fixtures, 85, imagerequest.FitCover)) + wholeSize := fileSize(t, path) + + // Removes the source as the reconciliation pass does: its row, then its file + _, err := svc.db.ExecContext(t.Context(), + `DELETE FROM source_content WHERE source_key = ?`, string(source.Key())) + if err != nil { + t.Fatalf("deleting the source's row: %v", err) + } + + err = os.Remove(path) + if err != nil { + t.Fatalf("removing the source's file: %v", err) + } + + getImage(t, svc, photoVariant(fixtures, 40, imagerequest.FitCover)) + getImage(t, svc, photoVariant(fixtures, 30, imagerequest.FitCover)) + + fetches := readCounters(t, svc).upstreamFetchCount + t.Logf("upstream fetches = %d", fetches) + + if fetches != 2 { + t.Errorf("upstream fetches = %d, want 2", fetches) + } + + size := fileSize(t, path) + if size != wholeSize { + t.Errorf("cached source is %d bytes, want %d", size, wholeSize) + } +} + +// TestProxy_Get_ConvertsAgainForRemovedCachedVariant caches a variant of +// the photo and removes its row and file, as the reconciliation pass does +// when the file is empty or cut short. internal/imgcache tests that the +// pass does so. The test then checks that Get converts the image again, and +// that the next Get serves the whole variant from the cache. +func TestProxy_Get_ConvertsAgainForRemovedCachedVariant(t *testing.T) { + t.Parallel() + + svc, fixtures := setupTestProxy(t) + req := photoVariant(fixtures, 85, imagerequest.FitCover) + + want, _ := getImage(t, svc, req) + + key := imagerequest.CacheKey(req) + + // Removes the variant as the reconciliation pass does: its row, then its file + _, err := svc.db.ExecContext(t.Context(), + `DELETE FROM variant_content WHERE cache_key = ?`, string(key)) + if err != nil { + t.Fatalf("deleting the variant's row: %v", err) + } + + err = os.Remove(variantFilePath(svc, key)) + if err != nil { + t.Fatalf("removing the variant's file: %v", err) + } + + got, status := getImage(t, svc, req) + t.Logf("after the removal: %d bytes, %s", len(got), status) + + if status != CacheMiss || !bytes.Equal(got, want) { + t.Errorf("after the removal: Get() = %d bytes, %s; want the %d bytes "+ + "converted again, %s", len(got), status, len(want), CacheMiss) + } + + got, status = getImage(t, svc, req) + t.Logf("next request: %d bytes, %s", len(got), status) + + if status != CacheHit || !bytes.Equal(got, want) { + t.Errorf("next request: Get() = %d bytes, %s; want the %d bytes, %s", + len(got), status, len(want), CacheHit) + } +} diff --git a/internal/imageproxy/pendingaccesstimes_internal_test.go b/internal/imageproxy/pendingaccesstimes_internal_test.go new file mode 100644 index 0000000..3fcb1cf --- /dev/null +++ b/internal/imageproxy/pendingaccesstimes_internal_test.go @@ -0,0 +1,78 @@ +package imageproxy + +import ( + "context" + "database/sql" + "errors" + "testing" + "time" + + "sneak.berlin/go/pixa/internal/imagerequest" +) + +// pendingAccessTimesWait is how long a test waits for the database's +// connection, for a cache hit to be answered, for an access time to reach +// the database, or for a request to return past its deadline. +const pendingAccessTimesWait = 5 * time.Second + +// holdDatabase takes the one connection of db, so that every other query +// waits for it, and returns the func that frees it. +func holdDatabase(t *testing.T, db *sql.DB) func() { + t.Helper() + + ctx, cancel := context.WithTimeout(t.Context(), pendingAccessTimesWait) + defer cancel() + + conn, err := db.Conn(ctx) + if err != nil { + t.Fatalf("failed to take the database connection: %v", err) + } + + return func() { _ = conn.Close() } +} + +// TestProxy_Get_ReturnsByItsDeadlineWhileTheDatabaseIsBusy holds the +// database's one connection while a request whose fetch is held reaches its +// deadline. The request must return with the deadline's error while the +// connection is still held, as it waits for no database write past its +// deadline. +func TestProxy_Get_ReturnsByItsDeadlineWhileTheDatabaseIsBusy(t *testing.T) { + t.Parallel() + + svc, fixtures, fetcher := setupHeldFetchProxy(t) + + svc.cache.StartPendingAccessTimeWrites() + defer func() { _ = svc.cache.StopPendingAccessTimeWrites(t.Context()) }() + + // Room for the request to reach its fetch on a busy host + const timeout = 2 * time.Second + + ctx, cancel := context.WithTimeout(t.Context(), timeout) + defer cancel() + + deadline, _ := ctx.Deadline() + + results := startGet(ctx, svc, photoVariant(fixtures, 85, imagerequest.FitCover)) + + // The request has made its database reads by the time it fetches. + select { + case <-fetcher.started: + case <-time.After(timeout): + t.Fatal("request did not reach its fetch by its deadline") + } + + releaseDatabase := holdDatabase(t, svc.db) + defer releaseDatabase() + + select { + case got := <-results: + t.Logf("Get() returned %v after its deadline, error = %v", + time.Since(deadline), got.err) + + if !errors.Is(got.err, context.DeadlineExceeded) { + t.Errorf("Get() error = %v, want %v", got.err, context.DeadlineExceeded) + } + case <-time.After(time.Until(deadline) + pendingAccessTimesWait): + t.Fatal("request did not return while the database was busy") + } +} diff --git a/internal/imgcache/stats_internal_test.go b/internal/imageproxy/stats_internal_test.go similarity index 83% rename from internal/imgcache/stats_internal_test.go rename to internal/imageproxy/stats_internal_test.go index 6e4912a..14fa7cf 100644 --- a/internal/imgcache/stats_internal_test.go +++ b/internal/imageproxy/stats_internal_test.go @@ -1,4 +1,4 @@ -package imgcache +package imageproxy import ( "bytes" @@ -12,15 +12,16 @@ import ( "sneak.berlin/go/pixa/internal/imagerequest" ) -// TestService_Get_CountsStats walks Get through a miss that fetches the +// TestProxy_Get_CountsStats walks Get through a miss that fetches the // source, a hit, a miss that reuses the cached source, and two misses whose -// source cannot be used, checking every counter of the cache after each. -func TestService_Get_CountsStats(t *testing.T) { +// source cannot be used, checking every counter of the proxy and its cache +// after each. +func TestProxy_Get_CountsStats(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) - // NewTestFS builds the same files the test service's fetcher serves. + // NewTestFS builds the same files the test proxy's fetcher serves. testFS, _ := NewTestFS(t) photo, err := fs.ReadFile(testFS, fixtures.GoodHostJPEG) @@ -76,7 +77,7 @@ func TestService_Get_CountsStats(t *testing.T) { _ = resp.Content.Close() } - got := readCounters(svc.cache) + got := readCounters(t, svc) if got != step.want { t.Fatalf("after the %s: counters = %+v, want %+v", step.name, got, step.want) } @@ -114,10 +115,11 @@ func (u *fakeUpstream) Read(p []byte) (int, error) { return 0, u.err } -// TestService_Get_CountsInterruptedMisses checks every counter of the cache -// after a miss whose request context ends during or after the upstream -// fetch, and after a miss whose upstream body is over the size limit. -func TestService_Get_CountsInterruptedMisses(t *testing.T) { +// TestProxy_Get_CountsInterruptedMisses checks every counter of the proxy +// and its cache after a miss whose request context ends during or after the +// upstream fetch, and after a miss whose upstream body is over the size +// limit. +func TestProxy_Get_CountsInterruptedMisses(t *testing.T) { t.Parallel() photo := generateTestJPEG(t, 100, 100, color.RGBA{255, 0, 0, 255}) @@ -145,7 +147,7 @@ func TestService_Get_CountsInterruptedMisses(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) ctx, cancel := context.WithCancel(t.Context()) defer cancel() @@ -182,7 +184,7 @@ func TestService_Get_CountsInterruptedMisses(t *testing.T) { _ = resp.Content.Close() } - got := readCounters(svc.cache) + got := readCounters(t, svc) if got != tc.want { t.Errorf("counters = %+v, want %+v", got, tc.want) } @@ -190,13 +192,13 @@ func TestService_Get_CountsInterruptedMisses(t *testing.T) { } } -// TestService_Get_CountsHitAfterRequestEnds checks every counter of the -// cache after a hit served with a request context that has already ended: -// only the hit count moves. -func TestService_Get_CountsHitAfterRequestEnds(t *testing.T) { +// TestProxy_Get_CountsHitAfterRequestEnds checks every counter of the +// proxy and its cache after a hit served with a request context that has +// already ended: only the hit count moves. +func TestProxy_Get_CountsHitAfterRequestEnds(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + svc, fixtures := setupTestProxy(t) req := &imagerequest.Request{ Source: imagerequest.Source{ @@ -219,7 +221,7 @@ func TestService_Get_CountsHitAfterRequestEnds(t *testing.T) { _ = resp.Content.Close() - want := readCounters(svc.cache) + want := readCounters(t, svc) want.hitCount++ ctx, cancel := context.WithCancel(t.Context()) @@ -236,7 +238,7 @@ func TestService_Get_CountsHitAfterRequestEnds(t *testing.T) { t.Fatalf("CacheStatus = %v, want %v", resp.CacheStatus, CacheHit) } - got := readCounters(svc.cache) + got := readCounters(t, svc) if got != want { t.Errorf("counters = %+v, want %+v", got, want) } diff --git a/internal/imageproxy/testutil_internal_test.go b/internal/imageproxy/testutil_internal_test.go new file mode 100644 index 0000000..4d8fa4d --- /dev/null +++ b/internal/imageproxy/testutil_internal_test.go @@ -0,0 +1,355 @@ +package imageproxy + +import ( + "bytes" + "context" + "database/sql" + "image" + "image/color" + "image/gif" + "image/jpeg" + "image/png" + "io/fs" + "path/filepath" + "testing" + "testing/fstest" + "time" + + "github.com/prometheus/client_golang/prometheus" + "sneak.berlin/go/pixa/internal/database" + "sneak.berlin/go/pixa/internal/httpfetcher" + "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imagerequest" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// Shared test data literals, extracted as constants for goconst. +const ( + testContentTypeJPEG = "image/jpeg" +) + +// sqliteTimestampFormat matches the format SQLite's CURRENT_TIMESTAMP +// produces, so injected timestamps compare correctly against ones the +// cache writes. +const sqliteTimestampFormat = "2006-01-02 15:04:05" + +// TestFixtures contains paths to test files in the mock filesystem. +type TestFixtures struct { + // Valid image files + GoodHostJPEG string // valid JPEG on GoodHost + GoodHostPNG string // valid PNG on GoodHost + GoodHostGIF string // valid GIF on GoodHost + OtherHostJPEG string // valid JPEG on OtherHost + OtherHostPNG string // valid PNG on OtherHost + + // Invalid/edge case files + InvalidFile string // file with wrong magic bytes + EmptyFile string // zero-byte file + TextFile string // text file masquerading as image + + // Hostnames + GoodHost string // a host the mock fetcher serves + OtherHost string // a second host the mock fetcher serves +} + +// DefaultFixtures returns the standard test fixture paths. +func DefaultFixtures() *TestFixtures { + return &TestFixtures{ + GoodHostJPEG: "goodhost.example.com/images/photo.jpg", + GoodHostPNG: "goodhost.example.com/images/logo.png", + GoodHostGIF: "goodhost.example.com/images/animation.gif", + OtherHostJPEG: "otherhost.example.com/uploads/image.jpg", + OtherHostPNG: "otherhost.example.com/uploads/icon.png", + InvalidFile: "goodhost.example.com/images/fake.jpg", + EmptyFile: "goodhost.example.com/images/empty.jpg", + TextFile: "goodhost.example.com/images/text.png", + GoodHost: "goodhost.example.com", + OtherHost: "otherhost.example.com", + } +} + +// NewTestFS creates a mock filesystem with test images. +func NewTestFS(t *testing.T) (fs.FS, *TestFixtures) { + t.Helper() + + fixtures := DefaultFixtures() + + // Generate test images + jpegData := generateTestJPEG(t, 100, 100, color.RGBA{255, 0, 0, 255}) + pngData := generateTestPNG(t, 100, 100, color.RGBA{0, 255, 0, 255}) + gifData := generateTestGIF(t, 100, 100, color.RGBA{0, 0, 255, 255}) + + // Create the mock filesystem + mockFS := fstest.MapFS{ + // Good host files + fixtures.GoodHostJPEG: &fstest.MapFile{Data: jpegData}, + fixtures.GoodHostPNG: &fstest.MapFile{Data: pngData}, + fixtures.GoodHostGIF: &fstest.MapFile{Data: gifData}, + + // Other host files + fixtures.OtherHostJPEG: &fstest.MapFile{Data: jpegData}, + fixtures.OtherHostPNG: &fstest.MapFile{Data: pngData}, + + // Invalid files + fixtures.InvalidFile: &fstest.MapFile{Data: []byte("not a real image file content")}, + fixtures.EmptyFile: &fstest.MapFile{Data: []byte{}}, + fixtures.TextFile: &fstest.MapFile{Data: []byte("This is just text, not a PNG")}, + } + + return mockFS, fixtures +} + +// generateTestJPEG creates a minimal valid JPEG image. +func generateTestJPEG(t *testing.T, width, height int, c color.Color) []byte { + t.Helper() + + img := image.NewRGBA(image.Rect(0, 0, width, height)) + for y := range height { + for x := range width { + img.Set(x, y, c) + } + } + + var buf bytes.Buffer + + err := jpeg.Encode(&buf, img, &jpeg.Options{Quality: 85}) + if err != nil { + t.Fatalf("failed to encode test JPEG: %v", err) + } + + return buf.Bytes() +} + +// generateTestPNG creates a minimal valid PNG image. +func generateTestPNG(t *testing.T, width, height int, c color.Color) []byte { + t.Helper() + + img := image.NewRGBA(image.Rect(0, 0, width, height)) + for y := range height { + for x := range width { + img.Set(x, y, c) + } + } + + var buf bytes.Buffer + + err := png.Encode(&buf, img) + if err != nil { + t.Fatalf("failed to encode test PNG: %v", err) + } + + return buf.Bytes() +} + +// generateTestGIF creates a minimal valid GIF image. +func generateTestGIF(t *testing.T, width, height int, c color.Color) []byte { + t.Helper() + + img := image.NewPaletted( + image.Rect(0, 0, width, height), + []color.Color{c, color.White}, + ) + for y := range height { + for x := range width { + img.SetColorIndex(x, y, 0) + } + } + + var buf bytes.Buffer + + err := gif.Encode(&buf, img, nil) + if err != nil { + t.Fatalf("failed to encode test GIF: %v", err) + } + + return buf.Bytes() +} + +// testProxy is a Proxy with the database and the state directory of its +// cache, through which a test reads and changes the cache behind the proxy. +// registry is the metrics registry the proxy and its cache share. +type testProxy struct { + *Proxy + + db *sql.DB + stateDir string + registry *prometheus.Registry +} + +// setupTestProxy creates a Proxy with mock fetcher for testing. +func setupTestProxy(t *testing.T) (*testProxy, *TestFixtures) { + t.Helper() + + mockFS, fixtures := NewTestFS(t) + + // Create temp directory for cache + tmpDir := t.TempDir() + + // Create in-memory database + db := setupTestDB(t) + + registry := prometheus.NewRegistry() + + cache, err := imgcache.NewCache(db, imgcache.Config{ + StateDir: tmpDir, + NegativeTTL: 5 * time.Minute, + MaxBytes: 1 << 30, + }, registry) + if err != nil { + t.Fatalf("failed to create cache: %v", err) + } + + proxy, err := NewProxy(&Config{ + Cache: cache, + Fetcher: httpfetcher.NewMock(mockFS), + Processor: imageprocessor.New(imageprocessor.Params{ + MaxInputBytes: 50 << 20, + MaxConcurrentProcessing: 2, + }), + MaxResponseSize: 50 << 20, + }, registry) + if err != nil { + t.Fatalf("failed to create proxy: %v", err) + } + + return &testProxy{ + Proxy: proxy, + db: db, + stateDir: tmpDir, + registry: registry, + }, fixtures +} + +// counterValues holds the values of the counters of a proxy and its cache. +type counterValues struct { + hitCount int64 + missCount int64 + upstreamFetchCount int64 + upstreamFetchBytes int64 + transformCount int64 +} + +// readCounters reads the counters of svc and its cache from their metrics +// registry. +func readCounters(t *testing.T, svc *testProxy) counterValues { + t.Helper() + + families, err := svc.registry.Gather() + if err != nil { + t.Fatalf("failed to gather the counters: %v", err) + } + + values := make(map[string]int64, len(families)) + + for _, family := range families { + values[family.GetName()] = int64(family.GetMetric()[0].GetCounter().GetValue()) + } + + return counterValues{ + hitCount: values["pixa_cache_hits_total"], + missCount: values["pixa_cache_misses_total"], + upstreamFetchCount: values["pixa_upstream_fetches_total"], + upstreamFetchBytes: values["pixa_upstream_fetch_bytes_total"], + transformCount: values["pixa_transforms_total"], + } +} + +// sourceFilePath returns the path of the file of the source key in svc's +// cache, as README.md gives it: /cache/sources///. +func sourceFilePath(svc *testProxy, key imagerequest.SourceKey) string { + name := string(key) + + return filepath.Join(svc.stateDir, "cache", "sources", name[0:2], name[2:4], name) +} + +// variantFilePath returns the path of the file of the variant key in svc's +// cache, as README.md gives it: /cache/variants///. +func variantFilePath(svc *testProxy, key imagerequest.VariantKey) string { + name := string(key) + + return filepath.Join(svc.stateDir, "cache", "variants", name[0:2], name[2:4], name) +} + +// setVariantLastAccessed backdates the last access time of a tracked +// variant. +func setVariantLastAccessed( + t *testing.T, db *sql.DB, key imagerequest.VariantKey, when time.Time, +) { + t.Helper() + + res, err := db.ExecContext(t.Context(), + `UPDATE variant_content SET last_accessed_at = ? WHERE cache_key = ?`, + when.UTC().Format(sqliteTimestampFormat), string(key), + ) + if err != nil { + t.Fatalf("failed to set variant last_accessed_at: %v", err) + } + + affected, err := res.RowsAffected() + if err != nil { + t.Fatalf("failed to read affected rows: %v", err) + } + + if affected != 1 { + t.Fatalf("variant %s has no accounting row (affected=%d); "+ + "stores must track variants in the database", key, affected) + } +} + +// setSourceLastAccessed backdates the last access time of a tracked +// source content blob. +func setSourceLastAccessed( + t *testing.T, db *sql.DB, key imagerequest.SourceKey, when time.Time, +) { + t.Helper() + + res, err := db.ExecContext(t.Context(), + `UPDATE source_content SET last_accessed_at = ? WHERE source_key = ?`, + when.UTC().Format(sqliteTimestampFormat), string(key), + ) + if err != nil { + t.Fatalf("failed to set source last_accessed_at: %v", err) + } + + affected, err := res.RowsAffected() + if err != nil { + t.Fatalf("failed to read affected rows: %v", err) + } + + if affected != 1 { + t.Fatalf("source %s has no accounting row (affected=%d)", key, affected) + } +} + +// countRows returns the number of rows the given query yields. +func countRows(t *testing.T, db *sql.DB, query string, args ...any) int { + t.Helper() + + var n int + + err := db.QueryRowContext(t.Context(), query, args...).Scan(&n) + if err != nil { + t.Fatalf("count query %q failed: %v", query, err) + } + + return n +} + +// setupTestDB creates an in-memory SQLite database for testing +// using the production schema. +func setupTestDB(t *testing.T) *sql.DB { + t.Helper() + + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatalf("failed to open test db: %v", err) + } + + // Use the real production schema via migrations + err = database.ApplyMigrations(context.Background(), db, nil) + if err != nil { + t.Fatalf("failed to apply migrations: %v", err) + } + + return db +} diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index e1b22a7..ab5cbcf 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -1,3 +1,10 @@ +// Package imgcache is the image cache. It keeps: +// - the source images fetched from upstream +// - the variants made from them +// - the negative cache: the source URLs whose fetch failed recently +// +// Each image is a file with a row in the database. Eviction removes the +// least recently used images to keep the cache under its size limit. package imgcache import ( @@ -26,8 +33,12 @@ const maxPendingAccessTimes = 10000 // have expired. const cleanExpiredBatchSize = 1000 -// CacheConfig holds cache configuration. -type CacheConfig struct { +// DefaultNegativeTTL is how long pixad keeps a failed fetch in the negative +// cache. +const DefaultNegativeTTL = 5 * time.Minute + +// Config holds cache configuration. +type Config struct { StateDir string NegativeTTL time.Duration @@ -48,7 +59,7 @@ type Cache struct { db *sql.DB srcContent *ContentStorage // source images by source key variants *VariantStorage // processed variants by cache key - config CacheConfig + config Config log *slog.Logger // Eviction machinery. The channels are created in NewCache so @@ -73,13 +84,10 @@ type Cache struct { pendingAccessTimesDone chan struct{} pendingAccessTimesCancel context.CancelFunc - // The counts of what the cache does, made and registered by + // The counts of cache hits and misses, made and registered by // registerCounters (see counters.go) - cacheHits prometheus.Counter - cacheMisses prometheus.Counter - upstreamFetches prometheus.Counter - upstreamFetchBytes prometheus.Counter - transforms prometheus.Counter + cacheHits prometheus.Counter + cacheMisses prometheus.Counter // reconciliationPageSize is the most rows one read of a content table // returns in the reconciliation pass. NewCache sets it to @@ -97,7 +105,7 @@ type Cache struct { // metricsRegistry. pixad passes Prometheus' default registry, the one // /metrics serves. It makes no database call. func NewCache( - db *sql.DB, config CacheConfig, metricsRegistry prometheus.Registerer, + db *sql.DB, config Config, metricsRegistry prometheus.Registerer, ) (*Cache, error) { log := config.Logger if log == nil { @@ -147,75 +155,35 @@ func NewCache( return c, nil } -// GetVariant opens a cached variant and returns a reader, its size and its +// Variant opens a cached variant and returns a reader, its size and its // content type, read from the variant file's first bytes. It opens that one -// file and makes no database call. A variant not in the cache, or any -// variant of a disabled cache, is ErrNotFound; a variant file whose first -// bytes are not those of an output format is another error. -func (c *Cache) GetVariant( +// file and makes no database call. A variant it opens has its access time +// handed to the background writer without waiting (see touchVariant). A +// variant not in the cache, or any variant of a disabled cache, is +// ErrNotFound; a variant file whose first bytes are not those of an output +// format is another error. +func (c *Cache) Variant( cacheKey imagerequest.VariantKey, ) (io.ReadCloser, int64, string, error) { if c.config.MaxBytes == 0 { return nil, 0, "", ErrNotFound } - return c.variants.LoadWithContentType(cacheKey) + reader, size, contentType, err := c.variants.LoadWithContentType(cacheKey) + if err != nil { + return nil, 0, "", err + } + + c.touchVariant(cacheKey) + + return reader, size, contentType, nil } -// StoreSource stores fetched source content: it writes the file named by -// the source's key, then the source's row. When the row cannot be written -// the file is removed and the store fails, as a file with no row is never -// served or evicted; if that removal fails too, the next reconciliation -// pass removes the file. On a disabled cache it is a no-op returning an -// empty key. -func (c *Cache) StoreSource( - ctx context.Context, - req *imagerequest.Request, - content io.Reader, -) (imagerequest.SourceKey, error) { - if c.config.MaxBytes == 0 { - return "", nil - } - - data, err := io.ReadAll(content) - if err != nil { - return "", fmt.Errorf("failed to read source content: %w", err) - } - - key := req.Source.Key() - - size, err := c.srcContent.Store(key, data) - if err != nil { - return "", fmt.Errorf("failed to store source content: %w", err) - } - - _, err = c.db.ExecContext(ctx, ` - INSERT INTO source_content (source_key, size_bytes) - VALUES (?, ?) - ON CONFLICT(source_key) DO UPDATE SET - size_bytes = excluded.size_bytes, - last_accessed_at = CURRENT_TIMESTAMP - `, string(key), size) - if err != nil { - deleteErr := c.srcContent.Delete(key) - if deleteErr != nil { - c.log.Warn("failed to remove source file whose row was not written", - "source_key", key, "error", deleteErr) - } - - return "", fmt.Errorf("failed to insert source content: %w", err) - } - - c.notifyWritePressure() - - return key, nil -} - -// StoreVariant stores a processed variant by its cache key and records +// PutVariant stores a processed variant by its cache key and records // it in the size accounting. On a disabled cache it is a no-op. The // accounting insert is best-effort (the startup reconciliation pass // adopts any variant file that misses its accounting row). -func (c *Cache) StoreVariant( +func (c *Cache) PutVariant( ctx context.Context, cacheKey imagerequest.VariantKey, content io.Reader, ) error { if c.config.MaxBytes == 0 { @@ -244,19 +212,18 @@ func (c *Cache) StoreVariant( return nil } -// LookupSource checks if we have cached source content for a request. -// Returns the source key if found, or an empty key if not; a row whose -// file is missing is not found. A source found has its access time -// handed to the background writer without waiting (see -// touchSourceContent); a disabled cache always reports no cached source. -func (c *Cache) LookupSource( - ctx context.Context, req *imagerequest.Request, -) (imagerequest.SourceKey, error) { +// Source opens the cached source of src and returns a reader and its size +// in bytes. A source whose row or file is missing is ErrNotFound, as is any +// source of a disabled cache. A source it opens has its access time handed +// to the background writer without waiting (see touchSourceContent). +func (c *Cache) Source( + ctx context.Context, src imagerequest.Source, +) (io.ReadCloser, int64, error) { if c.config.MaxBytes == 0 { - return "", nil + return nil, 0, ErrNotFound } - key := req.Source.Key() + key := src.Key() var rowExists bool @@ -264,25 +231,100 @@ func (c *Cache) LookupSource( SELECT EXISTS(SELECT 1 FROM source_content WHERE source_key = ?) `, string(key)).Scan(&rowExists) if err != nil { - return "", fmt.Errorf("failed to lookup source: %w", err) + return nil, 0, fmt.Errorf("failed to lookup source: %w", err) } if !rowExists { - return "", nil + return nil, 0, ErrNotFound } - // Verify the content file exists - if !c.srcContent.Exists(key) { - return "", nil + reader, size, err := c.srcContent.LoadWithSize(key) + if err != nil { + return nil, 0, err } c.touchSourceContent(key) + return reader, size, nil +} + +// PutSource stores fetched source content: it writes the file named by +// the source's key, then the source's row. When the row cannot be written +// the file is removed and the store fails, as a file with no row is never +// served or evicted; if that removal fails too, the next reconciliation +// pass removes the file. On a disabled cache it is a no-op returning an +// empty key. +func (c *Cache) PutSource( + ctx context.Context, + src imagerequest.Source, + content io.Reader, +) (imagerequest.SourceKey, error) { + if c.config.MaxBytes == 0 { + return "", nil + } + + data, err := io.ReadAll(content) + if err != nil { + return "", fmt.Errorf("failed to read source content: %w", err) + } + + key := src.Key() + + size, err := c.srcContent.Store(key, data) + if err != nil { + return "", fmt.Errorf("failed to store source content: %w", err) + } + + _, err = c.db.ExecContext(ctx, ` + INSERT INTO source_content (source_key, size_bytes) + VALUES (?, ?) + ON CONFLICT(source_key) DO UPDATE SET + size_bytes = excluded.size_bytes, + last_accessed_at = CURRENT_TIMESTAMP + `, string(key), size) + if err != nil { + deleteErr := c.srcContent.Delete(key) + if deleteErr != nil { + c.log.Warn("failed to remove source file whose row was not written", + "source_key", key, "error", deleteErr) + } + + return "", fmt.Errorf("failed to insert source content: %w", err) + } + + c.notifyWritePressure() + return key, nil } -// StoreNegative stores a negative cache entry for a failed fetch. -func (c *Cache) StoreNegative(ctx context.Context, req *imagerequest.Request) error { +// Failure reports whether src is in the negative cache. It only reads: a +// row past its expiry is not a hit, and the eviction goroutine deletes it +// with the other expired rows (see CleanExpired). +func (c *Cache) Failure( + ctx context.Context, src imagerequest.Source, +) (bool, error) { + var expiresAt time.Time + + err := c.db.QueryRowContext(ctx, ` + SELECT expires_at FROM negative_cache + WHERE source_host = ? AND source_path = ? AND source_query = ? + `, src.Host, src.Path, src.Query).Scan(&expiresAt) + + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + + if err != nil { + return false, fmt.Errorf("failed to check negative cache: %w", err) + } + + expired := time.Now().After(expiresAt) + + return !expired, nil +} + +// PutFailure stores a negative cache entry for src, whose fetch failed. +func (c *Cache) PutFailure(ctx context.Context, src imagerequest.Source) error { expiresAt := time.Now().UTC().Add(c.config.NegativeTTL) _, err := c.db.ExecContext(ctx, ` @@ -291,7 +333,7 @@ func (c *Cache) StoreNegative(ctx context.Context, req *imagerequest.Request) er VALUES (?, ?, ?, ?) ON CONFLICT(source_host, source_path, source_query) DO UPDATE SET expires_at = excluded.expires_at - `, req.Source.Host, req.Source.Path, req.Source.Query, expiresAt) + `, src.Host, src.Path, src.Query, expiresAt) if err != nil { return fmt.Errorf("failed to insert negative cache: %w", err) } @@ -299,18 +341,6 @@ func (c *Cache) StoreNegative(ctx context.Context, req *imagerequest.Request) er return nil } -// GetSourceContent returns a reader for cached source content by its key, -// and the content's size in bytes. -func (c *Cache) GetSourceContent( - key imagerequest.SourceKey, -) (io.ReadCloser, int64, error) { - if c.config.MaxBytes == 0 { - return nil, 0, ErrNotFound - } - - return c.srcContent.LoadWithSize(key) -} - // CleanExpired removes the negative cache rows past their expiry, one // batch per statement, until none is left. Once ctx ends it starts no // further batch and returns ctx's error. The eviction goroutine calls it @@ -334,34 +364,15 @@ func (c *Cache) CleanExpired(ctx context.Context) error { } } -// IncrementStats counts a cache hit or miss, and an upstream fetch that read -// fetchBytes bytes, as IncrementUpstreamFetch does. Like the other Increment -// methods, it only adds to the cache's Prometheus counters in memory (see -// counters.go), so it never waits for the database or fails. -func (c *Cache) IncrementStats(hit bool, fetchBytes int64) { +// IncrementStats counts a cache hit or miss. It only adds to the cache's +// Prometheus counters in memory (see counters.go), so it never waits for the +// database or fails. +func (c *Cache) IncrementStats(hit bool) { if hit { c.cacheHits.Inc() } else { c.cacheMisses.Inc() } - - c.IncrementUpstreamFetch(fetchBytes) -} - -// IncrementUpstreamFetch counts one upstream fetch that read fetchBytes bytes. -// A fetch that read no bytes is not counted. -func (c *Cache) IncrementUpstreamFetch(fetchBytes int64) { - if fetchBytes <= 0 { - return - } - - c.upstreamFetches.Inc() - c.upstreamFetchBytes.Add(float64(fetchBytes)) -} - -// IncrementTransformCount counts one image transcoded by the image processor. -func (c *Cache) IncrementTransformCount() { - c.transforms.Inc() } // touchVariant hands the access time of a variant served from the cache to @@ -388,32 +399,6 @@ func (c *Cache) touchSourceContent(key imagerequest.SourceKey) { } } -// checkNegativeCache checks if a request is in the negative cache. It only -// reads: a row past its expiry is not a hit, and the eviction goroutine -// deletes it with the other expired rows (see CleanExpired). -func (c *Cache) checkNegativeCache( - ctx context.Context, req *imagerequest.Request, -) (bool, error) { - var expiresAt time.Time - - err := c.db.QueryRowContext(ctx, ` - SELECT expires_at FROM negative_cache - WHERE source_host = ? AND source_path = ? AND source_query = ? - `, req.Source.Host, req.Source.Path, req.Source.Query).Scan(&expiresAt) - - if errors.Is(err, sql.ErrNoRows) { - return false, nil - } - - if err != nil { - return false, fmt.Errorf("failed to check negative cache: %w", err) - } - - expired := time.Now().After(expiresAt) - - return !expired, nil -} - // cleanExpiredBatch deletes up to cleanExpiredBatchSize negative cache // rows past their expiry in one statement, and returns how many it // deleted. DELETE ... LIMIT needs a SQLite build option, so the subquery diff --git a/internal/imgcache/cache_hit_internal_test.go b/internal/imgcache/cache_hit_internal_test.go index d4a3c76..068bce0 100644 --- a/internal/imgcache/cache_hit_internal_test.go +++ b/internal/imgcache/cache_hit_internal_test.go @@ -6,7 +6,6 @@ import ( "fmt" "image/color" "io" - "os" "testing" "time" @@ -15,10 +14,10 @@ import ( "sneak.berlin/go/pixa/internal/magic" ) -// TestGetVariant_ContentTypeFromFirstBytes stores an image in each output -// format as a variant and checks that GetVariant serves it whole with the +// TestVariant_ContentTypeFromFirstBytes stores an image in each output +// format as a variant and checks that Variant serves it whole with the // content type of its format, read from the file. -func TestGetVariant_ContentTypeFromFirstBytes(t *testing.T) { +func TestVariant_ContentTypeFromFirstBytes(t *testing.T) { t.Parallel() cache, _ := newEvictionTestCache(t, 1<<30) @@ -50,14 +49,14 @@ func TestGetVariant_ContentTypeFromFirstBytes(t *testing.T) { key := imagerequest.VariantKey("aabbccdd10" + string(format)) - err = cache.StoreVariant(t.Context(), key, bytes.NewReader(image)) + err = cache.PutVariant(t.Context(), key, bytes.NewReader(image)) if err != nil { - t.Fatalf("StoreVariant(%s) error = %v", format, err) + t.Fatalf("PutVariant(%s) error = %v", format, err) } - reader, size, contentType, err := cache.GetVariant(key) + reader, size, contentType, err := cache.Variant(key) if err != nil { - t.Fatalf("GetVariant(%s) error = %v", format, err) + t.Fatalf("Variant(%s) error = %v", format, err) } served, err := io.ReadAll(reader) @@ -80,11 +79,11 @@ func TestGetVariant_ContentTypeFromFirstBytes(t *testing.T) { } } -// TestGetVariant_RefusesFileOfNoOutputFormat checks that GetVariant refuses +// TestVariant_RefusesFileOfNoOutputFormat checks that Variant refuses // a variant file whose first bytes are not those of an output format, with // an error other than ErrNotFound, and that a variant with no file, or any // variant of a disabled cache, is ErrNotFound. -func TestGetVariant_RefusesFileOfNoOutputFormat(t *testing.T) { +func TestVariant_RefusesFileOfNoOutputFormat(t *testing.T) { t.Parallel() cache, _ := newEvictionTestCache(t, 1<<30) @@ -110,118 +109,30 @@ func TestGetVariant_RefusesFileOfNoOutputFormat(t *testing.T) { t.Fatalf("%s: storing the variant file: %v", tt.name, err) } - reader, _, contentType, err := cache.GetVariant(key) - t.Logf("%s: GetVariant() error = %v", tt.name, err) + reader, _, contentType, err := cache.Variant(key) + t.Logf("%s: Variant() error = %v", tt.name, err) if reader != nil || contentType != "" || !errors.Is(err, tt.wantErr) { - t.Errorf("%s: GetVariant() = reader %v, content type %q, error %v; "+ + t.Errorf("%s: Variant() = reader %v, content type %q, error %v; "+ "want no reader and %v", tt.name, reader, contentType, err, tt.wantErr) } } - reader, _, _, err := cache.GetVariant(testVariantKeyFour) + reader, _, _, err := cache.Variant(testVariantKeyFour) if reader != nil || !errors.Is(err, ErrNotFound) { - t.Errorf("GetVariant() of a variant with no file = reader %v, error %v; "+ + t.Errorf("Variant() of a variant with no file = reader %v, error %v; "+ "want no reader and %v", reader, err, ErrNotFound) } disabled, _ := newEvictionTestCache(t, 0) - reader, _, _, err = disabled.GetVariant(testVariantKeyOne) + reader, _, _, err = disabled.Variant(testVariantKeyOne) if reader != nil || !errors.Is(err, ErrNotFound) { - t.Errorf("GetVariant() on a disabled cache = reader %v, error %v; "+ + t.Errorf("Variant() on a disabled cache = reader %v, error %v; "+ "want no reader and %v", reader, err, ErrNotFound) } } -// TestService_Get_MakesAgainVariantOfNoOutputFormat replaces a cached -// variant's file with text, and checks that the next request is a miss -// answered with the variant made again, and the one after a hit. -func TestService_Get_MakesAgainVariantOfNoOutputFormat(t *testing.T) { - t.Parallel() - - svc, fixtures := SetupTestService(t) - req := photoVariant(fixtures, 85, imagerequest.FitCover) - - want, _ := getImage(t, svc, req) - - path := svc.cache.variants.keyToPath(imagerequest.CacheKey(req)) - - err := os.WriteFile(path, []byte("this is text, not an image"), StorageFilePerm) - if err != nil { - t.Fatalf("replacing the variant file: %v", err) - } - - got, status := getImage(t, svc, req) - t.Logf("after the replacement: %d bytes, %s", len(got), status) - - if status != CacheMiss || !bytes.Equal(got, want) { - t.Errorf("after the replacement: Get() = %d bytes, %s; want the %d bytes "+ - "made again, %s", len(got), status, len(want), CacheMiss) - } - - got, status = getImage(t, svc, req) - - if status != CacheHit || !bytes.Equal(got, want) { - t.Errorf("next request: Get() = %d bytes, %s; want the %d bytes, %s", - len(got), status, len(want), CacheHit) - } -} - -// TestService_Get_HitWaitsForNoDatabaseWrite holds the database's one -// connection while a cached variant is requested. The hit must be answered -// while the connection is still held, and its access time must reach the -// database once it is free. -func TestService_Get_HitWaitsForNoDatabaseWrite(t *testing.T) { - t.Parallel() - - svc, fixtures := SetupTestService(t) - req := photoVariant(fixtures, 85, imagerequest.FitCover) - key := imagerequest.CacheKey(req) - - getImage(t, svc, req) - - // Each connection to an in-memory SQLite database opens a new, empty one, - // so the goroutine that writes the access times must share the - // connection that holds the schema. - svc.cache.db.SetMaxOpenConns(1) - svc.cache.StartPendingAccessTimeWrites() - - defer func() { _ = svc.cache.StopPendingAccessTimeWrites(t.Context()) }() - - anHourAgo := time.Now().Add(-time.Hour) - setVariantLastAccessed(t, svc.cache, key, anHourAgo) - - releaseDatabase := holdDatabase(t, svc.cache) - defer releaseDatabase() - - results := startGet(t.Context(), svc, req) - - select { - case got := <-results: - if got.err != nil { - t.Fatalf("Get() of the cached variant error = %v", got.err) - } - case <-time.After(pendingAccessTimesWait): - t.Fatal("the cache hit waited for the database") - } - - releaseDatabase() - - deadline := time.Now().Add(pendingAccessTimesWait) - - for countRows(t, svc.cache, ` - SELECT COUNT(*) FROM variant_content - WHERE cache_key = ? AND last_accessed_at > ? - `, string(key), anHourAgo.UTC().Format(sqliteTimestampFormat)) == 0 { - if time.Now().After(deadline) { - t.Fatal("the hit's access time was not written") - } - - time.Sleep(10 * time.Millisecond) - } -} - // TestTouchVariantAndTouchSourceContent_DropAccessTimesPastTheLimit hands one // more access time of a variant, and one more of a source, than may wait to // be written. touchVariant and touchSourceContent must return each time diff --git a/internal/imgcache/cache_internal_test.go b/internal/imgcache/cache_internal_test.go index b37af29..0a1cf9b 100644 --- a/internal/imgcache/cache_internal_test.go +++ b/internal/imgcache/cache_internal_test.go @@ -50,7 +50,7 @@ func TestCache_CleanExpired(t *testing.T) { tmpDir := t.TempDir() db := setupTestDB(t) - cache, _ := NewCache(db, CacheConfig{ + cache, _ := NewCache(db, Config{ StateDir: tmpDir, NegativeTTL: 1 * time.Millisecond, MaxBytes: 1 << 30, @@ -103,7 +103,7 @@ func TestCache_StorageDirectoriesCreated(t *testing.T) { tmpDir := t.TempDir() db := setupTestDB(t) - _, err := NewCache(db, CacheConfig{ + _, err := NewCache(db, Config{ StateDir: tmpDir, MaxBytes: 1 << 30, }, prometheus.NewRegistry()) diff --git a/internal/imgcache/counters.go b/internal/imgcache/counters.go index 8e50780..edb14ac 100644 --- a/internal/imgcache/counters.go +++ b/internal/imgcache/counters.go @@ -19,22 +19,8 @@ func (c *Cache) registerCounters(metricsRegistry prometheus.Registerer) error { Name: "pixa_cache_misses_total", Help: "Image requests not served from the cache.", }) - c.upstreamFetches = prometheus.NewCounter(prometheus.CounterOpts{ - Name: "pixa_upstream_fetches_total", - Help: "Fetches of a source image from upstream that read any bytes.", - }) - c.upstreamFetchBytes = prometheus.NewCounter(prometheus.CounterOpts{ - Name: "pixa_upstream_fetch_bytes_total", - Help: "Bytes read from upstream by fetches of source images.", - }) - c.transforms = prometheus.NewCounter(prometheus.CounterOpts{ - Name: "pixa_transforms_total", - Help: "Images the image processor transformed.", - }) - for _, counter := range []prometheus.Counter{ - c.cacheHits, c.cacheMisses, c.upstreamFetches, c.upstreamFetchBytes, c.transforms, - } { + for _, counter := range []prometheus.Counter{c.cacheHits, c.cacheMisses} { err := metricsRegistry.Register(counter) if err != nil { return fmt.Errorf("failed to register the cache's counters: %w", err) diff --git a/internal/imgcache/counters_internal_test.go b/internal/imgcache/counters_internal_test.go index cb11aa3..0fd62f3 100644 --- a/internal/imgcache/counters_internal_test.go +++ b/internal/imgcache/counters_internal_test.go @@ -8,26 +8,6 @@ import ( "github.com/prometheus/client_golang/prometheus/testutil" ) -// counterValues holds the values of a cache's counters. -type counterValues struct { - hitCount int64 - missCount int64 - upstreamFetchCount int64 - upstreamFetchBytes int64 - transformCount int64 -} - -// readCounters reads cache's counters. -func readCounters(cache *Cache) counterValues { - return counterValues{ - hitCount: int64(testutil.ToFloat64(cache.cacheHits)), - missCount: int64(testutil.ToFloat64(cache.cacheMisses)), - upstreamFetchCount: int64(testutil.ToFloat64(cache.upstreamFetches)), - upstreamFetchBytes: int64(testutil.ToFloat64(cache.upstreamFetchBytes)), - transformCount: int64(testutil.ToFloat64(cache.transforms)), - } -} - // TestNewCache_RegistersItsCountersOnItsRegistry checks that NewCache // registers the cache's counters on the registry it is given, under the // names README.md gives them. @@ -37,7 +17,7 @@ func TestNewCache_RegistersItsCountersOnItsRegistry(t *testing.T) { registry := prometheus.NewRegistry() _, err := NewCache(evictionTestDB(t), - CacheConfig{StateDir: t.TempDir(), MaxBytes: 1 << 30}, registry) + Config{StateDir: t.TempDir(), MaxBytes: 1 << 30}, registry) if err != nil { t.Fatalf("NewCache() error = %v", err) } @@ -45,9 +25,6 @@ func TestNewCache_RegistersItsCountersOnItsRegistry(t *testing.T) { names := []string{ "pixa_cache_hits_total", "pixa_cache_misses_total", - "pixa_upstream_fetches_total", - "pixa_upstream_fetch_bytes_total", - "pixa_transforms_total", } count, err := testutil.GatherAndCount(registry, names...) @@ -70,13 +47,13 @@ func TestNewCache_FailsWhenItsCountersCannotBeRegistered(t *testing.T) { registry := prometheus.NewRegistry() _, err := NewCache(evictionTestDB(t), - CacheConfig{StateDir: t.TempDir(), MaxBytes: 1 << 30}, registry) + Config{StateDir: t.TempDir(), MaxBytes: 1 << 30}, registry) if err != nil { t.Fatalf("NewCache() of the first cache error = %v", err) } _, err = NewCache(evictionTestDB(t), - CacheConfig{StateDir: t.TempDir(), MaxBytes: 1 << 30}, registry) + Config{StateDir: t.TempDir(), MaxBytes: 1 << 30}, registry) t.Logf("NewCache() of the second cache error = %v", err) var alreadyRegistered prometheus.AlreadyRegisteredError diff --git a/internal/imgcache/eviction.go b/internal/imgcache/eviction.go index 2b1feb0..8b576ad 100644 --- a/internal/imgcache/eviction.go +++ b/internal/imgcache/eviction.go @@ -317,7 +317,7 @@ func (c *Cache) evictVariant( // evictSourceBlob removes one source: its row first, then its file, as // evictVariant does for a variant. It holds no lock: a store of the same // source in between can leave the source's row with no file, which -// LookupSource treats as a miss. +// Source treats as a miss. func (c *Cache) evictSourceBlob(ctx context.Context, key imagerequest.SourceKey) error { _, err := c.db.ExecContext(ctx, `DELETE FROM source_content WHERE source_key = ?`, string(key)) @@ -479,7 +479,7 @@ func (c *Cache) runCleanExpired(ctx context.Context) { // then on the first periodic eviction tick that comes at least ten times // as long after the last pass ended as that pass took, off the request // hot path: it adopts variant files that predate the accounting table -// (or whose accounting insert failed, e.g. StoreVariant's best-effort +// (or whose accounting insert failed, e.g. PutVariant's best-effort // insert under transient DB contention), drops variant accounting rows // whose files are missing, removes source blob files the database does // not know, removes source and variant files that are empty or not the diff --git a/internal/imgcache/eviction_internal_test.go b/internal/imgcache/eviction_internal_test.go index fec51d9..898d990 100644 --- a/internal/imgcache/eviction_internal_test.go +++ b/internal/imgcache/eviction_internal_test.go @@ -67,7 +67,7 @@ func newEvictionTestCache(t *testing.T, maxBytes int64) (*Cache, string) { db := evictionTestDB(t) // maxBytes zero turns the disk cache off, as cache_max_bytes: 0 does. - cache, err := NewCache(db, CacheConfig{ + cache, err := NewCache(db, Config{ StateDir: tmpDir, NegativeTTL: 5 * time.Minute, MaxBytes: maxBytes, @@ -98,11 +98,11 @@ func storeEvictionTestSource( }, } - key, err := cache.StoreSource( - context.Background(), req, bytes.NewReader(content), + key, err := cache.PutSource( + context.Background(), req.Source, bytes.NewReader(content), ) if err != nil { - t.Fatalf("StoreSource(%s%s) failed: %v", host, path, err) + t.Fatalf("PutSource(%s%s) failed: %v", host, path, err) } return key @@ -115,9 +115,9 @@ func storeEvictionTestVariant( ) { t.Helper() - err := cache.StoreVariant(t.Context(), key, bytes.NewReader(content)) + err := cache.PutVariant(t.Context(), key, bytes.NewReader(content)) if err != nil { - t.Fatalf("StoreVariant(%s) failed: %v", key, err) + t.Fatalf("PutVariant(%s) failed: %v", key, err) } } @@ -423,20 +423,20 @@ func assertDisabledCacheWritesAreNoOps( ctx := t.Context() - err := cache.StoreVariant( + err := cache.PutVariant( ctx, imagerequest.CacheKey(req), bytes.NewReader([]byte("data")), ) if err != nil { - t.Fatalf("StoreVariant on disabled cache must be a no-op, got error: %v", err) + t.Fatalf("PutVariant on disabled cache must be a no-op, got error: %v", err) } - key, err := cache.StoreSource(ctx, req, bytes.NewReader([]byte("data"))) + key, err := cache.PutSource(ctx, req.Source, bytes.NewReader([]byte("data"))) if err != nil { - t.Fatalf("StoreSource on disabled cache must be a no-op, got error: %v", err) + t.Fatalf("PutSource on disabled cache must be a no-op, got error: %v", err) } if key != "" { - t.Errorf("StoreSource on disabled cache returned key %q, want empty", key) + t.Errorf("PutSource on disabled cache returned key %q, want empty", key) } } @@ -449,13 +449,13 @@ func assertDisabledCacheReadsAlwaysMiss( ctx := t.Context() - key, err := cache.LookupSource(ctx, req) - if err != nil { - t.Fatalf("LookupSource on disabled cache failed: %v", err) + reader, _, err := cache.Source(ctx, req.Source) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("Source on disabled cache error = %v, want %v", err, ErrNotFound) } - if key != "" { - t.Errorf("LookupSource on disabled cache = %q, want empty", key) + if reader != nil { + t.Errorf("Source on disabled cache = reader %v, want none", reader) } } @@ -544,7 +544,7 @@ func TestEvictionRunsUnderWritePressure(t *testing.T) { } // TestEvictionRunsOnPeriodicSchedule writes three variant files straight -// to disk, bypassing StoreVariant, so they have no accounting rows and no +// to disk, bypassing PutVariant, so they have no accounting rows and no // write-pressure notification fires. Only a periodic reconciliation pass // can then adopt them, and only the eviction pass that follows it can // evict them. @@ -697,7 +697,7 @@ func TestStartEvictionReconcilesAccountingWithDisk(t *testing.T) { // TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup proves // reconciliation is not a one-shot startup-only pass: it must also run // on the periodic ticker, so a variant file that lands on disk with no -// accounting row well after startup (e.g. because StoreVariant's +// accounting row well after startup (e.g. because PutVariant's // best-effort accounting insert failed under transient contention, or // any other cause of an untracked file appearing during steady-state // operation) is still adopted into accounting eventually, rather than @@ -733,7 +733,7 @@ func TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup(t *testing.T) { // Simulate a variant whose accounting insert failed after the // process was already running and serving requests: the content - // file is written directly, bypassing StoreVariant's (and thus its + // file is written directly, bypassing PutVariant's (and thus its // accounting insert) entirely, exactly as would happen if that // insert had failed and only the file write had succeeded. untracked := bytes.Repeat([]byte{0x41}, 900) diff --git a/internal/imgcache/imgcache.go b/internal/imgcache/imgcache.go deleted file mode 100644 index 83dd0d8..0000000 --- a/internal/imgcache/imgcache.go +++ /dev/null @@ -1,29 +0,0 @@ -// Package imgcache provides types for the image caching proxy. -package imgcache - -import "io" - -// ImageResponse represents a processed image ready to serve -type ImageResponse struct { - // Content is the image data reader - Content io.ReadCloser - // ContentLength is the size in bytes (-1 if unknown) - ContentLength int64 - // ContentType is the MIME type of the response - ContentType string - // ETag is the entity tag for caching - ETag string - // CacheStatus indicates HIT or MISS - CacheStatus CacheStatus - // FetchedBytes is the number of bytes fetched from upstream (0 if cache hit) - FetchedBytes int64 -} - -// CacheStatus indicates whether the response was served from cache. -type CacheStatus string - -// Cache status values for response headers. -const ( - CacheHit CacheStatus = "HIT" - CacheMiss CacheStatus = "MISS" -) diff --git a/internal/imgcache/module.go b/internal/imgcache/module.go deleted file mode 100644 index 81d329a..0000000 --- a/internal/imgcache/module.go +++ /dev/null @@ -1,8 +0,0 @@ -package imgcache - -import "time" - -// CacheConfig defaults. -const ( - DefaultNegativeTTL = 5 * time.Minute -) diff --git a/internal/imgcache/negative_cache_internal_test.go b/internal/imgcache/negative_cache_internal_test.go index 709f090..0c980c0 100644 --- a/internal/imgcache/negative_cache_internal_test.go +++ b/internal/imgcache/negative_cache_internal_test.go @@ -17,7 +17,7 @@ func TestNegativeCache_StoreAndCheck(t *testing.T) { db := setupTestDB(t) dir := t.TempDir() - cache, err := NewCache(db, CacheConfig{ + cache, err := NewCache(db, Config{ StateDir: dir, NegativeTTL: 5 * time.Minute, MaxBytes: 1 << 30, @@ -35,7 +35,7 @@ func TestNegativeCache_StoreAndCheck(t *testing.T) { } // Initially should not be in negative cache - hit, err := cache.checkNegativeCache(ctx, req) + hit, err := cache.Failure(ctx, req.Source) if err != nil { t.Fatal(err) } @@ -45,13 +45,13 @@ func TestNegativeCache_StoreAndCheck(t *testing.T) { } // Store a negative entry - err = cache.StoreNegative(ctx, req) + err = cache.PutFailure(ctx, req.Source) if err != nil { t.Fatal(err) } // Now should be in negative cache - hit, err = cache.checkNegativeCache(ctx, req) + hit, err = cache.Failure(ctx, req.Source) if err != nil { t.Fatal(err) } @@ -67,7 +67,7 @@ func TestNegativeCache_Expired(t *testing.T) { db := setupTestDB(t) dir := t.TempDir() - cache, err := NewCache(db, CacheConfig{ + cache, err := NewCache(db, Config{ StateDir: dir, NegativeTTL: 1 * time.Millisecond, // very short TTL MaxBytes: 1 << 30, @@ -85,7 +85,7 @@ func TestNegativeCache_Expired(t *testing.T) { } // Store a negative entry with very short TTL - err = cache.StoreNegative(ctx, req) + err = cache.PutFailure(ctx, req.Source) if err != nil { t.Fatal(err) } @@ -94,7 +94,7 @@ func TestNegativeCache_Expired(t *testing.T) { time.Sleep(10 * time.Millisecond) // Should no longer be in negative cache - hit, err := cache.checkNegativeCache(ctx, req) + hit, err := cache.Failure(ctx, req.Source) if err != nil { t.Fatal(err) } @@ -104,71 +104,6 @@ func TestNegativeCache_Expired(t *testing.T) { } } -// TestService_Get_ServesCachedVariantOfNegativeCachedURL caches a variant of -// the test photo, then keeps a failed fetch of the photo's URL in the negative -// cache, as a failed fetch for another variant does once the source has been -// evicted. The cached variant is still served, and a variant not in the cache -// is made from the cached source. Once the source is evicted, a variant not in -// the cache needs a fetch and is refused. -func TestService_Get_ServesCachedVariantOfNegativeCachedURL(t *testing.T) { - t.Parallel() - - svc, fixtures := SetupTestService(t) - - resp, err := svc.Get(t.Context(), photoVariant(fixtures, 85, imagerequest.FitCover)) - if err != nil { - t.Fatalf("Get() first request error = %v", err) - } - - _ = resp.Content.Close() - - err = svc.cache.StoreNegative(t.Context(), - photoVariant(fixtures, 85, imagerequest.FitCover)) - if err != nil { - t.Fatalf("StoreNegative() error = %v", err) - } - - resp, err = svc.Get(t.Context(), photoVariant(fixtures, 85, imagerequest.FitCover)) - if err != nil { - t.Fatalf("Get() of the cached variant error = %v", err) - } - - _ = resp.Content.Close() - - if resp.CacheStatus != CacheHit { - t.Errorf("cached variant CacheStatus = %q, want %q", - resp.CacheStatus, CacheHit) - } - - resp, err = svc.Get(t.Context(), photoVariant(fixtures, 40, imagerequest.FitCover)) - t.Logf("Get() of a variant made from the cached source: error = %v", err) - - if err != nil { - t.Fatalf("Get() of a variant made from the cached source error = %v", err) - } - - _ = resp.Content.Close() - - sourceKey, err := svc.cache.LookupSource(t.Context(), - photoVariant(fixtures, 85, imagerequest.FitCover)) - if err != nil || sourceKey == "" { - t.Fatalf("LookupSource() = %q, %v, want the cached source", sourceKey, err) - } - - err = svc.cache.evictSourceBlob(t.Context(), sourceKey) - if err != nil { - t.Fatalf("evictSourceBlob() error = %v", err) - } - - _, err = svc.Get(t.Context(), photoVariant(fixtures, 60, imagerequest.FitCover)) - t.Logf("Get() of a variant whose source was evicted: error = %v", err) - - if !errors.Is(err, ErrNegativeCached) { - t.Errorf("Get() of a variant whose source was evicted error = %v, want %v", - err, ErrNegativeCached) - } -} - // TestEvictionLoopRemovesExpiredNegativeCacheRows stores a failed fetch // that expires a second later and starts the eviction loop with a short // interval: a later tick must remove its row although its URL is never @@ -190,7 +125,7 @@ func TestEvictionLoopRemovesExpiredNegativeCacheRows(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - cache, err := NewCache(evictionTestDB(t), CacheConfig{ + cache, err := NewCache(evictionTestDB(t), Config{ StateDir: t.TempDir(), NegativeTTL: time.Second, MaxBytes: tt.maxBytes, @@ -203,9 +138,9 @@ func TestEvictionLoopRemovesExpiredNegativeCacheRows(t *testing.T) { Source: imagerequest.Source{Host: testHostExample, Path: "/gone.jpg"}, } - err = cache.StoreNegative(t.Context(), expired) + err = cache.PutFailure(t.Context(), expired.Source) if err != nil { - t.Fatalf("StoreNegative failed: %v", err) + t.Fatalf("PutFailure failed: %v", err) } const notExpiredPath = "/later.jpg" @@ -269,7 +204,7 @@ func TestCleanExpiredDeletesInBatches(t *testing.T) { t.Parallel() cache, err := NewCache(evictionTestDB(t), - CacheConfig{StateDir: t.TempDir(), MaxBytes: 1 << 30}, + Config{StateDir: t.TempDir(), MaxBytes: 1 << 30}, prometheus.NewRegistry()) if err != nil { t.Fatalf("failed to create cache: %v", err) @@ -325,7 +260,7 @@ func TestCleanExpiredRemovesMoreThanOneBatch(t *testing.T) { t.Parallel() cache, err := NewCache(evictionTestDB(t), - CacheConfig{StateDir: t.TempDir(), MaxBytes: 1 << 30}, + Config{StateDir: t.TempDir(), MaxBytes: 1 << 30}, prometheus.NewRegistry()) if err != nil { t.Fatalf("failed to create cache: %v", err) @@ -357,7 +292,7 @@ func TestCleanExpiredStopsOnceContextEnds(t *testing.T) { t.Parallel() cache, err := NewCache(evictionTestDB(t), - CacheConfig{StateDir: t.TempDir(), MaxBytes: 1 << 30}, + Config{StateDir: t.TempDir(), MaxBytes: 1 << 30}, prometheus.NewRegistry()) if err != nil { t.Fatalf("failed to create cache: %v", err) @@ -410,15 +345,3 @@ func negativeCacheTableRowCount(tb testing.TB, cache *Cache) int { return count } - -func TestService_Get_ReturnsErrorForNegativeCachedURL(t *testing.T) { - t.Parallel() - - // This test verifies that Service.Get() checks the negative cache - // We can't easily test the full pipeline without vips, but we can - // verify the error type - err := ErrNegativeCached - if !errors.Is(err, ErrNegativeCached) { - t.Error("ErrNegativeCached should be identifiable with errors.Is") - } -} diff --git a/internal/imgcache/partial_cache_files_internal_test.go b/internal/imgcache/partial_cache_files_internal_test.go index 2179396..b89b720 100644 --- a/internal/imgcache/partial_cache_files_internal_test.go +++ b/internal/imgcache/partial_cache_files_internal_test.go @@ -12,67 +12,6 @@ import ( "sneak.berlin/go/pixa/internal/imagerequest" ) -// getImage calls Get for req and returns the image it answered with and -// its cache status. -func getImage( - t *testing.T, svc *Service, req *imagerequest.Request, -) ([]byte, CacheStatus) { - t.Helper() - - resp, err := svc.Get(t.Context(), req) - if err != nil { - t.Fatalf("Get() of quality %d error = %v", req.Transform.Quality, err) - } - - defer func() { _ = resp.Content.Close() }() - - body, err := io.ReadAll(resp.Content) - if err != nil { - t.Fatalf("reading the image of quality %d: %v", req.Transform.Quality, err) - } - - return body, resp.CacheStatus -} - -// fileSize returns the size of the file at path. -func fileSize(t *testing.T, path string) int64 { - t.Helper() - - info, err := os.Stat(path) - if err != nil { - t.Fatalf("stat %s: %v", path, err) - } - - return info.Size() -} - -// truncateFile cuts the file at path down to size bytes. -func truncateFile(t *testing.T, path string, size int64) { - t.Helper() - - t.Logf("cutting %s from %d to %d bytes", - filepath.Base(path), fileSize(t, path), size) - - err := os.Truncate(path, size) - if err != nil { - t.Fatalf("truncating %s: %v", path, err) - } -} - -// cachedSourcePath returns the path of the cached source file req uses. -func cachedSourcePath( - t *testing.T, svc *Service, req *imagerequest.Request, -) string { - t.Helper() - - sourceKey, err := svc.cache.LookupSource(t.Context(), req) - if err != nil || sourceKey == "" { - t.Fatalf("LookupSource() = %q, %v; want the cached source", sourceKey, err) - } - - return svc.cache.srcContent.keyToPath(sourceKey) -} - // reconcileAsAtStartup runs the cache's reconciliation pass, as pixa does // when it starts, and returns what the pass logged. func reconcileAsAtStartup(t *testing.T, cache *Cache) string { @@ -107,127 +46,65 @@ func assertWarningNames(t *testing.T, logs, path string) { t.Errorf("reconciliation logged no warning naming %s", path) } -// TestService_Get_FetchesAgainForEmptyCachedSource empties the cached source -// file, as a crash or a restored snapshot could, and runs the reconciliation -// pass, which must remove it with a warning naming it. It then asks for two -// new sizes of the source: the first must fetch it again and cache it whole, -// so that the second is made from the cache without a third fetch. -func TestService_Get_FetchesAgainForEmptyCachedSource(t *testing.T) { +// TestReconciliationRemovesDamagedFiles empties a cached source file and a +// cached variant file, or cuts them to half their size, as a crash or a +// restored snapshot could. It checks that the reconciliation pass removes +// each file and its row, with a warning naming the file. +func TestReconciliationRemovesDamagedFiles(t *testing.T) { t.Parallel() - svc, fixtures := SetupTestService(t) + content := []byte("the whole cached image") - getImage(t, svc, photoVariant(fixtures, 85, imagerequest.FitCover)) + for name, size := range map[string]int64{ + "empty": 0, + "partial": int64(len(content) / 2), + } { + t.Run(name, func(t *testing.T) { + t.Parallel() - path := cachedSourcePath(t, svc, photoVariant(fixtures, 85, imagerequest.FitCover)) - wholeSize := fileSize(t, path) + cache, _ := newEvictionTestCache(t, 1<<30) - truncateFile(t, path, 0) - assertWarningNames(t, reconcileAsAtStartup(t, svc.cache), path) + sourceKey := storeEvictionTestSource(t, cache, testHostCDN, testPathCat, content) + storeEvictionTestVariant(t, cache, testVariantKeyOne, content) - getImage(t, svc, photoVariant(fixtures, 40, imagerequest.FitCover)) - getImage(t, svc, photoVariant(fixtures, 30, imagerequest.FitCover)) + sourcePath := cache.srcContent.keyToPath(sourceKey) + variantPath := cache.variants.keyToPath(testVariantKeyOne) - fetches := readCounters(svc.cache).upstreamFetchCount - t.Logf("upstream fetches = %d", fetches) + for _, path := range []string{sourcePath, variantPath} { + err := os.Truncate(path, size) + if err != nil { + t.Fatalf("cutting %s to %d bytes: %v", path, size, err) + } + } - if fetches != 2 { - t.Errorf("upstream fetches = %d, want 2: the empty cached source "+ - "was kept and fetched again on every new size", fetches) + logs := reconcileAsAtStartup(t, cache) + + assertWarningNames(t, logs, sourcePath) + assertWarningNames(t, logs, variantPath) + + if cache.srcContent.Exists(sourceKey) { + t.Error("the damaged source file is still on disk") + } + + if cache.variants.Exists(testVariantKeyOne) { + t.Error("the damaged variant file is still on disk") + } + + rows := countRows(t, cache, + `SELECT COUNT(*) FROM source_content WHERE source_key = ?`, + string(sourceKey)) + if rows != 0 { + t.Errorf("the damaged source file kept %d rows, want 0", rows) + } + + rows = countRows(t, cache, + `SELECT COUNT(*) FROM variant_content WHERE cache_key = ?`, + string(testVariantKeyOne)) + if rows != 0 { + t.Errorf("the damaged variant file kept %d rows, want 0", rows) + } + }) } - - size := fileSize(t, path) - if size != wholeSize { - t.Errorf("cached source is %d bytes, want %d", size, wholeSize) - } -} - -// TestService_Get_FetchesAgainForPartialCachedSource cuts the cached source -// file to half its size, as a crash or a restored snapshot could, and runs -// the reconciliation pass, which must remove it with a warning naming it. -// It then asks for two new sizes of the source: the first must fetch it -// again rather than fail to decode the partial file, and the second is -// made from the cache. -func TestService_Get_FetchesAgainForPartialCachedSource(t *testing.T) { - t.Parallel() - - svc, fixtures := SetupTestService(t) - - getImage(t, svc, photoVariant(fixtures, 85, imagerequest.FitCover)) - - path := cachedSourcePath(t, svc, photoVariant(fixtures, 85, imagerequest.FitCover)) - wholeSize := fileSize(t, path) - - truncateFile(t, path, wholeSize/2) - assertWarningNames(t, reconcileAsAtStartup(t, svc.cache), path) - - getImage(t, svc, photoVariant(fixtures, 40, imagerequest.FitCover)) - getImage(t, svc, photoVariant(fixtures, 30, imagerequest.FitCover)) - - fetches := readCounters(svc.cache).upstreamFetchCount - t.Logf("upstream fetches = %d", fetches) - - if fetches != 2 { - t.Errorf("upstream fetches = %d, want 2", fetches) - } - - size := fileSize(t, path) - if size != wholeSize { - t.Errorf("cached source is %d bytes, want %d", size, wholeSize) - } -} - -// assertConvertedAgain caches a variant of the photo, cuts its file to size -// bytes and runs the reconciliation pass, which must remove it with a -// warning naming it. It then checks that Get converts the image again -// instead of serving the file, and that the next Get serves the whole -// variant from the cache. -func assertConvertedAgain(t *testing.T, size int64) { - t.Helper() - - svc, fixtures := SetupTestService(t) - req := photoVariant(fixtures, 85, imagerequest.FitCover) - - want, _ := getImage(t, svc, req) - - path := svc.cache.variants.keyToPath(imagerequest.CacheKey(req)) - - truncateFile(t, path, size) - assertWarningNames(t, reconcileAsAtStartup(t, svc.cache), path) - - got, status := getImage(t, svc, req) - t.Logf("after the cut: %d bytes, %s", len(got), status) - - if status != CacheMiss || !bytes.Equal(got, want) { - t.Errorf("after the cut: Get() = %d bytes, %s; want the %d bytes "+ - "converted again, %s", len(got), status, len(want), CacheMiss) - } - - got, status = getImage(t, svc, req) - t.Logf("next request: %d bytes, %s", len(got), status) - - if status != CacheHit || !bytes.Equal(got, want) { - t.Errorf("next request: Get() = %d bytes, %s; want the %d bytes, %s", - len(got), status, len(want), CacheHit) - } -} - -// TestService_Get_ConvertsAgainForEmptyCachedVariant empties a cached -// variant file, as a crash or a restored snapshot could, and checks that it -// is converted again rather than served empty. -func TestService_Get_ConvertsAgainForEmptyCachedVariant(t *testing.T) { - t.Parallel() - - assertConvertedAgain(t, 0) -} - -// TestService_Get_ConvertsAgainForPartialCachedVariant cuts a cached -// variant file to ten bytes, as a crash or a restored snapshot could, and -// checks that it is converted again rather than served cut short. -func TestService_Get_ConvertsAgainForPartialCachedVariant(t *testing.T) { - t.Parallel() - - assertConvertedAgain(t, 10) } // TestReconciliationRemovesEmptyVariantFileWithoutRow checks that the diff --git a/internal/imgcache/pendingaccesstimes_internal_test.go b/internal/imgcache/pendingaccesstimes_internal_test.go index aa8a100..b21ca78 100644 --- a/internal/imgcache/pendingaccesstimes_internal_test.go +++ b/internal/imgcache/pendingaccesstimes_internal_test.go @@ -5,6 +5,7 @@ import ( "context" "database/sql" "errors" + "io" "log/slog" "strings" "testing" @@ -15,8 +16,7 @@ import ( // pendingAccessTimesWait is how long a test waits for the database's // connection, for an access time to reach the database, for a write to start -// waiting for the connection, for a request to return past its deadline, or -// for StopPendingAccessTimeWrites to return. +// waiting for the connection, or for StopPendingAccessTimeWrites to return. const pendingAccessTimesWait = 5 * time.Second // otherWritesWait is how long a test gives other writes, once one write waits @@ -56,52 +56,6 @@ func waitForConnectionWaits(t *testing.T, db *sql.DB, waits int64) { } } -// TestService_Get_ReturnsByItsDeadlineWhileTheDatabaseIsBusy holds the -// database's one connection while a request whose fetch is held reaches its -// deadline. The request must return with the deadline's error while the -// connection is still held, as it waits for no database write past its -// deadline. -func TestService_Get_ReturnsByItsDeadlineWhileTheDatabaseIsBusy(t *testing.T) { - t.Parallel() - - svc, fixtures, fetcher := setupHeldFetchService(t) - - svc.cache.StartPendingAccessTimeWrites() - defer func() { _ = svc.cache.StopPendingAccessTimeWrites(t.Context()) }() - - // Room for the request to reach its fetch on a busy host - const timeout = 2 * time.Second - - ctx, cancel := context.WithTimeout(t.Context(), timeout) - defer cancel() - - deadline, _ := ctx.Deadline() - - results := startGet(ctx, svc, photoVariant(fixtures, 85, imagerequest.FitCover)) - - // The request has made its database reads by the time it fetches. - select { - case <-fetcher.started: - case <-time.After(timeout): - t.Fatal("request did not reach its fetch by its deadline") - } - - releaseDatabase := holdDatabase(t, svc.cache) - defer releaseDatabase() - - select { - case got := <-results: - t.Logf("Get() returned %v after its deadline, error = %v", - time.Since(deadline), got.err) - - if !errors.Is(got.err, context.DeadlineExceeded) { - t.Errorf("Get() error = %v, want %v", got.err, context.DeadlineExceeded) - } - case <-time.After(time.Until(deadline) + pendingAccessTimesWait): - t.Fatal("request did not return while the database was busy") - } -} - // TestStopPendingAccessTimeWrites_LetsTheWriteUnderWayFinish holds the // database's one connection while the goroutine that writes the pending // access times waits for it, hands over another access time, then stops that @@ -242,11 +196,11 @@ func TestStopPendingAccessTimeWrites_ReturnsWhenItsContextEnds(t *testing.T) { } } -// TestLookupSource_HandsAccessTimeToTheBackgroundWriter finds a cached source -// whose access time is an hour old. LookupSource must not write the access +// TestSource_HandsAccessTimeToTheBackgroundWriter finds a cached source +// whose access time is an hour old. Source must not write the access // time itself but hand it over to be written, and StopPendingAccessTimeWrites // must then write it. -func TestLookupSource_HandsAccessTimeToTheBackgroundWriter(t *testing.T) { +func TestSource_HandsAccessTimeToTheBackgroundWriter(t *testing.T) { t.Parallel() cache, _ := newEvictionTestCache(t, 1<<30) @@ -261,15 +215,21 @@ func TestLookupSource_HandsAccessTimeToTheBackgroundWriter(t *testing.T) { ` anHourAgoInDatabase := anHourAgo.UTC().Format(sqliteTimestampFormat) - found, err := cache.LookupSource(t.Context(), &imagerequest.Request{ - Source: imagerequest.Source{Host: "example.com", Path: "/photo.jpg"}, - }) - if err != nil || found != key { - t.Fatalf("LookupSource() = %q, %v; want %q", found, err, key) + reader, _, err := cache.Source(t.Context(), + imagerequest.Source{Host: "example.com", Path: "/photo.jpg"}) + if err != nil { + t.Fatalf("Source() error = %v; want the source of %q", err, key) + } + + found, err := io.ReadAll(reader) + _ = reader.Close() + + if err != nil || string(found) != "source bytes" { + t.Fatalf("Source() reads %q, %v; want the source of %q", found, err, key) } if countRows(t, cache, touchedQuery, string(key), anHourAgoInDatabase) != 0 { - t.Error("LookupSource wrote the source's access time itself") + t.Error("Source wrote the source's access time itself") } if waiting := len(cache.pendingSourceAccessTimes); waiting != 1 { diff --git a/internal/imgcache/source_content_internal_test.go b/internal/imgcache/source_content_internal_test.go index 595b7c6..3d12835 100644 --- a/internal/imgcache/source_content_internal_test.go +++ b/internal/imgcache/source_content_internal_test.go @@ -10,11 +10,11 @@ import ( "sneak.berlin/go/pixa/internal/imagerequest" ) -// TestStoreSource_StoresIdenticalBytesOfTwoURLsTwice stores the same bytes +// TestPutSource_StoresIdenticalBytesOfTwoURLsTwice stores the same bytes // for two source URLs and checks that each URL has a source_content row of // its own and a file of its own, named by its source key under // cache/sources///. -func TestStoreSource_StoresIdenticalBytesOfTwoURLsTwice(t *testing.T) { +func TestPutSource_StoresIdenticalBytesOfTwoURLsTwice(t *testing.T) { t.Parallel() cache, stateDir := newEvictionTestCache(t, 1<<30) @@ -84,18 +84,12 @@ func TestEvictSourceBlob_LeavesSourceWithIdenticalBytesServed(t *testing.T) { t.Errorf("the evicted source still has %d source_content rows", n) } - kept, err := cache.LookupSource(t.Context(), &imagerequest.Request{ - Source: imagerequest.Source{Host: testHostCDN, Path: "/kept.jpg"}, - }) - t.Logf("LookupSource() of the kept source = %q, error %v", kept, err) + reader, _, err := cache.Source(t.Context(), + imagerequest.Source{Host: testHostCDN, Path: "/kept.jpg"}) + t.Logf("Source() of the kept source: error %v", err) - if err != nil || kept == "" { - t.Fatalf("LookupSource() of the kept source = %q, %v; want it cached", kept, err) - } - - reader, _, err := cache.GetSourceContent(kept) if err != nil { - t.Fatalf("GetSourceContent() of the kept source error = %v", err) + t.Fatalf("Source() of the kept source error = %v; want it cached", err) } defer func() { _ = reader.Close() }() @@ -111,10 +105,10 @@ func TestEvictSourceBlob_LeavesSourceWithIdenticalBytesServed(t *testing.T) { } } -// TestStoreSource_FailedRowWriteLeavesNoFile drops source_content, so that -// StoreSource cannot write the source's row, and checks that the store fails +// TestPutSource_FailedRowWriteLeavesNoFile drops source_content, so that +// PutSource cannot write the source's row, and checks that the store fails // and leaves no file under the source's key. -func TestStoreSource_FailedRowWriteLeavesNoFile(t *testing.T) { +func TestPutSource_FailedRowWriteLeavesNoFile(t *testing.T) { t.Parallel() cache, _ := newEvictionTestCache(t, 1<<30) @@ -127,12 +121,12 @@ func TestStoreSource_FailedRowWriteLeavesNoFile(t *testing.T) { req := &imagerequest.Request{ Source: imagerequest.Source{Host: testHostCDN, Path: testPathCat}, } - key, err := cache.StoreSource(t.Context(), req, + key, err := cache.PutSource(t.Context(), req.Source, bytes.NewReader([]byte("source bytes"))) - t.Logf("StoreSource() = %q, error = %v", key, err) + t.Logf("PutSource() = %q, error = %v", key, err) if err == nil { - t.Error("StoreSource() error = nil, want the failed row write") + t.Error("PutSource() error = nil, want the failed row write") } if cache.srcContent.Exists(req.Source.Key()) { diff --git a/internal/imgcache/testutil_internal_test.go b/internal/imgcache/testutil_internal_test.go index ec44fb3..8d2ebf7 100644 --- a/internal/imgcache/testutil_internal_test.go +++ b/internal/imgcache/testutil_internal_test.go @@ -2,98 +2,19 @@ package imgcache import ( "bytes" - "context" - "database/sql" "image" "image/color" - "image/gif" "image/jpeg" - "image/png" - "io/fs" "testing" - "testing/fstest" - "time" - - "github.com/prometheus/client_golang/prometheus" - "sneak.berlin/go/pixa/internal/database" - "sneak.berlin/go/pixa/internal/httpfetcher" - "sneak.berlin/go/pixa/internal/imageprocessor" ) // Shared test data literals, extracted as constants for goconst. const ( - testHostCDN = "cdn.example.com" - testHostExample = "example.com" - testPathCat = "/photos/cat.jpg" - testContentTypeJPEG = "image/jpeg" + testHostCDN = "cdn.example.com" + testHostExample = "example.com" + testPathCat = "/photos/cat.jpg" ) -// TestFixtures contains paths to test files in the mock filesystem. -type TestFixtures struct { - // Valid image files - GoodHostJPEG string // valid JPEG on GoodHost - GoodHostPNG string // valid PNG on GoodHost - GoodHostGIF string // valid GIF on GoodHost - OtherHostJPEG string // valid JPEG on OtherHost - OtherHostPNG string // valid PNG on OtherHost - - // Invalid/edge case files - InvalidFile string // file with wrong magic bytes - EmptyFile string // zero-byte file - TextFile string // text file masquerading as image - - // Hostnames - GoodHost string // a host the mock fetcher serves - OtherHost string // a second host the mock fetcher serves -} - -// DefaultFixtures returns the standard test fixture paths. -func DefaultFixtures() *TestFixtures { - return &TestFixtures{ - GoodHostJPEG: "goodhost.example.com/images/photo.jpg", - GoodHostPNG: "goodhost.example.com/images/logo.png", - GoodHostGIF: "goodhost.example.com/images/animation.gif", - OtherHostJPEG: "otherhost.example.com/uploads/image.jpg", - OtherHostPNG: "otherhost.example.com/uploads/icon.png", - InvalidFile: "goodhost.example.com/images/fake.jpg", - EmptyFile: "goodhost.example.com/images/empty.jpg", - TextFile: "goodhost.example.com/images/text.png", - GoodHost: "goodhost.example.com", - OtherHost: "otherhost.example.com", - } -} - -// NewTestFS creates a mock filesystem with test images. -func NewTestFS(t *testing.T) (fs.FS, *TestFixtures) { - t.Helper() - - fixtures := DefaultFixtures() - - // Generate test images - jpegData := generateTestJPEG(t, 100, 100, color.RGBA{255, 0, 0, 255}) - pngData := generateTestPNG(t, 100, 100, color.RGBA{0, 255, 0, 255}) - gifData := generateTestGIF(t, 100, 100, color.RGBA{0, 0, 255, 255}) - - // Create the mock filesystem - mockFS := fstest.MapFS{ - // Good host files - fixtures.GoodHostJPEG: &fstest.MapFile{Data: jpegData}, - fixtures.GoodHostPNG: &fstest.MapFile{Data: pngData}, - fixtures.GoodHostGIF: &fstest.MapFile{Data: gifData}, - - // Other host files - fixtures.OtherHostJPEG: &fstest.MapFile{Data: jpegData}, - fixtures.OtherHostPNG: &fstest.MapFile{Data: pngData}, - - // Invalid files - fixtures.InvalidFile: &fstest.MapFile{Data: []byte("not a real image file content")}, - fixtures.EmptyFile: &fstest.MapFile{Data: []byte{}}, - fixtures.TextFile: &fstest.MapFile{Data: []byte("This is just text, not a PNG")}, - } - - return mockFS, fixtures -} - // generateTestJPEG creates a minimal valid JPEG image. func generateTestJPEG(t *testing.T, width, height int, c color.Color) []byte { t.Helper() @@ -114,104 +35,3 @@ func generateTestJPEG(t *testing.T, width, height int, c color.Color) []byte { return buf.Bytes() } - -// generateTestPNG creates a minimal valid PNG image. -func generateTestPNG(t *testing.T, width, height int, c color.Color) []byte { - t.Helper() - - img := image.NewRGBA(image.Rect(0, 0, width, height)) - for y := range height { - for x := range width { - img.Set(x, y, c) - } - } - - var buf bytes.Buffer - - err := png.Encode(&buf, img) - if err != nil { - t.Fatalf("failed to encode test PNG: %v", err) - } - - return buf.Bytes() -} - -// generateTestGIF creates a minimal valid GIF image. -func generateTestGIF(t *testing.T, width, height int, c color.Color) []byte { - t.Helper() - - img := image.NewPaletted( - image.Rect(0, 0, width, height), - []color.Color{c, color.White}, - ) - for y := range height { - for x := range width { - img.SetColorIndex(x, y, 0) - } - } - - var buf bytes.Buffer - - err := gif.Encode(&buf, img, nil) - if err != nil { - t.Fatalf("failed to encode test GIF: %v", err) - } - - return buf.Bytes() -} - -// SetupTestService creates a Service with mock fetcher for testing. -func SetupTestService(t *testing.T) (*Service, *TestFixtures) { - t.Helper() - - mockFS, fixtures := NewTestFS(t) - - // Create temp directory for cache - tmpDir := t.TempDir() - - // Create in-memory database - db := setupServiceTestDB(t) - - cache, err := NewCache(db, CacheConfig{ - StateDir: tmpDir, - NegativeTTL: 5 * time.Minute, - MaxBytes: 1 << 30, - }, prometheus.NewRegistry()) - if err != nil { - t.Fatalf("failed to create cache: %v", err) - } - - svc, err := NewService(&ServiceConfig{ - Cache: cache, - Fetcher: httpfetcher.NewMock(mockFS), - Processor: imageprocessor.New(imageprocessor.Params{ - MaxInputBytes: 50 << 20, - MaxConcurrentProcessing: 2, - }), - MaxResponseSize: 50 << 20, - }) - if err != nil { - t.Fatalf("failed to create service: %v", err) - } - - return svc, fixtures -} - -// setupServiceTestDB creates an in-memory SQLite database for testing -// using the production schema. -func setupServiceTestDB(t *testing.T) *sql.DB { - t.Helper() - - db, err := sql.Open("sqlite", ":memory:") - if err != nil { - t.Fatalf("failed to open test db: %v", err) - } - - // Use the real production schema via migrations - err = database.ApplyMigrations(context.Background(), db, nil) - if err != nil { - t.Fatalf("failed to apply migrations: %v", err) - } - - return db -} diff --git a/internal/providers/providers.go b/internal/providers/providers.go index 2a11e61..ea406b8 100644 --- a/internal/providers/providers.go +++ b/internal/providers/providers.go @@ -26,6 +26,7 @@ import ( "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/logger" "sneak.berlin/go/pixa/internal/seal" @@ -34,7 +35,7 @@ import ( // Constructors returns the constructors of the parts this package builds, // for fx.Provide. The fx graph must also provide the prometheus.Registerer -// the cache registers its counters on. +// the cache and the image service register their counters on. func Constructors() []any { return []any{ newKeys, @@ -79,7 +80,7 @@ func newCache(lc fx.Lifecycle, params cacheParams) (*imgcache.Cache, error) { return nil, err } - cache, err := imgcache.NewCache(db, imgcache.CacheConfig{ + cache, err := imgcache.NewCache(db, imgcache.Config{ StateDir: cfg.StateDir, NegativeTTL: imgcache.DefaultNegativeTTL, MaxBytes: maxBytes, @@ -213,25 +214,26 @@ func newProcessor(cfg *config.Config) *imageprocessor.ImageProcessor { type imageServiceParams struct { fx.In - Config *config.Config - Cache *imgcache.Cache - Fetcher *httpfetcher.HTTPFetcher - Processor *imageprocessor.ImageProcessor - Logger *logger.Logger + Config *config.Config + Cache *imgcache.Cache + Fetcher *httpfetcher.HTTPFetcher + Processor *imageprocessor.ImageProcessor + Logger *logger.Logger + MetricsRegistry prometheus.Registerer } // newImageService builds the image service from the cache, the fetcher // and the processor, with the largest upstream response the config allows. func newImageService( _ fx.Lifecycle, params imageServiceParams, -) (*imgcache.Service, error) { - return imgcache.NewService(&imgcache.ServiceConfig{ +) (*imageproxy.Proxy, error) { + return imageproxy.NewProxy(&imageproxy.Config{ Cache: params.Cache, Fetcher: params.Fetcher, Processor: params.Processor, MaxResponseSize: params.Config.UpstreamMaxResponseSize, Logger: params.Logger.Get(), - }) + }, params.MetricsRegistry) } // sessionManagerParams defines dependencies for newSessionManager. diff --git a/internal/server/source_url_internal_test.go b/internal/server/source_url_internal_test.go index f897262..bb37d2d 100644 --- a/internal/server/source_url_internal_test.go +++ b/internal/server/source_url_internal_test.go @@ -14,8 +14,8 @@ import ( "time" "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/imageproxy" "sneak.berlin/go/pixa/internal/imagerequest" - "sneak.berlin/go/pixa/internal/imgcache" "sneak.berlin/go/pixa/internal/seal" "sneak.berlin/go/pixa/internal/session" "sneak.berlin/go/pixa/internal/signature" @@ -227,7 +227,7 @@ func TestSourcePathsHoldingSpaceAndPercent20AreFetchedApart(t *testing.T) { for _, imageURL := range imageURLs { cacheStatus := getImage(t, s, imageURL).Header().Get("X-Pixa-Cache") - if cacheStatus != string(imgcache.CacheHit) { + if cacheStatus != string(imageproxy.CacheHit) { t.Errorf("second GET %s: X-Pixa-Cache = %q, want HIT", imageURL, cacheStatus) } @@ -256,7 +256,7 @@ func TestSourceQueryWrittenTwoWaysIsOneUpstreamRequest(t *testing.T) { second := getImage(t, s, "/v1/image/"+upstreamHost+"/photo.png%3Fname=my%20cat/orig.png") - if got := second.Header().Get("X-Pixa-Cache"); got != string(imgcache.CacheHit) { + if got := second.Header().Get("X-Pixa-Cache"); got != string(imageproxy.CacheHit) { t.Errorf("second X-Pixa-Cache = %q, want HIT", got) } @@ -365,7 +365,7 @@ func TestGeneratorMakesOneImageURLForTwoSpellingsOfASourceURL(t *testing.T) { getImage(t, s, imageURLs[0]) second := getImage(t, s, imageURLs[1]) - if got := second.Header().Get("X-Pixa-Cache"); got != string(imgcache.CacheHit) { + if got := second.Header().Get("X-Pixa-Cache"); got != string(imageproxy.CacheHit) { t.Errorf("second X-Pixa-Cache = %q, want HIT", got) }