Repository navigation
[vite-plugin] Route v1 DevTools WebSocket upgrades explicitly - #16538
Conversation
|
| Name | Type |
|---|---|
| @cloudflare/vite-plugin | Patch |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
|
@petebacondarwin (original author) @dario-piotrowicz (human ant-core approver): please check this v1 backport of #16519 and approve it if it looks good. The original routing fix, real-socket tests and existing changeset apply directly on the v1 stack; #16444 supplies the earlier runtime dependencies. Build, checks, retained tests and WebSockets dev/preview pass. |
|
Codeowners approval required for this PR:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!workerWebSocket) { | ||
| // This listener does not own the socket. Destroy it only if no | ||
| // other listener has written a response in the meantime. | ||
| if (!isClaimed()) { | ||
| socket.destroy(); | ||
| } | ||
| socket.destroy(); |
There was a problem hiding this comment.
🔴 Other plugins' WebSocket connections break
When another listener upgrades a non-DevTools path, handleWebSocket still dispatches it to the Worker. A non-WebSocket response destroys the claimed socket; a WebSocket response attempts a second handshake.
Learn more
Node invokes every upgrade listener for an incoming request. A Vite plugin can register its own listener for /custom-ws before this handler. The removed claimed-socket check protected an already-upgraded socket when the Worker later returned no WebSocket, and also prevented a second handshake when it did return one. Merely reserving the DevTools path does not protect other listeners. The socket is now destroyed or upgraded twice depending on the Worker response.
Example: A plugin accepts /custom-ws and sends a 101 response. The Worker returns 404 for /custom-ws; this handler calls socket.destroy() and closes the plugin's live connection.
Recommended fix: Restore an ownership check for non-reserved paths before destroying or upgrading a socket. Distinguish an earlier HTTP response on a pipelined connection from another listener's handshake, since the removed byte-count check rejected valid Worker upgrades in that case. Test a second listener on a non-DevTools path with both Worker response types.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if ((isViteRequest || isDevToolsRequest) && !isSandboxRequest) { | ||
| return; |
There was a problem hiding this comment.
🟡 Preview WebSocket route hangs without a handler
In preview, handleWebSocket skips /__devtools/__ws despite no DevTools listener handling that path. A Worker WebSocket at that path never receives the upgrade, leaving its client waiting.
Learn more
The same handleWebSocket function is installed by both devPlugin and previewPlugin. A preview server does not have the Vite DevTools handler that this pathname exemption expects. Returning from the only relevant upgrade listener does not send an HTTP response or close the socket. Previously the Worker received this request and could return a WebSocket.
Example: A Worker handles /__devtools/__ws with a WebSocket response. Connecting to that path on vite preview now receives neither a 101 response nor the Worker's messages.
Recommended fix: Scope the DevTools exemption to development servers where the DevTools upgrade listener is actually present, and preserve Worker forwarding in preview. Add a preview-specific upgrade test.
Was this helpful? React with 👍 or 👎 to provide feedback.
commit: |
|
💬 Approved with comments — The routing change is correct and well tested. The only item is an optional suggestion to describe the pipelined-upgrade fix in the changeset. Findings: 1 suggestion The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 🚀 Release: 💬 1 suggestionThe changeset could also mention the pipelined-upgrade fix. Recommendation: 💬 Optional Nits
View full reviewRisk: Low.
✅ All other checks passed🔒 Security: ✅ LGTMNo security concerns. The only upgrades kept away from the Worker are those to the exact DevTools path, and Sandbox origins are still forwarded. 📖 AGENTS.md: ✅ LGTMThe change follows the repository conventions: it has the 📋 Code Quality: ✅ LGTMThe DevTools path is matched on With the The tests use a real ⚡ Performance: ✅ LGTMNo performance concerns. 📝 Documentation: ✅ LGTMNo documentation changes are needed. The public API is unchanged. Reviewed commit: |
da7ec59 to
3ca16f5
Compare
Superseded by #16540, which places this fix directly on #16504 as requested.
While reordering, I pushed the rebased fix into this PR's old base branch (#16444) before changing its base. GitHub automatically marked this PR merged into that temporary branch and deleted its head. The
vite-plugin-v1target was never updated. GitHub rejects changing the base of a merged PR; #16540 preserves the same fix, tests and existing changeset in the correct position.Original description:
Backports #16519 to Vite plugin v1, stacked on #16444 and the existing v1 backports.
Vite DevTools upgrades at
/__devtools/__wsmust reach its own listener without being dispatched to the Worker. Replace the inherited write-count heuristic from #16504 with explicit routing, preserving Sandbox forwarding. This also fixes a legitimate Worker upgrade being rejected when an earlier pipelined HTTP response finishes during dispatch. The original source, socket regressions, and existing changeset apply directly; no dependency upgrade or additional changelog entry is needed.Validation: Node 22.22.1 / pnpm 10.33.0;
pnpm build,pnpm check,pnpm test:ci, and the WebSockets playground in dev and built preview.Note
This is a contribution from an AI agent: Codex, under the authorized v1 backport automation.