node:net: keep delivering when an onread callback calls pause() then read()

[robobun]

Problem

• An onread callback that calls socket.pause() and then socket.read() gets no more calls. The rest of the chunk stays in kOnreadTail until a later resume(). Node delivers it: the script in node:net: pause() then read() inside an onread callback stops the delivery #42418 prints [ 'abcd', 'efgh' ] in node and [ "abcd", "no call for 1 s" ] in bun.
• The cause is the stop condition in the deliver closure of the Socket constructor (src/js/node/net.ts): ret === false || self.isPaused(). Readable.prototype.read() does not clear the paused flag of the stream, so isPaused() is still true after pause(); read().

Fix

• Add kOnreadReading, the equivalent of node's handle.reading. readStop() clears it. The new readStart() helper (the handle.resume() plus restorePausedHold() pair that read(), _read(), resume() and the tail drain all had inline) and drainOnreadTail() set it. deliver and drainOnreadTailNT stop on that flag. This replaces the fromRead and kOnreadReadRequested plumbing, which only covered a read() that ran after the tail existed.
• This is node's model: pause() stops the handle, read() and resume() start it, and onStreamRead stops it only on a false return (net.js#L817-L845, stream_base_commons.js#L191-L198).
• A TLS socket over a wrapped net.Socket that is still connecting never called read(0), so its flag stayed unset. The TLS handle open now calls read(0) unless the socket is paused, like node's constructor for a socket built over a handle.
• Verified: test/js/node/net/node-net.test.ts (two new onread tests, the first fails on the released bun) and test/js/node/tls/node-tls-upgrade.test.ts (one new test). Also ran the other net, tls and http suites and node's test-net-* and test-tls-* tests that wrap a socket.

Background

• An onread socket hands each read to a user callback with a user buffer instead of push(). Node bounds each kernel read to that buffer. Bun reads bigger native chunks and slices them in deliver, so a stop in the middle of a chunk stores the rest in kOnreadTail for a later drainOnreadTailNT tick.
• readStop() pauses the native handle and drops its hold on the event loop. restorePausedHold() gives the hold back when reads start again.
• Self-reviewed: the review found one regression in the first version (the wrapped TLS socket case above), which the second commit fixes. node:net: drop the undelivered onread tail when the connection ends #42305 removes kOnreadReadRequested in dropOnreadTail(). Whichever PR lands second drops that line.

Notes

• Behaviour matrix checked against node v26.3.0 on a plain TCP client: pause(); read(), read(); pause(), pause(); resume(), false then read(), FIN together with data, pause(); read() on the last slice with a FIN, resume(); pause() inside the callback, a late FIN then resume(). All match.
• Unchanged divergences that this PR does not touch: a TLS client paused while connecting still delivers one slice before it stops (node starts no reads at all until resume()), and read(); pause() inside a TLS callback stops at once where node's TLSWrap still emits the bytes it already decrypted.
• pause() on a socket that is not reading still calls readStop(), as before. readStop() is idempotent.
• Pre-existing failures in this sandbox, identical on the released bun and on the base tree: the net.Socket read group (ECONNREFUSED on a localhost listener), unref should exit (DNS), the connect({path}) leak test (times out under ASAN), SNICallback runs even when..., and three node-http-connect and node-http-proxy-url tests that time out under ASAN.

---

no test proof · iteration 0 · 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]

Warning

Review limit reached

• Run on-demand review

_{On-demand reviews are free for the next 8 days. After that, they cost $0.25 per reviewed file.}

Or wait 1 minute for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: c647a5eb-f19b-4beb-8499-ed256f2c1235

📥 Commits

Reviewing files that changed from the base of the PR and between 263cca7 and 44fb299.

📒 Files selected for processing (1)

• src/js/node/net.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: f5539fef-65ee-4078-9738-7137278c88fc

📥 Commits

Reviewing files that changed from the base of the PR and between df573da and 263cca7.

📒 Files selected for processing (3)

• src/js/node/net.ts
• test/js/node/net/node-net.test.ts
• test/js/node/tls/node-tls-upgrade.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

---

Walkthrough

The socket implementation now tracks explicit onread reading state. Native reads, queued tails, pause, resume, and wrapped-handle setup use shared helpers. Regression tests cover net callback ordering and TLS upgrades during connection.

