Bun.serve websocket: deliver queued publish() messages when unsubscribing from last topic - #32852
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Updated 4:01 PM PT - Jun 28th, 2026
❌ @robobun, your commit 6c6b9ab has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32852That installs a local version of the PR into your bun-32852 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughIn WebSocket publish/unsubscribe fix
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bun-types/serve.d.ts`:
- Around line 988-995: Update the JSDoc for server.publish() in the serve.d.ts
declaration so it explicitly says the return value is the queued-byte count for
each current subscriber, not an aggregate total across the topic. Keep the
existing note about 0 meaning no subscribers and the batching/backpressure
behavior, but revise the phrasing near the publish documentation to remove any
ambiguity about total bytes.
- Around line 146-149: Update the `ws.publish*()` return documentation in
`serve.d.ts` so it no longer assumes `publishToSelf: false`; in the
`publish`/`publishText`/related doc blocks, rewrite the `0` return description
in terms of “no subscribed socket will receive the message” rather than “no
other subscribers,” since `publishToSelf: true` can make a sole subscriber
receive a non-zero queued-byte result.
In `@test/js/bun/websocket/websocket-server.test.ts`:
- Around line 423-430: The websocket test helper collect() is resolving too
early for client b by using received.length === 5 instead of waiting for the
sentinel message. Update the logic in collect() so the b path only resolves when
e.data === "done" (while keeping a’s ready flow intact), using the existing
ready and done PromiseWithResolvers hooks to make the assertion deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 71431670-8160-4307-aae5-31fa98309efe
📒 Files selected for processing (4)
docs/runtime/http/websockets.mdxpackages/bun-types/serve.d.tspackages/bun-uws/src/WebSocket.htest/js/bun/websocket/websocket-server.test.ts
There was a problem hiding this comment.
Both earlier comments are addressed and I didn't find anything new, but this changes message-delivery semantics in the vendored uWS C++ (WebSocket::unsubscribe now drains before freeing) and touches packages/bun-types/ which has a CODEOWNER, so I'd like a human to sign off.
Extended reasoning...
Overview
Four files: a one-line drain() call inserted before freeSubscriber() in packages/bun-uws/src/WebSocket.h (the actual fix), JSDoc rewrites for the publish* return values in packages/bun-types/serve.d.ts, a one-character doc fix for the backpressureLimit default in docs/runtime/http/websockets.mdx, and a ~70-line regression test in test/js/bun/websocket/websocket-server.test.ts. My two prior inline comments (the inaccurate "or this socket is closed" clause in the JSDoc and the order-dependent test predicate) were both addressed in dff7581 and cd18687, and the bug-hunter pass on the current head found nothing.
Security risks
None apparent. No auth, crypto, or user-controlled-input parsing is touched. The C++ change reuses the existing TopicTree::drain(Subscriber*) path that WebSocket::send() already invokes from the same handler contexts, and drain() no-ops when needsDrainage() is false, so the steady-state cost is a single boolean check.
Level of scrutiny
Medium-high. Although the C++ delta is tiny and follows an established pattern, it is a behavior change in the vendored uWebSockets fork on a hot pub/sub path: messages that were previously silently dropped on last-topic unsubscribe are now flushed synchronously inside unsubscribe(), which subtly shifts delivery ordering relative to the end-of-tick postCb drain. Separately, packages/bun-types/ is CODEOWNER-protected, so the type-doc rewrite should get owner eyes regardless.
Other factors
The PR is well-motivated (clear repro, root cause traced to freeSubscriber unlinking without draining, the other two freeSubscriber call sites analysed), the regression test is deterministic after the per-client isDone predicate fix, and all CodeRabbit/claude inline threads are resolved. CI on cd18687 shows failures in Build #65520 — likely pre-existing flakes given the latest commit only edited a JSDoc string, but worth a glance before merge.
|
Status: rebased onto #32889 fixed the
Re-verified post-rebase: the test fails with CI (#66477, final)284 jobs passed, 2 failed, both on macOS lanes with no overlap with this diff:
The one |
…ubscriber on last-topic unsubscribe When a ServerWebSocket unsubscribed from its last topic in the same event loop tick as a publish() that had queued messages for it, those messages were silently dropped. TopicTree::freeSubscriber unlinks the subscriber from the drainable list without draining, so any messages that publish() had already accepted never reached the socket. Drain the subscriber before freeing it in WebSocket::unsubscribe. The other two freeSubscriber callers do not need this: end() sends a close frame via send() which drains first, and the TCP close path has no live socket left to write to. Also fix the documented backpressureLimit default in websockets.mdx (16 MB, matching WebSocketServerContext.rs and serve.d.ts; it said 1 MB).
fef1599 to
6c6b9ab
Compare
Problem
publish()queues messages per subscriber in the uWS TopicTree; they are flushed at the end of the event loop tick. When aServerWebSocketcallsunsubscribe()on its last topic in the same tick as apublish(), the subscriber object is freed with its undrained queue still attached, so messages thatpublish()had already accepted (and returned a non-zero byte count for) are silently discarded.Keeping any other topic subscription on the same socket makes the messages arrive, which shows the loss is a subscriber-lifetime artifact rather than a delivery policy.
Cause
WebSocket::unsubscribe()callsTopicTree::freeSubscriber()when the last topic is removed.freeSubscriber()unlinks the subscriber from the drainable list without draining it and then deletes it, dropping any messages still referenced bymessageIndices[].Fix
Drain the subscriber before freeing it in
WebSocket::unsubscribe(). The other twofreeSubscribercall sites are unaffected:end()sends a close frame viasend(), which drains first; the TCPonClosepath has no live socket to write to.Also corrects the
backpressureLimitdefault indocs/runtime/http/websockets.mdx(16 MB, matchingWebSocketServerContext.rsand the type docs; it said 1 MB).Rebase note
This PR originally also rewrote the
publish()return-value JSDoc inpackages/bun-types/serve.d.tsto describe the (then) real behavior: the documented0/-1backpressure returns were unreachable andpublish()always returned the byte count.That is no longer the case. #32889 landed on
mainand fixes the underlying contract for real:publish()now aggregates the per-subscriberSendStatusout of uWS and returns0/-1on backpressure, and it updated the JSDoc accordingly. Theserve.d.tschanges here described the pre-#32889 behavior and are superseded, so they were dropped during the rebase. The remaining diff is the unsubscribe lifetime fix, its regression test, and thebackpressureLimitdoc correction, none of which overlap with #32889.Verification
Re-verified against
mainafter the rebase onto #32889 and later.