Repository navigation
[wrangler] Fail with an error instead of hanging under Bun - #15846
petebacondarwin merged 15 commits into
Conversation
🦋 Changeset detectedLatest commit: 617272b 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 |
@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/runtime-types
@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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
#15906 changed this PR's premise. Under bun, |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
… doors The guard in cloudflare#15845 ran inside listen(), so under Bun it also refused server.getWorker().fetch(), which never goes through the ProxyWorker and worked before. Move the check to server.fetch(), and have the ProxyWorker answer with a 503 once a control request arrives without its payload, so requests to the listen() URL fail loudly instead of queueing forever.
…er the harness under simulated Bun A request already forwarded to the UserWorker sits in neither queue when an empty control request drains them, and its retry is requeued asynchronously once the old connection fails. Route both requeue sites through requeueForRetry(), which answers with the same 503 while controlPayloadMissing is set instead of waiting for a play that cannot come. Add a createTestHarness() test that simulates Bun under Node (isBun() true, proxy control requests stripped of cf) and asserts listen() and getWorker().fetch() work while server.fetch() throws and the listen() URL answers 503.
…15906 routes getWorker() through the dispatcher too
f874268 to
4712825
Compare
…orkflows-shared Both are HeadersTimeoutError / waitUntil timeouts on the Windows runners. The same fixture ran 17/17 on Windows at this branch's previous head, and an unrelated PR failed it with the same HeadersTimeoutError.
…ing every binding request
startRemoteProxySession reported ready once the remote Worker reloaded,
but the `play` message that hands the ProxyWorker that session was sent
in the background. Under Bun it never arrives: Miniflare's dispatchFetch()
routes it through an undici dispatcher that Bun's fetch() ignores, so it
goes to the literal http://dummy/ URL and fails with ENOTFOUND. The error
was emitted after startup stopped listening, and the ProxyWorker queued
every remote binding request forever, as reported for
getPlatformProxy({ remoteBindings: true }) in cloudflare#15717.
ProxyController now records whether `play` was delivered, and startup
waits for it. Under Bun a failed delivery throws a UserError naming
oven-sh/bun#39247; elsewhere it goes through the existing startup error
path.
…ctly under Bun Since cloudflare#15906, Miniflare rewrites dispatchFetch()'s URL onto the ProxyWorker's origin inside the undici dispatcher, which Bun's fetch() ignores. Under Bun a control request now goes to the literal http://dummy/ URL and fails, so it never reaches the ProxyWorker, and the 503 this PR gives requests to the listen() URL never fired: on Bun 1.4.4 those requests hung again (no answer after 15 s), while server.fetch() still threw its UserError. When every retry of a control request fails under Bun, ProxyController now sends one without a payload straight to the ProxyWorker's own URL, which Bun can reach. The ProxyWorker's existing handling for a missing payload then answers queued and later requests with the 503. Measured on Bun 1.4.4 with wrangler built from this branch: fetch(listenUrl) gets the 503 naming oven-sh/bun#39247; Node is unchanged.
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
This pull request was not reviewed automatically because its author does not have write access to this repository. A maintainer with write access can start a review by commenting |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…y after updateBindings DevEnv only logs a failed `play` send and never re-emits it, so an error event that arrives while startup waits for `play` is a separate failure (authentication, the remote preview). Startup now reports it as itself even under Bun, and keeps the Bun-specific error for a `play` that was not delivered. ProxyController records that send failure in `latestSendError`, since the event bus never carried it; the test mock emitted it as an error event, which the real DevEnv does not, so the mock now matches DevEnv. updateBindings now waits for the new `play` to be acknowledged, as startup does, instead of only for the message mutex to drain. The changeset lists the Vite and Vitest plugins, which bundle the private @cloudflare/remote-bindings package.
Wrangler's rule is undici's fetch, never the global one. Wrangler bundles undici, so its fetch and dispatcher work under Bun (only Miniflare's runtime `undici` import resolves to Bun's dispatcher-less shim). Measured on Bun 1.4.4 with undici 7.29.1 inlined by esbuild: an Agent with rejectUnauthorized false reaches a self-signed HTTPS server (204), and the same request without it fails with DEPTH_ZERO_SELF_SIGNED_CERT.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…cribes an older message A non-204 answer, or a ProxyWorker that was not ready, returned false without touching latestSendError, so a failure left over from an earlier message was reported as the cause of a later rejected `play`. Each send now sets latestSendError to its own outcome: cleared on a 204, a status error on any other answer.
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
Please can you clean up the PR description here?
We don't need to know the history of changes to the PR. Instead a short clear description of what this PR achieves and why.
In general we don't land PRs to improve Bun compatibility - any compatibility issue is deemed on Bun to fix.
In this case it appears that yo are just trying to provide better error messages when Bun fails. We could consider this although as it stands the PR is quite complicated and are a loathe to add such logic just for Bun.
Each Bun failure now gets one check at its entry point: the harness's dispatchFetch() helper throws a UserError, the ProxyWorker answers with a 503 when ProxyController passes it the Bun message as a binding, and startRemoteProxySession() throws a UserError. This drops the play-delivery tracking in both ProxyController copies, the ProxyWorker's missing-payload state, the undici fallback request and the harness timeout.
oddharsh
changed the title
|
Ah for sure, so sorry about the wordiness on the description before - I'd assumed it'd gotten a bit lost as the PR morphed. Also totally understand with regards to not going out of our way to write this repo around Bun! I’ve rewritten the description and cut the change to error messages only: one isBun() check (Dario’s helper from #15845) wherever a request would otherwise hang. The diff went from 963 lines across 16 files to 177 across 10 (617272b). Happy to drop the remote-bindings check too if you’d rather keep this to the test harness! |
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
4 participants
Fixes #15717.
Under Bun, Wrangler's local dev requests hang with no error, because Bun's
fetch()ignores the undicidispatcherthat Miniflare routes them through (oven-sh/bun#39247). The real fix belongs in Bun. This PR adds oneisBun()check at each place a user would otherwise wait forever, and each check throws or responds with an error that names the Bun issue:createTestHarness(): thedispatchFetch()helper behindserver.fetch()andgetWorker()'sfetch(),email()andscheduled()throws aUserError.getEnv()andgetExport()don't dispatch, so they keep working.ProxyControllerpasses the error text to the ProxyWorker as a binding, and the ProxyWorker answers with a 503 instead of queueing the request.startRemoteProxySession()throws aUserErrorbefore starting the session.This builds on #15845 and includes its commit under its original author. #15845 fails in
listen(), which also blocksgetEnv().isBun(), and fails on a 200 without the ProxyWorker change. On Bun 1.4.4 with this branch built,getEnv()works, bothfetch()calls throw theUserError, and the dev server URL answers 503; Node is unchanged. Remote bindings were not run against a live session under Bun.server.fetch()JSDoc describes the Bun behaviourNote
This is a contribution from an AI agent: Claude Code, running Claude Opus 5.5. It was written on instructions from @oddharsh and under their supervision, and @oddharsh reviewed the change and this description before submission.