From 3321627eb68945415f3d6ecac1f75e7e74f63420 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 08:10:23 +0000 Subject: [PATCH] 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 | 17 ++++++++------- internal/imgcache/service.go | 40 +++++++++++++++++++----------------- 3 files changed, 35 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index d828dc6..5368183 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 d0031b8..938bd2f 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 a `:` in the source path or query no longer moves where the path ends and the query begins (closes #247): the signed data and the cache key joined host, path and query with `:` and no escaping, so a URL signed for @@ -38,13 +48,6 @@ P2: security: per-origin rate limiting and the query now have each `%` written as `%25` and each `:` as `%3A`; a path and query holding neither are signed and keyed as before. Signature Specification in `README.md` says so. -- 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 example config allowlists no host (closes #249): it allowlisted `github.com` and `user-images.githubusercontent.com`, where anyone can put an image, so a deployment set up as Getting Started says served those images to diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index 6054cb5..2301814 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -128,9 +128,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 @@ -161,21 +162,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 @@ -425,12 +411,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()