[dedsec-terminal]

Fixes #15654

Node dispatches upgrade to every registered listener. The dev-server handler forwarded the request to the Worker, then destroyed the raw socket when the Worker returned no route — even if another listener (for example Vite DevTools at /__devtools/__ws) had already completed the handshake. Clients saw onopen followed by close code 1006.

This replaces the earlier ownership-inference implementation with a focused check:

• Prepend the plugin listener so it can snapshot socket.bytesWritten before other listeners run.
• On a Worker miss, destroy the socket only if no other listener has written a response since that snapshot. Otherwise leave it alone.
• Skip the second handleUpgrade if another listener claimed the socket while dispatchFetch was in flight.
• Defer teardown on dispatchFetch rejection one tick so another listener can claim the socket first. A sole unclaimed upgrade is still destroyed.

This deliberately does not try to identify delayed or one-shot owners.

Tests in src/__tests__/websockets.spec.ts pass 11/11, including the DevTools-style owner case and the sole-listener unrouted teardown. Changeset included.

---

• 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: this restores existing dev-server WebSocket behavior and adds no user-facing API or configuration.

[changeset-bot]

🦋 Changeset detected

Latest commit: 3ffcca1

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

[workers-devprod]

Codeowners approval required for this PR:

• @cloudflare/wrangler

Show detailed file reviewers

• .changeset/witty-pandas-serve.md: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/tests/websockets.spec.ts: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/websockets.ts: [@cloudflare/wrangler]

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[pkg-pr-new]

@cloudflare/autoconfig

npm i https://pkg.pr.new/@cloudflare/autoconfig@15659

@cloudflare/build-output-utils

npm i https://pkg.pr.new/@cloudflare/build-output-utils@15659

@cloudflare/codemods

npm i https://pkg.pr.new/@cloudflare/codemods@15659

@cloudflare/config

npm i https://pkg.pr.new/@cloudflare/config@15659

@cloudflare/containers-shared

npm i https://pkg.pr.new/@cloudflare/containers-shared@15659

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@15659

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@15659

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@15659

miniflare

npm i https://pkg.pr.new/miniflare@15659

@cloudflare/pages-functions

npm i https://pkg.pr.new/@cloudflare/pages-functions@15659

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@15659

@cloudflare/runtime-types

npm i https://pkg.pr.new/@cloudflare/runtime-types@15659

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@15659

@cloudflare/vite-plugin

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

@cloudflare/vitest-plugin

npm i https://pkg.pr.new/@cloudflare/vitest-plugin@15659

@cloudflare/workers-auth

npm i https://pkg.pr.new/@cloudflare/workers-auth@15659

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@15659

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@15659

wrangler

npm i https://pkg.pr.new/wrangler@15659

commit: 3ffcca1

[dedsec-terminal]

Hi @cjol — friendly bump, this is waiting on human review (review also requested from @workers-devprod via existing request). CI is still running on latest head \3aef22f; will confirm once green. Let me know if you need anything else from my side. Thanks!

[devin-ai-integration]

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 3 new potential issues.

[cjol]

@dedsec-terminal please stop adding automated commits if you would like the PR to be reviewed; each bump puts it to the back of the queue, no matter how minor.

[dedsec-terminal]

Apologies for the repeated updates — they came across as automated queue bumps. I was working through the review suggestions and pushed fixes as I addressed them, but I understand that each update moved the PR back in the queue. That wasn't my intention. I'm consolidating the remaining review fixes and local verification into one final update before pushing again.

[dedsec-terminal]

Consolidated update in 99ed62e27 addressing the review items:

1. Repeated setup listener cleanup: Stored the active upgradeListener reference in UpgradeServerPatch and remove it before prepending the new handler upon repeated handleWebSocket() setup. Added regression test replaces its upgrade listener when setup repeats.
2. Malformed upgrade teardown: Replaced indefinite socket retention on URL parsing failure with deferUnclaimedTeardown(), safely closing the socket after giving asynchronous ownership transitions a tick when no other listener can own it. Added regression test destroys malformed upgrades when no other listener can own them.
3. Patch scoping: Patch state remains in a GC-able WeakMap keyed by the server instance.

Local verification:

• @cloudflare/vite-plugin check:type: PASS (tsc --build)
• websockets.spec.ts: 21/21 PASS

[workers-devprod]

Codeowners approval required for this PR:

• @cloudflare/wrangler

Show detailed file reviewers

• .changeset/witty-pandas-serve.md: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/tests/websockets.spec.ts: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/websockets.ts: [@cloudflare/wrangler]

[workers-devprod]

Codeowners approval required for this PR:

• @cloudflare/wrangler

Show detailed file reviewers

• .changeset/witty-pandas-serve.md: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/tests/websockets.spec.ts: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/websockets.ts: [@cloudflare/wrangler]

[workers-devprod]

Codeowners approval required for this PR:

• @cloudflare/wrangler

Show detailed file reviewers

• .changeset/witty-pandas-serve.md: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/tests/websockets.spec.ts: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/websockets.ts: [@cloudflare/wrangler]

[petebacondarwin]

Thank you @dedsec-terminal for this excellent PR. I really appreciated the comments in each test that helped me understand exactly what was being tests in each case.

