fix(limit-req): Make Redis path atomic via EVAL + use hash key with TTL

[falvaradorodriguez]

Description

The current limit-req Redis implementation uses two separate keys (excess and last) and updates them with multiple GET/SET operations.

• Under concurrent load, this leads to race conditions:

1. Several workers may read stale values in parallel and overwrite each other.
2. As a result, the plugin allows more requests than expected, effectively bypassing the intended rate limit.

• On Redis Cluster, the current approach is also problematic: atomic EVAL cannot be executed across two different keys located on different slots.

Solution

• Store both values (excess and last) under a single Redis hash key, so the state is managed as one unit.

• Use a single EVAL script that performs read → compute → write atomically inside Redis, removing race conditions. This approach is consistent with how the limit-count plugin already works.

• Add a TTL to the key to avoid buildup of stale state.

• Preserve existing semantics: the first request with no prior state does not consume tokens.

Which issue(s) this PR fixes:

Fixes #12592

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
• I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

[Baoyuantop]

‌Hi @falvaradorodriguez, we need to add the test case for this fix.

[Baoyuantop]

Hi @falvaradorodriguez, there are failed CI that need fixing.

[falvaradorodriguez]

Hi @falvaradorodriguez, there are failed CI that need fixing.

Hi @Baoyuantop!

The failures are not related to the current changes. Tests of the modified plugin seems to be fine.

Please, Can you review?

Thanks

[15:23:12] t/plugin/limit-req-redis-cluster.t ......... ok    15865 ms ( 0.01 usr  0.01 sys +  0.91 cusr  2.03 csys =  2.96 CPU)
[15:23:29] t/plugin/limit-req-redis.t ................. ok    16682 ms ( 0.02 usr  0.00 sys +  0.95 cusr  2.23 csys =  3.20 CPU)
2025/09/22 15:23:37 Processed 0 requests
[15:23:41] t/plugin/limit-req.t ....................... ok    11856 ms ( 0.02 usr  0.00 sys +  0.83 cusr  1.78 csys =  2.63 CPU)
[15:23:46] t/plugin/limit-req2.t ...................... ok     5010 ms ( 0.01 usr  0.00 sys +  0.81 cusr  0.42 csys =  1.24 CPU)
[15:23:47] t/plugin/limit-req3.t ...................... ok     1290 ms ( 0.00 usr  0.00 sys +  0.39 cusr  0.22 csys =  0.61 CPU)

[luarx]

When will this PR be merged? I am having the same issue that this PR fixes 🙏

[Baoyuantop]

I will promptly urge other community maintainers to review.

[shreemaan-abhishek]

please resolve the conflicts too.

[Copilot]

Pull request overview

This PR fixes race conditions in the limit-req Redis/Redis-Cluster policies by moving the limiter state into a single Redis hash key and updating it atomically via a single Lua EVAL script (with EVALSHA fast-path for standalone Redis), aligning behavior with the intended rate-limit semantics under concurrency.

Changes:

• Replaces multi-key GET/SET updates with an atomic Redis Lua script operating on one hash key (excess + last) plus TTL.
• Enables EVALSHA optimization for the standalone Redis policy; uses EVAL for Redis Cluster for reliability.
• Adds test cases for Redis and Redis Cluster to validate hash structure usage, TTL presence, and basic limiting behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
apisix/plugins/limit-req/util.lua	Implements atomic Redis script (hash-based state + TTL) with EVALSHA fallback logic.
apisix/plugins/limit-req/limit-req-redis.lua	Enables use_evalsha for standalone Redis limiter instances.
apisix/plugins/limit-req/limit-req-redis-cluster.lua	Disables use_evalsha for cluster limiter instances (uses EVAL).
t/plugin/limit-req-redis.t	Adds tests for hash-key state + TTL and a rapid-request rejection case for Redis policy.
t/plugin/limit-req-redis-cluster.t	Adds analogous tests for Redis Cluster policy.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[moonming]

Hi @falvaradorodriguez, thank you for making the Redis limit-req path atomic via EVAL!

Using a Lua script with EVAL to ensure atomicity of the rate limiting operation is the correct approach — the current non-atomic multi-command flow can have race conditions under high concurrency. With 12 reviews, this has been thoroughly discussed.

To confirm readiness:

1. Are all 12 review comments addressed?
2. Has the EVAL script been tested under concurrent load to verify it resolves the race condition?
3. The hash key with TTL approach — does it handle key expiration correctly for sliding windows?

This is an important correctness fix. Let's get it finalized! Thank you.

[Baoyuantop]

Hi @falvaradorodriguez, I see no problems with the code here. Could you please fix the failing test?

[falvaradorodriguez]

Hi @moonming.

I’m replying on your comment:

Hi @falvaradorodriguez, thank you for making the Redis limit-req path atomic via EVAL!

Using a Lua script with EVAL to ensure atomicity of the rate limiting operation is the correct approach — the current non-atomic multi-command flow can have race conditions under high concurrency. With 12 reviews, this has been thoroughly discussed.

To confirm readiness:

1. Are all 12 review comments addressed?

Yes, they have all been checked and corrected where necessary.

2. Has the EVAL script been tested under concurrent load to verify it resolves the race condition?

Yes, in fact, in my case it has been validated in an environment with a high volume of requests

3. The hash key with TTL approach — does it handle key expiration correctly for sliding windows?

Yes, also validated.

This is an important correctness fix. Let's get it finalized! Thank you.

Thank you for the review!

[falvaradorodriguez]

Hi @falvaradorodriguez, I see no problems with the code here. Could you please fix the failing test?

Hi @Baoyuantop!

As you can see here, the tests that were failing were not related to this PR.

Could you check that again?

Thanks

[Baoyuantop]

Okay, I'll check it after CI finishes executing.

[Baoyuantop]

It seems the failed CI is unrelated to this PR; I'll try to fix it.