diff --git a/.changeset/ewma-key-namespace.md b/.changeset/ewma-key-namespace.md new file mode 100644 index 000000000..574cd028d --- /dev/null +++ b/.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. diff --git a/src/utils/ewma-rate-limiter.ts b/src/utils/ewma-rate-limiter.ts index 0f212c6b1..51e59f7e6 100644 --- a/src/utils/ewma-rate-limiter.ts +++ b/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 +// `::`, 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()] ) diff --git a/test/integration/features/rate-limiter/rate-limiter.feature b/test/integration/features/rate-limiter/rate-limiter.feature index 52508e4b9..7e226aff6 100644 --- a/test/integration/features/rate-limiter/rate-limiter.feature +++ b/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 \ No newline at end of file + 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 diff --git a/test/integration/features/rate-limiter/rate-limiter.ts b/test/integration/features/rate-limiter/rate-limiter.ts index 5737a440c..8c4e3b4b3 100644 --- a/test/integration/features/rate-limiter/rate-limiter.ts +++ b/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'], []), diff --git a/test/unit/utils/ewma-rate-limiter.spec.ts b/test/unit/utils/ewma-rate-limiter.spec.ts index 53ec8f62e..72678789c 100644 --- a/test/unit/utils/ewma-rate-limiter.spec.ts +++ b/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:') + }) }) })