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()