Repository navigation
Conversation
…ealed record The write BIO of a usockets TLS socket refused a record that the kernel did not take while another socket owned the loop's spill slot. BoringSSL had already sealed that record and kept it for a retry with the same bytes, and us_internal_ssl_writev returned a count without it. The next write of other data then sent the stale record and lost as many accepted bytes, and a smaller next write failed with SSL_R_BAD_WRITE_RETRY. Each TLS connection now has its own queue for ciphertext that the kernel did not take, and the BIO takes every record. The spill slot of the loop is removed. The batching rule stays: while one connection holds the rest of a batch, the other sockets send record by record. A close_notify that the kernel does not take is queued, and the FIN follows it. A close that waits for queued ciphertext ends at the socket's timeout.
Collaborator
Author
|
Updated 6:08 AM PT - Oct 3rd, 2026
❌ @robobun, your commit 4f60a4f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 44529That installs a local version of the PR into your bun-44529 --bun |
Collaborator
Author
|
Status of #44529: draft. The self-review has not finished yet. How the problem was reproduced:
|
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
…s every record it sealed (#44529) While another TLS socket owned the loop's spill slot, the write BIO refused what the kernel did not take. BoringSSL had already sealed that record and kept it for a retry, and us_internal_ssl_writev did not count it. The next write of other data flushed the old record and dropped as many accepted bytes (silent stream corruption); a shorter one, or any write after setMaxSendFragment(), failed with BAD_WRITE_RETRY and the socket went fatal with no event. The BIO now takes every record. Only what the kernel refused is copied, to the connection (us_ssl_rare_t), and freed when it drains. The loop-wide rule stays: while one connection holds the rest of a batch flush nobody batches, so userspace holds one flush unit per loop plus at most the rest of one record per other stalled connection. That record used to sit, whole, in BoringSSL's own per-connection write buffer. 16 stalled writers on one loop: 340378 B before (94288 slot + 15 x 16406), 275968 B + 76 B per connection now. Nothing is allocated or copied while the kernel keeps up: 200 writes of 64 KiB, 203 send() calls and 0 spills, as before. us_ssl_rare_t 56 -> 64 bytes, loop_ssl_data 392 -> 368, us_socket_t 80 -> 80 (one bit of its padding). Gone with the slot: its owner, the relocation hook for it, and the "slot is another socket's, so this connection dies" branch. A close now waits behind the spill of every stalled connection, not only the slot owner's (16 peers that stop reading, destroy() on all: 9 fds stay open, 1 before). The next commit bounds that wait. tls-low-prio-queue-fixture.ts: a refused flight no longer leaves the next byte unread, so the burst that fails the parked handshakes is a fatal alert.
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.
Problem
Bun.connect/Bun.listenTLSwrite()can report N bytes while one more sealed record stays inside BoringSSL, not counted. The next write of other data sends that record and loses as many accepted bytes. A smaller next write fails withSSL_R_BAD_WRITE_RETRYand never drains.BIO_s_custom_write(packages/bun-usockets/src/crypto/openssl.c) refuses a record the kernel does not take. BoringSSL already sealed it, andus_internal_ssl_writevdoes not count it.Fix
test/js/bun/net/socket.test.ts(5 fail without the change), and the TLS fault-injection and backpressure suites.Background
send(). The spill slot held the part the kernel refused, one socket per event loop.Downsides
us_ssl_rare_tgrows from 56 to 64 bytes per accepted TLS connection.Notes
Reproduction (linux x64 loopback, release
1.4.3-canary.1+367d939d9and a debug build of main)Two TLS clients on one loop. The first fills the kernel and stays stalled. The second calls
write(1 MiB)until a write is short, then writes other data.write()did not report, and 16384 accepted bytes never arrivewrite()returns 0, nodrain, the socket is marked fatal with no eventsetMaxSendFragment(512)Mechanism
do_tls_write(BoringSSLssl/s3_pkt.cc) seals the record before it flushes. When the BIO refuses, it keeps the record andpending_write, and expects a retry with the same bytes.SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER, so a retry with another buffer of the same size or more flushes the old record and reports that many bytes of the new buffer as written. A smaller buffer fails withSSL_R_BAD_WRITE_RETRY.node:net,node:tlsandBun.servekeep the unsent remainder and retry exactly it, which is what BoringSSL expects.What changes besides the count
SSL_shutdown,SSL_do_handshakeandSSL_readwrite through the same BIO. Their records (close_notify, handshake flights, alerts) are queued too. The dropped close_notify branch inssl_handle_shutdownis gone: the alert goes out when the kernel takes it, then the FIN.loop.c,us_socket_resume).Bun.connectsocket then stayed open. When the holder armed no timeout and bytes are still unsent, the close arms 10 s, the default idle timeout ofBun.serve. The timeout wheel ticks every 4 s, so that is 8 to 12 s, and each drain progress starts it again.SSL_ERROR_WANT_WRITEarms of the handshake and read drivers withssl_read_wants_write, the foreign-slot kill inssl_flush_write_batch, the spill re-point inus_internal_ssl_socket_relocated.Worst case held in the queue: one flush unit (at most about 147 KB) for the one connection per loop that holds the rest of a batch, and the rest of one record (16 KiB of plaintext) plus a handshake flight or an alert for any other. Main held the same: the record sat inside BoringSSL. No record of the other drivers is unbounded: BoringSSL sends one KeyUpdate reply per write of ours, and client renegotiation is limited to 3 per 600 s.
Measurements (main
519963edc8against this branch)llvm-dwarfdump,llvm-size):us_socket_t80 to 80,us_ssl_rare_t56 to 64,loop_ssl_data384 to 368 bytes..textofopenssl.o22423 to 23121,loop.o5983 to 6079,socket.o4882 to 4896 bytes. A release binary was not built.send()calls, gdb breakpoint hit counts (releasebun-profileof 367d939 against the debug build of this branch): 200 writes of 64 KiB with nothing stalled 204 to 204. Writer beside a stalled socket 193 to 193. Single writer that stalls 27 to 27.ssl_out_queue_append): 0 for the 200 unstalled writes. In the new test (one run): 42256 bytes for the stalled socket (rest of a batch) and 7462 bytes for the socket under test (rest of one record).Bun.serve({ tls, idleTimeout: 2 }): connections still open 30 s after the timeout, 1 of 4 to 0 of 4 (runs of 40 s: 1 on the release build, 2 on this branch).perforvalgrindin the container) and server CPU beside a stalled socket (no release build of the branch).Open question for a maintainer
The usockets rewrite (#34037) records a directive to keep one loop-shared spill slot so that memory stays O(1), and it rejects a spill per socket. This PR keeps the byte bound (the same gate), but the bytes live on the connection. If the buffer itself must stay loop-shared, the alternative is to keep the slot and add a place for one refused record per connection.
Other PRs
end(data)test on release lanes).Not covered
ECONNRESETorEPIPEis still queued, as it was spilled before (tls: report a rejected send() as a write error instead of a clean close #42336).endfor a close_notify and for a bare FIN.Tests run on the debug build (linux x64):
socket.test.ts,socket-syscall-fault.test.ts,tls-syscall-fault.test.ts,node-tls-server.test.ts,node-http-backpressure.test.ts,node-tls-connect.test.ts,node-tls-duplex-end-verify.test.ts. Before the last rebase alsonode-tls-cert,node-tls-context,node-tls-upgrade,renegotiation,node-tls-raw-end,node-tls-socket-allow-half-open-option,node-tls-wrapped-socket-close,tls-connect-socket-churn,tcp-server,socket-retention,tls-reject-before-client-cert,bun-serve-ssl,tls-keepalive,node-http-pinned-write,node-https-checkServerIdentity,fetch.tls,node-http2. Failures seen were 5 s timeouts of tests that pass when run alone on the loaded machine, and one test that needs the public internet.The low-prio queue fixture (
tls-low-prio-queue-fixture.ts) failed sockets through the refused-flight close. Its clients now send a fatal alert instead. With the double-park guard inloop.cremoved it still aborts ongroup->low_prio_count == 0.