Test that CleanExpired removes more than one batch of expired rows

TestCleanExpiredRemovesMoreThanOneBatch stores one expired negative
cache row more than a batch holds and calls CleanExpired alone, so a
CleanExpired that stops after its first batch fails it.

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
@@ -419,9 +418,7 @@ HMAC-SHA256(secret, "host:path:query:width:height:format:expiration:quality:fit"
 
 Where:
 
-- `host` — source host as the URL writes it up to the first `/`, in lower case,
-  keeping an IPv6 address's brackets and any `:` and port after it (e.g.
-  `cdn.example.com`, `cdn.example.com:8443`, `[2001:db8::1]:8443`)
+- `host` — source origin hostname (e.g. `cdn.example.com`)
 - `path` — source path (e.g. `/photos/cat.jpg`), with each `%` written as `%25`
   and each `:` as `%3A`, so `/photos/a:b.jpg` is signed as `/photos/a%3Ab.jpg`
 - `query` — source query string, empty string if none, with `%` and `:` written
@@ -476,12 +473,6 @@ entry that is neither a host name (letters, digits, hyphens, underscores and
 dots, with at most one leading dot) nor an IP address, such as one with a port
 or a `*.` wildcard, aborts startup.
 
-pixa lower-cases the host of a `/v1/image/` URL before anything else uses it, so
-`CDN.Example.com` is matched, fetched, cached and counted against
-`upstream_connections_per_host` as `cdn.example.com`. Such a URL whose host has
-a port, as in `cdn.example.com:8443`, or a `:` with no port, needs a signature
-even when the host is allowlisted. A host holding `@` is refused with 400.
-
 ### Configuration
 
 Every setting can be given as an environment variable, in a YAML config file

TODO.md

@@ -30,6 +30,14 @@ P2: security: per-origin rate limiting
 
 # Completed Steps
 
+- 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
+  `https://h/a:b` verified for `https://h/a?b:`; the two also shared a cache
+  key, and whichever was fetched first was served for both. In both, the path
+  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 expired negative cache rows are removed on a schedule (closes
   #244): the row of a failed fetch, which makes pixa answer 502 for that source
   URL for 5 minutes, was deleted only when the same URL was requested again
@@ -41,35 +49,6 @@ P2: security: per-origin rate limiting
   expired. The goroutine runs with the disk cache disabled
   (`cache_max_bytes: 0`) too, where it does nothing else, as the negative cache
   is used either way.
-- 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
-  each spelling was fetched and cached on its own and had connections of its own
-  under `upstream_connections_per_host`. The image URL parser now lower-cases
-  the host and refuses one holding `@`, which a URL reads as a user name and
-  password, with 400. A host with a port, or with a `:` and no port, needs a
-  signature even when it is allowlisted. That is checked in the image service,
-  not in `allowlist.IsAllowed`, which also matches `referer_blocklist`, where a
-  `Referer` is blocked whatever its port. A URL is signed over its host as it
-  writes it, in lower case, with any IPv6 brackets, `:` and port kept.
-- 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
-  `https://h/a:b` verified for `https://h/a?b:`; the two also shared a cache
-  key, and whichever was fetched first was served for both. In both, the path
-  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 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

internal/imgcache/negative_cache_internal_test.go

@@ -95,71 +95,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)
-	}
-}
-
 // TestEvictionLoopRemovesExpiredNegativeCacheRows stores a failed fetch
 // that expires a second later and starts the eviction loop with a short
 // interval: a later tick must remove its row although its URL is never

internal/imgcache/service.go

