Repository navigation
Description
Summary
reinitializeAfterConnect starts the DB-backed rate limiter's cleanup goroutine with the context it was called with. That context is the 30-second request context built in handleHTTPRequest, or the Lambda invocation context, because ensureDB runs lazily on the first request. The worker's ticker period is 10 minutes, so the goroutine is always cancelled long before its first tick and cleanup() never runs from it. The documented 02-M2 mitigation, evicting perpetually-denied keys whose count never resets to 1, is therefore inert. Blast radius is narrow: the worker is only wired when IsLambda is true, and the opportunistic count==1 cleanup still runs, so the effect is unbounded rate_limits growth for keys under sustained denial, on Lambda deployments only. A secondary effect is that every failed reinitializeAfterConnect retry starts one more short-lived goroutine.
Location
internal/server/app.go:754 at 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd
Related: internal/server/http.go:158 (the 30s request context), internal/server/app.go:633 (ensureDB -> reinitializeAfterConnect), internal/api/db_rate_limiter.go:35 and :65-73 (10-minute ticker, exits on ctx.Done()).
Failure scenario
A Lambda deployment serves its first request. ensureDB connects, reinitializeAfterConnect builds the DBRateLimiter and calls StartCleanupWorker(ctx) with the request context. The request finishes, cancel() fires, and the worker's select takes ctx.Done() before the ticker ever fires. An attacker who keeps a login key permanently over its limit has a row whose count never returns to 1, so the opportunistic cleanup never evicts it, and the scheduled cleanup that was meant to catch that case has already exited. The table grows for as long as the abuse lasts.
Evidence
if app.appConfig.IsLambda {
dbRL := api.NewDBRateLimiter(dbConn.Pool())
// Start the scheduled cleanup worker so perpetually-denied keys (whose
// count never resets to 1) are still evicted on a fixed schedule (02-M2).
dbRL.StartCleanupWorker(ctx)
app.RateLimiter = dbRLSuggested fix
Give Application a process-lifetime context (cancelled in Close) and start the worker with that instead of the caller's context, and start it only once.
Found by the 2026-09-02 codebase audit, finding A06-004, reported by one reviewer and independently
confirmed by a second. Full report: docs/audits/codebase-audit-2026-09-02.md.
Activity
cristim commentedon Sep 30, 2026
Confirmed the request-context lifetime defect and tracked it in existing issue #269. PR #455 expands its exposure to all runtimes. The missing periodic cleanup is a retention defect; it does not prevent rate-limit windows from resetting.
Independent review of final commit ef58fda traced the SQL upsert: an expired window resets count to 1 and advances reset_time. A fresh PostgreSQL 17 race run of TestDBRateLimiter_WindowExpiry_AtomicReset passed without starting the cleanup worker, including a seeded expired count of 99. Opportunistic cleanup uses its own bounded background context and globally deletes stale rows, rather than only the triggering key.
The application-owned worker lifecycle and shutdown proof remain in #269. The independent reviewer found no rate-budget correctness dependency requiring that separate change in #455. This is a documented deferral of a valid defect, not a claim that scheduled cleanup works. The final shared-budget integration also passed on two real local HTTP servers and PostgreSQL with race and coverage enabled (45.688s); hosted CI passed for the same commit. Review used gpt-6-astra under the user-authorized substitution, not Opus.
fix(server): pin the rate-limiter cleanup start in reinitializeAfterC…