Skip to content

fix(limit-count): refresh the delayed-sync quota after the local delta expired - #14015

Merged
nic-6443 merged 3 commits into
apache:masterfrom
nic-6443:fix/delayed-sync-stale-snapshot
Oct 8, 2026
Merged

nic-6443 merged 3 commits into
apache:masterfrom
nic-6443:fix/delayed-sync-stale-snapshot

Conversation

@nic-6443

@nic-6443 nic-6443 commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

With delayed sync, a node that only rejects can stop refreshing its cached quota, and then it keeps rejecting on stale numbers until the quota itself expires.

The periodic sync flushes a key only while its local delta is present in the shared dict (sync_key, if delta then). That entry is created by incr(..., 0, 2 * window), and init_ttl only applies on creation, so the entry expires two windows after it was first written, whatever happens in between. Admitted requests recreate it; rejected ones don't. So once it expires on a node that is rejecting everything, the timer still runs for the key but skips the flush, and the cached quota is never refreshed again. The next refresh only happens when the cached reset runs out.

With a sliding window this matters a lot. The cached quota carries the previous window's weight as of the last sync, so the node keeps applying a weight of ~0.9 for the rest of the window instead of letting it decay. In a two-node test at 2× the limit, one 10s window admitted 9 requests where an exact sliding window would have admitted 100, because both nodes stopped syncing about 1s into the window. A fixed window isn't affected in practice: within a window the remote count only grows, so a stale cached quota can't wrongly reject, and the window end coincides with the cached reset, which forces a refresh.

The fix flushes a missing delta as 0 instead of skipping it. That refreshes the cached quota exactly like the existing flush of a zero delta, which already happens whenever the entry is present but empty. A failed shared dict read still skips the flush as before. When there was no local delta and Redis is unreachable, the fallback limiter is skipped: there is nothing to account for locally, and charging it the existing minimum of 1 would let a node that only rejects inflate its fallback count on every sync. The path where the entry exists, including a value of 0, is unchanged. For the same reason, sync_to_shm no longer creates an empty entry when nothing was flushed, so a zero-delta refresh doesn't later look like a pending delta.

t/plugin/limit-count-redis-delayed-sync3.t reproduces it without waiting two windows. It exhausts the quota, drops the local delta entry as its expiry would, frees the quota in Redis, and checks that a rejected request schedules a sync that lets the next request through. It returns 200 200 503 503 503 without the fix and 200 200 503 503 200 with it. A second case, a fixed window against an unreachable Redis, drops the entry and checks that the failed sync leaves the fallback counter alone. It goes from 1 to 2 without the fallback guard and stays at 1 with it. A third case covers a zero-delta refresh followed by a Redis error (a non-integer counter). Without the change in sync_to_shm, the fallback count goes from 0 to 1.

No documentation change: the documented delayed sync behavior doesn't change.

  • 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
  • I have verified that this change is backward compatible

…a expired

The periodic sync only flushed a key while its local delta was present in
the shared dict. That entry is created with a TTL of two windows and is
never renewed, so once it expired, a node that kept rejecting never
flushed or refreshed its cached quota again. With a sliding window it kept
judging on a stale weight of the previous window until the window ended,
rejecting requests that the current Redis state would admit.

Flush a missing delta as 0, which refreshes the cached quota.
Copilot AI balanced review requested due to automatic review settings October 8, 2026 07:46
…lta to sync

A missing local delta is now flushed as 0 to refresh the cached quota.
When Redis is unreachable, that flush must not fall back to charging the
local limiter its minimum of 1, or a node that only rejects would add one
to its fallback count on every sync.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

A successful zero-delta refresh can recreate an empty entry that causes incorrect fallback accounting during a later Redis outage.

1 open finding
What changed in this PR

This PR refreshes cached limit-count quotas after delayed-sync delta entries expire, preventing stale rejections.

Changes:

  • Flushes missing deltas as zero to refresh quotas.
  • Skips fallback accounting when the delta is absent.
  • Adds regression tests for quota refresh and fallback accounting.
File Description
t/​plugin/​limit-count-redis-delayed-sync3.t Tests expired-delta refresh and Redis failure handling.
apisix/​plugins/​limit-count/​delayed-syncer.lua Refreshes missing deltas and guards fallback accounting.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread apisix/plugins/limit-count/delayed-syncer.lua
…flushed

A zero-delta quota refresh recreated the expired local delta entry as 0
through incr(-0). If Redis then failed, the sync took that entry for a
pending delta and charged the fallback limiter its minimum of 1.
@nic-6443
nic-6443 merged commit 88c4b8c into apache:master Oct 8, 2026
23 checks passed
@nic-6443
nic-6443 deleted the fix/delayed-sync-stale-snapshot branch October 8, 2026 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants