fix(wrangler): keep wrangler dev alive when forwarding one request to the Worker fails - #15448
LiyanChen-X wants to merge 26 commits into
Conversation
🦋 Changeset detectedLatest commit: aee0c1c 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 |
2177181 to
15e40ed
Compare
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
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: |
…to the Worker fails
The ProxyWorker treated a rejected `fetch()` to the UserWorker as a fatal
ProxyWorker error, which made the whole dev session exit. The common way to
hit it is a client that disconnects while its request body is still being
uploaded ("Network connection lost."), including a request that was queued
during startup/reload and abandoned before it could be replayed.
A rejected forward is the outcome of that single request, not a proxy
defect: answer it with a 502, log at debug level and keep the session
running. Errors thrown while post-processing a response keep the existing
fatal handling.
Adds an e2e that destroys the connection mid-upload against a real
`wrangler dev` process and asserts the process survives (fails on main
with ECONNREFUSED because the process exited).
15e40ed to
bae544f
Compare
LiyanChen-X
changed the title
wrangler dev alive when forwarding one request to the Worker fails
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
|
Tested the 1. Your aborted-upload repro, old vs. this branchYour
A variant that keeps the socket open instead of destroying it gives the same split: process exits on 4.126.0, alive on this branch. 2. The part I think is new: it also fires on a bodyless GETFive runs of our Playwright suite (~96 tests, ~2.5 min each, one dev server) on the branch. Four runs fully green; in the fifth, the new path fired once: The dev session stayed up and the suite ran to completion (95 passed). That request is a plain This seems to support the design argument in your PR description — "a rejected forward should not be fatal regardless of why it rejected" — with a case the description doesn't currently cover. On 4.126.0 the same rejection would have taken the 3. One consequence worth a line in the changesetFor a browser-driven suite the 502 is not invisible: it surfaces as a page-level 4. Log shapes — three different ones for the same causeWorth flagging for anyone building log-based triage (we did, for our own tracking issue):
Our extractor keys on the 5. Suite-level rates cannot show this PR's effect, and I would not use them
Happy to run this on Linux CI as well, or with a larger N, if that would help before merging. |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
dario-piotrowicz
left a comment
dario-piotrowicz
left a comment
There was a problem hiding this comment.
LGTM, thanks @LiyanChen-X 🙂
|
@LiyanChen-X there is a merge conflict, could you have a look? 🙏 |
…sh evidence (#338) ## Summary The dev deep-check dispatch (run 34258260004) exposed a second crash class the retry contract cannot see. Its e2e job died with a bare message-less `[WebServer] ✘ [ERROR]` and a wrangler debug log that simply stops mid-stream at 17:41:25: none of the four workers-sdk#15317 signature strings appear (`ProxyController` shows only as routine startup traffic), so the probe correctly reported "no upstream signature" and, once armed, `deep-check-crash-retry.yml` would leave the failure standing. The kill is still infrastructure, not product: 44 tests passed after boot, then the server vanished and every remaining test failed with a connection refusal. What the silent class cannot hide is that aftermath. A real test failure never produces mass refusals because the server outlives the suite, so a double-digit refusal count is server-death evidence on its own. This PR captures it and teaches the probe to accept it: the CI reporter list gains a Playwright JSON summary, the crash-probe action folds that file into the evidence directory the failure artifact already uploads, and `probe.sh` grants the retry on either signature: the four proxy-crash strings in the debug log, or ten-plus connection refusals in the summary. The floor of 10 keeps a stray single-test connectivity flake from claiming the rerun, and the `run_attempt` cap still bounds any wrong grant at one wasted rerun. Upstream context recorded in `docs/TODOS.md` (entry rewritten to present state): the fix PR workers-sdk#15448 is open and unmerged, wrangler 4.130.0 ships without it, and 2026-09-08 produced four crashes in four consecutive e2e executions, so escalation options (supervised webServer restart, per-project sharding) are on file for the day one retry is not enough. ## Changelog ## Type of Change - [x] `ci`: CI/CD configuration changes - [x] `test`: Adding or updating tests ## Related Issues/Stories - Story: n/a - Issue: upstream cloudflare/workers-sdk#15317 (crash), cloudflare/workers-sdk#15448 (open fix PR) - Architecture: n/a - Related PRs: #337 (crash-retry workflow + shared probe this extends) ## Testing - [x] Unit tests added/updated - [x] Manual testing completed - [x] All tests passing **Test Summary:** - New `tests/wrangler-crash-probe.test.ts` pins both signatures, the refusal floor, and the JSON-only scope of refusal counting; the mass-refusal case was observed failing against the prior probe.sh (5 pass, 1 fail) before the widening - probe.sh exercised against real artifacts: run 34218430087's log matches signature one; run 34258260004's log alone matches nothing; a summary carrying that run's refusal volume (74) matches signature two; clean and below-floor fixtures match nothing - The JSON reporter was observed writing `playwright-summary.json` (with the run-level error) even when the webServer fails to boot - Full suite green (bun test, 0 fail); typecheck, biome, markdownlint, knip, actionlint, shellcheck clean ## Files Modified **Modified:** - `.github/actions/wrangler-crash-probe/probe.sh`: second signature (mass connection refusals in the JSON summary) - `.github/actions/wrangler-crash-probe/action.yml`: collect step folds the summary into the evidence directory - `playwright.config.ts`: CI reporters gain the JSON summary - `.gitignore`: ignore `playwright-summary.json` - `docs/TODOS.md`: wrangler-death entry rewritten to present state with upstream pin and escalation options **Created:** - `tests/wrangler-crash-probe.test.ts`: fixture tests for both signatures and the floor **Renamed:** - None. **Deleted:** - None. ## Benefits - The silent-kill crash class now earns the same single fresh-runner rerun as the signed proxy crash, instead of a red nightly and manual triage. ## Breaking Changes - [x] No breaking changes ## Deployment Notes - [x] No special deployment steps required
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
Requesting changes because the current integration broadens the non-fatal behavior beyond failed forwarded requests.
The marker in ProxyWorker.ts correctly converts a rejected UserWorker fetch into a debug-logged 502. However, packages/wrangler/src/api/startDevWorker/DevEnv.ts still consumes every "Error inside ProxyWorker" event as recoverable. Failures in preview-token processing, response-header rewriting, or live-reload injection therefore also become non-fatal, contrary to the intended scope.
Please remove or narrow that broad DevEnv exemption. Add regression coverage proving that a fetch rejection returns 502 and leaves the session alive, while an unexpected response post-processing failure remains fatal.
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
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
checkForPreviewTokenError reads a clone of every 400 response after the UserWorker fetch has resolved, so a body stream that fails mid-read (connection to the UserWorker dropped) rejected past the UserWorkerFetchError marker and became a fatal ProxyWorker report. The sniff is best-effort: skip it when the body cannot be read. Also cover the ProxyController hop that turns a ProxyWorker error report into the "Error inside ProxyWorker" event DevEnv treats as fatal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBs8hK2eBFkHBgik22QB97
The hop it covered is a one-line switch case and DevEnv no longer keys on the event's reason string; the fatal contract is pinned by the DevEnv test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RBs8hK2eBFkHBgik22QB97
|
Thanks for catching this! I've removed the broad DevEnv exemption, so unexpected ProxyWorker errors remain fatal. I also handled the response-stream case: if reading a 400 response body fails during the preview-token check, the proxy now returns a readable 502 and logs at debug level. Added workerd regression tests for both plain-text and HTML response streams, verifying that the request isn't retried and subsequent requests still succeed. These pass alongside the DevEnv fatal-error test. |
|
thanks for catching the issue @petebacondarwin! 🫶 |
| let text: string; | ||
| try { | ||
| text = await clone.text(); | ||
| } catch (cause) { | ||
| // Both branches of the cloned body are broken. The response has not | ||
| // been sent yet, so replace it with a per-request error response. | ||
| throw new UserWorkerRequestError(cause); | ||
| } |
There was a problem hiding this comment.
mh.... this function is called checkForPreviewTokenError but it might end up throwing a preview-token-unrelated error? 🤔 this doesn't seem super clean to me
Could we maybe externalize the cloning (+ .text() call) to outside the function and perform the check there? 🤔
There was a problem hiding this comment.
Good point, thanks! Done in dd64936: the clone + .text() now happen at the call site (inside the res.status === 400 branch of the fetch handler), and that's also where a failed read is wrapped as a UserWorkerRequestError. The function itself is now a pure isPreviewTokenError(text): boolean, so it can no longer throw anything unrelated to the preview token check. Existing ProxyWorker/DevEnv tests still pass.
Move the 400 body clone and read to the call site so that isPreviewTokenError is a pure string check and can no longer throw a preview-token-unrelated error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ny5vSQndbLzLozXeW6Y3rs
…-request' into fix/proxyworker-abandoned-queued-request
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
Hi @petebacondarwin could you please help take a look? |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
@LiyanChen-X does your change also cover POSTs? Mine fail a few times per test run. I send them again from the client for now, since nothing reached the Worker. Thanks for taking this on, a few of us are hitting it 😢. |
6 participants
Fixes #15447. Also removes the crash reported in #15203 (verified with the same shape of repro, see below); the signature is the same one as #15317 / #4562.
What this does
ProxyWorkerforwards every incoming request to the UserWorker withfetch(). When thatfetch()rejects while the request still targets the current UserWorker, the rejection was sent to theProxyControlleras{ type: "error" }, which is fatal:wrangler devexits.The most common way to get such a rejection has nothing to do with the proxy or the Worker: the client disconnected while its request body was still being uploaded, so workerd fails the read of the body (
Network connection lost./Can't read from request stream because client disconnected). A singlecurl -m 0.4 -X POST --data-binary @2mb.bin http://127.0.0.1:8787/uploadagainst an otherwise idlewrangler devis enough to take the whole session down. Requests the proxy queued during startup/reload and that were abandoned before they could be replayed (the scenario in #15447) are the same failure, just later.A rejected forward is the outcome of that one request, not a defect in the proxy. This PR:
fetch()in aUserWorkerFetchErrormarker so it can be told apart from errors thrown while post-processing the response (checkForPreviewTokenError, live-reload injection, …);502(Could not proxy this request to your Worker: <message>) and logs it through the existingdebug-logchannel, instead of failing the session;503for the rest) untouched.The discrimination is structural (which promise rejected), not based on error message text.
Why not
request.signalrequest.signal.abortedwould be the principled signal for "the client is gone", so I tried it first: it staysfalsefor these requests, both before the replay and inside the rejection handler. TheProxyWorkerruns withcompatibilityFlags: ["nodejs_compat"], withoutenable_request_signal; addingenable_request_signalandrequest_signal_passthroughto theProxyWorkeralone does not change that either, because the request reaches theProxyWorkerthrough Miniflare's core entry worker (connect_pass_throughonly), so the abort never crosses that hop. Making the signal usable would mean turningrequest_signal_passthroughon for Miniflare's entry worker, which changes cancellation semantics for every user Worker in local dev — a much larger change than this bug warrants. And a rejected forward should not be fatal regardless of why it rejected.Verification
All against a locally built wrangler from this branch vs
main, on macOS with wrangler 4.127.1 / workerd 1.20260828.1.mainwrangler dev,curl -m 0.4 -X POST --data-binary @2mb.bin /uploadError in ProxyController: Error inside ProxyWorker,Network connection lost.)502, session keeps servingassets.directoryconfigured, 100 KB POSTs to an existing asset path (#15203 shape)405 500 405 500 405 000 000)405/502alternating, no exit)client disconnects > does not exit wrangler dev when a client aborts a request mid-bodyconnect ECONNREFUSED— the dev process exitedNote on the assets row: the alternating
405/502onmain's405/500is the pre-existing keep-alive behaviour after an unconsumed body (cf. #14641); this PR only stops it from being fatal.The e2e destroys the TCP connection mid-body with a raw
node:netsocket (Content-Length: 2000000, one 64 KB chunk,destroy()), because anAbortControllerabort on anundicistreaming body ends the request gracefully and does not reproduce the failure.pnpm check:type,tsc -p e2e/tsconfig.json,oxlint --type-awareandoxfmtpass on the touched files.Notes for reviewers
wrangler devbehaviour only for the case that previously terminated the process; a502with the underlying message is what the client (if it is still there) now sees.errormessage type non-fatal inDevEnv.handleErrorEvent" because that would also swallow genuine proxy bugs; the marker keeps post-processing errors loud.wrangler dev; no user-facing configuration or documented behaviour changes.