From 00828b99984f346526eebc8f990301d3540aa097 Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Fri, 14 Aug 2026 01:40:05 -0700 Subject: [PATCH] fix: concurrency limiter heals legacy INCR keys instead of failing open (#3574) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The zset rework of the redis concurrent backend (#3529) kept the same key names the old INCR counter used. Against a leftover string key every zset command in the acquire MULTI fails WRONGTYPE while the trailing EXPIRE still succeeds — so steady traffic refreshes the stale key forever, and the failed zcard result turned into NaN, which admitted every caller unbounded. Release then errored WRONGTYPE on each request (the '[concurrent] release failed' log storm). Surface per-command MULTI errors instead of coercing them to NaN, and on WRONGTYPE delete the legacy key and re-acquire against a clean zset. Co-authored-by: Claude Fable 5 --- src/backend/core/http/middleware/rateLimit.js | 60 +++++++++++++------ .../core/http/middleware/rateLimit.test.js | 22 +++++++ 2 files changed, 64 insertions(+), 18 deletions(-) diff --git a/src/backend/core/http/middleware/rateLimit.js b/src/backend/core/http/middleware/rateLimit.js index a479ab318..192bf3c9f 100644 --- a/src/backend/core/http/middleware/rateLimit.js +++ b/src/backend/core/http/middleware/rateLimit.js @@ -116,6 +116,20 @@ async function checkMemory(key, limit, windowMs) { // -- Redis backend --------------------------------------------------- +// ioredis MULTI/EXEC reports per-command failures inside the exec() result +// (as `[err, res]` pairs) rather than throwing, so a failed command's result +// reads as `undefined` — and `Number(undefined)` is NaN, which every +// comparison below treats as "under the limit". Pull results through this so +// a command failure surfaces like a thrown one instead of silently admitting. +function multiResult(results, i) { + const entry = results[i]; + if (Array.isArray(entry)) { + if (entry[0]) throw entry[0]; + return entry[1]; + } + return entry; +} + async function checkRedis( /** @type {import('ioredis').Cluster} */ redis, @@ -144,9 +158,7 @@ async function checkRedis( .pexpire(redisKey, windowMs) .exec(); - const count = Number( - Array.isArray(results[2]) ? results[2][1] : results[2], - ); + const count = Number(multiResult(results, 2)); if (count > limit) { await redis.zrem(redisKey, member); return false; @@ -216,7 +228,7 @@ async function acquireMemoryConcurrent(key, limit) { }; } -async function acquireRedisConcurrent(redis, key, limit) { +async function acquireRedisConcurrent(redis, key, limit, retried = false) { const redisKey = `concurrent:${key}`; const member = `${Date.now()}-${crypto.randomUUID()}`; // One sorted-set member per held slot, scored by acquire time — the same @@ -231,20 +243,32 @@ async function acquireRedisConcurrent(redis, key, limit) { // nobody is actually using. Here the sweep below drops each slot on its own // age, so a leak drains on schedule no matter how hard anyone retries. const now = Date.now(); - const results = await redis - .multi() - // Slots older than the orphan window belonged to a process that died - // before releasing; drop them before counting. - .zremrangebyscore(redisKey, 0, now - ORPHAN_SAFETY_TTL_MS) - .zadd(redisKey, now, member) - .zcard(redisKey) - // Key-level TTL is only garbage collection for a bucket that goes - // quiet — the per-member sweep above is what bounds a live one. - .expire(redisKey, ORPHAN_SAFETY_TTL_SEC) - .exec(); - const count = Number( - Array.isArray(results[2]) ? results[2][1] : results[2], - ); + let count; + try { + const results = await redis + .multi() + // Slots older than the orphan window belonged to a process that + // died before releasing; drop them before counting. + .zremrangebyscore(redisKey, 0, now - ORPHAN_SAFETY_TTL_MS) + .zadd(redisKey, now, member) + .zcard(redisKey) + // Key-level TTL is only garbage collection for a bucket that goes + // quiet — the per-member sweep above is what bounds a live one. + .expire(redisKey, ORPHAN_SAFETY_TTL_SEC) + .exec(); + count = Number(multiResult(results, 2)); + } catch (err) { + // Keys left behind by the INCR-counter version of this backend are + // plain strings, so every zset command above fails WRONGTYPE — while + // the EXPIRE at the end still succeeds, meaning steady traffic keeps + // refreshing the stale key and it never ages out on its own. Drop the + // legacy key and count against a clean one. + if (!retried && /WRONGTYPE/.test(err?.message ?? '')) { + await redis.del(redisKey); + return acquireRedisConcurrent(redis, key, limit, true); + } + throw err; + } if (count > limit) { await redis.zrem(redisKey, member); return { ok: false }; diff --git a/src/backend/core/http/middleware/rateLimit.test.js b/src/backend/core/http/middleware/rateLimit.test.js index ad4a7b206..c39361fa6 100644 --- a/src/backend/core/http/middleware/rateLimit.test.js +++ b/src/backend/core/http/middleware/rateLimit.test.js @@ -959,6 +959,28 @@ describe('acquireConcurrent — orphan recovery (redis)', () => { expect((await acquireConcurrent(key, 1)).ok).toBe(true); }); + + it('heals a legacy string counter key and enforces the limit again', async () => { + // The pre-zset implementation stored these buckets as INCR string + // counters. Against such a key every zset command fails WRONGTYPE + // inside the MULTI while the trailing EXPIRE succeeds — so traffic + // kept the stale key alive indefinitely, and the undefined zcard + // result (NaN) admitted every caller unbounded. + // The file-level afterEach resets wiring to memory after every test, + // so re-point the default at this suite's redis for this test. + configureRateLimit({ default: 'redis', redis }); + const key = 'legacy-counter'; + await redis.set(`concurrent:${key}`, '7'); + + const first = await acquireConcurrent(key, 1); + expect(first.ok).toBe(true); + // The key was rebuilt as a zset and the cap is live again. + expect(await redis.type(`concurrent:${key}`)).toBe('zset'); + expect((await acquireConcurrent(key, 1)).ok).toBe(false); + + await first.release(); + expect((await acquireConcurrent(key, 1)).ok).toBe(true); + }); }); // ── concurrency: subscription-based limits ──────────────────────────