node:net: honor pause() made inside the 'connection' handler

[robobun]

What

onconnection calls _socket.resume() unconditionally after emit("connection", _socket). When the handler called s.pause(), that flipped readableFlowing from false back to true with no 'data' listener attached: inbound bytes were emitted into the void, the kernel receive buffer drained, and the writer saw 'drain' / bytesWritten progress against a paused reader that received nothing after resume().

Repro

import net from "node:net";
const server = net.createServer(); let srv;
server.on("connection", s => { srv = s; s.pause(); });          // documented backpressure idiom
await new Promise(r => server.listen(0, "127.0.0.1", r));
const cli = net.connect(server.address().port, "127.0.0.1");
await new Promise(r => cli.on("connect", r));
const chunk = Buffer.alloc(64 * 1024, 0x61); let writes = 0, drains = 0;
cli.on("drain", () => drains++);
while (writes < 4000) { writes++; if (!cli.write(chunk)) break; }
await new Promise(r => setTimeout(r, 400));
let got = 0; srv.on("data", d => got += d.length); srv.resume();
await new Promise(r => setTimeout(r, 1500));
console.log({ want: writes * chunk.length, got, drainsWhilePaused: drains });
// node: want==got, drainsWhilePaused 0
// bun:  got 0, drainsWhilePaused 1, cli.bytesWritten claims full delivery

Fix

Gate the post-emit resume() on _socket.readableFlowing === null (the handler left the stream untouched). A handler that paused (false) or attached 'data'/'readable' (true/false) is honored. Same for ServerHandlers.handshake.

A handler that left the stream untouched still resumes the way it did before, so a write-only handler's peer-close still tears the accepted socket down. (Node instead leaves readableFlowing at null there and relies on libuv's UV_EOF readStop to release the loop; matching that would require accepted sockets to hold the loop on their own.)

Verification

bun bd test test/js/node/net/node-net.test.ts -t "accepted socket honors pause"

Fails on main (flowing true, drainsWhilePaused 1, delivered false), passes with this change and matches Node line for line.

Related: #35006 and #34285 both cover the full readableFlowing === null accept semantics with the accompanying per-socket poll_ref rework. This PR is the scoped fix for the in-handler pause() case only; #35006 subsumes it if that lands first.

---

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.test.ts

[coderabbitai]

Walkthrough

Server and TLS socket resume logic now preserves pauses established by connection handlers. New fixtures and parameterized tests verify paused state, backpressure behavior, and complete data delivery after resuming.

Changes

Socket pause preservation

Layer / File(s)	Summary
Resume state guards src/js/node/net.ts	Post-handshake and post-connection resume calls now require readableFlowing === null, preserving explicit pauses and stream mode changes.
Pause regression coverage test/js/node/net/net-server-accepted-socket-pause-fixture.js, test/js/node/net/tls-server-accepted-socket-pause-fixture.js, test/js/node/net/node-net.test.ts	Net and TLS fixtures validate paused sockets under backpressure and after resuming; parameterized tests execute both scenarios in subprocesses.

Possibly related PRs

• oven-sh/bun#34285: Adjusts related accepted-socket and handshake resume behavior to preserve readableFlowing === null.

Suggested reviewers: cirospaciari

🚥 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 accurately summarizes the main change: preserving pause() behavior in the net connection handler.
Description check	✅ Passed	The description explains the bug, fix, repro, and verification, covering the template's required purpose and test details.

---

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

[robobun]

Reproduced on main: s.pause() inside the 'connection' handler is stomped by the post-emit _socket.resume() at net.ts:1067, so readableFlowing goes back to true with no listener, bytes are discarded, the writer sees 'drain' against a peer that gets 0 bytes after resume().

Fix: gate the post-emit resume() on readableFlowing === null so a pause() (or 'data'/'readable') made inside the handler is honored. Handlers that leave the stream untouched still resume as before. Same gate applied to ServerHandlers.handshake for the 'secureConnection' handler. Both covered by describe.each in node-net.test.ts.

Scoped down from the first pass (which removed the resume() entirely) after CI showed that write-only handlers then never reach 'end' with the peer's bytes buffered, and the listener's poll_ref holds the loop. The full readableFlowing === null accept semantics need the per-socket poll_ref rework in #35006 / #34285.

