Skip to content

fix(cloudflare): send close frame#177

Merged
pi0 merged 1 commit intoh3js:mainfrom
jclab-joseph:fix-cloudflare-ws
Jan 19, 2026
Merged

fix(cloudflare): send close frame#177
pi0 merged 1 commit intoh3js:mainfrom
jclab-joseph:fix-cloudflare-ws

Conversation

@jclab-joseph
Copy link
Contributor

@jclab-joseph jclab-joseph commented Jan 7, 2026

Fixes an issue where the readyState is stuck at 2 (closing) when websocket.close is called on the client (browser).

See

Summary by CodeRabbit

  • Bug Fixes
    • Improved server resource cleanup when WebSocket connections are terminated to ensure proper shutdown handling.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

The Cloudflare adapter's WebSocket close event handler now explicitly closes the server by calling server.close() when a "close" event is emitted, ensuring proper cleanup after peer connection termination.

Changes

Cohort / File(s) Summary
WebSocket Close Handler
src/adapters/cloudflare.ts
Added explicit server.close() call within the WebSocket "close" event handler to ensure proper server cleanup after peer connections terminate.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A server closed with care so neat,
When peers depart, the exit's sweet,
No lingering threads left in the night,
Clean shutdown happens—all just right!
Cloudflare's path now fully formed,
Where proper cleanup is the norm. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'send close frame' is related to the changeset but doesn't capture the main issue being fixed—that WebSocket readyState gets stuck in CLOSING state.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 440a088 and 95e911b.

📒 Files selected for processing (1)
  • src/adapters/cloudflare.ts
🔇 Additional comments (1)
src/adapters/cloudflare.ts (1)

133-137: Good fix for the Cloudflare close handshake issue.

The explicit server.close() call properly completes the WebSocket close handshake, ensuring the readyState transitions from CLOSING (2) to CLOSED (3). This is necessary in Cloudflare's fallback path when using WebSocketPair.

Consider adding a brief comment explaining this workaround:

Suggested comment
 server.addEventListener("close", (event) => {
   peers.delete(peer);
   hooks.callHook("close", peer, event);
+  // Explicitly close server side to complete close handshake (workerd issue)
   server.close();
 });

Note: The Durable Object close path (handleDurableClose, lines 182-188) doesn't include a similar explicit close call because it uses a different event system where the close event is only invoked after the close handshake is already complete.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thanks!

@pi0 pi0 merged commit aa94192 into h3js:main Jan 19, 2026
3 checks passed
@codecov
Copy link

codecov bot commented Jan 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants