node: fix the genuine-hang class — MessagePort loop starvation, pinned sockets, https handshakeTimeout, TLS alert errors (+2 tests) - #35538
Draft
cirospaciari wants to merge 20 commits into
Conversation
…or messages The merge of claude/node-v26-permission-wave2 dropped the reject_bad_negations field from the ParseOptions initializer in Arguments.rs (added by the cli negation-errors commit), breaking the build, and resolved three node_fs.rs call sites back to the pre-parity generic 'path must be a string' errors, orphaning PathOrFdExt::from_js_required. Restores the field and the from_js_required calls; deletes BUFFER_EXPECTED_TYPES, superseded by throw_invalid_argument_type_list at its only former call site.
The 'Merge origin/claude/node-v26-combined-34660' merge (de14ef6) resolved src/runtime/cli/Arguments.rs and src/js/node/util.ts wholesale to the pre-merge side, silently reverting the combined branch's work: - Arguments.rs lost the -e/-p/--print value-binding rewrite (including every 'eval.provided = true' write, whose consumer in mod.rs survived), so 'bun -e code' printed the help text and exited 0 — breaking every subprocess spawned with -e. Also lost: --check/-c, --input-type, --inspect-port/--debug-port parsing, NODE_OPTIONS validation, and the --no-<flag> negation errors. Restored via a proper 3-way merge against the original merge base, keeping the later permission-wave2 hunks. - util.ts lost util.diff() (its myers_diff backend survived unreferenced). readline.ts was also resolved to one side, but its dropped hunks are superseded by the newer promises rework already on this branch; no change needed there.
A same-thread postMessage ping-pong (port1.on('message') re-posting to
port2) never let timers run: the drain task reschedules itself with an
ordinary postTask, and EventLoop::tick() drains the task queue until
empty including tasks enqueued by the tasks it runs, so the rescheduled
drain executed in the same tick forever. Node yields here via
uv_async_send in MessagePort::OnMessage — the continuation runs on the
next loop iteration, after due timers.
Adds that primitive: EventLoop::next_loop_iteration_tasks, promoted into
the task queue only at tick() entry, folded into the poll-timeout and
loop-liveness checks, and reclaimed by the existing per-tag shutdown
path. Exposed to C++ as ScriptExecutionContext::postTaskNextLoopIteration.
The batch-limit reschedules in MessagePortPipe::drainAndDispatch and
Worker::drainToWorker/drainToParent now use it. Fixes the infinite
re-post loop in test-worker-message-port-infinite-message-loop (the
setTimeout that closes the port now fires).
newDetachedSocket (node:net's pre-connect handle) materialized its JS wrapper through get_this_value, which unconditionally took a Strong ref 'until the socket is closed / marked inactive' — but a handle destroyed before its connect starts (DNS still resolving, or aborted between attempts) never runs either, so the wrapper was pinned forever. A net.connect()+destroy() churn leaked every such socket (and its timeout timer via the callback closure); test-gc-net-timeout stalled at 65/193 collected. get_this_value now takes only a Weak ref when the handler is fully detached — the JS side's socket._handle reference keeps the wrapper alive, matching the reuse-prev reconnect path — and the existing connect upgrades (connect_finish, mark_active) still pin it once native events become possible. Vendors test-gc-net-timeout (upstream v26.3.0), which now passes: 193/193 sockets collected.
https.createServer delegates to the native node:http server, whose connection notification only fired after the TLS handshake completed — a raw TCP client that never handshakes was invisible to JS, so options.handshakeTimeout was ignored and no 'clientError' ever fired; test-https-timeout-server hung until the generic idle timeout. - uws HttpContext::onOpen now runs the connection filters with value 2 at raw TCP accept for node-compat TLS contexts (value 1 still fires at handshake completion), and the server's filter thunk forwards both. - node:http's onServerConnection creates the socket at accept (matching node, where 'connection' fires pre-handshake and 'secureConnection' after), arms an unref'd handshake timer (default 120s, validateNumber, like lib/internal/tls/wrap.js), and on the second notification emits 'secureConnection' and clears the timer. - On deadline the server emits 'tlsClientError' with ERR_TLS_HANDSHAKE_TIMEOUT; TLS servers install node's https.Server 'tlsClientError' -> 'clientError' mapping (destroying the connection when unhandled), so conn._secureEstablished is observable as false in the handler. Vendors test-https-timeout-server (upstream v26.3.0), passing 3/3 runs.
A fatal TLS alert received after the handshake completed (e.g. a TLS1.3 server's certificate_required when the client sent no certificate) was swallowed: the parked-OpenSSL-reason pipeline only existed for handshake failures, so the socket just closed and node:tls clients saw a bare 'close' (or a synthesized ECONNRESET) where Node reports ERR_SSL_TLSV1_ALERT_CERTIFICATE_REQUIRED via the socket 'error' event. - openssl.c: ssl_park_fatal_reason now parks unconditionally; ssl_on_data's fatal branch hands the reason string to on_close when the handshake had completed, closing with CONNECTION_RESET so the graceful shutdown path cannot defer the close and drop it. Handshake-time reasons keep flowing through ssl_trigger_handshake as before. - NewSocket::on_close builds a JS Error from the handed reason (copied before entering JS; the pointer is the loop's scratch). - net.ts decomposes the OpenSSL string into node's library/function/reason/ERR_SSL_<REASON> shape in one shared helper (decorateOpenSSLError, extracted from tlsHandshakeError) applied in both close sinks, so the error keeps its identity instead of the ECONNRESET synthesis. Covered by the new node-tls-server.test.ts case (fails on the unfixed build with the client reporting nothing / ECONNRESET). The upstream test-tls-client-auth now gets the correct codes in all four stanzas but still hangs at exit on a separate native loop-ref leak under concurrent TLS pairs, so it is not vendored here.
Collaborator
Contributor
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
…aude/node-v26-hang-fixes # Conflicts: # src/runtime/cli/Arguments.rs
…b_worker.rs
The merge of origin/claude/callback-throw-uncaught dropped the
`parent_ref` local binding in WebWorker::create but left one caller;
switch it to the surrounding `unsafe { (*parent) }` pattern.
The same merge took main's deletion of BunHeapProfiler.h (503b01c)
without noticing this branch's v8.ts now $newCppFunction()s into
BunHeapProfiler.cpp, so the JS2Native codegen #includes a header that no
longer exists. Reinstate the header with the three host-function decls.
…aude/node-v26-hang-fixes # Conflicts: # src/jsc/bindings/BunHeapProfiler.h
…aude/node-v26-hang-fixes
robobun
changed the title
… socket timer on destroy
Main removed the constant as unused; the TLS accept notification handling here tells the handshake-complete callback (opened == 2) apart from the accept by it.
This was referenced Sep 10, 2026
Closed
dylan-conway
added a commit
that referenced
this pull request
Sep 18, 2026
…e it connects (#42295) ### Problem - A `node:net`/`node:tls` socket destroyed before its connect starts keeps its `TCPSocket`/`TLSSocket` wrapper (and its `net.Socket`) alive forever. 1000 `net.connect()` + `destroy()` per round leave 1001, 2001, 3001 `TCPSocket` objects after full GC. `destroy()` during the DNS lookup, an early `AbortSignal`, and a `connect()` that throws leak the same way. - So does every failed name lookup: 100 `net.connect({ host: "does-not-exist.invalid" })` per round leave 101 then 201 `TCPSocket` on stock bun, 1 then 1 with this change. A reconnect loop during a DNS outage grows without bound. - Cause: `new_detached_socket` (`src/runtime/node/node_net_binding.rs`) creates the wrapper through `get_this_value`, which holds it strong. Only a connect attempt's teardown downgrades that reference. A handle that never connects reaches no teardown: `_handle.close()` on a detached socket is a no-op. ### Fix - The fixing line: `new_detached_socket` now creates the idle handle with `JsRef::init_weak`. `net.Socket._handle` owns it until a connect starts. - `connect_finish` already upgraded the reference for each attempt. That moves into `NewSocket::this_value_for_connect`, and the Windows named-pipe connect arms now call it too (they did not upgrade before). - Correct because the reference is now strong exactly while native work is pending, like other `JsRef` users (`Bun.spawn`, Valkey) that start weak. - Verified: new test in `test/js/bun/net/socket-retention.test.ts` fails on stock bun 1.4.3-canary (101 `TCPSocket` / 26 `TLSSocket` retained, `protectedObjectTypeCounts` 100 / 25) and passes on the debug ASAN build (1 / 1, protected 0 / 0). It asserts both the object counts and that no `TCPSocket`/`TLSSocket` is held by a strong handle. Other suites in the notes, Linux and Windows. ### Related - Supersedes "Fix 2: never-connected sockets pinned forever" (`a812bee39a`) in draft #35538. Same root cause; that commit makes `get_this_value` take a weak reference whenever the socket is detached. This PR instead makes the weak reference explicit at the one creation site and leaves `get_this_value` unchanged, and `this_value_for_connect` also pins the two Windows named-pipe connect arms, which an `is_detached()` gate in `get_this_value` would leave weak during `uv_pipe_connect`. That hunk can be dropped from #35538 once this lands. - #34441 recorded the symptom: upstream `test-gc-net-timeout.js` not collecting all of its sockets. - Upstream `test-gc-net-timeout.js` (the test `a812bee39a` vendors) is not added here. It passes with this change on every lane of build 114199 except alpine 3.23 x64, where it timed out in 4 of 4 attempts. It passes here on glibc x64 with a release build of this branch: 5 of 5 runs, 16 of 16 pinned to 1, 2, 4 and 8 CPUs, and with `localhost` forced to two addresses where the first one fails. It observes collection through timing (a socket leaks only when its 10 ms timeout wins the race with the connect), like the tests #35182 deleted. The guard that fails on stock bun every time is the `socket-retention.test.ts` test above. ### Not covered - `destroy()` from inside a `'connectionAttempt'` listener (#42307). net.ts nulls `_handle` before `kConnectTcp` runs, so `doConnect(null, ...)` allocates a fresh native socket that nothing owns, and it stays strong-rooted and connected. Separate root cause in `src/js/node/net.ts`, reproduces the same on stock bun, not changed here. - Windows named pipes: a `destroy()` before the pipe open completes (traced statically only). The second named-pipe path found here, the attempt ref that a synchronous `WindowsNamedPipeContext::connect`/`open` failure never released (#42309), is fixed on main by #42317. This branch has main merged in, and both changes are in the two named-pipe arms of `Listener::connect_inner`. - No test fails if the two named-pipe arms in `Listener::connect_inner` go back to `get_this_value`. The upgrade there is what keeps a first `node:net` pipe connect alive across `uv_pipe_connect` when JS drops the socket; the Windows leak tests listed below check the opposite direction (nothing stays pinned). - Nothing asserts that `this_value` is strong where a native attempt starts (`do_connect`, `WindowsNamedPipeContext::connect`/`open`). A `debug_assert!` there would make every debug net test check it; left for a follow-up. ### Background - `NewSocket<SSL>` (`socket_body.rs`) is the native side of a `TCPSocket`/`TLSSocket`. Its `this_value: JsRef` is `Strong` (a `StrongRootBlock` root) or `Weak`. The wrapper's finalizer derefs the native socket. - `Socket.prototype.connect` calls `newDetachedSocket()` and stores it in `_handle` before the lookup and the `process.nextTick` that starts the connect. `doConnect(_handle)` later reaches `connect_finish` with it as `prev`. - A detached handle has no `us_socket_t`, so closing it dispatches nothing native. <details><summary>Notes</summary> - Heap snapshot (`generateHeapSnapshotForDebugging`) of the retained cells before the fix: each `TCPSocket` has exactly one incoming edge, `Internal` from `StrongRootBlock`, which is a GC root. No JS retainer. `heapStats().protectedObjectTypeCounts` counts the same strong handles, which is what the test asserts on. - Scenarios measured on stock bun 1.4.3-canary (200 sockets each, count after GC): immediate destroy 201, `tls.connect` + destroy 201 `TLSSocket`, destroy during lookup of `localhost` 201, destroy in the `'lookup'` event 201, `AbortSignal` abort 201, `connect()` throwing `ERR_SOCKET_BAD_PORT` 201. Sockets that connect and then close (destroy after `'connect'`, server-side destroy, `end()`, ECONNREFUSED) do not leak: 1. The constant 1 is the class's prototype object, which `heapStats()` counts under the same name. - With the fix all of the above report 1 on the debug ASAN build, and the reporter's 1000-per-round loop stays at 1. - The terminal paths that downgrade a connect attempt's reference: `mark_inactive` after `on_close`, `handle_connect_error`, and the semi-socket branch of `close()`/`terminate()`. - What keeps an unreferenced `net.Socket` alive before its connect starts: the `process.nextTick` closure (literal IP) or the lookup callback (hostname) captures it. Probes: 200 `net.connect()` calls with no JS reference kept, `Bun.gc(true)` forced between creation and the deferred connect and on a 1 ms interval. All 200 connect, receive data, and close (also with `BUN_GARBAGE_COLLECTOR_LEVEL=2`). The same probe with `host: "localhost"` (with and without `autoSelectFamily`) and with a custom async `lookup`: 100 of 100 each. - The test lives in `socket-retention.test.ts` because that file is the guard for the `JsRef` retention model and already drives it through `node:net` in a child process. - Suites run on the Linux debug ASAN build: `test/js/bun/net/socket-retention.test.ts`, `test/js/bun/net/socket.test.ts`, `test/js/node/net/node-net.test.ts` (the 10 failures left are environment failures that stock bun shows too: `localhost` resolution and no outbound DNS), `handle-leak.test.ts`, `net-mongodb-pattern-leak.test.ts`, `double-connect.test.ts`, `socket-reconnect-live.test.ts`, `connect-autoselectfamily-stale-timer.test.ts`, `node-tls-connect.test.ts`, `node-tls-upgrade.test.ts`, 53 `test-net-*` and 38 `test-tls-*` Node parallel tests. - Windows x64 debug build: the new test (fails on stock bun there too, 101), `should work with named pipes` (700 pipe connections, `TCPSocket` count check), `should not leak when connect({path}) fails asynchronously while polling for a pipe`, the `Bun.connect` named-pipe lifecycle tests in `socket.test.ts`, and `node-tls-namedpipes.test.ts` (400 TLS pipe connections, `TLSSocket` count at most 3) pass. The Windows runs predate the `protectedObjectTypeCounts` assertion and the comment trims, which change no compiled code. - `cargo check --target x86_64-pc-windows-msvc` passes for the `#[cfg(windows)]` arms. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/net/socket-retention.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
This branch has not been deployed
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
2 participants
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 from the "genuine hangs" slice of the true-gap triage — each of these was a test (and a user program shape) that never exits. Base:
claude/node-v26-fix-tls. Two upstream v26.3.0 tests vendored byte-verbatim, one bun-side regression test added. Now two upstream files:test-worker-message-port-infinite-message-looplanded on main separately in #37075, so it is no longer in this diff.Base-branch repairs (blocking everything else)
The mega branch did not compile, and
bun -ewas entirely broken on it. Two commits repair evil-merge damage:de14ef6099(merge of combined-34660) resolvedsrc/runtime/cli/Arguments.rsandsrc/js/node/util.tswholesale to the pre-merge side. That reverted the whole-e/-pvalue-binding rewrite — everyeval.provided = truewrite vanished while its consumer inmod.rssurvived, sobun -e codeprinted the help text and exited 0. Every subprocess-spawning test on the branch failed from this. Also dropped:--check/-c,--input-type,--inspect-port/--debug-port, NODE_OPTIONS validation,--no-<flag>negation errors, andutil.diff()(itsmyers_diffbackend survived unreferenced). Restored via a proper 3-way merge against the original merge base, preserving the later permission-wave2 hunks.reject_bad_negationsfrom aParseOptionsinitializer (build break) and regressed threenode_fs.rscall sites to pre-parity generic error messages, orphaningPathOrFdExt::from_js_required. Restored; deletedBUFFER_EXPECTED_TYPES, genuinely superseded bythrow_invalid_argument_type_listat its only former call site.Fix 1: MessagePort/Worker message drains starve timers (vendored:
test-worker-message-port-infinite-message-loop)A same-thread
postMessageping-pong never let timers run: the drain task's batch-limit reschedule used an ordinarypostTask, andEventLoop::tick()drains the task queue until empty including tasks enqueued by the tasks it runs — so the rescheduled drain executed in the same tick, forever. Node yields here viauv_async_sendinMessagePort::OnMessage: the continuation runs on the next loop iteration, after due timers.The fix adds that primitive:
EventLoop::next_loop_iteration_tasks, promoted into the task queue only attick()entry, folded into the poll-timeout and loop-liveness checks, and reclaimed by the existing per-tag shutdown path. Exposed to C++ asScriptExecutionContext::postTaskNextLoopIteration, used by the batch-limit reschedules inMessagePortPipe::drainAndDispatchandWorker::drainToWorker/drainToParent(same class).Fix 2: never-connected sockets pinned forever (vendored:
test-gc-net-timeout)newDetachedSocket(node:net's pre-connect handle) materialized its wrapper throughget_this_value, which unconditionally took a JSC Strong ref "until closed / marked inactive" — but a socket destroyed before its connect starts (DNS still resolving) never runs either, so the wrapper (and its timeout timer, via the callback closure) was pinned for process lifetime. The test stalled at 65/193 collected; heap-snapshot roots showed 128TCPSockets held byStrongHandles.get_this_valuenow takes a Weak ref when the handler is fully detached — the JS-sidesocket._handlereference keeps the wrapper alive, matching the reconnect-reuse path — and the existing connect-time upgrades (connect_finish,mark_active) still pin it once native events are possible. 193/193 collected after the fix.Fix 3:
handshakeTimeouton native https servers (vendored:test-https-timeout-server)https.createServerdelegates to the native node:http server, which only notified JS of a connection after the TLS handshake — a raw TCP client that never handshakes was invisible,options.handshakeTimeoutwas ignored, and noclientErrorever fired. Now uws'sHttpContext::onOpenruns the connection filters with a distinct value at raw TCP accept for node-compat TLS contexts;onServerConnectioncreates the socket at accept (matching node's pre-handshake'connection'), arms an unref'd handshake timer (default 120s,validateNumber, perlib/internal/tls/wrap.js), emits'secureConnection'and clears the timer on the completion notification, and on deadline emits'tlsClientError'withERR_TLS_HANDSHAKE_TIMEOUT. TLS servers install node'shttps.ServertlsClientError→clientErrormapping (destroy when unhandled).Fix 4: post-handshake fatal TLS alerts swallowed (new test in
node-tls-server.test.ts)A fatal alert received after the handshake completed (e.g. a TLS1.3 server's
certificate_requiredwhen the client sent no certificate) surfaced as a bare close / synthesized ECONNRESET; Node reportsERR_SSL_TLSV1_ALERT_CERTIFICATE_REQUIREDon the socket'error'event. The parked-OpenSSL-reason pipeline only existed for handshake failures; it now parks unconditionally,ssl_on_data's fatal branch hands the reason toon_close(closing with CONNECTION_RESET so the graceful-shutdown path can't defer and drop it), the Ruston_closecopies it into a JS Error, and net.ts decomposes it into node'slibrary/function/reason/ERR_SSL_<REASON>shape via one shared helper. New test fails on the unfixed build.This makes all four stanzas of upstream
test-tls-client-authproduce the correct error codes, but that test still hangs at exit on a separate native loop-ref leak (all sockets/servers closed,getActiveResourcesInfo()empty, loop refcount nonzero — reproducible with its stanza 3+4 pairs running concurrently), so it is not vendored.Slow-list disposition (rest of the slice)
test-inspector*,test-esm-loader-hooks-inspect-*,test-runner-inspect,test-debugger-pid): hang awaiting the/json/listdiscovery endpoint. The entire CDP stack lives on the combined-34719 lineage where node: inspector/debugger v26 compat fixes (+17 tests) #35493 already landed that fix; this base has none of it (debugger.tshas/json/listas a TODO). Porting would duplicate a PR chain — left to the inspector lineage.test-http-server-multiple-client-error: needs external-socket adoption viaserver.emit('connection', socket)— owned by open PR node:http: adopt external sockets fed in via server.emit('connection') #35285.test-perf-hooks-eventlooputilization: ELU implementation is in node:worker_threads: per-thread --use-system-ca, real eventLoopUtilization, --cpu-prof in workers, node's online timing, error.code / stack-getter / timeOrigin fixes, async_hooks WORKER resource, worker_threads dc channel (+10 upstream tests) #34424.test-async-hooks-http-parser-destroy: needs async_hooks init/destroy for HTTPINCOMINGMESSAGE/HTTPCLIENTREQUEST resources (async_hooks resource-type gap, worker_threads: MESSAGEPORT async_hooks init, worker-visible warnings, data: URL module formats (+4 tests) #35366 lineage).test-http-regr-gh-2928:HTTPParser.prototype.consume()is a TODO stub inJSHTTPParserPrototype.cpp(http-parser lineage, http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432).test-http2-pack-end-stream-flag: needsPerformanceObserver'http2'entries (Http2SessionwithframesReceived).test-heapsnapshot-near-heap-limit-worker:setHeapSnapshotNearHeapLimit+ worker OOM snapshots unimplemented.test-tls-async-cb-after-socket-end: needs server-side legacy TLS1.2 session cache (newSession/resumeSessionevents with async done-callback;SSL_CTX_sess_set_get_cbis not wired).test-stack-size-limit: not fixable at this layer — Bun executes CJS/-ein strict mode, so JSC's proper tail calls turn(function explode(){ return explode(); })()into a genuine infinite loop instead of a RangeError (V8 has no PTC). Needs sloppy-mode CJS execution.Verification
Each vendored test fails (times out) on the unmodified base binary and passes 3/3 runs on this branch, CI-style (debug build,
bunfig.node-test.toml, flagless). Preflight OK (byte-verbatim, expectations.txt untouched, no silent passes).Regression suites on the final debug build:
test/js/node/tls245/245;test/js/node/httpfailure set identical to the base binary (6, pre-existing); workers structured-clone 231/231;test/js/node/netper-file sweep clean (three files that tripped a shared-machine-load timeout pass standalone);test/js/web/workers372/373 — the one failure (worker-terminate-lifetime"terminate() while dns.lookup() is in flight", an ASan leak report for the worker's fsBinding) reproduces identically on a build of the base plus only the merge-repair commits, i.e. it is pre-existing on this base under debug+ASan and untouched by the feature commits here.