CI on 07c1bfd (build 77839) is green for this diff. The [new] failures are test/cli/install/{bun-install-lifecycle-scripts,bun-add}.test.ts hitting api.github.com - 504, which go through bun install's native HTTP client (not node:net); reported as main breaks. node-net.test.ts passed on all lanes including the darwin-aarch64 lane the previous push missed on.

Ready for review.

[robobun]

^{Updated 9:30 AM PT - Jul 22nd, 2026}

❌ @robobun, your commit 07c1bfd has 3 failures in Build #77839 (All Failures):

• step failed outside runner - exit 1 on 🍎 26 aarch64 - test-bun
• test/cli/install/bun-add.test.ts - code 1 on 🐧 3.23 x64
• test/cli/install/bun-install-lifecycle-scripts.test.ts - code 1 on 🐧 3.23 x64

---

🧪   To try this PR locally:

bunx bun-pr 35108

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

bun-35108 --bun

[github-actions]

Found 3 issues this PR may fix:

1. node:net: client data event never fires when write callback triggers end() against a loopback echo server #31383 - Server-side data loss: client data event never fires when write callback triggers end() against a loopback echo server, caused by premature resume() on the accepted socket
2. Readable.pipe(net.Socket) closes connection before peer response is delivered #32231 - Readable.pipe(net.Socket) closes connection before peer response is delivered, because the server socket's premature resume() emits data before a listener is attached
3. Unable to read raw data from http/https requests, using either "node:http" or "node:https" #11924 - Unable to read raw data from http/https request sockets — reporter noted "the only stream event called on the socket was resume", matching the exact bug this PR fixes

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #31383
Fixes #32231
Fixes #11924

🤖 Generated with Claude Code

[robobun]

Verified the three suggested issues against this change on 5f68b3df3 (main) and this branch:

• node:net: client data event never fires when write callback triggers end() against a loopback echo server #31383, Readable.pipe(net.Socket) closes connection before peer response is delivered #32231: both repros already pass on current main without this change (the client-side connect() path was fixed previously via read(0) in afterConnect).
• Unable to read raw data from http/https requests, using either "node:http" or "node:https" #11924: http.Server/https.Server accept connections through the native src/runtime/server/ path, not through node:net's onconnection, so this change does not reach it. Still reproduces with this change applied.

None of the three are closed by this PR. The bug this PR fixes is the accepted-socket face via net.createServer.

[github-actions]

This PR may be a duplicate of:

1. node:net: start sockets with readableFlowing null so early bytes buffer instead of being discarded #34285 - Superset of this PR: also removes the post-emit resume() in onconnection, removes else self.resume() in the TLS handshake handler, and adds read(0) after push(null) in SocketEmitEndNT, plus additional per-socket poll_ref rework
2. fix(net): server socket loses data when 'data' listener is added asynchronously #28289 - Same fix: removes premature resume() on accepted sockets and adds read(0) after push(null) in SocketEmitEndNT to prevent data loss
3. node:tls,node:net: follow-ups from the v26.3.0 review (error routing, manualStart reads, handshake timeout, setSecureContext) #35006 - Partial overlap: removes force-resume() on server-side TLS sockets after the handshake (the TLS half of this PR's fix)

🤖 Generated with Claude Code

[robobun]

On the duplicate candidates:

• node:net: start sockets with readableFlowing null so early bytes buffer instead of being discarded #34285 is referenced in the PR body; it's a broader rework (per-socket poll_ref, maybeUnrefIdle, client connect() path) with an open Windows http2 regression.
• fix(net): server socket loses data when 'data' listener is added asynchronously #28289 is stale and conflicts with main (predates the onconnection refactor).
• node:tls,node:net: follow-ups from the v26.3.0 review (error routing, manualStart reads, handshake timeout, setSecureContext) #35006 overlaps on the onconnection/TLS-handshake resume() but replaces it with read(0) rather than dropping the call; either way readableFlowing stays null/false, so node:tls,node:net: follow-ups from the v26.3.0 review (error routing, manualStart reads, handshake timeout, setSecureContext) #35006 also fixes this bug. It does not add read(0) after push(null) in SocketEmitEndNT, and it does not carry the writer-side-backpressure regression test. If node:tls,node:net: follow-ups from the v26.3.0 review (error routing, manualStart reads, handshake timeout, setSecureContext) #35006 lands first I'll rebase this to keep only the SocketEmitEndNT change, the regression test, and the two resume() additions in the affected tests.

