tls: deliver parked sessions before the data decrypted with them on the duplex path

[robobun]

Symptom

A TLS client running over a JS stream (tls.connect({ socket: duplex }), and the same engine behind WindowsNamedPipe) never emits 'session' when its 'data' handler destroys the socket and the server's NewSessionTicket records were decrypted in the same read pass as the response. That is the normal TLS 1.3 shape on loopback: the server's tickets go out the moment it sees the client's Finished, its response follows in the same tick, and the client reads both in one chunk. Destroying inside 'data' is what an https.Agent with keep-alive off does as soon as a response completes, so such a client can never cache a resumable session.

const raw = net.connect(port, "127.0.0.1");
await once(raw, "connect");
const client = tls.connect({ socket: new SocketProxy(raw), rejectUnauthorized: false });
client.on("secureConnect", () => client.write("x"));   // server answers any data
client.on("session", () => events.push("session"));
client.on("data", () => { events.push("data"); client.destroy(); });
await once(client, "close");
// node v26.3.0 and plain tls.connect():  secureConnect, session, session, data
// tls.connect({ socket }) before:         secureConnect, data

Today the event is delivered by accident if the handler happens to write() before destroying: the write re-enters the engine and that nested pass flushes the queue from inside the data callback (so it arrives after the data). #37467 stops nested passes from doing that, after which the accidental delivery goes away too.

Cause

BoringSSL reports new sessions and keylog lines from inside SSL_read/SSL_do_handshake, so openssl.c parks them on the SSL and the engine delivers them once the read has returned. On the us_socket_t path, ssl_on_data flushes the parked entries before every us_dispatch_data (buffer full, end of the read loop, close_notify) plus once at the tail. SSLWrapper (src/uws/lib.rs) only flushed after the whole read loop in handle_traffic, and after the data on the close_notify branch. When the data callback destroys the socket, UpgradedDuplex::on_close -> teardown -> deinit frees the SSL, and SSL_free frees the parked queue with it; the later flush finds nothing.

The flush could not simply be moved in front of the dispatch: us_ssl_pop_pending_* copied each entry into a caller buffer, and the only buffer handle_reading has is the 64 KiB one holding the decrypted bytes it is about to deliver.

Fix

• openssl.c / libusockets.h: us_ssl_pop_pending_session / us_ssl_pop_pending_keylog now detach the oldest entry and return it (payload via out-params) instead of copying; us_ssl_free_pending releases it. The entry is unlinked from the SSL before the callback runs, so the callback may close the connection (and SSL_free the rest of the queue) without invalidating the bytes being delivered. These two functions have no other callers.
• SSLWrapper::flush_pending_events no longer needs a buffer. handle_reading routes its three dispatch sites (buffer full, loop exit, close_notify) through dispatch_read, which flushes the parked entries and then delivers the data, with the existing closed/deinited re-checks after each step. The tail flush in handle_traffic stays for entries parked by a handshake flight or a read that produced no application data. The flush also stops once closed_notified is set, like every other callback trigger in the wrapper and like the us_socket_is_closed check in the C flush.

This is the order node uses (its NewSessionCallback runs before the data of that read reaches JS) and the order bun's own us_socket_t path already uses; it also puts the events in wire order, since the ticket records precede the data they were decrypted with. Explicitly destroying the socket from inside a 'session' listener drops the rest of that pass, which is what the C path does as well.

Interaction with #37467 (nested handle_traffic calls stop decrypting): independent changes, both touch the tail of handle_traffic (here the flush just loses its buffer argument), so whichever lands second has a one-line merge. Until #37467 lands, a 'session'/'keylog' listener that synchronously writes to the socket during a burst larger than 64 KiB would have the nested pass deliver the following chunk first, the same nesting #37467 removes for 'data' handlers; no built-in listener writes from those events.

Verification

test/js/node/tls/node-tls-connect.test.ts, new describe("over a duplex, 'session' is emitted before the data that followed it on the wire"), one case per dispatch site plus the handshake-parked case:

• response written after the request (read-loop exit)
• response and close_notify in one flight (close_notify branch)
• 160 KiB response handed to the engine in a single pass (buffer-full branch, destroy on the first 64 KiB)
• TLS 1.2 banner server (session parked while the handshake completes, banner decrypted in the same pass)

All four fail on the released binary (0 pass 4 fail in 8 of 8 runs, each receiving ["secureConnect", "data"]) and pass on the debug build. Client and server share one event loop, so the tickets and the response always land in one read. Also ran on the debug build: the rest of node-tls-connect.test.ts, node-tls-server, node-tls-upgrade, node-tls-duplex-close-throw-uaf, renegotiation, node-tls-socket-allow-half-open-option, tls-connect-socket-churn, and the proxy-tunnel consumers of the same engine (proxy.test.ts, proxy-stress-protocol, proxy-stress-lifecycle, websocket-proxy, fetch-proxy-connect-tunnel-split-envelope; 299 pass). The one failure seen (SNICallback runs even when the requested servername matches the bind hostname) fails identically on the unmodified released binary in this sandbox, where localhost resolves to ::1 first.

[robobun]

Status

Reproduced on the released binary: tls.connect({ socket: duplex }) client whose 'data' handler destroys the socket receives ["secureConnect", "data"], never 'session', while node v26.3.0 and plain tls.connect() give ["secureConnect", "session", "session", "data"]. Four variants (read-loop exit, close_notify in the same flight, >64 KiB burst in one pass, TLS 1.2 banner) all fail 8/8 runs unfixed and pass with this PR.

CI is green on 22eeda1 (Buildkite build 92181, 191 jobs, 0 failed). Review threads are resolved. Ready for a maintainer.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f08f694f-f571-4ea4-8fbc-e9a9b3a0b0bc

