fix(discord): pace requests to the rate-limit bucket - #212
Conversation
The hourly discord-roles sweep fired 7 requests into the per-guild
GET /guilds/{id}/members/{user_id} bucket within ~1s — the bot's own
membership preamble plus one per linked member — with no pacing of any
kind in the REST client. The guild has 6 linked members and the bucket
admits 5, so the last two members 429'd on every run. pg-boss's
retry ladder then replayed the identical burst, producing the observed
5 partial runs across ~19 minutes.
The client now:
- serializes requests sharing a bucket key through a per-key mutex and
waits out the window when x-ratelimit-remaining is exhausted, so the
6th and 7th calls queue instead of failing;
- honors retry-after (fractional seconds) on a 429, bounded at 3
attempts before falling through to the existing transient
DiscordApiError so pg-boss's job-level retry still owns the tail;
- treats x-ratelimit-scope: global as a client-wide backoff;
- logs 429s. assertOk discards headers and body on error, which is why
the original incident left nothing to diagnose from;
- caches the preamble (guild roles, bot user id, bot's own member) so a
sweep stops spending a members-bucket slot re-reading data that is
almost always unchanged.
The bucket key is a self-computed path template rather than Discord's
x-ratelimit-bucket header: that header is only known after a first
response, which cannot gate the first request of a burst — precisely
what failed here.
Retry-on-partial semantics in discord-roles.ts are deliberately
unchanged; five sibling jobs share that convention.
…thod Follow-ups from review of the rate-limit pacing commit. The 10007 branch is the one path that turns a 404 into a recoverable null instead of a throw, so the body reaching that comparison is now parsed by a schema rather than cast. A string "10007" was previously unequal-but-plausible at the comparison and reported as `code 10007`; it is now correctly a malformed envelope, so a member still in the guild cannot be deroled by a wrong-typed field. `routeKey` takes an HttpMethod union instead of a bare string, carried up through a DiscordRequestInit, so the role-mutation bucket branch is checkable when a method is added later. Member and DiscordRole are exported so callers can name them without re-deriving the inference. docs/ops.md records the second reason worker=1 is deliberate: the pacing state lives in one per-process closure, so a second worker would burst into the same guild limit without coordinating.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Discord REST client adds validated error parsing, typed methods, serialized rate-limit handling, bounded retries, timeouts, response cleanup, and selective caching. Tests cover malformed errors, retry behavior, global and bucket limits, pacing, and cache boundaries. Operations guidance documents the single-worker requirement. ChangesDiscord REST hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DiscordRestClient
participant RateLimitQueues
participant DiscordAPI
participant PreambleCache
DiscordRestClient->>PreambleCache: read cached preamble data
PreambleCache-->>DiscordRestClient: return cached data or miss
DiscordRestClient->>RateLimitQueues: wait for global and bucket capacity
RateLimitQueues->>DiscordAPI: send serialized request
DiscordAPI-->>RateLimitQueues: return response or rate-limit headers
RateLimitQueues-->>DiscordRestClient: return typed result or validated error
DiscordRestClient->>PreambleCache: store cacheable data
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/discord/rest.ts`:
- Around line 359-372: Update the 429 handling around the willRetry check so the
response body is also cancelled on the retry-exhausting attempt before returning
the response. Reuse the same non-awaited best-effort cancellation pattern as the
retry path, while preserving the existing return behavior and retry delay flow.
- Around line 424-433: Update getGuildRoles() to return a shallow copy of the
roles array on both the valid guildRolesCache path and the fresh parseBody
result path, while storing the original roles array in the cache so callers
cannot mutate cached data in place.
In `@tests/discord-rest.test.ts`:
- Around line 416-462: Add a focused non-finite-header case near the existing
burst test, using the member-fetch request handler to return one response with
x-ratelimit-remaining set to 0 and x-ratelimit-reset-after set to Infinity, then
verify the second fetch dispatches without timer advancement and no 429 response
is produced. Assert observable request/response behavior rather than internal
bucket state, and use fake timers with proper cleanup as in the existing test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 095573da-ddd6-4c23-98e9-2e9abccfeaa8
📒 Files selected for processing (3)
docs/ops.mdsrc/lib/discord/rest.tstests/discord-rest.test.ts
… cached roles array Three review findings on the pacing change, verified against the code first. The 429 body cancellation moves out of the willRetry branch. Nothing reads a 429 body on any path -- assertOk inspects only res.status, and the sole body read in fetchGuildMember is on 404 -- but undici holds the socket until the body is consumed or cancelled, so the retry-exhausting attempt was returning a response whose socket stayed pinned. That is the worst moment to leak one: the route is already rate limited. getGuildRoles now copies on the way out of both arms while still caching the original. No caller mutates it today (core/role-diff.ts only builds a Map from it), so this is hardening rather than a live bug -- but the cache is process-lifetime state shared by every sweep, so an in-place sort or splice would rewrite what the next five minutes of runs diff against. Adds the missing test for the non-finite reset-after guard. A response pairing remaining=0 with reset-after=Infinity must not enter the bucket: the second fetch has to dispatch with the clock untouched. Mutating the guard to Number.isNaN fails exactly this test with 'expected 1 to be 2' and nothing else.
1 participant
The failure
Production
discord-rolesruns were coming backpartial, five runs in a row, always the same two Discord IDs:Every run took under 1.1s and reported "no change".
Why
Each sweep puts 7 calls into the same per-guild members bucket inside about a second, with no pacing at all: the bot's own member lookup (part of the config preamble) plus 6 real members. The bucket admits 5. So the last two are rejected — deterministically, which is why it is the same two IDs each run and why the run finishes in under a second instead of slowing down.
The client classified 429 as transient but never read
retry-afterand never throttled, so nothing slowed the burst down and nothing recorded the 429 headers.assertOkdiscarded them, which is why the incident left no diagnostic trace beyondfailed (429).The pg-boss job-level retry (
retryLimit: 5,retryBackoff) was amplifying this, not causing it — each retry replayed the same unpaced burst.The fix
Pace to the bucket. Bucket state (
x-ratelimit-remaining/-reset-after) is recorded per route key and waited on before each request, so the wait learned from member #1 correctly paces member #6.Honor
retry-after. A 429 sleeps the header's duration and retries, bounded at 3 retries before surfacing the existing transient error and handing back to pg-boss.x-ratelimit-scope: globalopens a window across every bucket, recorded on the retry-exhausting attempt too — a global throttle that outlasts three attempts is exactly when other buckets most need to know.Cache the preamble. The guild roles and the bot's own member are cached (5 min TTL; the bot's user id indefinitely, since a token's id never changes). This removes the bot's own lookup from the members bucket. Real members are never cached — reading their current roles is the entire point of the call.
Log the 429 headers, so the next occurrence is diagnosable.
Route keys are templated from the path rather than taken from Discord's
x-ratelimit-bucket, because that id is only known after a response for the route — useless for gating the first request of a burst, which is precisely what failed here. Same-key requests are serialized through a mutex; the current caller loop is sequential, so this is insurance against a future concurrent caller silently degrading pacing to none.The second commit is review follow-up: the 404 error envelope is now schema-validated instead of cast (a string
"10007"was reported ascode 10007while not matching the branch — it is now correctly "malformed body"),routeKeytakes anHttpMethodunion, andMember/DiscordRoleare exported.Scope note
docs/ops.mdnow records a second reasonworker=1is deliberate: this pacing state lives in one per-process closure, so a second worker would burst into the same guild limit without coordinating. There is no cross-process coordination today. Worth a reviewer's attention — it makes a previously Wanderer-only constraint load-bearing for rate-limit correctness.Verification
The new tests were mutation-tested rather than assumed. Each mutation killed only its own test:
parsed > 0→>= 0(emptyretry-after→ 0s backoff)scope === "global"→&& willRetrysleep(retryAfterSec * 1000)→sleep(1000)z.number().optional()→z.any().optional()(cast semantics)This also exposed a pre-existing hole: the old
retry-aftertest advanced the full 1000ms, so it passed for a client that ignored the header entirely. It now advances 500ms and asserts the retry has already fired.Not verified against real Discord. The proof this fixes production is arithmetic (7 calls into a bucket of 5, matching every observed property), not an observed green run. First real signal is the next hourly tick after deploy.
Left open, deliberately
src/jobs/discord-roles.tsreturnsretry: trueon apartialresult, which contradicts the contract documented atsrc/services/sync-run.ts:41-46. Five sibling jobs share that convention and a test pins it, so changing it is a convention decision rather than part of this bug fix — and with correct pacing,transientFailuresshould reach 0 anyway.Summary by CodeRabbit
Bug Fixes
Performance
Documentation