[robobun]

net.Socket's onread option is the zero-allocation read path: the callback is supposed to get back the buffer it supplied, holding at most buffer.length bytes, and returning false is supposed to stop reads. None of that held.

Repro

const net = require("net");
const buf = Buffer.alloc(64);
let calls = 0;

const server = net.createServer(c => {
  c.write(Buffer.alloc(4096, 0x61));
  const t = setInterval(() => c.write(Buffer.alloc(4096, 0x61)), 5);
  c.on("close", () => clearInterval(t));
});

server.listen(0, () => {
  net.connect({
    port: server.address().port,
    onread: {
      buffer: buf,
      callback(nread, b) {
        console.log({ sameBuffer: b === buf, nread, bounded: nread <= buf.length });
        if (++calls === 3) return false; // should pause
      },
    },
  });
});

	node v26	bun 1.4.0
b === buf	true	false (Bun's internal chunk)
nread <= buf.length	always	nread is 4096 against a 64-byte buffer
buffer as a factory function	called once before the first read, once per delivery	never called
return false	stops reads	keeps delivering (55 more chunks in 300ms)

Code written to the documented contract reads out of range with buffer.subarray(0, nread), never sees its pooled buffers reused, and cannot apply backpressure.

Cause

src/js/node/net.ts installed a data handler that forwarded Bun's chunk straight through:

data(socket, buffer) {
  onread.callback(buffer.length, buffer);
}

Node instead pushes a CustomBufferJSListener that uses the user's buffer as the uv_buf_t for every read, so nread is bounded by the buffer and the callback's false return reaches readStop().

Fix

Keep Node's per-socket state (buffer, callback, factory) on the socket and emulate the listener in JS:

• each incoming chunk is copied into the user's buffer in buffer-sized slices, one callback per slice, so nread <= buffer.length and the callback always gets its own buffer back;
• the factory form is consulted before the first read and again after every delivery, matching initSocketHandle + onStreamRead;
• a false return stops the kernel reads (pause + unref, as Socket.prototype.pause() does) and holds the bytes the callback has not taken; resume() hands those over first, and reads only restart if the callback accepts them;
• pause() parks the socket in that same state, so a callback reaching for backpressure with this.pause() stops the rest of the chunk too, the way Node's readStop() does. The docs promise it ("pause() and resume() will also behave as expected") and it was taking all 64 slices of a 4096-byte chunk where Node hands it 1;
• bytesRead counts bytes delivered to the callback, as Node's native bytesRead does.

The acceptance check now matches Node too: an onread that is not { buffer: Uint8Array | Function, callback: Function } is ignored (socket keeps emitting 'data') instead of throwing TypeError.

Holding bytes back safely

Node never holds undelivered bytes in JS: the user's buffer is the uv_buf_t, so anything the callback declines simply stays unread in the kernel. Emulating the contract means Bun has to hold them, and that brought four hazards with it. Each is fixed with its own test, and each test fails when its guard is removed:

• Post-destroy delivery. The callback runs user JS and can destroy() the socket between two slices of the same chunk. onreadDeliver re-checks destroyed after each callback; without it, 7 more callbacks land on a dead socket.
• Overwritten on a second dispatch. The TLS read loops (openssl.c's restart after a full plaintext buffer, SSLWrapper::handle_reading when its 64 KB buffer fills) keep decrypting and dispatching without re-checking the socket's pause flag, and pause() cannot reach a TLS socket wrapped over a generic Duplex at all. A 4096 + 4096 byte exchange delivered 4608 bytes over 9 calls instead of 8192 over 1. Incoming chunks now queue behind the undelivered ones.
• 'end' before the drain. A FIN landing while bytes are held back ended the readable side through the inherited end/close handlers, which destroyed the socket before resume() could deliver them: c.end(4096 bytes) produced 512 delivered and 3584 lost. Both handlers go through endReadableSide(), which defers push(null) until the held bytes drain. Non-onread sockets never have pending bytes (kBufferPending is only written behind the kBufferCb gate), so onreadIsPaused() is always false there and that path is unchanged. The one genuinely shared change is resume() now making its Duplex.prototype.resume tail call first; see the audit in the comments.
• Survived a reconnect. kBufferPending is per-connection but initSocketHandle() did not clear it, so reusing a Socket replayed the old connection's bytes into the new one: 64 callbacks carrying connection A's payload where Node delivers 1.
• Cleared the pause marker. Once pause() parked a socket by setting kBufferPending, that same initSocketHandle() clear wiped it, so s.pause(); s.connect(...) delivered everything. The marker is now derived from isPaused(), which is the gate Node reaches through if (!socket.isPaused()) socket.read(0): held-back bytes still die with their connection, an unlifted pause still belongs to the socket.
• Wiped the pause a callback took mid-flush. resume() flushed the held bytes before its tail call into Duplex.prototype.resume, so a callback that paused during that synchronous flush had its pause overwritten a line later. Node never hits this because its delivery is asynchronous, so the pause lands last. The tail call runs first now; the delivery loop keys off kBufferPending rather than the flowing flag, so the flush is unaffected.

Verification

bun bd test test/js/node/net/node-net.test.ts -t "net.Socket onread" — 11 tests covering buffer identity, the nread bound, no 'data' events, pause-on-false and pause-by-method then resume with nothing lost, the factory call pattern, the six holding-bytes cases above, and the ignored-option shape. All 11 fail against main's net.ts.

Because endReadableSide sits on the shared end/close path and resume() is on every socket's, test/js/node/net/, node-tls-connect.test.ts and node-http.test.ts were run together: 330 pass, no new failures.

---

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

[robobun]

^{Updated 1:52 AM PT - Jul 10th, 2026}

❌ @robobun, your commit 9d3e74f has 1 failures in Build #71398 (All Failures):

• test/js/bun/http/proxy-stress-concurrent.test.ts - code 1 on 🍎 14 x64

---

🧪   To try this PR locally:

bunx bun-pr 33382

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

bun-33382 --bun

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This PR adds onread support to node:net Socket, including state setup, native chunk delivery, pause/resume handling for pending bytes, and tests for fixed buffers, TLS transport behavior, reconnects, and invalid option shapes.

Changes

Socket onread implementation and tests

Layer / File(s)	Summary
State and initialization src/js/node/net.ts	Adds typed-array helpers, symbol-backed onread state, socket initialization, and reset cleanup for pending and ended markers.
Delivery pipeline and activation src/js/node/net.ts	Implements buffer selection, chunk slicing, callback delivery, pending-byte queuing, FIN deferral, native data routing, and supported-shape onread activation.
Pause and resume control src/js/node/net.ts	resume(), pause(), read(), and _read() coordinate pending onread bytes with native read resumption and loop-hold state.
Core onread behavior tests test/js/node/net/node-net.test.ts	Adds test scaffolding, fixed-buffer delivery coverage, stop-and-resume cases, and buffer-factory behavior.
Transport and lifecycle tests test/js/node/net/node-net.test.ts	Adds TLS-over-Duplex queuing, delayed end handling, destroy handling, reconnect behavior, and invalid option-shape coverage.

Sequence Diagram(s)

sequenceDiagram
  participant Native socket handle
  participant SocketHandlersOnRead
  participant onread.callback

  Native socket handle->>SocketHandlersOnRead: incoming chunk
  SocketHandlersOnRead->>onread.callback: deliver buffer slice and nread
  onread.callback-->>SocketHandlersOnRead: return true, false, or pause
  SocketHandlersOnRead->>SocketHandlersOnRead: store pending remainder or defer end

Loading

Related Issues: None referenced in the summaries.

Related PRs: None referenced in the summaries.

Suggested labels: node:net, enhancement, needs-tests

Suggested reviewers: None identified from the provided information.

Poem
A socket learns a callback path,
Buffers split by measured math,
Pause on false, resume in time,
Pending bytes rejoin the line.

🚥 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 summarizes the main change: Node-like onread delivery into the user buffer with bounded, pausable reads.
Description check	✅ Passed	The description covers what changed and how it was verified, matching the template's required content despite different headings.

---

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

[github-actions]

This PR may be a duplicate of:

1. net: support Socket({handle, manualStart}) with stream-wrap onread #29139 - Also implements net.Socket onread option support using the same symbols (kBuffer, kBufferCb, kBufferGen), the same bounded-copy-into-user-buffer mechanism, and the same readStop-on-false-return backpressure logic

🤖 Generated with Claude Code

[coderabbitai]

Actionable comments posted: 3

🤖 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 1280-1286: The read loop in the net socket receive path can keep
running after user JS tears down the socket, so re-check liveness immediately
after the callback. Update the callback handling in the
`self[kBufferCb].$call(...)` path to re-validate `self.destroyed` (and related
stop conditions used by this loop) before calling `onreadNextBuffer(self)` or
continuing, so a callback that `destroy()`s, `end()`s, or `close()`s the socket
cannot be invoked again on the next slice.

In `@test/js/node/net/node-net.test.ts`:
- Line 1024: The server-side socket error handlers in the net tests are
swallowing failures, which can leave write callbacks unresolved and hide the
real cause of a hang. Update the affected `c.on("error", ...)` cases in the
pause/resume and related tests to forward the error into the existing
client-side rejection or hang-guard promise instead of discarding it, so
failures surface through the test logic tied to `secondWritten.promise` and the
other corresponding promises.
- Around line 1010-1018: The withServer helper only waits for the successful
listen callback, so listen failures can hang the test instead of rejecting.
Update withServer to use createServer and server.listen with proper error
handling so the promise rejects on the server's error event, and make sure the
cleanup in the try/finally still closes the server. Use the withServer helper
and server.listen call as the entry points for the fix.

🪄 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: be5f5ad7-4f62-4386-aea5-26c82d2173a3

📥 Commits

Reviewing files that changed from the base of the PR and between fb50cce and e5c642c.

📒 Files selected for processing (2)

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

[robobun]

Not a duplicate of #29139

#29139 wires its onStreamRead only behind $isCallable(self._handle.readStart), i.e. stream-wrap handles (new Socket({ handle }), TTY/Pipe wraps). On that PR the native uSockets path still installs the old handler:

this[khandlers] = {
  ...SocketHandlers2,
  data(socket, buffer) {
    onread.callback(buffer.length, buffer);  // Bun's chunk, unbounded nread, no pause
  },
};

So net.connect({ port, onread }) (the path in this PR's repro) keeps handing the callback Bun's internal chunk there. The two changes are complementary and the symbol names match, so reconciling them if #29139 lands is mechanical: its onStreamRead covers the stream-wrap handles, onreadDeliver here covers the native socket data handler.

Review feedback (73f9300)

All three findings were valid and are addressed.

self.destroyed after the callback. The check is placed after onreadNextBuffer(self) rather than before it: Node's onStreamRead calls bufGen() unconditionally once the callback returns, even if that callback destroyed the stream, so the factory keeps getting exactly one call per delivery. The guard still stops the next slice of the chunk from reaching a dead socket, which was the point. New test for it; with the guard removed, 7 extra callbacks land on a destroyed socket.

withServer listen failure and swallowed server errors. The helper now rejects on the server's 'error' event during listen(), and races server-side connection errors against the test body so a failed c.write(second, cb) surfaces the real error instead of hanging on secondWritten.promise. The one remaining c.on("error", () => {}) is in the destroy-from-callback test, where the client tears the connection down mid-stream and an unflushed server write legitimately reports ECONNRESET/EPIPE; that is commented in place.

[robobun]

Pushed 5818833 for the second review pass. One of the three findings turned out to be a real data-loss bug this PR would have shipped.

kBufferPending overwritten on a second dispatch (fixed)

The review pointed out that the SSL read loops never re-check the socket's pause flag between dispatches. They do not, and for a TLS socket wrapped over a generic Duplex the situation is worse: pause() cannot reach the transport at all, so every subsequent chunk both re-entered the callback that had returned false and overwrote the bytes it had not taken.

Against tls.connect({ socket: <Duplex>, onread: { buffer: Buffer.alloc(512), callback } }), with the server writing 4096 bytes, the client pausing, and the server then writing 4096 more:

{"callsAtPause":1,"callsAfterGo":9,"maxNread":512,"totalBytes":4608,"wantBytes":8192,"payloadIntact":false}

9 callbacks after the pause, and 3584 bytes gone. SocketHandlersOnRead.data now queues an incoming chunk behind the undelivered bytes whenever kBufferPending is set, so resume() hands the callback the whole stream in order. New test queues bytes that arrive after the callback stopped reads covers it; removing the guard brings back the 9 calls and the truncated payload.

Framing-dependent nread assertions (fixed)

minNread: payload.length % buffer.length only held when the kernel delivered the whole 64 KB as one chunk. Every nread assertion is now nreads.filter(nread => nread < 1 || nread > buffer.length) against [], which holds for any chunking; the payload reassembly already covers the tail slices. The pause/resume and destroy tests had the same dependency through Math.max(...nreads) and were changed the same way.

self.destroyed after the callback

Already in 73f9300. The check sits after onreadNextBuffer(self) rather than before it, so the factory still gets exactly one call per delivery the way Node's onStreamRead does; it stops the next slice of the chunk either way.

All 6 onread tests fail against main's net.ts and pass with the diff. The rest of test/js/node/net/ is unchanged (40 pass; the 10 failures reproduce identically on main in this sandbox and are DNS/localhost resolution). node-tls-connect.test.ts and node-http.test.ts are unchanged too.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/js/node/net.ts:1494 — kBufferPending is per-connection state but is never cleared on reconnect — if the callback stopped mid-chunk (returned false / destroyed / threw) and the user then calls .connect() again on the same Socket, the process.nextTick resume() scheduled by connect() will replay the held-back bytes from the old connection to the callback (and add them to bytesRead) before any byte of the new connection arrives. Node has no equivalent — unaccepted bytes stay in the closed fd's kernel buffer and vanish. Adding self[kBufferPending] = null; to initSocketHandle() (or to the if (this.destroyed) reset block in connect()) would fix it.

  Extended reasoning...

  What the bug is

  kBufferPending holds the bytes of an already-received chunk that the onread callback declined (it returned false, threw, or destroyed the socket mid-chunk). It is state that belongs to a specific connection — those bytes came from one particular peer over one particular fd. But it is only ever assigned in three places: the constructor (line 1494, null), onreadDeliver (line 1294, the leftover subarray), and onreadFlushPending (line 1322, null right before re-delivering). Nothing in the destroy or reconnect path touches it.

  initSocketHandle() (net.ts:3819–3830) is the "reset per-connection state" hook — it calls _undestroy(), clears _sockname/kclosed/kended — and Socket.prototype.connect() calls it whenever it creates a fresh handle for a reused Socket (line 1942). It does not clear kBufferPending, so the stale subarray survives into the new connection.

  The code path that triggers it

  1. const s = new Socket({ onread: { buffer: Buffer.alloc(64), callback(n, b) { s.destroy(); } } }); s.connect(portA);
  2. Peer A sends ≥128 bytes in one chunk. SocketHandlersOnRead.data → onreadDeliver: iteration 1 copies 64 bytes, callback destroys the socket, self.destroyed is now true → stopped = true, break. Line 1294: self[kBufferPending] = chunk.subarray(64) — the remaining bytes of connection A's chunk. (Any path that sets stopped with offset < total works: return false, throw, or destroy.)
  3. After 'close', user calls s.connect(portB). connect() (line 1733–1739) schedules process.nextTick(() => { if (!this.isPaused()) this.resume(); }). Then at line 1928 this.destroyed is true so _handle is nulled; at line 1940–1942 a fresh detached handle is created and initSocketHandle(this) runs → _undestroy() clears self.destroyed. kBufferPending is untouched.
  4. Next tick fires. onreadStop never called Duplex.prototype.pause(), so isPaused() is false → this.resume() runs. Socket.prototype.resume (line 2098) calls onreadFlushPending(this) first.
  5. onreadFlushPending (line 1319–1325): pending is the stale subarray from step 2, it is nulled out, pending.length > 0 && !self.destroyed (now false after _undestroy) → onreadDeliver(self, pending). Connection A's held-back bytes are copied into the user's buffer, handed to the callback, and added to bytesRead — before a single byte of connection B has been read.

  Why existing code doesn't prevent it

  onreadFlushPending's only guard is self.destroyed, which _undestroy() has already cleared by the time the nextTick fires. onreadStop early-returns when destroyed and doesn't clear the pending bytes either. Nothing ties the pending buffer to the handle it came from.

  Node comparison and impact

  Node has no JS-side equivalent of kBufferPending: the user's buffer is the uv_buf_t, so bytes the callback declines simply stay unread in the old fd's kernel receive buffer and vanish when that fd is closed. A reconnect on the same net.Socket therefore starts clean. In Bun after this PR, the callback instead sees [tail of connection A][connection B], which is straight data corruption for anything parsing a protocol.

  The trigger is narrow — it needs onread + the callback stopping mid-chunk + reusing the same Socket instance via .connect() after destroy — so this is filed as a nit rather than a blocker. But it's new state introduced by this PR and the fix is a one-liner.

  Fix

  Add self[kBufferPending] = null; to initSocketHandle() (alongside the kclosed/kended resets), or to the if (this.destroyed) { this._handle = null; ... } block at line 1928 in Socket.prototype.connect. Either drops the stale bytes exactly as closing the old fd does in Node.

[robobun]

Pushed 4634e6f. The reconnect finding was real, and it was the worst one so far.

kBufferPending survives a reconnect (fixed)

kBufferPending belongs to one connection, but initSocketHandle() never cleared it. Reusing a Socket after the callback stopped mid-chunk replayed the old connection's bytes into it before the new peer sent anything. Two servers, the first writing 4096 bytes of 'A', the second deliberately silent, with the callback pausing on its first delivery:

node:       {"totalCalls":1,"seen":[{"connection":"A","nread":64,"byte":"A"}]}
bun (pre):  {"totalCalls":64,"seen":[{"connection":"A",...,"byte":"A"},
                                     {"connection":"B","nread":64,"byte":"A"},
                                     {"connection":"B","nread":64,"byte":"A"}, ...]}

63 callbacks on the new connection carrying the old connection's payload. Node has nothing equivalent because the user's buffer is the uv_buf_t, so declined bytes stay unread in the fd it is about to close. initSocketHandle() now clears kBufferPending alongside kclosed/kended, which drops them exactly the way closing the fd does. New test does not replay bytes held back from a previous connection; deleting the one-line clear brings the failure straight back.

Destroy-test framing dependency

Already handled in 5818833: expect(nreads).toEqual([buffer.length]) became

expect({
  deliveries: nreads.length,
  outOfRange: nreads.filter(nread => nread < 1 || nread > buffer.length),
}).toEqual({ deliveries: 1, outOfRange: [] });

deliveries: 1 is the invariant the test is actually about, and it holds for any chunking.

All 7 onread tests fail against main's net.ts and pass with the diff. The rest of test/js/node/net/ is unchanged (41 pass; the 10 failures reproduce identically on main in this sandbox and are DNS/localhost resolution).

[robobun]

Pushed 35814a7. Another data-loss bug out of this round.

'end' fired while the callback still had bytes held back (fixed)

SocketHandlersOnRead only overrode data, so the inherited end/close handlers still ran push(null); read(0). Over a Duplex-backed TLS socket, where pause() never reaches the transport, a c.end(payload) of 4096 bytes against a 512-byte buffer gave:

{"totalDelivered":512,"expected":4096,"events":["cb(512)","END","CLOSE","-- resume() --"]}

The readable side ended, allowHalfOpen: false destroyed the socket, and the 3584 held bytes were gone before resume() ever ran.

Both sites now go through endReadableSide(), which defers push(null) while kBufferPending is set and fires it from onreadFlushPending once the callback has drained. That is what Node does implicitly: stopped reads mean the FIN sitting behind those bytes is never observed. Non-onread sockets never have pending bytes, so that path is byte-for-byte unchanged. Same run now:

["cb(512)","-- resume() --","cb(512)" x7,"END","CLOSE"]   // 4096 of 4096

Quadratic queue copy (fixed)

kBufferPending holds either a single chunk or an array of them now, concatenated once on flush. One allocation per flush instead of one per arrival. The single-chunk fast path is kept, so plain TCP (where the pause does reach the transport and at most one chunk lands behind it) stores the subarray as-is.

kBufferPending across reconnect

Already in 4634e6f, one commit before the review ran. initSocketHandle() clears it alongside kclosed/kended, and does not replay bytes held back from a previous connection covers it.

State

8 onread tests, all failing against main's net.ts and passing with the diff. Ran all of test/js/node/net/ plus node-tls-connect.test.ts and node-http.test.ts because endReadableSide sits on the shared end/close path: 327 pass, no new failures (the 11 reds reproduce identically on main in this sandbox and are DNS / localhost resolution / an http-proxy fixture).

The red darwin lane on the previous run was buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun', not a test failure. This push re-runs it.

[coderabbitai]

♻️ Duplicate comments (2)

test/js/node/net/node-net.test.ts (2)

1402-1467: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

serverB's error handler swallows failures with no forwarding, unlike serverA's documented exception.

Line 1409 (serverA) is intentionally justified with a comment explaining the client destroys mid-stream. Line 1413 (serverB) has no such justification, and no premature destroy happens against serverB before ended.promise resolves — an unexpected server error here would silently hang the test instead of surfacing a clear failure.

🐛 Proposed fix

     const serverB = createServer(c => {
-      c.on("error", () => {});
+      c.on("error", err => ended.reject(err));
       c.end(second);
     });

ended is declared later in the same test function but assigned well before serverB's connection listener can fire (only after reconnecting to portB), so this closure reference is safe.

As per coding guidelines, "Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise — never throw inside event callbacks."

🤖 Prompt for 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.

In `@test/js/node/net/node-net.test.ts` around lines 1402 - 1467, The test’s
serverB error handling is swallowing failures, which can leave ended.promise
hanging instead of failing clearly. Update the node-net test around the Socket
reconnect flow so serverB’s connection path forwards any unexpected error to the
awaited promise/rejection path, similar to how the socket error handler rejects
firstCall and ended. Keep serverA’s existing justified error suppression, but
make serverB’s listener or its surrounding promise wiring surface errors from
createServer, c.end(second), or the connection callback into ended.promise so
the test fails deterministically.

Source: Coding guidelines

---

1307-1366: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Server-side TLS error swallowed without forwarding to the hang-guard promise.

c.on("error", () => {}) (line 1312) discards errors from the server-side TLS socket, but unlike the mid-stream-destroy tests, nothing here destroys the client prematurely before firstCall/endEvent settle. An unexpected server-side error (e.g. TLS write failure) would leave the test hanging on await firstCall.promise / await endEvent.promise until the runner's generic timeout, with no diagnostic — the same failure mode already fixed elsewhere in this file for the non-TLS tests.

🐛 Proposed fix

     const server = tls.createServer({ cert: tlsCert.cert, key: tlsCert.key }, c => {
-      c.on("error", () => {});
+      c.on("error", err => {
+        firstCall.reject(err);
+        endEvent.reject(err);
+      });
       c.end(payload); // payload and FIN in one go
     });

firstCall/endEvent are declared later in the same test function's scope but assigned before the connection listener can ever fire, so this closure reference is safe.

As per coding guidelines, "Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise — never throw inside event callbacks."

🤖 Prompt for 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.

In `@test/js/node/net/node-net.test.ts` around lines 1307 - 1366, The TLS test
currently swallows server-side errors by using c.on("error", () => {}), which
can leave firstCall and endEvent waiting forever on unexpected failures. Update
the test around the tls.createServer callback to forward any server socket error
into the same hang-guard promises used in this test, similar to the other
error-handling patterns in node-net.test.ts. Keep the existing firstCall and
endEvent Promise.withResolvers wiring, but make sure any server-side error
rejects them so the test fails fast with diagnostics instead of timing out.

Source: Coding guidelines

🤖 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.

Duplicate comments:
In `@test/js/node/net/node-net.test.ts`:
- Around line 1402-1467: The test’s serverB error handling is swallowing
failures, which can leave ended.promise hanging instead of failing clearly.
Update the node-net test around the Socket reconnect flow so serverB’s
connection path forwards any unexpected error to the awaited promise/rejection
path, similar to how the socket error handler rejects firstCall and ended. Keep
serverA’s existing justified error suppression, but make serverB’s listener or
its surrounding promise wiring surface errors from createServer, c.end(second),
or the connection callback into ended.promise so the test fails
deterministically.
- Around line 1307-1366: The TLS test currently swallows server-side errors by
using c.on("error", () => {}), which can leave firstCall and endEvent waiting
forever on unexpected failures. Update the test around the tls.createServer
callback to forward any server socket error into the same hang-guard promises
used in this test, similar to the other error-handling patterns in
node-net.test.ts. Keep the existing firstCall and endEvent Promise.withResolvers
wiring, but make sure any server-side error rejects them so the test fails fast
with diagnostics instead of timing out.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 29080497-9ff6-4446-8538-a2143745acdf

📥 Commits

Reviewing files that changed from the base of the PR and between 5818833 and 35814a7.

📒 Files selected for processing (2)

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

[robobun]

Pushed 09b6010, tests only. Both findings were right: the TLS end-of-stream test and the reconnect test's second server only see the client tear down after the test body has finished, so c.on("error", () => {}) there could strand endEvent.promise / ended.promise until the runner's generic timeout with no diagnostic. Those two now forward the error into the promises the body awaits, and the hang-guard deferreds are declared above the servers so the closure can never see them in TDZ.

The two servers whose client does destroy mid-stream (serverA in the reconnect test, and the destroy-from-callback test) keep their swallow, each with the reason written next to it: an unflushed write there legitimately reports ECONNRESET/EPIPE, and that is the behavior under test.

8 onread tests still pass and still fail against main's net.ts.

[robobun]

Status: ready for review, CI is red on a cross-branch flake

The diff is green. After rebasing onto main (9d3e74f), build 71398 finished 283 jobs passed, 1 failed. The :darwin: 26 aarch64 - test-bun artifact-download timeout that blocked every pre-rebase build has cleared.

I've used my one re-roll and won't push an empty commit; here's the evidence so a maintainer doesn't have to re-derive it. (Commits after that re-roll are review fixes and the rebase, not retriggers.)

The single red lane, and why it isn't this diff

test/js/bun/http/proxy-stress-concurrent.test.ts on :darwin: 14 x64: one of ×1200 requests failed in the https-proxy → https-origin mode=complete scenario.

That test hits the same failure on 8 of the last 30 builds across 7 branches — including 71402 for farm/c87f6b11/spawnsync-stdin-epipe, whose diff is:

src/js/node/child_process.ts
src/runtime/api/bun/spawn/stdio.rs
src/runtime/api/bun/subprocess.rs
src/spawn/static_pipe_writer.rs
test/js/bun/spawn/spawnSync.test.ts
test/js/node/child_process/child_process.test.ts
...

No socket code, JS or native. Other branches hitting it: claude/http-server-hotpath-opts, farm/30574f96/bun-listen-tls-authorized, farm/24ea6a49/socket-writev-remainder, farm/053dbb60/diag-postgres-connect-refused-windows, claude/foreign-owned-tail, claude/foreign-owned-batch. The test was last touched by #33622 (make some tests faster), and every one of those branches includes that commit.

The test does create its CONNECT proxy with node:net, so I ran it locally: passes with my build.

The one I chased down pre-rebase

test/js/node/test/parallel/test-net-connect-memleak.js failed on alpine x64 in two of my pre-rebase builds. It's a conservative-GC assertion (is the implicit once('connect') closure collected after one gc() + one setImmediate). This diff adds no closure and no retainer on the connect path (the five new symbols are null/false on every non-onread socket). It passed alpine on the two later builds with a strictly larger diff, and locally 10/10 both with and without the diff.

What the diff is verified against

• bun bd test test/js/node/net/node-net.test.ts -t "net.Socket onread" — 11 tests, all 11 fail against main's net.ts. Each guard was confirmed load-bearing by deleting it and watching its test go red.
• test/js/node/net/ + node-tls-connect.test.ts + node-http.test.ts together, because endReadableSide sits on the shared end/close path and resume() is on every socket's: 330 pass, no new failures.

[claude]

All prior review feedback has been addressed (including the resume() ordering in 9e014ce, with a new test covering it) and I found nothing further. That said, this is a substantial rewrite of the onread delivery path that also touches the shared end/close handler via endReadableSide and introduces JS-side byte-holding to emulate Node's kernel-level readStop(), so it warrants a maintainer's look — particularly the design choice and coordination with #29139.

Extended reasoning...

Overview

This PR rewrites net.Socket's onread option in src/js/node/net.ts to match Node's documented contract: the callback receives the user's own buffer, nread is bounded by buffer.length, the factory form is honoured, return false stops reads, and pause()/resume() behave as documented. Because Bun receives its own chunk from the native layer rather than reading directly into the user's buffer, the implementation slices each chunk into buffer-sized deliveries and holds undelivered bytes in JS (kBufferPending). That design brought several hazards (post-destroy delivery, TLS multi-dispatch overwrite, FIN-before-drain, reconnect replay, pause-across-connect, resume-ordering) — each was surfaced in review, fixed, and given its own test. The diff is ~200 lines of implementation in net.ts plus ~670 lines of new tests (11 tests) in node-net.test.ts.

Security risks

None identified. The change is confined to the read-side delivery path of node:net sockets; no auth, crypto, filesystem, or permission surfaces are touched. The two TLS tests use rejectUnauthorized: false against a local in-process server with a harness cert, which is the standard pattern. Input handling is bounded (buffer-length slicing) and no untrusted lengths are trusted for allocation.

Level of scrutiny

High. src/js/node/net.ts is core Node compatibility code, and this PR modifies the shared SocketHandlers2.end/close path via the new endReadableSide() — that path is exercised by every net/tls/http socket, not just onread ones. The author verified non-onread sockets are byte-for-byte unchanged (onreadIsPaused is always false there) and ran test/js/node/net/ + node-tls-connect + node-http with no new failures, but a maintainer should confirm that reasoning. The JS-side byte-holding design is also a genuine architectural decision (Node never holds undelivered bytes in JS because the user's buffer is the uv_buf_t), and the interaction with pause()/resume()/initSocketHandle/reconnect is intricate enough that the state machine deserves a human read.

Other factors

• Review history: nine rounds of automated review feedback, all addressed with fix commits and dedicated regression tests. Every guard was verified load-bearing by the author (deleting it fails its test). The final commit 9e014ce reorders Duplex.prototype.resume before onreadFlushPending so a pause() from within the flush survives, with a new test keeps a pause() the callback takes during resume()'s flush.
• Test quality: strong. Tests await conditions rather than sleeping, wire failure paths to reject, avoid kernel-framing dependencies (all nread assertions are range-bounded), pin host: "127.0.0.1", and each isolates one invariant. All 11 fail against main's net.ts.
• Related work: #29139 implements onread for the stream-wrap handle path with the same symbol names; the author's analysis says the two are complementary, but a maintainer should decide sequencing.
• CI: the author reports remaining red lanes are infra (darwin artifact download timeout hitting many branches) and known flakes, not this diff.

Given the scope, the shared-path modification, and the design decision involved, this should not be auto-approved.

[robobun]

Agreed this needs a human, and the review is right about where the risk sits. Two things to make that read cheaper, and one correction to my own earlier phrasing.

The shared end/close path: the invariant, not my word for it

endReadableSide() reduces to the exact two lines it replaced (self.push(null); self.read(0)) for any socket without onread, because onreadIsPaused() is provably always false there. Every write to kBufferPending in the file:

line	write	reachable for a non-onread socket?
1297	onreadDeliver tail	no: only called from SocketHandlersOnRead.data, or from onreadFlushPending past its if (!pending) return true guard
1326, 1331	onreadQueuePending	no: only called from SocketHandlersOnRead.data
1340	onreadFlushPending	no: after the same guard
1537	constructor	yes, = null
2183	pause()	gated on this[kBufferCb]
3882	initSocketHandle	gated on self[kBufferCb]

and kBufferCb is only ever set non-null at line 1626, inside the validated onread block. So for a socket with no onread, kBufferPending is permanently null, which makes onreadIsPaused() permanently false, which makes endReadableSide, read() and _read() identical to what they were. SocketHandlersOnRead is a separate table that only an onread socket ever gets installed.

One place non-onread sockets do change, which I should have called out

resume() (9e014ce) now runs Duplex.prototype.resume.$call(this) before this._handle?.resume?.() rather than after. That is a real ordering change for every socket, not just onread ones. It is inert: both are synchronous, nothing can arrive between them on a single-threaded loop, neither reads the other's state, and the resume_ nextTick that Duplex.prototype.resume schedules calls read(0) → _handle.resume() anyway. test/js/node/net/ + node-tls-connect + node-http run together at 330 pass, no new failures. But "byte-for-byte unchanged" was too strong for resume() specifically, and a reviewer should look at that line rather than take the claim.

Design and #29139

The JS-side byte-holding is the part I'd most want a second opinion on. Node never needs it because the user's buffer is the uv_buf_t, so bytes the callback declines simply stay unread in the kernel. Bun is handed its own chunk, so emulating return false means holding the remainder somewhere, and every hazard this PR fixed after the first commit came out of that one decision. An alternative is native useUserBuffer support so reads land in the user's buffer directly, which would delete kBufferPending and everything hanging off it. That is a much larger change, and #29139 already sketches the stream-wrap half of it.

Sequencing with #29139 is a maintainer call. As it stands the two touch disjoint paths (it wires onStreamRead behind $isCallable(handle.readStart); this covers the native uSockets data handler, which it leaves on the old forwarding code) and they share symbol names, so either order rebases mechanically.

[robobun]

Heads-up from #42313 (approved, not merged yet). It turns net.Socket.prototype.bytesRead into a getter that reads this._handle.bytesRead, with no setter, as in node.

• The builtins run in strict mode. self.bytesRead += length in this PR's onread path throws TypeError once node:net: count bytesRead on the native socket handle #42313 is on main.
• The hunks do not overlap, so git does not flag it on a rebase.
• node:net: count bytesRead on the native socket handle #42313 counts onread bytes on the native handle, one slice per callback, so the JS increment is not needed after a rebase.

[robobun]

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-10, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.