Keep the metrics entry at the top of Completed Steps

The rebase onto next placed the entry for this change below the one
next gained meanwhile; the TODO.md workflow puts a finished step at
the top.

Model: opus-5-5

README.md

@@ -200,10 +200,9 @@ 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 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.
+  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.
 - `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
   `If-None-Match` matches the image's `ETag`; 400 for a token that does not

TODO.md

@@ -30,16 +30,6 @@ 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 request metrics are labelled by route, not by path (closes
   #254): their `handler` label was each request's path, so every new path, a 404
   or an encrypted URL, added series to the Prometheus registry that were never

internal/imgcache/negative_cache_internal_test.go

@@ -94,71 +94,6 @@ 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()
 

internal/imgcache/service.go

@@ -130,13 +130,25 @@ 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 {
@@ -421,28 +433,12 @@ 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. It fetches nothing while the
-// source URL is in the negative cache.
+// reading the response or a later step fails.
 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()
 

internal/middleware/middleware_internal_test.go

@@ -227,12 +227,10 @@ func TestMetricsRecordsServedRequest(t *testing.T) {
 // each request with the pattern of the route that answers it, never with its
 // path: two paths one route answers share that route's pattern, and requests
 // no route answers, a 404 or a 405, share "unmatched", among them requests for
-// the path the subrouter is mounted at, /v1 and /v1/. A request with an empty
-// path is answered by the / route, as chi routes it as /, and is labelled /.
-// With a label per path, any client could add series to the registry without
-// limit. The middleware registers with a registry of its own here, as another
-// test in this package registers with Prometheus' default registry, which can
-// be done only once.
+// the path the subrouter is mounted at, /v1 and /v1/. With a label per path,
+// any client could add series to the registry without limit. The middleware
+// registers with a registry of its own here, as another test in this package
+// registers with Prometheus' default registry, which can be done only once.
 func TestMetricsLabelsRequestsByRoute(t *testing.T) {
 	t.Parallel()
 
@@ -246,9 +244,6 @@ func TestMetricsLabelsRequestsByRoute(t *testing.T) {
 	// The image route is in a subrouter, as in pixa's router.
 	router := chi.NewRouter()
 	router.Use(mw.Metrics())
-	router.Get("/", func(w http.ResponseWriter, _ *http.Request) {
-		w.WriteHeader(http.StatusOK)
-	})
 	router.Route("/v1", func(r chi.Router) {
 		r.Get("/image/*", func(w http.ResponseWriter, _ *http.Request) {
 			w.WriteHeader(http.StatusOK)
@@ -263,21 +258,15 @@ func TestMetricsLabelsRequestsByRoute(t *testing.T) {
 		{http.MethodGet, "/v1"},
 		{http.MethodGet, "/v1/"},
 		{http.MethodPost, "/v1/image/a.example.com/one.jpg/1x1.png"},
-		// The request line GET http://pixa.example HTTP/1.1 leaves both
-		// r.URL.Path and r.URL.RawPath empty.
-		{http.MethodGet, "http://pixa.example"},
 	} {
-		r := httptest.NewRequestWithContext(
-			t.Context(), req.method, req.path, nil)
-		t.Logf("%s %s: r.URL.Path %q, r.URL.RawPath %q",
-			req.method, req.path, r.URL.Path, r.URL.RawPath)
-		router.ServeHTTP(httptest.NewRecorder(), r)
+		router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext(
+			t.Context(), req.method, req.path, nil))
 	}
 
 	got := requestCountsByHandler(t, registry)
 	t.Logf("requests counted by handler label: %v", got)
 
-	want := map[string]uint64{"/": 1, "/v1/image/*": 2, "unmatched": 5}
+	want := map[string]uint64{"/v1/image/*": 2, "unmatched": 5}
 	if !maps.Equal(got, want) {
 		t.Errorf("requests counted by handler label = %v, want %v", got, want)
 	}