@@ -9,7 +9,6 @@ import (
 	"log/slog"
 	"net/url"
 	"runtime/debug"
-	"strings"
 	"time"
 
 	"github.com/dustin/go-humanize"
@@ -130,13 +129,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 {
@@ -207,16 +218,7 @@ func (s *Service) ValidateRequest(req *ImageRequest) error {
 		return fmt.Errorf("invalid source URL: %w", err)
 	}
 
-	// An allowlist entry names no port, so a host followed by ":", with or
-	// without a port, needs a signature. The port is read from the parsed
-	// URL, as the fetcher reads it: in an IPv6 address written without
-	// brackets, such as 2001:db8::1:8443, it is what follows the last ":".
-	// This is checked here and not in allowlist.IsAllowed, which also matches
-	// referer_blocklist, where a Referer must be blocked whatever its port.
-	hostHasPort := parsedURL.Port() != "" ||
-		strings.HasSuffix(parsedURL.Host, ":")
-
-	if !hostHasPort && s.allowlist.IsAllowed(parsedURL) {
+	if s.allowlist.IsAllowed(parsedURL) {
 		return nil
 	}
 
@@ -421,28 +423,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/imgcache/service_internal_test.go

@@ -250,97 +250,6 @@ func TestService_ValidateRequest_SignatureExactHostMatch(t *testing.T) {
 	}
 }
 
-// TestService_ValidateRequest_HostWithPortNeedsSignature verifies that a host
-// followed by a port, or by a colon with no port, matches no allowlist entry,
-// so that a request for it without a signature is refused, while the same
-// host with no port is allowlisted.
-func TestService_ValidateRequest_HostWithPortNeedsSignature(t *testing.T) {
-	t.Parallel()
-
-	svc, _ := SetupTestService(t,
-		WithSigningKey(testSigningKey),
-		WithAllowlist(testHostCDN, "203.0.113.5", "2001:db8::1"),
-	)
-
-	tests := []struct {
-		host        string
-		allowlisted bool
-	}{
-		{testHostCDN, true},
-		{"cdn.example.com:8443", false},
-		{"cdn.example.com:443", false},
-		{"cdn.example.com:", false},
-		{"203.0.113.5", true},
-		{"203.0.113.5:8080", false},
-		{"[2001:db8::1]", true},
-		{"[2001:db8::1]:8443", false},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.host, func(t *testing.T) {
-			t.Parallel()
-
-			req := &ImageRequest{
-				SourceHost: tt.host,
-				SourcePath: testPathCat,
-				Size:       Size{Width: 50, Height: 50},
-				Format:     FormatJPEG,
-				Quality:    85,
-				FitMode:    FitCover,
-			}
-
-			err := svc.ValidateRequest(req)
-			t.Logf("ValidateRequest() for host %q with no signature: %v",
-				tt.host, err)
-
-			if allowlisted := err == nil; allowlisted != tt.allowlisted {
-				t.Errorf("host %q served without a signature = %v, want %v",
-					tt.host, allowlisted, tt.allowlisted)
-			}
-		})
-	}
-}
-
-// TestService_ValidateRequest_UnbracketedIPv6PortNeedsSignature verifies that
-// an allowlisted IPv6 address written without brackets and followed by a
-// port, or by a colon with no port, needs a signature. Go's URL parser, which
-// the fetcher uses, reads 2001:db8::1:8443 as the host 2001:db8::1 with the
-// port 8443.
-func TestService_ValidateRequest_UnbracketedIPv6PortNeedsSignature(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	svc, _ := SetupTestService(t,
-		WithSigningKey(testSigningKey),
-		WithAllowlist("2001:db8::1"),
-	)
-
-	for _, host := range []string{"2001:db8::1:8443", "2001:db8::1:"} {
-		t.Run(host, func(t *testing.T) {
-			t.Parallel()
-
-			req := &ImageRequest{
-				SourceHost: host,
-				SourcePath: testPathCat,
-				Size:       Size{Width: 50, Height: 50},
-				Format:     FormatJPEG,
-				Quality:    85,
-				FitMode:    FitCover,
-			}
-
-			err := svc.ValidateRequest(req)
-			t.Logf("ValidateRequest() for host %q with no signature: %v",
-				host, err)
-
-			if err == nil {
-				t.Errorf("host %q served without a signature, want refused",
-					host)
-			}
-		})
-	}
-}
-
 func TestService_Get_InvalidFile(t *testing.T) {
 	t.Parallel()
 

internal/imgcache/urlparser.go

@@ -15,7 +15,6 @@ import (
 var (
 	ErrInvalidPath       = errors.New("invalid image path")
 	ErrMissingHost       = errors.New("missing source host")
-	ErrInvalidHost       = errors.New("invalid source host")
 	ErrMissingSize       = errors.New("missing size specification")
 	ErrInvalidSize       = errors.New("invalid size format")
 	ErrInvalidFormat     = errors.New("invalid or unsupported format")
@@ -144,17 +143,6 @@ func parseImageComponents(remainder string) (*ParsedURL, error) {
 		return nil, ErrMissingHost
 	}
 
-	// A URL reads what comes before "@" as a user name and password, and the
-	// rest as the host
-	if strings.Contains(host, "@") {
-		return nil, fmt.Errorf("%w: %q holds @", ErrInvalidHost, host)
-	}
-
-	// Host names do not depend on letter case, so the allowlist, the upstream
-	// connection limits, the cache and the signature see one spelling of each
-	// host
-	host = strings.ToLower(host)
-
 	// Extract query string if present in path
 	if qIndex := strings.Index(path, "?"); qIndex != -1 {
 		query = path[qIndex+1:]

internal/imgcache/urlparser_internal_test.go

@@ -234,49 +234,6 @@ func TestParseImagePath(t *testing.T) {
 	}
 }
 
-// TestParseImagePath_LowerCasesHost verifies that the host is lower-cased, so
-// that every spelling of its letter case is the same host to the allowlist,
-// the upstream connection limits and the cache, and that the path keeps its
-// case.
-func TestParseImagePath_LowerCasesHost(t *testing.T) {
-	t.Parallel()
-
-	got, err := ParseImagePath("CDN.Example.COM/Photos/Cat.jpg/800x600.webp")
-	if err != nil {
-		t.Fatalf("ParseImagePath() error = %v", err)
-	}
-
-	t.Logf("parsed host %q, path %q", got.Host, got.Path)
-
-	if got.Host != testHostCDN {
-		t.Errorf("Host = %q, want %q", got.Host, testHostCDN)
-	}
-
-	if got.Path != "/Photos/Cat.jpg" {
-		t.Errorf("Path = %q, want %q", got.Path, "/Photos/Cat.jpg")
-	}
-}
-
-// TestParseImagePath_RefusesUserinfo verifies that a host holding "@" is
-// refused: a URL reads what comes before the "@" as a user name and password
-// and the rest as the host.
-func TestParseImagePath_RefusesUserinfo(t *testing.T) {
-	t.Parallel()
-
-	for _, input := range []string{
-		"user@cdn.example.com/photos/cat.jpg/800x600.webp",
-		"user:password@cdn.example.com/photos/cat.jpg/800x600.webp",
-	} {
-		got, err := ParseImagePath(input)
-		t.Logf("ParseImagePath(%q) = %+v, error = %v", input, got, err)
-
-		if !errors.Is(err, ErrInvalidHost) {
-			t.Errorf("ParseImagePath(%q) error = %v, want %v",
-				input, err, ErrInvalidHost)
-		}
-	}
-}
-
 func TestParsedURL_ToImageRequest(t *testing.T) {
 	t.Parallel()