From 74f93eda6d20b9907073563d11e79170d834f9af Mon Sep 17 00:00:00 2001 From: NotDemonix <90858555+NotDemonix@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:14:22 +0200 Subject: [PATCH] feat(rest): add signal to makeRequest and stop re-sending proxied requests by default (#5158) * fix(rest): don't resend timed out requests when proxied When a request to a rest proxy hits requestTimeout, only our side of the connection gets aborted - the proxy keeps processing the request (often it's just queued behind a rate limit) and it can still reach Discord. Resending it executes non-idempotent requests twice. We hit this in production: a single ticket action created several empty channels because the channel create kept being resent while the original was waiting out a rate limit on the proxy. Timeouts now fail immediately in proxy mode. Only attempts that never reached the proxy at all (connection refused/reset) are retried, with a small backoff so a proxy that's restarting isn't hammered in a tight loop. Direct-to-Discord behavior is unchanged. Follow-up to #5085. * feat(rest): support aborting requests that are still queued * fix(rest): let fetch handle the abort signal Combine the caller signal with the timeout via AbortSignal.any like review suggested, so aborting also cancels an attempt already in flight. Same as nirn - the discord request inherits the incoming request context, so a disconnect cancels it mid flight too. Dropped the dispatched flag, not needed anymore. Also wired the signal into the bigbot rest example off the close event. * fix(rest): drop redundant abort check, fetch already does it * tests: Fix broken tests due to sinon fake timers and Node.js v22 microtask behavior * feat(rest): let users opt into retrying timed out proxied requests * fix(rest): cap proxy connection retries and reuse the error format Retrying a request that never reached the proxy was bound by maxRetryCount, which is Infinity by default, so a proxy that stays down kept every request alive forever. Cap it at 3 attempts instead. The proxied path also threw the raw fetch error, so proxy users got a different error shape than everyone else. Both paths now build the error through the same helper. * fix(rest): make the proxy connection retry count configurable Re-sending a request that never reached the proxy is now bound by rest.maxProxyConnectionRetryCount (3 by default, and by maxRetryCount when that one is lower) instead of a hardcoded limit. Also moves the error builder and the error prototype out of createRestManager since neither depends on anything in it. * fix(rest): don't re-send proxied requests that already reached the proxy A timed out attempt only aborts our side of the connection, the proxy keeps processing the request and it can still reach Discord, so re-sending it executes non-idempotent requests twice. proxy.retryOnTimeout opts back in for setups that deduplicate requests. Connection failures reject right away, same as a failed fetch does when talking to Discord directly. Moves the error building into rest.createRequestError so a proxied request fails with the same message and cause as a direct one. * fix(rest): re-send proxied requests that never reached the proxy A failed fetch is only safe to send again when it failed while connecting, the proxy restarting being the usual case. A socket that dies once the request is on the wire may have been forwarded to Discord already, so those keep rejecting right away. Bound by maxProxyConnectionRetryCount, 3 by default, since maxRetryCount is Infinity and a proxy that stays down would keep every request alive. * fix(rest): recognize connect failures on Deno too Deno puts no code on the errors fetch throws, the only place the phase the request failed in shows up is the message, so match on that as well. * fix(rest): go off the phase a request failed in, not the error code Node.js names the syscall that failed, so matching on that covers every reason a connection could not be established (refused, host or network unreachable, dns) instead of the few codes that were listed, and it is read or write once the request is on the wire. Bun reports a code of its own and Deno only says it in the message, so those two keep their own check. * fix(rest): never re-send once an error says the request went out A connection that dies while it is carrying the request is reported in terms of that request, and a runtime can wrap that in the wording for a failure to connect, which used to read as never having sent anything. Looking for it first, and over the whole chain, settles those. Also drops the codes and wordings that were guesses. What is left is what node, bun and deno were seen to report, with the node ones kept as a fallback for a runtime that borrows the code without the syscall. * fix(rest): release cancelled requests and gate proxy re-sends behind one option Cancelling a request only settled the caller. The entry stayed in the queue, spent a rate limit slot and reached fetch with an aborted signal. The waiter is spliced out of `waiting` now, `Queue.makeRequest` stops before pushing to `pending` when the signal fired while it waited for a slot, and `processPending` drops an entry whose caller gave up before anything is spent on it. `processWaiting` cleans up when its loop ends, otherwise a queue whose waiters were all cancelled never schedules its own deletion. In proxy mode an abort landing while the response body was being read was swallowed by the `null` fallback and came back as a successful empty response. A read the abort killed rejects now, one that finished is still returned, the same way the queued path keeps a result that arrived before the signal fired. `proxy.retryOnTimeout` becomes `proxy.retryRequests` and covers every attempt that failed without producing a response. Telling a failure to connect apart from a connection that died carrying the request meant reading syscalls, error codes and error text from three runtimes, and it could never be more than a guess: fetch does not say which phase a failure happened in, and a wrong guess re-sends a request the proxy already has. So that block is gone and the caller decides, the same way it already did for timeouts. One `retryCount` bounded by `maxRetryCount` and `maxProxyRetryCount`, so alternating failures can no longer hand out more re-sends than the cap allows. Co-Authored-By: Claude Opus 5 * fix(rest): give a proxy that is restarting time to come back Three re-sends 250ms apart is 750ms of patience, which is sized for a blip rather than for the case it was added for. A container being restarted or redeployed is not back in that time, so the budget ran out before the proxy was reachable again. Each further attempt now waits 250ms longer than the one before it, and the default count is 15, so the ladder spans about 30 seconds. Stepping up rather than doubling keeps the gaps small enough to notice the proxy coming back instead of sleeping well past it. Co-Authored-By: Claude Opus 5 * refactor(rest): move the proxy re-send delay onto the manager It was a module level constant sitting next to maxProxyRetryCount's own limit, so it is a manager field now like the counts it goes with, and the tests set it to 0 instead of really waiting out the backoff. * Update packages/rest/tests/unit/manager.spec.ts Co-authored-by: Fleny * Run biome --------- Co-authored-by: Fleny Co-authored-by: Awesome Stickz <38146668+AwesomeStickz@users.noreply.github.com> Co-authored-by: Claude Opus 5 --- examples/bigbot/src/rest/index.ts | 9 + packages/rest/src/manager.ts | 211 +++++++++++----- packages/rest/src/queue.ts | 40 ++- packages/rest/src/types.ts | 70 +++++- packages/rest/tests/unit/manager.spec.ts | 299 ++++++++++++++++++++++- 5 files changed, 563 insertions(+), 66 deletions(-) diff --git a/examples/bigbot/src/rest/index.ts b/examples/bigbot/src/rest/index.ts index a58a74d4d..47edcbfc9 100644 --- a/examples/bigbot/src/rest/index.ts +++ b/examples/bigbot/src/rest/index.ts @@ -20,9 +20,18 @@ app.all('/*', async (req, res) => { const hasBody = req.method !== 'GET' && req.method !== 'DELETE'; const body = hasBody ? (isMultipart ? await parseMultiformBody(req.body) : req.body) : undefined; + // Tie the request to the connection: when the client aborts, the request is dropped from the queue instead + // of being executed with nobody waiting for the result anymore. + // https://fastify.dev/docs/latest/Guides/Detecting-When-Clients-Abort/ + const controller = new AbortController(); + req.raw.on('close', () => { + if (req.raw.aborted) controller.abort(); + }); + try { const result = await restManager.makeRequest(req.method as RequestMethods, url, { body, + signal: controller.signal, }); if (result) { diff --git a/packages/rest/src/manager.ts b/packages/rest/src/manager.ts index 0b0c3fa43..259a410c0 100644 --- a/packages/rest/src/manager.ts +++ b/packages/rest/src/manager.ts @@ -117,7 +117,10 @@ export function createRestManager(options: CreateRestManagerOptions): RestManage invalidBucket: createInvalidRequestBucket({ logger: options.logger }), isProxied: !baseUrl.startsWith(DISCORD_API_URL), updateBearerTokenEndpoint: options.proxy?.updateBearerTokenEndpoint, + retryProxiedRequests: options.proxy?.retryRequests ?? false, maxRetryCount: Infinity, + maxProxyRetryCount: 15, + proxyRetryDelayStep: 250, requestTimeout: options.requestTimeout ?? 30000, processingRateLimitedPaths: false, queues: new Map(), @@ -320,6 +323,48 @@ export function createRestManager(options: CreateRestManagerOptions): RestManage }; }, + createRequestError(error, reason) { + let errorText: string; + + switch (reason.status) { + case 400: + errorText = "The options was improperly formatted, or the server couldn't understand it."; + break; + case 401: + errorText = 'The Authorization header was missing or invalid.'; + break; + case 403: + errorText = 'The Authorization token you passed did not have permission to the resource.'; + break; + case 404: + errorText = "The resource at the location specified doesn't exist."; + break; + case 405: + errorText = 'The HTTP method used is not valid for the location specified.'; + break; + case 429: + errorText = "You're being ratelimited."; + break; + case 502: + errorText = 'There was not a gateway available to process your options. Wait a bit and retry.'; + break; + default: + errorText = reason.statusText ?? reason.error ?? 'Unknown error'; + } + + error.message = `[${reason.status}] ${errorText}`; + + // If discord sent us JSON, it is probably going to be an error message from which we can get and add some information about the error to the error message, the full body will be in the error.cause + // https://docs.discord.com/developers/reference#error-messages + if (typeof reason.body === 'object' && hasProperty(reason.body, 'code') && hasProperty(reason.body, 'message')) { + error.message += `\nDiscord error: [${reason.body.code}] ${reason.body.message}`; + } + + error.cause = Object.assign(Object.create(baseErrorPrototype), reason); + + return error; + }, + processRateLimitedPaths() { const now = Date.now(); @@ -462,7 +507,13 @@ export function createRestManager(options: CreateRestManagerOptions): RestManage // Give this attempt a hard deadline. `AbortSignal.timeout` aborts the fetch (and any in-progress body // read) once it fires, which makes the awaited fetch reject so a stalled connection can never keep this // queue's `processPending` loop (and therefore the whole queue) wedged forever. Omitted when disabled. - const request = new Request(url, { ...payload, signal: rest.requestTimeout > 0 ? AbortSignal.timeout(rest.requestTimeout) : undefined }); + // The caller's signal is handed to fetch too: fetch refuses to send a request whose signal is already aborted, so a queue entry that got + // aborted while waiting never goes out, and aborting mid-request tears down the connection. + const signals: AbortSignal[] = []; + if (options.requestBodyOptions?.signal) signals.push(options.requestBodyOptions.signal); + if (rest.requestTimeout > 0) signals.push(AbortSignal.timeout(rest.requestTimeout)); + + const request = new Request(url, { ...payload, signal: signals.length > 0 ? AbortSignal.any(signals) : undefined }); rest.events.request(request, { body: options.requestBodyOptions?.body, }); @@ -473,6 +524,17 @@ export function createRestManager(options: CreateRestManagerOptions): RestManage // Mark request as completed rest.invalidBucket.handleCompletedRequest(999, false); + // The caller aborted the request, never re-send it, Discord may have received it already. The caller was rejected the moment the signal fired. + if (options.requestBodyOptions?.signal?.aborted) { + options.reject({ + ok: false, + status: 999, + error: 'The request was aborted.', + errorObject: error, + }); + return; + } + // The attempt hit `rest.requestTimeout` and was aborted. Treat it like a transient failure and retry // it through the queue, so a single stalled connection doesn't permanently fail the request. if (isTimeoutError(error)) return await handleTimeout(options, error); @@ -644,49 +706,96 @@ export function createRestManager(options: CreateRestManagerOptions): RestManage const url = `${rest.baseUrl}/v${rest.version}${route}`; - // Retry on timeout up to `maxRetryCount`, mirroring the queued `sendRequest` path, so a stalled proxy - // connection doesn't permanently fail the request (and can never hang the caller forever). - for (let retryCount = 0; ; retryCount++) { - // Give the request to the proxy a hard deadline too via `AbortSignal.timeout` (omitted when disabled). + // An attempt that failed without a response is not re-sent by default. Whatever it failed on, the proxy may already have the request + // (a timeout only aborts our side of the connection, and a socket that dies once the request is on the wire may still have delivered + // it), so re-sending it would execute non-idempotent requests twice. `fetch` gives no way to tell the two apart, so the caller is the + // one who decides: `proxy.retryRequests` says re-sending against this proxy is safe. Retrying towards Discord is the proxy's job, it + // is the one talking to Discord. + let retryCount = 0; + + for (;;) { + // The request may have been aborted while the previous attempt was timing out, never send it in that case. + if (options?.signal?.aborted) { + throw rest.createRequestError(error, { ok: false, status: 999, error: 'The request was aborted.', errorObject: options.signal.reason }); + } + + // Give the attempt a deadline (omitted when disabled) so a stalled connection can't hang the caller forever, and hand the caller's + // signal to fetch so aborting tears down a request that is already on the wire. + const signals: AbortSignal[] = []; + if (options?.signal) signals.push(options.signal); + if (rest.requestTimeout > 0) signals.push(AbortSignal.timeout(rest.requestTimeout)); + const request = new Request(url, { ...rest.createRequestBody(method, options), - signal: rest.requestTimeout > 0 ? AbortSignal.timeout(rest.requestTimeout) : undefined, + signal: signals.length > 0 ? AbortSignal.any(signals) : undefined, }); rest.events.request(request, { body: options?.body, }); - const result = await fetch(request).catch((fetchError) => { + const result = await fetch(request).catch(async (fetchError) => { rest.events.requestError(request, fetchError, { body: options?.body }); - // The attempt hit `rest.requestTimeout` and was aborted; retry it until the budget is exhausted. - if (isTimeoutError(fetchError) && retryCount < rest.maxRetryCount) { - rest.logger.debug(`request to proxy ${url} timed out after ${rest.requestTimeout}ms, retrying.`, fetchError); + // The caller aborted the request, never re-send it, the proxy may have received it already. + if (options?.signal?.aborted) { + throw rest.createRequestError(error, { ok: false, status: 999, error: 'The request was aborted.', errorObject: fetchError }); + } + + const timedOut = isTimeoutError(fetchError); + + if (rest.retryProxiedRequests && retryCount < rest.maxRetryCount && retryCount < rest.maxProxyRetryCount) { + retryCount++; + rest.logger.debug(`request to proxy ${url} failed without a response, retrying.`, fetchError); + // Wait a little longer before each further attempt so a proxy that is down isn't hammered in a tight loop, while staying + // frequent enough to notice it coming back. A timed out attempt already sat out `requestTimeout`, so it goes again right away. + if (!timedOut) await delay(rest.proxyRetryDelayStep * retryCount); return undefined; } - throw fetchError; + if (timedOut) { + rest.logger.debug(`request to proxy ${url} timed out after ${rest.requestTimeout}ms.`); + throw rest.createRequestError(error, { + ok: false, + status: 999, + error: 'The request timed out and it maxed out the retries limit.', + errorObject: fetchError, + }); + } + + rest.logger.debug(`request fetch to proxy ${url} failed.`, fetchError); + throw rest.createRequestError(error, { + ok: false, + status: 999, + error: + 'Possible network or request shape issue occurred. If this is rare, its a network glitch. If it occurs a lot something is wrong.', + errorObject: fetchError, + }); }); - // If result is undefined, the attempt timed out and is being retried. + // If result is undefined, the attempt failed in a way that is safe to re-send and is being retried. if (!result) continue; // Sometimes the Content-Type may be "application/json; charset=utf-8", for this reason, we need to check the start of the header - const body = await (result.headers.get('Content-Type')?.startsWith('application/json') ? result.json() : result.text()).catch(() => null); + let bodyReadFailed = false; + const body = await (result.headers.get('Content-Type')?.startsWith('application/json') ? result.json() : result.text()).catch(() => { + bodyReadFailed = true; + return null; + }); rest.events.response(request, result, { requestBody: options?.body, responseBody: body, }); - if (!result.ok) { - error.cause = Object.assign(Object.create(baseErrorPrototype), { - ok: false, - status: result.status, - body, - }); + // `fetch` resolves once the headers are in, so an abort can land while the body above is still being read. That read rejects, the + // fallback turns it into a `null` body and the caller gets it back as a successful empty response. A read that did finish is kept, + // the same way the queued path hands back a result that arrived before the signal fired. + if (bodyReadFailed && options?.signal?.aborted) { + throw rest.createRequestError(error, { ok: false, status: 999, error: 'The request was aborted.', errorObject: options.signal.reason }); + } - throw error; + if (!result.ok) { + throw rest.createRequestError(error, { ok: false, status: result.status, statusText: result.statusText, body }); } return result.status !== 204 ? (typeof body === 'string' ? JSON.parse(body) : body) : undefined; @@ -694,6 +803,18 @@ export function createRestManager(options: CreateRestManagerOptions): RestManage } return await new Promise(async (resolve, reject) => { + const signal = options?.signal; + const abortListener = () => { + // Reject right away instead of waiting for the queue to reach the request. The signal is attached to the fetch itself as well, so an + // attempt that is already in flight gets its connection torn down too. + payload.reject({ + ok: false, + status: 999, + error: 'The request was aborted.', + errorObject: signal?.reason, + }); + }; + const payload: SendRequestOptions = { route, method, @@ -703,51 +824,25 @@ export function createRestManager(options: CreateRestManagerOptions): RestManage await rest.processRequest(payload); }, resolve: (data) => { + signal?.removeEventListener('abort', abortListener); resolve(data.status !== 204 ? (data.body as Parameters[0]) : undefined!); }, reject: (reason) => { - let errorText: string; - - switch (reason.status) { - case 400: - errorText = "The options was improperly formatted, or the server couldn't understand it."; - break; - case 401: - errorText = 'The Authorization header was missing or invalid.'; - break; - case 403: - errorText = 'The Authorization token you passed did not have permission to the resource.'; - break; - case 404: - errorText = "The resource at the location specified doesn't exist."; - break; - case 405: - errorText = 'The HTTP method used is not valid for the location specified.'; - break; - case 429: - errorText = "You're being ratelimited."; - break; - case 502: - errorText = 'There was not a gateway available to process your options. Wait a bit and retry.'; - break; - default: - errorText = reason.statusText ?? 'Unknown error'; - } - - error.message = `[${reason.status}] ${errorText}`; - - // If discord sent us JSON, it is probably going to be an error message from which we can get and add some information about the error to the error message, the full body will be in the error.cause - // https://docs.discord.com/developers/reference#error-messages - if (typeof reason.body === 'object' && hasProperty(reason.body, 'code') && hasProperty(reason.body, 'message')) { - error.message += `\nDiscord error: [${reason.body.code}] ${reason.body.message}`; - } - - error.cause = Object.assign(Object.create(baseErrorPrototype), reason); - reject(error); + signal?.removeEventListener('abort', abortListener); + reject(rest.createRequestError(error, reason)); }, runThroughQueue: options?.runThroughQueue, }; + // Reject a queued request as soon as its signal aborts instead of when the queue reaches it, so a caller enforcing a deadline gets its + // answer right away. Fetch refusing to send an already aborted request is what guarantees the stale queue entry never goes out. + if (signal?.aborted) { + abortListener(); + return; + } + + signal?.addEventListener('abort', abortListener, { once: true }); + await rest.processRequest(payload); }); }, diff --git a/packages/rest/src/queue.ts b/packages/rest/src/queue.ts index ff06c1bce..97650dc9c 100644 --- a/packages/rest/src/queue.ts +++ b/packages/rest/src/queue.ts @@ -56,15 +56,27 @@ export class Queue { return this.remaining > 0; } - /** Pauses the execution until a request is allowed to be made. */ - async waitUntilRequestAvailable(): Promise { + /** Pauses the execution until a request is allowed to be made, or until the given signal aborts. */ + async waitUntilRequestAvailable(signal?: AbortSignal): Promise { return await new Promise(async (resolve) => { // If whatever amount of requests is left is more than the safety margin, allow the request if (this.isRequestAllowed()) { // this.remaining++; resolve(); } else { - this.waiting.push(resolve); + const stopWaiting = () => { + signal?.removeEventListener('abort', onAbort); + resolve(); + }; + // A caller that gives up is taken back out of the queue right away, so its request and body aren't kept alive until the queue resumes. + const onAbort = () => { + const index = this.waiting.indexOf(stopWaiting); + if (index !== -1) this.waiting.splice(index, 1); + stopWaiting(); + }; + + this.waiting.push(stopWaiting); + signal?.addEventListener('abort', onAbort, { once: true }); await this.processWaiting(); } }); @@ -89,6 +101,9 @@ export class Queue { // Mark as false so next pending request can be triggered by new loop. this.processing = false; + // The waiting list can empty out without anything moving on to the pending one, when every request in it was cancelled, and nothing would + // look at this queue again. + this.cleanup(); } /** Process the queue of all requests pending to be sent. */ @@ -101,6 +116,14 @@ export class Queue { while (this.pending.length > 0) { this.rest.logger.debug(`Queue ${this.queueType} ${this.url} process pending while loop ran with ${this.pending.length}.`); + + // Drop the requests whose caller gave up, they were rejected the moment their signal fired. This happens before anything is spent on + // them: sending them would use up a rate limit slot and delay the requests that are still wanted, and fetch refuses them anyway. + if (this.pending[0]?.requestBodyOptions?.signal?.aborted) { + this.pending.shift(); + continue; + } + if (!this.firstRequest && !this.isRequestAllowed()) { const now = Date.now(); const future = this.frozenAt + this.interval; @@ -169,7 +192,16 @@ export class Queue { /** Checks if a request is available and adds it to the queue. Also triggers queue processing if not already processing. */ async makeRequest(options: SendRequestOptions): Promise { - await this.waitUntilRequestAvailable(); + const signal = options.requestBodyOptions?.signal; + + await this.waitUntilRequestAvailable(signal); + + // The caller gave up while this was waiting for a slot, it has already been rejected and there is nothing left to send. + if (signal?.aborted) { + this.cleanup(); + return; + } + this.pending.push(options); this.processPending(); } diff --git a/packages/rest/src/types.ts b/packages/rest/src/types.ts index b2b0eeae2..c633a9d17 100644 --- a/packages/rest/src/types.ts +++ b/packages/rest/src/types.ts @@ -202,6 +202,25 @@ export interface CreateRestManagerOptions { * This value is actually required if you want to use `updateTokenQueues` */ updateBearerTokenEndpoint?: string; + /** + * Whether an attempt that failed without producing a response should be re-sent to the proxy. + * + * @remarks + * This covers both an attempt that hit {@link CreateRestManagerOptions.requestTimeout | requestTimeout} and one whose + * connection failed. Neither tells us whether the proxy got the request: the timeout only aborts our side of the + * connection while the proxy keeps processing (it may simply be queued behind a rate limit), and a socket that dies + * once the request is on the wire may still have delivered it. `fetch` gives no way to tell those apart from never + * having connected at all, so re-sending can execute non-idempotent requests twice (e.g. duplicate channel creates). + * + * Only enable this when re-sending a request to your proxy is safe, for example by attaching an idempotency + * key to each request that the proxy tracks in a store of your choosing (in memory, redis, ...) so a + * re-sent request is recognized and executed only once. Discordeno does not provide such a mechanism. + * + * Bounded by {@link RestManager.maxRetryCount} and {@link RestManager.maxProxyRetryCount}. + * + * @default false + */ + retryRequests?: boolean; }; /** * The api versions which can be used to make requests. @@ -220,8 +239,13 @@ export interface CreateRestManagerOptions { * * @remarks * This is a total deadline for each attempt (it also covers reading the response body), not a per-chunk timeout. - * When an attempt times out it is retried through the queue up to {@link RestManager.maxRetryCount} times before failing. - * Without it, a connection that stalls after connecting could keep a queue from ever progressing. + * When talking to Discord directly, a timed-out attempt is retried through the queue up to + * {@link RestManager.maxRetryCount} times before failing. Without it, a connection that stalls after connecting + * could keep a queue from ever progressing. + * + * When a `proxy` is configured, a timed-out attempt is NOT retried: the proxy keeps processing the request after + * the timeout aborts our side of the connection (it may simply be queued behind a rate limit), so re-sending it + * could execute it twice. If re-sending against your proxy is safe, `proxy.retryRequests` opts back into it. * * Because it is a total deadline rather than a per-chunk one, a slow but healthy request (e.g. uploading a * large attachment over a slow connection) can legitimately exceed it and be aborted/retried. Raise this value @@ -259,9 +283,36 @@ export interface RestManager { authorizationHeader: string; /** The endpoint to use for `updateTokenQueues` when working with a rest proxy */ updateBearerTokenEndpoint?: string; + /** Whether a proxied attempt that failed without producing a response is re-sent to the proxy. Only safe when re-sending against the proxy is. Defaults to false. */ + retryProxiedRequests: boolean; /** The maximum amount of times a request should be retried. Defaults to Infinity */ maxRetryCount: number; - /** The maximum time in milliseconds a single request attempt may take before it is aborted and retried. Defaults to 30000 (30 seconds). Set to 0 to disable. */ + /** + * The maximum amount of times a proxied request should be re-sent. Defaults to 15. + * + * @remarks + * This has its own limit because {@link RestManager.maxRetryCount} is Infinity by default, which would keep every request alive for as + * long as the proxy stays down. + * + * Each further attempt waits {@link RestManager.proxyRetryDelayStep} longer than the one before it, so the defaults span about 30 seconds + * in total, enough to sit out a proxy being restarted or redeployed. + * + * Whichever of the two is lower applies. Only used when {@link RestManager.retryProxiedRequests} is enabled. + */ + maxProxyRetryCount: number; + /** + * How much longer to wait before each further re-send to a proxy, in milliseconds. Defaults to 250. + * + * @remarks + * The nth attempt waits n times this (250ms, then 500ms, then 750ms, ...), so a proxy that is down isn't hammered in a tight loop while + * still being checked often enough to notice it coming back. + * + * An attempt that timed out already sat out {@link RestManager.requestTimeout}, so it is re-sent right away without this delay. + * + * Only used when {@link RestManager.retryProxiedRequests} is enabled. + */ + proxyRetryDelayStep: number; + /** The maximum time in milliseconds a single request attempt may take before it is aborted. Timed-out attempts are only retried when talking to Discord directly, or through a proxy when `retryProxiedRequests` is enabled. Defaults to 30000 (30 seconds). Set to 0 to disable. */ requestTimeout: number; /** Whether or not the manager is rate limited globally across all requests. Defaults to false. */ globallyRateLimited: boolean; @@ -293,6 +344,8 @@ export interface RestManager { changeToDiscordFormat: (obj: any) => any; /** Creates the request body and headers that are necessary to send a request. Will handle different types of methods and everything necessary for discord. */ createRequestBody: (method: RequestMethods, options?: CreateRequestBodyOptions) => RequestBody; + /** Fills in the message and the cause of the error that is given to the user when a request fails. The error itself is created by the caller because of how stack traces get calculated. */ + createRequestError: (error: Error, reason: RestRequestRejection) => Error; /** This will create a infinite loop running in 1 seconds using tail recursion to keep rate limits clean. When a rate limit resets, this will remove it so the queue can proceed. */ processRateLimitedPaths: () => void; /** Processes the rate limit headers and determines if it needs to be rate limited and returns the bucket id if available */ @@ -3430,6 +3483,17 @@ export interface CreateRequestBodyOptions { unauthorized?: boolean; reason?: string; files?: FileContent[]; + /** + * An `AbortSignal` to cancel the request. + * + * @remarks + * Aborting rejects a request that is still waiting in the queue right away and guarantees it is never sent. + * An attempt that is already in flight gets its connection cancelled as well, though that cannot recall it: + * Discord (or the proxy) may have received it and may still process it, only the response is discarded. + * This lets a rest proxy drop requests that nobody is waiting on anymore (client disconnect, deadline) + * instead of leaving them in the queue. + */ + signal?: AbortSignal; } export type MakeRequestOptions = Omit & Pick; diff --git a/packages/rest/tests/unit/manager.spec.ts b/packages/rest/tests/unit/manager.spec.ts index 10c2eb0c4..743fe17cc 100644 --- a/packages/rest/tests/unit/manager.spec.ts +++ b/packages/rest/tests/unit/manager.spec.ts @@ -1,10 +1,13 @@ -import { expect } from 'chai'; +import { use as chaiUse, expect } from 'chai'; +import chaiAsPromised from 'chai-as-promised'; import { afterEach, beforeEach, describe, it } from 'mocha'; import sinon from 'sinon'; import { createRestManager } from '../../src/manager.js'; import type { RestManager } from '../../src/types.js'; import { fakeToken as token } from '../constants.js'; +chaiUse(chaiAsPromised); + describe('[rest] manager', () => { describe('create a rest manager with only a token', () => { const rest = createRestManager({ token }); @@ -229,4 +232,298 @@ describe('[rest] manager', () => { }); }); }); + + describe('rest.makeRequest with a proxy', () => { + let rest: RestManager; + let fetchStub: sinon.SinonStub; + + beforeEach(() => { + rest = createRestManager({ + token, + proxy: { + baseUrl: 'https://localhost:8000', + authorization: token, + }, + }); + fetchStub = sinon.stub(globalThis, 'fetch'); + }); + + afterEach(() => { + fetchStub.restore(); + }); + + // Re-sending is checked by counting the calls to fetch, never by timing, so the backoff between them is turned off to keep the suite fast + // and free of a wait that could get flaky under load. + const createRetryingRest = (): RestManager => { + const retryingRest = createRestManager({ + token, + proxy: { baseUrl: 'https://localhost:8000', authorization: token, retryRequests: true }, + }); + retryingRest.proxyRetryDelayStep = 0; + + return retryingRest; + }; + + it('Will not re-send the request when the attempt times out', async () => { + const timeoutError = new DOMException('The operation timed out.', 'TimeoutError'); + fetchStub.rejects(timeoutError); + + const error = await expect(rest.makeRequest('GET', '/gateway/bot')).to.eventually.be.rejectedWith(Error); + expect(fetchStub.callCount).to.be.equal(1); + // The failure is reported with the same error as a request that is not proxied + expect(error.message).to.be.equal('[999] The request timed out and it maxed out the retries limit.'); + expect(error.cause).to.deep.include({ ok: false, status: 999, errorObject: timeoutError }); + }); + + it('Will not re-send the request when the connection fails', async () => { + const fetchError = new TypeError('fetch failed', { cause: new Error('connect ECONNREFUSED 127.0.0.1:8000') }); + fetchStub.rejects(fetchError); + + const error = await expect(rest.makeRequest('GET', '/gateway/bot')).to.eventually.be.rejectedWith(Error); + expect(fetchStub.callCount).to.be.equal(1); + expect(error.cause).to.deep.include({ ok: false, status: 999, errorObject: fetchError }); + }); + + // Both of these failed without giving us a response, so neither says whether the proxy got the request. They are re-sent together or not + // at all, which is what `proxy.retryRequests` decides. + const failuresWithoutAResponse = { + timeout: () => new DOMException('The operation timed out.', 'TimeoutError'), + connect: () => new TypeError('fetch failed', { cause: new Error('connect ECONNREFUSED 127.0.0.1:8000') }), + }; + + for (const [name, createFetchError] of Object.entries(failuresWithoutAResponse)) { + it(`Will re-send the request when proxy.retryRequests is enabled (${name})`, async () => { + const retryingRest = createRetryingRest(); + + fetchStub.onFirstCall().rejects(createFetchError()); + fetchStub + .onSecondCall() + .resolves(new Response(JSON.stringify({ url: 'wss://gateway.discord.gg' }), { headers: { 'Content-Type': 'application/json' } })); + + expect(await retryingRest.makeRequest('GET', '/gateway/bot')).to.be.deep.equal({ url: 'wss://gateway.discord.gg' }); + expect(fetchStub.callCount).to.be.equal(2); + }); + } + + it('Will stop re-sending once maxRetryCount is exhausted, even with proxy.retryRequests enabled', async () => { + const retryingRest = createRetryingRest(); + retryingRest.maxRetryCount = 0; + fetchStub.rejects(new DOMException('The operation timed out.', 'TimeoutError')); + + await expect(retryingRest.makeRequest('GET', '/gateway/bot')).to.eventually.be.rejected; + expect(fetchStub.callCount).to.be.equal(1); + }); + + it('Will stop re-sending once maxProxyRetryCount is exhausted', async () => { + const retryingRest = createRetryingRest(); + retryingRest.maxProxyRetryCount = 2; + fetchStub.rejects(failuresWithoutAResponse.connect()); + + const error = await expect(retryingRest.makeRequest('GET', '/gateway/bot')).to.eventually.be.rejectedWith(Error); + expect(fetchStub.callCount).to.be.equal(3); + expect(error.cause).to.deep.include({ ok: false, status: 999 }); + }); + + it('Will count re-sends of different failures against the same maxRetryCount', async () => { + const retryingRest = createRetryingRest(); + retryingRest.maxRetryCount = 1; + + fetchStub.onFirstCall().rejects(failuresWithoutAResponse.timeout()); + fetchStub.onSecondCall().rejects(failuresWithoutAResponse.connect()); + fetchStub + .onThirdCall() + .resolves(new Response(JSON.stringify({ url: 'wss://gateway.discord.gg' }), { headers: { 'Content-Type': 'application/json' } })); + + await expect(retryingRest.makeRequest('GET', '/gateway/bot')).to.eventually.be.rejectedWith(Error); + // The timed out attempt used up the single retry this request gets, the failure to connect does not get one of its own + expect(fetchStub.callCount).to.be.equal(2); + }); + }); + + describe('rest.makeRequest with an AbortSignal', () => { + let fetchStub: sinon.SinonStub; + + beforeEach(() => { + fetchStub = sinon.stub(globalThis, 'fetch'); + }); + + afterEach(() => { + fetchStub.restore(); + }); + + it('Will reject an already aborted request without sending it', async () => { + const rest = createRestManager({ token }); + + await expect(rest.makeRequest('GET', '/gateway/bot', { signal: AbortSignal.abort() })).to.eventually.be.rejected; + expect(fetchStub.callCount).to.be.equal(0); + }); + + it('Will reject a queued request when the signal aborts', async () => { + // queueMicrotask is excluded from the faked timers because reading a Response body schedules one internally, and a faked one is never run + // as a real microtask, which makes the read spin forever. + const clock = sinon.useFakeTimers({ toNotFake: ['queueMicrotask'] }); + + try { + const rest = createRestManager({ token }); + // Hold the first request open so the next request stays stuck in the queue behind it + let finishFirstRequest: (value: Response) => void = () => {}; + fetchStub.callsFake(async (request: Request) => { + // Like real fetch, refuse to send a request whose signal is already aborted + if (request.signal.aborted) throw request.signal.reason; + return await new Promise((resolve) => { + finishFirstRequest = resolve; + }); + }); + + const first = rest.makeRequest('GET', '/gateway/bot'); + // Let the first request reach fetch and block the queue + await clock.tickAsync(0); + + const controller = new AbortController(); + const queued = rest.makeRequest('GET', '/gateway/bot', { signal: controller.signal }); + controller.abort(); + + // The caller is rejected right away, while the request is still stuck in the queue + await expect(queued).to.eventually.be.rejected; + expect(fetchStub.callCount).to.be.equal(1); + + // And the queue no longer holds on to it + const queue = [...rest.queues.values()][0]; + expect(queue.waiting.length).to.be.equal(0); + expect(queue.pending.length).to.be.equal(0); + + // Let the first request finish so the queue drains + finishFirstRequest( + new Response('{}', { + headers: { 'Content-Type': 'application/json', 'x-ratelimit-limit': '5', 'x-ratelimit-remaining': '4', 'x-ratelimit-reset-after': '1' }, + }), + ); + await clock.tickAsync(1000); + await first; + // Nothing is left of the cancelled request, so the queue never spends a rate limit slot on it + await clock.tickAsync(1000); + expect(fetchStub.callCount).to.be.equal(1); + } finally { + clock.restore(); + } + }); + + it('Will drop a queued request that was aborted before the queue got to it', async () => { + const clock = sinon.useFakeTimers({ toNotFake: ['queueMicrotask'] }); + + try { + const rest = createRestManager({ token }); + let finishFirstRequest: (value: Response) => void = () => {}; + fetchStub.callsFake(async (request: Request) => { + // Like real fetch, refuse to send a request whose signal is already aborted + if (request.signal.aborted) throw request.signal.reason; + return await new Promise((resolve) => { + finishFirstRequest = resolve; + }); + }); + + const first = rest.makeRequest('GET', '/gateway/bot'); + // Let the first request reach fetch and block the queue + await clock.tickAsync(0); + + const controller = new AbortController(); + const queued = rest.makeRequest('GET', '/gateway/bot', { signal: controller.signal }); + // Let it get past the waiting list and into the pending one before giving up on it + await clock.tickAsync(1000); + controller.abort(); + + await expect(queued).to.eventually.be.rejected; + + finishFirstRequest( + new Response('{}', { + headers: { 'Content-Type': 'application/json', 'x-ratelimit-limit': '5', 'x-ratelimit-remaining': '4', 'x-ratelimit-reset-after': '1' }, + }), + ); + await clock.tickAsync(1000); + await first; + await clock.tickAsync(1000); + + // The cancelled request is dropped instead of being sent with an already aborted signal + expect(fetchStub.callCount).to.be.equal(1); + const queue = [...rest.queues.values()][0]; + expect(queue.pending.length).to.be.equal(0); + } finally { + clock.restore(); + } + }); + + it('Will cancel an in flight request when the signal aborts', async () => { + const rest = createRestManager({ token }); + // Behave like real fetch: never settle until the signal on the request aborts + fetchStub.callsFake( + async (request: Request) => + await new Promise((_resolve, reject) => { + request.signal.addEventListener('abort', () => reject(request.signal.reason), { once: true }); + }), + ); + + const controller = new AbortController(); + const promise = rest.makeRequest('GET', '/gateway/bot', { signal: controller.signal }); + // Let the request go out before aborting + await new Promise((resolve) => setImmediate(resolve)); + controller.abort(); + + await expect(promise).to.eventually.be.rejected; + expect(fetchStub.callCount).to.be.equal(1); + // The signal must be forwarded to fetch so the connection itself gets torn down + expect((fetchStub.firstCall.args[0] as Request).signal.aborted).to.be.equal(true); + }); + + it('Will throw for an aborted request in proxy mode without contacting the proxy', async () => { + const rest = createRestManager({ token, proxy: { baseUrl: 'https://localhost:8000', authorization: token } }); + + const error = await expect(rest.makeRequest('GET', '/gateway/bot', { signal: AbortSignal.abort() })).to.eventually.be.rejectedWith(Error); + expect(fetchStub.callCount).to.be.equal(0); + expect(error.message).to.be.equal('[999] The request was aborted.'); + }); + + it('Will cancel an in flight request to the proxy when the signal aborts, without re-sending it', async () => { + const rest = createRestManager({ token, proxy: { baseUrl: 'https://localhost:8000', authorization: token } }); + // Behave like real fetch: never settle until the signal on the request aborts + fetchStub.callsFake( + async (request: Request) => + await new Promise((_resolve, reject) => { + request.signal.addEventListener('abort', () => reject(request.signal.reason), { once: true }); + }), + ); + + const controller = new AbortController(); + const promise = rest.makeRequest('GET', '/gateway/bot', { signal: controller.signal }); + // Let the request go out before aborting + await new Promise((resolve) => setImmediate(resolve)); + controller.abort(); + + await expect(promise).to.eventually.be.rejected; + expect(fetchStub.callCount).to.be.equal(1); + }); + + it('Will reject when the signal aborts while the proxy response body is being read', async () => { + const rest = createRestManager({ token, proxy: { baseUrl: 'https://localhost:8000', authorization: token } }); + // Behave like real fetch: resolve as soon as the headers are in and fail the body read once the signal aborts + fetchStub.callsFake(async (request: Request) => { + const body = new ReadableStream({ + start(streamController) { + streamController.enqueue(new TextEncoder().encode('{"url":')); + request.signal.addEventListener('abort', () => streamController.error(request.signal.reason), { once: true }); + }, + }); + + return new Response(body, { headers: { 'Content-Type': 'application/json' } }); + }); + + const controller = new AbortController(); + const promise = rest.makeRequest('GET', '/gateway/bot', { signal: controller.signal }); + // Let the headers arrive, the body is still being read at this point + await new Promise((resolve) => setImmediate(resolve)); + controller.abort(); + + const error = await expect(promise).to.eventually.be.rejectedWith(Error); + expect(error.message).to.be.equal('[999] The request was aborted.'); + }); + }); });