Repository navigation
fix(vite-plugin): don't mistake pipelined HTTP responses for claimed WebSockets - #16516
dedsec-terminal wants to merge 1 commit into
Conversation
The write-count check treated an in-flight HTTP response on a pipelined keep-alive connection as another upgrade listener's handshake, dropping a valid Worker WebSocket. Detect ownership from the socket's data listener instead, which is only attached once a listener takes over the connection. Add a regression test that pipelines a normal request and an upgrade on one connection while the Worker's 101 response is pending. Signed-off-by: dedsec-terminal <dedsec.terminal@gmail.com>
|
|
Hi @dedsec-terminal. Thanks for taking the time to contribute to Workers SDK. This PR has been automatically closed because you are not currently a registered (vouched) collaborator for this repository. AI tools now let our team generate proposed code changes quickly. Reviewing changes, checking that they solve the right problem, and maintaining code quality over time still take substantial care and effort. We therefore limit PRs to registered collaborators so we can focus that effort where it is most useful. For the majority of external contributors, the most effective way to help is through issues: describing a problem or use case, providing a minimal reproduction, investigating its cause, or helping us verify a fix. Those contributions help us understand what needs to change and are very welcome. We will take an initial look at this PR and decide whether it is something we want to invest time in reviewing and maintaining. We may reopen it if we decide to proceed, but we cannot guarantee that it will be reopened or receive a full review. |
|
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}
| const isClaimed = () => | ||
| socket.destroyed || | ||
| ((socket as unknown as Socket).bytesWritten ?? 0) > bytesWrittenAtStart; | ||
| socket.destroyed || socket.listenerCount("data") > 0; |
There was a problem hiding this comment.
🔴 Raw upgrade owners lose their sockets
When another listener writes an upgrade response but uses readable instead of data, isClaimed() reports false. The plugin destroys that connection or sends a second handshake, depending on the Worker response.
Learn more
The HTTP server sends each upgrade event to every registered listener. A listener can own the connection by writing an HTTP upgrade response and consuming subsequent input through readable, without ever installing a data listener. The new check then treats that socket as unclaimed in both the no-WebSocket and WebSocket branches, destroying an owned connection or issuing a second handshake. The previous write-count check caught these written handshakes but incorrectly counted pending pipelined HTTP responses.
Example: A Vite plugin writes HTTP/1.1 101 Switching Protocols and registers socket.on('readable', ...) for its protocol. If the Worker returns 404, this handler destroys the plugin's upgraded socket; if the Worker returns a WebSocket, it writes another 101.
Recommended fix: Track ownership using a mechanism that can distinguish the upgrade listener's response from preceding pipelined HTTP responses, without relying exclusively on data listeners. Test an owner that writes its handshake and reads via readable alongside the pipelined-response case.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const isClaimed = () => | ||
| socket.destroyed || | ||
| ((socket as unknown as Socket).bytesWritten ?? 0) > bytesWrittenAtStart; | ||
| socket.destroyed || socket.listenerCount("data") > 0; |
There was a problem hiding this comment.
🟡 Passive listeners block Worker WebSockets
When another upgrade listener observes socket data without answering, isClaimed() reports ownership. A valid Worker WebSocket then receives no handshake and remains pending.
Learn more
An upgrade listener can register a data listener to inspect incoming bytes without sending an HTTP response or claiming the socket. This check cannot distinguish that observer from a WebSocket owner. When the Worker supplies a WebSocket, the existing isClaimed() branch returns without upgrading or destroying the connection, leaving the client waiting. The former write-count check allowed the Worker handshake because no competing response was written.
Example: A Vite extension attaches socket.on('data', inspectBytes) during every upgrade event, but handles only /inspector. A request to /worker-ws reaches a Worker returning status 101, yet this plugin skips its handshake because the inspector callback is still registered.
Recommended fix: Base ownership on evidence of another handler taking responsibility for this specific upgrade, not on the mere presence of a socket reader. Preserve the case where an ordinary HTTP response to an earlier pipelined request is still in flight.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const isClaimed = () => | ||
| socket.destroyed || | ||
| ((socket as unknown as Socket).bytesWritten ?? 0) > bytesWrittenAtStart; | ||
| socket.destroyed || socket.listenerCount("data") > 0; |
There was a problem hiding this comment.
🔍 Changeset describes outdated ownership behavior
The existing patch changeset says a written response protects the socket. The new check uses data listeners instead; update the release note to match.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
clanker review |
|
💬 Approved with comments — The listener-count ownership check fixes the pipelined-response false positive, and the regression test covers that case. The only follow-ups are a stale code comment, stale changeset wording and one edge case worth knowing about. All are INFO. Findings: 3 info The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 🔧 Fix with your agentPaste the following into your coding agent to investigate and fix these issues. 📋 Code Quality: 💬 2 infoThe fix is correct and the regression test covers it. Two INFO notes: the changeset wording is now out of date, and one edge case is no longer detected. Recommendation: 💬 Optional Nits
View full review
Correctness: Node's HTTP server removes its own 📝 Documentation: 💬 1 infoA code comment still describes the write-count baseline this change removes. Recommendation: 💬 Optional Nits
View full review
Reviewed commit: |
@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: |
|
Actually, I think we cannot reliably guess the claiming owner of a websocket due to how Node.js websockets work. |
Follow-up to #15659 (refs #15654).
The ownership check added in #15659 compared
socket.bytesWrittenagainst a baseline captured when the upgrade handler started. On a keep-alive connection, an in-flight HTTP response from an earlier pipelined request can finish whiledispatchFetchis still running. Its bytes increasebytesWritten, so the handler treats the socket as claimed and drops a valid Worker WebSocket upgrade.wsattaches adatalistener when it takes ownership of the socket, while an ordinary HTTP response only writes bytes. Detect ownership from the listener count instead.The regression test pipelines
GET /pageandGET /wson one connection, delays the/pageresponse until after the upgrade handler starts, and asserts the Worker's101is delivered. It fails with the write-count check and passes with this change.The same write-count check exists in the v1 backport #16504, so it needs the same change.
All 13 websocket tests pass;
tsc --build,oxlint, andoxfmt --checkare clean.