diff --git a/gocache/gocached/gocached.go b/gocache/gocached/gocached.go index 68e47cb..3cfa704 100644 --- a/gocache/gocached/gocached.go +++ b/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 diff --git a/gocache/gocached/gocached_test.go b/gocache/gocached/gocached_test.go index 30ee848..e2811c7 100644 --- a/gocache/gocached/gocached_test.go +++ b/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()