[API Policies] Advanced Rate Limit Policy Issues

Overview

This document outlines critical reliability issues found in the Advanced Rate Limit Policy implementation affecting Redis operations, GCRA algorithms, and thread safety.

---

Issues

1. Race Condition in Fixed Window Redis Operations

File: algorithms/fixedwindow/redis.go (lines 92-113)

Description:
The INCRBY and EXPIRE operations are not atomic. If INCRBY succeeds but EXPIRE fails, the key persists without a TTL, causing permanent rate limiting for that key.

Proposed Fix:
Use a Lua script to make both operations atomic:

local key = KEYS[1]
local ttl = ARGV[1]
local increment = ARGV[2]
local value = redis.call('INCRBY', key, increment)
redis.call('EXPIRE', key, ttl)
return value

Impact: Keys without TTL cause incorrect rate limiting behavior indefinitely.

---

2. Division by Zero in GCRA Policy

File: algorithms/gcra/policy.go (lines 47-51)

Description:
The EmissionInterval() function will panic with division by zero if p.Limit is 0.

Proposed Fix:
Add validation in NewPolicy() to ensure Limit > 0, or guard the division.

Impact: Panic when rate limit is configured with zero limit.

---

3. Unchecked Type Assertions in GCRA Redis

File: algorithms/gcra/redis.go (lines 117-136)

Description:
Type assertions are performed without checking, which will panic if the Lua script returns unexpected data types or fewer elements.

Proposed Fix:
Add defensive type checking before assertions, with proper error handling.

Impact: Unexpected Redis responses cause application panics.

---

4. Thread-Safety Issue in GCRA Memory

File: algorithms/gcra/memory.go (lines 66-70)

Description:
The clock field can be modified via WithClock while AllowN or cleanupLoop are reading it, creating a data race.

Proposed Fix:
Document that WithClock must be called before any operations, or protect the clock field with the existing mutex.

Impact: Concurrent access leads to undefined behavior and potential crashes.

---

5. Unassigned API Metadata Fields

File: ratelimit.go (lines 97-100)

Description:
Variables apiId, apiName, and apiVersion are declared but never assigned. They remain empty and are passed to getBaseCacheKey(), leaving cache keys and stored metadata incomplete.

Proposed Fix:
Populate these fields from appropriate sources (metadata, params, context) or remove if not needed.

Impact: Cache keys are incomplete, potentially causing incorrect rate limiting isolation.

---

6. Race Conditions in Test Clock Access

File: Test files for both fixedwindow and gcra algorithms

Description:
Data races detected in TestMemoryLimiter_CleanupExpired tests. Concurrent access to FixedClock without proper synchronization:

• Test goroutine writes to the clock
• Cleanup goroutine reads from the clock

Proposed Fix:
Add proper synchronization to the test clock implementation, or ensure clock is not modified after limiter initialization in tests.

Impact: Race detector failures in tests, potential undefined behavior in concurrent scenarios.

Reference

• https://github.com/wso2/gateway-controllers/pull/1#:~:text=%F0%9F%9F%A0%20Major%20comments%20(20,1%2D13%20(1)