fix(limit-conn): implement configurable redis key expiry

[shreemaan-abhishek]

Description

When using limit-conn, when an APISIX node crashes and is unable to perform the "decrement count" operation, this key remains permanently in Redis as there is not expiry configured, causing the user's concurrency quota to be permanently occupied or even the user to be blocked.

UPDATE:

Thanks to @ChuanFF's comment: #12872 (comment), we realised that the existing method of counting connections would not yield accurate and correct connection limiting in distributed deployment scenario.

The correct solution to ensure keys expire timely would be when an expiry was tied to each individual increment (each connection). To achieve this we use redis sorted set to count each connection by a request_id, assign a ttl to each request_id and remove timely remove all request_ids that have exceeded the ttl.

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)

[kayx23]

1. update limit-req doc for the parameter
2. add the corresponding zh docs

[shreemaan-abhishek]

done

[moonming]

The TTL for limit-req is hardcoded to 2 seconds. This feels arbitrary and lacks flexibility.

[moonming]

The ttl_policy_schema function modifies a shared table, is this safe?

[shreemaan-abhishek]

No it is not. Good catch. Let me fix this.

[moonming]

Need to add a scenario where a key's TTL expires. The test should verify that the counter is effectively gone and that a new request can proceed without being blocked by a stale counter.

•Example: Set a short TTL (e.g., 2s), make a request, wait for 3s, and then confirm that a subsequent request does not inherit the old counter value.

[shreemaan-abhishek]

done

[Copilot]

Pull request overview

This PR adds TTL (Time To Live) configuration to Redis keys used by the limit-conn and limit-req plugins to prevent keys from persisting indefinitely when APISIX nodes crash before decrementing counters. This addresses a critical issue where user quotas could be permanently occupied or users could be permanently banned due to orphaned Redis keys.

Changes:

• Added configurable key_ttl parameter to limit-conn plugin with a default value of 60 seconds
• Implemented hardcoded 2-second TTL for limit-req plugin Redis keys
• Added comprehensive test coverage for both plugins with Redis and Redis Cluster configurations

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
apisix/utils/redis-schema.lua	Added ttl_policy_schema function to extend Redis policy schemas with TTL configuration
apisix/plugins/limit-conn.lua	Updated schema to use ttl_policy_schema function for Redis policies
apisix/plugins/limit-conn/util.lua	Implemented TTL setting using Redis pipeline for both increment and decrement operations
apisix/plugins/limit-req/util.lua	Added hardcoded 2-second TTL to Redis SET operations for rate limiting keys
docs/en/latest/plugins/limit-conn.md	Documented the new key_ttl configuration parameter
t/plugin/limit-req-redis-ttl.t	Added test to verify TTL is set correctly for limit-req with Redis
t/plugin/limit-req-redis-cluster-ttl.t	Added test to verify TTL is set correctly for limit-req with Redis Cluster
t/plugin/limit-conn-redis-ttl.t	Added tests for default and custom TTL values with Redis
t/plugin/limit-conn-redis-cluster-ttl.t	Added tests for default and custom TTL values with Redis Cluster

Comments suppressed due to low confidence (1)

apisix/plugins/limit-conn/util.lua:52

• When a request is rejected because it exceeds max + burst, the counter is decremented (line 48) but the TTL is not refreshed in this path. This could lead to the key expiring prematurely if there are many rejected requests, potentially causing the counter to be lost even though there are still active connections.

Consider adding TTL refresh logic after the decrement operation on line 48, similar to how it's done in the leaving function.

        if conn > max + self.burst then
            conn, err = red:incrby(key, -1)
            if not conn then
                return nil, err
            end
            return nil, "rejected"

---

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

[Copilot]

The comment indicates that the TTL calculation formula is ceil(burst/rate) + 1, which would equal 11 seconds for rate=1 and burst=10. However, this formula and comment appear to be copied from limit-req tests but are actually checking a hardcoded 2-second TTL in the implementation. This comment is misleading and should be updated to reflect the actual TTL value of 2 seconds that is set in the implementation.

Suggested change

	-- for rate=1, burst=10 -> ttl = ceil(10/1)+1 = 11.
	if ttl > 0 and ttl <= 11 then
	-- ttl is expected to be 2 seconds as set by the current limit-req implementation.
	if ttl == 2 then

[Copilot]

This function mutates the original schema object from policy_to_additional_properties by directly adding the key_ttl property. This means that after the first call, the schema will be modified permanently for all subsequent uses. This could lead to unexpected behavior if the schema is reused elsewhere.

Consider creating a deep copy of the schema before adding the key_ttl property to avoid mutating the shared schema object.

[Copilot]

The limit-req plugin uses a hardcoded TTL of 2 seconds for Redis keys, while the limit-conn plugin has a configurable key_ttl parameter with a default of 60 seconds. This inconsistency means:

1. Users cannot configure the TTL for limit-req Redis keys
2. The 2-second TTL may be too short for high-burst scenarios where requests are queued

Consider making the TTL configurable for limit-req as well, similar to limit-conn. The TTL could be calculated as ceil(burst/rate) + a safety margin to ensure keys persist long enough for delayed requests to be processed.

