From 2790125cb274d1a90bb1def85110842bc42fb801 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Thu, 15 Oct 2026 13:45:25 +0200 Subject: [PATCH] Serve a cached variant while its source URL is in the negative cache (closes #245) Get checked the negative cache before the variant cache, so after one failed fetch of a source every variant of it answered 502 for five minutes, also one on disk or one that could be made from the cached source. The check now sits at the start of fetchAndProcess, which runs only when no cached source can be used. A refusal from the negative cache now counts as a miss. Model: opus-5-5 --- README.md | 7 +- TODO.md | 10 +++ .../imgcache/negative_cache_internal_test.go | 65 +++++++++++++++++++ internal/imgcache/service.go | 36 +++++----- 4 files changed, 99 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 609ad9e..9dc720d 100644 --- a/README.md +++ b/README.md @@ -200,9 +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; 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 d1ccf51..712a369 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,16 @@ 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 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/negative_cache_internal_test.go b/internal/imgcache/negative_cache_internal_test.go index f2c2115..ffb6079 100644 --- a/internal/imgcache/negative_cache_internal_test.go +++ b/internal/imgcache/negative_cache_internal_test.go @@ -94,6 +94,71 @@ 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, 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) + } + + 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 whose source was evicted error = %v, want %v", + err, ErrNegativeCached) + } +} + func TestService_Get_ReturnsErrorForNegativeCachedURL(t *testing.T) { t.Parallel() diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index 01f0db3..57f4e82 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -130,25 +130,13 @@ func NewService(cfg *ServiceConfig) (*Service, error) { var ErrNegativeCached = errors.New("request is in negative cache (recently failed)") // 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 - // 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 { @@ -433,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()