[claude]

I didn't find bugs, but this changes streaming semantics for every accepted net/tls server socket (drops the post-emit resume() and adds read(0) after push(null)), and two existing tests already had to be adjusted to compensate — worth a human look given the blast radius and the overlap with #34285.

What was reviewed:

• onconnection / ServerHandlers.handshake: confirmed Node's onconnection and onServerSocketSecure leave readableFlowing at null; the client-side connect path here already uses read(0), so this brings the server side in line.
• SocketEmitEndNT read(0): matches Node's onStreamRead; checked it's inside the !self[kended] guard so it can't double-fire.
• The two updated tests: the added resume() matches what Node requires for server.close() to complete when the handler never reads.

Extended reasoning...

Overview

Three changes in src/js/node/net.ts: (1) onconnection no longer calls _socket.resume() after emit('connection'), (2) the TLS ServerHandlers.handshake sibling drops its else self.resume(), and (3) SocketEmitEndNT adds self.read(0) after push(null) so 'end' fires on a never-flowed empty-buffer stream. A new subprocess fixture + test in node-net.test.ts covers both the pause-in-handler and buffer-early-bytes cases; two existing tests (node-tls-connect ECONNRESET, node-http-connect backpressure) add an explicit resume() because they previously relied on the auto-resume to drain unread bytes so server.close() could complete.

Security risks

None. This is stream flow-control semantics only; no auth, crypto, or untrusted-input parsing is touched.

Level of scrutiny

High. node:net is production-critical and this is a behavioral change on every accepted TCP and TLS server socket. Removing an unconditional resume() means any consumer that was implicitly relying on flowing mode (without attaching 'data' or calling resume()/pipe()) will now buffer instead of discard — which is the Node-correct behavior, but the fact that two in-tree tests had to be patched shows the old behavior was load-bearing in places. The read(0) in SocketEmitEndNT is a second behavior change (when 'end' fires) that runs on every socket EOF.

Other factors

