gocache/gocached: evict Actions younger than maxAge when over size

gocache/gocached/gocached.go

@@ -3118,26 +3118,36 @@ func (srv *Server) cleanupTick(ctx context.Context) (countAndSize, error) {
 	all.Count += dCount
 	all.Size += dBytes
 
-	// Default cutoff to "any age" for size-only cleanup. evictOldestActions
-	// still uses idx_actions_access (the WHERE/ORDER BY drive the plan), so
-	// the very-large cutoff just means "no upper bound".
-	cutoff := int64(math.MaxInt64)
-	overAge := false
-	if srv.maxAge > 0 {
-		cutoff = srv.now().Add(-srv.maxAge).Unix()
-		overAge = us.All().Count-us.ActionsLE[srv.maxAge].Count > 0
-	}
+	// When the cache is over both limits, remove over-age Actions first.
+	// Then do size-only cleanup with no age cutoff. If you use the maxAge
+	// cutoff for size-only cleanup, the cleanup can evict only Actions that
+	// are older than maxAge, and the cache can grow past maxSize without
+	// limit.
+	//
+	// overAge comes from the ActionsLE histograms, and eviction does not
+	// update them. Thus overAge can stay true until the stats loop scans
+	// the shards again (up to shardStalenessTarget), after the last
+	// over-age Actions are gone. If the age pass evicts nothing, do not
+	// trust overAge. Continue to the size pass.
+	overAge := srv.maxAge > 0 && us.All().Count-us.ActionsLE[srv.maxAge].Count > 0
 	overSize := srv.maxSize > 0 && all.Size > srv.maxSize
-	if !overAge && !overSize {
-		return ret, nil
+	if overAge {
+		// maxAge cleanup runs to the count limit, because all stale
+		// Actions must go.
+		cutoff := srv.now().Add(-srv.maxAge).Unix()
+		var err error
+		ret, err = srv.evictOldestActions(ctx, cutoff, cleanupBatchSize, math.MaxInt64)
+		if err != nil || ret.Count > 0 || !overSize {
+			return ret, err
+		}
 	}
-	// maxAge cleanup runs to count limit (every stale Action must go);
-	// size-only cleanup runs until we've reclaimed enough bytes.
-	maxBytes := int64(math.MaxInt64)
-	if !overAge && overSize {
-		maxBytes = all.Size - srv.maxSize
+	if !overSize {
+		return ret, nil
 	}
-	return srv.evictOldestActions(ctx, cutoff, cleanupBatchSize, maxBytes)
+	// Size-only cleanup runs until it frees enough bytes. evictOldestActions
+	// still uses idx_actions_access (the WHERE/ORDER BY drive the plan), so
+	// the very-large cutoff means "no upper bound".
+	return srv.evictOldestActions(ctx, math.MaxInt64, cleanupBatchSize, all.Size-srv.maxSize)
 }
 
 // checkpointTruncate runs PRAGMA wal_checkpoint(TRUNCATE) and returns SQLite's

gocache/gocached/gocached_test.go

@@ -805,6 +805,111 @@ func TestCleanOldObjectsBySize(t *testing.T) {
 	}
 }
 
+// TestCleanOldObjectsBySizeWithMaxAge checks that size-based cleanup can
+// evict Actions that are younger than maxAge. The cmd/gocached binary sets a
+// maxAge by default, so this is the production configuration.
+func TestCleanOldObjectsBySizeWithMaxAge(t *testing.T) {
+	const day = 24 * time.Hour
+	st := newServerTester(t, WithMaxAge(60*day))
+
+	c1 := st.mkClient()
+	st.wantPut(c1, "0001", "9901", "1")
+	st.advanceClock(time.Second)
+	st.wantPut(c1, "0002", "9902", "22")
+	st.advanceClock(time.Second)
+	st.wantPut(c1, "0003", "9903", "333")
+	st.advanceClock(time.Second)
+	st.wantPut(c1, "0004", "9904", "4444")
+	st.advanceClock(time.Second)
+
+	if got := st.cleanOldObjects(); got != (countAndSize{}) {
+		t.Errorf("cleanOldObjects got %v, want no clean", got)
+	}
+
+	// All Actions are younger than maxAge, so only size-based cleanup
+	// applies. The only way to get to 8 bytes or less is to delete "1" and
+	// "22" (3 bytes).
+	st.srv.maxSize = 8
+	if got, want := st.cleanOldObjects(), (countAndSize{Count: 2, Size: 3}); got != want {
+		t.Errorf("cleanOldObjects got %v, want %v", got, want)
+	}
+	if got, want := st.usageStats().All(), (countAndSize{Count: 2, Size: 7}); got != want {
+		t.Errorf("usageStats: %v; want %v", got, want)
+	}
+
+	// Make "333" and "4444" older than maxAge. Then add young data so that
+	// the cache is over both limits. Age-based cleanup removes "333" and
+	// "4444" (7 bytes). The remaining 11 bytes are still more than 8, so
+	// size-based cleanup then removes "55555", which is younger than maxAge.
+	st.advanceClock(61 * day)
+	st.wantPut(c1, "0005", "9905", "55555")
+	st.advanceClock(time.Second)
+	st.wantPut(c1, "0006", "9906", "666666")
+	if got, want := st.cleanOldObjects(), (countAndSize{Count: 3, Size: 12}); got != want {
+		t.Errorf("cleanOldObjects got %v, want %v", got, want)
+	}
+	if got, want := st.usageStats().All(), (countAndSize{Count: 1, Size: 6}); got != want {
+		t.Errorf("usageStats: %v; want %v", got, want)
+	}
+}
+
+// TestCleanupBySizeWithStaleAgeStats checks that size-based cleanup runs when
+// the ActionsLE histograms still show over-age Actions that eviction has
+// already removed. The stats loop scans each shard again only after
+// shardStalenessTarget, so in production the histograms can be stale for up
+// to that duration. This test calls cleanupTick directly and does not scan
+// the shards between ticks.
+func TestCleanupBySizeWithStaleAgeStats(t *testing.T) {
+	const day = 24 * time.Hour
+	st := newServerTester(t, WithMaxAge(60*day))
+	st.srv.maxSize = 8
+	ctx := context.Background()
+
+	c1 := st.mkClient()
+	st.wantPut(c1, "0001", "9901", "1")
+	st.advanceClock(time.Second)
+	st.wantPut(c1, "0002", "9902", "22")
+	st.advanceClock(61 * day)
+	st.wantPut(c1, "0003", "9903", "333")
+	st.advanceClock(time.Second)
+	st.wantPut(c1, "0004", "9904", "4444")
+	st.advanceClock(time.Second)
+	st.wantPut(c1, "0005", "9905", "55555")
+	st.advanceClock(time.Second)
+
+	// Scan the shards one time. After this, the histograms stay as they are.
+	_ = st.usageStats()
+
+	// The first tick removes the over-age Actions "1" and "22".
+	res, err := st.srv.cleanupTick(ctx)
+	if err != nil {
+		t.Fatalf("cleanupTick: %v", err)
+	}
+	if want := (countAndSize{Count: 2, Size: 3}); res != want {
+		t.Errorf("first cleanupTick got %v, want %v", res, want)
+	}
+
+	// Make sure that the histograms are stale. If they are not, this test
+	// does not examine the stale case.
+	us := st.srv.lastUsage.Load()
+	if n := us.All().Count - us.ActionsLE[st.srv.maxAge].Count; n == 0 {
+		t.Fatalf("lastUsage shows no over-age Actions; want stale histograms")
+	}
+
+	// The cache has 12 bytes, which is more than 8. The second tick must do
+	// size-based cleanup. It removes "333" and "4444" (7 bytes).
+	res, err = st.srv.cleanupTick(ctx)
+	if err != nil {
+		t.Fatalf("cleanupTick: %v", err)
+	}
+	if want := (countAndSize{Count: 2, Size: 7}); res != want {
+		t.Errorf("second cleanupTick got %v, want %v", res, want)
+	}
+	if got, want := st.usageStats().All(), (countAndSize{Count: 1, Size: 5}); got != want {
+		t.Errorf("usageStats: %v; want %v", got, want)
+	}
+}
+
 func TestLZ4Storage(t *testing.T) {
 	st := newServerTester(t)
 	c := st.mkClient()