Skip to content

[miniflare] Close WebSocket servers during dispose#13521

Closed
petebacondarwin wants to merge 1 commit intomainfrom
fix/close-websocket-servers-dispose
Closed

[miniflare] Close WebSocket servers during dispose#13521
petebacondarwin wants to merge 1 commit intomainfrom
fix/close-websocket-servers-dispose

Conversation

@petebacondarwin
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin commented Apr 15, 2026

The #liveReloadServer and #webSocketServer (both ws.WebSocketServer instances created with noServer: true) were never explicitly closed during Miniflare.dispose(). Stopping the loopback HTTP server doesn't cascade to noServer WebSocket servers, so connected WebSocket clients (e.g., browser live reload, devtools) would keep sockets alive, preventing the Node.js event loop from draining and contributing to wrangler dev hanging on Ctrl-C.

This PR terminates all connected WebSocket clients and closes both servers during dispose(), placed after #stopLoopbackServer().


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because: straightforward cleanup addition; existing test suite confirms no regression
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: internal implementation fix with no user-facing API change

Open with Devin

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 15, 2026

🦋 Changeset detected

Latest commit: 46bacd0

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

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

@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Apr 15, 2026
@workers-devprod workers-devprod requested review from a team and emily-shen and removed request for a team April 15, 2026 15:30
@workers-devprod
Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • @cloudflare/wrangler
Show detailed file reviewers
  • packages/miniflare/src/index.ts: [@cloudflare/wrangler]

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 15, 2026

I've reviewed the full context. Let me verify one more thing — compare the #handleReload pattern (which uses ws.close()) with the dispose() pattern (which uses ws.terminate()), to understand if terminate() is the right choice:

  • #handleReload() at line 1108 uses ws.close(1012, "Service Restart") — graceful close with a status code
  • dispose() uses ws.terminate() — hard termination

This is actually the correct approach. During dispose, we want immediate teardown. ws.close() initiates a graceful close handshake which would require waiting for the peer to respond, while ws.terminate() destroys the socket immediately. Since we're shutting down, terminate() is appropriate.

The change is clean, correct, and well-placed:

  1. Both servers are noServer: true, so they are correctly not torn down by #stopLoopbackServer().
  2. terminate() is the right call during dispose (vs close() during reload).
  3. The .close() on the server itself prevents new connections and cleans up server state.
  4. Placement after #stopLoopbackServer() is logical — no new upgrades can come in.
  5. The changeset is correctly marked as patch for miniflare.

LGTM

github run

@github-actions
Copy link
Copy Markdown
Contributor

✅ All changesets look good

Summary of reviewed changesets:

  1. fix-close-websocket-servers-dispose.md (miniflare: patch)

    • Bug fix for WebSocket server cleanup during dispose
    • Clear description explaining the issue and solution
  2. c3-frameworks-update-12714.md (create-cloudflare: patch)

    • Dependency update for create-cloudflare
    • Well-formatted with table showing version changes
  3. nasty-years-taste.md (wrangler: patch)

    • Improvement to R2 CORS validation and error messaging
    • Concise description of the change
  4. fix-ipv6-localhost-spin.md (miniflare: patch)

    • Bug fix for IPv6 localhost binding issues
    • Excellent technical explanation of the problem and solution

All changesets:

  • Use appropriate version types (patch for bug fixes/improvements)
  • Have meaningful descriptions
  • Contain no prohibited h1/h2/h3 headers

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 15, 2026

create-cloudflare

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

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13521

@cloudflare/workers-editor-shared

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

wrangler

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

commit: 46bacd0

@petebacondarwin
Copy link
Copy Markdown
Contributor Author

Superseded by #13515 which includes this fix (closing #liveReloadServer and #webSocketServer in dispose()).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants