Repository navigation
Conversation
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
Sliding window Lua fix honors requestCount via a zadd loop but records non-unique ZSET members, so usage is under-counted and the limiter over-admits. The PR's own added tests assert ZCARD values the implementation cannot produce.
Evidence
- sliding_window_request_rate_limiter.lua RIGHT line 49:
redis.call('zadd', tokens_key, now, timestamp_key .. ':' .. i)builds the member only from the constant per-handletimestamp_key(KEYS[2]) and the loop index; successive calls to the same handle re-add identical members, so ZCARD never exceedsrequested. - PR tests contradict the code: with FIXED_NOW and identical keys,
slidingWindowHonoursRequestedCountasserts ZCARD=10 andslidingWindowUnitsAreUniquePerCallasserts ZCARD=4, but member collisions yield ZCARD=5 and ZCARD=2 respectively. - RedisRateLimiter.java:60 supplies requestCount as ARGV[4]; the script reads it, but corrective counting is defeated by non-unique members, causing the limiter to over-admit (rate-limit bypass).
Recommended next steps
- Use a per-request unique member (e.g. a request-scoped UUID or monotonic counter) instead of
timestamp_key .. ':' .. i, so repeated requests to the same handle accumulate distinct ZSET entries and ZCARD reflects true usage. - Re-run the added unit tests;
slidingWindowHonoursRequestedCount(expects ZCARD=10) andslidingWindowUnitsAreUniquePerCall(expects ZCARD=4) currently assert counts the implementation cannot produce with constant per-handle members.
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: eff07857-dc99-46dd-b7a5-e0914f3b849c · Commit: 549542248b5e31a92da6737de73aadf856f2a817
Review verdict: NEEDS_CHANGES
The sliding window script now reads ARGV[4]=requestCount and records requested zadd entries, but the ZSET member is timestamp_key .. ':' .. i, which is constant for a given handle id. Because KEYS[2] (timestamp_key) is fixed per handle, repeated requests to the same handle re-add identical members and ZCARD never grows beyond requested, so usage is under-counted and the limiter over-admits (defeating the sliding-window limit). The PR's own added tests assert ZCARD=10/4, which the implementation cannot produce, confirming the fix is incorrect.
Publication: REQUEST_CHANGES (VERIFIED_BLOCKING_FINDINGS)
- shenyu-plugin/shenyu-plugin-fault-tolerance/shenyu-plugin-ratelimiter/src/main/resources/META-INF/scripts/sliding_window_request_rate_limiter.lua:49 [RIGHT] ZSET member
timestamp_key .. ':' .. iis constant per handle (KEYS[2] is fixed for a given id), so repeated requests to the same handle re-add identical members; ZCARD never exceedsrequested, under-counting usage and allowing the limiter to over-admit (rate-limit bypass). This also makes the PR's own tests asserting ZCARD=10/4 fail.
|
@Aias00 The finding rests on an assumption that doesn't hold: This uniqueness mechanism is pre-existing, not introduced here: the unmodified script already did The added tests exercise exactly this boundary and pass against real Redis: If a per-timestamp member (e.g. |
Fixes #8442.
Problem
RedisRateLimiterforwardsrequestCountto every algorithm as the fourth script argument, butsliding_window_request_rate_limiter.luanever readsARGV[4]: it admits while the number of recorded entries is belowcapacityand records exactly one entry per request. A rule withalgorithmName=slidingWindowandrequestCount=5therefore admits five times the traffic of the same handle undertokenBucket, andremain_requestis computed before the current request is recorded, so it always over-reports by one.Approach
The script now honours
requestCount: it admits only whenlast_requested + requested <= capacity, recordsrequestedunits in the existing sorted set (membersKEYS[2] .. ':' .. i, the per-call unique key already supplied bySlidingWindowRateLimiterAlgorithm.getKeys), and returnscapacity - last_requested - requestedafter recording, clamped at zero on rejection. No new keys, no key-schema change and no migration:zcardremains the usage counter, total members stay bounded bycapacitybecause admission requireslast_requested + requested <= capacity, and the TTL is unchanged.requestCountis a request count, so non-positive or missing values normalize to one request (fail-safe, never free traffic) and fractions round up to whole units, so a fractional count never admits more traffic than configured. No Java change is needed:RedisRateLimiteralready passes the value, andRateLimiterPluginonly consumes the allowed flag, so the remainder is not client-visible. The token/leakyrequestCount=0quirk and the missing AdminrequestCountfield are pre-existing and out of scope.This intentionally makes existing
slidingWindowrules withrequestCount > 1enforce the configured weight instead of one unit per request.Testing
Nine new deterministic cases run the real script against embedded Redis with fixed epoch arguments and windows of several seconds, so no sleeping is involved and a slow CI cannot expire a key mid-test. They cover the issue's 10-requests-at-requestCount-5 scenario (two admitted, matching
tokenBucket), the exact recorded unit count, per-call member uniqueness, non-negative remainders, zero/negative normalization, fractional rounding, and whole-unit expiry. The two existing sliding-window expectations are updated from the pre-fix remainder values (100->99,1->0); they now also pass the algorithm's real two-key list.shenyu-plugin-ratelimiter56 tests (7 new) green; Checkstyle and Apache RAT clean.