From 95f72b0b1a5629d67622095525948a9948f873d6 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 14 Aug 2026 01:41:43 +0100 Subject: [PATCH] fix(cache): make memory Clear counter accounting race-safe Clear reset the counters with StoreInt64(0) after deleting entries, so a Set racing the reset left a live entry while counters read zero, and the next Delete drove them permanently negative. Rewrite Clear to LoadAndDelete each key and subtract the linearized returned entry, matching the rest of the file. Size the key slice from the live entry count. --- cache/memory/memory.go | 37 ++++++++++-- cache/memory/memory_additional_test.go | 82 ++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 4 deletions(-) diff --git a/cache/memory/memory.go b/cache/memory/memory.go index 8951b01..48a1f46 100644 --- a/cache/memory/memory.go +++ b/cache/memory/memory.go @@ -221,13 +221,42 @@ func (c *Cache) Delete(key string) { } } +// Clear removes all entries from the cache and adjusts the entry/memory +// counters to match exactly what was removed. +// +// Clear does not promise a consistent point-in-time snapshot: it takes the +// key list first, so an entry Set concurrently with Clear can survive it. +// The reverse can also happen: a concurrent Set that overwrites a key +// already collected by the Range pass can itself be deleted by Clear, +// because LoadAndDelete removes whatever mapping is current for that key +// when it runs, not necessarily the one Range observed. +// +// What Clear does guarantee is counter exactness. It LoadAndDeletes each +// collected key and subtracts exactly the MemorySize that LoadAndDelete +// returned, the same linearized pattern Delete/Get/CleanExpiredEntries/the +// evictors use, instead of the previous unconditional Delete followed by +// StoreInt64(0). That previous approach could race with a concurrent Set +// landing after the Range pass but before or after the StoreInt64 calls, +// leaving a live entry in the map while the counters read zero. The next +// Delete of that entry would then drive the counters permanently negative. func (c *Cache) Clear() { - c.entries.Range(func(key, value any) bool { - c.entries.Delete(key) + n := atomic.LoadInt64(&c.entryCount) + if n < 0 { + n = 0 + } + keys := make([]string, 0, n) + c.entries.Range(func(key, _ any) bool { + keys = append(keys, key.(string)) return true }) - atomic.StoreInt64(&c.entryCount, 0) - atomic.StoreInt64(&c.memoryUsage, 0) + + for _, key := range keys { + if removed, exists := c.entries.LoadAndDelete(key); exists { + removedEntry := removed.(CacheEntry) + atomic.AddInt64(&c.entryCount, -1) + atomic.AddInt64(&c.memoryUsage, -removedEntry.MemorySize) + } + } } func (c *Cache) CountQueries() int64 { diff --git a/cache/memory/memory_additional_test.go b/cache/memory/memory_additional_test.go index 872f4f0..df9e2ad 100644 --- a/cache/memory/memory_additional_test.go +++ b/cache/memory/memory_additional_test.go @@ -238,6 +238,88 @@ func TestMemoryCacheConcurrentSetDeleteEvict_CountersMatchActualState(t *testing } } +func TestMemoryCacheConcurrentSetDeleteClear_CountersMatchActualState(t *testing.T) { + // Regression for Clear(): the previous implementation Ranged the + // sync.Map calling Delete per key, then unconditionally reset both + // counters with StoreInt64(0). A Set racing Clear could land after the + // Range pass but before or after the StoreInt64 calls, leaving a live + // entry in the map while the counters read zero - the next Delete of + // that key then drove the counters permanently negative. + // + // Clear now follows the same linearized LoadAndDelete-then-subtract + // pattern as Delete/Get/CleanExpiredEntries/the evictors, so it no + // longer promises a consistent point-in-time snapshot (a concurrent Set + // can survive a Clear) but does guarantee entryCount/memoryUsage always + // match the entries actually removed. As in + // TestMemoryCacheConcurrentSetDeleteEvict_CountersMatchActualState, + // which key wins a given race is nondeterministic, so the assertion + // recomputes "expected" from the final map contents rather than + // asserting a fixed number. + const ( + numKeys = 50 + numWorkers = 16 + numOpsEach = 300 + ) + + cache := NewWithSize(DefaultTestExpiration, DefaultMaxMemorySize, DefaultMaxCacheSize) + defer cache.Shutdown() + + keyFor := func(i int) string { return fmt.Sprintf("clear-key-%d", i) } + + // Pre-seed so Clear and the Delete workers have entries to act on from + // the start. + for i := range numKeys { + cache.Set(keyFor(i), []byte("seed-value"), DefaultTestExpiration) + } + + var wg sync.WaitGroup + for w := range numWorkers { + wg.Add(1) + go func(worker int) { + defer wg.Done() + for op := range numOpsEach { + k := keyFor((worker + op) % numKeys) + switch op % 2 { + case 0: + cache.Set(k, []byte("value"), DefaultTestExpiration) + case 1: + cache.Delete(k) + } + } + }(w) + } + + wg.Add(1) + go func() { + defer wg.Done() + for range 100 { + cache.Clear() + } + }() + + wg.Wait() + + // Stop the background cleanup goroutine before recomputing expected + // state, so it cannot race with the Range below (nothing has expired + // yet - DefaultTestExpiration is 5s - but this removes any doubt). + cache.Shutdown() + + var wantCount, wantMemory int64 + cache.entries.Range(func(_, v any) bool { + entry := v.(CacheEntry) + wantCount++ + wantMemory += entry.MemorySize + return true + }) + + if got := cache.CountQueries(); got != wantCount { + t.Fatalf("entryCount = %d, want %d (actual live entries after join)", got, wantCount) + } + if got := cache.GetMemoryUsage(); got != wantMemory { + t.Fatalf("memoryUsage = %d, want %d (sum of live entries' MemorySize after join)", got, wantMemory) + } +} + func TestMemoryCacheClose_IdempotentAndStillUsable(t *testing.T) { // Compile-time guard: the memory backend must satisfy io.Closer so // cache.Shutdown() can stop its cleanup goroutine (it otherwise leaks