Skip to content

fix(rate-limiting): sync periods not longer than sync_rate in realtime - #15037

Open
rajtherengan wants to merge 1 commit into
Kong:masterfrom
rajtherengan:fix/rate-limiting-sync-rate
Open

rajtherengan wants to merge 1 commit into
Kong:masterfrom
rajtherengan:fix/rate-limiting-sync-rate

Conversation

@rajtherengan

@rajtherengan rajtherengan commented Sep 29, 2026 •

Copy link
Copy Markdown

Summary

With policy = redis and sync_rate >= period (e.g. sync_rate = 1 with a second limit), counters are not shared across workers and nodes. The first request in a window runs INCR against Redis and caches the result per worker; later requests are counted locally and pushed by a timer that fires sync_rate seconds later — after the window has already closed. Other workers and nodes never see those increments while they matter, so the effective limit becomes roughly limit × workers × nodes.

This PR implements option 4 from the issue, plus a warning:

  • Any period whose length is <= sync_rate is treated as realtime for that period only: it is counted in Redis on every request, same as sync_rate = -1.
  • Longer periods keep async batching. For example, second + minute with sync_rate = 1 → second is realtime, minute stays async.
  • A warning is logged when the fallback applies, since Redis traffic goes up for those periods. It is logged once per worker, per plugin instance and period; making it once per node would need a shared dict — happy to add that if preferred.

I chose this over rejecting the configuration (option 1) so existing declarative configs keep loading on upgrade. Strict validation could still be added in a future major release.

The change is contained to the Redis policy in kong/plugins/rate-limiting/policies/init.lua: a per-period is_realtime() check used by usage() and increment(). increment() also skips scheduling a sync timer when every limited period is realtime.

Note: this makes the late-sync path in #14998 (EXPIREAT receiving milliseconds, so recreated keys never expire) much rarer, but does not fix it for async periods. The two changes are independent.

Checklist

Issue reference

Fixes #15018

@CLAassistant

CLAassistant commented Sep 29, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rate-limiting: Redis sync should not have a sync_rate higher than the period

2 participants