📥 Commits

Reviewing files that changed from the base of the PR and between 9a0995b and 22eeda1.

📒 Files selected for processing (2)

• src/uws/lib.rs
• test/js/node/tls/node-tls-connect.test.ts

Walkthrough

The pending TLS event APIs now return detachable queue entries. TLS read dispatch drains session and keylog events before decrypted data. New duplex-stream tests cover event ordering across TLS versions, buffering, close-notify races, and client destruction.

Changes

TLS pending event dispatch

Layer / File(s)	Summary
Detachable pending-event API packages/bun-usockets/src/crypto/openssl.c, packages/bun-usockets/src/libusockets.h, src/uws/lib.rs	Pending session and keylog APIs now return opaque entries with payload pointers and lengths. Callers release entries with us_ssl_free_pending.
TLS read and event dispatch src/uws/lib.rs	Read and close paths dispatch pending session and keylog events before decrypted data. Dispatch stops when callbacks close or deinitialize the SSL wrapper.
TLS event-ordering validation test/js/node/tls/node-tls-connect.test.ts	Duplex TLS tests verify session-before-data ordering across TLS 1.2, TLS 1.3, buffered responses, close-notify races, and client destruction.

Possibly related PRs

• oven-sh/bun#37094: Modifies TLS read and handshake dispatch paths in src/uws/lib.rs.
• oven-sh/bun#37467: Coordinates pending-event delivery and TLS traffic dispatch in src/uws/lib.rs.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main TLS duplex change: delivering parked sessions before decrypted data.
Description check	✅ Passed	The description explains the symptom, cause, fix, implementation details, and verification results in sufficient detail.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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/tls/node-tls-connect.test.ts`:
- Around line 818-826: Update the event-order assertion in the “response written
after the request” test to inspect the full events array rather than calling
distinct(events). Assert that the last “session” event occurs before the first
“data” event, preserving tolerance for multiple TLS tickets while detecting any
ticket delivered afterward; remove the now-unneeded distinct helper.

🪄 Autofix

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: 087cb7fd-b46b-400b-81ed-5ac2c0893678

📥 Commits

Reviewing files that changed from the base of the PR and between da3851e and 9a0995b.

📒 Files selected for processing (4)

• packages/bun-usockets/src/crypto/openssl.c
• packages/bun-usockets/src/libusockets.h
• src/uws/lib.rs
• test/js/node/tls/node-tls-connect.test.ts

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks FFI memory ownership for the parked-session queue (openssl.c ↔ SSLWrapper) and reorders callback dispatch on a re-entrant TLS path, a human look would still be worthwhile.

What was reviewed:

• Confirmed us_ssl_pop_pending_* has no callers outside src/uws/lib.rs; the C ssl_on_data path uses its own ssl_flush_pending_session and is unaffected by the API change.
• Traced drain_pending ownership: entry is unlinked before deliver runs, scopeguard frees it exactly once, and closed_notified/ssl.is_none() gate the next iteration so a callback that tears down the wrapper cannot reuse a freed ctx or double-free the queue.
• Checked FFI signatures match (*mut *const u8/*mut usize vs const unsigned char **/size_t *; uint32_t length widens to size_t).
• CodeRabbit's assertion-strength concern was addressed in 289dae2 (expectSessionsThenData asserts the exact sequence); comment-cop threads were resolved in 22eeda1.

Extended reasoning...

Overview

The PR fixes event ordering on the tls.connect({ socket: duplex }) path so that 'session' (and 'keylog') fire before the application data that was decrypted in the same SSL_read pass, matching Node and Bun's own us_socket_t path. To do this without a scratch buffer, it changes us_ssl_pop_pending_session/_keylog from copy-into-caller-buffer to detach-and-return-entry (with a new us_ssl_free_pending), then routes all three dispatch sites in SSLWrapper::handle_reading through a new dispatch_read that flushes parked entries first. Four new tests cover each dispatch site plus the TLS 1.2 handshake-parked case.

Security risks

The change is in TLS session/keylog delivery ordering, not verification, key handling, or crypto primitives. The main risk class here is memory safety across the C↔Rust boundary under re-entrancy (a JS callback closing the socket mid-drain). I traced that: each popped entry is unlinked from the SSL before the callback runs, so SSL_free (via deinit) freeing the remaining queue cannot touch it; the scopeguard frees it exactly once after deliver returns; and the closed_notified check on the next loop iteration prevents dispatching into a torn-down owner. No new user-controlled input reaches allocation sizes or bounds.

Level of scrutiny

High. This is a non-trivial refactor of FFI memory ownership plus callback re-ordering on a path where callbacks are known to synchronously tear down the engine (the PR description itself explains the UAF that motivated the parking design). The change is well-executed, but it is not a mechanical or config-level change — it belongs with a human reviewer familiar with the SSLWrapper/UpgradedDuplex lifetime model and the pending #37467 interaction the PR calls out.

Other factors

• The changed C API has no other callers (grep confirms only src/uws/lib.rs uses us_ssl_pop_pending_*; the us_socket_t path uses ssl_flush_pending_session which is untouched).
• flush_pending_events now snapshots handlers once instead of per-iteration; only shutdown_read mutates handlers and it touches on_data alone, so this is behaviorally equivalent.
• All prior bot feedback is resolved: the test assertion was strengthened to the exact [secureConnect, ...sessions, data] sequence, and the flagged inline comments were trimmed to doc-comment form.
• The PR notes a one-line merge with #37467; whichever lands second should re-verify the tail-flush hunk in handle_traffic.

[Jarred-Sumner]

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.