Delete expired negative cache rows in batches

CleanExpired deletes at most 1000 expired rows per statement and starts
no further statement once its context ends, so a request on the one
database connection waits behind one batch at most.

Model: opus-5-5

README.md

@@ -26,6 +26,11 @@ make docker
 docker run -p 8080:8080 -e PIXA_SIGNING_KEY="$(openssl rand -base64 32)" pixa:latest
 ```
 
+The example config allowlists no host (`allowlist_hosts`), so pixa serves an
+image only for a signed or encrypted URL. Every image on an allowlisted host is
+served to anyone without a signature, at any size and in any format, so
+allowlist only hosts you control, where nobody else can put images.
+
 A container takes its settings from environment variables (see Configuration
 below for the list). Only `PIXA_SIGNING_KEY` is required; if it is unset the
 container exits at startup naming the variable. Everything else has a built-in
@@ -115,7 +120,9 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app for pixa needs:
     - `PIXA_SIGNING_KEY` (required): secret for signed and encrypted URLs and
       login, 32+ characters, for example from `openssl rand -base64 32`
     - `PIXA_ALLOWLIST_HOSTS`: upstream hosts served without a signature,
-      comma-separated
+      comma-separated; default none. Every image on a listed host is served to
+      anyone without a signature, so list only hosts you control, where nobody
+      else can put images.
     - `PIXA_CACHE_MAX_BYTES`: disk cache limit in bytes; `0` disables it;
       default 75% of (free space + what the cache holds)
     - the rest are in the table under Configuration below

TODO.md

@@ -30,6 +30,25 @@ P2: security: per-origin rate limiting
 
 # Completed Steps
 
+- 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
+  after that, so a client asking for a new missing path each time added rows
+  that stayed for good. The eviction goroutine now calls `CleanExpired` at
+  startup and on each periodic tick, every 5 minutes in pixad, so a row stays at
+  most about 10 minutes. `CleanExpired` deletes at most 1000 rows per statement,
+  so a request waits behind 1000 deletions at most, however many rows have
+  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 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
+  anyone without a signature, at any size and in any format.
+  `configs/config.example.yml` now leaves `allowlist_hosts` out, with one
+  commented entry under `example.com`. Getting Started and "Running under upaas"
+  in `README.md` say that every image on an allowlisted host is served without a
+  signature, so an allowlisted host must be one the operator controls.
 - 2026-10-13 `fit=outside` no longer makes an output above 8192 on a side
   (closes #257): it scales the image to cover the requested size, and the side
   scaled past that size was held to no maximum, so `1x8192?fit=outside` of a

configs/config.example.yml

@@ -45,15 +45,14 @@ state_dir: ./data
 signing_key: "CHANGE_ME_generate_with_openssl_rand_base64_32"
 
 # Hosts that don't require signatures (default: none)
+# Every image on an allowlisted host is served to anyone without a
+# signature, at any size and in any format, so list only hosts you
+# control, where nobody else can put images.
 # Use "." prefix for wildcard subdomain matching (e.g., ".example.com" matches "cdn.example.com")
 # An entry that is neither a host name nor an IP address (IPv6 without
 # brackets), such as one with a port or a "*." wildcard, aborts startup.
-allowlist_hosts:
-  - s3.sneak.cloud
-  - static.sneak.cloud
-  - sneak.berlin
-  - github.com
-  - user-images.githubusercontent.com
+# allowlist_hosts:
+#   - images.example.com
 
 # Hosts whose pages may not show pixa's images, written as for
 # allowlist_hosts. A request to /v1/image/ or /v1/e/ whose Referer header

internal/config/concurrency_limits_internal_test.go

@@ -32,28 +32,6 @@ func TestOmittedConcurrencyLimitsUseDefaults(t *testing.T) {
 	}
 }
 
-// TestOmittedMaxConcurrentProcessingIsAtMostFour checks that an omitted
-// max_concurrent_processing is the number of CPUs Go uses, but never more
-// than 4, so a container with no CPU limit on a host with many CPUs does not
-// process one image per CPU at once.
-func TestOmittedMaxConcurrentProcessingIsAtMostFour(t *testing.T) {
-	t.Parallel()
-
-	c, err := configFromYAML(t, signingKeyLine)
-	if err != nil {
-		t.Fatalf("minimal config should be valid, got error: %v", err)
-	}
-
-	want := min(runtime.GOMAXPROCS(0), 4)
-	t.Logf("GOMAXPROCS %d, MaxConcurrentProcessing %d",
-		runtime.GOMAXPROCS(0), c.MaxConcurrentProcessing)
-
-	if c.MaxConcurrentProcessing != want {
-		t.Errorf("MaxConcurrentProcessing = %d, want %d",
-			c.MaxConcurrentProcessing, want)
-	}
-}
-
 // TestExplicitConcurrencyLimitsAreUsed checks that valid values for the
 // two limits are used as given.
 func TestExplicitConcurrencyLimitsAreUsed(t *testing.T) {

internal/config/config_internal_test.go

@@ -92,6 +92,32 @@ func TestGetStringSlice_Empty(t *testing.T) {
 	}
 }
 
+// TestExampleConfigAllowlistsNoHost loads configs/config.example.yml
+// with a real signing key, as Getting Started in README.md has it copied
+// and edited, and checks that it allowlists no host: every image on an
+// allowlisted host is served to anyone without a signature.
+func TestExampleConfigAllowlistsNoHost(t *testing.T) {
+	t.Setenv("PIXA_SIGNING_KEY", validTestSigningKey)
+
+	examplePath := filepath.Join("..", "..", "configs", "config.example.yml")
+
+	sc, err := loadTestConfig(examplePath)
+	if err != nil {
+		t.Fatalf("failed to load %s: %v", examplePath, err)
+	}
+
+	c, err := newFromSmartConfig(sc)
+	if err != nil {
+		t.Fatalf("%s is not a valid config: %v", examplePath, err)
+	}
+
+	t.Logf("allowlist_hosts in %s: %v", examplePath, c.AllowlistHosts)
+
+	if len(c.AllowlistHosts) != 0 {
+		t.Errorf("%s allowlists %v, want no host", examplePath, c.AllowlistHosts)
+	}
+}
+
 // loadTestConfig is a helper to load a config file for testing.
 func loadTestConfig(path string) (*smartconfig.Config, error) {
 	return smartconfig.NewFromConfigPath(path)

internal/handlers/handlers.go

@@ -124,7 +124,8 @@ func (s *Handlers) initImageService() error {
 	s.imgCache = cache
 
 	// Background eviction: startup reconciliation, then periodic and
-	// write-pressure passes. No-op when the disk cache is disabled.
+	// write-pressure passes, and the removal of expired negative cache
+	// rows, which is all it does when the disk cache is disabled.
 	cache.StartEviction(imgcache.DefaultEvictionInterval)
 
 	// Writes the counts requests could not write by their deadline

internal/imageprocessor/max_concurrent_processing_internal_test.go

@@ -132,27 +132,6 @@ func TestNewDefaultsMaxConcurrentProcessingToCPUs(t *testing.T) {
 	}
 }
 
-// TestNewDefaultMaxConcurrentProcessingIsAtMostFour checks that New, given
-// no MaxConcurrentProcessing, makes one slot per CPU Go uses, but never more
-// than 4.
-func TestNewDefaultMaxConcurrentProcessingIsAtMostFour(t *testing.T) {
-	t.Parallel()
-
-	want := min(runtime.GOMAXPROCS(0), 4)
-
-	for _, limit := range []int{0, -1} {
-		proc := New(Params{MaxConcurrentProcessing: limit})
-		got := cap(proc.processingSemaphore)
-		t.Logf("MaxConcurrentProcessing %d, GOMAXPROCS %d: %d slots",
-			limit, runtime.GOMAXPROCS(0), got)
-
-		if got != want {
-			t.Errorf("MaxConcurrentProcessing %d: %d slots, want %d",
-				limit, got, want)
-		}
-	}
-}
-
 // TestProcessNeverExceedsMaxConcurrentProcessing starts more Process calls
 // than MaxConcurrentProcessing allows and holds the first ones inside
 // Process until the test lets them go. No more than the limit may be

internal/imgcache/cache.go

@@ -32,6 +32,12 @@ const httpStatusOK = 200
 // variant not among them is served as before, reading its .meta file.
 const metaCacheSize = 10000
 
+// cleanExpiredBatchSize is the most expired negative cache rows one
+// statement of CleanExpired deletes. Each batch is a statement of its
+// own, so a request waits behind one batch at most, however many rows
+// have expired.
+const cleanExpiredBatchSize = 1000
+
 // CacheConfig holds cache configuration.
 type CacheConfig struct {
 	StateDir    string
@@ -50,8 +56,9 @@ type CacheConfig struct {
 
 	// DisableDiskCache turns the disk cache off entirely: no cache
 	// directories are created, lookups always miss, stores are
-	// no-ops, and no eviction machinery runs. The config layer sets
-	// this when the operator configures cache_max_bytes: 0.
+	// no-ops, and the eviction goroutine only removes the expired
+	// negative cache rows. The config layer sets this when the
+	// operator configures cache_max_bytes: 0.
 	DisableDiskCache bool
 
 	// Logger receives accounting and eviction log output. A nil
@@ -482,17 +489,27 @@ func (c *Cache) GetSourceContent(
 	return c.srcContent.LoadWithSize(contentHash)
 }
 
-// CleanExpired removes expired entries from the cache.
+// CleanExpired removes the negative cache rows past their expiry, one
+// batch per statement, until none is left. Once ctx ends it starts no
+// further batch and returns ctx's error. The eviction goroutine calls it
+// at startup and on every periodic tick, so a row stays about
+// NegativeTTL plus one eviction interval, whether or not its URL is
+// requested again.
 func (c *Cache) CleanExpired(ctx context.Context) error {
-	// Clean expired negative cache entries
-	_, err := c.db.ExecContext(ctx, `
-		DELETE FROM negative_cache WHERE expires_at < CURRENT_TIMESTAMP
-	`)
-	if err != nil {
-		return fmt.Errorf("failed to clean negative cache: %w", err)
-	}
+	for {
+		if ctx.Err() != nil {
+			return ctx.Err()
+		}
 
-	return nil
+		deleted, err := c.cleanExpiredBatch(ctx)
+		if err != nil {
+			return err
+		}
+
+		if deleted < cleanExpiredBatchSize {
+			return nil
+		}
+	}
 }
 
 // Stats returns cache statistics.
@@ -756,3 +773,27 @@ func (c *Cache) checkNegativeCache(
 
 	return true, nil
 }
+
+// cleanExpiredBatch deletes up to cleanExpiredBatchSize negative cache
+// rows past their expiry in one statement, and returns how many it
+// deleted. DELETE ... LIMIT needs a SQLite build option, so the subquery
+// picks the rows.
+func (c *Cache) cleanExpiredBatch(ctx context.Context) (int64, error) {
+	result, err := c.db.ExecContext(ctx, `
+		DELETE FROM negative_cache WHERE id IN (
+			SELECT id FROM negative_cache
+			WHERE expires_at < CURRENT_TIMESTAMP
+			LIMIT ?
+		)
+	`, cleanExpiredBatchSize)
+	if err != nil {
+		return 0, fmt.Errorf("failed to clean negative cache: %w", err)
+	}
+
+	deleted, err := result.RowsAffected()
+	if err != nil {
+		return 0, fmt.Errorf("failed to count cleaned negative cache rows: %w", err)
+	}
+
+	return deleted, nil
+}

internal/imgcache/eviction.go

@@ -437,14 +437,16 @@ func (c *Cache) notifyWritePressure() {
 }
 
 // StartEviction launches the background eviction goroutine, which
-// reconciles the database accounting with the cache directories at
-// startup and again on every periodic tick thereafter, and evicts to
-// the configured limit on the given periodic interval and on
-// write-pressure notifications. It is a no-op on a disabled cache or
-// when already started. The goroutine outlives the caller, so it runs
-// with its own context, which StopEviction cancels.
+// reconciles the database accounting with the cache directories and
+// removes the expired negative cache rows at startup and again on every
+// periodic tick thereafter, and evicts to the configured limit on the
+// given periodic interval and on write-pressure notifications. On a
+// disabled cache it only removes the expired negative cache rows, as
+// the negative cache is used either way. It is a no-op when already
+// started. The goroutine outlives the caller, so it runs with its own
+// context, which StopEviction cancels.
 func (c *Cache) StartEviction(interval time.Duration) {
-	if c.disabled || c.evictionCancel != nil {
+	if c.evictionCancel != nil {
 		return
 	}
 
@@ -480,6 +482,7 @@ func (c *Cache) evictionLoop(ctx context.Context, interval time.Duration) {
 	defer close(c.evictionDone)
 
 	c.runReconciliationPass(ctx)
+	c.runCleanExpired(ctx)
 	c.runEvictionPass(ctx)
 
 	ticker := time.NewTicker(interval)
@@ -500,6 +503,7 @@ func (c *Cache) evictionLoop(ctx context.Context, interval time.Duration) {
 			// unaccounted for to one interval, on a process that is
 			// already running this loop regardless.
 			c.runReconciliationPass(ctx)
+			c.runCleanExpired(ctx)
 		case <-c.evictionPressure:
 		}
 
@@ -535,6 +539,20 @@ func (c *Cache) runReconciliationPass(ctx context.Context) {
 	}
 }
 
+// runCleanExpired runs CleanExpired, logging a failure instead of
+// propagating it (the loop must keep running). It does nothing once ctx
+// is cancelled.
+func (c *Cache) runCleanExpired(ctx context.Context) {
+	if ctx.Err() != nil {
+		return
+	}
+
+	err := c.CleanExpired(ctx)
+	if err != nil {
+		c.log.Warn("negative cache cleanup failed", "error", err)
+	}
+}
+
 // reconcileAccounting synchronizes the database size accounting with
 // the actual contents of the cache directories. It runs at startup and
 // again on every periodic eviction tick thereafter, off the request

internal/imgcache/negative_cache_internal_test.go

@@ -3,6 +3,7 @@ package imgcache
 import (
 	"context"
 	"errors"
+	"strconv"
 	"testing"
 	"time"
 )
@@ -94,6 +95,211 @@ func TestNegativeCache_Expired(t *testing.T) {
 	}
 }
 
+// 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
+// requested again, and must keep a row that has not expired. It runs
+// with the disk cache enabled and disabled, as the negative cache is
+// used either way.
+func TestEvictionLoopRemovesExpiredNegativeCacheRows(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name             string
+		disableDiskCache bool
+	}{
+		{name: "disk cache enabled", disableDiskCache: false},
+		{name: "disk cache disabled", disableDiskCache: true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			cache, err := NewCache(evictionTestDB(t), CacheConfig{
+				StateDir:         t.TempDir(),
+				NegativeTTL:      time.Second,
+				DisableDiskCache: tt.disableDiskCache,
+			})
+			if err != nil {
+				t.Fatalf("failed to create cache: %v", err)
+			}
+
+			expired := &ImageRequest{
+				SourceHost: testHostExample, SourcePath: "/gone.jpg",
+			}
+
+			err = cache.StoreNegative(t.Context(), expired, 404, "not found")
+			if err != nil {
+				t.Fatalf("StoreNegative failed: %v", err)
+			}
+
+			const notExpiredPath = "/later.jpg"
+
+			_, err = cache.db.ExecContext(t.Context(), `
+				INSERT INTO negative_cache
+					(source_host, source_path, status_code, expires_at)
+				VALUES (?, ?, 404, datetime('now', '+1 hour'))
+			`, testHostExample, notExpiredPath)
+			if err != nil {
+				t.Fatalf("failed to insert a row that has not expired: %v", err)
+			}
+
+			started := time.Now()
+
+			cache.StartEviction(100 * time.Millisecond)
+			defer func() { _ = cache.StopEviction(t.Context()) }()
+
+			for negativeCacheRowCount(t, cache, expired.SourcePath) > 0 {
+				if time.Since(started) > 10*time.Second {
+					t.Fatal("the expired row is still in negative_cache")
+				}
+
+				time.Sleep(50 * time.Millisecond)
+			}
+
+			t.Logf("the expired row was removed %s after the loop started",
+				time.Since(started))
+
+			if negativeCacheRowCount(t, cache, notExpiredPath) != 1 {
+				t.Error("the row that has not expired was removed")
+			}
+		})
+	}
+}
+
+// negativeCacheRowCount returns how many negative_cache rows hold
+// sourcePath.
+func negativeCacheRowCount(tb testing.TB, cache *Cache, sourcePath string) int {
+	tb.Helper()
+
+	var count int
+
+	err := cache.db.QueryRowContext(tb.Context(),
+		`SELECT COUNT(*) FROM negative_cache WHERE source_path = ?`,
+		sourcePath,
+	).Scan(&count)
+	if err != nil {
+		tb.Fatalf("failed to count negative_cache rows: %v", err)
+	}
+
+	return count
+}
+
+// TestCleanExpiredDeletesInBatches stores one expired negative cache row
+// more than one batch holds, and one row that has not expired. One
+// statement must delete one batch and no more, so a request waits behind
+// one batch at most; CleanExpired must then remove the expired row left
+// over and keep the row that has not expired.
+func TestCleanExpiredDeletesInBatches(t *testing.T) {
+	t.Parallel()
+
+	cache, err := NewCache(evictionTestDB(t), CacheConfig{StateDir: t.TempDir()})
+	if err != nil {
+		t.Fatalf("failed to create cache: %v", err)
+	}
+
+	ctx := t.Context()
+
+	insertExpiredNegativeCacheRows(t, cache, cleanExpiredBatchSize+1)
+
+	const notExpiredPath = "/later.jpg"
+
+	_, err = cache.db.ExecContext(ctx, `
+		INSERT INTO negative_cache
+			(source_host, source_path, status_code, expires_at)
+		VALUES (?, ?, 404, datetime('now', '+1 hour'))
+	`, testHostExample, notExpiredPath)
+	if err != nil {
+		t.Fatalf("failed to insert a row that has not expired: %v", err)
+	}
+
+	deleted, err := cache.cleanExpiredBatch(ctx)
+	if err != nil {
+		t.Fatalf("cleanExpiredBatch failed: %v", err)
+	}
+
+	t.Logf("one statement deleted %d of %d expired rows",
+		deleted, cleanExpiredBatchSize+1)
+
+	if deleted != cleanExpiredBatchSize {
+		t.Errorf("one statement deleted %d rows, want %d",
+			deleted, cleanExpiredBatchSize)
+	}
+
+	err = cache.CleanExpired(ctx)
+	if err != nil {
+		t.Fatalf("CleanExpired failed: %v", err)
+	}
+
+	if count := negativeCacheTableRowCount(t, cache); count != 1 {
+		t.Errorf("negative_cache holds %d rows after CleanExpired, want 1", count)
+	}
+
+	if negativeCacheRowCount(t, cache, notExpiredPath) != 1 {
+		t.Error("the row that has not expired was removed")
+	}
+}
+
+// TestCleanExpiredStopsOnceContextEnds runs CleanExpired with a context
+// that has already ended: it must return the context's error and delete
+// no row.
+func TestCleanExpiredStopsOnceContextEnds(t *testing.T) {
+	t.Parallel()
+
+	cache, err := NewCache(evictionTestDB(t), CacheConfig{StateDir: t.TempDir()})
+	if err != nil {
+		t.Fatalf("failed to create cache: %v", err)
+	}
+
+	insertExpiredNegativeCacheRows(t, cache, 1)
+
+	ctx, cancel := context.WithCancel(t.Context())
+	cancel()
+
+	err = cache.CleanExpired(ctx)
+	if !errors.Is(err, context.Canceled) {
+		t.Errorf("CleanExpired() error = %v, want context.Canceled", err)
+	}
+
+	if count := negativeCacheTableRowCount(t, cache); count != 1 {
+		t.Errorf("negative_cache holds %d rows, want the expired row kept", count)
+	}
+}
+
+// insertExpiredNegativeCacheRows inserts count negative_cache rows that
+// expired an hour ago, each for a different source path.
+func insertExpiredNegativeCacheRows(t *testing.T, cache *Cache, count int) {
+	t.Helper()
+
+	for i := range count {
+		_, err := cache.db.ExecContext(t.Context(), `
+			INSERT INTO negative_cache
+				(source_host, source_path, status_code, expires_at)
+			VALUES (?, ?, 404, datetime('now', '-1 hour'))
+		`, testHostExample, "/expired/"+strconv.Itoa(i)+".jpg")
+		if err != nil {
+			t.Fatalf("failed to insert an expired row: %v", err)
+		}
+	}
+}
+
+// negativeCacheTableRowCount returns how many rows negative_cache holds.
+func negativeCacheTableRowCount(tb testing.TB, cache *Cache) int {
+	tb.Helper()
+
+	var count int
+
+	err := cache.db.QueryRowContext(tb.Context(),
+		`SELECT COUNT(*) FROM negative_cache`,
+	).Scan(&count)
+	if err != nil {
+		tb.Fatalf("failed to count negative_cache rows: %v", err)
+	}
+
+	return count
+}
+
 func TestService_Get_ReturnsErrorForNegativeCachedURL(t *testing.T) {
 	t.Parallel()