fix(dev): do not leak a rejection when a proxied upgrade fails - #4494
fix(dev): do not leak a rejection when a proxied upgrade fails#4494kedrzu wants to merge 1 commit into
Conversation
`DevServer.handleUpgrade` is async and both callers drop the promise it
returns: the `upgrade` method handed out by `createDevServer`, and the
`server.on("upgrade")` listener in `listen`. It rejects when no worker is
available, and — far more often — from `NodeDevWorker.handleUpgrade`,
which returns `proxy.ws(...)` unawaited. Replacing the dev worker kills
every websocket proxied to it, so that promise rejects with `ECONNRESET`
and nothing is there to catch it.
For Nitro alone that is a stray unhandled rejection. Under Nuxt it ends
the session, because the CLI treats any unhandled rejection as fatal and
restarts the dev server; with a client that reconnects it loops.
Make `handleUpgrade` never reject. There is nothing left to reply with
once a handshake is in flight, so a failed upgrade closes the socket, and
only errors that are not a peer disconnect are reported.
Resolves nitrojs#4493
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@kedrzu is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔗 Linked issue
Resolves #4493
❓ Type of change
📚 Description
DevServer.handleUpgradeisasync, and both of its callers drop the promise it returns:It rejects when no worker is available, and — far more often — from
NodeDevWorker.handleUpgrade, which returnsproxy.ws(...)without awaiting or catching it. Replacing the dev worker kills every websocket proxied to it, so that promise rejects withECONNRESETand nothing catches it.For Nitro alone that is a stray unhandled rejection. Under Nuxt it ends the session: the CLI treats any unhandled rejection as fatal and restarts the dev server, and with a client that keeps reconnecting it loops until the dev server is unusable. It has surfaced there repeatedly — nuxt/cli#247, nuxt/cli#968, nuxt/cli#994 — where it reads as a Windows/HMR flake, but the leak is here and platform-independent.
I traced it rather than inferred it: I wrapped both candidate call sites in a real project and logged which one rejected. Over 30 seconds —
NodeDevWorker.handleUpgrade8, the CLI request handler 0. Adding a.catch()at that call site removed the rejections, and the restarts with them.The change.
handleUpgradenow never rejects. There is nothing left to reply to the client with once a handshake is in flight, so a failed upgrade closes the socket, and only errors that are not a peer disconnect are reported. Fixing it here rather than in thelistenlistener is deliberate: Nuxt calls the exportedupgrademethod directly, so a guard on the listener alone would miss the case that motivated this.DevWorker["handleUpgrade"]is widened toPromise<void> | void, which is whatNodeDevWorkeralready returned, so awaiting it is type-correct.mainhas the same shape insrc/dev/server.ts—upgrade()isasyncandserver.on("upgrade", ...)drops the promise. Happy to follow up there if you would like it handled the same way.📝 Checklist