From 2778b9ccbb403b84aebc743c8335cae80c13ba46 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Thu, 10 Sep 2026 21:19:16 +0300 Subject: [PATCH] =?UTF-8?q?fix:=20RateLimit=20memory=20leak=20regression?= =?UTF-8?q?=20=E2=80=94=20proper=20LRU=20via=20container/list?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REGRESSION in v0.5.2: insertOrd slice grew unbounded because Cleanup() removed keys from map but not from insertOrd. Replaced with proper LRU using container/list + map[string]*list.Element. - GetLimiter: MoveToBack on access (true LRU, not FIFO) - Cleanup: removes from both map and LRU list - evictLRU: O(1) via list.Front + Remove - No stale entries, no duplicate keys - TDD: 2 regression tests (leak repro, duplicate key eviction) --- middleware/ratelimit.go | 51 +++++++++------ middleware/ratelimit_regression_test.go | 85 +++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 19 deletions(-) create mode 100644 middleware/ratelimit_regression_test.go diff --git a/middleware/ratelimit.go b/middleware/ratelimit.go index d069c8d..89ecca8 100644 --- a/middleware/ratelimit.go +++ b/middleware/ratelimit.go @@ -6,6 +6,7 @@ package middleware import ( + "container/list" "fmt" "net/http" "sync" @@ -90,16 +91,19 @@ type RateLimitStore interface { } // inMemoryStore is the default in-memory store for rate limiters. +// Uses container/list for true LRU eviction with O(1) operations. type inMemoryStore struct { limiters map[string]*limiterEntry + lruList *list.List // doubly-linked list for LRU order + lruIndex map[string]*list.Element // key → list element for O(1) lookup mu sync.RWMutex maxKeys int - insertOrd []string // insertion-order list for O(1) eviction - cleanupOnce sync.Once // ensures at most one cleanup goroutine + cleanupOnce sync.Once } // limiterEntry stores a rate limiter with its last access time. type limiterEntry struct { + key string limiter *rate.Limiter lastAccess time.Time } @@ -127,6 +131,8 @@ func newInMemoryStore(maxKeys int) *inMemoryStore { return &inMemoryStore{ limiters: make(map[string]*limiterEntry), + lruList: list.New(), + lruIndex: make(map[string]*list.Element), maxKeys: maxKeys, } } @@ -136,41 +142,44 @@ func (s *inMemoryStore) GetLimiter(key string, r rate.Limit, burst int) *rate.Li s.mu.Lock() defer s.mu.Unlock() - // Check if limiter exists. + // Check if limiter exists — move to back (most recently used). if entry, ok := s.limiters[key]; ok { entry.lastAccess = time.Now() + if elem, ok := s.lruIndex[key]; ok { + s.lruList.MoveToBack(elem) + } return entry.limiter } - // Evict oldest entries until under maxKeys (O(1) amortized via insertion-order list). + // Evict least recently used entries until under maxKeys. for s.maxKeys > 0 && len(s.limiters) >= s.maxKeys { - s.evictOldest() + s.evictLRU() } // Create new limiter. limiter := rate.NewLimiter(r, burst) - s.limiters[key] = &limiterEntry{ + entry := &limiterEntry{ + key: key, limiter: limiter, lastAccess: time.Now(), } - s.insertOrd = append(s.insertOrd, key) + s.limiters[key] = entry + elem := s.lruList.PushBack(key) + s.lruIndex[key] = elem return limiter } -// evictOldest removes the oldest entry by insertion order. -// O(1) amortized: pops from front of insertOrd, skipping already-deleted keys. -func (s *inMemoryStore) evictOldest() { - for len(s.insertOrd) > 0 { - key := s.insertOrd[0] - s.insertOrd = s.insertOrd[1:] - - if _, ok := s.limiters[key]; ok { - delete(s.limiters, key) - return - } - // Key was already removed by Cleanup; skip and try next. +// evictLRU removes the least recently used entry. O(1). +func (s *inMemoryStore) evictLRU() { + front := s.lruList.Front() + if front == nil { + return } + key := front.Value.(string) + s.lruList.Remove(front) + delete(s.lruIndex, key) + delete(s.limiters, key) } // Cleanup removes expired limiters. @@ -182,6 +191,10 @@ func (s *inMemoryStore) Cleanup(expireAfter time.Duration) { for key, entry := range s.limiters { if now.Sub(entry.lastAccess) > expireAfter { delete(s.limiters, key) + if elem, ok := s.lruIndex[key]; ok { + s.lruList.Remove(elem) + delete(s.lruIndex, key) + } } } } diff --git a/middleware/ratelimit_regression_test.go b/middleware/ratelimit_regression_test.go new file mode 100644 index 0000000..26d09dc --- /dev/null +++ b/middleware/ratelimit_regression_test.go @@ -0,0 +1,85 @@ +package middleware + +import ( + "fmt" + "testing" + "time" + + "golang.org/x/time/rate" +) + +// TDD: insertOrd slice grows unbounded — Cleanup removes from map but not +// from insertOrd. After many cycles of add→cleanup, insertOrd has thousands +// of stale entries while map is empty. +func TestRateLimit_InsertOrdMemoryLeak(t *testing.T) { + store := newInMemoryStore(100000) // high max so eviction doesn't trigger + + // Phase 1: Add 1000 keys. + for i := 0; i < 1000; i++ { + store.GetLimiter(fmt.Sprintf("k%d", i), rate.Limit(10), 20) + } + + // Phase 2: Expire all keys and cleanup. + store.mu.Lock() + for k, entry := range store.limiters { + entry.lastAccess = time.Now().Add(-2 * time.Hour) + store.limiters[k] = entry + } + store.mu.Unlock() + store.Cleanup(1 * time.Hour) + + // Phase 3: Verify map is empty. + store.mu.RLock() + mapLen := len(store.limiters) + store.mu.RUnlock() + + if mapLen != 0 { + t.Fatalf("expected 0 limiters after cleanup, got %d", mapLen) + } + + // Phase 4: LRU list should also be cleaned up, not 1000 stale entries. + store.mu.RLock() + lruLen := store.lruList.Len() + indexLen := len(store.lruIndex) + store.mu.RUnlock() + + if lruLen > 100 { + t.Errorf("REGRESSION: lruList has %d stale entries after cleanup (should be 0)", lruLen) + } + if indexLen > 100 { + t.Errorf("REGRESSION: lruIndex has %d stale entries after cleanup (should be 0)", indexLen) + } +} + +// TDD: Re-added key after cleanup creates duplicate in insertOrd. +// When eviction happens, the stale entry deletes the fresh one. +func TestRateLimit_DuplicateInsertOrd(t *testing.T) { + store := newInMemoryStore(3) + + // Add key "A". + store.GetLimiter("A", rate.Limit(10), 20) + store.GetLimiter("B", rate.Limit(10), 20) + + // Expire "A" and cleanup. + store.mu.Lock() + if e, ok := store.limiters["A"]; ok { + e.lastAccess = time.Now().Add(-2 * time.Hour) + } + store.mu.Unlock() + store.Cleanup(1 * time.Hour) + + // Re-add "A" — now "A" is in insertOrd twice (stale + fresh). + store.GetLimiter("A", rate.Limit(10), 20) + store.GetLimiter("C", rate.Limit(10), 20) + + // Add "D" — triggers eviction. Should evict "B" (oldest active), not "A". + store.GetLimiter("D", rate.Limit(10), 20) + + store.mu.RLock() + _, aExists := store.limiters["A"] + store.mu.RUnlock() + + if !aExists { + t.Error("REGRESSION: re-added key 'A' was evicted due to stale insertOrd duplicate") + } +}