Caching breaks the rate limiting
Closed this issue · 4 comments
I'm pretty sure the caching expiration logic used is incorrect and causes requests to not be counted or rate limited
I had to remove the caching for it to actually work once deployed. I don't see how you can cache anything and count responses to be rate limited at the same time but I could be misunderstanding it 🤔
Hello @sathoro.
Sorry for the late response, I'm not that active here atm.
The caching is used to check if a request is already being rate-limited, as when the rate-limit is triggered, the DO response contains expire headers which are used to off-load traffic to the DO for identical subsequent requests, for the rate-limiting period.
I don't see how the above behavior can break the rate-limiting, as the requests would indeed be rate-limited for that period of time even with caching disabled and the extra counts would make no difference as they would refer to the same (already rate-limited) time window.
I hope this helps. Let me know if you think I'm missing something here.
@honzabit Hey, no worries!
I'm referring to the caching happening here: https://github.com/honzabit/durable-limiter/blob/main/src/index.ts#L29
Doesn't this cache the rate limiting call, so that subsequent requests aren't counted for rate limiting purposes? We are running a modified version of this in production and it works great but we did remove the caching in that file because in my testing it was making it never actually do any rate limiting
Hello again.
Yes, it does, and that's actually the purpose of this code, to see if we already have a cached response from the rate limiter for the given set of keys (the keys shown here) so that when we do, it means that there's no need to consult again the rate-limiter as the response would be 429.
The cached response will expire though when the time window progresses (for fixed time window configuration), or, in the case of the sliding algo, when the "virtual time window" does.
The calculation of the expirations times can be found in the else block here.
Hope this helps.
Closing this as invalid until proven otherwise. Let me know in case you have new feedback regarding this case though. Thanks.