Bug: Fix memory leak in rate limiter cleanup

backend/middleware/rate_limit.go

@@ -13,10 +13,21 @@ import (
 
 // RateLimiter stores rate limit information per user
 type RateLimiter struct {
-	mu       sync.RWMutex
-	limits   map[string]*UserLimit
-	config   *config.Config
+	mu              sync.RWMutex
+	limits          map[string]*UserLimit
+	config          *config.Config
 	cleanupInterval time.Duration
+	// staleAfter is how long an entry may go unaccessed before cleanup
+	// removes it. Separated from cleanupInterval (how often the sweep
+	// runs) so tests can exercise cleanup with short, deterministic
+	// intervals without changing what "stale" means in production.
+	staleAfter time.Duration
+	// stop, closed via Stop(), signals the cleanup goroutine to exit —
+	// without this there was previously no way to shut the goroutine down
+	// (fine for the process-lifetime singleton in production, but it
+	// meant a test constructing its own RateLimiter had no way to avoid
+	// leaking a goroutine that outlives the test).
+	stop chan struct{}
 }
 
 // UserLimit tracks request counts for a user
@@ -37,35 +48,67 @@ var (
 	rateLimiterOnce   sync.Once
 )
 
+// defaultCleanupInterval and defaultStaleAfter match this middleware's
+// documented behavior (#196): sweep for stale entries every 5 minutes,
+// removing anything not accessed in the last hour.
+const (
+	defaultCleanupInterval = 5 * time.Minute
+	defaultStaleAfter      = 1 * time.Hour
+)
+
 // GetRateLimiter returns the singleton rate limiter instance
 func GetRateLimiter(cfg *config.Config) *RateLimiter {
 	rateLimiterOnce.Do(func() {
-		globalRateLimiter = &RateLimiter{
-			limits:          make(map[string]*UserLimit),
-			config:          cfg,
-			cleanupInterval: 10 * time.Minute,
-		}
-		// Start cleanup goroutine
-		go globalRateLimiter.cleanup()
+		globalRateLimiter = newRateLimiter(cfg, defaultCleanupInterval, defaultStaleAfter)
 	})
 	return globalRateLimiter
 }
 
-// cleanup removes stale entries periodically
+// newRateLimiter constructs a RateLimiter and starts its cleanup goroutine.
+// Exposed (unexported) separately from GetRateLimiter so tests can build
+// an independent instance with short cleanupInterval/staleAfter values
+// instead of sharing — and being unable to reconfigure — the process-wide
+// singleton.
+func newRateLimiter(cfg *config.Config, cleanupInterval, staleAfter time.Duration) *RateLimiter {
+	rl := &RateLimiter{
+		limits:          make(map[string]*UserLimit),
+		config:          cfg,
+		cleanupInterval: cleanupInterval,
+		staleAfter:      staleAfter,
+		stop:            make(chan struct{}),
+	}
+	go rl.cleanup()
+	return rl
+}
+
+// Stop terminates the cleanup goroutine. Safe to call at most once per
+// RateLimiter (matching a channel's close-once semantics) — the process-
+// lifetime singleton returned by GetRateLimiter is never expected to call
+// this; it exists for tests that construct their own RateLimiter and want
+// to avoid leaking the cleanup goroutine past the test.
+func (rl *RateLimiter) Stop() {
+	close(rl.stop)
+}
+
+// cleanup removes stale entries periodically until Stop() is called.
 func (rl *RateLimiter) cleanup() {
 	ticker := time.NewTicker(rl.cleanupInterval)
 	defer ticker.Stop()
 
-	for range ticker.C {
-		rl.mu.Lock()
-		now := time.Now()
-		for key, limit := range rl.limits {
-			// Remove entries that haven't been accessed in the last hour
-			if now.Sub(limit.LastAccess) > time.Hour {
-				delete(rl.limits, key)
+	for {
+		select {
+		case <-rl.stop:
+			return
+		case <-ticker.C:
+			rl.mu.Lock()
+			now := time.Now()
+			for key, limit := range rl.limits {
+				if now.Sub(limit.LastAccess) > rl.staleAfter {
+					delete(rl.limits, key)
+				}
 			}
+			rl.mu.Unlock()
 		}
-		rl.mu.Unlock()
 	}
 }
 

backend/middleware/rate_limit_cleanup_test.go

@@ -0,0 +1,83 @@
+package middleware
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/yourusername/gpay-remit/config"
+)
+
+// These tests build their own RateLimiter (via newRateLimiter, not the
+// process-wide GetRateLimiter singleton) with short cleanupInterval/
+// staleAfter values, so the cleanup goroutine's actual behavior can be
+// observed deterministically in test time rather than waiting on the
+// production 5-minute/1-hour defaults.
+
+func TestRateLimiter_CleanupRemovesStaleEntries(t *testing.T) {
+	rl := newRateLimiter(&config.Config{}, 20*time.Millisecond, 30*time.Millisecond)
+	defer rl.Stop()
+
+	rl.IncrementAndCheck("stale-key", 10, time.Minute)
+	assert.NotNil(t, rl.GetLimit("stale-key"), "entry should exist immediately after creation")
+
+	// Past staleAfter, and at least one cleanup tick has had time to run.
+	time.Sleep(80 * time.Millisecond)
+
+	assert.Nil(t, rl.GetLimit("stale-key"), "cleanup should have removed the entry once it exceeded staleAfter")
+}
+
+func TestRateLimiter_CleanupKeepsRecentlyAccessedEntries(t *testing.T) {
+	rl := newRateLimiter(&config.Config{}, 20*time.Millisecond, 200*time.Millisecond)
+	defer rl.Stop()
+
+	rl.IncrementAndCheck("active-key", 10, time.Minute)
+
+	// A cleanup tick runs well before staleAfter elapses; the entry must
+	// survive since it hasn't gone unaccessed for staleAfter yet.
+	time.Sleep(60 * time.Millisecond)
+
+	assert.NotNil(t, rl.GetLimit("active-key"), "cleanup must not remove entries accessed within staleAfter")
+}
+
+func TestRateLimiter_CleanupOnlyRemovesTheStaleEntry(t *testing.T) {
+	rl := newRateLimiter(&config.Config{}, 20*time.Millisecond, 30*time.Millisecond)
+	defer rl.Stop()
+
+	rl.IncrementAndCheck("will-go-stale", 10, time.Minute)
+
+	// Give "will-go-stale" time to become stale, then touch a second key
+	// right before the assertion so it's fresh.
+	time.Sleep(50 * time.Millisecond)
+	rl.IncrementAndCheck("stays-fresh", 10, time.Minute)
+	time.Sleep(30 * time.Millisecond)
+
+	assert.Nil(t, rl.GetLimit("will-go-stale"), "the genuinely stale key should be removed")
+	assert.NotNil(t, rl.GetLimit("stays-fresh"), "a key accessed just before the sweep must survive it")
+}
+
+func TestRateLimiter_StopTerminatesCleanupGoroutine(t *testing.T) {
+	rl := newRateLimiter(&config.Config{}, 5*time.Millisecond, time.Hour)
+
+	done := make(chan struct{})
+	go func() {
+		rl.Stop()
+		close(done)
+	}()
+
+	select {
+	case <-done:
+		// Stop() returned (channel close doesn't block on the goroutine
+		// exiting, but this at least confirms Stop() itself doesn't hang
+		// or panic on a fresh, running limiter).
+	case <-time.After(time.Second):
+		t.Fatal("Stop() did not return in time")
+	}
+}
+
+func TestDefaultRateLimiter_UsesDocumentedIntervals(t *testing.T) {
+	// Pins #196's acceptance criteria ("run cleanup every 5 minutes")
+	// against a regression that silently changes the production defaults.
+	assert.Equal(t, 5*time.Minute, defaultCleanupInterval)
+	assert.Equal(t, 1*time.Hour, defaultStaleAfter)
+}