From 73f4cf03c14a71fbbe1afe975f599a9b53320b57 Mon Sep 17 00:00:00 2001
From: sneak <sneak@sneak.berlin>
Date: Thu, 15 Oct 2026 05:33:05 +0000
Subject: [PATCH] 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 250463e..7c7af30 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/<token>/<name>` — 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 b85bf37..cc4fb00 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 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 52684a9..6054cb5 100644
--- a/internal/imgcache/service.go
+++ b/internal/imgcache/service.go
@@ -128,26 +128,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 {
@@ -174,6 +161,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