From d6a7bd9ab67d7eeafbf3a79fb6db04c6938b1773 Mon Sep 17 00:00:00 2001
From: clawbot <35+clawbot@noreply.example.org>
Date: Sun, 4 Oct 2026 13:30:24 +0000
Subject: [PATCH] Stop TestEvictionRunsOnPeriodicSchedule racing the evictor
 (closes #183)

The test wrote each variant file and then inserted its accounting row by
hand while the evictor was running. A reconciliation pass between the two
steps adopted the file first, and the hand insert failed on the unique key.

The test now writes the files only, while it holds the test database's
only connection, so the evictor's startup pass waits after walking the
still empty variant directory. A periodic reconciliation pass then adopts
the files and the eviction pass after it evicts them; no write-pressure
notification fires. The test waits until two of the three files are gone,
then checks that usage is within the limit and that no row points at a
missing file.

Model: opus-5-5
---
 TODO.md                                     |  9 +++
 internal/imgcache/eviction_internal_test.go | 69 ++++++++++++++++-----
 2 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/TODO.md b/TODO.md
index 1fa426a..ec2ee54 100644
--- a/TODO.md
+++ b/TODO.md
@@ -29,6 +29,15 @@ P2: security: referer blacklist
 
 # Completed Steps
 
+- 2026-10-04 `TestEvictionRunsOnPeriodicSchedule` no longer races the evictor
+  (closes #183): it wrote each variant file and then inserted its accounting row
+  by hand, and a reconciliation pass between the two adopted the file first, so
+  the insert failed. It now writes the files only, while holding the test
+  database's only connection so the evictor's startup pass waits after walking
+  the empty variant directory; a periodic reconciliation pass then adopts the
+  files and the eviction pass after it evicts them. No other test in
+  `internal/imgcache` inserts a row by hand after starting the evictor. Test
+  only.
 - 2026-10-04 deployment guide and example Caddy config (closes #89):
   "Deployment" in `README.md` says what the reverse proxy in front of pixa must
   do (terminate TLS; pass `Host`, `Origin` and `Referer` on unchanged; set
diff --git a/internal/imgcache/eviction_internal_test.go b/internal/imgcache/eviction_internal_test.go
index 5b51f18..4b0bbf7 100644
--- a/internal/imgcache/eviction_internal_test.go
+++ b/internal/imgcache/eviction_internal_test.go
@@ -681,6 +681,11 @@ func TestEvictionRunsUnderWritePressure(t *testing.T) {
 	assertNoDanglingReferences(t, cache)
 }
 
+// TestEvictionRunsOnPeriodicSchedule writes three variant files straight
+// to disk, bypassing StoreVariant, so they have no accounting rows and no
+// write-pressure notification fires. Only a periodic reconciliation pass
+// can then adopt them, and only the eviction pass that follows it can
+// evict them.
 func TestEvictionRunsOnPeriodicSchedule(t *testing.T) {
 	t.Parallel()
 
@@ -688,13 +693,29 @@ func TestEvictionRunsOnPeriodicSchedule(t *testing.T) {
 
 	cache, _ := newEvictionTestCache(t, limit)
 
-	// Start the evictor while the cache is empty, then create tracked
-	// over-limit state WITHOUT going through the store methods, so no
-	// write-pressure notification fires and only the periodic ticker
-	// can trigger eviction.
+	// Hold the test database's only connection, so the startup pass
+	// waits for it after walking the still empty variant directory: the
+	// files written while it waits are first seen by a periodic pass.
+	conn, err := cache.db.Conn(t.Context())
+	if err != nil {
+		t.Fatalf("failed to take the database connection: %v", err)
+	}
+
+	defer func() { _ = conn.Close() }()
+
 	cache.StartEviction(100 * time.Millisecond)
 	defer func() { _ = cache.StopEviction(t.Context()) }()
 
+	deadline := time.Now().Add(5 * time.Second)
+
+	for cache.db.Stats().WaitCount == 0 {
+		if time.Now().After(deadline) {
+			t.Fatal("the startup pass never waited for the database")
+		}
+
+		time.Sleep(10 * time.Millisecond)
+	}
+
 	keys := []VariantKey{
 		testVariantKeyOne, testVariantKeyTwo, testVariantKeyThree,
 	}
@@ -703,25 +724,43 @@ func TestEvictionRunsOnPeriodicSchedule(t *testing.T) {
 	for i, key := range keys {
 		content := bytes.Repeat([]byte{fills[i]}, 1000)
 
-		_, err := cache.variants.Store(key, bytes.NewReader(content), "image/webp")
+		_, err = cache.variants.Store(key, bytes.NewReader(content), "image/webp")
 		if err != nil {
 			t.Fatalf("failed to store variant file: %v", err)
 		}
+	}
 
-		_, err = cache.db.ExecContext(t.Context(),
-			`INSERT INTO variant_content (cache_key, size_bytes, content_type)
-			 VALUES (?, ?, ?)`,
-			string(key), len(content), "image/webp",
-		)
-		if err != nil {
-			t.Fatalf("failed to insert variant accounting row: %v", err)
+	_ = conn.Close()
+
+	// Only one of the 1000-byte files fits under the limit: wait until
+	// the evictor has removed the other two.
+	stored := len(keys)
+	deadline = time.Now().Add(5 * time.Second)
+
+	for stored > 1 && time.Now().Before(deadline) {
+		time.Sleep(25 * time.Millisecond)
+
+		stored = 0
+
+		for _, key := range keys {
+			if cache.variants.Exists(key) {
+				stored++
+			}
 		}
 	}
 
-	usage := waitForUsageAtOrBelow(t, cache, limit, 5*time.Second)
+	if stored > 1 {
+		t.Fatalf("periodic schedule did not trigger eviction: %d of %d "+
+			"variant files still on disk, want at most 1", stored, len(keys))
+	}
+
+	usage, err := cache.UsageBytes(t.Context())
+	if err != nil {
+		t.Fatalf("UsageBytes failed: %v", err)
+	}
+
 	if usage > limit {
-		t.Errorf("periodic schedule did not trigger eviction: usage = %d, want <= %d",
-			usage, limit)
+		t.Errorf("usage after eviction = %d, want <= %d", usage, limit)
 	}
 
 	assertNoDanglingReferences(t, cache)
-- 
2.54.0