diff --git a/.changeset/tidy-moons-provide.md b/.changeset/tidy-moons-provide.md new file mode 100644 index 00000000000..c5a8949ee00 --- /dev/null +++ b/.changeset/tidy-moons-provide.md @@ -0,0 +1,9 @@ +--- +"wrangler": patch +--- + +`wrangler dev` no longer exits when a request to your Worker fails transiently + +Previously, a transient network failure on a single request — most commonly a request arriving just as an idle internal connection was closed, after roughly five seconds without traffic — could take down the whole dev server with an empty `✘ [ERROR]`, leaving the port unbound until restarted. In CI test suites, one such failure caused every remaining test to fail with connection errors. + +`wrangler dev` now automatically retries the affected request if it is safe to repeat (GET and HEAD requests). If a request still fails, it fails individually — the error is logged with the request method and URL — and the dev server keeps serving. diff --git a/packages/wrangler/src/__tests__/api/startDevWorker/DevEnv.test.ts b/packages/wrangler/src/__tests__/api/startDevWorker/DevEnv.test.ts index 3179032a7fb..9fb178d46db 100644 --- a/packages/wrangler/src/__tests__/api/startDevWorker/DevEnv.test.ts +++ b/packages/wrangler/src/__tests__/api/startDevWorker/DevEnv.test.ts @@ -95,5 +95,31 @@ describe("DevEnv", () => { void devEnv.teardown(); }); + + test("should log ProxyWorker request errors without tearing down the dev session", ({ + expect, + }) => { + const devEnv = new DevEnv(); + + const fatalEvents: unknown[] = []; + devEnv.on("error", (event) => fatalEvents.push(event)); + + devEnv.dispatch({ + type: "error", + reason: "Error inside ProxyWorker", + cause: new Error( + "GET http://127.0.0.1:8787/ (failed after 3 attempts): Network connection lost." + ), + source: "ProxyController", + data: {}, + }); + + expect(std.err).toContain("Error inside ProxyWorker"); + expect(std.err).toContain("Network connection lost."); + // one failed proxied request must not become a fatal dev-session error + expect(fatalEvents).toHaveLength(0); + + void devEnv.teardown(); + }); }); }); diff --git a/packages/wrangler/src/api/startDevWorker/DevEnv.ts b/packages/wrangler/src/api/startDevWorker/DevEnv.ts index 6752a46f178..7678bbdbc5a 100644 --- a/packages/wrangler/src/api/startDevWorker/DevEnv.ts +++ b/packages/wrangler/src/api/startDevWorker/DevEnv.ts @@ -187,6 +187,22 @@ export class DevEnv extends EventEmitter implements ControllerBus { logger.debug(`Error in ${event.source}: ${event.reason}\n`, event.cause); logger.debug("=> Error contextual data:", event.data); } + // A proxied request to the UserWorker failed while the UserWorker was NOT + // being reloaded — e.g. the UserWorker's HTTP server closed a reused + // keep-alive connection at the same moment the ProxyWorker wrote a + // request into it. The affected request has already failed (and the + // ProxyWorker retries GET/HEAD before reporting), but the dev session + // itself is healthy: tearing it down would turn one failed request into + // a dead dev server (see https://github.com/cloudflare/workers-sdk/issues/14926). Log it and keep serving. + else if ( + event.source === "ProxyController" && + event.reason.startsWith("Error inside ProxyWorker") + ) { + logger.error( + `${event.reason} (the affected request failed; the dev server continues): ${event.cause.message}` + ); + logger.debug("=> Error contextual data:", event.data); + } // Parse errors are recoverable by changing your Wrangler configuration file and saving // All other errors from the ConfigController are non-recoverable else if ( diff --git a/packages/wrangler/templates/startDevWorker/ProxyWorker.ts b/packages/wrangler/templates/startDevWorker/ProxyWorker.ts index 59b507d72fe..fed5b41a4f8 100644 --- a/packages/wrangler/templates/startDevWorker/ProxyWorker.ts +++ b/packages/wrangler/templates/startDevWorker/ProxyWorker.ts @@ -163,81 +163,150 @@ export class ProxyWorker implements DurableObject { } } - // explicitly NOT await-ing this promise, we are in a loop and want to process the whole queue quickly + synchronously - void fetch(userWorkerUrl, new Request(request, { headers })) - .then(async (res) => { - res = new Response(res.body, res); - rewriteUrlRelatedHeaders(res.headers, innerUrl, outerUrl); - - await checkForPreviewTokenError(res, this.env, proxyData); - - if (isHtmlResponse(res)) { - res = insertLiveReloadScript(request, res, this.env, proxyData); - } - - if (isSseResponse(res)) { - void sendMessageToProxyController(this.env, { - type: "sseResponseDetected", - }); - } - - deferredResponse.resolve(res); - }) - .catch((error: Error) => { - // errors here are network errors or from response post-processing - // to catch only network errors, use the 2nd param of the fetch.then() - - // we have crossed an async boundary, so proxyData may have changed - // if proxyData.userWorkerUrl has changed, it means there is a new downstream UserWorker - // and that this error is stale since it was for a request to the old UserWorker - // only report the error if the request still targets the current - // UserWorker. isSameUserWorkerOrigin compares origin (not href) so a - // genuine error on a non-root path isn't misread as a reload — see - // its docs. - if ( - isSameUserWorkerOrigin(userWorkerUrl, this.proxyData?.userWorkerUrl) - ) { - void sendMessageToProxyController(this.env, { - type: "error", - error: { - name: error.name, - message: error.message, - stack: error.stack, - cause: error.cause, - }, - }); - - deferredResponse.reject(error); - } - - // if the request can be retried (subset of idempotent requests which have no body), requeue it - else if (request.method === "GET" || request.method === "HEAD") { - this.requestRetryQueue.set(request, deferredResponse); - // we would only end up here if the downstream UserWorker is chang*ing* - // i.e. we are in a `pause`d state and expecting a `play` message soon - // this request will be processed (retried) when the `play` message arrives - // for that reason, we do not need to call `this.processQueue` here - // (but, also, it can't hurt to call it since it bails when - // in a `pause`d state i.e. `this.proxyData` is undefined) - } - - // if the request cannot be retried, respond with 503 Service Unavailable - // important to note, this is not an (unexpected) error -- it is an acceptable flow of local development - // it would be incorrect to retry non-idempotent requests - // and would require cloning all body streams to avoid stream reuse (which is inefficient but not out of the question in the future) - // this is a good enough UX for now since it solves the most common GET use-case - else { - deferredResponse.resolve( - new Response( - "Your worker restarted mid-request. Please try sending the request again. Only GET or HEAD requests are retried automatically.", - { - status: 503, - headers: { "Retry-After": "0" }, - } + /** + * Sends the request to the UserWorker and settles `deferredResponse` + * with the outcome — or requeues the request if the UserWorker is + * mid-reload. The promise chain is deliberately not awaited (`void`): + * we are in a loop and want to process the whole queue quickly + + * synchronously. + * + * A connection-level failure (the `fetch` itself rejects, so no + * response was received) on a same-origin GET/HEAD request is retried + * before being reported — see the rejection handler. + * + * Kept as a `const` arrow function: the handlers re-read + * `this.proxyData` across async boundaries to detect UserWorker + * reloads, so they need the ProxyWorker's lexical `this`. + * + * @param attempt the number of attempts that have already failed + * (`0` on the first try) + */ + const attemptUserWorkerFetch = (attempt = 0) => + void fetch(userWorkerUrl, new Request(request, { headers })) + .then( + async (res) => { + if (attempt > 0) { + console.warn( + `ProxyWorker: ${request.method} ${request.url} recovered on attempt ${attempt + 1} after a dropped connection to the UserWorker` + ); + } + + res = new Response(res.body, res); + rewriteUrlRelatedHeaders(res.headers, innerUrl, outerUrl); + + await checkForPreviewTokenError(res, this.env, proxyData); + + if (isHtmlResponse(res)) { + res = insertLiveReloadScript(request, res, this.env, proxyData); + } + + if (isSseResponse(res)) { + void sendMessageToProxyController(this.env, { + type: "sseResponseDetected", + }); + } + + deferredResponse.resolve(res); + }, + (error: Error) => { + // the fetch itself rejected: a connection-level failure, and no + // response was received. Errors thrown while post-processing a + // received response skip this handler and land in the .catch + // below, so they are reported rather than retried and can never + // re-run the UserWorker's handler. + // + // When the UserWorker origin is unchanged (i.e. this is not a + // reload — see the same check in the .catch below), the failure + // is most commonly the UserWorker's HTTP server closing a reused + // keep-alive connection at the same moment this request was + // written to it (kj's client pool idleTimeout and server + // pipelineTimeout both default to 5s, so a connection idling ~5s + // races the close). Retrying bodyless requests draws a fresh + // connection and absorbs the race, mirroring the requeue in the + // .catch for reloads: retry immediately, then once more after + // 250ms (3 attempts in total). + if ( + isSameUserWorkerOrigin( + userWorkerUrl, + this.proxyData?.userWorkerUrl + ) && + (request.method === "GET" || request.method === "HEAD") && + attempt < 2 + ) { + setTimeout( + () => attemptUserWorkerFetch(attempt + 1), + attempt === 0 ? 0 : 250 + ); + return; + } + + throw error; + } + ) + .catch((error: Error) => { + // errors here are from response post-processing, or connection- + // level failures rethrown by the rejection handler above (a + // non-retriable method, or the retry budget was exhausted) + + // we have crossed an async boundary, so proxyData may have changed + // if proxyData.userWorkerUrl has changed, it means there is a new downstream UserWorker + // and that this error is stale since it was for a request to the old UserWorker + // only report the error if the request still targets the current + // UserWorker. isSameUserWorkerOrigin compares origin (not href) so a + // genuine error on a non-root path isn't misread as a reload — see + // its docs. + if ( + isSameUserWorkerOrigin( + userWorkerUrl, + this.proxyData?.userWorkerUrl ) - ); - } - }); + ) { + const attemptsNote = ` (failed after ${attempt + 1} ${ + attempt === 0 ? "attempt" : "attempts" + })`; + void sendMessageToProxyController(this.env, { + type: "error", + error: { + name: error.name, + message: `${request.method} ${request.url}${attemptsNote}: ${error.message}`, + stack: error.stack, + cause: error.cause, + }, + }); + + deferredResponse.reject(error); + } + + // if the request can be retried (subset of idempotent requests which have no body), requeue it + else if (request.method === "GET" || request.method === "HEAD") { + this.requestRetryQueue.set(request, deferredResponse); + // we would only end up here if the downstream UserWorker is chang*ing* + // i.e. we are in a `pause`d state and expecting a `play` message soon + // this request will be processed (retried) when the `play` message arrives + // for that reason, we do not need to call `this.processQueue` here + // (but, also, it can't hurt to call it since it bails when + // in a `pause`d state i.e. `this.proxyData` is undefined) + } + + // if the request cannot be retried, respond with 503 Service Unavailable + // important to note, this is not an (unexpected) error -- it is an acceptable flow of local development + // it would be incorrect to retry non-idempotent requests + // and would require cloning all body streams to avoid stream reuse (which is inefficient but not out of the question in the future) + // this is a good enough UX for now since it solves the most common GET use-case + else { + deferredResponse.resolve( + new Response( + "Your worker restarted mid-request. Please try sending the request again. Only GET or HEAD requests are retried automatically.", + { + status: 503, + headers: { "Retry-After": "0" }, + } + ) + ); + } + }); + + attemptUserWorkerFetch(); } } }