fix(rate-limiter): namespace EWMA keys away from sliding-window keys (#945)

Author: chappie-daemon

.changeset/ewma-key-namespace.md

@@ -0,0 +1,12 @@
+---
+"nostream": patch
+---
+
+Namespace EWMA rate-limiter keys so they cannot collide with sliding-window keys
+
+The two strategies store different Redis types under the same key name: a sliding
+window keeps a sorted set, EWMA keeps a hash. A cache volume carried over from 2.x,
+or an operator switching `limits.rateLimiter.strategy`, therefore left EWMA reading a
+sorted set as a hash — a `WRONGTYPE` error on every connection and message until the
+old key expired. EWMA now writes under an `ewma:` prefix, which also keeps both
+strategies' windows intact if the strategy is switched back.

src/utils/ewma-rate-limiter.ts

@@ -29,6 +29,21 @@ const rateLimitScript = {
     `,
   }
 
+// Every limiter key is namespaced by strategy, because the window layouts are
+// not interchangeable: a sliding window stores a sorted set under a key named
+// `<identity>:<bucket>:<period>`, while EWMA stores a hash under the same name.
+// The strategies share Redis and an operator can switch between them (and 2.x
+// shipped only the sliding window), so reading a key this strategy did not
+// write makes the script fail with WRONGTYPE on every request that hits it -
+// and the failure path is the hot path, per connection and per message.
+//
+// A prefix, rather than deleting a key of the wrong type, keeps both strategies'
+// data intact: switching back to sliding_window resumes its window instead of
+// starting from zero.
+export const EWMA_KEY_PREFIX = 'ewma:'
+
+export const ewmaKey = (key: string): string => `${EWMA_KEY_PREFIX}${key}`
+
 export const calculateEWMA = (
   rOld: number,
   deltaT: number,
@@ -52,7 +67,7 @@ export class EWMARateLimiter implements IRateLimiter {
     const { rate, period } = options
 
     const result = await this.cache.eval(rateLimitScript.SCRIPT,
-       [key],
+       [ewmaKey(key)],
        [Date.now().toString(), rate.toString(), period.toString(), step.toString()]
     )
 

test/integration/features/rate-limiter/rate-limiter.feature

@@ -4,4 +4,12 @@ Feature: Rate Limiter
     Given someone called Alice
     And Alice's message rate is already at the limit
     When Alice sends a text_note event expecting to be rate limited
-    Then Alice receives a notice with rate limited
+    Then Alice receives a notice with rate limited
+
+  @rate-limiter
+  Scenario: Alice's rate limit survives a sliding-window key left in Redis
+    Given someone called Alice
+    And Alice's Redis holds a sliding-window key left by an older version
+    And Alice's message rate is already at the limit
+    When Alice sends a text_note event expecting to be rate limited
+    Then Alice receives a notice with rate limited

test/integration/features/rate-limiter/rate-limiter.ts

@@ -7,6 +7,7 @@ import { createEvent } from '../helpers'
 import { Event } from '../../../../src/@types/event'
 import { getCacheConfig } from '../../../../src/cache/client'
 import { SettingsStatic } from '../../../../src/utils/settings'
+import { ewmaKey } from '../../../../src/utils/ewma-rate-limiter'
 
 let testCacheClient: any
 const rateLimitKeys: string[] = []
@@ -25,7 +26,9 @@ Given(/(\w+)'s message rate is already at the limit/, async function(name: strin
   const ws = this.parameters.clients[name] as WebSocket
   const address = (ws as any)._socket?.remoteAddress ?? '::1'
   const period = 60000
-  const key = `${address}:message:${period}`
+  // EWMA reads its own namespace; seeding the un-namespaced key would leave the
+  // limiter on a fresh window and the scenario would not rate limit at all.
+  const key = ewmaKey(`${address}:message:${period}`)
 
   await testCacheClient.hSet(key, {
     rate: '999',
@@ -54,6 +57,22 @@ When(/(\w+) sends a text_note event expecting to be rate limited/, async functio
   this.parameters.events[name].push(event)
 })
 
+Given(/(\w+)'s Redis holds a sliding-window key left by an older version/, async function(name: string) {
+  const ws = this.parameters.clients[name] as WebSocket
+  const address = (ws as any)._socket?.remoteAddress ?? '::1'
+  const period = 60000
+  // Exactly what a 2.x relay, or a sliding_window run, leaves behind: a sorted
+  // set under the key name EWMA used to share. Reading it as a hash raised
+  // WRONGTYPE on every message, which is the regression this guards.
+  const key = `${address}:message:${period}`
+
+  await testCacheClient.del(key)
+  await testCacheClient.zAdd(key, [{ score: Date.now(), value: `${Date.now()}:1` }])
+  await testCacheClient.pExpire(key, period)
+
+  rateLimitKeys.push(key)
+})
+
 After({ tags: '@rate-limiter' }, async function() {
   SettingsStatic._settings = pipe(
     assocPath(['limits', 'message', 'rateLimits'], []),

test/unit/utils/ewma-rate-limiter.spec.ts

@@ -1,7 +1,7 @@
 import { expect } from 'chai'
 import Sinon from 'sinon'
 
-import { calculateEWMA, EWMARateLimiter } from '../../../src/utils/ewma-rate-limiter'
+import { calculateEWMA, EWMA_KEY_PREFIX, EWMARateLimiter, ewmaKey } from '../../../src/utils/ewma-rate-limiter'
 import { ICacheAdapter } from '../../../src/@types/adapters'
 import { IRateLimiter } from '../../../src/@types/utils'
 
@@ -68,5 +68,22 @@ describe('EWMARateLimiter', () => {
         const result = await rateLimiter.hit('key', 1, { period: 120000, rate: 10 })
         expect(result).to.be.true
         })
+
+        // EWMA and sliding_window store different Redis types under the same
+        // logical key name, so they must not share the physical key: a 2.x
+        // volume, or an operator switching strategy, leaves a sorted set where
+        // EWMA would then HSET and fail with WRONGTYPE on every request.
+        it('namespaces the key it writes', async () => {
+        evalStub.resolves(0)
+        await rateLimiter.hit('1.2.3.4:connection:120000', 1, { period: 120000, rate: 10 })
+
+        expect(evalStub.calledOnce).to.be.true
+        expect(evalStub.firstCall.args[1]).to.deep.equal(['ewma:1.2.3.4:connection:120000'])
+        })
+
+        it('builds the namespaced key from any caller key', () => {
+        expect(ewmaKey('abc:message:60000')).to.equal('ewma:abc:message:60000')
+        expect(EWMA_KEY_PREFIX).to.equal('ewma:')
+        })
     })
 })