The PR is well-researched (cites Node's lib/net.js line, verified the two adjusted tests hang in Node the same way, ran 713 test-{net,tls,http,https,http2,cluster,stream}-* parallel tests). The new fixture is thorough and polls conditions rather than sleeping. However, #34285 carries a broader version of this fix that also touches poll_ref/unref semantics — a maintainer should decide whether to land this scoped subset now or coordinate with that PR. Given the blast radius across node:http/node:http2/node:tls server paths and the existence of a competing broader PR, this should get human sign-off.

[coderabbitai]

Actionable comments posted: 4

🤖 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 `@src/js/node/net.ts`:
- Around line 1068-1075: Shorten the comments to three lines or fewer: in
src/js/node/net.ts lines 1068-1075, retain only the durable readable-flow-state
invariant; in test/js/node/net/net-server-accepted-socket-pause-fixture.js lines
2-8, retain only the pause-preservation invariant; in
test/js/node/net/tls-server-accepted-socket-pause-fixture.js lines 2-5, condense
the TLS explanation; and in test/js/node/net/node-net.test.ts lines 1136-1142,
retain only the subprocess-isolation and pause-preservation rationale.

In `@test/js/node/net/net-server-accepted-socket-pause-fixture.js`:
- Around line 12-17: Bound the waitFor polling in
test/js/node/net/net-server-accepted-socket-pause-fixture.js:12-17 and
test/js/node/net/tls-server-accepted-socket-pause-fixture.js:13-18 with a
maximum turn count that rejects using a targeted error; update the initial wait
in the net fixture to report a missing 'connection' event and the TLS fixture to
report a missing 'secureConnection' event.
- Around line 36-59: Extract descriptive shared constants for chunk size,
maximum writes, settling poll limit, and delivery poll limit in
test/js/node/net/net-server-accepted-socket-pause-fixture.js:36-59, then use
them in the write loop and both waitFor conditions. Apply the equivalent named
workload and polling constants in
test/js/node/net/tls-server-accepted-socket-pause-fixture.js:38-58; preserve the
existing limits and behavior.

In `@test/js/node/net/node-net.test.ts`:
- Around line 1159-1161: Remove the exact-empty stderr assertion from this test,
while retaining stderr in the combined assertion or failure diagnostics so
benign native/ASAN output does not fail the test. Keep the existing stdout and
exitCode behavioral assertions unchanged.

🪄 Autofix (Beta)

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: b392253e-d108-4424-816a-4117cfa07804

📥 Commits

Reviewing files that changed from the base of the PR and between 47597ab and d1bc38c.

📒 Files selected for processing (4)

• src/js/node/net.ts
• test/js/node/net/net-server-accepted-socket-pause-fixture.js
• test/js/node/net/node-net.test.ts
• test/js/node/net/tls-server-accepted-socket-pause-fixture.js

[claude]

LGTM — the readableFlowing === null gate is a strict narrowing of when the post-emit resume() fires, and both sibling paths (TCP onconnection + TLS ServerHandlers.handshake) are now covered by the describe.each fixtures.

What was reviewed

• Traced the gate against all three handler-touched states (pause() → false, 'data' → true, 'readable' → false) and the untouched null case; only null still resumes, matching the documented rationale.
• Confirmed pauseOnConnect interaction is unchanged (line 1031 sets false before emit; !pauseOnConnect still guards independently).
• TLS fixture key paths (test/js/node/test/fixtures/keys/agent1-*.pem) exist; expect(stderr).toBe("") matches 5+ neighboring subprocess tests in the same file.

Extended reasoning...

Overview

Two-line logic change in src/js/node/net.ts: the unconditional _socket.resume() after emit('connection') (and its TLS twin after emit('secureConnection')/'secureConnect') is now gated on readableFlowing === null. Two new subprocess fixtures (net-server-accepted-socket-pause-fixture.js, tls-server-accepted-socket-pause-fixture.js) and a describe.each in node-net.test.ts assert that a pause() inside the handler leaves readableFlowing at false, backpressure holds, and every byte is delivered after resume().

Security risks

None. This is stream-flow-state bookkeeping in the Node compat layer; no input parsing, auth, crypto, or trust-boundary changes.

Level of scrutiny

Medium — node:net accept-path semantics are subtle and there are overlapping open PRs (#34285, #35006) reworking the same area. But the change here is a strict narrowing: resume() can only be skipped relative to before, and only when the handler already touched flow state (which is exactly the bug case). The null-still-resumes behavior is unchanged and its rationale (write-only handler + peer-close teardown, pending the per-socket poll_ref rework) is documented in the inline comment. The author already scoped down from removing resume() entirely after CI showed that regressed write-only handlers.

Other factors

• My prior review nit (missing TLS coverage for the ServerHandlers.handshake gate) was addressed in 4e80eb0 and the thread is resolved.
• CodeRabbit's two remaining unresolved inline comments are stylistic: the magic-numbers-in-fixture one was declined with a stated rationale (each value appears once with an adjacent comment); the expect(stderr).toBe("") one matches the exact pattern used by five other subprocess tests in the same file, so accepting it here would diverge from local convention.
• robobun reports CI green on 9a3578b with only an unrelated bun-install-proxy.test.ts flake hitting neighboring builds.
• Fixtures use bounded polling for the bug-distinguishing waits and were verified to fail on main and match Node.

[robobun]

Closing: this was fixed on main by #32630, which landed a few hours after this PR was opened. onconnection in net.ts now kicks off reading with _socket.read(0) after emitting 'connection' instead of calling resume(), so a pause() made inside the handler (or a 'data'/'readable' listener attached there) is honored, which is the Node behavior this PR was after. The two tests this PR adds to test/js/node/net/node-net.test.ts (accepted socket honors pause() made inside the connection and secureConnection handlers) pass unmodified against a debug build of main at 04148c8, twice in a row.