fix(limit-count): make sliding-window limiter check-and-increment atomic#13574
Open
shreemaan-abhishek wants to merge 1 commit into
Open
fix(limit-count): make sliding-window limiter check-and-increment atomic#13574shreemaan-abhishek wants to merge 1 commit into
shreemaan-abhishek wants to merge 1 commit into
Conversation
The sliding window read the counter, evaluated the limit, then incremented in separate steps. With redis the get and incr are separate round trips, so concurrent requests could all pass the check before any increment landed and exceed the configured rate. Fold the decision and the increment into one atomic store operation (check_and_incr): a redis Lua script for the redis-backed stores and a single non-yielding sequence for the shared-dict store. It increments only when the request is accepted, preserving the existing contract that incoming() never increments on reject (commit() still flushes delayed-sync deltas). The redis script touches two keys (current and previous window), so the counter key now carries a hash tag to keep both on one redis-cluster slot.
dosubot
Bot
added
the
size:L
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
limit-countsliding-window limiter read the current counter, evaluated the limit (including the smoothed carry-over from the previous window), and only then incremented, as three separate steps. With the redis stores thegetandincrare separate round trips, so under concurrency multiple requests can all pass the check before any increment is observed and exceed the configured rate.This folds the accept/reject decision and the increment into a single atomic store operation,
check_and_incr:get/decide/incrsequence.The existing contract is preserved:
incoming()still never increments on reject (verified by the delayed-sync regression test), whilecommit()keeps flushing already-permitted deltas. The previous reactive post-increment re-check is no longer needed and is removed.Because the redis script now touches two keys (current and previous window), the counter key carries a
{...}hash tag so both windows hash to the same redis-cluster slot.Which issue(s) this PR fixes:
N/A (hardening; reported internally against the API7 fork)
Checklist