Skip to content

node:net: let server.unref() release a Windows named-pipe listener - #37079

Merged
Jarred-Sumner merged 4 commits into
mainfrom
farm/11185e1b/windows-pipe-listener-unref
Aug 7, 2026
Merged

node:net: let server.unref() release a Windows named-pipe listener#37079
Jarred-Sumner merged 4 commits into
mainfrom
farm/11185e1b/windows-pipe-listener-unref

Conversation

@robobun

@robobun robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Repro

On Windows, a named-pipe server that has been unref()'d never lets the process exit:

const net = require("net");
const server = net.createServer();
server.listen("\\\\.\\pipe\\demo-" + process.pid, () => server.unref());
// process should exit immediately; instead it stays alive forever

Node exits promptly. server.close() works in Bun; only unref() is broken, and only for named pipes (TCP and unix-socket listeners are fine).

Cause

The named-pipe listener is the one listen path that runs on a raw libuv handle instead of usockets. uv_listen marks the uv_pipe_t active and ref'd, so libuv's own active_handles count keeps uv_loop_alive() truthy. Listener::unref only drops the Listener's KeepAlive counter; nothing ever dropped libuv's, so the loop stayed alive no matter what. usockets' libuv backend avoids this for TCP by calling uv_unref on every handle it creates and letting Bun's KeepAlive be the single source of truth (packages/bun-usockets/src/eventing/libuv.c).

Fix

WindowsNamedPipeListeningContext::listen now calls uv_unref on the pipe handle right after uv_listen succeeds, applying the same contract: the owning Listener's poll_ref is the only thing keeping the process alive, so unref()/ref() behave like Node. Accepted connections are unaffected; each one holds the loop through its own KeepAlive.

Verification

The bug is Windows-only, so the new test exercises the fix on Windows; on POSIX it covers the (already correct) unix-socket listener path and passes with or without the src change.

On Windows x64:

  • fail-before: the new test fails on unfixed main (1.4.0-canary.1+31711361d) with the child printing HUNG after 4s
  • pass-after: passes with this change (child exits in ~600ms)
  • server.unref(); server.ref() still keeps the process alive, and a live connection still holds an unref'd server open (matches Node 26.3.0 behavior for all three scenarios)
  • test/js/node/net/node-net-server.test.ts (22), test/js/node/net/node-net.test.ts (70), test/js/bun/net/socket.test.ts (73), and test/js/bun/net/named-pipe-listen-error.test.ts all pass on the debug build

On Linux: node-net-server.test.ts all 22 pass.


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/node/net/node-net-server.test.ts

uv_listen marks the listening uv_pipe_t active+ref'd, and nothing ever
stripped that ref, so after server.unref() dropped the Listener's
KeepAlive the uv handle still pinned uv_loop_alive and the process never
exited. unref the pipe handle right after listen succeeds so the
Listener's poll_ref is the only loop hold, matching what usockets'
libuv backend does for TCP handles.
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Windows named-pipe listeners now unreference their libuv handles after registration. Regression coverage checks that unreferenced pipe and Unix-domain-socket servers exit naturally without closing the server.

Changes

Listener unref lifecycle

Layer / File(s) Summary
Named-pipe registration lifecycle
src/runtime/socket/Listener.rs
Successful named-pipe registration calls uv_pipe.unref(). Listener::poll_ref remains responsible for listener liveness.
Server unref regression coverage
test/js/node/net/node-net-server.test.ts
A subprocess test uses a temporary path and verifies clean exit for unreferenced pipe and Unix-domain-socket servers within four seconds.

Possibly related PRs

  • oven-sh/bun#36181: Both changes modify Listener.rs lifecycle handling, but address different listener finalization issues.
  • oven-sh/bun#36562: Both changes adjust event-loop keep-alive behavior for unreferenced servers or listeners.
  • oven-sh/bun#36930: Both changes modify listener reference handling in Listener.rs.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly and concisely describes the main change: allowing server.unref() to release Windows named-pipe listeners.
Description check ✅ Passed The description explains the bug, cause, fix, regression test, and verification results, although it does not use the template headings exactly.

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

@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Status: fix verified on Windows (fail-before on unfixed main, pass-after with this change; ref/unref and live-connection semantics match Node 26). Previous full CI run was 195/196 green with the only red being test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts (SIGABRT on x64-asan), which is pre-existing on main and tracked separately. The cargo clippy failure was a pre-existing break on main, fixed by #37078; after the merge from main it passes here too. CI is re-running on the merge commit.

Comment thread test/js/node/net/node-net-server.test.ts Outdated
The child exits without close(), so the socket file was left behind in
the system tmpdir on POSIX. Pass the listen path in via env, which also
drops the nested backslash escaping for the Windows pipe name.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the earlier tmpdir-hermeticity note is addressed in 605a45f (socket now binds under a tempDir the parent disposes, path passed via env).

What was reviewed

  • The one-line uv_pipe.unref() after successful uv_listen — mirrors usockets' uv_unref contract in packages/bun-usockets/src/eventing/libuv.c:262; Listener::poll_ref remains the sole loop-hold so ref()/unref() round-trip correctly.
  • Checked the sibling client path: WindowsNamedPipeContext already unrefs its own uv_pipe (WindowsNamedPipe.rs:585/836), so the listener was the only gap.
  • tty.rs / ansi_renderer.rs / bake_body.rs / server_body.rs are autofix.ci formatting drift from main, no behavior change.
  • New test: subprocess with bounded HUNG-guard, concurrent pipe drain, tempDir-scoped .sock, env-passed listen path — matches harness conventions.
