fix(limit-count): make sliding-window limiter check-and-increment atomic#13574
Open
shreemaan-abhishek wants to merge 3 commits into
Open
fix(limit-count): make sliding-window limiter check-and-increment atomic#13574shreemaan-abhishek wants to merge 3 commits into
shreemaan-abhishek wants to merge 3 commits 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
redis-cluster rejects EVAL with more than one key, so the two-key check-and-increment script failed on cluster deployments. The previous window is frozen, so read it with a single-key GET and pass it to a single-key script that atomically checks and increments the current window. Drops the now-unneeded hash tag on the counter key.
…ross workers The comment claimed get/decide/incr runs without interleaving, which only holds within a worker. Across workers the three shared-dict ops aren't atomic, so a concurrent burst can admit a few extra requests at a window boundary. Note this and that the redis store is exact.
membphis
approved these changes
Jun 22, 2026
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
2 participants
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