[petebacondarwin]

Backports #15659 to Vite plugin v1.

V1's upgrade handler also destroys the socket when its Worker does not route a WebSocket, even if another listener already completed the handshake. Capture the socket write count before other listeners run and leave a claimed socket alone, including asynchronous error paths. Unclaimed upgrades still close normally.

Retain the original real-socket regressions for an owning DevTools-style listener and an unclaimed route. V1 keeps its existing, equivalent response-cookie forwarding implementation; main's behavior-equivalent cleanup #16488 is not a prerequisite and is not copied just to maintain mapping.

Stacked on #16503 (export-cache cleanup). This PR contains only the useful remaining WebSocket ownership fix, its plugin tests, and original patch changeset.

Validation: Node 22.22.1 / pnpm 10.33.0; pnpm build, pnpm check (including types), pnpm test:ci, pnpm test:fixtures, and 4 WebSocket playground tests in each of dev and preview pass.

---

• Tests
  • Tests included/updated
  • Automated tests not possible - manual testing has been completed as follows:
  • Additional testing not necessary because:
• Public documentation
  • Cloudflare docs PR(s):
  • Documentation not necessary because: fixes coexistence of existing upgrade listeners without adding configuration.

Note

This is a contribution from an AI agent: Codex.

---

[changeset-bot]

🦋 Changeset detected

Latest commit: 15f30ec

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@cloudflare/vite-plugin	Patch

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

[petebacondarwin]

@dedsec-terminal (recorded author of #15659) and @petebacondarwin (verified human ant-core approver), could you please check this Vite plugin v1 backport and approve it if it looks good?

V1 has the same shared upgrade-listener ownership bug. The original fix and both real-socket regressions apply directly. Main's equivalent cookie-forwarding refactor #16488 is unnecessary: v1 keeps its existing response-cookie handling, while earlier #16501 supplies the useful incoming-header fix. Build, checks/types, all 219 plugin tests, retained fixtures and 4 WebSocket tests in each of dev and preview pass.

[workers-devprod]

Codeowners approval required for this PR:

• ✅ @cloudflare/wrangler

[clanky-the-clanker]

💬 Approved with comments — The ownership check is correct for the DevTools case and unclaimed upgrades still close. One WARNING about test coverage for two new branches (no production impact) and one SUGGESTION about an unclosed Worker WebSocket in an edge case.

Findings: 1 warning · 1 suggestion

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #16504.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (1)

### [Code Quality] Two new branches have no tests: the claimed-socket early return when the Worker returns a WebSocket, and the delayed `setImmediate` teardown in the catch path (claimed vs. unclaimed). This is a test-coverage gap, not a runtime defect.
- **File:** packages/vite-plugin-cloudflare/src/websockets.ts:135

## SUGGESTION (1)

### [Code Quality] When the socket is already claimed and the Worker returned a WebSocket, `workerWebSocket` is never closed and stays open until Miniflare is disposed. Consider accepting and closing it before returning.
- **File:** packages/vite-plugin-cloudflare/src/websockets.ts:105

---

📋 Code Quality: ⚠️ 1 warning · 1 suggestion

The ownership guard is correct for listeners that write the handshake synchronously. Two new branches have no tests, and in one edge case the Worker's WebSocket is left open.

Recommendation: 📝 Minor Comments

• [WARNING] Two new branches have no tests: the claimed-socket early return when the Worker returns a WebSocket, and the delayed setImmediate teardown in the catch path (claimed vs. unclaimed). This is a test-coverage gap, not a runtime defect. (packages/vite-plugin-cloudflare/src/websockets.ts:135)
• [SUGGESTION] When the socket is already claimed and the Worker returned a WebSocket, workerWebSocket is never closed and stays open until Miniflare is disposed. Consider accepting and closing it before returning. (packages/vite-plugin-cloudflare/src/websockets.ts:105)

View full review

• Two new branches have no tests (packages/vite-plugin-cloudflare/src/websockets.ts:103-107, :132-139).

  • The new tests only cover the !workerWebSocket branch, once claimed and once unclaimed.
  • The early return when the Worker does return a WebSocket on an already-claimed socket has no test.
  • The catch path's teardown now runs later, inside setImmediate, and is skipped for a claimed socket. That also has no test.
  • The PR description specifically says asynchronous error paths are covered. Suggested tests: make dispatchFetch reject after another listener has written the 101 and assert the socket is not destroyed; make it reject on an unclaimed socket and assert the socket closes.

• The Worker's WebSocket is left open (websockets.ts:105-107).

  • When isClaimed() is true and response.webSocket exists, the handler returns without closing workerWebSocket. The Worker side stays open until Miniflare is disposed.
  • This only happens when another listener claims a path that the Worker also handles as a WebSocket. Consider accepting and then closing workerWebSocket before returning.

• Limitation of the write-count check (for awareness, no change needed).

  • The check only sees a handshake the other listener has already written by the time dispatchFetch settles.
  • A listener that claims the socket but writes its response later, after its own async work, would still have the socket destroyed.
  • This is acceptable for the DevTools case, which writes synchronously.

• Switch to prependListener. I found no problem. The baseline is read synchronously before any await, and Vite HMR sockets still return early.

---

✅ All other checks passed

🔒 Security: ✅ LGTM

No security issues found in the WebSocket upgrade ownership change.

---

📖 AGENTS.md: ✅ LGTM

No AGENTS.md updates needed.

---

⚡ Performance: ✅ LGTM

No performance concerns.

---

📝 Documentation: ✅ LGTM

The changeset and code comments describe the fix clearly.

---

_{Reviewed commit: 15f30eca}

[devin-ai-integration]

Devin Review found 2 potential issues.

[pkg-pr-new]

npm i https://pkg.pr.new/@cloudflare/vite-plugin@16504

commit: 15f30ec

[workers-devprod]

Codeowners reviews satisfied

[dedsec-terminal]

The pipelining issue in the review is real — I reproduced it with a pipelined GET /page + GET /ws: the 200 bumps bytesWritten and the Worker's 101 is dropped. The fix is to detect ownership from the socket's data listener instead of the write count:

- const bytesWrittenAtStart = (socket as unknown as Socket).bytesWritten ?? 0;
- const isClaimed = () => socket.destroyed || ((socket as unknown as Socket).bytesWritten ?? 0) > bytesWrittenAtStart;
+ const isClaimed = () => socket.destroyed || socket.listenerCount("data") > 0;

I opened #16516 with the change and a regression test, but it was auto-closed by the collaborator policy. The test is still visible there if useful. Happy to re-review once the backport has the updated check.