Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a fixed window rate limiting implementation to complement the existing token and leaky bucket rate limiters. The key changes include adding a new FixedLimiter type that tracks requests within fixed time windows, along with comprehensive test coverage and integration into the existing registry test suite.
- Implements a thread-safe fixed window rate limiter with configurable limit and window duration
- Adds clock injection support for testability via
NewFixedLimiterWithClock - Updates registry tests to validate all three rate limiting strategies (token bucket, leaky bucket, and fixed window)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| window/fixed.go | Implements the core fixed window rate limiter with mutex-based synchronization and window boundary alignment logic |
| window/fixed_test.go | Provides comprehensive test coverage including edge cases, concurrent access, and window reset behavior |
| registry/registry.go | Updates package name from bucket to registry to reflect broader scope |
| registry/registry_test.go | Refactors tests to support multiple limiter strategies and adds fixed window limiter to test matrix |
Comments suppressed due to low confidence (1)
registry/registry_test.go:155
- The comment "Each user has their own bucket" is misleading because this test now runs against multiple limiter strategies including fixed window limiters, not just bucket-based limiters. Consider updating to a more generic comment like "Each user has their own limiter" to accurately reflect the test's purpose.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NewFixedLimiterWithClock creates a new fixed window limiter with a custom clock. | ||
| // Use this constructor for testing with a mock clock. | ||
| func NewFixedLimiterWithClock(limit uint32, window time.Duration, clock clock) *FixedLimiter { | ||
| if window == 0*time.Second { |
There was a problem hiding this comment.
The comparison window == 0*time.Second is unnecessarily verbose and unconventional. It should be simplified to window == 0 since time.Duration is an int64 type and comparing to zero is more idiomatic.
| if window == 0*time.Second { | |
| if window == 0 { |
| l.count = 0 | ||
| } | ||
|
|
||
| if l.count+1 <= l.limit { |
There was a problem hiding this comment.
The condition l.count+1 <= l.limit has a potential integer overflow risk. When both l.count and l.limit are uint32, if l.count equals math.MaxUint32, then l.count+1 will overflow to 0, causing incorrect behavior. Consider rewriting this as l.count < l.limit to avoid overflow.
| if l.count+1 <= l.limit { | |
| if l.count < l.limit { |
Adding fixed window approach