Changes

Socket onread state control

Layer / File(s)	Summary
Explicit reading state and native control src/js/node/net.ts	Adds kOnreadReading, readStart, and readStop. Read, resume, _read, and wrapped-handle setup use the shared reading controls.
Onread delivery and queued tails src/js/node/net.ts	Onread delivery and queued tail draining now use explicit reading state to stop, retain, and resume data delivery.
Pause, resume, and TLS validation test/js/node/net/node-net.test.ts, test/js/node/tls/node-tls-upgrade.test.ts	Adds regression coverage for pause/read ordering and TLS reading on a still-connecting wrapped socket.

Suggested reviewers: cirospaciari

Priority: ⚪ Not assessed

Merge Risk: ⚪ Minimal · up to 263cc

The change has regression coverage for the updated socket and TLS read behavior, with no unresolved actionable risk identified.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description clearly explains the problem, the implementation, the TLS behavior, and the verification performed. It does not use the template headings exactly, but it contains the required informat…
Title check	✅ Passed	The title clearly and concisely describes the primary change: preserving onread delivery when a callback calls pause() followed by read().
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.

---

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

[robobun]

^{Updated 2:20 AM PT - Sep 12th, 2026}

✅ @robobun, your commit 44fb29931defa514bb9e4c840b55c20e6d9079f4 passed in Build #114644! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 42444

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

bun-42444 --bun

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I traced kOnreadReading initialization: every connect path (afterConnect at net.ts:3446, TLS handshake at :1044, server accept at :1294, and the new wrapped-open at :1324) reaches read(0) → readStart before the first deliver call, so switching the stop condition from isPaused() to !self[kOnreadReading] doesn't halt on the very first slice. Also confirmed no stale kOnreadReadRequested references remain, and the resume() early return ret is safe because readStart already calls restorePausedHold internally.

Extended reasoning...

