fix(api): distributed rate limiting for the public read endpoints (GH#2487) - #2490
fix(api): distributed rate limiting for the public read endpoints (GH#2487)#2490dcccrypto wants to merge 1 commit into
Conversation
…#2487)
/api/trader/[wallet]/trades, /api/trader/[wallet]/stats and /api/stats
limited requests with createMemoryRateLimiter, whose state is a Map in
one process. On serverless that makes the effective limit
`configured x instance count` — a client spread across warm instances is
not bounded by the number on the tin, so the endpoints are open to the
scraping and profiling the limit exists to stop.
Switch all three to createUpstashRateLimiter, which shares the window
through Redis when configured and degrades to the same in-memory
behaviour when it is not — so local dev and CI are unchanged, and only
production gains the global bound.
GH#2487 names the two trader routes. /api/stats had the identical defect
and its own comment already predicted this ("Per-process only
(multi-instance: effective limit = 60 x N). At mainnet scale, replace
with Redis-backed rate limiting."), so it is included rather than left to
be rediscovered.
Each route gets a distinct Redis prefix (rl:trader-trades,
rl:trader-stats, rl:stats) so one endpoint cannot spend another's budget.
Limits, response bodies, 429 headers and getClientIp-based keying are
unchanged; X-RateLimit-Remaining now comes from the single check() result
instead of a second call.
Note the API is async: `if (rateLimiter.check(ip))` without await is a
Promise, always truthy, and would silently disable the limit. The tests
assert the awaited form at each call site for that reason.
Tests cover the wiring, not just the limiter — a limiter-only test stays
green with the routes reverted, which is how this shipped in the first
place.
Mutation-verified: reverting one route to the per-process limiter fails 3
tests; dropping the await fails 1.
Suite: 2937 passed / 0 failed. tsc --noEmit clean.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Fixes #2487.
Premise verified on
playground@f2a3bbe5: both trader routes constructcreateMemoryRateLimiter, whose state is aMapin one process. Confirmed.A third route has the same defect
/api/statsuses the identical pattern, and its own comment already predictedthis outcome:
Included here rather than left to be rediscovered separately.
/api/statswasthe only other
createMemoryRateLimitercaller in the app — after this, noneremain outside the lib and its own unit test.
The change
All three move to
createUpstashRateLimiter. As you noted, it already falls backto in-memory when Upstash is unconfigured, so dev and CI behave exactly as
before and only production gains the shared window.
Each route gets a distinct Redis prefix —
rl:trader-trades,rl:trader-stats,rl:stats— so one endpoint cannot spend another's budget. Limits, responsebodies, 429 headers and
getClientIp-based keying are unchanged;X-RateLimit-Remainingnow comes from the singlecheck()result rather than asecond call into the limiter.
One trap worth flagging for anyone doing this elsewhere: the new API is
async.if (rateLimiter.check(ip))withoutawaitis a Promise — alwaystruthy — so the check inverts into "always limited", and the mirror slip
(
if (!rateLimiter.check(ip))) silently disables the limit entirely whilelooking correct. The tests assert the awaited form at each call site for exactly
that reason.
On the suggested regression test
The proposed test fires 120 real
fetches at a live endpoint and expects ≥60 tobe 429. That needs a running server and a shared Redis, so in CI it would either
be skipped or assert the in-memory fallback — i.e. pass without testing the
distributed property. The property you actually care about (shared across
instances) can't be observed without Redis at all.
So the binding assertion here is the wiring: each route constructs the
distributed limiter, no longer imports the per-process one, uses a distinct
prefix, and awaits the check. That's what fails if this regresses. A test of the
limiter alone stays green with the routes reverted — which is how the
per-process version shipped in the first place.
Mutation-verified:
createMemoryRateLimiterawaitoncheck()Deployment note
If
UPSTASH_REDIS_REST_URL/_TOKENare not set in production, these threeendpoints keep exactly today's per-instance behaviour — the fix is inert until
Upstash is configured. #2478 (pending) makes that state alert loudly instead of
passing silently, which is the other half of making this real.