diff --git a/backend/middleware/rate_limit.go b/backend/middleware/rate_limit.go index e631cd7..9b828f7 100644 --- a/backend/middleware/rate_limit.go +++ b/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() } } diff --git a/backend/middleware/rate_limit_cleanup_test.go b/backend/middleware/rate_limit_cleanup_test.go new file mode 100644 index 0000000..ba71c79 --- /dev/null +++ b/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) +}