diff --git a/README.md b/README.md index 7c7af30..6d2fa08 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 3884a64..32db961 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 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()