The inline finding covers the test-wiring gap. Beyond it, the main risk in this diff is that kOnreadReading starts life as undefined, and the delivery loop now stops on !self[kOnreadReading] where it previously stopped on self.isPaused() — so if any path can reach deliver() before something sets the flag, the first native chunk would be truncated to one slice. I walked every read(0) site that fronts a data-producing handle (client afterConnect, TLS secureConnect, server-side accept, the constructor's nextTick for already-open handles, and the newly added wrapped-socket open) and each reaches readStart (or drainOnreadTail, which also sets the flag) before data can arrive. The resume() refactor's new early return looked like it might skip the trailing restorePausedHold, but readStart calls it itself, so the hold is still restored on that branch. The TLS test's dual onread + 'data' accumulator was also examined: it can't distinguish which path delivered, but the regression it guards (reads never restart, so end never fires) fails regardless of which listener would have received the bytes.

[robobun]

Addressed the test wiring nit in 263cca7: the read-then-pause test now rejects both awaited promises on the client's error and close events.

[robobun]

Shortened the three flagged comments in net.ts to one line each (44fb299).

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[robobun]

Replied to the nit on the TLS test: the data listener stays. Node emits data (not onread) for a TLS socket over a wrapped socket, and #42425 aligns bun with that. The test asserts that the reads run to the FIN, whichever path delivers the bytes.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[robobun]

This change also fixes a wider group of stalls that have the same cause. I worked on that group and did not open a PR, because this PR covers it. The notes below are for this PR to use.

The other symptoms

An onread socket stalls after the first buffer when the stream is in paused mode and nobody called pause(). The callback gets 4,096 of 1,048,576 bytes, then nothing. The socket never emits 'end' or 'close'. These all stall on bun 1.4.0 to 1.4.3 and on main:

• a 'readable' listener (added before 'connect', on 'connect', or between two callbacks)
• for await (const chunk of socket)
• pause() then resume() under a 'readable' listener (resume() leaves readableFlowing === false, so the drain tick returned on isPaused())
• a false return, then resume() or read(0), under a 'readable' listener

Node v26.3.0 and bun 1.3.14 deliver every byte in each case. So this is a regression from #32630 (cc40d78), first released in v1.4.0. It is the same over TLS and over a unix socket.

What I checked against this PR

I applied the net.ts change of this PR on top of the head of #42313 and ran my tests and a node comparison of 30 cells (17 plain, 13 TLS). Every cell matches node v26.3.0, and all my tests pass.

Tests you can take

Commit a181e85 on robobun/01c93444/net-onread-readable-listener-stall changes test/js/node/net/node-net.test.ts only:

• 24 cells, 8 cases over tcp, tls and a unix socket (the cases above, plus pause() then read() inside the callback from node:net: pause() then read() inside an onread callback stops the delivery #42418)
• read() then pause() before the drain tick, as a second case of the existing resume() then pause() test

All 25 fail on bun 1.4.3 (24 time out on the stall) and pass with the change of this PR. The tests of this PR do not cover a 'readable' listener or an async iterator. The branch is stacked on #42313. On main the commit also needs createServer as createTLSServer, connect as tlsConnect in the node:tls import line (on my branch that line comes from #42313).

The second commit on that branch (d67e4f6) is my version of the fix, for reference. It keeps the flag narrow: only the pause() override sets it, and resume() / read() / _read() clear it in drainOnreadTail(). It does not touch readStop() and adds no read(0) to the open handler. It gives the same results as this PR on all 30 cells.

Landing order with #42313

#42313 is approved and changes the same loop, so it will probably land first. Two things to know for the rebase:

• It changes the line above both stop conditions to if (self.destroyed || self.connecting) return;. These two hunks conflict with this PR.
• It adds dropOnreadTail(), which contains self[kOnreadReadRequested] = false. Git merges that hunk with no conflict. This PR deletes the symbol. A rebase that resolves only the marked conflicts leaves that line, and then every connect() on an onread socket throws. The body here names node:net: drop the undelivered onread tail when the connection ends #42305 for this function. node:net: count bytesRead on the native socket handle #42313 has the same one.

[robobun]

I also worked on the 'readable' listener / for await stall and did not open a PR, because this PR covers it. One result is new compared with the note above.

A 218-cell comparison with node v26.3.0, run on the head of this PR (44fb299)

A cell is: tcp or tls, the bytes in one burst or one slice per read, one of 11 consumers of the read side ('data', 'readable', for await, pause(), resume(), read() and combinations of them), attached before 'connect', on 'connect' or inside the first callback, with the second callback returning undefined or false.

• main (4b5862f): 7 of the 11 consumer groups fail.
• This PR: 210 of 218 cells equal node.
• 4 of the 8 cells that differ also differ on main, and this change is not the cause. A tick that the first callback queued runs after the second slice of the same native read. In node it runs before that slice.
• The other 4 are a TLS client that calls pause() before 'connect': tls/paced/pauseResume/sync/* and tls/paced/pauseRead/sync/*. Node delivers every slice and does not wait for resume(). With this PR the callback runs once, then the socket waits for resume() or read().

The notes in the PR body say that node starts no reads at all until resume() in that case. Node v26.3.0 does read: after tls.connect({ onread }) with pause() in the same tick and no resume(), the callback gets all five slices, then 'end' and 'close' follow.

The cause, and a small change that fixes the 4 cells

kOnreadReading starts as undefined. A TLS client that is paused before connect skips read(0) in afterConnect, and afterConnect does not stop an encrypted socket. So the native handle reads while the flag is still falsy, and the first slice stops it.

In my version the flag starts as true in the if (onread) block of the constructor, because the native socket reads from open. initSocketHandle() sets it to true again for a socket that connects again. readStop() and drainOnreadTail() write it only when self[kOnreadBuffer] !== undefined, so a socket without onread gets no new property. With that, all 214 cells that can match do match. The read(0) in the wrapped open handler is then not needed for the flag: the tests of this PR (node-tls-upgrade.test.ts, and the onread tests of node-net.test.ts) pass on my version without it. Commit 30282e6 on robobun/5d42797e/onread-readable-listener-stall has it for reference, +18/-12 in net.ts.

The table as a test, if you want it

Commit 59a2c36 on the same branch adds test/js/node/net/node-net-onread.test.ts, a fixture and the stored node traces. Nothing in it sleeps. A step that has to happen "later" runs when the server has flushed its writes and the trace has not moved for 5 loop turns, and a cell that cannot move any more records STALL. It is test-only and can land after this PR. On the head of this PR it needs the 4 TLS cells above in its list of known differences.