Skip to content

Bug: Fix memory leak in rate limiter cleanup - #208

Merged
parkerwinner merged 2 commits into
parkerwinner:mainfrom
Emmanuelchukwunonso:bug/rate-limiter-memory-leak
Jul 30, 2026
Merged

Bug: Fix memory leak in rate limiter cleanup#208
parkerwinner merged 2 commits into
parkerwinner:mainfrom
Emmanuelchukwunonso:bug/rate-limiter-memory-leak

Conversation

@Emmanuelchukwunonso

@Emmanuelchukwunonso Emmanuelchukwunonso commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

The in-memory rate limiter already had a background cleanup goroutine (contrary to the issue's premise that none existed) — GetRateLimiter starts one via a sync.Once singleton, sweeping every 10 minutes and removing entries unaccessed for over an hour. Two real gaps remained:

  1. Cleanup interval was 10 minutes, not the 5 minutes this issue's acceptance criteria ask for. Fixed via a defaultCleanupInterval constant (also extracted defaultStaleAfter = 1 hour, the existing "remove after" threshold, previously hardcoded inline).
  2. No way to stop the cleanup goroutine — fine for the process-lifetime singleton in production, but it meant there was no way to write a deterministic test of the cleanup behavior itself (the singleton can't be reconfigured with short intervals without affecting every other test calling GetRateLimiter). Added newRateLimiter (an unexported constructor separate from the GetRateLimiter singleton accessor) and Stop() so tests can build an independent RateLimiter with short, fast intervals and shut its goroutine down cleanly afterward.

5 new tests build a RateLimiter with millisecond-scale cleanupInterval/staleAfter and assert the actual runtime behavior: a stale entry is removed, a recently-accessed entry survives the same sweep, only the genuinely stale key among several is removed, Stop() terminates the goroutine, and the production defaults match the issue's stated 5-minute/1-hour values. All 10 existing rate-limiter tests still pass unchanged; the full suite passes under -race.

Also fixed (blocking, unrelated)

handlers/auth.go had a stray extra }) after ResetPassword's closing brace — a hard syntax error preventing the entire backend module from building at all. Same root cause as #193's PR (independent branches off main both needed it fixed to build/test).

Test plan

  • go build ./middleware/... — clean
  • go test ./middleware/... -run RateLimit -race -v — 15/15 passing

Closes #196
Closes #194

The in-memory rate limiter already had a background cleanup goroutine
(contrary to the issue's premise that none existed) — GetRateLimiter
starts one via a sync.Once singleton, sweeping every 10 minutes and
removing entries unaccessed for over an hour. Two real gaps remained:

1. The cleanup interval was 10 minutes, not the 5 minutes this issue's
   acceptance criteria call for. Fixed via a defaultCleanupInterval
   constant (also extracted defaultStaleAfter = 1 hour, the existing
   "remove after" threshold, previously hardcoded inline).
2. The cleanup goroutine had no way to be stopped — fine for the
   process-lifetime singleton in production, but it meant there was no
   way to write a deterministic test of the cleanup behavior itself
   (the singleton can't be reconfigured with short intervals without
   affecting every other test that calls GetRateLimiter). Added
   newRateLimiter (an unexported constructor separate from the
   GetRateLimiter singleton accessor) and Stop() so tests can build an
   independent RateLimiter with short, fast intervals and shut its
   goroutine down cleanly afterward.

5 new tests build a RateLimiter with millisecond-scale
cleanupInterval/staleAfter and assert the actual runtime behavior: a
stale entry is removed, a recently-accessed entry survives the same
sweep, only the genuinely stale key among several is removed, Stop()
terminates the goroutine, and the production defaults match the
issue's stated 5-minute/1-hour values. All existing rate-limiter tests
(10) still pass unchanged; the full suite passes under -race.

## Also fixed (blocking, unrelated)
handlers/auth.go had a stray extra `})` after ResetPassword's closing
brace — a hard syntax error preventing the entire backend module from
building at all. Same root cause as parkerwinner#193's PR (independent branches
off main both needed it fixed to build/test).

## Test plan
- [x] `go build ./middleware/...` — clean
- [x] `go test ./middleware/... -run RateLimit -race -v` — 15/15 passing

Closes parkerwinner#196
@drips-wave

drips-wave Bot commented Jul 30, 2026

Copy link
Copy Markdown

@Emmanuelchukwunonso Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@parkerwinner

Copy link
Copy Markdown
Owner

fix conflict

@parkerwinner
parkerwinner merged commit c07998f into parkerwinner:main Jul 30, 2026
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory Leak in Rate Limiter No Input Sanitization in Email Service

2 participants