Suggested change

	-- limit-req does rate limiting per second, so we set the ttl to 2 seconds
	local ttl = 2
	-- limit-req does rate limiting per second. Allow configurable TTL via self.key_ttl,
	-- defaulting to 2 seconds to preserve existing behavior if not set.
	local ttl = self.key_ttl or 2

[Copilot]

The test expects a TTL of up to 11 seconds (based on the formula ceil(burst/rate) + 1 = ceil(10/1) + 1 = 11), but the implementation in apisix/plugins/limit-req/util.lua sets a hardcoded TTL of 2 seconds. This test will pass since 2 is within the range (0, 11], but the test is validating against the wrong expected value.

The test should check for TTL <= 2 to match the actual implementation, or the implementation should be updated to use a calculated TTL based on burst and rate parameters.

Suggested change

	if ttl > 0 and ttl <= 11 then
	if ttl > 0 and ttl <= 2 then

[Copilot]

Similar to the redis test, this test expects a TTL of up to 11 seconds based on the formula ceil(burst/rate) + 1, but the implementation uses a hardcoded 2-second TTL. The test should check for TTL <= 2 to match the actual implementation, or the implementation should use a calculated TTL.

Suggested change

	-- for rate=1, burst=10 -> ttl = ceil(10/1)+1 = 11.
	if ttl > 0 and ttl <= 11 then
	-- implementation currently uses a fixed TTL of 2 seconds
	if ttl > 0 and ttl <= 2 then

[membphis]

I think:

you'd better split this PR to two PRs

One PR for one plugin, eg: limit-count, limit-req and limit-conn

Smaller PR is much easier to review

[ChuanFF]

when an APISIX node crashes and is unable to perform the "decrement count" operation, this key remains permanently in Redis as there is not expiry configured, causing the user's concurrency quota to be permanently occupied or even the user to be banned.

Setting a TTL for Redis keys does not seem to solve the scenario where a node in an APISIX cluster goes down under high traffic. When the traffic is heavy, APISIX keeps performing Redis operations, which continuously refresh the TTL. As a result, the counter associated with the failed node in Redis cannot be reset through TTL expiration.

[shreemaan-abhishek]

As a result, the counter associated with the failed node in Redis cannot be reset through TTL expiration.

I am not sure if I fully understand what you are trying to convey. The counter key will be same for all apisix instances in a cluster. Am I missing something 🤔

[membphis]

I think the default value can be 3600, means 1hour

it should be enough

[shreemaan-abhishek]

makes sense, I have set the default ttl to be 3600.

[shreemaan-abhishek]

@ChuanFF I have updated the redis script to ensure each redis key has a ttl assigned to it. So the issue that you described should not occur.

[membphis]

you can take a look at this example, which is better performance

we can cache the sha1 of Lua script, then we call evalsha

local redis = require "resty.redis"
local red = redis:new()

-- 1. Define your fixed script and pre-calculate its SHA1 locally
-- You can use an online SHA1 tool or calculate it via code
local script_content = "return redis.call('GET', KEYS[1])"
local script_sha1 = "6bce267422d7793d3e0034d7c99602029c5737e6"

red:set_timeout(1000) -- 1 sec
local ok, err = red:connect("127.0.0.1", 6379)
if not ok then
    ngx.log(ngx.ERR, "failed to connect: ", err)
    return
end

--- Function to execute script with EVALSHA optimization
local function optimized_eval(red, sha1, script, num_keys, ...)
    -- Try EVALSHA first
    local res, err = red:evalsha(sha1, num_keys, ...)
    
    -- If the script is missing in Redis (NOSCRIPT error)
    if not res and err and string.find(err, "NOSCRIPT") then
        ngx.log(ngx.INFO, "Script not found in cache, loading now...")
        
        -- Load the script to get the SHA1 (or verify it)
        local new_sha, load_err = red:script("LOAD", script)
        if not new_sha then
            return nil, "failed to load script: " .. (load_err or "")
        end
        
        -- Retry EVALSHA with the loaded script
        return red:evalsha(new_sha, num_keys, ...)
    end
    
    return res, err
end

-- 2. Call the function
local res, err = optimized_eval(red, script_sha1, script_content, 1, "my_key")

if not res then
    ngx.say("Error: ", err)
else
    ngx.say("Result: ", res)
end

-- Put it back in the connection pool
red:set_keepalive(10000, 100)

[shreemaan-abhishek]

done

[membphis]

another example, gen the sha1 by redis:script:

local function fetch_latest_redis_script_sha1()
    if redis_incoming_script_sha then
        return redis_incoming_script_sha
    end
    
    local sha1, load_err = red:script("LOAD", lua_script)
    if load_err then
        return nil, load_err
    end

    redis_incoming_script_sha = sha1
    return redis_incoming_script_sha
end

local sha1, err = fetch_latest_redis_script_sha1()
if not sha1 then
    ngx.log(ngx.ERR, "Failed to load Lua script into Redis: ", err)
    return nil, err
end

local res, err = red:evalsha(sha1, ...)
if err and core.string.has_prefix(err, "NOSCRIPT") then
    redis_incoming_script_sha = nil
end

[membphis]

it is more safer than we generate it by ngx api

[shreemaan-abhishek]

got it, I have fixed this too now.

FYI, lua-resty-redis-cluster didn't work with evalsha so I ensured a conditional usage of plain redis eval by rediscluster policy and evalsha usage by redis policy.