Repository navigation
usockets: close a TLS socket whose send() keeps failing instead of spinning the writable dispatch - #34510
Closed
robobun wants to merge 8 commits into
Closed
usockets: close a TLS socket whose send() keeps failing instead of spinning the writable dispatch#34510robobun wants to merge 8 commits into
robobun wants to merge 8 commits into
Conversation
us_socket_raw_write (packages/bun-usockets/src/socket.c) has no errno classification: any send() -1 sets last_write_failed and re-arms the writable poll. The SSL spill-drain path (us_internal_ssl_on_writable -> ssl_drain_spill -> us_socket_raw_write) therefore loops as long as the kernel keeps the fd writable and send() keeps failing. Fixture arms fault injection to return -1 EPIPE for N consecutive send() calls on a TLS client fd after the handshake completes, issues one write (which spills), and waits for the server to receive the data. On main, all N failures are silently consumed by the writable re-arm loop and the server receives; the socket never errors or closes. N=50000: SPIN received=true errored=false closed=false N=50000 cpu=2305ms wall=1342ms Same result for ECONNRESET and EINVAL. Plain TCP (node:net) does not spin because its write path uses us_socket_write_check_error, which has bounded retry plus peer-gone classification.
… cannot spin on a dead fd us_socket_raw_write (and us_socket_write / us_socket_raw_writev / us_socket_write2 / us_socket_ipc_write_fd) treated any -1 from bsd_send as a partial write: set last_write_failed, re-arm the writable poll, return 0. The SSL ciphertext spill path (us_internal_ssl_on_writable -> ssl_drain_spill -> us_socket_raw_write) therefore re-entered on every writable event for as long as send() kept failing and the kernel kept the fd writable, pinning a core at 100%. Observed on macOS as idle long-lived processes spinning in us_internal_ssl_on_writable -> __sendto with the kernel logging from inside every sosend, consistent with a Network Extension content filter returning an errno on a socket it has not closed. Reproduced on Linux and macOS via fault injection (50000 consecutive EPIPE failures were silently retried and the data eventually delivered once the fault disarmed; the socket never errored or closed). Fix: factor the errno classification from us_socket_write_check_error into us_internal_on_short_write() and call it from every write helper. A peer-gone errno, or an unclassified errno that persists past the existing bounded retry window, latches fatal_send_errno on the socket; loop.c closes the socket with that errno after the writable dispatch instead of re-arming for another retry. Would-block, transient ENOBUFS/ENOMEM, and bounded unclassified errnos still re-arm. fatal_send_errno lives in the pad-to-pointer gap before group, so struct us_socket_t does not grow.
Collaborator
Author
|
Updated 10:26 PM PT - Jul 17th, 2026
❌ @robobun, your commit 9dc972c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34510That installs a local version of the PR into your bun-34510 --bun |
Contributor
WalkthroughChangesThe socket layer now uses unified pending actions for deferred closure, shutdown, and detachment. POSIX send failures are classified with bounded retries, fatal writes close during writable dispatch, TLS paths adopt the shared state, and skipped tests cover fault scenarios. Socket pending actions and send handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Contributor
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review follow-up: us_internal_on_short_write is only called on written != length, so its counter reset never ran for a full write and the counter measured lifetime (not consecutive) unclassified errnos on the TLS spill path. Reset it on written == length in each write helper, matching us_socket_write_check_error. Also refactor us_socket_write_check_error's POSIX errno branch to use the shared us_internal_classify_send_error so the classification lives in one place. New test asserts 50 rounds of one-off EPROTOTYPE each recovered by a successful drain leave the socket open (it closed at round 33 before the reset).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@test/js/bun/net/socket-syscall-fault.test.ts`:
- Around line 241-269: Update tlsPair to pre-register cleanup immediately after
creating the server and socket, then wrap listening, secureConnect, and fd
validation in try/catch; on setup failure, clear fault, destroy the socket,
close the server, and rethrow the original error. Preserve the existing
Symbol.dispose cleanup for successful setup.
- Around line 295-307: Update the unclassified-errno retry test around the
existing “unclassified errno” case to cover both sides of the 32-failure
boundary: add a case where exactly 32 consecutive EINVAL send failures recover
and deliver the bytes, and a case where the 33rd consecutive failure closes the
socket with EINVAL. Replace the broad repeat: 200 assertion with exact failure
counts and verify both closure/error behavior and server delivery.
- Around line 319-328: Bound the receive-polling loop in the test around
pair.serverReceived() with a finite maximum number of event-loop turns; if the
expected 8 * (i + 1) bytes are not received and sawClose remains false, fail the
test while reporting the expected and actual byte counts. Preserve the existing
early exit on sawClose and the per-round errno-then-success behavior.
🪄 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: 72a27781-ba76-4742-b239-f7cea78abaa7
📒 Files selected for processing (5)
packages/bun-usockets/src/context.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/loop.cpackages/bun-usockets/src/socket.ctest/js/bun/net/socket-syscall-fault.test.ts
Collaborator
Author
|
CI on 9dc972c (build #74980): the new Since the last update: 32441db folds the latch into the existing deferred-close state (one Ready for re-review. |
The comment said the write helpers stop re-arming on fatal, but us_internal_on_short_write re-arms once so loop.c's check delivers the close. Match the (correct) socket.c comment.
Contributor
There was a problem hiding this comment.
No further findings — the two earlier rounds (counter-reset on full success, classifier dedup, stale header comment) are all addressed in 1e38d8e / 7ab3ec2. Deferring to a maintainer since this touches the write-error path shared by every TCP/TLS socket; worth a human eye on the close-from-writable-dispatch shape.
What was reviewed: errno classification in us_internal_classify_send_error / us_internal_on_short_write and its five call sites; the fatal_send_errno latch-then-close ordering in loop.c (runs after the writable handler, so one bounded retry then close — no spin); fatal_send_errno = 0 initialization at all four socket-creation sites; struct layout (byte in the existing pad gap, no growth); the send_errno > 2 clamp matches the existing poll-error branch's CloseCode-collision guard.
Extended reasoning...
Overview
Factors send()-errno classification out of us_socket_write_check_error into a shared us_internal_classify_send_error helper and applies it (via us_internal_on_short_write) to all five us_socket_*write* helpers in packages/bun-usockets/src/socket.c. Adds a one-byte fatal_send_errno field to struct us_socket_t (in the existing pad-to-pointer gap) and a check in loop.c's writable dispatch that closes the socket with that errno. Initializes the field at the four socket-creation sites (accept, connect, listen, from_fd). Three new fault-injection tests cover peer-gone errno, sustained unclassified errno, and sporadic errnos that recover.
Security risks
None identified. This is defensive error handling on the send path; no new attack surface. The errno set in us_internal_send_errno_is_peer_gone is unchanged (moved, not modified). No user-controlled input reaches the classification.
Level of scrutiny
High — this is the low-level write path underneath every TCP/TLS socket in Bun (Bun.serve, fetch, node:net/tls/http2, WebSocket, SQL drivers). The behavior change is that sockets whose send() persistently fails now close instead of spinning, which is correct but is a semantic change on a hot shared path. A maintainer should confirm the design choice of checking fatal_send_errno after us_internal_ssl_on_writable/us_dispatch_writable returns (rather than before), and that us_internal_socket_close_raw from inside the writable branch is the right teardown shape here.
Other factors
Two prior review rounds from me were addressed: the missing unclassified_send_failures reset on fully-successful sends (1e38d8e, with a dedicated regression test), the classifier duplication in write_check_error (1e38d8e), and the stale fatal_send_errno field comment (7ab3ec2). CodeRabbit's three test-shape comments were declined with reasoning matching file-local convention and test/CLAUDE.md, and CodeRabbit withdrew them. The author's CI report on 1e38d8e shows the new tests passing on every lane with only pre-existing/flaky failures elsewhere. The fault-injection tests demonstrate fail-before/pass-after and cover the counter-reset regression the first review round caught.
Jarred-Sumner
requested changes
Jul 17, 2026
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
Collaborator
There was a problem hiding this comment.
This seems like the wrong place to put it? Do we really need to store this?
Collaborator
Author
|
The constraint is that the fatal errno surfaces from inside the write helpers, and one of The byte sits in the existing pad-to-pointer gap before If the objection is to storing the errno value at all, I can switch to a single bit in the spare half of the |
us_socket_t had grown four independent "do X later" fields — ssl_shutdown_after_spill, ssl_close_after_spill, ssl_pending_detach plus ssl_pending_close_code, and fatal_send_errno. They are mutually exclusive by construction (a close outranks a shutdown, and a close that must wait for the SSL call on the stack to unwind outranks all), so they collapse into one 3-bit pending_action enum ordered by priority plus a single payload byte, latched through us_internal_socket_set_pending which never downgrades a parked close. Impossible combinations (shutdown && close both latched, a close code with nothing pending) are now unrepresentable. Also drop ssl_is_server: written once in ssl_attach and read nowhere; the role lives in BoringSSL (SSL_set_accept_state / connect_state) and in the Rust TLS_SERVER_ROLE flag.
Contributor
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 `@packages/bun-usockets/src/internal/internal.h`:
- Around line 334-340: Update us_internal_socket_set_pending so an equal-action
relatch with code zero does not overwrite an existing nonzero pending_code;
continue allowing higher actions to replace both fields and preserve normal
updates when the new code is nonzero. This must retain FAST_SHUTDOWN/RESET
payloads when DETACH(0) is later re-latched.
🪄 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: 773d8c18-f7ed-4fd4-b310-901dbed19875
📒 Files selected for processing (6)
packages/bun-usockets/src/context.cpackages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/loop.cpackages/bun-usockets/src/socket.csrc/runtime/socket/socket_body.rs
us_internal_socket_set_pending with action == pending_action overwrote pending_code unconditionally. The SNI-abort path (openssl.c:2267, :2452) re-latches DETACH(0) after a JS callback may already have parked DETACH(RESET) via us_internal_socket_close_raw, which dropped the close code. Equal-action re-latch now only writes the code when the new code is nonzero or no code was parked. Drops the inline workaround at us_internal_ssl_detach that did the same thing manually. Fixes the tls.Server destroySoon test that 32441db regressed.
The field was folded into pending_action in 32441db.
Contributor
There was a problem hiding this comment.
No bugs found, but this reworks the usockets deferred-close state machine (folding four ssl_* latches into a priority-ordered pending_action enum across openssl.c/socket.c/loop.c) on top of the send-errno fix — worth a human look at the ordering/supersede semantics before merge.
What was reviewed:
- errno classification factored into
us_internal_on_short_writeand applied to all five write helpers;us_socket_write_check_errornow shares the same classifier on POSIX pending_actioninit at every socket-creation site (accept, connect, listen, from_fd);ssl_attachdeliberately does not reset itus_internal_socket_set_pendingsupersede rule and the DETACH(0)-after-DETACH(code) case fixed in d3dd45e- removed
ssl_is_server/ssl_*_after_spill/ssl_pending_*have no remaining references
Extended reasoning...
Overview
The PR fixes #24845 (100% CPU spin when send() on a TLS socket keeps returning a fatal errno while the kernel still reports the fd writable) by extending the existing errno classification from us_socket_write_check_error to every us_socket_*write* helper. On top of that, commit 32441db folds four separate SSL deferred-action bitfields (ssl_shutdown_after_spill, ssl_close_after_spill, ssl_pending_detach, ssl_pending_close_code) plus the new fatal-errno latch into a single enum us_socket_pending_action + pending_code byte on struct us_socket_t, with a priority-ordered us_internal_socket_set_pending() helper (higher action supersedes; equal action preserves a nonzero code). Touches internal.h, socket.c, loop.c, context.c, openssl.c, plus a comment in socket_body.rs and three new fault-injection tests.
Security risks
None identified. This is send-error handling on already-open sockets; no auth/crypto/parsing of untrusted input is affected. The errno set classified as peer-gone matches libuv's, and the bounded-retry window for unclassified errnos is unchanged from the pre-existing us_socket_write_check_error behavior.
Level of scrutiny
High. usockets is the C foundation under every TCP/TLS/HTTP/WebSocket connection in Bun. The core errno-classification fix is a mechanical extension of existing code, but the accompanying pending_action refactor is a state-machine redesign: it collapses four independent flags into one totally-ordered enum whose supersede semantics already produced one regression during this PR (32441db broke tls.Server socket destroySoon, fixed in d3dd45e). The interleaving cases — e.g. a CLOSE_RAW latched by a fatal send while DETACH is already parked (lower is ignored, DETACH's us_socket_close covers it), or SHUTDOWN parked then CLOSE arrives (CLOSE supersedes, which is correct) — look right to me, but a maintainer should sign off on the ordering and on us_internal_ssl_attach no longer resetting the deferred-close latch.
Other factors
All prior inline findings from earlier runs (stale comment text, contradicting re-arm wording) were addressed and the threads resolved. CodeRabbit's findings were addressed or withdrawn. CI on the earlier revision passed the new tests on every lane. The removed ssl_is_server bitfield has no remaining references. No human has reviewed yet; the author's own comment at 00:00Z offered an alternative single-bit design, and the enum approach was chosen without explicit maintainer input — that design choice is the main thing I'd want a human to confirm.
Contributor
There was a problem hiding this comment.
No bugs found this pass, but this touches the core usockets write path and TLS state machine and folds four independent deferred-close bits into a single precedence-ordered enum — worth a human look before merge.
What was reviewed:
- errno classification in
us_internal_on_short_writeand its call sites across all five write helpers;unclassified_send_failuresreset on full-success sends (fixed in 1e38d8e) pending_actionprecedence vs the old four-flag semantics: CLOSE_RAW correctly outranks a parked SHUTDOWN/CLOSE when the spill-drain send goes fatal; DETACH still wins over CLOSE_RAW so ssl_in_use deferral is preservedus_internal_socket_set_pendingequal-action re-latch keeps a nonzero code (d3dd45e);ssl_is_serverremoval is dead-code cleanup (write-only field, no remaining readers)- init sites for the new fields cover accept/connect/listen/from_fd;
ssl_attachdeliberately does not resetpending_action(comment explains adopt_tls case)
Extended reasoning...
Overview
Fixes #24845 (100% CPU spin when send() on a TLS socket keeps returning a non-EAGAIN errno on an fd the kernel still reports writable). Two intertwined changes across packages/bun-usockets/:
-
Errno classification for all write helpers. Factors the existing bounded-retry logic from
us_socket_write_check_errorintous_internal_classify_send_error()+us_internal_on_short_write(), and applies it tous_socket_write,us_socket_raw_write,us_socket_raw_writev,us_socket_write2, andus_socket_ipc_write_fd. A peer-gone errno or an unclassified errno past the 32-retry ceiling now parksUS_PENDING_ACTION_CLOSE_RAW;loop.c's writable dispatch closes the socket with that errno on the next event instead of retrying forever. -
Deferred-close state refactor. Folds four separate
us_socket_tbitfields (ssl_shutdown_after_spill,ssl_close_after_spill,ssl_pending_detach,ssl_pending_close_code) into onepending_action : 3enum +pending_codebyte, with a precedence rule (us_internal_socket_set_pending: higher supersedes; equal keeps a nonzero code). The newCLOSE_RAWaction slots into this. Also drops the write-onlyssl_is_serverbit.
Plus: field init at all four socket-creation sites, a one-line comment update in socket_body.rs, and three new fault-injection tests in socket-syscall-fault.test.ts.
Security risks
None identified. No new user-facing surface, no parsing of untrusted input; the change tightens error handling on an internal write path. The errno is truncated to unsigned char before storage, and loop.c clamps values ≤2 to ECONNRESET so they can't collide with the libus close-code enum.
Level of scrutiny
High. This is hot-path C in the core networking layer — every TLS and plain-TCP write in Bun routes through the touched helpers, and loop.c's writable dispatch runs on every writable event. The four-bitfield → single-enum refactor is a semantic transformation whose correctness depends on the precedence ordering matching every combination the old independent flags allowed. One iteration of this PR (32441db) already regressed tls.Server socket destroySoon until d3dd45e fixed the equal-action re-latch rule, which demonstrates the interaction space is subtle. There's also a design question the author explicitly left open for a maintainer (whether to store the errno byte vs. a single bit + SO_ERROR fallback) that was resolved by the enum refactor without visible maintainer sign-off.
Other factors
- All prior inline findings (mine and CodeRabbit's) are addressed and resolved; the current head (9dc972c) is a comment-only fix on top of d3dd45e.
- CI on 1e38d8e was green for the new tests on all lanes per the robobun report; the enum refactor landed after that report, so a fresh CI run on the head commit should be confirmed.
- Minor behavior alignment I did not flag as a bug:
us_socket_write2now setslast_write_failedon a short write (previously only re-armed) — this matches every other write helper and looks intentional. - Grepped for stragglers: no remaining references to the four removed field names or
ssl_is_serveranywhere in the tree.
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 22, 2026
…ge https response after peer FIN (#35109) ### Problem ```js import https from 'node:https'; const server = https.createServer({ key, cert }, (req, res) => { res.writeHead(200, { 'content-length': String(8 * 1024 * 1024) }); res.write(Buffer.alloc(8 * 1024 * 1024, 'a')); res.end(); }); // raw tls client: socket.end('GET / HTTP/1.1\\r\\nHost: x\\r\\n\\r\\n') // Node: body bytes 8388608 // Bun: body bytes 2637824 (truncated at the first kernel-accepted batch) ``` 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: * **TLS spill not counted at the close gates**: `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's `ssl_spill` slot and returns the full plaintext count as written, so `AsyncSocket::getBufferedAmount()` (which reads only `AsyncSocketData::buffer`) reports 0 while up to one batch of ciphertext is still in userspace. The `shouldCloseConnection()` close gates in `HttpResponse::internalEnd` / `HttpResponse::cork` / `HttpContext::onData` / `HttpContext::onWritable` key on `getBufferedAmount() == 0` and so fire early; `us_internal_ssl_close(code=0)` does one best-effort drain and frees the rest. * **`allow_half_open` gated on `!SSL` for node:http**: `HttpContext::onOpen` only set `allow_half_open` for non-TLS `IsNodeHttp` sockets, so a client FIN on a node:https connection made `us_internal_ssl_on_end` force-close the socket right after dispatching `onEnd`, discarding the buffered response. `onEnd<IsNodeHttp>`'s existing defer was never reached. #35034 fixed the plain-TCP case; the TLS side was left gated out because the spill made the close gates unsafe. * **Windows eof-drain** (surfaced by the new test's client side): `poll_cb` (libuv.c) maps AFD `UV_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. Reproduces on released bun against a Node.js server: a half-closed `net.Socket`/`tls` client 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 by `us_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::onData` tail / `onWritable` / `onEnd<IsNodeHttp>`) switch from `getBufferedAmount() == 0` to this. `getBufferedAmount()` itself is unchanged so WebSocket's `maxBackpressure` policy and the JS-exposed `bufferedAmount` stay a plaintext count. The spill is bounded (≤ one 128 KB batch) and `us_internal_ssl_on_writable` drains it before dispatching the user-level writable, so the existing drain loops terminate: once `AsyncSocketData::buffer` empties, the next writable event drains the final spill and the close gate fires with nothing pending. * **`HttpContext::onOpen<IsNodeHttp>`**: drop the `!SSL` guard on `allow_half_open`. `us_internal_ssl_on_end` already honours the flag; `onEnd<IsNodeHttp>`'s `hasQueuedOutgoing` now accounts for the spill, and `onWritable`'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). #34510 is the general fix for stuck TLS sends; this is the narrow case the new `allow_half_open` path opens. * **`loop.c` Windows read loop**: drain on the eof hint like the POSIX branch already does (`recv()` returning 0 or `WSAEWOULDBLOCK` ends the loop, bounded by the kernel receive buffer). ### Relation to #35088 \#35088 is the `Bun.serve` (`!IsNodeHttp`) sibling and is currently gated on `!SSL` because the spill was invisible to its `onEnd` defer's `doneButBuffered` check and to `internalEnd`'s close gate (its scope note says so). With `hasFullyDrained()` at those gates that PR's `onEnd` defer is accurate for TLS too, so it can drop its `!SSL` gates (switching its `getBufferedAmount() > 0` to `!hasFullyDrained()`). ### Verification New `describe('https')` in `test/js/node/http/node-http-backpressure.test.ts` mirrors the existing plain-HTTP half-close tests over TLS for `res.write()+res.end()`, `res.end(payload)` (the `optional=false` `internalEnd` buffer path), and `httpAllowHalfOpen` with `res.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.ts` pass (pre-existing container-only / debug-timeout failures unchanged from main). <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 1 · 9 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 3 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-backpressure.test.ts bun test v1.4.0 (41c2fbb) test/js/node/http/node-http-backpressure.test.ts: (pass) backpressure > should handle backpressure [588.62ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the client requested the close [400.31ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the server sets Connection: close on a keep-alive request [190.66ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the whole body is passed to res.end() [216.37ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() [179.05ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() without res.end() [136.15ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (3d1c56b) test/js/node/http/node-http-backpressure.test.ts: (pass) backpressure > should handle backpressure [32.49ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the client requested the close [22.78ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the server sets Connection: close on a keep-alive request [18.46ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the whole body is passed to res.end() [11.04ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() [9.60ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() without res.end() [7.83ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() after drain, httpAllowHalfOpen [6.33ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-backpressure.test.ts bun test v1.4.0 (41c2fbb) test/js/node/http/node-http-backpressure.test.ts: (pass) backpressure > should handle backpressure [517.49ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the client requested the close [423.69ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the server sets Connection: close on a keep-alive request [187.06ms] (pass) backpressure > Connection: close does not truncate a response that is still flushing > when the whole body is passed to res.end() [172.00ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() then res.end() [215.92ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still flushing > res.write() without res.end() [158.36ms] (pass) backpressure > a client FIN right after the request does not truncate a response that is still ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 738ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [0/25] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_output v0.0.0 (/workspace/bun/src/output) �[1m�[92m Compiling�[0m bun_clap v0.0.0 (/workspace/bun/src/clap) �[1m�[92m Compiling�[0m bun_valkey v ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` packages/bun-usockets/src/crypto/openssl.c | 27 +++++++ packages/bun-usockets/src/internal/internal.h | 1 + packages/bun-usockets/src/libusockets.h | 5 ++ packages/bun-usockets/src/loop.c | 13 ++++ packages/bun-usockets/src/socket.c | 7 ++ packages/bun-uws/src/AsyncSocket.h | 15 ++++ packages/bun-uws/src/HttpContext.h | 21 +++--- packages/bun-uws/src/HttpResponse.h | 6 +- test/js/node/http/node-http-backpressure.test.ts | 95 ++++++++++++++++++++++++ 9 files changed, 176 insertions(+), 14 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests packages/bun-usockets/src/crypto/openssl.c 9 4 0 packages/bun-usockets/src/internal/internal.h 1 1 0 packages/bun-usockets/src/libusockets.h 1 1 0 packages/bun-usockets/src/loop.c 4 1 0 packages/bun-usockets/src/socket.c 6 1 0 packages/bun-uws/src/AsyncSocket.h 5 3 0 packages/bun-uws/src/HttpContext.h 4 7 0 packages/bun-uws/src/HttpResponse.h 5 3 0 test/js/node/http/node-http-backpressure.test.ts 4 8 0 ``` </details> <!-- robobun:evidence:end -->
This was referenced Sep 25, 2026
dylan-conway
added a commit
that referenced
this pull request
Oct 2, 2026
…3957) Follow-up to #43947, which is merged. Rebased on main. ### Problem - After `shutdown()` or `end()` during a handshake the socket still seals handshake records. `us_socket_raw_write` sends nothing after a FIN, so `ssl_flush_write_batch` (`packages/bun-usockets/src/crypto/openssl.c`) parks them as a spill that can never drain. - `us_internal_ssl_close` waits for that spill, so the socket never closes. The spill holds the loop's one spill slot, so the next half-closed handshake never finishes. - No report is linked. 1.4.2 reaches it through `socket.shutdown()`, node:tls since #42181 (not released). ### Fix - `ssl_flush_write_batch` and the unbatched path of `BIO_s_custom_write` drop records that the socket can never send. - One predicate, `us_internal_socket_can_raw_write`, gates the raw writes, the raw shutdown and both drops. - Correct because these records can never reach the peer. Node completes the same handshakes. - Verified: 12 new tests, 10 fail without this change when run alone. Node v26.3.0 passes 34 of 34. Self-reviewed: 16 concerns raised, 14 addressed. ### Background - A spill is the part of a batch that the kernel did not take. The loop has one spill slot. - While the slot is taken, every socket writes record by record. - Considered a close that ignores the spill, as #43946 does for a forceful close. The slot stays taken until the close. ### Downsides - A slow reader still takes the spill slot. This PR removes only the owner that never lets go. - The peer never gets the dropped records. Nor does it on the base or on Node. - Release `.text`: 58154908 to 58153884 B. Each flush costs 1 more direct call. <details><summary>Notes</summary> **Cases, each test run alone, on the head of #43947 before its merge (771b423) and with this change** Each socket calls `shutdown()` or `end()` while its handshake runs. The peer keeps the connection open. | Case | #43947 | This PR | |---|---|---| | `Bun.listen`, `requestCert`, `rejectUnauthorized: true`, TLS 1.2, untrusted client | `handshake(true, authorized=false, error)`, the socket never closes | same report, then close | | Same, trusted client (control) | stays open until the client ends the connection, then both close | same | | Both rows, while another socket waits for a slow reader | the handshake never reports | same as the two rows above | | Two `Bun.connect` clients, TLS 1.3 | `close` never fires | both report the handshake, both close | | `Bun.connect` client, TLS 1.2, ended after the server's first flight | `close` never fires | `handshake(false)`, close | | `Bun.connect` client that writes to a second TLS socket from its handshake callback | `close` never fires | the write arrives, close | | Two `tls.connect` clients, the first (TLS 1.3 or TLS 1.2) stays open | the second never emits `secureConnect` | it emits it, as on Node v26.3.0 | | `tls.createServer`, TLS 1.2, `end()` in the handshake, untrusted client (control) | both sockets close | same | | Four `tls.connect` sockets after one such handshake in the same process | flight and first write leave in 2 segments | 1 segment | Run in one process, more tests fail on #43947 than the table shows: a test that times out leaves the socket that holds the spill slot. That is also the only way in which `TLS 1.3 client sees 'end', not ECONNRESET, when the server tears down right after rejecting its certificate (#40653)` failed. No CI run has shown the lost batching. The two-client probe gives the same result on the released 1.4.2 (`1.4.2+744846f84`): the second client never reports its handshake. **Mechanism** - With batching the handshake records go into the batch, and `ssl_flush_write_batch` sends them. After a FIN `us_socket_raw_write` returns 0. The flush parked all of it as a spill and took the spill slot. A spill drains from the writable event, and a socket that sent its FIN gets none. - `us_internal_ssl_close` defers a close with code 0 or 2 while the socket has a spill. - Batching needs a free slot (`hs_batching` in `us_internal_ssl_on_data`, `batching` in `us_internal_ssl_write`). With the slot taken, `BIO_s_custom_write` writes each record at once. After a FIN that write returns 0, the BIO asks for a retry, and `SSL_do_handshake` answers `SSL_ERROR_WANT_WRITE` until the socket closes. - Three places turn a short raw write into a wait for a writable event: the unbatched write, the flush, and `ssl_drain_spill`. The first two now drop. `ssl_drain_spill` has no guard on purpose: `us_internal_ssl_shutdown` defers the FIN behind a spill, and a close releases the spill, so a socket that sent its FIN has no spill to drain. **Why a drop is correct** - SSL counts a sealed record as written. It cannot be sent again, and after the FIN it cannot be sent at all. - The base already reports a completed handshake for the first half-closed socket on a loop: the batch takes the records and the report runs before the flush. The drop makes the second socket, and a socket next to a slow reader, behave like the first. - `us_internal_ssl_write` refuses application data after a FIN, and `ssl_handle_shutdown` sends no close_notify after one. So only handshake records and alerts reach the new branches. **Not in this PR** - A slow reader takes the spill slot like before. The slot is state of the loop, and a spill per connection is a different design. - A close on an open socket whose peer never reads still waits for its spill. That spill can drain. - A `send` that the kernel refuses with `ECONNRESET` or `EPIPE` still parks a spill. That needs the error of the raw write (#42336, #34510), not the state of the socket. - An `allowHalfOpen` socket that shut down during its handshake stays open after the peer's close_notify until the peer's FIN (since 1.4.1, #40384). Main does the same, and it does not reach the new branches. - #43946 changes what a forceful close does with a batch that is still held. It does not cover records that are sealed after a FIN. - `end()` from the `'connect'` listener sends the FIN before the ClientHello. Node sends the ClientHello first. **Self-review** Rejected, with the reason: - Move the tests that fail by a timeout into child processes. With this change no test times out. `afterAll` now releases what a timed-out test left, so the tests after the block are not affected. - A test for the case with no relay. The peer has to keep the connection open after our FIN, and a relay is how a test decides that. **Measurements** Call counts are gdb breakpoint hit counts on debug builds with the debug info stripped. Sizes are from release builds of both trees. - Healthy handshake, 100 in-process `Bun.listen` / `Bun.connect` connections: `SSL_do_handshake` 600 and 600, `BIO_s_custom_write` 700 and 700, `ssl_flush_write_batch` 300 and 300, `bsd_send` 600 and 600, `bsd_recv` 600 and 600, `us_internal_socket_close_raw` 200 and 200. - Both new branches are behind a write that the wire did not take in full. A socket that can write pays one more compare there, and nothing on a full write. - Release `.text`: 58154908 to 58153884 B (`size -A`). Stripped `bun`: 80844320 B both. `ssl_flush_write_batch` grows from 214 to 254 B and is no longer inlined into its callers: `BIO_s_custom_write` 683 to 512 B, `us_internal_ssl_close` 1195 to 999 B, `us_internal_ssl_on_data` 2651 to 2240 B, `us_internal_ssl_shutdown` 608 to 412 B. So each flush is one direct call more in a release build, 3 per healthy connection pair. - The shared predicate changes no size: `us_socket_raw_write` 178 B, `us_socket_raw_writev` 356 B and `us_internal_socket_raw_shutdown` 85 B before and after. - Allocations: the change adds none, and a dropped flight saves the `us_malloc` of its spill. **Tests** - `test/js/bun/net/socket.test.ts`: 8 new tests, 17 with "while the handshake runs" in the name. Run alone on #43947, 7 of the 8 fail, and the trusted TLS 1.2 client is the control. - `test/js/node/tls/node-tls-duplex-end-verify.test.ts`: 3 new tests. Run alone on #43947, 2 fail, and the TLS 1.2 server is the control. Node v26.3.0 passes 34 of 34 (`node --test`). - `test/js/node/tls/node-tls-connect.test.ts`: 1 new test, the one-segment flight. It fails on #43947 with 3 chunks for each connection. - The block was run 5 times on this change: 5 passes. - `test/js/node/tls/` and `test/js/bun/net/` together (728 tests): no failure is only on this change. Failures on the base too: `SNICallback runs even when the requested servername matches the bind hostname`, `should not call drain before handshake`, and tests that wait for a garbage collection. - After the rebase on main (ecf3490, with #43947 merged) the two directories have 802 tests, and 6 fail: 4 tests that wait for a garbage collection or for workers, a cluster test, and `should not call drain before handshake`. Each of them fails on main on the test machine too. Before that rebase, on e375701, the two directories had 765 tests. 6 fail, the same 6 as on #43947 at that base. In one process, #43947 at that base fails 8 of the 17 tests with "while the handshake runs" in the name, and the one-segment test. - The vendored `test-tls-*`, `test-https-*`, `test-http2-*` and `test-net-*` files: 660 pass, and the 3 that fail do so on #43924 too. This is a check for regressions only: no vendored file changes its result with this PR. </details> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Collaborator
|
Superseded by #44618, which consolidates the open TLS pull requests. This fix and its tests are in there, either as written, rewritten smaller, or merged with the other PRs that patched the same cause (see the "By area" list in that PR). Closing in favor of it. |
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 6, 2026
…essure (#34510, #38176, #42336) Every failed TLS write came back as "0 bytes, wait for writable": - send() rejected (EPIPE, ECONNRESET, ...): us_socket_raw_write folded -1 to 0 and re-armed writable. The kernel kept reporting writable, so the loop spun at 100% CPU (#24845), and a real reset ended as a clean close with the lost write reported as done. - SSL_write failed, or sealed records were lost to an allocation failure: the socket was marked fatal and nothing ever closed it. - SSL_write failed on a later 16 KiB record of one write: a plain partial write. A rejected send() of sealed records now ends the write side (the state our own FIN already has, so the BIO and the batch flush drop what can no longer leave) and asks for one writable event. There, outside any caller's write, the socket closes with an error, unless the peer's earlier bytes are still queued: then the read side delivers them first (a 413 sent before the server reset an upload). A fatal socket is closed at the same boundary. us_socket_write_check_error reports both: the errno of the send, or EPROTO, so node:tls fails the write like Node and Bun.connect's write()/end() return the documented -1. No new per-socket state: one int of loop scratch carries the errno from the raw send to the write that is still on the stack.
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 7, 2026
…essure (#34510, #38176, #42336) Every failed TLS write came back as "0 bytes, wait for writable": - send() rejected (EPIPE, ECONNRESET, ...): us_socket_raw_write folded -1 to 0 and re-armed writable. The kernel kept reporting writable, so the loop spun at 100% CPU (#24845), and a real reset ended as a clean close with the lost write reported as done. - SSL_write failed, or sealed records were lost to an allocation failure: the socket was marked fatal and nothing ever closed it. - SSL_write failed on a later 16 KiB record of one write: a plain partial write. A rejected send() of sealed records now ends the write side (the state our own FIN already has, so the BIO and the batch flush drop what can no longer leave) and asks for one writable event. There, outside any caller's write, the socket closes with an error, unless the peer's earlier bytes are still queued: then the read side delivers them first (a 413 sent before the server reset an upload). A fatal socket is closed at the same boundary. us_socket_write_check_error reports both: the errno of the send, or EPROTO, so node:tls fails the write like Node and Bun.connect's write()/end() return the documented -1. No new per-socket state: one int of loop scratch carries the errno from the raw send to the write that is still on the stack.
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 8, 2026
…essure (#34510, #38176, #42336) Every failed TLS write came back as "0 bytes, wait for writable": - send() rejected (EPIPE, ECONNRESET, ...): us_socket_raw_write folded -1 to 0 and re-armed writable. The kernel kept reporting writable, so the loop spun at 100% CPU (#24845), and a real reset ended as a clean close with the lost write reported as done. - SSL_write failed, or sealed records were lost to an allocation failure: the socket was marked fatal and nothing ever closed it. - SSL_write failed on a later 16 KiB record of one write: a plain partial write. A rejected send() of sealed records now ends the write side (the state our own FIN already has, so the BIO and the batch flush drop what can no longer leave) and asks for one writable event. There, outside any caller's write, the socket closes with an error, unless the peer's earlier bytes are still queued: then the read side delivers them first (a 413 sent before the server reset an upload). A fatal socket is closed at the same boundary. us_socket_write_check_error reports both: the errno of the send, or EPROTO, so node:tls fails the write like Node and Bun.connect's write()/end() return the documented -1. No new per-socket state: one int of loop scratch carries the errno from the raw send to the write that is still on the stack.
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 10, 2026
…essure (#34510, #38176, #42336) Every failed TLS write came back as "0 bytes, wait for writable": - send() rejected (EPIPE, ECONNRESET, ...): us_socket_raw_write folded -1 to 0 and re-armed writable. The kernel kept reporting writable, so the loop spun at 100% CPU (#24845), and a real reset ended as a clean close with the lost write reported as done. - SSL_write failed, or sealed records were lost to an allocation failure: the socket was marked fatal and nothing ever closed it. - SSL_write failed on a later 16 KiB record of one write: a plain partial write. A rejected send() of sealed records now ends the write side (the state our own FIN already has, so the BIO and the batch flush drop what can no longer leave) and asks for one writable event. There, outside any caller's write, the socket closes with an error, unless the peer's earlier bytes are still queued: then the read side delivers them first (a 413 sent before the server reset an upload). A fatal socket is closed at the same boundary. us_socket_write_check_error reports both: the errno of the send, or EPROTO, so node:tls fails the write like Node and Bun.connect's write()/end() return the documented -1. No new per-socket state: one int of loop scratch carries the errno from the raw send to the write that is still on the stack.
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 10, 2026
…ps, WebSocket, SQL) (#44618) ### What does this PR do? Consolidates the open TLS pull requests into one. Each was reproduced on `main` and, for `node:*` behavior, on Node v26.3.0 first. About a third are ported as written, the rest are rewritten smaller or merged into one fix where several PRs patched the same cause. One commit per fix, so it can be read commit by commit. Fixes #43520, fixes #31396, fixes #43635, fixes #37193, fixes #43846, fixes #17932, fixes #41061, fixes #36887, fixes #31810, fixes #35240, fixes #32234, fixes #44365, fixes #43807, fixes #42280, fixes #44517. Addresses #41856 (SNI and `servername`; not `checkServerIdentity` for SQL), #24845 (the spin is gone, shown with fault injection on Linux; not run on macOS), #19754 (node-fetch forwards the agent's TLS options; the Kubernetes client itself was not run). #### The ones that matter most | | On `main` | PRs | |---|---|---| | Client certificate disclosure | `https.request()` with a client certificate sends it to a server it then refuses (wrong name, `checkServerIdentity`, `destroy()` in `'secureConnect'`, `terminate()` in `handshake`). A server can force it with a junk record behind its Finished | #43946 | | False `authorized` | Over a Duplex, `secureConnect` with `authorized === true` for a peer that failed the key proof; `secureConnect` for a plaintext peer with `rejectUnauthorized: false` | #44422, #32929 | | Cleartext https | `https.createServer()` without a usable key/cert answers plain HTTP | #41672, #33539 | | Revoked client certificates | An https mTLS server never sees `crl`, so a revoked client is `authorized` | #41641 | | Pooled sockets | Requests with different client certificates or CAs share an `https.Agent` socket and session | #42498 | | Silent plaintext | `tls: [...]` given to `Bun.listen` / `Bun.connect` is plain TCP | #41490 | | Server weakened by a client knob | `NODE_TLS_REJECT_UNAUTHORIZED=0` turns off a server's client-certificate enforcement | #35245 | | Pins never checked | `WebSocket` never calls `tls.checkServerIdentity` and ignores `tls.serverName` | #41648 | | `verify-full` dropped | `PGSSLMODE=verify-*` is lost next to a `TLS_*` URL variable; `tls: true` sends no SNI | #44498 | | Crashes | use-after-free from `destroy()` in `ALPNCallback` over a Duplex; `abort()` on a late `setSession()`; SIGABRT in `fetch` with an https proxy from the environment and a `Bun.file()` body | #44462, #41671, #44458 | | Stream corruption | A TLS `write()` can lose 16 KiB it reported as written while another socket on the loop is stalled | #44529 | | Hangs and spins | 100% CPU on a failing `send()`; a fatal `SSL_write` leaves the socket open forever; `idleTimeout` never sheds a TLS client that ignores `close_notify` | #34510, #38176, #42336 | | Wrong certificate (regression since 1.3.14) | Connections accepted before `stop()` / `close()` get the default certificate and skip their entry's `requestCert` / `ca` | #42355 | | Quadratic Duplex / proxy tunnel | Reading one chunk over a Duplex, CPU: 8 MB 0.88 s → 0.14 s, 16 MB 3.18 s → 0.23 s, 32 MB 11.75 s → 0.39 s; `fetch` upload through CONNECT: 1.7 s → 0.18 s (debug build) | #44464 | #### By area - **fd engine, write path** (`openssl.c`, `socket.c`): #42352, #34510 + #38176 + #42336 as one change, #44529, #44458, #44192. A rejected `send()` ends the write side only and closes at the next writable event unless the peer's bytes are still queued (a 413 sent before a reset is still read). No new per-socket state. Also, on kqueue, **a FIN no longer ends a socket that waits in the low-priority queue** (`loop.c`): with more than 5 TLS handshakes at once, a client that ended right after its handshake could be reset and its server socket report `socket hang up`, because the eof that the sentinel read knote reports was acted on ahead of the unread Finished. That is on `main` too (the macOS entry for `node-tls-server.test.ts` in `test/flaky-tests.txt`: 7 of 48 recent builds of other branches), and this branch made it likelier (6 of 8 builds), since Finished now leaves in one segment with the close_notify. - **Error reporting, both engines**: #44422, #32929, #44516, #37094, #41272 + #42324 + #44223 as one change, #44021, #37472, #43946, #33630. One channel: a fatal error on an established session is reported, then **the engine closes the connection itself**, whatever the owner does with the report. `test/js/bun/net/tls-fatal-error-closes.test.ts` asserts closed-and-nothing-delivered for every owner (node:tls, `Bun.connect`, `Bun.listen`, `fetch` direct and through CONNECT, `Bun.serve`, `WebSocket` direct and through a proxy, Postgres, MySQL, Valkey, Duplex). - **Duplex engine** (`SSLWrapper`, `UpgradedDuplex`): #44462, #43529, #42332, #44464. #43877 + #44394 were in and are **out again**, see "Worth a look" 5. - **node:tls wrap lifecycle** (`net.ts`, `tls.ts`): #38007, #38058, #38028 + #38122 + #38076 as one change (six copies of the attach code become two helpers), #38311, #39008, #38154, #42340 + #42343 + #42339 + #42453 as one change, #43791, #42425, #44085, #42683, #39088, #39040, #40375, and what was still real of #36534. - **SNI, ALPN, server contexts**: #43080, #42050, #37195 + #43849 as one change (**one** SNI matcher for TCP and HTTP/3), #42355, #42285, #33253, part of #37896, part of #37013. A `tls.Server` has one `SSL_CTX`. - **Verification and options**: #44738, #41490, #37005 + the cwd pin of #40984, #31811, #43982, #33483 + #35245, #41810, #32235, #44441, #38092. - **node:tls API and CA store**: #41671, #38145, #32824, #43594, #39997, #41696, #33534, #34748, #42991, #42996, #42970. - **node:https, Agent, `ws`, node-fetch**: #41672 (https half), #41641, #38261, #42498, #44346, #35609, #31397, #42325. - **WebSocket client**: #41648, #37487 + #43048 as one change. - **SQL, Redis**: #33666, #41711, #44498, part of #42054. - **Tests only**: #41426, #40040, #44395, #44016, #37860, #40591, #44440, #41424. Found on the way and fixed here: an upload that a TLS 1.2 server interrupts with a renegotiation never completes on `main` (0 of 32 runs over `https.request`, `fetch`, `node:tls` and `Bun.connect`: the renegotiation ClientHello lands inside an application record that is still unsent, or the socket gets no `drain` again) and completes here, with two tests from robobun; the fix for #40653 (final flight and first write in one segment) stopped working whenever another TLS socket on the loop was stalled, on `main` too; the `tls.Server` prototype pinned the last server constructed and every `SSL_CTX` it owned; `Object.create(process.env).NODE_TLS_REJECT_UNAUTHORIZED = "0"` turned verification off process-wide once a `SHARE_ENV` worker existed; two debug panics when wrapping a shut-down or still-connecting socket; a `fetch` POST through a proxy sent its headers twice when the origin renegotiated; `BlockList` ignored IPv6 zone ids; a test now ties `root_certs.der` to `certdata.txt`. #### Behavior changes - **A server's `ca` without `requestCert` no longer asks for a client certificate** (`Bun.serve`, `Bun.listen`, HTTP/3, node:tls). It matches the docs and Node. On `main` such a server refused clients with no certificate but served any unrelated self-signed one, so it was never authentication. **Set `requestCert: true` to require a certificate.** A matrix test pins that `requestCert: true` still refuses no certificate and an untrusted one on 8 kinds of server, TLS 1.2 and 1.3, with `NODE_TLS_REJECT_UNAUTHORIZED` unset and `0`. - `NODE_TLS_REJECT_UNAUTHORIZED=0` no longer relaxes a server. - `Bun.connect` / `Bun.listen` hear of a fatal TLS error after the handshake through `error(socket, err)`. With no `error` handler the socket just closes. - HTTP/3 server names match like TCP: `*.` covers exactly one label, case is ignored, a trailing dot is ignored, the last registration of a name wins. - `requestCert` on node:https is `=== true`, as in Node. - An array where a generated options dictionary is expected throws (`tls: []`, `jest.useFakeTimers([])`). - `key` / `cert` arrays serve every identity. A client that can use both gets ECDSA, where `main` served whichever pair came last. - `ecdhCurve` is forwarded by node:https, `ws` and node-fetch now, so a group BoringSSL lacks (`X448`) throws there as it already does in `tls.createServer`. - A wrapped socket's error is re-emitted on the TLS socket as in Node, so `raw.destroy(err)` with a listener on `raw` only is uncaught, as in Node. - `sql.options.tls` is always an object, never `true`. `RedisClient` sends SNI. - `tls: { secureContext }` alone asks for TLS on `Bun.listen` / `Bun.connect` (it was plain TCP), and a value that is not a `SecureContext` throws. The context is served as it is: the `requestCert` / `rejectUnauthorized` it was created with hold whatever the options next to it say, and `requestCert` in the options over a context that does not ask throws at `listen()`. - `tls.DEFAULT_CIPHERS` reaches every client once assigned (`fetch`, `WebSocket`, `Bun.connect`, `RedisClient`, `Bun.SQL`, `S3Client`, proxy tunnels) and servers again. A list that selects no cipher throws `ERR_SSL_NO_CIPHER_MATCH` at the assignment. `fetch.preconnect()` dials nothing after an assignment. - The warning for an unreadable `NODE_EXTRA_CA_CERTS` is Node's one line, without the `warn:` prefix. - `BUN_CONFIG_WS_CLOSE_TIMEOUT` (default 30 s): how long a `WebSocket` client waits for the server to close the connection after the closing handshake. #### Worth a look in review 1. **#44529**: the kernel-refused remainder of a TLS write moves from the loop's one slot onto the connection (in the existing rare struct), so the write BIO never refuses a sealed record. Nothing is allocated on an unstalled path (200 writes: 0 appends, same `send()` count as `main`), memory with 16 stalled writers is lower than on `main` (276 KB vs 340 KB, which `main` holds inside BoringSSL's buffers), `us_socket_t` stays 80 bytes. It needs a bound on how long a deferred close waits, or a peer that stops reading pins the fd past `destroy()`: `US_SSL_CLOSE_AFTER_SPILL_TIMEOUT` is a fixed 10 s, not re-armed on progress. Separate commits, but the fix that keeps the client certificate off the wire beside a stalled socket builds on them. 2. **The default name check of node:tls also runs inside the handshake**, so a wrong-name server gets no client certificate on TLS 1.2 either. JS still runs it after every successful handshake, so a difference between the two matchers can only refuse. Error objects are byte-identical. 3. **#44441** widens trust by design: a self-issued leaf whose `keyUsage` lacks `keyCertSign` (`dotnet dev-certs`) is its own anchor when the store holds a byte-identical copy. No BoringSSL change. Expired pin, same subject with another key, wrong EKU and a pinned intermediate are tested to fail. 4. **#32235** only adds Ed25519 and ECDSA P-521 to the verify list. A captured ClientHello shows `main`'s list with the two inserted; `rsa_pkcs1_sha1` stays. 5. **A stream that a TLS socket wraps, when that TLS socket closes.** An earlier state of this branch lost data here while CI was green (found by #44709's report): with the peer closing first, 4 of 8 MiB arrived with TLS in TLS, 4 of 32 MiB on the http2 `emit("connection")` path, and a `write()` with no `'error'` listener ended the process. Three Node-parity changes only hold together: destroying the wrapped stream at the close (#38028 + #38122 + #38076, #38154) is safe only if every write has really completed (#43877), which in turn needs Node's handling of the peer's close_notify, which needs half-open sockets that the GC can collect. So: - #43877 + #44394 are reverted and reopened. A write over a stream completes once the stream has taken the ciphertext, as on `main`. - Until the verdict on the peer lets the session through, the application cannot have written over it. There the wrapped stream is destroyed as in Node, with the sessions below it. That keeps the release of the connection after a failed handshake, a rejected certificate and an early `destroy()`. The same for an http2 socket the application never got, and for `resetAndDestroy()`. - After that it is `main`'s teardown: a `net.Socket` only gets the engine's `end()`, closes at its peer's FIN, keeps its own timeout and reports its own errors. Any other stream is destroyed with the TLS socket. The regular suites cannot see any of this (999 files were green on every broken variant), so it was steered by eleven seeded differential fuzzers run on this build, `main`, Node v26.3.0 and the earlier state: close, `end()`, `destroy()`, `destroySoon()`, resets, hung and half-open peers, paused writers, timeouts, two and three sessions deep, over TCP and over Duplexes, before, at and after the handshake, and http2 requests. See "How did you verify". #### Known limits - `fetch` with a `checkServerIdentity` function still sends the client certificate (not the request) to a server the function refuses. On TLS 1.2 any verdict a JS callback gives is too late, as in Node. - `addContext()` / `SNICallback` still do not apply to a server-side socket on the stream engine (`emit("connection", duplex)`, TLS in TLS, unflushed writes, named pipes), as on `main`. - A CA bundled in a pfx extends an explicit `ca` only, for `ws` / node-fetch / `WebSocket`: the native `ca` can only replace the default store, and that store keeps `SSL_CERT_FILE` / `SSL_CERT_DIR`. - P-521 leaves work on TLS 1.3 only. TLS 1.2 needs secp521r1 in every ClientHello (`it.todo`). - Once `tls.DEFAULT_CIPHERS` is assigned, `fetch(url, { protocol: "http3" })` is `HTTP3Unsupported`, as with an explicit `ciphers`. - `addCACert()` by hand does not extend the chains of a context with several identities. - A throwing `ALPNCallback` sends `no_application_protocol` on both engines. Node sends nothing and its client sees `ECONNRESET`. - TLS in TLS, peer FIN while the outer handshake runs: the inner socket gets one `write EPIPE`, where Node gives `ECONNRESET` (`main` gives it no error at all). - `@SECLEVEL` in `ciphers` is dropped by the `ws` / node-fetch shims, which used to ignore `ciphers`. node:tls keeps throwing `ERR_SSL_INVALID_COMMAND`. - Beside a stalled TLS socket only the first record (16 KiB) of the first write leaves with the handshake flight. The rest goes record by record, which is what bounds the memory of stalled writers. - After a fatal error on an established session the socket emits `'error'` and then `'close'`. Node emits `'error'` and leaves the socket open. - A paused reader whose own write the kernel rejects loses what it had not read yet, with an `EPIPE`, as on Node. `main` reports no error there and delivers it. - On `main` too: a `Bun.listen` socket without `allowHalfOpen` that has unsent ciphertext when the client's `shutdown()` arrives loses that ciphertext (32 KiB), and over plain TCP `end()` with the peer still sending is a close over unread input, so a reset. - Differences from both `main` and Node that the differential runs below found and that stay, all with a peer that aborts: `ECONNRESET` instead of a clean `'end'` after the socket's own `'finish'` when the peer destroyed with unread data; under TLS 1.2, a zero-length `write()` followed by `destroy()` in `'secureConnection'` leaves the client without `'secureConnect'` (a plain `destroy()` there matches Node); a TLS 1.2 client that destroys in `'secureConnect'` gets no `'session'`; a `ClientRequest` whose handshake fails with an alert emits `'error'` and `'close'` but no `'finish'` (`writableFinished` is true). - Once `tls.DEFAULT_CIPHERS` is assigned, `fetch.preconnect()` opens nothing: `fetch()` then uses a context of its own, and a socket warmed under the default one would never be picked up. - A TLS `send()` that the kernel refuses outside a `write()` call (the drain of unsent ciphertext) is reported with the close, as `read EPIPE` / `read ECONNRESET`. Node says `write EPIPE`. `main` does not report it at all. - Once the application has a session over a `net.Socket` (TLS in TLS, http2 `emit("connection")`), a peer that never sends its FIN holds that socket after the TLS socket closed, as on `main`. Node destroys it. Two tests of #38154 are `todo` for this. Closing it any earlier (at its `'finish'`, say) makes the kernel drop what it has not sent yet as soon as the peer's close_notify arrives. - Plaintext that was queued on a socket before it was wrapped (STARTTLS with a backlog) is dropped when the TLS socket is destroyed, or its handshake fails, before the session is accepted. Node drops it too, except on `destroySoon()`. `main` sends it. - Over a stream that is no `net.Socket`, `end()` can still cut what that stream has buffered, and there is no backpressure, both as on `main` (#43877). - `tls.secureContext` (the undocumented door node:tls uses) is not read by a Windows named pipe listener, which builds its context from the options. On `upgradeTLS({ isServer: true })` the options next to it are the policy, as with Node's `SetVerifyMode`. - `selectServerName()` rebuilds the name tree per ClientHello for injected sockets of a server with `addContext()` entries: 0.4 µs for 1 entry, 3.7 µs for 10, 41 µs for 100, against 631–1111 µs for a handshake. #### Not included Left open, because they need a decision or are not TLS: #43877 + #44394 (see "Worth a look" 5; #43874 stays open with them), #38548, #38591 (both shrink who is trusted), #41589 (`verify-full` vs `NODE_TLS_REJECT_UNAUTHORIZED=0`), #37197, #41706, #43216, #33487, #33545, #36707, #32435, #37255, #28691, #40275, #30314 (features), #38120 (needs the BoringSSL fork, as did #33517, which the stale bot has closed since), #38529 (needs a Windows measurement), #34342, #38232, #43089, #44454, #40451, #42710, #44527, #38088, #38093, #41898. #37896, #42054 and #37013 stay open for the halves not taken. `http.createServer({ key, cert })` keeps serving TLS on purpose. One open question: `tls: {}` (an object that names no TLS option) is plain TCP on `Bun.listen` / `Bun.connect`, here and on `main`. It is the same trap as `tls: []`, but changing it changes a Bun default, so it is left alone. ### How did you verify your code works? - Every new test fails on `main` for the stated reason and passes here, except guards that pin existing behavior, each shown to fail when its clause is removed. `node:*` tests also pass on Node v26.3.0; the few that cannot say which Node version has the behavior. - 212 test files that touch TLS, sockets, http, http2, fetch, WebSocket, SQL, Valkey and workers: 6154 pass, 2 fail. Both are seen on `main` too: `serve.test.ts` "root range port" (the box runs as root), and `worker_threads.test.ts` "terminate(): nothing of the worker's runs after the request", which is flaky there and passed in the run below. - 58 of those files the way the ASAN lane runs them (LeakSanitizer + `BUN_JSC_validateExceptionChecks`): 58 files, 48 of them with leak checking, 4132 pass, 3 fail. All three also fail on `main`: `serve.test.ts` "root range port", `node-net.test.ts` "should not leak when connect({path}) fails synchronously on a reused handle" (times out under this environment), `worker_threads.test.ts` "process.exit() with a shell cp in flight" (a `ShellCpTask` leak). - 647 vendored `test-tls-*`, `test-https-*`, `test-net-*`, `test-http2-*`: the only two failures also fail on `main`. - The SNI matcher was diffed against both old matchers: 3 seeds × 1.23 M lookups × 3 registration flavours, every difference in one of the intended classes, TCP and HTTP/3 identical on every lookup. - The headline rows were also driven by hand with scripts against this build, `main` and Node v26.3.0: cleartext https, `crl`, `tls: []` / `{ secureContext }`, the `ca` / `requestCert` matrix, the client certificate on a wrong-name server, late `setSession()`, `destroy()` in `ALPNCallback`, `[rsa, ec]` identities with an intermediate from `ca`, `WebSocket` `checkServerIdentity`, a corrupted record, the Duplex read above, `tls.DEFAULT_CIPHERS`. - The `setSession()` guard was checked against the real `abort()` at 43 handshake states. - `bun run rust:check-all`: 12 of 12 targets. `tsc`, oxlint, source lints, prettier, rustfmt, mordant clean. - usockets' `_Nonnull` is compiled out of debug builds, so 105 of those files were also run on a local release ASAN build with the CI runner's environment (92 with leak checking): 4595 pass, 1 fail, `child_process.test.ts` "spawn reports EPERM after dropping privileges", which cannot pass as root and fails on `main` too. - The close of a TLS socket over another stream ("Worth a look" 5): eleven seeded differential fuzzers, 8,424 scenarios compared, each run on a release ASAN build of this branch, on `main`, on Node v26.3.0 and on the earlier state of the branch. Against `main`: - Data that `main` delivers in full is cut in 5 scenarios, and about 150 that `main` cuts arrive in full. Of the 5, in 2 `main` never notices the peer's close and keeps the socket for good, 2 call `end()` on the middle one of three sessions over an in-memory Duplex, and 1 does the same on Node. - No dead timeout, no silent reset and no uncaught error that `main` does not have (4 uncaught errors fewer). - A socket stays open where `main` closes it in 109, and closes where `main` keeps it in 295. 92 of the 109 do the same on Node or on the earlier state (a `destroy()` that an in-memory Duplex does not show its peer, half-open peers). 14 wait for a peer that paused reading and so does not read the FIN (#42332's backpressure, as in Node); the socket's own timeout fires there. 3 are left: one on a 5 ms timer, two with three sessions over an in-memory Duplex. - The earlier state of the branch cut data in 173 of the 400 scenarios of one of them, where `main` cuts none and this cuts none. - 23 new tests pin what they found. Each earlier attempt at this fix fails the ones that describe it, the earlier state of the branch fails 7, and all pass on Node. - After that change: 999 test files on the release ASAN build (20,246 pass; the 11 files that fail need a database, Docker, DNS or a non-root user, or share a temp directory with a parallel run and pass alone), 61 on the debug build. - TLS over a file descriptor (`openssl.c`, the path of `fetch`, `Bun.serve`, `tls.connect`, `Bun.connect`) got the same treatment after the rebase: seeded differential fuzzers on CI's release build of this branch, on `main` and, for `node:*`, on Node v26.3.0. Every runtime also against itself for the noise floor, injected faults and known bugs of `main` as positive controls, and a difference counts only if it shows in 5 of 5 fresh processes. - `node:tls` over TCP: 11,500 scenarios (one connection with Node as the oracle line by line; 2 to 60 connections beside stalled neighbours; raw peers that break the handshake). HTTPS: about 136,000 runs over `Bun.serve` + `fetch`, `node:https`, `node:http2` and `wss://`, also with the two ends in different runtimes. `Bun.connect` / `Bun.listen` / `upgradeTLS`: 11,500 scenarios and 720 slow connections, with writers driven by what `write()` returns, beside up to 6 stalled, dripping, closing or resetting neighbours, and plain TCP as a second oracle. No crash, hang, duplication, reordering or silent truncation, and no change in time or in connection reuse. - They found six things that `main` does better, none of which any test showed. All are fixed, each with a test that fails on the build before: what the peer sent lost behind a rejected `send()` (23 scenarios, and an early HTTPS response lost with only `EPIPE`), the same silently for a paused reader, `server.close()` never calling back on a half-open server after a ClientHello and a reset (17), `closeAllConnections()` taking 12 s with a stalled client, `end()` losing up to 1.3 of 4 MiB that `write()` had reported while the peer still uploads, and `end()` a little after a stall never closing beside other stalled TLS sockets. The last two fixes also deliver the 1 to 2 MiB that `main` loses there, and close the socket that `main` keeps for good without such neighbours. - All of them again after every fix, on CI's release build of it. That caught one regression of a fix itself (a reader stopped for backpressure lost 86,385 bytes, 1 of 6,000 scenarios), fixed too. On the last build: scenarios that lose data where `main` does not 23 → 2, and Node loses it in both, with the same `EPIPE`; `server.close()` that never calls back 17 → 0; connections held 4 → 0; requests that end in an error only where `main` has a response 6 → 0. With a Node server in another process, a request ends in an error only in 8 and 10 of 1,500 scenarios here, 5 and 3 on `main`, 10 with Node as the client. - `Bun.connect` / `Bun.listen` on the last build against `main`, in scenarios: hangs 0 against 1,031, sockets and fds never released 0 against 965, corrupted data 0 against 345, `abort()` 0 against 26 (`setSession()` after the handshake), writers that never close 0 against 101 of 720 connections. No kind of failure shows here and not on `main`. About a third of the slow connections close later than on `main`, in 1 to 16 s instead of at once, waiting for unsent ciphertext or for the peer's close_notify, and 79 more of them deliver all that `write()` reported. RSS and time with 16 to 256 stalled writers are the same. - What they found that `main` does worse: a `WebSocket` that calls `close()` with sends pending loses messages in 81 of 999 scenarios (0 here), 37 server sockets left open, 10 `server.close()` that never call back, 20 write callbacks that never run. - The kqueue fix cannot be run on Linux. The `connectionListener` count test now says what became of a missing connection, which is how the cause was found (`'tlsClientError'` "socket hang up", then `read ECONNRESET` at the client of the same port, after its `'secureConnect'`). On macOS x64 it failed every attempt of the three builds before the fix and passed at the first attempt of the build with it. - Windows and macOS were only run by CI. Four new tests asserted what only the Linux kernel does (a FIN read ahead of a reset, unread bytes surviving a reset, loopback buffer sizes, `fstat()` on a socket) and now say so per platform. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #24845
What does this PR do?
Problem
us_socket_raw_write(andus_socket_write,us_socket_raw_writev,us_socket_write2,us_socket_ipc_write_fd) treated any-1frombsd_sendas a partial write: setlast_write_failed, re-arm the writable poll, return 0. No errno classification. The SSL ciphertext spill path routes throughus_socket_raw_write:so a
send()that keeps returning an errno on a socket the kernel still reports writable pins a core at 100%.Observed on macOS as idle long-lived processes (holding a TLS keepalive connection) simultaneously spinning in
us_internal_ssl_on_writable -> __sendto, with the kernel logging (sosend -> _os_log_internal) inside every send call. That is consistent with a Network Extension content filter returning an errno on a flow it has not closed:send()fails,SS_CANTSENDMOREis never set, so noEV_EOFarrives to end it.The plain-TCP node:net write path was already bounded because it uses
us_socket_write_check_error, which has the classification this PR extends to the other helpers.Reproduction
Fault-injection repro (Linux and macOS, debug build): arm
sendto return-1 EPIPEforever on a TLS client fd after the handshake, issue one write.macOS
sampleof the spin (ciabatta, debug build)Process at 128-131% CPU; a bounded run of 500000 injected failures was silently retried in 2.7s and the data delivered once the fault disarmed; the socket never errored or closed.
Fix
Factor the errno classification from
us_socket_write_check_errorintous_internal_on_short_write()and call it from everyus_socket_*write*helper. A peer-gone errno (EPIPE,ECONNRESET,ENOTCONN,ETIMEDOUT, ...), or an unclassified errno that persists past the existing bounded retry window, latches a pending close on the socket; the writable dispatch inloop.ccloses the socket with that errno after the handler returns. Would-block, transientENOBUFS/ENOMEM, and bounded unclassified errnos still re-arm as before.The deferred close is carried by the existing pending-action mechanism (previously four separate
ssl_shutdown_after_spill/ssl_close_after_spill/ssl_pending_detach/ssl_pending_close_codefields, folded here into onepending_actionenum +pending_codebyte), sostruct us_socket_tdoes not grow.Relation to #34487 / #34498
Same code neighborhood (socket's writable poll never settling) but a different bug: those address half-open TLS shutdown / Windows level-triggered DISCONNECT re-arm; neither touches send-errno handling in the raw write helpers.
How did you verify your code works?
New tests in
test/js/bun/net/socket-syscall-fault.test.tsinject a sustainedEPIPE, a sustained unclassifiedEINVAL, and 50 sporadic one-offEPROTOTYPEs on a TLS client fd after the handshake and assert the socket closes with the errno for the sustained cases (and stays open for the sporadic-with-recovery case).Ran
test/js/node/tls/+test/js/bun/net/with the fix vs without; the delta is exactly the new tests, no regressions. Also passing: the existing h2/WS/Bun.serve fault-injection suites.Possibly related: #32600 (plain HTTP on Linux, profiler points at JS microtask churn rather than the C-level writable spin; not claiming it here).
no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/socket-syscall-fault.test.ts