From d3d0b0c5669c1978e88343129e3d79aaa5c52e0f Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 04:56:23 +0000 Subject: [PATCH 1/4] Test that a cached variant is served while its URL is negative-cached Get refuses a cached variant with the negative-cache error while its source URL has a negative-cache row. The new test caches a variant, stores a row for its URL, and expects the variant to be served from the cache, and a variant not in the cache to be refused as before. It fails until Get checks the negative cache only on a miss (https://git.eeqj.de/sneak/pixa/issues/245). Model: opus-5-5 --- .../imgcache/negative_cache_internal_test.go | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/internal/imgcache/negative_cache_internal_test.go b/internal/imgcache/negative_cache_internal_test.go index f2c2115..e9b5f8a 100644 --- a/internal/imgcache/negative_cache_internal_test.go +++ b/internal/imgcache/negative_cache_internal_test.go @@ -94,6 +94,50 @@ 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; a variant not in the cache is +// still refused. +func TestService_Get_ServesCachedVariantOfNegativeCachedURL(t *testing.T) { + t.Parallel() + + svc, fixtures := SetupTestService(t) + + resp, err := svc.Get(t.Context(), photoVariant(fixtures, 85, FitCover)) + if err != nil { + t.Fatalf("Get() first request error = %v", err) + } + + _ = resp.Content.Close() + + err = svc.cache.StoreNegative(t.Context(), photoVariant(fixtures, 85, FitCover), + httpStatusBadGateway, "upstream answered 503") + if err != nil { + t.Fatalf("StoreNegative() error = %v", err) + } + + resp, err = svc.Get(t.Context(), photoVariant(fixtures, 85, 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) + } + + _, err = svc.Get(t.Context(), photoVariant(fixtures, 40, FitCover)) + t.Logf("Get() of a variant not in the cache: error = %v", err) + + if !errors.Is(err, ErrNegativeCached) { + t.Errorf("Get() of a variant not in the cache error = %v, want %v", + err, ErrNegativeCached) + } +} + func TestService_Get_ReturnsErrorForNegativeCachedURL(t *testing.T) { t.Parallel() -- 2.54.0 From d3c0c265798fe649621441e39c26e528ebbf51d7 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 05:33:05 +0000 Subject: [PATCH 2/4] Serve a cached variant while its source URL is negative-cached (closes #245) Get checked the negative cache before it looked for the variant on disk, so after one failed fetch of a source every cached variant of it answered 502 for 5 minutes. Get now serves a cached variant first and checks the negative cache only on a miss, before processing. The 502 line under /v1/image/ in README.md and TODO.md say so. Model: opus-5-5 --- README.md | 3 ++- TODO.md | 7 +++++++ internal/imgcache/service.go | 34 ++++++++++++++++++---------------- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 609ad9e..61235ac 100644 --- a/README.md +++ b/README.md @@ -201,7 +201,8 @@ path under `/v1/` answers 200, in maintenance mode too. `Content-Type` that is not one of the source formats (below), or with an image whose first bytes libvips reads as another format; 502 when the upstream answered with an error status, and for 5 minutes after that for the same - source URL; 503 when pixa is busy or in maintenance mode; 500 for any other + source URL, though an image of it that is already in the cache is still + served; 503 when pixa is busy or in maintenance mode; 500 for any other failure. - `GET` or `HEAD` `/v1/e//` — an image through an encrypted URL (see Encrypted URLs). Needs: nothing but the URL. Answers: 200; 304 when diff --git a/TODO.md b/TODO.md index d1ccf51..6e3c9de 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,13 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-15 a cached variant is served while its source URL is in the negative + cache (closes #245): `Get` checked the negative cache before the variant + cache, so once a fetch of the source failed, as one does when another variant + misses after the source was evicted and the upstream is down, every cached + variant of that source answered 502 for 5 minutes. `Get` now serves a cached + variant first and checks the negative cache only on a miss, before processing. + `README.md` says so under `/v1/image/`. - 2026-10-15 the host of a `/v1/image/` URL is lower-cased, and one with a port needs a signature (closes #246): with `cdn.example.com` allowlisted, `CDN.Example.com` and `cdn.example.com:8443` passed the allowlist too, and diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index 01f0db3..caa60b3 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -129,26 +129,13 @@ func NewService(cfg *ServiceConfig) (*Service, error) { // ErrNegativeCached is returned when a URL is in the negative cache (recently failed). var ErrNegativeCached = errors.New("request is in negative cache (recently failed)") -// Get retrieves a processed image, fetching and processing if necessary. +// Get retrieves a processed image, fetching and processing if necessary. A +// cached variant is served even while its source URL is in the negative +// cache, which refuses only a variant that is not cached. func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, error) { // Propagate AllowHTTP setting to the request req.AllowHTTP = s.allowHTTP - // Check negative cache first - skip fetching for recently-failed URLs - negHit, err := s.cache.checkNegativeCache(ctx, req) - if err != nil { - s.log.Warn("negative cache check failed", "error", err) - } - - if negHit { - s.log.Debug("negative cache hit", - "host", req.SourceHost, - "path", req.SourcePath, - ) - - return nil, fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamError, ErrNegativeCached) - } - // Check variant cache first (disk only, no DB) result, err := s.cache.Lookup(ctx, req) if err != nil { @@ -175,6 +162,21 @@ func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, e } } + // Check negative cache - skip fetching for recently-failed URLs + negHit, err := s.cache.checkNegativeCache(ctx, req) + if err != nil { + s.log.Warn("negative cache check failed", "error", err) + } + + if negHit { + s.log.Debug("negative cache hit", + "host", req.SourceHost, + "path", req.SourcePath, + ) + + return nil, fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamError, ErrNegativeCached) + } + // 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 -- 2.54.0 From 1827dad541306a9942308c398e7b1a51599c055c Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 07:43:46 +0000 Subject: [PATCH 3/4] Test that a variant is made from a cached source of a negative-cached URL The negative cache refuses only a fetch from the upstream: a variant not in the cache is made from the cached source, and refused once that source is evicted. Model: opus-5-5 --- .../imgcache/negative_cache_internal_test.go | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/internal/imgcache/negative_cache_internal_test.go b/internal/imgcache/negative_cache_internal_test.go index e9b5f8a..ffb6079 100644 --- a/internal/imgcache/negative_cache_internal_test.go +++ b/internal/imgcache/negative_cache_internal_test.go @@ -97,8 +97,9 @@ 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; a variant not in the cache is -// still refused. +// 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() @@ -129,11 +130,31 @@ func TestService_Get_ServesCachedVariantOfNegativeCachedURL(t *testing.T) { resp.CacheStatus, CacheHit) } - _, err = svc.Get(t.Context(), photoVariant(fixtures, 40, FitCover)) - t.Logf("Get() of a variant not in the cache: error = %v", err) + resp, err = svc.Get(t.Context(), photoVariant(fixtures, 40, 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() + + contentHash, _, err := svc.cache.LookupSource(t.Context(), + photoVariant(fixtures, 85, FitCover)) + if err != nil || contentHash == "" { + t.Fatalf("LookupSource() = %q, %v, want the cached source", contentHash, err) + } + + err = svc.cache.evictSourceBlob(t.Context(), contentHash) + if err != nil { + t.Fatalf("evictSourceBlob() error = %v", err) + } + + _, err = svc.Get(t.Context(), photoVariant(fixtures, 60, 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 not in the cache error = %v, want %v", + t.Errorf("Get() of a variant whose source was evicted error = %v, want %v", err, ErrNegativeCached) } } -- 2.54.0 From bf2671cd9f96062956fcd5862d52fdb6b3db55be Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 08:10:23 +0000 Subject: [PATCH 4/4] Check the negative cache only before a fetch from the upstream (closes #245) The check moves from Get to the start of fetchAndProcess, so a variant not in the cache is still made from a cached source while the URL is in the negative cache. A refusal now counts as a miss. Model: opus-5-5 --- README.md | 8 ++++---- TODO.md | 11 ++++++---- internal/imgcache/service.go | 40 +++++++++++++++++++----------------- 3 files changed, 32 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 61235ac..9dc720d 100644 --- a/README.md +++ b/README.md @@ -200,10 +200,10 @@ path under `/v1/` answers 200, in maintenance mode too. `image_requests_per_minute` (below); 415 when the upstream answers with a `Content-Type` that is not one of the source formats (below), or with an image whose first bytes libvips reads as another format; 502 when the upstream - answered with an error status, and for 5 minutes after that for the same - source URL, though an image of it that is already in the cache is still - served; 503 when pixa is busy or in maintenance mode; 500 for any other - failure. + answered with an error status, and for 5 minutes after that for an image of + the same source URL that pixa can neither serve from its cache nor make from + the source image still in its cache; 503 when pixa is busy or in maintenance + mode; 500 for any other failure. - `GET` or `HEAD` `/v1/e//` — an image through an encrypted URL (see Encrypted URLs). Needs: nothing but the URL. Answers: 200; 304 when `If-None-Match` matches the image's `ETag`; 400 for a token that does not diff --git a/TODO.md b/TODO.md index 6e3c9de..712a369 100644 --- a/TODO.md +++ b/TODO.md @@ -33,10 +33,13 @@ P2: security: per-origin rate limiting - 2026-10-15 a cached variant is served while its source URL is in the negative cache (closes #245): `Get` checked the negative cache before the variant cache, so once a fetch of the source failed, as one does when another variant - misses after the source was evicted and the upstream is down, every cached - variant of that source answered 502 for 5 minutes. `Get` now serves a cached - variant first and checks the negative cache only on a miss, before processing. - `README.md` says so under `/v1/image/`. + misses after the source was evicted and the upstream is down, every variant of + that source answered 502 for 5 minutes, also one on disk or one that could be + made from the cached source. The negative cache is now checked only just + before a fetch from the upstream, so a cached variant is served and a variant + not in the cache is made from the cached source when there is one. `README.md` + says so under `/v1/image/`. A refusal from the negative cache now counts as a + miss, as any other request that misses the variant cache does. - 2026-10-15 the host of a `/v1/image/` URL is lower-cased, and one with a port needs a signature (closes #246): with `cdn.example.com` allowlisted, `CDN.Example.com` and `cdn.example.com:8443` passed the allowlist too, and diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index caa60b3..57f4e82 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -129,9 +129,10 @@ func NewService(cfg *ServiceConfig) (*Service, error) { // ErrNegativeCached is returned when a URL is in the negative cache (recently failed). var ErrNegativeCached = errors.New("request is in negative cache (recently failed)") -// Get retrieves a processed image, fetching and processing if necessary. A -// cached variant is served even while its source URL is in the negative -// cache, which refuses only a variant that is not cached. +// 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. func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, error) { // Propagate AllowHTTP setting to the request req.AllowHTTP = s.allowHTTP @@ -162,21 +163,6 @@ func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, e } } - // Check negative cache - skip fetching for recently-failed URLs - negHit, err := s.cache.checkNegativeCache(ctx, req) - if err != nil { - s.log.Warn("negative cache check failed", "error", err) - } - - if negHit { - s.log.Debug("negative cache hit", - "host", req.SourceHost, - "path", req.SourcePath, - ) - - return nil, fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamError, ErrNegativeCached) - } - // 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 @@ -435,12 +421,28 @@ func (s *Service) processFromSourceOrFetch( // 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. +// reading the response or a later step fails. It fetches nothing while the +// source URL is in the negative cache. func (s *Service) fetchAndProcess( ctx context.Context, req *ImageRequest, cacheKey VariantKey, ) (*processedVariant, error) { + // Check negative cache - skip fetching for recently-failed URLs + negHit, err := s.cache.checkNegativeCache(ctx, req) + if err != nil { + s.log.Warn("negative cache check failed", "error", err) + } + + if negHit { + s.log.Debug("negative cache hit", + "host", req.SourceHost, + "path", req.SourcePath, + ) + + return nil, fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamError, ErrNegativeCached) + } + // Fetch from upstream sourceURL := req.SourceURL() -- 2.54.0