Skip to content

fix(limit-count): commit() must report the window-weighted remaining#13704

Merged
nic-6443 merged 2 commits into
apache:masterfrom
nic-6443:fix/limit-count-sliding-commit-weighted-remaining
Jul 20, 2026
Merged

fix(limit-count): commit() must report the window-weighted remaining#13704
nic-6443 merged 2 commits into
apache:masterfrom
nic-6443:fix/limit-count-sliding-commit-weighted-remaining

Conversation

@nic-6443

Copy link
Copy Markdown
Member

Description

commit() in the sliding-window limiter reports limit - current_count as the remaining quota, while incoming() reports the window-weighted value (limit - current_count - last_count * remaining_time / window). Delayed sync (sync_interval) flushes locally-permitted deltas through commit() 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 used incoming() 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

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change (no documented behavior changes)
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

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 dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jul 17, 2026
@membphis

membphis commented Jul 20, 2026

Copy link
Copy Markdown
Member

For Redis-backed sliding-window limits, we strongly recommend enabling delayed sync (sync_interval). It batches counter updates instead of accessing Redis on every request, greatly reducing Redis pressure and generally improving overall stability and performance. With delayed sync enabled, the additional previous-window lookup occurs only during synchronization, not on every request.

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nic-6443
nic-6443 merged commit 3978aa3 into apache:master Jul 20, 2026
17 of 18 checks passed
@nic-6443
nic-6443 deleted the fix/limit-count-sliding-commit-weighted-remaining branch July 20, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants