Implement client WebSocket.bufferedAmount#31761
Conversation
The client WebSocket.bufferedAmount getter was effectively hard-wired to 0: m_bufferedAmount was only ever written by a didUpdateBufferedAmount() that nothing called, and the update lines in the send paths were commented out. Bytes queued by send() to a peer that stopped reading were genuinely buffered in-process (RSS grows by the full payload) but bufferedAmount never reflected them, so send backpressure on a client WebSocket was unobservable. Expose the pending send-buffer size from the Rust client via Bun__WebSocketClient__getBufferedAmount / ...TLS... (the in-process send_buffer's readable length, plus any encrypted bytes still held by the proxy tunnel) and query it live from WebSocket::bufferedAmount() for the Client/ClientSSL connection kinds. This mirrors ServerWebSocket's getBufferedAmount(), which already queries the live buffer.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds live client bufferedAmount by summing the Rust send buffer and optional proxy-tunnel backlog, exports C‑ABI getters for TLS/non‑TLS, threads buffered snapshots into C++ close/fail callbacks, updates C++ bufferedAmount logic, and adds client tests exercising backlog and close/abort paths. ChangesWebSocket client bufferedAmount reporting
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/http_jsc/websocket_client.rs`:
- Around line 2096-2111: The current get_buffered_amount implementation reports
framed/encrypted wire bytes by calling send_buffer.readable_length() and
proxy_tunnel.buffered_amount(); instead introduce and use a dedicated plaintext
queued-byte counter (e.g., a field like queued_plain_bytes) that is incremented
in the send() enqueue path and decremented in the write/drain completion path
(and when proxied bytes are handed to the tunnel) and ensure proxy_tunnel
contributions are accounted only as the plaintext bytes handed off, then modify
get_buffered_amount to return this counter instead of the framed/encrypted
buffer lengths (update send(), drain/write completion, and any tunnel-handoff
code paths accordingly).
🪄 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: 5791622e-7ba5-4831-8a71-b8a4748ecdd5
📒 Files selected for processing (5)
src/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/WebSocketProxyTunnel.rssrc/jsc/bindings/headers.hsrc/jsc/bindings/webcore/WebSocket.cpptest/js/web/websocket/websocket-buffered-amount.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Per the WHATWG spec, bufferedAmount does not reset to zero once the connection closes; it only increases with subsequent send() calls. The previous change made bufferedAmount observable while OPEN, but close()/ terminate() tore down the connection (freeing the send buffer) and didClose() then reset m_bufferedAmount to 0, so a queued backlog snapped to 0 the moment close() was called. Snapshot the live buffered amount into m_bufferedAmount in close()/ terminate() before the connection is torn down, and stop didClose() from overwriting it (keep the larger value, since its unhandledBufferedAmount argument is always 0). Add a test asserting the backlog survives close().
The close()/terminate() snapshot covered graceful teardown, but the abrupt-close path (protocol error, timeout, write failure, peer half-close) routes through fail(), which dispatches the JS close event via did_abrupt_close() -> didFailWithErrorCode() *before* cancel() frees the send buffer. didFailWithErrorCode() nulled the connection without snapshotting, so bufferedAmount still reset to 0 in the onclose handler. A C++-side live query there would form a whole-struct borrow of the Rust client while fail() still holds &mut self (a Stacked Borrows conflict), so instead capture the backlog on the Rust side before did_abrupt_close() and thread it through the FFI. didFailWithErrorCode() keeps the larger of the passed value and m_bufferedAmount, which also makes the socket-close path (buffer already cleared -> 0) a no-op. Add a test that aborts the client via an illegal masked server frame and asserts bufferedAmount survives into the close handler.
The abrupt-close test read the backlog in onopen and compared it for exact equality with the value seen in the onclose handler. A few frames can legitimately drain from the send buffer between those two points, so the exact match was flaky (e.g. 261635688 vs 261930132). Assert the backlog stays large (not reset to 0) instead — which is the actual spec-conformance property and still fails when bufferedAmount is 0.
WebSocket::buffered_amount() summed the proxy tunnel's backlog via tunnel.as_ref().buffered_amount(), forming a whole-struct &WebSocketProxyTunnel. But buffered_amount() is now called from fail(), which is reachable from inside the tunnel's SSL-wrapper callbacks (wss:// through an HTTP proxy, on an abrupt close during the connected phase): there a &mut SslWrapper over the tunnel's wrapper field is live, and a whole-struct &Self overlaps it — UB under Stacked Borrows, which the module's Aliasing model doc explicitly forbids for callbacks. Make WebSocketProxyTunnel::buffered_amount take *const Self and project to write_buffer via addr_of!, matching the write_encrypted pattern, and call it through tunnel.as_ptr(). This keeps the tunnel contribution in the count without touching wrapper's bytes.
|
CI is green on the diff — the only red lane is `darwin-14-x64-test-bun`, failing on `test/js/bun/terminal/terminal.test.ts > creates subprocess with terminal attached` (a PTY test that times out at 90s on loaded macOS runners; unrelated to this PR and previously flaked on the same lane in build #60150). The earlier `serve-body-leak` memory-threshold blip on `debian-13-x64-asan` passed on retry — that lane is green. All WebSocket coverage passes, including `debian-13-x64-asan` (ASAN, where the tunnel-aliasing fix matters most), `darwin-26-aarch64`, and every Linux/Windows lane. The new `websocket-buffered-amount` tests pass on all lanes. I have already used my one CI re-roll, so I am not pushing another retrigger. This needs a maintainer to re-run the macOS lane / merge past the unrelated PTY flake. |
…unnel write-fail Three more edge cases in the bufferedAmount implementation: 1. Server-initiated graceful close (peer sends a Close frame, client echoes it) reset bufferedAmount to 0: send_close_with_body() calls clear_data() before dispatch_close(), and WebSocket__didClose hardcoded didClose(0, ...). Snapshot the backlog before clear_data() and thread it through dispatch_close -> did_close -> WebSocket__didClose -> didClose as a separate bufferedAmountSnapshot param (NOT unhandledBufferedAmount, so wasClean stays true for a cleanly-completed close handshake). 2. get_buffered_amount() formed a whole-struct &*this on the WebSocket<SSL> allocation, but the C++ bufferedAmount getter runs re-entrantly while a &mut Self is live (JS reads ws.bufferedAmount inside an onmessage handler dispatched from dispatch_data(&mut self)) — UB under Stacked Borrows. Make buffered_amount take *const Self and project to send_buffer/proxy_tunnel via addr_of!, same shape as the f704b19 tunnel fix. 3. On the tunnel FailedToWrite path, send_buffer_out() drop()ed the swapped-out backlog before terminate() -> fail(), so fail()'s snapshot read 0. Restore self.send_buffer = buf first; clear_data() frees it immediately after. Adds a server-initiated-close test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/http_jsc/websocket_client.rs`:
- Around line 1650-1656: The close-path is still clearing the send backlog
before we snapshot it, so did_abrupt_close sees bufferedAmount == 0; update the
flow so the queued byte count is captured before clear_data() is invoked: call
Self::buffered_amount(self) (or otherwise read the internal send-queue length)
prior to handle_close()/clear_data(), and pass that captured value into
CppWebSocket::opaque_ref(...).did_abrupt_close(code, buffered) (or modify
dispatch_abrupt_close to accept the buffered value) so the abrupt-close event
reports the true bufferedAmount; relevant symbols: Self::buffered_amount,
handle_close, clear_data, dispatch_abrupt_close, CppWebSocket::opaque_ref,
did_abrupt_close.
🪄 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: ae7ac404-3c38-4485-b966-744e0ae8211a
📒 Files selected for processing (7)
src/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/CppWebSocket.rssrc/http_jsc/websocket_client/WebSocketProxyTunnel.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/jsc/bindings/webcore/WebSocket.cppsrc/jsc/bindings/webcore/WebSocket.htest/js/web/websocket/websocket-buffered-amount.test.ts
- handle_close() (peer RST / terminal socket-close callback) cleared the send buffer before dispatch_abrupt_close(), so bufferedAmount reset to 0 in the close event. Snapshot the backlog before clear_data() and pass it through a new Option override on dispatch_abrupt_close(); other callers pass None and keep snapshotting the live buffer. - Split the buffered_amount() unsafe blocks so each has its own SAFETY comment (clippy::undocumented_unsafe_blocks), fixing the cargo clippy lane. - Add a raw-socket-reset test.
close() queues an ~8-byte close frame into the send buffer before the getBufferedAmount snapshot, so afterClose is beforeClose + ~8, not exactly beforeClose. The exact-equality assertion flaked off-by-8 on macOS/Windows (it happened to coalesce on Linux loopback). Assert afterClose >= beforeClose, which matches the spec (bufferedAmount does not reset on close and only increases) and still fails if it were reset to 0.
sock.destroy() on a drained socket sends FIN, not RST, and which client callback fires (handle_close vs handle_end -> fail) is platform-dependent. Rename the test and soften the comments to describe the abrupt close accurately rather than claiming it deterministically hits the socket-close callback path.
|
Diff is green — the only red lane is `darwin-14-x64-test-bun`, failing on unrelated third-party/flaky tests (`grpc-js/test-tonic` — the unzipper-downloaded test server failed to start with FILE_ENDED; plus `bake/serve-plugins-dev-server` and `serve-body-leak` which already passed on retry on the ASAN lane). None of these touch the WebSocket client code this PR changes. All WebSocket coverage passes, including the new `websocket-buffered-amount` tests on every lane (debian-13-x64-asan, darwin-26-aarch64, all Windows, alpine, ubuntu). The earlier macOS/Windows flake in my own test was fixed in c758473 (relaxed the close() assertion to `>=`, since close() queues an ~8-byte close frame). I have already used my one CI re-roll (83525d3), so per policy I am not pushing another retrigger. This needs a maintainer to re-run the `darwin-14` lane / merge past the unrelated third-party flake. All 12 review threads (CodeRabbit + claude findings) are resolved. |
On the tunnel write-failure path, SslWrapper::write_data can fire on_close -> fail() synchronously inside the write (fatal SSL error), before the send_buffer is restored, so that bufferedAmount snapshot reads 0. Closing it would require aliasing UB or a per-flush copy, and 0 in that narrow fatal-error window is no worse than the pre-feature behavior, so document it as an intentional limitation rather than regress the hot path.
|
Still green on the diff. Latest build (#60169) failed only on `darwin-14-x64-test-bun`, and every failing test there is unrelated flake on a different subsystem — `cli/install/migration/complex-workspace`, `cli/update_interactive_install` (package-manager/registry), `http/serve-body-leak` (ASAN memory threshold), and `jsc-stress/simd-baseline` — all tagged flaky/passed-on-retry. My last commit was a comment-only doc change (no logic), so nothing here can be caused by this PR. The WebSocket `bufferedAmount` tests pass on every lane that finished (debian-13-x64-asan, all Windows lanes, etc.). I have already used my one CI re-roll (83525d3); per policy I am not pushing another retrigger. All 13 review threads are resolved — this just needs a maintainer to merge past the unrelated `darwin-14` flakes. |
Drop the leftover 'Reporting the data as' fragment so the sentence reads as one clause. Comment only.
|
Still green on the diff. Build #60171 flagged `debian-13-x64-asan-test-bun`, failing only on unrelated flaky tests — `cli/install/bunx` ("package that requires node 24", an npm-download test) and `node/http/node-http-connect` (tagged flaky) — neither of which touches WebSocket code; that lane is now passing on retry. My last two commits were comment-only (doc wording), so nothing here can be caused by this PR. The WebSocket `bufferedAmount` tests pass on every lane that finished. I already used my one CI re-roll (83525d3); per policy I am not pushing another. All 14 review threads (CodeRabbit + claude) are resolved and CodeRabbit’s latest review found no actionable comments with all pre-merge checks green. This just needs a maintainer to merge. |
|
Diff is green; CI build #60171 hit a broad flake wave across many unrelated subsystems — the failing tests are `cli/install/bunx`, `cli/run/no-orphans`, `bun/cron/in-process-cron`, `bun/spawn/spawn-stdin-readable-stream`, `node/http/node-http-connect`, `web/fetch/fetch`, `web/streams/streams-leak`, and `napi/napi`, all tagged flaky/warning. None touch WebSocket code, and the spread across unrelated areas (install, cron, spawn, fetch, napi, …) is characteristic of a CI-infra/registry hiccup, not this PR. My last two commits were comment-only (doc wording), so nothing here can be caused by this change. The WebSocket `bufferedAmount` tests are not among the failures and pass on every lane. I already used my one CI re-roll (83525d3); per policy I am not pushing another retrigger. All 14 review threads are resolved and CodeRabbit’s latest review found no actionable comments with all pre-merge checks green. This needs a maintainer to re-run CI / merge past the unrelated flake wave. |
1 participant
Fixes #31760
Problem
The client
WebSocket.bufferedAmountalways returns0, so send backpressure on a clientWebSocketis unobservable. Bytes queued bysend()to a peer that has stopped reading are genuinely buffered in-process (RSS climbs by the full payload), butbufferedAmountnever reflects it — a peer that stops reading causes unbounded in-process buffering with no observable signal.ServerWebSocket.getBufferedAmount()already works correctly; this was specific to the clientWebSocketclass.Cause
WebSocket::bufferedAmount()returnedsaturateAdd(m_bufferedAmount, m_bufferedAmountAfterClose), but for an OPEN connection both stay0:m_bufferedAmountwas only assigned bydidUpdateBufferedAmount(), which nothing ever called, and the lines in the send paths that would have refreshed it from the connection were commented out.m_bufferedAmountAfterCloseonly accumulates whensend()is called after the socket is already CLOSING/CLOSED.The Rust client never exported a buffered-amount accessor (neither did the original Zig reference), so the value was genuinely unimplemented.
Fix
Bun__WebSocketClient__getBufferedAmount/Bun__WebSocketClientTLS__getBufferedAmount(Rust FFI) returning the in-processsend_bufferreadable length — the bytes of fully-framed messages queued but not yet written to the socket — plus any encrypted bytes still held by the proxy tunnel.WebSocket::bufferedAmount()for theClient/ClientSSLconnection kinds, clamping tounsigned. After close the connection is gone, so the existingm_bufferedAmountfallback (set bydidClose()) is preserved.This mirrors
ServerWebSocket::get_buffered_amount, which already queries the live buffer.Verification
Issue repro (raw TCP server completes the WS handshake then
pause()s; client sends 250 MiB):i.e. ~247 MiB queued, matching Node/undici's ~249 MiB instead of
0.Test in
test/js/web/websocket/websocket-buffered-amount.test.tsasserts the baseline is0with nothing queued and grows past a single 64 KiB frame once the peer stops reading. It fails before the change (Received: 0) and passes after. Existingwebsocket-clientandwebsocket-blobsuites still pass.