fix(rate-limit): namespace Redis buckets so limiters stop sharing a window - #22
Merged
Merged
Conversation
…indow #15 gave every in-process rateLimit() instance its own bucket namespace, but rate-limit.js transparently swaps to redis-rate-limit.js whenever REDIS_URL is set — and that module keyed its sorted set on `prefix + rawKey`, ignoring the `name` option outright. So the fix only ever covered single-replica deployments. With Redis configured, auth (10/min), discovery (5/min), export (10/min) and sendEmail (20/min) still drew from ONE window per IP: five CSV exports exhausted the discovery limiter for that IP, and the tightest max effectively governed all four. consume() had the same gap. On Redis both it and the middleware were un-namespaced, so batch-send's reservation landed in the right bucket by accident, while sharing that bucket with every other limiter. - redis-rate-limit.js: mirror rate-limit.js's namespacing — rateLimit() takes `name` (positional `rl<n>` fallback), consume() takes `name` (default 'shared'), both key on `prefix + name|key` - warn once per process when a Redis limiter is built unnamed: positional names are safe in-process but only agree across replicas while limiter construction order is identical, and a disagreement silently splits one limiter into two buckets (doubling the effective cap) - index.js: name inviteLookupLimiter, the last unnamed limiter — all six are now explicit, so the warning never fires today - test stub: zadd only accepted (key, score, member), but consume() calls it variadically with n tickets. It was silently dropping every ticket past the first, which made a 50-recipient batch look like it cost one ticket Note: the key shape changes, so existing Redis buckets are orphaned on deploy. Harmless — worst case a few users get a fresh window. Verified by reverting namespaced() to a no-op: 4 of the 6 new tests fail against pre-fix behaviour, including the discovery-5/auth-10 scenario. npm test 662/662 green (was 656). node --check clean on all three files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
背景
#15给每个 in-processrateLimit()实例分配了独立的 bucket 命名空间。但server/rate-limit.js在REDIS_URL存在时会透明切换到redis-rate-limit.js,而后者把 sorted set 的 key 写成prefix + rawKey,完全忽略name选项。结果:#15 只修好了单副本路径。只要配了 Redis,生产环境仍然是老样子 —— auth (10/min)、discovery (5/min)、export (10/min)、sendEmail (20/min) 共用同一个 IP 窗口:
max形同虚设,最紧的那个实际上统治了全部四个consume()有同样的问题。在 Redis 路径上它和 middleware 都没有命名空间,所以 batch-send 的预留是碰巧落进了正确的 bucket —— 同时也和其他所有限流器共享这个 bucket。改动
server/redis-rate-limit.js—— 镜像rate-limit.js的命名空间实现:rateLimit()接受name,缺省回退到位置式rl<n>consume()接受name(默认'shared'),与rate-limit.js契约一致prefix + name|key建 key未命名限流器现在会 warn 一次(每进程)。 位置式名字在 in-process 下是安全的(bucket 是本地的),但在 Redis 下 bucket 跨副本共享 —— 两个副本对命名空间不一致时,会静默地把一个逻辑限流器拆成两个 bucket,实际上限翻倍。它只在"所有限流器都在模块加载时无条件、同序构造"的前提下成立。
server/index.js—— 给inviteLookupLimiter加上name,这是最后一个未命名的限流器。六个全部显式命名后,上面那条 warning 今天永远不会触发。测试桩 bug ——
zadd只接受(key, score, member)三参,但consume()是变参调用、一次塞n张票。桩把第一张之后的票全部静默丢弃了,这会让一个 50 收件人的批量看起来只花 1 张票。验证
6 个新测试。为确认它们不是同义反复,我把
namespaced()临时改回 no-op 跑了一遍 —— 4 个失败,包括 discovery-5 / auth-10 那个真实配置场景:npm test662/662 通过(原 656)node --check三个文件均干净部署注意
key 形状变了,已有的 Redis bucket 在部署后会被孤立。无害 —— 最坏情况是少数用户拿到一个新窗口。
未包含
docs/memory.md的"已关闭"表把限流器桶隔离记为 #15 已解决,实际只对了一半。这次没动,需要的话可以补一行修正。🤖 Generated with Claude Code