We've discussed this as a team and are concerned by the nature of having to guess/infer who the owner of the Websocket is.

Ideally there should be some mechanism in the Vite eco-system that plugins can use to register their interest in incoming sockets, but this doesn't appear to be likely.

Since the main problem, that triggered this issue is quite well defined, we think it would be better to provide a simple more focussed fix that just addresses that case (perhaps by looking at the URL path), and avoiding trying to solve all cases.

It might be worth keeping this PR as it is, but closing it, and replacing with the simpler approach.

[workers-devprod]

Codeowners approval required for this PR:

• @cloudflare/wrangler

Show detailed file reviewers

• .changeset/witty-pandas-serve.md: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/tests/websockets.spec.ts: [@cloudflare/wrangler]
• packages/vite-plugin-cloudflare/src/websockets.ts: [@cloudflare/wrangler]

[clanky-the-clanker]

💬 Approved with comments — The fix works for upgrade listeners that answer synchronously, and the pull request description says delayed owners are out of scope. The only findings are one WARNING about whether the new regression test can actually fail, and one SUGGESTION about a no-op line.

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 #15659.

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] The 'does not destroy a WebSocket another upgrade listener has already claimed' test checks client-side `socket.destroyed` after only one setImmediate, so it may pass even without the fix. Assert on a client close/end event after a short wait, or spy on the server socket's destroy, instead.
- **File:** packages/vite-plugin-cloudflare/src/__tests__/websockets.spec.ts:473

## SUGGESTION (1)

### [Code Quality] `workerResponseHeaders.delete(request)` in the `isClaimed()` early-return branch does nothing, because the only `set` for that request comes later (line 117).
- **File:** packages/vite-plugin-cloudflare/src/websockets.ts:107

---

📋 Code Quality: ⚠️ 1 warning · 1 suggestion

The fix looks correct. The main regression test may not fail without the fix, and one line does nothing.

Recommendation: 📝 Minor Comments

• [WARNING] The 'does not destroy a WebSocket another upgrade listener has already claimed' test checks client-side socket.destroyed after only one setImmediate, so it may pass even without the fix. Assert on a client close/end event after a short wait, or spy on the server socket's destroy, instead. (packages/vite-plugin-cloudflare/src/__tests__/websockets.spec.ts:473)
• [SUGGESTION] workerResponseHeaders.delete(request) in the isClaimed() early-return branch does nothing, because the only set for that request comes later (line 117). (packages/vite-plugin-cloudflare/src/websockets.ts:107)

View full review

The regression test may pass against the old code (packages/vite-plugin-cloudflare/src/__tests__/websockets.spec.ts:470-475)

• The test resolves the deferred dispatchFetch, waits one setImmediate, then checks that the client-side socket.destroyed === false.
• Without the fix, the server would call socket.destroy() in a microtask once the promise resolves. But the client socket only marks itself destroyed after its poll phase has handled the incoming FIN or close. One setImmediate turn is probably not enough for that, so the assertion could still pass against the old code.
• The HTTP/1.1 101 assertion also tells us nothing new: it was already true before the deferred resolved.
• Two ways to make the test meaningful:
  • Attach a close/end listener to the client and assert it has not fired after a short wait.
  • Capture the server-side socket in an upgrade listener, spy on its destroy, and assert it was not called.
• Please check that the test fails when the isClaimed() guard is removed.

No-op delete (packages/vite-plugin-cloudflare/src/websockets.ts:107)

• In the new isClaimed() early-return branch, workerResponseHeaders.delete(request) runs before the only workerResponseHeaders.set(request, …) (line 117). The map never has an entry for this request at that point, so the call does nothing and can be removed.

Noted, no change needed

• The bytesWritten baseline only catches listeners that write before the Worker responds. The pull request description says delayed or one-shot owners are intentionally not handled.

---

✅ All other checks passed

🔒 Security: ✅ LGTM

No security issues found in this change.

---

📖 AGENTS.md: ✅ LGTM

The change does not make any AGENTS.md guidance out of date.

---

⚡ Performance: ✅ LGTM

No performance issues found in this change.

---

📝 Documentation: ✅ LGTM

The changeset and the code comments describe the change accurately.

---

_{Reviewed commit: 2e5c30c8}

[dedsec-terminal]

Thanks for the steer, Pete! I have reworked this into a small focused fix: it no longer tries to infer who owns a socket, it just checks whether another listener has already written a response before deciding to close anything, and leaves the socket alone if it has. The timing-based tests are gone as well. Could you take another look when you have a moment?

[petebacondarwin]

clanker review

[petebacondarwin]

Can you address the two concerns in the AI review above: #15659 (comment)

Otherwise, this is looking promising.

[dedsec-terminal]

Addressed both findings in 9df5a7d:

• Removed the no-op workerResponseHeaders.delete(request) from the claimed-socket early return.
• The regression test now asserts on the server socket's destroy directly. I verified it fails when the isClaimed() guard is removed and passes with the guard in place. All 11 websockets tests pass.

[dedsec-terminal]

Checks is green on 3ffcca1. The Windows runtime partition hit an unrelated miniflare flake — EPERM opening .miniflare-startup.lock in test/storage-owner.spec.ts. Could that job be retried when convenient?