From 159faa68e0b6a0841641dc2c50263495471db877 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 14 Aug 2026 11:09:57 +0100 Subject: [PATCH] perf(cache): sort-based eviction and consistent non-positive TTL handling The count-based evictOldest used an O(n squared) selection sort with a candidate cap over an unordered map range, so it could evict the wrong entries. It now collects all entries and sorts by expiry, matching the memory-based evictor. A non-positive CACHE_TTL behaved differently per backend: the standard memory cache panicked, Redis stored a permanent key, and the LRU cache expired entries immediately. Each backend now clamps a non-positive TTL to a shared default at its Set boundary. The nil-logger fallback in the Redis wrapper now uses the constructor so it cannot panic. --- cache/memory/eviction_test.go | 100 +++++++++++++++++++++++++ cache/memory/lru_memory_cache.go | 8 ++ cache/memory/lru_memory_cache_test.go | 26 +++++++ cache/memory/memory.go | 75 ++++++++++++++----- cache/memory/memory_additional_test.go | 38 ++++++++++ cache/memory/memory_test.go | 27 ++++++- cache/redis/redis.go | 18 +++++ cache/redis/redis_coverage_test.go | 36 +++++++-- cache/redis/wrapper.go | 5 +- 9 files changed, 308 insertions(+), 25 deletions(-) diff --git a/cache/memory/eviction_test.go b/cache/memory/eviction_test.go index 94a31c2..03bcab2 100644 --- a/cache/memory/eviction_test.go +++ b/cache/memory/eviction_test.go @@ -2,6 +2,7 @@ package libpack_cache_memory import ( "fmt" + "sync" "testing" "time" @@ -212,3 +213,102 @@ func TestEvictToFreeMemoryWithSmallMaxEntriesReachesTarget(t *testing.T) { assert.Equal(t, int64(0), cache.GetMemoryUsage(), "an oversized request should evict every entry") } + +// TestEvictOldest_EvictsCorrectOldestEntries is a determinism regression for +// P2: evictOldest used to selection-sort only a capped subset (n*2) of +// sync.Map's unordered Range, which could miss the true oldest entries +// entirely and evict the wrong ones. This verifies it evicts exactly the n +// entries with the earliest ExpiresAt, matching evictToFreeMemory's +// collect-all-then-sort.Slice approach. +func TestEvictOldest_EvictsCorrectOldestEntries(t *testing.T) { + cache := NewWithSize(5*time.Second, DefaultMaxMemorySize, DefaultMaxCacheSize) + + const total = 50 + keys := make([]string, total) + for i := 0; i < total; i++ { + key := fmt.Sprintf("key-%03d", i) + keys[i] = key + // Distinct, strictly increasing TTLs give distinct, strictly + // increasing ExpiresAt without depending on wall-clock sleeps between + // insertions. + cache.Set(key, []byte("v"), time.Duration(i+1)*time.Minute) + } + assert.Equal(t, int64(total), cache.CountQueries()) + + const evictCount = 20 + cache.evictOldest(evictCount) + + assert.Equal(t, int64(total-evictCount), cache.CountQueries()) + + // The evictCount entries with the smallest ExpiresAt (keys[0:evictCount]) + // must be gone; the remaining, longer-lived entries must all survive. + for i := 0; i < evictCount; i++ { + _, found := cache.Get(keys[i]) + assert.False(t, found, "oldest key %s should have been evicted", keys[i]) + } + for i := evictCount; i < total; i++ { + _, found := cache.Get(keys[i]) + assert.True(t, found, "newer key %s should survive eviction", keys[i]) + } +} + +// TestEvictOldest_MoreThanAvailable_EvictsAllWithoutPanic verifies evictOldest +// clamps n to the number of live entries instead of indexing past the sorted +// slice. +func TestEvictOldest_MoreThanAvailable_EvictsAllWithoutPanic(t *testing.T) { + cache := NewWithSize(5*time.Second, DefaultMaxMemorySize, DefaultMaxCacheSize) + for i := 0; i < 5; i++ { + cache.Set(fmt.Sprintf("k%d", i), []byte("v"), time.Minute) + } + + cache.evictOldest(1000) + + assert.Equal(t, int64(0), cache.CountQueries()) + assert.Equal(t, int64(0), cache.GetMemoryUsage()) +} + +// TestEvictOldest_ConcurrentHammer_StaysConsistent drives Set/Get/Delete from +// many goroutines against a small maxCacheSize, which forces the count-based +// eviction path (evictOldest) to run repeatedly and concurrently. Run with +// -race: the entryCount/memoryUsage counters must stay lock-free-consistent +// (never negative, always matching the live map) throughout. +func TestEvictOldest_ConcurrentHammer_StaysConsistent(t *testing.T) { + const maxEntries = 50 + cache := NewWithSize(5*time.Second, DefaultMaxMemorySize, maxEntries) + + const goroutines = 16 + const opsPerGoroutine = 200 + + var wg sync.WaitGroup + wg.Add(goroutines) + for g := 0; g < goroutines; g++ { + go func(g int) { + defer wg.Done() + for i := 0; i < opsPerGoroutine; i++ { + key := fmt.Sprintf("g%d-k%d", g, i%20) + cache.Set(key, []byte("v"), time.Duration(i%10+1)*time.Second) + cache.Get(key) + if i%7 == 0 { + cache.Delete(key) + } + } + }(g) + } + wg.Wait() + + // Counters must never go negative. + assert.GreaterOrEqual(t, cache.CountQueries(), int64(0)) + assert.GreaterOrEqual(t, cache.GetMemoryUsage(), int64(0)) + + // Counters must match the live map contents exactly. + var actualCount, actualMemory int64 + cache.entries.Range(func(_, v any) bool { + actualCount++ + actualMemory += v.(CacheEntry).MemorySize + return true + }) + assert.Equal(t, actualCount, cache.CountQueries(), + "entryCount must match the number of live entries after concurrent Set/evictOldest") + assert.Equal(t, actualMemory, cache.GetMemoryUsage(), + "memoryUsage must match the sum of live entries' MemorySize after concurrent Set/evictOldest") +} diff --git a/cache/memory/lru_memory_cache.go b/cache/memory/lru_memory_cache.go index c4a4494..5f9ada8 100644 --- a/cache/memory/lru_memory_cache.go +++ b/cache/memory/lru_memory_cache.go @@ -65,6 +65,14 @@ func (c *LRUMemoryCache) Set(key string, value []byte, ttl time.Duration) { } entrySize := int64(len(key) + len(finalValue) + 64) // 64 bytes overhead estimate + + // A non-positive TTL is clamped to defaultTTL (defined in memory.go) so + // this backend does not stamp the entry as already-expired (immediate + // miss on every read). Keeps behavior identical to the standard memory + // backend and Redis, which clamp the same way at their own Set boundary. + if ttl <= 0 { + ttl = defaultTTL + } expiresAt := time.Now().Add(ttl) c.mu.Lock() diff --git a/cache/memory/lru_memory_cache_test.go b/cache/memory/lru_memory_cache_test.go index 011e2f6..732d6f9 100644 --- a/cache/memory/lru_memory_cache_test.go +++ b/cache/memory/lru_memory_cache_test.go @@ -106,6 +106,32 @@ func (suite *LRUMemoryCacheTestSuite) TestExpiration() { suite.Nil(val) } +// TestNonPositiveTTL_ClampedToDefaultTTL verifies the C11 cross-backend +// rule: a non-positive TTL is clamped to defaultTTL instead of stamping the +// entry as already-expired (which used to make every read miss +// immediately, unlike the Redis backend which used to keep the entry +// forever). +func (suite *LRUMemoryCacheTestSuite) TestNonPositiveTTL_ClampedToDefaultTTL() { + cache := NewLRUMemoryCache(1024*1024, 100) + + cache.Set("zero-ttl", []byte("value"), 0) + val, found := cache.Get("zero-ttl") + suite.True(found, "zero TTL should be clamped to defaultTTL, not stored as already-expired") + suite.Equal([]byte("value"), val) + + cache.Set("negative-ttl", []byte("value"), -1*time.Second) + val, found = cache.Get("negative-ttl") + suite.True(found, "negative TTL should be clamped to defaultTTL, not stored as already-expired") + suite.Equal([]byte("value"), val) + + cache.mu.RLock() + entry, ok := cache.entries["zero-ttl"] + cache.mu.RUnlock() + suite.Require().True(ok) + suite.WithinDuration(time.Now().Add(defaultTTL), entry.expiresAt, 2*time.Second, + "clamped entry should expire ~defaultTTL from now") +} + func (suite *LRUMemoryCacheTestSuite) TestEvictionByCount() { cache := NewLRUMemoryCache(1024*1024, 3) // Max 3 entries diff --git a/cache/memory/memory.go b/cache/memory/memory.go index 48a1f46..c5e36c3 100644 --- a/cache/memory/memory.go +++ b/cache/memory/memory.go @@ -28,6 +28,29 @@ const DefaultMaxCacheSize = 10000 // This accounts for the CacheEntry struct overhead, map entry, and synchronization const approxEntryOverhead = 64 +// defaultTTL is the sane fallback applied whenever a configured or per-entry +// TTL is non-positive (<=0). +// +// It serves two purposes: +// 1. Sizing the background cleanup ticker (cleanupRoutine): time.NewTicker +// panics for a non-positive duration, so a non-positive globalTTL is +// replaced with defaultTTL before the ticker is created. +// 2. Clamping a non-positive per-entry TTL passed to Set: time.Now().Add(ttl) +// with ttl<=0 stamps an entry as already expired, so every read would +// miss immediately. +// +// Cross-backend TTL rule (see also cache/memory/lru_memory_cache.go and +// cache/redis/redis.go, which each define and apply their own equivalent +// constant at their Set boundary): a non-positive TTL is always clamped to +// defaultTTL before being applied, for every backend. This keeps behavior +// identical regardless of backend. Without this rule, the three backends +// diverge: go-redis treats expiration<=0 as "no EX/PX option", i.e. the key +// never expires, the LRU backend stamps the entry as already expired so every +// read misses, and the standard (this) backend would otherwise panic in +// time.NewTicker. None of those are acceptable, so all three backends clamp +// to the same bounded default instead. +const defaultTTL = 60 * time.Second + type CacheEntry struct { ExpiresAt time.Time Value []byte @@ -82,6 +105,13 @@ func NewWithSize(globalTTL time.Duration, maxMemorySize int64, maxCacheSize int6 } func (c *Cache) cleanupRoutine(globalTTL time.Duration) { + // time.NewTicker panics for a non-positive duration. A non-positive + // globalTTL (e.g. a misconfigured CACHE_TTL<=0) must not crash the + // process, so fall back to defaultTTL before deriving the interval. + if globalTTL <= 0 { + globalTTL = defaultTTL + } + // Clean up more frequently when the cache is large ticker := time.NewTicker(globalTTL / 4) defer ticker.Stop() @@ -131,6 +161,12 @@ func (c *Cache) Set(key string, value []byte, ttl time.Duration) { c.evictOldest(int(c.maxCacheSize / 10)) // Evict 10% of entries } + // A non-positive TTL is clamped to defaultTTL so this backend expires + // entries the same way the LRU and Redis backends do. See the defaultTTL + // doc comment above for the cross-backend rule. + if ttl <= 0 { + ttl = defaultTTL + } expiresAt := time.Now().Add(ttl) // Only compress if the value is larger than the threshold @@ -319,37 +355,42 @@ func (c *Cache) CleanExpiredEntries() { }) } -// evictOldest removes the oldest n entries from the cache +// evictOldest removes the oldest n entries from the cache, oldest by +// ExpiresAt first. It collects every live entry and sorts once with +// sort.Slice (O(n log n)), the same approach evictToFreeMemory below uses, +// instead of a nested-loop selection sort (O(n^2)). +// +// Collecting ALL entries (not a capped subset) matters for correctness, not +// just performance: sync.Map.Range iterates in unspecified order, so an +// early-terminated scan over an arbitrary subset can miss the true oldest +// keys entirely and evict the wrong entries. func (c *Cache) evictOldest(n int) { type keyExpiry struct { expiresAt time.Time key string } - // Collect all entries with their expiry times - entries := make([]keyExpiry, 0, n*2) + liveEntries := atomic.LoadInt64(&c.entryCount) + if liveEntries < 0 { + liveEntries = 0 + } + entries := make([]keyExpiry, 0, liveEntries) c.entries.Range(func(k, v any) bool { key := k.(string) entry := v.(CacheEntry) entries = append(entries, keyExpiry{entry.ExpiresAt, key}) - return len(entries) < cap(entries) + return true }) // Sort by expiry time (oldest first) - // Using a simple selection sort since we only need to find the n oldest - for i := 0; i < n && i < len(entries); i++ { - oldest := i - for j := i + 1; j < len(entries); j++ { - if entries[j].expiresAt.Before(entries[oldest].expiresAt) { - oldest = j - } - } - // Swap - if oldest != i { - entries[i], entries[oldest] = entries[oldest], entries[i] - } + sort.Slice(entries, func(i, j int) bool { + return entries[i].expiresAt.Before(entries[j].expiresAt) + }) - // Delete this entry + if n > len(entries) { + n = len(entries) + } + for i := 0; i < n; i++ { if entry, exists := c.entries.LoadAndDelete(entries[i].key); exists { cacheEntry := entry.(CacheEntry) atomic.AddInt64(&c.entryCount, -1) diff --git a/cache/memory/memory_additional_test.go b/cache/memory/memory_additional_test.go index df9e2ad..1dd53e5 100644 --- a/cache/memory/memory_additional_test.go +++ b/cache/memory/memory_additional_test.go @@ -346,3 +346,41 @@ func TestMemoryCacheClose_IdempotentAndStillUsable(t *testing.T) { t.Fatal("cache should remain writable after Close") } } + +// TestNewWithSize_NonPositiveTTL_DoesNotPanic is a regression test for C1: a +// non-positive globalTTL (e.g. a misconfigured CACHE_TTL<=0) used to reach +// time.NewTicker(globalTTL/4) unguarded inside cleanupRoutine, and +// time.NewTicker panics for d<=0. That panic happens in the background +// goroutine spawned by NewWithSize, so a regression here crashes the whole +// process, not just this test. +func TestNewWithSize_NonPositiveTTL_DoesNotPanic(t *testing.T) { + tests := []struct { + name string + ttl time.Duration + }{ + {"zero TTL", 0}, + {"negative TTL", -5 * time.Second}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cache := NewWithSize(tt.ttl, DefaultMaxMemorySize, DefaultMaxCacheSize) + defer cache.Shutdown() + + // Give the background cleanup goroutine a moment to construct its + // ticker. + time.Sleep(20 * time.Millisecond) + + // Cache must remain fully usable. + cache.Set("key", []byte("value"), DefaultTestExpiration) + retrieved, found := cache.Get("key") + assert.True(t, found, "cache should be usable after construction with a non-positive TTL") + assert.Equal(t, []byte("value"), retrieved) + }) + } +} + +// TestDefaultTTLValue documents the sane fallback used both to size the +// cleanup ticker (C1) and to clamp a non-positive per-entry TTL (C11). +func TestDefaultTTLValue(t *testing.T) { + assert.Equal(t, 60*time.Second, defaultTTL) +} diff --git a/cache/memory/memory_test.go b/cache/memory/memory_test.go index 948091c..d175399 100644 --- a/cache/memory/memory_test.go +++ b/cache/memory/memory_test.go @@ -150,12 +150,35 @@ func (suite *MemoryTestSuite) Test_LargeItems() { suite.Assert().Equal(largeValue, retrieved) } +// Test_ZeroTTL verifies the C11 cross-backend rule: a non-positive TTL is +// clamped to defaultTTL instead of being stored as already-expired. Before +// this fix, Set("zero-ttl", ..., 0) stamped ExpiresAt at time.Now(), so the +// very next Get always missed. func (suite *MemoryTestSuite) Test_ZeroTTL() { cache := New(5 * time.Second) cache.Set("zero-ttl", []byte("value"), 0) - _, found := cache.Get("zero-ttl") - suite.Assert().False(found, "Item with zero TTL should not be stored") + retrieved, found := cache.Get("zero-ttl") + suite.Assert().True(found, "zero TTL should be clamped to defaultTTL, not stored as already-expired") + suite.Assert().Equal([]byte("value"), retrieved) + + raw, ok := cache.entries.Load("zero-ttl") + suite.Require().True(ok) + entry, ok := raw.(CacheEntry) + suite.Require().True(ok) + suite.Assert().WithinDuration(time.Now().Add(defaultTTL), entry.ExpiresAt, 2*time.Second, + "clamped entry should expire ~defaultTTL from now") +} + +// Test_NegativeTTL verifies the same clamp applies to a negative TTL, not +// just exactly zero. +func (suite *MemoryTestSuite) Test_NegativeTTL() { + cache := New(5 * time.Second) + cache.Set("negative-ttl", []byte("value"), -1*time.Second) + + retrieved, found := cache.Get("negative-ttl") + suite.Assert().True(found, "negative TTL should be clamped to defaultTTL, not stored as already-expired") + suite.Assert().Equal([]byte("value"), retrieved) } func (suite *MemoryTestSuite) Test_LongTTL() { diff --git a/cache/redis/redis.go b/cache/redis/redis.go index 3afccc0..ec8996b 100644 --- a/cache/redis/redis.go +++ b/cache/redis/redis.go @@ -65,6 +65,21 @@ type RedisClientConfig struct { RedisDB int } +// defaultTTL is the sane fallback applied to a non-positive per-entry TTL +// passed to Set. +// +// go-redis treats a non-positive expiration as "send SET with no EX/PX +// option", which makes the key permanent (see redis/go-redis/v9 +// string_commands.go: `if expiration > 0 { ... }`, no else branch adds an +// expiry). That would silently diverge from the in-memory backends: the +// standard memory cache and the LRU cache both clamp a non-positive TTL to +// their own equivalent defaultTTL constant (cache/memory/memory.go and +// cache/memory/lru_memory_cache.go) so the entry still expires. Clamping here +// too keeps all three backends' behavior identical: a misconfigured +// (non-positive) TTL always results in a bounded expiry, never a permanent +// entry. +const defaultTTL = 60 * time.Second + func New(redisClientConfig *RedisClientConfig) (*RedisConfig, error) { c := &RedisConfig{ client: redis.NewClient(&redis.Options{ @@ -89,6 +104,9 @@ func New(redisClientConfig *RedisClientConfig) (*RedisConfig, error) { } func (c *RedisConfig) Set(key string, value []byte, ttl time.Duration) error { + if ttl <= 0 { + ttl = defaultTTL + } return c.client.Set(c.ctx, c.prependKeyName(key), value, ttl).Err() } diff --git a/cache/redis/redis_coverage_test.go b/cache/redis/redis_coverage_test.go index c30638d..156df55 100644 --- a/cache/redis/redis_coverage_test.go +++ b/cache/redis/redis_coverage_test.go @@ -134,14 +134,40 @@ func TestGet_MissingKey_ReturnsFalseNoError(t *testing.T) { assert.Nil(t, val) } -func TestSet_TTLZero_KeyPersists(t *testing.T) { +// TestSet_TTLZero_ClampedToDefaultTTL verifies the C11 cross-backend rule: a +// non-positive TTL is clamped to defaultTTL instead of go-redis's native +// behavior of sending SET with no EX/PX option (which makes the key +// permanent). This keeps Redis's expiry semantics identical to the memory +// and LRU backends instead of silently diverging into "never expires". +func TestSet_TTLZero_ClampedToDefaultTTL(t *testing.T) { t.Parallel() rc, s := newTestRedis(t) - require.NoError(t, rc.Set("persist", []byte("yes"), 0)) - s.FastForward(24 * time.Hour) - _, found, err := rc.Get("persist") + require.NoError(t, rc.Set("clamped", []byte("yes"), 0)) + + // Still present before defaultTTL elapses. + s.FastForward(defaultTTL - 5*time.Second) + _, found, err := rc.Get("clamped") assert.NoError(t, err) - assert.True(t, found) + assert.True(t, found, "clamped entry should still be present before defaultTTL elapses") + + // Gone once defaultTTL elapses -- i.e. it is bounded, not permanent. + s.FastForward(10 * time.Second) + _, found, err = rc.Get("clamped") + assert.NoError(t, err) + assert.False(t, found, "zero TTL should expire after defaultTTL, not persist forever") +} + +// TestSet_NegativeTTL_ClampedToDefaultTTL verifies the same clamp applies to +// a negative TTL, not just exactly zero. +func TestSet_NegativeTTL_ClampedToDefaultTTL(t *testing.T) { + t.Parallel() + rc, s := newTestRedis(t) + require.NoError(t, rc.Set("neg", []byte("yes"), -1*time.Second)) + + s.FastForward(defaultTTL + 5*time.Second) + _, found, err := rc.Get("neg") + assert.NoError(t, err) + assert.False(t, found, "negative TTL should expire after defaultTTL, not persist forever") } func TestSet_WithTTL_KeyExpires(t *testing.T) { diff --git a/cache/redis/wrapper.go b/cache/redis/wrapper.go index 20cfc4e..739aa49 100644 --- a/cache/redis/wrapper.go +++ b/cache/redis/wrapper.go @@ -16,7 +16,10 @@ type CacheWrapper struct { // NewCacheWrapper creates a new cache wrapper func NewCacheWrapper(config *RedisConfig, logger *libpack_logger.Logger) *CacheWrapper { if logger == nil { - logger = &libpack_logger.Logger{} + // Use the proper constructor, not a zero-value &Logger{} -- its + // timeFormat field is a nil atomic.Pointer, so a zero-value Logger + // panics on the first log call instead of degrading gracefully. + logger = libpack_logger.New() } return &CacheWrapper{ redis: config,