🐛 fix(limiter): floor sub-second expiration in sliding window - #4564
🐛 fix(limiter): floor sub-second expiration in sliding window#4564nikolauspschuetz wants to merge 2 commits into
Conversation
SlidingWindow computed the expiration as uint64(expirationDuration.Seconds()), so any positive sub-second ExpirationFunc value (e.g. 500ms) truncated to 0. The window weight is then float64(resetInSec) / float64(expiration), a division by zero that makes the rate NaN, so every request — including the first — was rejected with 429 regardless of Max. Floor the truncated window to 1 second. Adds a regression test that is rejected with 429 before the fix and returns 200 after.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe sliding-window limiter now floors sub-second expiration durations to one second, with a regression test covering a 500ms expiration and successful initial request. ChangesSliding window expiration
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4564 +/- ##
==========================================
- Coverage 93.29% 93.25% -0.04%
==========================================
Files 140 140
Lines 14854 14856 +2
==========================================
- Hits 13858 13854 -4
- Misses 620 625 +5
- Partials 376 377 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@nikolauspschuetz The linter workflow is failing |
Satisfies the gocritic httpNoBody linter.
Description
SlidingWindowcomputes the window length as:Any positive sub-second
ExpirationFuncvalue truncates to0. The window weight is thenso
ratebecomes garbage and every request — including the first — is rejected with429, regardless ofMax. A limiter configured with, say,ExpirationFunc: func(fiber.Ctx) time.Duration { return 500*time.Millisecond }blocks all traffic.Fix
Floor the truncated window to 1 second (a sub-second window is treated as a 1-second window rather than breaking):
Testing
Added
TestLimiterSlidingSubSecondExpiration, which is rejected with429before this change and returns200after.go test ./middleware/limiter/(full package),go vet, andgofmtare clean.Note:
FixedWindowshares the sameuint64(...Seconds())truncation; I scoped this PR toSlidingWindowsince that's the one with a clean, demonstrable failure (the NaN-rate 429). Happy to follow up on the fixed-window window-sizing behavior separately if maintainers agree it's worth addressing.