Issue #92557 identified misleading RetryAfter metadata in FixedWindowRateLimiter.
PR #124478 addressed this by changing FixedWindowRateLimiter to return the time until the next replenishment/window boundary, regardless of whether the limiter is using a queue.
TokenBucketRateLimiter and SlidingWindowRateLimiter also expose RetryAfter metadata, but their behavior appears inconsistent with the updated FixedWindowRateLimiter implementation:
SlidingWindowRateLimiter returns a failed lease where the RetryAfter value is null.
TokenBucketRateLimiter appears to follow the same approach that FixedWindowRateLimiter used prior to #124478, by calling CreateFailedTokenLease and returning the full ReplenishmentPeriod rather than the time remaining until the next replenishment.
In #92557, @BrennanConroy mentioned that the other rate limiters exposing RetryAfter should be reviewed as well.
The question is whether the semantics of RetryAfter across the built-in rate limiters should be aligned, and if so, whether TokenBucketRateLimiter and SlidingWindowRateLimiter should adopt behavior similar to the updated FixedWindowRateLimiter.
The simplest approach would be to mirror the changes made in FixedWindowRateLimiter, but it would be useful to confirm the intended behavior before making that change.
Related issues:
#77991
#92557
Issue #92557 identified misleading RetryAfter metadata in FixedWindowRateLimiter.
PR #124478 addressed this by changing FixedWindowRateLimiter to return the time until the next replenishment/window boundary, regardless of whether the limiter is using a queue.
TokenBucketRateLimiter and SlidingWindowRateLimiter also expose RetryAfter metadata, but their behavior appears inconsistent with the updated FixedWindowRateLimiter implementation:
SlidingWindowRateLimiter returns a failed lease where the RetryAfter value is null.
TokenBucketRateLimiter appears to follow the same approach that FixedWindowRateLimiter used prior to #124478, by calling CreateFailedTokenLease and returning the full ReplenishmentPeriod rather than the time remaining until the next replenishment.
In #92557, @BrennanConroy mentioned that the other rate limiters exposing RetryAfter should be reviewed as well.
The question is whether the semantics of RetryAfter across the built-in rate limiters should be aligned, and if so, whether TokenBucketRateLimiter and SlidingWindowRateLimiter should adopt behavior similar to the updated FixedWindowRateLimiter.
The simplest approach would be to mirror the changes made in FixedWindowRateLimiter, but it would be useful to confirm the intended behavior before making that change.
Related issues:
#77991
#92557