mirror of
https://github.com/discordeno/discordeno.git
synced 2026-09-17 08:47:22 +00:00
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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <Fleny113@outlook.com> * Run biome --------- Co-authored-by: Fleny <fleny113@outlook.com> Co-authored-by: Awesome Stickz <38146668+AwesomeStickz@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
Fleny
Awesome Stickz
parent
de232c06b3
commit
74f93eda6d
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user