A FixedWindow always endsintervalInSeconds after the start time (timer). So when calculating the time to consume some tokens the tokens will always be available attimer + intervalInSeconds. But currently this is reported incorrectly ascalculateTimeForTokens() calculates the time based on the desired amount of tokens (and cycles) while it doesn't take into accountmaxSize amount of tokens become available at the windows end.

Furthermore calculating the amount of needed cycles is incorrect. This as all tokens become available at once (at the windows end) and you can't consume more tokens thanmaxSize (which is validated at the start ofFixedWindowLimiter::reserve, in case oftokens > limit it throws).

Note: I don't think that changing the signature ofcalculateTimeForTokens is a deprecation. This as theWindow class is marked as@internal. So it should only be used by theRateLimiter component.

carsonbot added Status: Needs Review Bug RateLimiter labels

Jan 23, 2023

carsonbot added this to the5.4 milestone

Jan 23, 2023

Copy link

ContributorAuthor

RobertMe commentedJan 23, 2023

I'm uncertain about the Psalm failure.return ceil(...) results in afloat (while it is an integer value), and the return type is declared asint. But the old code also was based onceil() resulting in afloat return value, while the return type was declared asint. So I guess that's fine?

Copy link

Contributor

Robert-Embloom commentedFeb 6, 2023

@wouterj it seems like you're the original developer of the RateLimiter component. So may I ask you to have a look at this? (as it seems to have gone unnoticed in general)

nicolas-grekas approved these changes

Jul 7, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jul 7, 2023

nicolas-grekas reviewed

Jul 7, 2023

View reviewed changes

src/Symfony/Component/RateLimiter/Policy/Window.php OutdatedShow resolvedHide resolved

RobertMe force-pushed thefixed-window-retry-after branch fromc612584 to5c380e9Compare

July 10, 2023 10:31

[RateLimiter] fix incorrect retryAfter of FixedWindow

2316932

A FixedWindow always ends `intervalInSeconds` after the start time(`timer`). So when calculating the time to consume some tokens thetokens will always be available at `timer + intervalInSeconds`. Butcurrently this is reported incorrectly as `calculateTimeForTokens()`calculates the time based on the desired amount of tokens (and cycles)while it doesn't take into account `maxSize` amount of tokens becomeavailable at the windows end.Furthermore calculating the amount of needed cycles is incorrect. Thisas all tokens become available at once (at the windows end) and youcan't consume more tokens than `maxSize` (which is validated at thestart of `FixedWindowLimiter::reserve`, in case of `tokens > limit` itthrows).

RobertMe force-pushed thefixed-window-retry-after branch from5c380e9 to2316932Compare

July 10, 2023 11:11