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 6a1c2d51..c09ec6bd 100644 --- a/src/lib/discord/rest.ts +++ b/src/lib/discord/rest.ts @@ -48,6 +48,40 @@ const memberSchema = z.object({ .catch(null), }); const userSchema = z.object({ id: z.string() }); +/** + * 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 @@ -67,24 +101,280 @@ 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: HttpMethod, 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: DiscordRequestInit = {}, + ): Promise { + const method = init.method ?? "GET"; + 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)"), + ); + // 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) { + await sleep(retryAfterSec * 1000); + continue; + } + } + return res; + } + }); } function assertOk(res: Response, method: string, path: string): Response { @@ -97,40 +387,82 @@ 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); } 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: 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). + * 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 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${code !== undefined ? `, code ${code}` : ", malformed body"})`, + { status: 404, transient: false }, + ); + } + assertOk(res, "GET", path); + return parseBody(memberSchema, res, "GET", path); + } + 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]; + } 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..f07112b1 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"; @@ -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`, () => @@ -157,17 +175,383 @@ 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("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; + 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 () => {