From ffc45092869c3baea0e7aaf30da5b58a0b546681 Mon Sep 17 00:00:00 2001 From: guarzo Date: Mon, 10 Aug 2026 08:34:10 -0400 Subject: [PATCH 1/3] fix(discord): pace requests to the rate-limit bucket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/lib/discord/rest.ts | 353 +++++++++++++++++++++++++++++++++---- tests/discord-rest.test.ts | 347 ++++++++++++++++++++++++++++++++++-- 2 files changed, 656 insertions(+), 44 deletions(-) diff --git a/src/lib/discord/rest.ts b/src/lib/discord/rest.ts index 6a1c2d51..e0d1b4f4 100644 --- a/src/lib/discord/rest.ts +++ b/src/lib/discord/rest.ts @@ -48,6 +48,7 @@ const memberSchema = z.object({ .catch(null), }); const userSchema = z.object({ id: z.string() }); +type Member = z.infer; /** Malformed bodies are deterministic — fail closed as permanent, never * retry-loop. Reads the body here so invalid JSON classifies the same way as @@ -67,24 +68,273 @@ async function parseBody( } } +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +/** + * Discord always sends `retry-after` (seconds, may be fractional) on a real + * 429. A missing or unparseable value is unexpected, but must not throw or + * wait forever — 1s is a short, safe default that still backs off rather than + * hammering the route again immediately. + * + * The floor is `> 0`, not `>= 0`, because `Number("")` is `0`, not `NaN`: an + * empty `retry-after` would otherwise pass a `>= 0` guard and return a 0s + * backoff — the exact immediate re-hammer this default exists to prevent. + * A literal `retry-after: 0` is treated the same way, and deliberately: the + * next attempt is worth one second of patience either way. + */ +function parseRetryAfterSeconds(res: Response): number { + const header = res.headers.get("retry-after"); + const parsed = header === null ? NaN : Number(header); + return Number.isFinite(parsed) && parsed > 0 ? parsed : 1; +} + +/** + * A real 429 that keeps recurring past this many attempts is not something a + * few more seconds of backoff will fix — either Discord is having a bad day + * on this route or something is misconfigured. Give up and surface the + * existing transient `DiscordApiError`: pg-boss's own job-level retry (see + * the `retry` field `discord-roles.ts` returns) takes over from there, on its + * own schedule. + * + * What this bounds is how long OTHER work waits, not whether it waits: every + * `getGuildMember` shares one `routeKey` and is serialized through `enqueue`, + * so each member behind the one currently retrying is already blocked. The + * cap is what keeps that block to ~seconds inside a single job tick instead + * of unbounded. + */ +const MAX_429_RETRIES = 3; + +/** + * ~5 minutes. `getGuildRoles` and the bot's own guild member rarely change + * between runs, but they CAN — an operator fixing role hierarchy or + * permissions after a misconfiguration is exactly the case + * `validateRoleConfig` exists to catch, and `botMemberCache` caches a `null` + * bot member (kicked, not yet re-invited) just as readily as a present one. + * Caching them delays detecting either recovery by up to this TTL, during + * which `postOpsWebhook` keeps alerting about an already-fixed condition. + * + * Note what this TTL does and does not buy. The scheduled `discord-roles` run + * is hourly (`"15 * * * *"` in `src/core/schedules.ts`) and fetches the + * preamble once per run, before the member loop — so for the anchor sweep the + * entry is always expired by the next tick and the cache neither helps nor + * costs anything. It earns its keep only when two runs land within the TTL: + * the account- and discord-user-scoped `discord-roles` jobs that + * `src/core/dispatch-plan.ts` fans out per outbox event, which cluster. Do not + * widen this window on the theory that it is saving the sweep a fetch. + */ +const PREAMBLE_TTL_MS = 5 * 60 * 1000; + export function createDiscordClient(cfg: Config, fetchImpl: typeof fetch = fetch) { - async function rawRequest(path: string, init: RequestInit = {}): Promise { - try { - return await fetchImpl(`${API}${path}`, { - ...init, - headers: { - authorization: `Bot ${cfg.discord.botToken}`, - "content-type": "application/json", - ...(init.headers as Record | undefined), - }, - signal: AbortSignal.timeout(30_000), - }); - } catch (err) { - throw new DiscordApiError( - `discord request failed: ${err instanceof Error ? err.message : String(err)}`, - { transient: true }, - ); + /** + * Per-bucket rate-limit state, keyed by `routeKey` below (our own path + * template, NOT Discord's opaque `x-ratelimit-bucket` id — see that + * function's comment for why). Populated from `x-ratelimit-remaining` / + * `x-ratelimit-reset-after` on every response that carries them. + * + * `buckets`, `chains`, and `globalResetAt` all live in this closure, so + * this pacing is per-PROCESS, not cluster-wide. `createDiscordClient` is + * constructed once per worker process (see `src/worker/index.ts`), and the + * `worker` group runs a single machine, so one closure is the whole picture + * today. That count is NOT visible in `fly.toml` — machine count is not a + * field there (`fly scale count` sets it); `docs/ops.md` under "Sizing" is + * where it is recorded, as "`worker=1`, deliberately." Scaling `worker` + * past one machine would give each process its own independent view of the + * buckets and reintroduce uncoordinated bursts across machines — this file + * does not attempt any cross-process coordination (e.g. via Redis/Postgres) + * for that case. + */ + const buckets = new Map(); + /** + * Set when a response carries `x-ratelimit-scope: global`, which means + * back off EVERY route, not just the one that got the 429. Checked before + * every request regardless of its own bucket key. + */ + let globalResetAt = 0; + /** + * Per-bucket-key mutex: chains requests sharing a key so they run one at a + * time, in call order. This is what makes the plain "wait if remaining is + * exhausted" check below race-free — without it, several calls fired + * concurrently could all read "remaining > 0" before any of them has + * updated it from a response. + * + * Note this is NOT the shape of the incident: `discord-roles.ts` awaits + * `getGuildMember` inside a sequential `for` loop, so that burst was + * fast-but-serial and `waitForCapacity` alone paces it. The mutex is here + * for the concurrency no current caller produces but any future one could + * — two `discord-roles` jobs overlapping, or a `Promise.all` over members — + * where the un-mutexed check would silently degrade to no pacing at all. + */ + const chains = new Map>(); + + function enqueue(key: string, task: () => Promise): Promise { + const prior = chains.get(key) ?? Promise.resolve(); + // Run `task` after `prior` settles either way — a failed request must not + // permanently jam every later request sharing its bucket key. + const result = prior.then(task, task); + chains.set( + key, + result.then( + () => undefined, + () => undefined, + ), + ); + return result; + } + + /** + * Discord's real bucketing is keyed by an opaque `x-ratelimit-bucket` id + * that is only known AFTER the first response for a route — useless for + * gating the FIRST request of a burst, which is exactly what mattered in + * the incident this fixes (calls landing in a bucket with zero prior + * responses to learn an id from). Templating the path ourselves instead + * gives a key usable from the very first call, at the cost of potentially + * grouping routes together that Discord's real scheme might keep separate. + * For the small, fixed set of routes this client calls, that's a feature: + * every `getGuildMember` call, regardless of user id, shares one key, which + * is what lets the bucket state learned from member #1 correctly pace the + * wait before member #6. + */ + function routeKey(method: string, path: string): string { + const KNOWN_SEGMENTS = new Set(["guilds", "roles", "members", "users", "@me"]); + const templated = path + .split("/") + .map((segment) => (segment === "" || KNOWN_SEGMENTS.has(segment) ? segment : ":id")) + .join("/"); + // Conservative choice: neither the author nor review could confirm from + // Discord's docs whether adding a role (PUT) and removing one (DELETE) + // on this same route share one real bucket or two. Sharing ONE key across + // both methods risks over-throttling — serializing add/remove calls + // Discord might actually pace independently — rather than + // under-throttling, which is the failure mode that caused the incident + // this file exists to fix. Confirm the real keying later from the + // `x-ratelimit-bucket` header in production logs (that header is NOT + // used as the key itself — see the comment above this function for why) + // before splitting this back into two. + const ROLE_MUTATION_PATH = "/guilds/:id/members/:id/roles/:id"; + if ((method === "PUT" || method === "DELETE") && templated === ROLE_MUTATION_PATH) { + return `ROLE_MUTATION ${templated}`; } + return `${method} ${templated}`; + } + + async function waitForCapacity(key: string): Promise { + // Loops rather than checking each condition once: sleeping out a BUCKET + // window can take us past the moment another request on a different key + // received a global-scoped 429 and opened a global window. A single pass + // would read `globalResetAt` before that happened and never look again, + // issuing the request straight into the global throttle. Terminates + // because each branch sleeps until its own deadline has passed, and a + // deadline is only ever pushed forward by a fresh 429. + for (;;) { + const now = Date.now(); + if (now < globalResetAt) { + await sleep(globalResetAt - now); + continue; + } + const bucket = buckets.get(key); + if (bucket && bucket.remaining <= 0 && bucket.resetAt > now) { + await sleep(bucket.resetAt - now); + continue; + } + return; + } + } + + function recordBucketHeaders(key: string, res: Response): void { + const remainingHeader = res.headers.get("x-ratelimit-remaining"); + const resetAfterHeader = res.headers.get("x-ratelimit-reset-after"); + if (remainingHeader === null || resetAfterHeader === null) return; + const remaining = Number(remainingHeader); + const resetAfter = Number(resetAfterHeader); + // `Number.isFinite`, not `!Number.isNaN`: `Number("Infinity")` is not NaN, + // and an infinite `resetAt` makes `waitForCapacity` compute an infinite + // wait, which Node silently clamps to 1ms (with a TimeoutOverflowWarning) + // — disabling pacing for this key instead of enforcing it. Negative values + // are rejected for the same reason: they would record an already-expired + // window as if it were a live one. + if (!Number.isFinite(remaining) || !Number.isFinite(resetAfter) || resetAfter < 0) { + return; + } + buckets.set(key, { remaining, resetAt: Date.now() + resetAfter * 1000 }); + } + + async function rawRequest(path: string, init: RequestInit = {}): Promise { + const method = (init.method ?? "GET").toUpperCase(); + const key = routeKey(method, path); + return enqueue(key, async () => { + for (let attempt = 0; ; attempt++) { + await waitForCapacity(key); + let res: Response; + try { + res = await fetchImpl(`${API}${path}`, { + ...init, + headers: { + authorization: `Bot ${cfg.discord.botToken}`, + "content-type": "application/json", + ...(init.headers as Record | undefined), + }, + signal: AbortSignal.timeout(30_000), + }); + } catch (err) { + throw new DiscordApiError( + `discord request failed: ${err instanceof Error ? err.message : String(err)}`, + { transient: true }, + ); + } + recordBucketHeaders(key, res); + if (res.status === 429) { + const retryAfterSec = parseRetryAfterSeconds(res); + const scope = res.headers.get("x-ratelimit-scope"); + const willRetry = attempt < MAX_429_RETRIES; + // `scope: global` means Discord is throttling the whole bot token, + // not just this route — every OTHER bucket needs to back off too, + // not only the one that happened to receive this 429. Recorded on + // EVERY 429 including the retry-exhausting one: a global throttle + // that outlasts three attempts is exactly when the other buckets + // most need to know, and gating this on `willRetry` would drop it + // precisely then. `Math.max` so a later, shorter window can never + // shrink one that is already open. + if (scope === "global") { + globalResetAt = Math.max(globalResetAt, Date.now() + retryAfterSec * 1000); + } + // Overwrite whatever `recordBucketHeaders` just derived from + // x-ratelimit-remaining/-reset-after above with the SAME window + // `retry-after` gives us, rather than trusting both independently. + // Today Discord's `retry-after` and its reset-after header agree, + // but nothing guarantees that; if they diverged, the bucket state + // above and the `sleep` below would be sourced from two different + // header pairs, and next attempt's `waitForCapacity` could sleep a + // SECOND time off whichever window is longer. Pinning the bucket + // to the same `retryAfterSec` we are about to sleep makes the + // upcoming `waitForCapacity` compute ~0 by construction. + buckets.set(key, { remaining: 0, resetAt: Date.now() + retryAfterSec * 1000 }); + // The 429 response body/headers are otherwise discarded by + // `assertOk` below, which is why the production incident this + // fixes left no trace. Logging here is the diagnostic record — + // and it runs on the EXHAUSTING attempt too, which is the one an + // operator investigating a surfaced `failed (429)` needs to see. + console.error( + `discord rate limited: ${method} ${path} retry-after=${retryAfterSec}s ` + + `scope=${scope ?? "route"} attempt=${attempt + 1}/${MAX_429_RETRIES + 1}` + + (willRetry ? "" : " (retries exhausted)"), + ); + if (willRetry) { + // Nothing ever reads a retried response's body, and undici holds + // the socket until the body is consumed or cancelled. Released + // deliberately WITHOUT awaiting: under the mocked fetch this + // suite uses, `cancel()` never settles, and awaiting it hung + // every 429 retry test. The retry must not be gated on a + // best-effort cleanup either way. + void res.body?.cancel().catch(() => undefined); + await sleep(retryAfterSec * 1000); + continue; + } + } + return res; + } + }); } function assertOk(res: Response, method: string, path: string): Response { @@ -103,34 +353,69 @@ export function createDiscordClient(cfg: Config, fetchImpl: typeof fetch = fetch const guild = cfg.discord.guildId; + // The preamble the config-check in `discord-roles.ts` runs on EVERY sweep: + // the guild's roles, the bot's own user id, and the bot's own guild member + // (for its current role ids). None of these need a fresh fetch every run — + // see `PREAMBLE_TTL_MS` above for the tradeoff that caching them accepts. + let botUserIdCache: string | null = null; + let guildRolesCache: { roles: z.infer[]; expiresAt: number } | null = + null; + let botMemberCache: { member: Member | null; expiresAt: number } | null = null; + + /** null ONLY for Discord code 10007 (Unknown Member — user not in guild). + * Any other 404 (10004 Unknown Guild = bad config, malformed body) is a + * permanent error: the role job must fail loudly, not skip everyone. */ + async function fetchGuildMember(userId: string): Promise { + const path = `/guilds/${guild}/members/${userId}`; + const res = await rawRequest(path); + if (res.status === 404) { + const body = (await res.json().catch(() => undefined)) as + { code?: number } | undefined; + if (body?.code === 10007) return null; + throw new DiscordApiError( + `discord GET ${path} failed (404${body?.code !== undefined ? `, code ${body.code}` : ", malformed body"})`, + { status: 404, transient: false }, + ); + } + assertOk(res, "GET", path); + return parseBody(memberSchema, res, "GET", path); + } + return { async getGuildRoles() { + if (guildRolesCache && Date.now() < guildRolesCache.expiresAt) { + return guildRolesCache.roles; + } const path = `/guilds/${guild}/roles`; const res = await request(path); - return parseBody(z.array(roleSchema), res, "GET", path); + const roles = await parseBody(z.array(roleSchema), res, "GET", path); + guildRolesCache = { roles, expiresAt: Date.now() + PREAMBLE_TTL_MS }; + return roles; }, + // The bot's own id is fixed for the lifetime of the token — cached + // indefinitely, no TTL needed (there is no "operator fixed it" case to + // detect: a bot token doesn't get reissued a new user id). async getBotUserId(): Promise { + if (botUserIdCache !== null) return botUserIdCache; const path = "/users/@me"; const res = await request(path); - return (await parseBody(userSchema, res, "GET", path)).id; + botUserIdCache = (await parseBody(userSchema, res, "GET", path)).id; + return botUserIdCache; }, - /** null ONLY for Discord code 10007 (Unknown Member — user not in guild). - * Any other 404 (10004 Unknown Guild = bad config, malformed body) is a - * permanent error: the role job must fail loudly, not skip everyone. */ - async getGuildMember(userId: string): Promise | null> { - const path = `/guilds/${guild}/members/${userId}`; - const res = await rawRequest(path); - if (res.status === 404) { - const body = (await res.json().catch(() => undefined)) as - { code?: number } | undefined; - if (body?.code === 10007) return null; - throw new DiscordApiError( - `discord GET ${path} failed (404${body?.code !== undefined ? `, code ${body.code}` : ", malformed body"})`, - { status: 404, transient: false }, - ); + async getGuildMember(userId: string): Promise { + // Only the BOT's own member lookup is cached. Every other member fetch + // must stay live — the whole point of this call for a real member is + // to read their CURRENT roles for the diff, and a stale cache there + // would silently paper over an actual role drift instead of fixing it. + const isBot = botUserIdCache !== null && userId === botUserIdCache; + if (isBot && botMemberCache && Date.now() < botMemberCache.expiresAt) { + return botMemberCache.member; + } + const member = await fetchGuildMember(userId); + if (isBot) { + botMemberCache = { member, expiresAt: Date.now() + PREAMBLE_TTL_MS }; } - assertOk(res, "GET", path); - return parseBody(memberSchema, res, "GET", path); + return member; }, // Dry-run guarded; the reads above are not, so the role diff stays real. async addMemberRole(userId: string, roleId: string): Promise { diff --git a/tests/discord-rest.test.ts b/tests/discord-rest.test.ts index b6a18084..80225717 100644 --- a/tests/discord-rest.test.ts +++ b/tests/discord-rest.test.ts @@ -1,6 +1,6 @@ import { http, HttpResponse } from "msw"; import { setupServer } from "msw/node"; -import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { createDiscordClient, DiscordApiError } from "@/lib/discord/rest"; import { testConfig } from "./helpers/config"; @@ -157,17 +157,344 @@ describe("createDiscordClient", () => { expect((err as DiscordApiError).transient).toBe(false); }); - it("classifies 429 as transient", async () => { + // No `retry-after` header here, so every attempt falls back to the client's + // 1s default — three retries under `MAX_429_RETRIES` add up to real seconds + // of sleeping. Fake timers keep this test instant instead of slow. + it("classifies 429 as transient, after exhausting its bounded retries", async () => { + vi.useFakeTimers(); + try { + let calls = 0; + server.use( + http.get(`${API}/guilds/9000/members/u1`, () => { + calls++; + return HttpResponse.json({}, { status: 429 }); + }), + ); + const pending = createDiscordClient(cfg) + .getGuildMember("u1") + .catch((e: unknown) => e); + await vi.advanceTimersByTimeAsync(10_000); + const err = await pending; + expect(err).toBeInstanceOf(DiscordApiError); + expect((err as DiscordApiError).transient).toBe(true); + // Pins the bound explicitly: one initial attempt plus MAX_429_RETRIES. + // Without this, an unbounded retry loop is only caught incidentally, by + // `await pending` never resolving inside vitest's wall-clock timeout. + expect(calls).toBe(4); + // Same message shape production log lines and this suite depend on — + // the retry/backoff logic must not change what a caller sees on + // permanent (retry-exhausted) failure. + expect((err as DiscordApiError).message).toBe( + "discord GET /guilds/9000/members/u1 failed (429)", + ); + } finally { + vi.useRealTimers(); + } + }); + + it("honors retry-after on a single 429 and succeeds on the retried request", async () => { + vi.useFakeTimers(); + try { + let calls = 0; + server.use( + http.get(`${API}/guilds/9000/members/u1`, () => { + calls++; + if (calls === 1) { + return new HttpResponse(JSON.stringify({ message: "rate limited" }), { + status: 429, + headers: { "content-type": "application/json", "retry-after": "0.5" }, + }); + } + return HttpResponse.json({ roles: ["10"] }); + }), + ); + const pending = createDiscordClient(cfg).getGuildMember("u1"); + // Advance exactly the header's 0.5s and no further. Advancing a full + // second here (as this test originally did) would also pass for a + // client that ignored `retry-after` entirely and always slept its 1s + // fallback — the two are only distinguishable in between. + await vi.advanceTimersByTimeAsync(500); + expect(calls).toBe(2); + await vi.advanceTimersByTimeAsync(1000); + const member = await pending; + expect(member?.roles).toEqual(["10"]); + expect(calls).toBe(2); + } finally { + vi.useRealTimers(); + } + }); + + it("backs off ALL routes on a global-scoped 429, not just the one that received it", async () => { + vi.useFakeTimers(); + try { + let memberCalls = 0; + let rolesCalls = 0; + const state: { rolesCallTime: number | null } = { rolesCallTime: null }; + server.use( + http.get(`${API}/guilds/9000/members/u1`, () => { + memberCalls++; + if (memberCalls === 1) { + return new HttpResponse(JSON.stringify({ message: "rate limited" }), { + status: 429, + headers: { + "content-type": "application/json", + "retry-after": "0.5", + "x-ratelimit-scope": "global", + }, + }); + } + return HttpResponse.json({ roles: [] }); + }), + http.get(`${API}/guilds/9000/roles`, () => { + rolesCalls++; + state.rolesCallTime = Date.now(); + return HttpResponse.json([ + { id: "10", name: "Member", position: 5, permissions: "0" }, + ]); + }), + ); + const client = createDiscordClient(cfg); + const memberPromise = client.getGuildMember("u1"); + // Let the member call's first request actually land BEFORE the roles + // call is even issued below. This is the fix for the mutation gap: + // firing both concurrently at t=0 let both clear `waitForCapacity` + // (different bucket keys) before either 429 was received, so the old + // version of this test passed even with global backoff disabled. + // `advanceTimersByTimeAsync` flushes the pending microtasks from the + // mocked fetch even though nothing here is timer-based yet, so this + // resolves the member call's first attempt (setting `globalResetAt` + // synchronously) without letting its subsequent `sleep(500)` elapse. + await vi.advanceTimersByTimeAsync(0); + expect(memberCalls).toBe(1); + const expectedResetAt = Date.now() + 500; + const rolesPromise = client.getGuildRoles(); + await vi.advanceTimersByTimeAsync(1000); + await Promise.all([memberPromise, rolesPromise]); + expect(memberCalls).toBe(2); + // Sanity check only, NOT proof of waiting: the roles handler never + // returns 429, so this is 1 whether or not `globalResetAt` gated it. + // The timing assertion below is what carries the proof. + expect(rolesCalls).toBe(1); + // The load-bearing assertion: not just "eventually called", but never + // dispatched before the global window elapsed. Disabling the + // `globalResetAt = ...` assignment makes `getGuildRoles` dispatch + // immediately (at the time just after `advanceTimersByTimeAsync(0)` + // above), which is well before `expectedResetAt` and fails this. + expect(state.rolesCallTime).not.toBeNull(); + expect(state.rolesCallTime as number).toBeGreaterThanOrEqual(expectedResetAt); + } finally { + vi.useRealTimers(); + } + }); + + it("treats an empty retry-after as the 1s default, not a 0s immediate retry", async () => { + vi.useFakeTimers(); + try { + let calls = 0; + server.use( + http.get(`${API}/guilds/9000/members/u1`, () => { + calls++; + if (calls === 1) { + return new HttpResponse(JSON.stringify({ message: "rate limited" }), { + status: 429, + // `Number("")` is 0, not NaN — a `>= 0` guard would accept it and + // sleep 0ms, re-hammering the route Discord just throttled. + headers: { "content-type": "application/json", "retry-after": "" }, + }); + } + return HttpResponse.json({ roles: [] }); + }), + ); + const pending = createDiscordClient(cfg).getGuildMember("u1"); + await vi.advanceTimersByTimeAsync(999); + expect(calls).toBe(1); + await vi.advanceTimersByTimeAsync(1); + expect(calls).toBe(2); + await pending; + } finally { + vi.useRealTimers(); + } + }); + + it("records a global window from the retry-EXHAUSTING 429, not only from retried ones", async () => { + vi.useFakeTimers(); + try { + const state: { rolesCallTime: number | null } = { rolesCallTime: null }; + server.use( + http.get( + `${API}/guilds/9000/members/u1`, + () => + new HttpResponse(JSON.stringify({ message: "rate limited" }), { + status: 429, + headers: { + "content-type": "application/json", + "retry-after": "0.5", + "x-ratelimit-scope": "global", + }, + }), + ), + http.get(`${API}/guilds/9000/roles`, () => { + state.rolesCallTime = Date.now(); + return HttpResponse.json([]); + }), + ); + const client = createDiscordClient(cfg); + const memberPromise = client.getGuildMember("u1").catch((e: unknown) => e); + // Burn all three retries (3 × 500ms) so the FOURTH 429 — the one that + // exhausts the bound and surfaces the error — is the most recent one. + await vi.advanceTimersByTimeAsync(1500); + expect(await memberPromise).toBeInstanceOf(DiscordApiError); + const expectedResetAt = Date.now() + 500; + const rolesPromise = client.getGuildRoles(); + await vi.advanceTimersByTimeAsync(1000); + await rolesPromise; + // Handling `scope` only inside the `attempt < MAX_429_RETRIES` branch + // leaves the final 429 setting no global window at all, and this + // dispatches immediately — which is the case an operator investigating a + // surfaced `failed (429)` is actually living through. + expect(state.rolesCallTime).not.toBeNull(); + expect(state.rolesCallTime as number).toBeGreaterThanOrEqual(expectedResetAt); + } finally { + vi.useRealTimers(); + } + }); + + it("derives the 429 wait from retry-after alone, not retry-after plus x-ratelimit-reset-after", async () => { + vi.useFakeTimers(); + try { + let calls = 0; + server.use( + http.get(`${API}/guilds/9000/members/u1`, () => { + calls++; + if (calls === 1) { + return new HttpResponse(JSON.stringify({ message: "rate limited" }), { + status: 429, + headers: { + "content-type": "application/json", + "retry-after": "0.5", + // Deliberately larger than, and inconsistent with, + // retry-after — pins that the 429 path does not ALSO honor + // this header and wait its difference on top of retry-after. + "x-ratelimit-reset-after": "5", + "x-ratelimit-remaining": "0", + }, + }); + } + return HttpResponse.json({ roles: [] }); + }), + ); + const pending = createDiscordClient(cfg).getGuildMember("u1"); + // Advance by just over the retry-after amount. If the larger + // reset-after header were also honored (the two-source bug), the + // request would still be asleep here and `calls` would still be 1. + await vi.advanceTimersByTimeAsync(600); + expect(calls).toBe(2); + await pending; + } finally { + vi.useRealTimers(); + } + }); + + it("paces a burst of member fetches so it never exceeds the bucket's remaining count", async () => { + vi.useFakeTimers(); + try { + const BUCKET_MAX = 5; + let remaining = BUCKET_MAX; + let windowResetAt = 0; + let exceeded = false; + server.use( + http.get(`${API}/guilds/9000/members/:id`, () => { + if (Date.now() >= windowResetAt) { + remaining = BUCKET_MAX; + windowResetAt = Date.now() + 1000; + } + if (remaining <= 0) { + exceeded = true; + return new HttpResponse(JSON.stringify({ message: "rate limited" }), { + status: 429, + headers: { + "content-type": "application/json", + "retry-after": String(Math.max(0, windowResetAt - Date.now()) / 1000), + }, + }); + } + remaining--; + return HttpResponse.json( + { roles: [] }, + { + headers: { + "x-ratelimit-remaining": String(remaining), + "x-ratelimit-reset-after": String( + Math.max(0, windowResetAt - Date.now()) / 1000, + ), + }, + }, + ); + }), + ); + const client = createDiscordClient(cfg); + const ids = Array.from({ length: 7 }, (_, i) => `u${i}`); + const pending = Promise.all(ids.map((id) => client.getGuildMember(id))); + await vi.advanceTimersByTimeAsync(5000); + await pending; + expect(exceeded).toBe(false); + } finally { + vi.useRealTimers(); + } + }); + + it("caches the preamble (roles, bot id, bot member) instead of re-fetching every run", async () => { + let rolesCalls = 0; + let meCalls = 0; + let memberCalls = 0; server.use( - http.get(`${API}/guilds/9000/members/u1`, () => - HttpResponse.json({}, { status: 429 }), - ), + http.get(`${API}/guilds/9000/roles`, () => { + rolesCalls++; + return HttpResponse.json([ + { id: "10", name: "Member", position: 5, permissions: "0" }, + ]); + }), + http.get(`${API}/users/@me`, () => { + meCalls++; + return HttpResponse.json({ id: "bot-user" }); + }), + http.get(`${API}/guilds/9000/members/bot-user`, () => { + memberCalls++; + return HttpResponse.json({ roles: ["10"] }); + }), ); - const err = await createDiscordClient(cfg) - .getGuildMember("u1") - .catch((e: unknown) => e); - expect(err).toBeInstanceOf(DiscordApiError); - expect((err as DiscordApiError).transient).toBe(true); + const client = createDiscordClient(cfg); + for (let i = 0; i < 3; i++) { + await client.getGuildRoles(); + const botId = await client.getBotUserId(); + await client.getGuildMember(botId); + } + expect(rolesCalls).toBe(1); + expect(meCalls).toBe(1); + expect(memberCalls).toBe(1); + }); + + it("does not cache a REAL member's lookup under the bot-member cache", async () => { + let meCalls = 0; + let memberCalls = 0; + server.use( + http.get(`${API}/users/@me`, () => { + meCalls++; + return HttpResponse.json({ id: "bot-user" }); + }), + http.get(`${API}/guilds/9000/members/real-user`, () => { + memberCalls++; + return HttpResponse.json({ roles: [String(memberCalls)] }); + }), + ); + const client = createDiscordClient(cfg); + await client.getBotUserId(); + const first = await client.getGuildMember("real-user"); + const second = await client.getGuildMember("real-user"); + expect(meCalls).toBe(1); + expect(memberCalls).toBe(2); + expect(first?.roles).toEqual(["1"]); + expect(second?.roles).toEqual(["2"]); }); it("adds and removes member roles via PUT/DELETE", async () => { From 172314d0392ad814a722b60f6f87d50dae6bc81f Mon Sep 17 00:00:00 2001 From: guarzo Date: Mon, 10 Aug 2026 09:11:54 -0400 Subject: [PATCH 2/3] refactor(discord): validate the 404 error code, narrow the request method 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. --- docs/ops.md | 12 ++++++-- src/lib/discord/rest.ts | 61 +++++++++++++++++++++++++++++++------- tests/discord-rest.test.ts | 18 +++++++++++ 3 files changed, 78 insertions(+), 13 deletions(-) diff --git a/docs/ops.md b/docs/ops.md index 9e659470..5a6ffdf2 100644 --- a/docs/ops.md +++ b/docs/ops.md @@ -132,8 +132,16 @@ goes slower than 90 minutes, change it there too. stateless — sessions and OAuth PKCE state are both in Postgres — so extra instances are safe. Machine count is not a `fly.toml` field; set it with `fly scale count`, after the headroom check below. -- **`worker=1`, deliberately.** The Wanderer reconcile is destructive; a second - worker is not a change to make casually. +- **`worker=1`, deliberately.** Two independent reasons now, either sufficient: + the Wanderer reconcile is destructive, and the Discord client's rate-limit + pacing is per-process. `createDiscordClient` holds its bucket state, its + per-route mutex, and its global-backoff deadline in one closure + (`src/lib/discord/rest.ts`), constructed once per worker process. A second + worker gets its own view of every bucket, so the two would burst into the + same per-guild limit without coordinating — reintroducing the 429s that + pacing was added to fix, silently and with no error to point at. There is no + cross-process coordination (Redis, Postgres advisory locks) for this today. + A second worker is not a change to make casually. - **Single-node Postgres, deliberately.** HA adds real operational weight to an unmanaged `postgres-flex` cluster you already patch yourself. diff --git a/src/lib/discord/rest.ts b/src/lib/discord/rest.ts index e0d1b4f4..8fdf665c 100644 --- a/src/lib/discord/rest.ts +++ b/src/lib/discord/rest.ts @@ -48,7 +48,40 @@ const memberSchema = z.object({ .catch(null), }); const userSchema = z.object({ id: z.string() }); -type Member = z.infer; +/** + * Discord's error envelope, narrowed to the one field this client branches on. + * Non-strict, so the `message`/`errors` keys Discord also sends are ignored + * rather than failing the parse. Validated rather than cast: `code` decides + * whether a 404 means "not a member" (recoverable, returns null) or "bad guild + * config" (throws permanently), and a cast would let a string `"10007"` reach + * that comparison as an unequal-but-plausible value. + */ +const errorBodySchema = z.object({ code: z.number().optional() }); + +/** + * The shapes this client hands back, exported so callers can name them without + * re-deriving the inference (or re-declaring a structural copy that would not + * track a schema change here). Both are inferred from the schemas above, so + * they cannot drift from what is actually parsed. + */ +export type DiscordRole = z.infer; +export type Member = z.infer; + +/** + * The methods this client issues. Narrow rather than `string` so `routeKey`'s + * role-mutation branch is checkable: adding a method later is a compile-time + * prompt to decide which bucket key it belongs to, instead of silently + * defaulting to its own. + */ +type HttpMethod = "GET" | "PUT" | "DELETE"; + +/** + * `RequestInit` with `method` narrowed to the methods this client issues, so + * the narrowing at `routeKey` holds all the way from the call site. Dropping + * `.toUpperCase()` on the way through is safe because the type now admits only + * the uppercase literals. + */ +type DiscordRequestInit = Omit & { method?: HttpMethod }; /** Malformed bodies are deterministic — fail closed as permanent, never * retry-loop. Reads the body here so invalid JSON classifies the same way as @@ -196,7 +229,7 @@ export function createDiscordClient(cfg: Config, fetchImpl: typeof fetch = fetch * is what lets the bucket state learned from member #1 correctly pace the * wait before member #6. */ - function routeKey(method: string, path: string): string { + function routeKey(method: HttpMethod, path: string): string { const KNOWN_SEGMENTS = new Set(["guilds", "roles", "members", "users", "@me"]); const templated = path .split("/") @@ -260,8 +293,11 @@ export function createDiscordClient(cfg: Config, fetchImpl: typeof fetch = fetch buckets.set(key, { remaining, resetAt: Date.now() + resetAfter * 1000 }); } - async function rawRequest(path: string, init: RequestInit = {}): Promise { - const method = (init.method ?? "GET").toUpperCase(); + async function rawRequest( + path: string, + init: DiscordRequestInit = {}, + ): Promise { + const method = init.method ?? "GET"; const key = routeKey(method, path); return enqueue(key, async () => { for (let attempt = 0; ; attempt++) { @@ -347,7 +383,7 @@ export function createDiscordClient(cfg: Config, fetchImpl: typeof fetch = fetch return res; } - async function request(path: string, init: RequestInit = {}): Promise { + async function request(path: string, init: DiscordRequestInit = {}): Promise { return assertOk(await rawRequest(path, init), init.method ?? "GET", path); } @@ -358,8 +394,7 @@ export function createDiscordClient(cfg: Config, fetchImpl: typeof fetch = fetch // (for its current role ids). None of these need a fresh fetch every run — // see `PREAMBLE_TTL_MS` above for the tradeoff that caching them accepts. let botUserIdCache: string | null = null; - let guildRolesCache: { roles: z.infer[]; expiresAt: number } | null = - null; + let guildRolesCache: { roles: DiscordRole[]; expiresAt: number } | null = null; let botMemberCache: { member: Member | null; expiresAt: number } | null = null; /** null ONLY for Discord code 10007 (Unknown Member — user not in guild). @@ -369,11 +404,15 @@ export function createDiscordClient(cfg: Config, fetchImpl: typeof fetch = fetch const path = `/guilds/${guild}/members/${userId}`; const res = await rawRequest(path); if (res.status === 404) { - const body = (await res.json().catch(() => undefined)) as - { code?: number } | undefined; - if (body?.code === 10007) return null; + const raw: unknown = await res.json().catch(() => undefined); + const parsed = errorBodySchema.safeParse(raw); + // An unparseable envelope and one carrying no `code` are the same thing + // to this branch: not a confirmed 10007, so it must throw rather than + // silently skip the member. + const code = parsed.success ? parsed.data.code : undefined; + if (code === 10007) return null; throw new DiscordApiError( - `discord GET ${path} failed (404${body?.code !== undefined ? `, code ${body.code}` : ", malformed body"})`, + `discord GET ${path} failed (404${code !== undefined ? `, code ${code}` : ", malformed body"})`, { status: 404, transient: false }, ); } diff --git a/tests/discord-rest.test.ts b/tests/discord-rest.test.ts index 80225717..2c72b621 100644 --- a/tests/discord-rest.test.ts +++ b/tests/discord-rest.test.ts @@ -94,6 +94,24 @@ describe("createDiscordClient", () => { expect((err as DiscordApiError).transient).toBe(false); }); + it("does not read a non-numeric 404 code as 'left the guild'", async () => { + // The 10007 branch is the ONE path that turns a 404 into a recoverable + // null instead of a throw, so what reaches that comparison is validated, + // not cast. A string "10007" is a malformed envelope: it must throw, + // rather than silently deroling a member who is still in the guild. + server.use( + http.get(`${API}/guilds/9000/members/u1`, () => + HttpResponse.json({ message: "Unknown Member", code: "10007" }, { status: 404 }), + ), + ); + const err = await createDiscordClient(cfg) + .getGuildMember("u1") + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(DiscordApiError); + expect((err as DiscordApiError).transient).toBe(false); + expect((err as DiscordApiError).message).toContain("malformed body"); + }); + it("carries the member's names through, and survives a payload without them", async () => { server.use( http.get(`${API}/guilds/9000/members/u1`, () => From a8fa9df8497ec604d976450d51c368c99110fbce Mon Sep 17 00:00:00 2001 From: guarzo Date: Mon, 10 Aug 2026 09:35:32 -0400 Subject: [PATCH 3/3] fix(discord): release the exhausting 429's body, stop handing out the 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. --- src/lib/discord/rest.ts | 26 ++++++++++++++++--------- tests/discord-rest.test.ts | 39 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/lib/discord/rest.ts b/src/lib/discord/rest.ts index 8fdf665c..c09ec6bd 100644 --- a/src/lib/discord/rest.ts +++ b/src/lib/discord/rest.ts @@ -356,14 +356,18 @@ export function createDiscordClient(cfg: Config, fetchImpl: typeof fetch = fetch `scope=${scope ?? "route"} attempt=${attempt + 1}/${MAX_429_RETRIES + 1}` + (willRetry ? "" : " (retries exhausted)"), ); + // Nothing ever reads a 429 body — `assertOk` inspects only + // `res.status`, and the one body read below is on 404 — while undici + // holds the socket until the body is consumed or cancelled. Released + // on BOTH arms: the exhausting attempt returns a response whose body + // is just as dead as a retried one's, and leaking that socket is + // worst precisely when the route is already rate limited. Released + // deliberately WITHOUT awaiting: under the mocked fetch this suite + // uses, `cancel()` never settles, and awaiting it hung every 429 + // retry test. Neither the retry nor the return may be gated on a + // best-effort cleanup. + void res.body?.cancel().catch(() => undefined); if (willRetry) { - // Nothing ever reads a retried response's body, and undici holds - // the socket until the body is consumed or cancelled. Released - // deliberately WITHOUT awaiting: under the mocked fetch this - // suite uses, `cancel()` never settles, and awaiting it hung - // every 429 retry test. The retry must not be gated on a - // best-effort cleanup either way. - void res.body?.cancel().catch(() => undefined); await sleep(retryAfterSec * 1000); continue; } @@ -422,14 +426,18 @@ export function createDiscordClient(cfg: Config, fetchImpl: typeof fetch = fetch return { async getGuildRoles() { + // Copied on the way out, both arms. The cache outlives the call and is + // shared by every sweep in this process, so handing out the stored array + // would let one caller's in-place `sort`/`splice` silently rewrite what + // the next five minutes of runs diff against. if (guildRolesCache && Date.now() < guildRolesCache.expiresAt) { - return guildRolesCache.roles; + return [...guildRolesCache.roles]; } const path = `/guilds/${guild}/roles`; const res = await request(path); const roles = await parseBody(z.array(roleSchema), res, "GET", path); guildRolesCache = { roles, expiresAt: Date.now() + PREAMBLE_TTL_MS }; - return roles; + return [...roles]; }, // The bot's own id is fixed for the lifetime of the token — cached // indefinitely, no TTL needed (there is no "operator fixed it" case to diff --git a/tests/discord-rest.test.ts b/tests/discord-rest.test.ts index 2c72b621..f07112b1 100644 --- a/tests/discord-rest.test.ts +++ b/tests/discord-rest.test.ts @@ -461,6 +461,45 @@ describe("createDiscordClient", () => { } }); + it("ignores a non-finite reset-after rather than pacing on an infinite window", async () => { + vi.useFakeTimers(); + try { + let calls = 0; + server.use( + http.get(`${API}/guilds/9000/members/:id`, () => { + calls++; + // remaining=0 is the arm that makes `waitForCapacity` sleep at all; + // reset-after decides for how long. `Number("Infinity")` passes a + // `!Number.isNaN` check, so only the isFinite guard keeps this pair + // out of the bucket. + return HttpResponse.json( + { roles: [] }, + { + headers: { + "x-ratelimit-remaining": "0", + "x-ratelimit-reset-after": "Infinity", + }, + }, + ); + }), + ); + const client = createDiscordClient(cfg); + await client.getGuildMember("u1"); + const pending = client.getGuildMember("u2"); + // Flushes microtasks WITHOUT moving the clock. If the infinite window + // had been recorded, the second fetch would be parked on a sleep that no + // amount of advancement could ever fire, and `calls` would still be 1. + await vi.advanceTimersByTimeAsync(0); + expect(calls).toBe(2); + // Resolving at all is the "no 429" assertion: the handler never rate + // limits, so a 429 here could only come from the client's own pacing + // having failed to keep the two calls apart. + await expect(pending).resolves.toEqual({ roles: [] }); + } finally { + vi.useRealTimers(); + } + }); + it("caches the preamble (roles, bot id, bot member) instead of re-fetching every run", async () => { let rolesCalls = 0; let meCalls = 0;