fix(wrangler): keep wrangler dev alive when a proxied request to the UserWorker fails transiently - #15252
Conversation
…UserWorker fails transiently When a request proxied to the UserWorker failed while the UserWorker's origin was unchanged — most commonly a reused keep-alive connection the UserWorker's HTTP server closed at the same moment the 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) — the ProxyWorker reported a fatal error and the whole dev server exited with an empty error message, leaving the port unbound. The ProxyWorker now retries bodyless (GET/HEAD) requests before reporting, absorbing the transient failure on a fresh connection, and DevEnv classifies an exhausted or non-retriable report as recoverable: it is logged with the request method, URL, attempt count and underlying exception, and the dev server keeps serving. Only the affected request fails. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5fc41f0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| When a request proxied to the UserWorker failed while the UserWorker's origin was unchanged — most commonly a reused keep-alive connection that the UserWorker's HTTP server closed at the same moment the request was written to it — the ProxyWorker reported a fatal error and the whole dev server exited with an empty `✘ [ERROR]`, leaving the port unbound. In CI test suites one such transient failure killed every remaining test. | ||
|
|
||
| The ProxyWorker now retries bodyless (GET/HEAD) requests before reporting, which absorbs the transient failure on a fresh connection, and an exhausted or non-retriable failure is logged — including the request method, URL and underlying exception — while the dev server keeps serving. Only the affected request fails. |
There was a problem hiding this comment.
🟡 Changeset text describes internal implementation instead of user-facing impact
The changeset explains the internal proxy component and its retry mechanics (.changeset/tidy-moons-provide.md:9), which conflicts with the repository requirement that changesets describe user-facing impact rather than implementation details.
Impact: The published changelog entry reads as maintainer-facing internals instead of describing the fix to Wrangler users.
Details
REVIEW.md: "Changesets should target users of the tools (e.g. Wrangler users) rather than maintainers. Avoid including implementation details ... Instead, focus on user-facing impact and benefits." The third paragraph names the internal ProxyWorker, its retry-before-reporting behaviour and the GET/HEAD distinction; the user-facing statement (wrangler dev survives a transient proxied-request failure, only the affected request fails and it is logged) is sufficient.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Devin's comment is correct, could you make the changeset more user-facing? 🙏
dario-piotrowicz
left a comment
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Thank you very much @GregoryCollett 😄
The PR looks overall good to me but there are a few bits that I think should be looked at before merging, could you please have a look? 🙏
| "GET http://127.0.0.1:8787/ (attempt 3): Network connection lost." | ||
| ), | ||
| source: "ProxyController", | ||
| data: undefined, |
There was a problem hiding this comment.
This type is incorrect
| data: undefined, | |
| data: {}, |
|
|
||
| When a request proxied to the UserWorker failed while the UserWorker's origin was unchanged — most commonly a reused keep-alive connection that the UserWorker's HTTP server closed at the same moment the request was written to it — the ProxyWorker reported a fatal error and the whole dev server exited with an empty `✘ [ERROR]`, leaving the port unbound. In CI test suites one such transient failure killed every remaining test. | ||
|
|
||
| The ProxyWorker now retries bodyless (GET/HEAD) requests before reporting, which absorbs the transient failure on a fresh connection, and an exhausted or non-retriable failure is logged — including the request method, URL and underlying exception — while the dev server keeps serving. Only the affected request fails. |
There was a problem hiding this comment.
Devin's comment is correct, could you make the changeset more user-facing? 🙏
| // 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 #14926). Log it and keep serving. |
There was a problem hiding this comment.
| // a dead dev server (see #14926). Log it and keep serving. | |
| // a dead dev server (see https://github.com/cloudflare/workers-sdk/pull/14926). Log it and keep serving. |
| ); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we have a warn/error log if attempts >= 3 specifying that we did try to re-handle the request?
| } | ||
| }); | ||
|
|
||
| attemptUserWorkerFetch(1); |
There was a problem hiding this comment.
Could we have the argument optional? (and can we start from 0?)
| attemptUserWorkerFetch(1); | |
| attemptUserWorkerFetch(); |
| status: 503, | ||
| headers: { "Retry-After": "0" }, | ||
| } | ||
| const attemptUserWorkerFetch = (attempt: number) => |
There was a problem hiding this comment.
As I suggested in my other comment, I think it'd be nice to have the attempt number optional (and starting from 0)
| const attemptUserWorkerFetch = (attempt: number) => | |
| const attemptUserWorkerFetch = (attempt = 0) => |
- Move the GET/HEAD retry into fetch's rejection handler so errors
thrown while post-processing a received response are reported, never
retried (and can never re-run the UserWorker's handler).
- Make the attempt parameter optional and 0-indexed.
- Report exhausted retries explicitly ("failed after 3 attempts").
- data: {} in the DevEnv test; full issue URL in the DevEnv comment.
- Rewrite the changeset to be user-facing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All review comments addressed in 8adab46:
Re-verified end-to-end on the updated branch: SIGKILLing the UserWorker runtime under a 4-thread GET storm — 1034 requests, 0 client-visible failures, 4 recovered on the delayed third attempt, dev server kept serving. While in here I noticed |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| logger.error( | ||
| `${event.reason} (the affected request failed; the dev server continues): ${event.cause.message}` | ||
| ); |
There was a problem hiding this comment.
🟡 Logged error for a failed dev request shows a blank reason instead of the request details
The failed proxied request is logged (event.cause.message at packages/wrangler/src/api/startDevWorker/DevEnv.ts:202) using the wrong property, so the method, URL, attempt count and underlying error come out blank and the user only sees a generic message with nothing after the colon.
Impact: When a request to the Worker fails, the dev server keeps running (good) but the logged error is empty of any useful detail, reproducing the very empty-message symptom this change claims to fix.
Why event.cause.message is empty after the JSON boundary
The ProxyWorker packs the useful text (method, URL, attempt count, underlying error.message) into message.error.message and posts it as JSON (packages/wrangler/templates/startDevWorker/ProxyWorker.ts:267-275). On the controller side ProxyController.onProxyWorkerMessage calls emitErrorEvent("Error inside ProxyWorker", message.error) (packages/wrangler/src/api/startDevWorker/ProxyController.ts:478), passing a plain SerializedError object (it crossed JSON.stringify/await req.json(), so it is NOT an Error instance).
emitErrorEvent sets cause: castErrorCause(message.error) (packages/wrangler/src/api/startDevWorker/ProxyController.ts:653). Because the serialized error is not instanceof Error, castErrorCause returns new Error() with an empty message and stashes the serialized object on .cause (packages/wrangler/src/api/startDevWorker/events.ts:32-41).
So in production event.cause.message is "", while the real detail lives at event.cause.cause.message. The new log at DevEnv.ts:201-203 interpolates the empty event.cause.message, yielding Error inside ProxyWorker (the affected request failed; the dev server continues): with nothing after the colon.
The new test DevEnv.test.ts:107-115 masks this because it dispatches an ErrorEvent whose cause is a real new Error("GET ... Network connection lost.") rather than the empty-wrapper Error that castErrorCause actually produces in production.
Prompt for agents
In DevEnv.handleErrorEvent, the recoverable branch for "Error inside ProxyWorker" logs `event.cause.message`, but for this path `event.cause` is an empty-message wrapper Error produced by castErrorCause (events.ts) because ProxyController.onProxyWorkerMessage passes a plain SerializedError object (post-JSON) to emitErrorEvent. The human-readable detail (method, URL, attempt count, underlying message that ProxyWorker.ts assembles) is therefore located at event.cause.cause.message, not event.cause.message. As written the log prints a blank tail after the colon, which reproduces the empty-message symptom this PR intends to fix. Fix the log so it surfaces the underlying message: either read the nested serialized cause's message when event.cause.message is empty, or pass event.cause as an additional argument to logger.error so its cause chain is rendered. Consider also updating the test in DevEnv.test.ts to dispatch a cause shaped like what castErrorCause actually produces (an empty-message Error with the real SerializedError on .cause) so the test reflects production behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Fixes #14926. Also mitigates the empty-message symptom of #14906 for this path by including the request method, URL, attempt count and underlying exception in the logged error.
The bug
wrangler devruns the ProxyWorker (binding the public port) and the UserWorker runtime as separate workerd processes. When a proxied request to the UserWorker fails while the UserWorker's origin is unchanged, the ProxyWorker reports{type: "error"},ProxyController.onProxyWorkerMessageescalates it viaemitErrorEvent,DevEnv.handleErrorEventfalls through to the fatal re-emit, and the entire dev server exits with an empty✘ [ERROR](the error crosses a JSON boundary, socastErrorCauseproduces a message-lessError), leaving the port unbound. In CI test suites, one transient failure kills every remaining test withERR_CONNECTION_REFUSED— several independent reports on #14926 describe exactly this.Root cause of the transient failure itself
kj HTTP defaults collide: the client pool closes idle connections after 5s (
HttpClientSettings.idleTimeout) and the server closes idle keep-alive connections after 5s (HttpServerSettings.pipelineTimeout). For a pooled ProxyWorker→UserWorker connection idling ≈5s, both timers fire ~simultaneously; a request written into the connection as the server's close is in flight gets an RST, surfaced asError: Network connection lost..Reproduced organically (no fault injection): bursts of ~48 concurrent GETs separated by idle gaps swept 3.0→7.9s in 0.1s steps against
wrangler devhit the fatal within one sweep, at a 4.2s outer gap (per-connection idle 4.2–5.2s given the ~1s burst spread) — failed requests interleaved with same-millisecond 200s (stale pool checkouts die, fresh connections succeed), while the UserWorker runtime never exited and its isolate was never re-created. Deterministic variant: SIGKILL the UserWorker runtime under request load (miniflare restarts it reusing the port, so the in-flight failures are same-origin).The fix (two small pieces)
ProxyWorker.ts: same-origin fetch failures now retry bodyless (GET/HEAD) requests — immediately, then once more after 250ms — before reporting. The retry draws a fresh connection, absorbing the race. This mirrors the existing requeue-on-reload behaviour and its rationale comment ("it would be incorrect to retry non-idempotent requests"); non-GET/HEAD behaviour is unchanged. Recovered requests log aconsole.warnso absorbed events remain visible. The reported error now includes method, URL and attempt count.DevEnv.ts:handleErrorEventclassifies"Error inside ProxyWorker"reports as recoverable — logged vialogger.errorwith the underlying exception — instead of falling through to the fatal top-level re-emit. This sits alongside the existing recoverable carve-out for other ProxyController reasons. One failed proxied request should never take down the dev session.With both pieces, the kill-the-runtime repro goes from "dev server exits, port unbound" to: in-flight GETs retry across the restart (5 of 11 recovered in my run), the rest fail individually with the error logged, and the server keeps serving.
DevEnv.test.tsgains a test asserting a ProxyWorker error report is logged and does not re-emit a fatal top-levelerrorevent.wrangler devsession two ways — (a) the organic idle-gap sweep described above (7,200 requests: zero server-side failures with the fix; the fatal within one sweep without it), and (b) SIGKILL-ing the UserWorker runtime under load (server survives; GETs recover across the restart). Note: I could not run the wrangler vitest suite locally because@cloudflare/remote-bindings#buildfails in my environment on a pristinemaincheckout (its tsdownembed-workersplugin resolves template paths incorrectly there), so I'm relying on CI for the full suite;turbo check:type --filter=wranglerandoxfmtpass locally.wrangler devinternals; a changeset is included.Note
This is a contribution from an AI agent: Claude Code (Claude Fable 5), working under the direction and review of Gregory Collett.