[PR] fix(limit-count): keep delayed sync counters per rule [apisix]

["lordcheng10 (via GitHub)" <gi...@apache.org>]

lordcheng10 opened a new pull request, #14064:
URL: https://github.com/apache/apisix/pull/14064

   ### Description
   
   Fix the `limit-count` plugin corrupting per-rule counters when the
   `redis`/`redis-cluster`/`redis-sentinel` policy is used together with
   `sync_interval` and multiple `rules`.
   
   With `rules`, every rule creates its own limiter (`rule.count`,
   `rule.time_window`), but the delayed-sync instance id was derived only from the
   plugin name, policy and `_vid`:
   
   ```lua
   local extra_key = name .. '#' .. conf.policy
   ```
   
   so all rules of one plugin instance shared a single delayed-sync queue and a
   single sync timer. `sync_key()` flushes every queued key through
   `self.limiter`, which belongs to whichever rule started the timer. Keys of the
   other rules were therefore synced to Redis using the wrong `count` and
   `time_window`, and `sync_to_shm()` stored that wrong remaining value for their
   keys.
   
   The observable effect matches #14063: right after the first request each
   `X-<prefix>-RateLimit-Remaining` header is correct, but once the delayed sync
   runs, the remaining value of every other rule becomes the value that the first
   rule's limiter produces (for example rule 2 with `count: 500` jumps to the
   remaining of rule 1 with `count: 30`).
   
   Fix: give each rule its own syncer id by appending the rule index, so each rule
   gets its own queue, timer and limiter. `get_rules()` now records the stable
   rule index (`idx`), which is `1` for the single-rule configuration, so the
   existing behaviour for configurations without `rules` is unchanged.
   
   This only changes the delayed-sync bookkeeping; the rate-limiting decision
   path, Redis key format and headers are untouched.
   
   #### Which issue(s) this PR fixes:
   
   Fixes #14063
   
   ### Checklist
   
   - [x] I have explained the need for this PR and the problem it solves
   - [x] I have explained the changes or the new features added to this PR
   - [x] I have added tests corresponding to this change
   - [ ] I have updated the documentation to reflect this change
   - [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
   
   ### Additional notes
   
   No documentation change is needed: the configuration surface and the
   documented `sync_interval` semantics are unchanged, this only fixes the
   internal bookkeeping that broke per-rule counters.
   
   Regression test added to `t/plugin/limit-count-redis-delayed-sync2.t`:
   
   - TEST 10 configures two rules (`count: 5`, `time_window: 10` and
     `count: 100`, `time_window: 20`) with `sync_interval: 0.5`;
   - TEST 11 asserts `4`/`99` on the first request and `3`/`98` after the delayed
     sync. Without the fix the second rule is synced through the first rule's
     limiter and reports `3` instead of `98`.
   
   Reviewer note on verification: this environment has no OpenResty or Redis, so
   `prove t/plugin/limit-count-redis-delayed-sync2.t` could not be executed
   locally. The failure path was verified by tracing the code: the timer closure
   captures the syncer of the rule that started it, `sync_key()` uses
   `self.limiter` for every key in the shared queue, and with per-rule syncer ids
   each queue only ever contains keys of its own rule. CI is expected to run the
   full test file.
   
   Upgrade note: the syncer id changes once, so delayed-sync queues created by the
   previous version are not drained after an in-place upgrade. They expire with
   the shared dict and contain only pending counter deltas, not configuration.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(limit-count): keep delayed sync counters per rule [apisix]

["lordcheng10 (via GitHub)" <gi...@apache.org>]

lordcheng10 commented on PR #14064:
URL: https://github.com/apache/apisix/pull/14064#issuecomment-6332114464

   CI note: the current failures are all in the dependency-install step, before any test runs:
   
   ```
   Error: Unable to locate package libpcre3
   Error: Package 'libpcre3-dev' has no installation candidate
   ```
   
   They reproduce on the same workflow for other branches as well; `ubuntu-latest` has moved to Ubuntu 26.04, where `libpcre3-dev` is no longer available. `Validate PR title`, `build-test-push` (amd64/arm64) are green. I'll rerun the affected jobs once the workflow image/package list is updated upstream.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(limit-count): keep delayed sync counters per rule [apisix]

["lordcheng10 (via GitHub)" <gi...@apache.org>]

lordcheng10 commented on PR #14064:
URL: https://github.com/apache/apisix/pull/14064#issuecomment-6332094277

   PTAL, thanks! @nic-6443 @AlinsRan @Baoyuantop
   
   @nic-6443 this touches the delayed-sync bookkeeping you recently worked on in #14015 / #13996, so the multi-rule queue/timer split is probably the part worth a close look.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org