fix(limit-count): commit() must report the window-weighted remaining#13704
Open
nic-6443 wants to merge 2 commits into
Open
fix(limit-count): commit() must report the window-weighted remaining#13704nic-6443 wants to merge 2 commits into
nic-6443 wants to merge 2 commits into
Conversation
commit() reported limit - current_count, ignoring the previous window's weighted share that incoming() subtracts. Delayed sync (sync_interval) flushes local deltas through commit() and caches the reported remaining as the global quota, so every new window started from a full budget and the sliding window degraded into a per-window fixed window: under a sustained 2-instance load at 1.6x the limit it over-admitted by ~18% instead of converging. Read the previous window counter after the increment and report the same weighted remaining as incoming(). A failed read must not fail the commit: the delta is already recorded, and the caller would retry and double-count it; degrade to the unweighted remaining instead. Introduced in apache#13443.
dosubot
Bot
added
size:M
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
commit()in the sliding-window limiter reportslimit - current_countas the remaining quota, whileincoming()reports the window-weighted value (limit - current_count - last_count * remaining_time / window). Delayed sync (sync_interval) flushes locally-permitted deltas throughcommit()and caches the reported remaining as the global quota for the next interval, so at every window boundary each gateway instance starts again from a nearly full budget: the sliding window effectively degrades into a per-window fixed window.The effect is easy to measure: 2 instances sharing one Redis,
count=500,time_window=1,window_type=sliding,sync_interval=0.2, sustained 800 rps for 30s. Ideal admission is 500/s. Before this change the gateways admit ~590/s (+18% over the limit, bursty at each window start); with this change they admit ~438/s and converge, matching what the pre-#13443 flush path (which usedincoming()for flushing) produced. The remaining gap below the limit is the inherent cost of the delayed-sync snapshot, not new behavior.The fix reads the previous window counter after the increment and reports the same weighted remaining as
incoming(). A failed read must not fail the commit — the delta is already recorded, and the delayed syncer would retry and double-count it — so it degrades to the unweighted remaining and logs instead.The unconditional increment behavior from #13443 is unchanged: only the reported remaining is corrected.
Which issue(s) this PR fixes:
Introduced in #13443.
Checklist