Extended reasoning...

Overview

One functional line: unsafe { (*this).uv_pipe.unref() } in WindowsNamedPipeListeningContext::listen (Windows-only, #[cfg(windows)]), placed after uv_listen succeeds and before the cleanup guard is disarmed. The four other .rs files are pure rustfmt/import-order changes from an autofix.ci commit picking up main drift (confirmed by the author and by inspecting the diff — whitespace and import reordering only). One new subprocess test in node-net-server.test.ts.

Correctness

The fix applies the exact contract usockets already uses for every libuv handle it creates (packages/bun-usockets/src/eventing/libuv.c:262 — "sockets have a Async.KeepAlive … used instead of the usockets internals"). The named-pipe listener is the one listen path that bypasses usockets and drives a raw uv_pipe_t, so it never got that uv_unref. After the fix, Listener::poll_ref (a KeepAlive) is the single source of truth: listen() refs it, unref() drops it, ref_() re-refs it (verified the ListenerType::NamedPipe arm passes the != None guard in ref_). Accepted named-pipe connections each activate their own poll_ref in on_name_pipe_created, so an unref'd server with live connections still holds the loop — matching the PR's Windows verification and Node's behavior. I also grepped the client-side sibling (WindowsNamedPipeContext) and it already unrefs its uv_pipe, so the listener was the only site missing this.

Security risks

None. This is event-loop ref-counting on a Windows named-pipe handle; no untrusted input parsing, no auth/crypto/permissions surface.

Level of scrutiny

Low-to-medium. The functional change is one line, Windows-only, in a well-understood lifecycle path, and mirrors an established in-tree pattern. The PR description documents fail-before/pass-after on Windows plus the unref();ref() and live-connection scenarios against Node 26.3.0, and the related test suites (node-net-server, node-net, socket, named-pipe-listen-error) all pass on the debug build.

Other factors

I previously flagged (now resolved) that the POSIX arm of the new test leaked a .sock in the system tmpdir; 605a45f moved it under a tempDir the parent disposes and passes the path via env, which also removed the nested backslash escaping for the pipe name. The test follows harness conventions: bunEnv spread, bunExe(), concurrent stdout/stderr/exited drain, stdout asserted before exitCode, bounded 4s HUNG-guard with .unref(), using/await using for cleanup. On POSIX the test covers the already-correct unix-socket path (passes with or without the src change), which is fine — the src change is #[cfg(windows)]-gated so fail-before is only meaningful on Windows, where the PR verified it.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the tempDir fix from 605a45f addresses the hermeticity concern I raised earlier.

What was reviewed:

  • Confirmed UvHandle::unref() (libuv.rs:638) resolves to uv_unref, and that usockets' libuv backend applies the same contract to its handles (packages/bun-usockets/src/eventing/libuv.c:262).
  • Checked accepted named-pipe connections already unref their uv handle in WindowsNamedPipe.rs (start() at :836, on_connect at :585), so the listener was the one gap.
  • Test: pipes drained concurrently, tempDir-scoped socket path, bounded 4s hang guard is itself unref'd; Windows CI lanes green.
Extended reasoning...

Overview

One behavioral line: WindowsNamedPipeListeningContext::listen now calls uv_pipe.unref() immediately after uv_listen succeeds. This strips libuv's own active-handle ref so the owning Listener's poll_ref: KeepAlive becomes the sole source of truth for keeping the process alive — the same contract usockets already applies to every handle it creates on Windows (libuv.c:262), and the same contract WindowsNamedPipeContext already applies to per-connection pipe handles (WindowsNamedPipe.rs:585,836). The named-pipe listener was the one raw-libuv handle that was missed. A new subprocess test in node-net-server.test.ts covers server.unref() on a pipe/unix-socket listener letting the process exit naturally.

Security risks

None. uv_unref only affects loop-liveness bookkeeping; it does not change accept dispatch, permissions, or resource ownership. No user input reaches the changed code path.

Level of scrutiny

Low-to-medium. The src change is a one-liner in Windows-only code that brings the named-pipe listener in line with an established, documented contract every other libuv handle in this codebase already follows. I verified the unref() call resolves to uv_unref via the UvHandle trait (imported at the top of the file under cfg(windows)), and that Listener::listen's named-pipe branch subsequently calls poll_ref.ref_() so the process stays alive by default until JS calls server.unref(). Listener::ref_() restores the KeepAlive without needing to re-ref the uv handle — the PR description confirms unref(); ref() was tested on Windows.

Other factors

  • CI at 605a45f: 195/196 green including all Windows lanes; the two red checks (worker-transfer-terminate-stress on x64-asan, cargo clippy in parsers/yaml.rs) are pre-existing on main and unrelated to this diff.
  • My earlier review comment (stale .sock file in system tmpdir) was addressed in 605a45f by binding under a tempDir the parent disposes and passing the path via env; that thread is resolved.
  • The test follows harness conventions: bunEnv spread, concurrent stdout/stderr/exited drain, bounded hang guard that is itself unref'd, tempDir cleanup, stderr surfaced only on non-zero exit so debug-build noise doesn't flake it.
  • On POSIX the test exercises the already-correct unix-socket path (passes with or without the src change), which is acceptable coverage; the Windows-specific fail-before/pass-after is documented in the PR description and confirmed by Windows CI.

@Jarred-Sumner
Jarred-Sumner merged commit 9b7c272 into main Aug 7, 2026
54 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/11185e1b/windows-pipe-listener-unref branch August 7, 2026 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants