node:tls: inherit allowHalfOpen from the wrapped socket by robobun · Pull Request #39066 · oven-sh/bun

robobun · 2026-08-15T13:21:09Z

Problem

new tls.TLSSocket(socket, options) and tls.connect({ socket }) hard-code allowHalfOpen: false on the TLS socket (src/js/node/tls.ts:738), whatever the wrapped socket was created with; tls.connect({ socket, allowHalfOpen: true }) conversely turns it on. Node takes the wrapped socket's own value and only honors the option when the TLS socket opens its own connection (wrap.js#L592; connect() passes options.socket as that argument, #L1756-L1757).
Effect: a STARTTLS-style server, net.createServer({ allowHalfOpen: true }, raw => new TLSSocket(raw, { isServer: true, key, cert })), gets allowHalfOpen === false sockets, which end themselves the moment the client ends, so the server cannot answer after the client's EOF. Same for sockets injected into a tls.Server (the 'connection' listener in tls.ts) and for tls.connect({ socket }).
Node 26.3.0 prints true for every wrapped half-open case below, Bun prints false (and true for tls.connect({ socket: regular, allowHalfOpen: true }), where Node prints false). Full matrix in the details block.

Fix

tls.ts: the constructor copies allowHalfOpen from the wrapped socket, taken from the first argument or from options.socket (the tls.connect({ socket }) path); with no wrapped socket the option applies as before. This is the fixing hunk; it matches Node.
net.ts: with the value inherited, a wrap over a half-open connection (every plain stream.Duplex is one by default) no longer ends itself when its transport goes away, so the connection closing underneath it has to take it down, which Node does with wrap.on('close', () => this.destroy()) (wrap.js#L739-L741). destroyWhenUpgradedCloses arms that listener at the seven upgrade sites. kCloseRawConnection (Bun's own retirement of an fd-upgraded net.Socket object when the TLS socket gets 'end') removes it first, so the peer's EOF still leaves a half-open wrap alive; when the connection's owner had already destroyed it, the pending 'close' is the real thing and stays armed. Without this hunk renegotiation.test.ts ("exceeds the renegotiation limit over a duplex socket") hangs and the four "wrapped socket closing" tests below time out.
Why the JS layer is the right place: Bun's net layer already keeps the native socket half-open and lets the Duplex's allowHalfOpen decide what happens at EOF (see the comment in kConnectTcp), and an adopted fd keeps its native flags through us_socket_adopt, so the constructor was the only point where the wrapped socket's setting was dropped.
Scope: on current main the native TLS layer still closes a node:tls socket when the peer's close_notify arrives (ssl_wants_eof_dispatch in openssl.c is uWS-only); node: tls/https v26 compat wave 2 — allowHalfOpen close_notify, fetch setDefaultCACertificates, https.Server setSecureContext/keylog/TLSSocket (+6 tests) #35535 extends it to node:tls sockets. This PR fixes the stream layer (the property, no automatic end(), teardown on transport close). With the flag forced to false, node: tls/https v26 compat wave 2 — allowHalfOpen close_notify, fetch setDefaultCACertificates, https.Server setSecureContext/keylog/TLSSocket (+6 tests) #35535 would not have applied to wrapped sockets at all; with both, a half-open wrap also gets its late writes delivered.
The existing test/js/node/tls/node-tls-socket-allow-half-open-option.test.ts asserted false for new TLSSocket(new Duplex(), { allowHalfOpen: true }); Node returns true there (a Duplex defaults to allowHalfOpen: true), so those assertions encoded the bug. The file now asserts the "option is ignored" property in both directions, plus the cases below.
Verified with test/js/node/tls/node-tls-socket-allow-half-open-option.test.ts (property matrix for both construction forms, server wrap / injected tls.Server socket / tls.connect({ socket }) staying writable after the peer's EOF, a regular wrap still ending itself, and the four upgrade paths being destroyed when the wrapped socket or duplex closes):
- bun bd test with src/ stashed: 9 of 11 fail (the 2 that pass are the unchanged-behavior contracts); with the diff: 11 pass.
- with only the tls.ts hunk: the 4 "wrapped socket closing" tests time out.
- test/js/node/tls/renegotiation.test.ts: 8 pass.
- all 245 upstream test-tls-* / test-https-* files in test/js/node/test/parallel pass on the debug build.
- test/js/node/tls/, test/js/node/net/, test/js/node/http2/, test/js/node/http/: the remaining failures (localhost resolving to a different family than the listener, and 5s timeouts of subprocess tests on the debug+ASAN build) reproduce without the diff.

Background

allowHalfOpen is a Duplex option. When it is false, the stream calls end() on itself once its readable side ends (the peer's EOF), so the socket closes by itself; when it is true, the application decides when to end its own side. net.Socket defaults to false, stream.Duplex to true.
A "wrap" is TLS over an already established socket (STARTTLS). The two public forms are new TLSSocket(socket, ...) and tls.connect({ socket }). Bun performs either by adopting the connection's fd into the TLS engine (a net.Socket with a handle) or by running the engine over the stream itself (a generic Duplex, TLS over TLS, or a socket with unflushed plain writes); net.ts has one upgrade site per form and path, seven in total.
In net.ts, this[kupgraded] is the TLS socket's pointer to the connection it was upgraded over. On the fd path, kCloseRawConnection runs on the TLS socket's 'end' and destroys the original net.Socket object, which no longer owns the fd; that destroy emits a 'close' on the connection that has nothing to do with the transport.

Node vs Bun outputs

Property matrix (node v26.3.0 / bun 1.4.0, before this change):

                                                                    node   bun
new TLSSocket(net.Socket{allowHalfOpen:true}, {allowHalfOpen:false}) true   false
new TLSSocket(net.Socket{}, {allowHalfOpen:true})                    false  false
new TLSSocket(Duplex{}, {allowHalfOpen:false})                       true   false
new TLSSocket(Duplex{allowHalfOpen:false}, {allowHalfOpen:true})     false  false
new TLSSocket()                                                      false  false
new TLSSocket(undefined, {allowHalfOpen:true})                       true   true
tls.connect({socket: net.Socket{allowHalfOpen:true}, allowHalfOpen:false}) true   false
tls.connect({socket: net.Socket{}, allowHalfOpen:true})              false  true
tls.connect({socket: PassThrough, allowHalfOpen:false})              true   false
socket injected into tls.createServer() from net.createServer({allowHalfOpen:true})  true  false

Server wrap over a half-open raw socket, client ends after the handshake, server writes from its 'end' handler:

node:  raw.allowHalfOpen=true tls.allowHalfOpen=true / server end writableEnded=false / server late write cb ok / server finish / client end got="late" / client close
bun:   raw.allowHalfOpen=true tls.allowHalfOpen=false / server end writableEnded=false / client end got="" / client close

(The "late" bytes reaching the client additionally needs #35535; with this PR alone the wrap stays open at the stream layer and closes when the application ends it.)

Close propagation with the diff (TLS socket's events; raw close on the EOF line is Bun's existing retirement of the wrapped object):

                              node                          bun (this PR)
peer EOF, half-open raw       tls end                       tls end | raw close
peer EOF, regular raw         tls end | raw close | tls close   same
raw.destroy(), half-open raw  raw close | tls close         tls end | raw close | tls close
underlying duplex closes      tls end | ... | tls close     tls end | ... | tls close

Before the net.ts hunk, the last two rows never reached tls close.

new TLSSocket(socket) and tls.connect({ socket }) forced allowHalfOpen to false on the TLS socket; node takes the wrapped socket's own value and only honors the option when the TLS socket opens its own connection. With the value inherited, a TLS socket over a half-open connection (any generic Duplex defaults to allowHalfOpen: true) no longer ends itself when the peer ends, so the connection closing underneath it has to take it down the way node's wrap 'close' listener does. Arm that at every upgrade site; the internal retirement of an fd-upgraded net.Socket on 'end' is excluded so a half-open wrap still survives the peer's EOF.

coderabbitai · 2026-08-15T13:21:17Z

Warning

Review limit reached

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

Next review available in: 6 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: 0dadaa4f-03b3-4f9d-a6e7-fede7cc75269

📥 Commits

Reviewing files that changed from the base of the PR and between 88a6398 and ad7dd0d.

📒 Files selected for processing (3)

src/js/node/net.ts
src/js/node/tls.ts
test/js/node/tls/node-tls-socket-allow-half-open-option.test.ts

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

robobun · 2026-08-15T13:22:01Z

Status: fix pushed, waiting for CI.

Reproduced on the released binary with USE_SYSTEM_BUN=1 bun test test/js/node/tls/node-tls-socket-allow-half-open-option.test.ts (every wrapped socket reported allowHalfOpen === false, and a server-side wrap over a half-open raw socket had ended itself by the time the peer's 'end' was observed) and with a debug build via git stash push -- src/ && bun bd test <same file> (9 of 11 fail; the 2 that pass are the unchanged-behavior contracts). The same scenarios under node v26.3.0 give the values the tests now assert; outputs are in the PR description.

Completeness of the net.ts half: net.ts hands a connection to the TLS engine in exactly seven places (upgradeDuplexToTLS x4, upgradeTLSDeferred x2, handle.upgradeTLS x1; four of them in Socket.prototype.connect, three in the server-side upgrade), and each is followed by destroyWhenUpgradedCloses. The only other caller of the engine binding is _http2_upgrade.ts, which does not build a TLSSocket and is untouched. Without the net.ts hunk, the four "wrapped socket closing" tests and the duplex case in renegotiation.test.ts time out. The !connection.destroyed gate in kCloseRawConnection is pinned from both sides: the "stays writable after the peer ends" tests assert destroyed: false after the retirement has run (listener removed), and the "socket being destroyed" tests need the listener to stay armed.

Follow-up commits: 2431401a51 initializes the listener slot in the constructor (review nit); d5424074b0 and ad7dd0d032 only shorten comments (the code is unchanged since 2431401a51).

Fix: #39066 (this PR).

…uctor Arm it before the 'end' retirement at the fd upgrade sites as well, so kCloseRawConnection always finds it set.

claude

Thanks for adding the kOnUpgradedClose constructor init in 2431401 — that addresses the earlier nit. I re-reviewed and found no bugs. Given this rewires TLS-wrap teardown across all seven upgrade sites in net.ts and adds a subtle interaction between kCloseRawConnection and the new close listener, a human look would still be worthwhile.

On the four comment-cop flags: those comments are Node-source citations for the compat behavior (wrap.js line refs), not workaround justifications, so they read as false positives to me — but a maintainer should make that call.

What was reviewed:

allowHalfOpen inheritance in the TLSSocket constructor for both new TLSSocket(socket, ...) and tls.connect({ socket }) paths against the Node reference.
All seven destroyWhenUpgradedCloses call sites and the kupgraded guard in onUpgradedClose.
The !connection.destroyed conditional in kCloseRawConnection — removes the listener only when the retirement's own destroy() would emit the spurious close; a real owner-initiated close stays armed.
Test file: property matrix, half-open-stays-writable, and the four wrapped-socket-closing cases; error events wired to reject, resources released in finally.

Extended reasoning...

Overview

The PR changes allowHalfOpen handling for wrapped TLS sockets to match Node.js: a TLSSocket built over an existing socket/duplex inherits that socket's allowHalfOpen instead of hard-coding false (or honoring the caller's option on the tls.connect({ socket }) path). Because a half-open wrap no longer auto-ends itself on the peer's EOF, net.ts gains a destroyWhenUpgradedCloses helper that arms a 'close' listener on the wrapped connection at all seven upgrade sites so the TLS socket is torn down when its transport goes away — mirroring Node's wrap.on('close', () => this.destroy()). kCloseRawConnection (Bun's fd-adoption retirement path) is updated to remove that listener before it destroys the retired net.Socket object, so the spurious 'close' from retirement doesn't tear down a half-open wrap; a connection its owner already destroyed keeps the listener armed. The test file is rewritten from a 2-assertion stub (which encoded the bug) into an 11-test suite covering the property matrix, live-connection behavior, and close propagation for each upgrade path.

What changed since the last review

My previous run left one nit (initialize this[kOnUpgradedClose] in the Socket constructor for shape stability). Commit 2431401 addressed it — the diff now includes this[kOnUpgradedClose] = undefined; next to this[kupgraded] = null;. No new bugs were found on this pass.

Security risks

TLS-adjacent but not a security-boundary change: no cert verification, cipher selection, or trust-store logic is touched. The change affects stream-layer half-open semantics and lifecycle teardown. The main risk class is a leaked/hung socket if the close listener is disarmed on the wrong path, or premature teardown if it fires on a spurious close — both are exercised by the new tests (four "wrapped socket closing" cases plus the "stays writable after peer ends" cases).

Level of scrutiny

Moderate-to-high. The tls.ts hunk is small and directly matches the cited Node source. The net.ts changes are the part that warrants human eyes: seven call sites, a new per-instance listener slot, and a conditional in kCloseRawConnection whose correctness depends on understanding Bun's fd-adoption retirement (a Bun-specific mechanism with no Node analogue). The PR description explains this thoroughly and the test coverage is comprehensive (property matrix in both directions, injected tls.Server sockets, all four upgrade-path close scenarios, plus the author reports 245 upstream test-tls-*/test-https-* files passing), but the interaction is subtle enough that a maintainer familiar with the net.ts upgrade machinery should confirm the seven sites are the complete set and that the !connection.destroyed gate is the right discriminator.

Other factors

The comment-cop bot flagged four multi-line comments. Reading them, they cite Node source lines for the compat behavior being matched (wrap.js#L592, #L739-L741, #L1756-L1757) rather than justifying a workaround, so they look like heuristic false positives — but that's a maintainer call and they are technically unaddressed.
The rewritten test file replaces assertions that encoded the pre-fix (wrong) behavior; the PR description justifies this against Node 26.3.0 output.
Tests follow harness conventions: port: 0, error events wired to reject the awaited promise, cleanup in finally, describe.concurrent for the independent connection tests.

… code

claude

I reviewed this PR and didn't find any bugs; the earlier nit about initializing this[kOnUpgradedClose] in the Socket constructor is now addressed. Because it changes TLS-wrap lifecycle across all seven upgrade sites in net.ts (plus the !connection.destroyed gate in kCloseRawConnection) and rewrites an existing test file's assertions, a human look would still be worthwhile.

What was reviewed:

allowHalfOpen inheritance in the TLSSocket constructor for both the positional-socket and options.socket paths — matches the cited Node wrap.js behavior.
All seven destroyWhenUpgradedCloses call sites line up with the upgrade points in Socket.prototype.connect and bunUpgradeServerTLS; the bound listener uses a named module-level function and the new symbol slot is initialized in the constructor.
The kCloseRawConnection interplay: removeListener is guarded on !connection.destroyed, and onUpgradedClose re-checks self[kupgraded] === connection before destroying — no stale-listener destroy path found.
Test rewrite: the old assertions encoded the bug (new TLSSocket(new Duplex(), { allowHalfOpen: true }).allowHalfOpen === false); the new assertions were checked against the Node v26.3.0 matrix in the PR description.

Extended reasoning...

Overview

The PR fixes Node compat for allowHalfOpen on wrapped TLS sockets. src/js/node/tls.ts changes the TLSSocket constructor to inherit allowHalfOpen from the wrapped socket (positional socket argument or options.socket) instead of hard-coding false. src/js/node/net.ts adds a 'close' → destroy() listener on the wrapped connection at each of the seven TLS-upgrade sites (destroyWhenUpgradedCloses), a new kOnUpgradedClose symbol slot, and a removeListener in kCloseRawConnection gated on !connection.destroyed so Bun's own retirement of the fd-upgraded raw object does not tear the half-open TLS wrap down. The test file is fully rewritten from a two-assertion stub (which asserted the bug) into an 11-test suite covering the property matrix, behavioral half-open tests over real connections, and four wrapped-socket-closing scenarios.

Security risks

None identified. The change is stream-lifecycle semantics, not the TLS handshake, certificate verification, or any crypto path. No security options are read or defaulted differently.

Level of scrutiny

Medium-high. node:tls / node:net socket lifecycle is a critical, subtle code path where event-ordering mistakes surface as hangs or leaked sockets. The change touches seven upgrade sites and introduces a listener whose removal depends on whether connection.destroyed was set before kCloseRawConnection runs — that interplay is well-argued in the PR description and pinned by tests from both directions, but it is not a mechanical change. The test file rewrite also replaces assertions that were previously green; the PR description justifies this against Node v26.3.0 output, and REVIEW.md permits updating tests that encoded a bug, but a human confirming the Node-parity claim is worthwhile.

Other factors

The prior review nit (constructor initialization of kOnUpgradedClose) was addressed in 2431401, and the comment-cop feedback on long comments was addressed in d542407 / ad7dd0d — all inline threads are resolved. Test coverage is thorough (property matrix, four upgrade paths, both directions of the !connection.destroyed gate), and the author reports the 245 upstream test-tls-/test-https- files pass. The bug hunter found nothing this run. Still, the seven-site lifecycle wiring and the kCloseRawConnection gate are the kind of change that benefits from a maintainer familiar with net.ts's upgrade paths signing off.

robobun · 2026-08-15T17:05:23Z

^{Updated 11:05 AM PT - Aug 15th, 2026}

❌ @robobun, your commit d542407 has some failures in Build #98274 (All Failures)

🧪 To try this PR locally:

bunx bun-pr 39066

That installs a local version of the PR into your bun-39066 executable, so you can run:

bun-39066 --bun

github-actions Bot added the claude label Aug 15, 2026

claude Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread src/js/node/net.ts

net: initialize the upgraded-close listener slot in the Socket constr…

2431401

…uctor Arm it before the 'end' retirement at the fd upgrade sites as well, so kCloseRawConnection always finds it set.