Repository navigation
fix(service): keep fractional refill and sub-second windows in the Redis rate limiter store - #4375
NitinKumar004 wants to merge 2 commits into
Conversation
…dis rate limiter store The Redis token bucket script floored each refill to whole tokens but reset last_refill on every call, so keys polled faster than one token interval never refilled. The Go caller also passed the window in whole seconds, so a 500ms window became 0 (limiting effectively disabled) and a 1500ms window allowed 1.5x the configured rate. Refill continuously from elapsed nanoseconds with a forward-only clock, pass the window in nanoseconds, and add an unexported allowAt seam so the script can be tested with pinned time. Fixes gofr-dev#4363
Umang01-hash
left a comment
There was a problem hiding this comment.
LGTM. Went through the script math, the tests, and ran it end-to-end against a real Redis 7. Thanks @NitinKumar004.
Bug is real (verified): the old script floored refill to whole tokens and reset last_refill every call, so a key polled faster than one token-interval never accumulated; and the Go caller passed the window as whole seconds, so a 500ms window became 0 (infinite rate → limiting disabled) and 1500ms gave 1.5x. Fixes #4363.
Fix is correct:
- Continuous fractional refill
tokens += (now-last_refill)*requests/window_ns, capped at burst;retry_after = ceil((1-tokens)*window_ns/requests/1e6)ms. Hand-checked the 30/min-polled-500ms and clock-skew sequences — the expected values are genuinely right. - Clock-forward-only guard (
if now > last_refill) means a lagging pod can't re-credit elapsed time andlast_refillnever moves backward. - No public API change (
allowAtunexported); key/fields/units unchanged, so rolling deploy + rollback are safe (the old-format-key row proves it).
Real Redis 7 E2E (miniredis's Lua differs from real Redis, so I EVAL'd the actual script): 5/s polled every 100ms → {1,0} {0,100} {1,0} {0,100} {1,0}, TTL 600 — the #4363 scenario now refills correctly on real Redis.
Tests are non-vacuous — reintroducing integer flooring fails 6 cases; dropping the clock guard fails only the lagging-clock case. gofmt/vet clean, golangci-lint --new-from-rev 0 issues, full service suite green under -race. Window<=0/Requests<=0 are defaulted by Validate(), so window_ns can't be zero.
Re: #4361 (same file, local bucket): package-scope names are disjoint — redisBucketStep/runRedisBucketSteps and a method allowAt on *RedisRateLimiterStore vs *tokenBucket — so no redeclaration on merge, just a normal rebase. Good to merge.
The Redis token bucket script floored each refill to whole tokens but reset
last_refillon every call. Any key polled faster than one token interval therefore never refilled, e.g. 30/min polled every 500ms, or 10/s polled every 50ms.The Go caller also passed the window as whole seconds:
Changes (Redis path only)
config.Window.Nanoseconds()), so sub-second and non-integer windows are honored.Allowdelegates to an unexportedallowAt(now)so tests can pin time. No public API changes.Compatibility: the key (
gofr:ratelimit:<key>), its fields (tokens,last_refill) and their units (ns) are unchanged. Old and new pods can share keys during a rolling deploy, and rollback needs no migration.Tests
TestRedisRateLimiterStore_AllowAtruns the Lua script against miniredis with pinned timestamps. Previously the script was only covered by redismock argument matching.Fixes #4363