Repository navigation
fix(ratelimit): set counter TTL atomically on every request - #73
Conversation
EXPIRE only ran when INCR returned 1, so a counter that lost its TTL (an EXPIRE not yet replicated when Redis failed over) never got one again and blocked its IP forever. 60 of 73 production keys had no TTL, including the node address kubelet probes come from, which failed every probe on that node.
There was a problem hiding this comment.
The PR replaces the INCR + count==1-only EXPIRE + separate TTL sequence with a single TxPipelined (MULTI/EXEC) containing INCR, EXPIRE NX, and TTL, so a counter that lost its TTL gets healed on every request. The pipeline ordering is correct (TTL runs after EXPIRE NX, so it reads the restored TTL), the ttl <= 0 fallback to window keeps resetAt sane if EXPIRE NX fails, the error path keeps the existing fail_open/fail_closed handling, and the Allow signature and caller (internal/middleware/ratelimit.go:67) are unchanged. The new regression test matches the described production failure mode, including precondition, denial-while-over-limit, TTL restoration, and reset after expiry. No real problems found.
Reviewed 2 changed file(s) @ 2b7a0e24 — no blocking issues found.
"Make it work, make it right, make it fast." — Kent Beck
Rate limit counters are now updated with INCR, EXPIRE NX and TTL in one MULTI transaction on every request, instead of setting EXPIRE only when INCR returns 1. A counter that lost its TTL, for example when an EXPIRE was not yet replicated at a Redis failover, never regained one and blocked that IP permanently. In production 60 of 73 keys had no TTL, including the node address kubelet probes come from, so lab pods on that node failed every health check.