Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a sliding window rate limiting strategy to complement the existing fixed window implementation. A sliding window rate limiter tracks individual request timestamps and expires them individually, providing more granular rate limiting compared to the fixed window approach which resets all requests at window boundaries.
Key Changes:
- Implements
SlidingLimiterwith timestamp-based request tracking and automatic cleanup - Adds comprehensive test coverage for concurrent access, window expiry, and edge cases
- Integrates sliding window strategy into the registry test suite alongside existing strategies
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| window/sliding.go | Core sliding window limiter implementation with queue-based timestamp tracking and memory optimization |
| window/sliding_test.go | Test suite covering basic functionality, concurrency safety, window expiry behavior, and partial expiry scenarios |
| registry/registry_test.go | Integration of SlidingWindowConfig into the registry test infrastructure to validate compatibility with other rate limiting strategies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| l.head = 0 | ||
| } | ||
|
|
||
| if len(l.q)-l.head+1 > int(l.limit) { |
There was a problem hiding this comment.
The limit check logic is incorrect. The condition len(l.q)-l.head+1 > int(l.limit) adds 1 to the current count before checking, which causes the limiter to reject requests one too early. For example, with a limit of 2, this rejects the second request instead of the third.
The correct logic should check if the current count (without the +1) is already at or above the limit: len(l.q)-l.head >= int(l.limit).
| if len(l.q)-l.head+1 > int(l.limit) { | |
| if len(l.q)-l.head >= int(l.limit) { |
| type SlidingWindowConfig struct { | ||
| limit uint32 | ||
| duration time.Duration | ||
| } |
There was a problem hiding this comment.
The struct field naming is inconsistent with other config types in this file. TokenBucketConfig, LeakyBucketConfig, and FixedWindowConfig all use exported (capitalized) field names, but SlidingWindowConfig uses unexported (lowercase) fields 'limit' and 'duration'. This breaks the established naming pattern.
The fields should be renamed to 'Limit' and 'Duration' (or 'Window' to match FixedWindowConfig) for consistency.
| TokenBucketConfig{Capacity: capacity, Rate: rate}, | ||
| LeakyBucketConfig{Capacity: capacity, Rate: rate}, | ||
| FixedWindowConfig{Limit: capacity, Window: win}, | ||
| SlidingWindowConfig{limit: capacity, duration: win}, |
There was a problem hiding this comment.
The struct instantiation uses lowercase field names while other config types in the same function use capitalized field names. This inconsistency occurs because SlidingWindowConfig has unexported fields.
Once the SlidingWindowConfig fields are properly exported, this line should be updated to use capitalized field names: SlidingWindowConfig{Limit: capacity, Duration: win} (or Window: win if that field name is chosen).
Add sliding window option