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

@@ -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")
+}

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()

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
 

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)

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)
+}

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() {

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()
 }
 

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) {

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,