usockets: wait for the TLS spill at the HTTP close gates; drain a large https response after peer FIN - #35109
Conversation
…ttps response after peer FIN us_internal_ssl_write() seals plaintext into 16 KB TLS records and flushes them to the kernel in ~128 KB batches. A partial kernel write parks the remainder of the batch in loop_ssl_data->ssl_spill and returns the full plaintext count as written, so uWS's AsyncSocket::getBufferedAmount() (only AsyncSocketData::buffer) reports 0 while up to one batch of ciphertext is still in userspace. Every shouldCloseConnection() close gate in HttpResponse/HttpContext then fires early; us_internal_ssl_close(code=0) does one best-effort drain and frees the rest. Separately, HttpContext::onOpen only set allow_half_open for non-TLS node:http sockets, so a client FIN on a node:https connection made us_internal_ssl_on_end force-close the socket before the buffered response drained (#35034 fixed the plain-TCP case). - openssl.c/socket.c/libusockets.h: add us_socket_ssl_spill_pending(), the ciphertext bytes already sealed for this socket still waiting on a writable event. - AsyncSocket::getBufferedAmount(): add the spill for SSL sockets so the close-after-drain gates wait for it. - HttpContext::onOpen<IsNodeHttp=true>: set allow_half_open for TLS too. onEnd<IsNodeHttp=true>'s existing getBufferedAmount()/onWritable defer and onWritable's close gate are now accurate for both transports. Bun.serve (!IsNodeHttp) TLS half-close stays with #35088; with the spill counted in getBufferedAmount() that PR's onEnd defer is safe to un-gate from !SSL.
WalkthroughChangesTLS half-close and backpressure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:52 AM PT - Jul 22nd, 2026
❌ @robobun, your commit 41c2fbb has 4 failures in
🧪 To try this PR locally: bunx bun-pr 35109That installs a local version of the PR into your bun-35109 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two test-file nits, I traced every getBufferedAmount() caller for the semantic shift: the WebSocket drain-progress check (backpressure > getBufferedAmount() in WebSocketContext.h) still terminates because us_internal_ssl_on_writable drains the spill to 0 before dispatching, and the !SSL gate on the WebSocket zero-copy fast path (WebSocket.h:147) makes that caller unaffected. us_internal_ssl_on_end does honour allow_half_open on both the raw-FIN and close_notify paths, so dropping the !SSL guard in onOpen is wired through.
Extended reasoning...
The two inline findings are cosmetic (comment placement, dead test parameter). The native change itself — counting the loop's TLS spill in getBufferedAmount() and enabling allow_half_open for node:https — reads correctly to me, but it changes the semantics of a function every HTTPS/WSS close/drain gate in uWS keys on, so it's worth a human look rather than an auto-approve.
…w-closing a half-closed socket poll_cb (libuv.c) maps AFD DISCONNECT to the eof hint for a socket whose write side we already shut down. AFD reports DISCONNECT while the tail of the peer's stream is still queued in the kernel, but the Windows branch of loop.c's read loop only did one extra recv (the RST probe) before falling through to the is_shut_down raw-close, discarding the rest. POSIX already drains on eof; do the same on Windows. Also address review: move the https describe after the torn-write it() so the existing comment stays attached to its test, and drop the dead halfClose parameter.
![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 `@test/js/node/http/node-http-backpressure.test.ts`:
- Around line 235-258: Update halfCloseTlsRequestBodyBytes to propagate TLS
socket errors instead of swallowing them: register the close and error
completion listeners before awaiting secureConnect, reject the helper when the
socket emits error, and preserve the existing body/ended result on successful
close. Remove the empty socket error handler so transport failures cannot hang
or be hidden.
🪄 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: 6cd1c2f4-a17e-4e45-8679-c6378fef314b
📒 Files selected for processing (8)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/libusockets.hpackages/bun-usockets/src/loop.cpackages/bun-usockets/src/socket.cpackages/bun-uws/src/AsyncSocket.hpackages/bun-uws/src/HttpContext.htest/js/node/http/node-http-backpressure.test.ts
There was a problem hiding this comment.
Thanks for addressing the two earlier nits — the describe("https") block now sits after the torn-write test with its own comment, and the dead halfClose parameter is gone. No correctness issues found this pass, but this touches core usockets/uWS TLS lifecycle (getBufferedAmount() semantics for every SSL AsyncSocket caller including WebSocket maxBackpressure, allow_half_open for node:https, and a new Windows AFD eof-drain branch in the hot read loop), so it warrants a human look.
Checked: us_internal_ssl_spill_pending reads the same ssl_spill_len - ssl_spill_off invariant that ssl_drain_spill maintains and gates on ssl_spill_owner == s; the added spill (≤ one ~128 KB batch) is drained by us_internal_ssl_on_writable before user-level writable dispatch, so the == 0 close gates in HttpResponse/HttpContext/WebSocketContext still converge; the Windows continue mirrors the existing POSIX eof-drain and is bounded by recv() returning 0/WSAEWOULDBLOCK.
Extended reasoning...
Overview
The PR fixes truncated node:https responses when a client half-closes right after its request. It adds us_socket_ssl_spill_pending() (openssl.c/socket.c/libusockets.h/internal.h) exposing the loop-wide TLS ciphertext spill for a given socket, folds that into AsyncSocket<SSL>::getBufferedAmount(), drops the !SSL guard on allow_half_open in HttpContext::onOpen<IsNodeHttp>, and (in a follow-up commit) adds a Windows eof-drain continue in loop.c's read loop mirroring the POSIX branch. Tests: three new https half-close cases in node-http-backpressure.test.ts, each looped 5×.
Changes since my last pass
Both prior 🟡 nits were addressed: the describe("https") block was moved after the torn-write it() with its own separate comment (the original why-comment is back adjacent to its test), and halfCloseTlsRequestBodyBytes dropped the dead halfClose parameter/branch. Commit f3fa327 additionally added the Windows loop.c eof-drain.
Security risks
None identified. The spill accessor is read-only over already-trusted loop state; allow_half_open for TLS node:http sockets defers close (bounded by the existing idle timeout and the onWritable zero-progress-after-FIN close), not an unbounded resource hold.
Level of scrutiny
High. getBufferedAmount() is called from every shouldCloseConnection() gate in HttpResponse/HttpContext, from WebSocketContext's drain/shutdown logic, and from WebSocket::send()'s maxBackpressure check — the last now compares a plaintext limit against a value that includes up to ~128 KB of ciphertext. That's bounded and directionally conservative, but it is a semantic shift for every SSL caller. The Windows loop.c change alters control flow in the hot per-read dispatch and interacts with the existing AFD RST probe just below it (the eof-drain continue fires on repeat_recv_count == 0 iterations too, so the probe's own repeat_recv_count++ == 0 guard is still reached only after the drain exhausts). us_internal_ssl_on_end honouring allow_half_open for node:https is a new codepath that a maintainer familiar with the TLS close_notify / raw-FIN sequencing should sign off on.
Other factors
The new function is small and mirrors ssl_drain_spill's arithmetic exactly. Test coverage is good for the node:https server side (three end-mode variants, 5 iterations each) and doubles as the Windows client-side eof-drain regression per its comment. CI (#77707) is still building. Given the blast radius across TLS HTTP, WebSocket backpressure, and a Windows-only loop change, deferring to a human reviewer.
…getBufferedAmount() getBufferedAmount() stays the plaintext AsyncSocketData::buffer count (WebSocket maxBackpressure and the JS-exposed bufferedAmount keep their existing semantics). The new hasFullyDrained() adds the TLS spill so the HttpResponse/HttpContext close gates wait for it. Also: release a zero-progress spill in us_internal_ssl_on_writable once the peer's readable side has ended, so a FIN-then-RST client cannot wedge the writable dispatch before the close gate is reached. Covered by a new closes-promptly test.
robobun
changed the title
…close is O(1) Releasing the spill and falling through re-sealed the whole buffered response 128 KB at a time before the hasFullyDrained() gate fired. Setting ssl_fatal_error makes us_internal_ssl_write return 0, so the uWS flushed==0-after-FIN guard (or hasFullyDrained() when nothing is buffered) closes on the first dispatch. Dispatch directly to bypass the is_shut_down gate that ssl_fatal_error would otherwise trip.
There was a problem hiding this comment.
All four rounds of my prior inline feedback have been addressed (comment placement, dead halfClose param, stale getBufferedAmount() comments, and the O(n) re-encryption cycle on a stuck spill — the last fixed in 41c2fbb by marking the SSL fatal and dispatching writable directly). No new issues found on this pass, but this is low-level cross-layer TLS/networking work — spill accounting in openssl.c, dropping the !SSL gate on allow_half_open, a Windows-specific eof-drain in loop.c, and six close-gate rewires — so it's worth a human look.
What was reviewed:
hasFullyDrained()vsgetBufferedAmount()at every close gate; confirmed the plain-TCP path is unchanged (if constexpr (SSL)) and WebSocketmaxBackpressurestill reads plaintext.- The 41c2fbb
ssl_fatal_errorpath: tracedus_internal_ssl_write→ returns 0 viais_shut_down, soflushed==0 && HTTP_NODE_RECEIVED_FINfires O(1); confirmed the directus_dispatch_writablereturn correctly bypasses the line-1864 shut-down gate. - Windows eof-drain
continue: bounded by recv()==0/WSAEWOULDBLOCK, guarded on!is_paused/!closedlike the POSIX sibling. us_internal_ssl_spill_pending: owner check prevents another socket's spill from blocking this one's close gate.
Extended reasoning...
Overview
Nine files across bun-usockets and bun-uws. Adds us_socket_ssl_spill_pending() (openssl.c/socket.c/libusockets.h/internal.h) to expose the loop-wide TLS ciphertext spill for a given socket; adds AsyncSocket::hasFullyDrained() and switches six close-after-drain gates in HttpContext.h/HttpResponse.h from getBufferedAmount()==0 to it. Drops the !SSL guard on allow_half_open in onOpen<IsNodeHttp>. Adds a zero-progress-after-EOF spill release in us_internal_ssl_on_writable that marks the SSL fatal and dispatches writable directly (41c2fbb). Adds a Windows eof-drain continue in loop.c mirroring the POSIX branch. New describe("https") block with four tests in node-http-backpressure.test.ts.
Security risks
None identified. The change is in the drain/close-timing layer, not in TLS verification, cert handling, or handshake logic. rejectUnauthorized: false in the test is client-side against a self-signed test cert. Setting ssl_fatal_error on a post-EOF stuck spill only accelerates a close that was already going to happen; it does not weaken any verification path.
Level of scrutiny
High. This is production-critical networking code with subtle cross-platform (kqueue/epoll/libuv-AFD) and cross-layer (usockets loop → openssl.c spill → uWS close gates) interactions. The 41c2fbb fix in particular sets ssl_fatal_error, which has non-local effects: us_internal_ssl_is_shut_down returns true, us_socket_is_shut_down returns true, and every subsequent us_internal_ssl_write returns 0. I traced that this is the intended O(1) close (the peer is already gone), but the reach of that flag across shutdown/close/write paths is exactly the kind of thing a maintainer should confirm.
Other factors
The PR has been through four rounds of my automated review with substantive findings each time, all addressed by the author. Test coverage is good: three end-mode variants × 5 iterations, plus a FIN-then-RST wedge test. The PR description explicitly names the relationship to #35088 (Bun.serve sibling) and #34510 (general stuck-TLS-send fix), and CI is building on 41c2fbb. The Windows loop.c change is small but platform-specific and only exercised via the client side of the new test — a maintainer familiar with the AFD/libuv poll_cb path should confirm the continue cannot spin (it's bounded by recv() returning 0/WSAEWOULDBLOCK, same as the POSIX branch).
|
CI status for 41c2fbb (build #77767, 190/196 lanes passed):
Ready for review. |
…35088) ### Problem ```js const server = Bun.serve({ port: 0, fetch: () => new Response(Buffer.alloc(8 * 1024 * 1024, 'a')), }); // raw net client: socket.end('GET / HTTP/1.1\r\nHost: x\r\n\r\n') // expected: body bytes 8388608 // actual: body bytes 2621440 (truncated at the first kernel send), then connection drops ``` A raw-socket client that sends its request with `socket.end(...)` (half-closes its write side) receives only what the kernel accepted on the first `send()`; the rest of the response body is dropped. Same result for a static `routes: { '/': new Response(bigBuffer) }` and for `Bun.serve({ tls })`. ### Cause `HttpContext::onOpen<false>` does not set `s->flags.allow_half_open`, so loop.c force-closes the socket right after dispatching `onEnd<false>`, which unconditionally closes. A `tryEnd` that did not complete holds its tail as `HttpResponseData::offset < total` with nothing in `AsyncSocketData::buffer` (it writes with `optional=true`); that tail is discarded on close. ### Fix * `onOpen`: set `allow_half_open` for every HTTP server socket (the `IsNodeHttp` guard is lifted; #35109 already dropped the `!SSL` guard). `onEnd` closes in its fall-through so this is not a behavior change on its own. * `onEnd<false>`: defer close only when the response is already fully determined: a `tryEnd` tail (`HTTP_END_CALLED` set by the content-length `internalEnd` path while `HTTP_RESPONSE_PENDING` is still set), or a completed response that has not fully drained. The connection shuts down from the existing `shouldCloseConnection()` gates once those bytes have drained. A streaming body the application is still producing (`HTTP_END_CALLED` clear, `HTTP_RESPONSE_PENDING` set) closes here as before so `onAborted` / `request.signal` fires on client disconnect; the `onWritable` slot being armed is not treated as pending output because `do_render_stream` keeps it armed for a streaming response's lifetime regardless of backpressure. The drain checks use `hasFullyDrained()` (from #35109), which accounts for the TLS ciphertext spill, so the defer is accurate for both transports. * `onWritable`: close a deferred connection on a zero-progress writable event after FIN, for both deferred shapes: a stuck buffered flush (`flushed == 0`, the existing node:http check lifted out of its `IsNodeHttp` guard) and a `tryEnd` retry whose `offset` did not advance. ### Scope The sendfile path is intentionally left for follow-up: `new Response(Bun.file(big))` bytes are driven by `FileResponseStream` directly on the fd rather than handed to uWS, and `uws_res_prepare_for_sendfile` does not set `HTTP_END_CALLED` (only `uws_res_end_sendfile` does, at completion), so the defer would need a new state bit. Behavior unchanged from main. ### Relation to #35034 / #35109 \#35034 (merged) is the node:http sibling and is scoped `if constexpr (IsNodeHttp)`; this PR is the `Bun.serve` (`!IsNodeHttp`) side. #35109 (merged) added `hasFullyDrained()` and made the `shouldCloseConnection()` close gates wait for the TLS ciphertext spill; this PR's `onEnd<false>` defer and `onWritable` zero-progress check use that helper so both transports are covered. ### Verification New `describe` in `test/js/bun/http/serve.test.ts` (requests send no `Connection: close`, so the post-drain shutdown is driven by the `HTTP_NODE_RECEIVED_FIN` clause of `shouldCloseConnection()`): * fetch-handler, static-route, and https fetch-handler tryEnd-tail cases receive 2621440 bytes on main and the full 8 MiB with the fix. * a destroy()-mid-drain case pins the zero-progress close (bounded poll on `server.pendingRequests`; idleTimeout 60 so a spin misses the deadline rather than being masked by an idle-timeout close). On Windows loopback the whole body fits the kernel send buffer so there is no tryEnd tail; `pendingRequests` is the portable observable. * `request.signal` still fires on client FIN for a streaming (SSE-shaped) response. `serve.test.ts`, `node-http-backpressure.test.ts` (including #35034's and #35109's cases), `bun-serve-static.test.ts`, `bun-serve-routes.test.ts`, `bun-serve-file.test.ts`, `bun-serve-ssl.test.ts` pass. 20/20 runs of the new tests on Windows aarch64 (including the HTTPS case). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/serve.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
Problem
A node:https server responding with a body that backpressures truncates at the first kernel-accepted write when the client half-closes right after its request. Same for
res.end(bigBuffer). Node.js delivers the full body.Cause
Two server-side layers and one Windows-only client-side layer:
us_internal_ssl_write()seals plaintext into 16 KB TLS records and flushes them to the kernel in ~128 KB batches. A partial kernel write parks the remainder of the batch in the loop'sssl_spillslot and returns the full plaintext count as written, soAsyncSocket::getBufferedAmount()(which reads onlyAsyncSocketData::buffer) reports 0 while up to one batch of ciphertext is still in userspace. TheshouldCloseConnection()close gates inHttpResponse::internalEnd/HttpResponse::cork/HttpContext::onData/HttpContext::onWritablekey ongetBufferedAmount() == 0and so fire early;us_internal_ssl_close(code=0)does one best-effort drain and frees the rest.allow_half_opengated on!SSLfor node:http:HttpContext::onOpenonly setallow_half_openfor non-TLSIsNodeHttpsockets, so a client FIN on a node:https connection madeus_internal_ssl_on_endforce-close the socket right after dispatchingonEnd, discarding the buffered response.onEnd<IsNodeHttp>'s existing defer was never reached. node:http: drain already-written response bytes when the peer half-closes #35034 fixed the plain-TCP case; the TLS side was left gated out because the spill made the close gates unsafe.poll_cb(libuv.c) maps AFDUV_DISCONNECTto the eof hint for a socket whose write side we already shut down. AFD reports DISCONNECT while the tail of the peer's stream is still queued in the kernel, but the Windows branch ofloop.c's read loop only did one extrarecv()(the RST probe) before falling through to theis_shut_downraw-close, discarding the rest. Reproduces on released bun against a Node.js server: a half-closednet.Socket/tlsclient intermittently loses the end of a large response on Windows only.Fix
us_socket_ssl_spill_pending()(openssl.c / socket.c / libusockets.h): ciphertext bytes already sealed for this socket and reported as written byus_socket_write(), still waiting on a writable event. Returns 0 for plain-TCP sockets.AsyncSocket::hasFullyDrained():buffer.length() == 0 && spill_pending == 0. The HTTP close-after-drain gates (HttpResponse::internalEnd/cork,HttpContext::onDatatail /onWritable/onEnd<IsNodeHttp>) switch fromgetBufferedAmount() == 0to this.getBufferedAmount()itself is unchanged so WebSocket'smaxBackpressurepolicy and the JS-exposedbufferedAmountstay a plaintext count. The spill is bounded (≤ one 128 KB batch) andus_internal_ssl_on_writabledrains it before dispatching the user-level writable, so the existing drain loops terminate: onceAsyncSocketData::bufferempties, the next writable event drains the final spill and the close gate fires with nothing pending.HttpContext::onOpen<IsNodeHttp>: drop the!SSLguard onallow_half_open.us_internal_ssl_on_endalready honours the flag;onEnd<IsNodeHttp>'shasQueuedOutgoingnow accounts for the spill, andonWritable's zero-progress-after-FIN close is not gated on!SSL.us_internal_ssl_on_writable: release a zero-progress spill once the peer's readable side has ended, so a FIN-then-RST client does not wedge the writable dispatch before the close gate is reached (the drain would otherwise re-arm writable on a send() that keeps failing). usockets: close a TLS socket whose send() keeps failing instead of spinning the writable dispatch #34510 is the general fix for stuck TLS sends; this is the narrow case the newallow_half_openpath opens.loop.cWindows read loop: drain on the eof hint like the POSIX branch already does (recv()returning 0 orWSAEWOULDBLOCKends the loop, bounded by the kernel receive buffer).Relation to #35088
#35088 is the
Bun.serve(!IsNodeHttp) sibling and is currently gated on!SSLbecause the spill was invisible to itsonEnddefer'sdoneButBufferedcheck and tointernalEnd's close gate (its scope note says so). WithhasFullyDrained()at those gates that PR'sonEnddefer is accurate for TLS too, so it can drop its!SSLgates (switching itsgetBufferedAmount() > 0to!hasFullyDrained()).Verification
New
describe('https')intest/js/node/http/node-http-backpressure.test.tsmirrors the existing plain-HTTP half-close tests over TLS forres.write()+res.end(),res.end(payload)(theoptional=falseinternalEndbuffer path), andhttpAllowHalfOpenwithres.end()after drain; each receives ~2.6 MB on main and the full 8 MiB with the fix, matching Node.js. Looped 5× per case so the on_writable drain cycle is exercised past the first kernel-accepted write; the loop also covers the Windows client-side eof-drain. A fourth test half-closes then destroys the client after first data and asserts the server-side socket'close'fires (would wedge on a stuck spill).Verified on linux-x64 (14/14) and windows-x64 (14/14, 5 consecutive runs of the new tests).
node-http-backpressure.test.ts,node-http-pinned-write.test.ts,node-http-server-socket-end-drain.test.ts,bun-serve-ssl.test.ts,node-tls-connect.test.ts,node-https-checkServerIdentity.test.ts,serve.test.ts,socket.test.tspass (pre-existing container-only / debug-timeout failures unchanged from main).[review] gate passed · iteration 1 · 9 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file