Repository navigation
fix(rate-limiter): namespace EWMA keys away from sliding-window keys - #945
Conversation
🦋 Changeset detectedLatest commit: 37685c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
8ce1593 to
5e994d2
Compare
|
🔍 Site preview for this PR (includes all changes on this branch): https://nostream-web-pr-945.cameri.workers.dev/preview/945/ The https://ema.tnsor.network/preview/945/ form serves the same build once this PR merges — the production router that proxies /preview/* ships with it. Updates with every push to this PR; deleted when the PR closes. |
 [Medium impact] The PR appears safe to merge; no actionable new issues were found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Coverage Report for CI Build 40101173613Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.003%) to 75.742%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
EWMA and sliding_window store different Redis types under the same key name: a sliding window keeps a sorted set, EWMA keeps a hash. Both write `<identity>:<bucket>:<period>`, and 2.x shipped only the sliding window, so a cache volume carried across the upgrade holds sorted sets where the EWMA script then runs HGET - a WRONGTYPE error raised on every connection and every message until the old key expires. Switching `limits.rateLimiter.strategy` back and forth does the same thing to a live instance. EWMA now writes under an `ewma:` prefix. A prefix rather than a delete-if-wrong-type guard, so both strategies keep their own window and a switch back to sliding_window resumes it instead of starting from zero. Reproduced before the change on a real Redis 6.2 with a zset seeded at the shared key name: `@user_script:6: WRONGTYPE`, the same error and line the issue reports. After it, the same call returns normally, writes a hash under `ewma:`, and leaves the legacy zset untouched. The unit test pins the key the script receives. The integration scenario seeds a real sliding-window zset under the shared name and asserts the client is still rate limited, so a regression fails on the behaviour rather than on a string.
5e994d2 to
7c650d9
Compare
Takes @cameri's call in the issue and @Perlover's diagnosis: namespace the EWMA keys. Closes #930.
The bug, reproduced
Seeded a real Redis 6.2 with a sliding-window sorted set at
1.2.3.4:connection:120000and ran the shipped EWMA script against it:Same error, same line as the report. Both strategies build their key the same way (
<identity>:<bucket>:<period>) but store different types — a sorted set versus a hash — so any key left bysliding_windowbreaks everyewmarequest that touches it.The fix
EWMARateLimiternow writes under anewma:prefix:A prefix rather than the self-healing
TYPE/DELguard the issue also suggested. Deleting the key would destroy the sliding window's state, so switchingstrategyback would silently start from a fresh window — and that switch is a normal operation, not an upgrade path.Affected call sites are unchanged; the prefix is applied once, inside the limiter, so nothing has to remember it.
Verification
hit()against a legacy zsetWRONGTYPE @user_script:6…:connection:120000(hash, collided)ewma:…:connection:120000(hash)zsetThe before/after used the built
EWMARateLimiterclass against real Redis 6.2, not a stub — a mocked adapter cannot produce aWRONGTYPE, which is exactly why the existing unit test never caught this.Tests. A unit test pins the key the script receives. An integration scenario seeds a real sliding-window zset under the shared name and still expects the client to be rate limited, so a regression fails on behaviour. The existing
@rate-limiterstep now seeds the namespaced key, since seeding the old name would leave the limiter on a fresh window and the original scenario would stop covering anything.Full unit suite: 2521 passing, 5 failing — the same
user-subscription-repositoryfailures at base, untouched by this change.tsc --noEmitandbiomeclean.Upgrade note
No manual Redis cleanup: old zsets stay where they are and simply stop being read. They expire on their own TTL, and the
ewma:keys start empty, so the first request after the upgrade is unrestricted — the same fresh-start a restart already gives a sliding window.