Repository navigation
fix(tls): separate transport destruction and graceful shutdown - #144
Merged
Merged
Conversation
steipete
changed the title
steipete
marked this pull request as ready for review
October 8, 2026 11:43
steipete
force-pushed
the
claude/tls-duplex-destroy-parity
branch
from
October 8, 2026 11:57
a1c3a45 to
06726b4
Compare
Match Node 24 destruction for Duplex-backed TLS while keeping graceful shutdown separate. Retain adopted-fd close ownership and release HTTP/2 injected transports when their TLS proxy is destroyed. Adapt HTTP/2 lifecycle coverage from oven-sh#38154. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
Keep adopted-fd ownership on the TLS socket after its raw handle detaches. Separate peer EOF, graceful writable shutdown, and full TLS destruction; wait for transport completion without forwarding its error a second time. Use the inherited tls.Server connection path for injected HTTP/2 sockets instead of maintaining a second TLS transport adapter. Port HTTP/2 destroy-versus-close and final event ordering from oven-sh#38195, and the destroyed-socket EOF guard from oven-sh#43392. Retain the six-case destruction regression and add Node 24 controls for half-open replies, raw EOF, renegotiation shutdown, and transport ownership. Synchronize conformance assertions with the sessionError event itself. Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
Continue draining encrypted output after close_notify while the transport remains open. Add a Node 24 parity case that writes outside the receive callback and waits for peer receipt before ending, so shutdown cannot mask a missing flush.
steipete
force-pushed
the
claude/tls-duplex-destroy-parity
branch
from
October 8, 2026 12:16
5589417 to
5da4013
Compare
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
1 participant
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.
READY for coordinator merge. Final head:
5da4013bc4c8fd0a5a802776fb667833059adbf8.Destroying a TLS socket over a supplied Duplex must destroy its transport without reading or calling
end(). Graceful shutdown remains separate. This change preserves adopted transport ownership after native-handle detachment, distinguishes peer close-notify from full native close, and retains the writable half of half-open connections.HTTP/2 session destruction now releases its accepted transport and emits terminal session events after socket closure. The duplicate
_http2_upgrade.tsadapter is removed:Http2SecureServeruses the inheritedtls.Serverconnection listener. A full tracked-tree search finds no remaining references to the removed module or helper. The internal module registry and build tables are generated from source discovery.node-http2-upgrade.test.mtsdirectly covers injected TLS sockets, ALPN, requests, TLS options, disconnects and transport release;h2-conformance.test.tsseparately covers shared session and wire behavior over h2c.The unrelated resolver options refactor was extracted into #149, merged separately, and is absent from this PR's diff. This head is rebased onto main
9e640c399bb42c4b102be1e89a95ce659d112c43; the existing VM, resolver and TLS changelog entries are all preserved.Independent P1 review caught an additional half-open write bug: the TLS engine accepted data after peer close-notify but did not flush it until
end(). The fix drains pending output while the writable half remains open. Its new regression deferswrite("tail")out of the receive callback and callsend()only after peer receipt; Node 24 passes and the prior candidate fails. A proposed change to HTTP/2's writable-finish gating was rejected after Node 24 source inspection and a held-final/held-peer probe produced identical behavior. The final-head P1 review is scoped-clean.Head:
5da4013bc4c8fd0a5a802776fb667833059adbf8.publish=falseBoth native comparisons use identical final-head test inputs and verified executable hashes, with zero retries or relaxed limits. The CI merge-ref tree equals the PR head tree. The baseline executable tree equals merged main's tree. Inherited broader-suite diagnostics match after normalizing only the runtime revision. Darwin main's extra failure was
ERR_SSL_WRONG_VERSION_NUMBERin the unchanged PFX-only context replacement case; it is retained as a baseline observation, not claimed as a separate fix.This builds on oven-sh#42350, ports relevant session lifecycle behavior from oven-sh#38195 and the destroyed-EOF guard from oven-sh#43392, and retains regression credit for oven-sh#38154. Thanks @robobun. Receiving server-initiated renegotiation is covered; Bun-initiated
renegotiate()remains the existing documented unsupported API. No event-loop changes from #142 are included.The Windows fd-offset deadline tracked in #148 did not recur in this final run. Its timeout and test inputs are unchanged; this result does not claim to fix the intermittent. No merge, tag, release or repository settings change was performed as part of this preparation.