feat(orchestrator): per-client token-bucket rate limiter for LLM routes#65
feat(orchestrator): per-client token-bucket rate limiter for LLM routes#65hoangsonww wants to merge 1 commit into
Conversation
Adds a reliability primitive alongside the circuit breaker, cost tracker, and DLQ: an in-memory token-bucket rate limiter that guards the expensive LLM-backed routes from runaway usage / abuse. - core/rate-limiter.js: `RateLimiter` (per-key buckets, configurable capacity + refill, injectable clock for deterministic tests) and an Express `rateLimitMiddleware` factory that sets X-RateLimit-* headers and returns 429 + Retry-After when a bucket is empty. - index.js: guard /api/supervisor/process, /api/agent/run, and /api/batch/process; expose GET /api/rate-limit stats; include rate-limit in /health. Tunable via RATE_LIMIT_CAPACITY / RATE_LIMIT_REFILL_PER_SEC. - 14 new unit tests (bucket limits, refill/cap, per-key isolation, cost > 1, reset/resetAll, stats, config validation, and the middleware). Full suite: 116/116 passing. - Documented in README and .env.example. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for docuthinker-ai-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Code Review
This pull request introduces an in-memory token-bucket rate limiter and middleware to protect expensive LLM-backed routes from runaway usage, along with corresponding configuration, endpoints, and unit tests. While the implementation is clean, several critical issues were identified: an unbounded Map for client buckets poses a memory leak and DoS risk; deploying behind a reverse proxy without configuring Express's trust proxy will cause all clients to share a single bucket; static costs do not reflect the varying loads of different routes; and requests with a cost exceeding the bucket's capacity will be permanently blocked.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| this.capacity = capacity; | ||
| this.refillPerSec = refillPerSec; | ||
| this.now = now || (() => Date.now()); | ||
| this.buckets = new Map(); // key -> { tokens, lastRefill } |
There was a problem hiding this comment.
Using an unbounded Map to store client buckets in-memory introduces a potential memory leak and Denial of Service (DoS) vulnerability. Every unique client IP/key will create a new entry in this.buckets that is never cleaned up. Over time, or under a distributed traffic/DoS attack with rotating IPs, this map will grow indefinitely and eventually crash the process due to an Out-Of-Memory (OOM) error.
To mitigate this, you can implement a simple cleanup mechanism. Since any bucket that hasn't been active for capacity / refillPerSec seconds is guaranteed to be fully refilled, it can be safely deleted from the map. Alternatively, you can use a limited-size LRU cache or run a periodic sweep to delete idle buckets.
| capacity: parseInt(process.env.RATE_LIMIT_CAPACITY || "60", 10), | ||
| refillPerSec: parseFloat(process.env.RATE_LIMIT_REFILL_PER_SEC || "1"), | ||
| }); | ||
| const llmRateLimit = rateLimitMiddleware(rateLimiter); |
There was a problem hiding this comment.
When using req.ip as the rate-limiting key, Express requires app.set('trust proxy', true) (or a specific proxy configuration) to be enabled if the application is deployed behind a reverse proxy (e.g., Nginx, Cloudflare, AWS ALB, Kubernetes Ingress). Without this setting, req.ip will resolve to the upstream proxy's IP address for all requests, causing all clients to share a single rate-limiting bucket and leading to accidental global rate limiting.
Consider adding app.set('trust proxy', true) (or your specific proxy trust configuration) right after initializing app in index.js.
| function rateLimitMiddleware(limiter, { keyFn, cost = 1 } = {}) { | ||
| return (req, res, next) => { | ||
| const key = (keyFn ? keyFn(req) : req.ip) || "global"; | ||
| const result = limiter.tryConsume(key, cost); |
There was a problem hiding this comment.
The current middleware implementation only supports a static cost value. However, the guarded routes have vastly different resource consumption profiles: /api/supervisor/process runs a single pipeline, /api/agent/run can run up to 10 iterations, and /api/batch/process processes an array of documents. Applying a static cost of 1 to all of them does not accurately reflect their actual load.
Allowing cost to be a function (req) => number enables dynamic cost calculation (e.g., scaling with the number of documents in a batch request).
| function rateLimitMiddleware(limiter, { keyFn, cost = 1 } = {}) { | |
| return (req, res, next) => { | |
| const key = (keyFn ? keyFn(req) : req.ip) || "global"; | |
| const result = limiter.tryConsume(key, cost); | |
| function rateLimitMiddleware(limiter, { keyFn, cost = 1 } = {}) { | |
| return (req, res, next) => { | |
| const key = (keyFn ? keyFn(req) : req.ip) || "global"; | |
| const computedCost = typeof cost === "function" ? cost(req) : cost; | |
| const result = limiter.tryConsume(key, computedCost); |
| tryConsume(key, cost = 1) { | ||
| const bucket = this._refill(key); |
There was a problem hiding this comment.
If a request is made with a cost greater than the rate limiter's capacity, it will be permanently blocked. This is because the bucket's token count is capped at capacity, so bucket.tokens >= cost will always be false. Furthermore, the returned retryAfterMs will be calculated based on a deficit that can never be resolved, returning a misleading retry time to the client.
Consider validating that cost <= this.capacity at the beginning of tryConsume and throwing an error to fail fast.
tryConsume(key, cost = 1) {
if (cost > this.capacity) {
throw new Error("RateLimiter: cost " + cost + " exceeds capacity " + this.capacity);
}
const bucket = this._refill(key);
Summary
Adds a token-bucket rate limiter to the orchestrator — a new reliability primitive that sits alongside the existing circuit breaker, cost tracker, and dead-letter queue. It guards the expensive LLM-backed routes from runaway usage / abuse (which directly maps to API spend), per client.
What's included
core/rate-limiter.jsRateLimiter— per-key token buckets with configurablecapacityandrefillPerSec. The clock is injectable (now) so refill behavior is fully deterministic in tests (no real timers/sleeps). API:tryConsume(key, cost),getState(key),reset(key),resetAll(),getStats(); validates its config.rateLimitMiddleware(limiter, { keyFn, cost })— Express factory that setsX-RateLimit-Limit/X-RateLimit-Remainingon every response and, when a bucket is empty, returns429 Too Many Requestswith aRetry-Afterheader and{ error, retryAfterMs }.index.jswiring/api/supervisor/process,/api/agent/run,/api/batch/process(keyed by client IP).GET /api/rate-limitstats route; rate-limit stats also surfaced in/health.RATE_LIMIT_CAPACITY(default 60) andRATE_LIMIT_REFILL_PER_SEC(default 1).Docs:
README.md(new endpoint section + env) and.env.example.Testing (TDD)
Built test-first (red → green). 14 new unit tests cover capacity limits, time-based refill + cap, per-key isolation,
cost > 1, reset/resetAll, stats, config validation, and the middleware (headers, 429 + Retry-After, customkeyFn).Notes
orchestrator/per the project's module conventions.🤖 Generated with Claude Code