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: fc9c3f1 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: |
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? 🙏
- 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>
The ProxyWorker's report crosses a JSON boundary, so castErrorCause wraps a plain object in a message-less Error and the real detail (method, URL, attempt count, underlying error) sits on .cause. Reading event.cause.message logged nothing after the colon — the same empty-error symptom this PR fixes. The test now builds its cause through JSON and castErrorCause, so it can no longer pass with a message the production path never has. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codeowners approval required for this PR:
Show detailed file reviewers |
A retry scheduled before a reload begins would fire with the captured proxyData, so it could reach the pre-reload Worker or send a stale preview token. Re-check proxyData identity when the timer fires and requeue instead, letting processQueue rebuild the request from current routing and headers. Compares identity rather than origin because the origin can be unchanged while the headers are stale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
|
Thanks for the changes @GregoryCollett 🙏 |
The rejected UserWorker fetch is now answered with a 502 inside the ProxyWorker itself, so the "Error inside ProxyWorker" report can only mean the ProxyWorker failed while post-processing a received response — a genuine proxy defect. Drop the recoverable branch DevEnv gained in cloudflare#15252 so such reports are fatal again, and flip its regression test to assert the re-emit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBs8hK2eBFkHBgik22QB97
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.