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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
