[robobun]

What does this PR do?

A server-accepted net.Socket that receives DATA + FIN before the app attaches a reader was being destroyed one setImmediate after the FIN, with the buffered payload still unread. Any server that hands the accepted socket to an async pipeline (auth lookup, worker handoff, any await before .on('data')) lost the entire request of every client that sends and then half-closes (curl, shutdown(SHUT_WR), write-then-FIN RPC clients). No error, no 'end': the socket was just dead with the bytes stranded in readableLength.

Repro

import net from "node:net";
let sock; const ev = [];
const srv = net.createServer(s => {
  sock = s;
  s.on("close", h => ev.push("close:" + h));
  s.on("end", () => ev.push("end"));
});
srv.listen(0, "127.0.0.1", async () => {
  const c = net.connect(srv.address().port, "127.0.0.1");
  await new Promise(r => c.once("connect", r));
  c.end("PAYLOAD-1234567890");                    // request bytes, then half-close
  await new Promise(r => setTimeout(r, 300));     // app busy for 300 ms
  const snap = { destroyed: sock.destroyed, rl: sock.readableLength, ev };
  let late = "";
  sock.on("data", d => (late += d)); sock.resume();
  await new Promise(r => setTimeout(r, 300));
  console.log(snap, "lateGot=" + JSON.stringify(late));
  // node : { destroyed:false, rl:18, ev:[] }               lateGot="PAYLOAD-1234567890"
  // bun  : { destroyed:true,  rl:18, ev:["close:false"] }  lateGot=""
  process.exit();
});

Cause

SocketEmitEndNT scheduled setImmediate(destroyAbandonedNT, self) on peer FIN when no reader had been engaged synchronously from the 'connection' handler. destroyAbandonedNT then called destroySoon() precisely when readableLength > 0 with no listener, i.e. it destroyed exactly the sockets that still held unread inbound data (an empty-buffer FIN was spared). Introduced in 85f22bb (#34598).

Node has no such reclaim: a paused socket with buffered data stays open until it is read or destroyed, and 'end' only fires after the buffer is drained. The handle is already unref()'d by finishSocketEnd, so leaving the socket open does not pin the event loop.

Fix

Remove destroyAbandonedNT and the kReaderInterest bookkeeping that fed it (27 lines deleted, no replacement). An accepted socket with buffered payload + peer FIN now stays open until the app reads (then 'end' fires and the normal allowHalfOpen / autoDestroy path closes it) or destroys it, matching Node.

Verification

$ bun bd test test/js/node/net/node-net.test.ts -t "accepted-socket buffering"
(pass) delivers bytes buffered before a 'readable' listener attaches, past peer FIN
(pass) keeps a client socket's buffered response available for a late reader after peer FIN
(pass) delivers bytes to a 'data' listener attached via setImmediate from the connection handler
(pass) keeps a server socket open while buffered data from a write-then-FIN client is unread
 4 pass  0 fail

The new test fails on main (destroyed: true, events: ["close:false"], readableLength: 18) and passes with the fix. node-net-server.test.ts 21/21 and node-net-allowHalfOpen.test.js 2/2 unchanged.

---

no test proof · iteration 4 · 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 7:05 AM PT - Jul 29th, 2026}

@Jarred-Sumner, your commit 427ddfb is building: #85211

[robobun]

Reproduced with bun bd test test/js/node/net/node-net.test.ts -t "keeps a server socket open while buffered data" on main: { destroyed: true, events: ["close:false"], readableLength: 18 }, late reader receives nothing. Passes with this diff: { destroyed: false, readableLength: 18, events: [] }, late reader receives "PAYLOAD-1234567890" then 'end'.

PR: #36332

CI status: node-net.test.ts passes on every lane that ran. Remaining red is unrelated to this diff: three darwin test lanes expired without running (fleet infra, same on both builds 84976 and 85056), and every other failure is tagged flaky by the CI helper (passed alone or on retry), including the single node-net-server.test.ts "should listen on unix domain socket" flake on alpine x64 which passed on retry and is on the listen path this PR does not touch. Ready for review.

[coderabbitai]

Walkthrough

Removes internal reader-interest tracking from Node-compatible socket teardown and server connection handling. Adds regression coverage confirming buffered data remains readable and the server socket stays open after a client writes data and sends FIN before listeners attach.

Changes

Socket EOF lifecycle

Layer / File(s)	Summary
Remove reader-interest cleanup src/js/node/net.ts	Removes kReaderInterest and its use in socket end cleanup, TLS handshake handling, and accepted-socket initialization.
Validate buffered FIN handling test/js/node/net/node-net.test.ts	Adds a write-then-FIN regression test and updates comments in related late-reader tests.

Possibly related PRs

• oven-sh/bun#35939: Both changes address half-open socket EOF handling and duplicate or premature end processing.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 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 matches the main fix: keeping accepted net sockets open until unread buffered data is consumed.
Description check	✅ Passed	The description includes the PR purpose and verification steps, with a clear test command and results.

---

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

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@test/js/node/net/node-net.test.ts`:
- Around line 1147-1148: Update the teardown comment near the readable-stream
check to remove references to the removed abandoned-socket teardown,
setImmediate, peer FIN, and _readableState. Keep only the durable rationale that
lifecycle notifications are allowed to settle before buffered data is checked.
- Around line 1164-1168: Update the finally cleanup block to await
server.close() before the test exits, preserving the existing socket and client
destruction calls and ensuring the server listener is fully closed before
cleanup completes.

🪄 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: 5bd858bb-7f7b-46a3-80c3-92bedd3fee44

📥 Commits

Reviewing files that changed from the base of the PR and between 59242d6 and b1bb66e.

📒 Files selected for processing (2)

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

💤 Files with no reviewable changes (1)

• src/js/node/net.ts

[github-actions]

This PR may be a duplicate of:

1. fix(net): server socket loses data when 'data' listener is added asynchronously #28289 - Fixes the same server-socket data-loss bug when a 'data' listener is added asynchronously after 'connection'
2. node:net: start sockets with readableFlowing null so early bytes buffer instead of being discarded #34285 - Superset fix that starts sockets with readableFlowing null so early bytes buffer instead of being discarded, addressing the same premature destruction / data-loss scenario

🤖 Generated with Claude Code

[robobun]

Not duplicates:

• fix(net): server socket loses data when 'data' listener is added asynchronously #28289 targets the pre-tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) #34598 tree (its diff patches SocketEmitEndNT at a context that no longer exists and removes a _socket.resume() call that has since been replaced by read(0)). It force-resume()s to drain the buffer, whereas this PR keeps the buffer intact for a late reader like Node does. That PR is in CONFLICTING state and would need a full rewrite against main.
• node:net: start sockets with readableFlowing null so early bytes buffer instead of being discarded #34285 changes the initial readableFlowing state; this PR removes the destroyAbandonedNT reclaim added by tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) #34598, which already checks readableFlowing !== null as one of its bail conditions. Different layer.

[claude]

No issues found — the deletion is clean, finishSocketEnd already unref()s the handle so the removed reclaim wasn't needed to release the loop, and the new test fails on main / passes with the fix. Deferring because this reverts socket-lifecycle logic added by #34598 five days ago in a core compat module; worth a human confirming there was no intentional fd-reclaim motive behind destroyAbandonedNT that isn't captured in a test.

What was reviewed:

• Confirmed kReaderInterest/destroyAbandonedNT have no remaining references and no test in #34598 depended on the reclaim firing (its two related tests only asserted it didn't fire).
• Verified finishSocketEnd unrefs the native handle at src/js/node/net.ts:576, so an abandoned FIN'd socket won't pin the event loop after this change.
• The bounded _readableState.ended poll now used in both sibling tests replaces the prior 50ms sleep (my earlier nit, addressed in 867cc6e).

Extended reasoning...

Overview

The PR deletes 27 lines from src/js/node/net.ts: the kReaderInterest symbol, the post-emit('connection') / post-emit('secureConnection') reader-interest checks, the setImmediate(destroyAbandonedNT, self) scheduled from SocketEmitEndNT, and destroyAbandonedNT itself. It adds one regression test and updates two sibling tests in the net.Server accepted-socket buffering describe block to drop stale references to the removed teardown and replace a 50ms sleep with a bounded _readableState.ended poll.

Deleted code was load-bearing?

I traced the origin to 85f22bb (#34598, merged 2026-07-24). That PR added destroyAbandonedNT alongside two tests, but both tests assert the reclaim does not fire (client sockets, setImmediate-attached readers) — nothing asserts it does fire. The bail condition self.readableLength === 0 means it only destroyed sockets that still held unread data, which matches the PR's diagnosis that the guard was inverted. Node has no equivalent path. finishSocketEnd (called immediately before the removed schedule) already socket.unref?.()s the native handle, so leaving the JS Socket alive does not pin the event loop.

Security risks

None identified. No user-controlled parsing, no auth/crypto surface. The only lifetime concern would be fd retention for accepted sockets whose app never reads them; that matches Node semantics and the fd is released when the app drops its reference or the process exits (handle is unref'd).

Level of scrutiny

Medium-high: src/js/node/net.ts is a core Node-compat module and this alters accepted-socket lifecycle at FIN time. The diff itself is a pure deletion with no new branches, so the mechanical review is straightforward — the open question is whether #34598 had an unstated reason for the reclaim (e.g., fd pressure under an accept-and-ignore workload) that a maintainer should confirm before it's removed.

Other factors

• My earlier inline nit (bare 50ms sleep left uncommented) was addressed in 867cc6e by switching to the bounded poll.
• CodeRabbit's two comments were addressed/withdrawn.
• robobun independently reproduced fail-on-main / pass-with-fix.
• Grep confirms no remaining kReaderInterest / destroyAbandonedNT references anywhere in src/.

[claude]

No blocking issues found — the deletion is clean (no orphaned kReaderInterest/destroyAbandonedNT references remain) and the new regression test pins the Node-compat behavior. Deferring to a human because this reverts part of #34598's server-socket FIN handling that landed five days ago; worth having the author of that PR confirm destroyAbandonedNT wasn't load-bearing for something the TLS suite doesn't cover.

Also checked: finishSocketEnd already unref()s the handle after peer FIN (net.ts:574-577), so leaving the buffered socket open does not pin the event loop; the TLS secureConnection post-emit reader check removed at ServerHandlers.handshake had no consumer other than the deleted destroyAbandonedNT gate.

Extended reasoning...

Overview

Pure deletion in src/js/node/net.ts (27 lines: kReaderInterest symbol, its three write sites in onconnection/ServerHandlers.handshake/SocketEmitEndNT, and the destroyAbandonedNT function), plus one new regression test and comment/poll-loop adjustments to two existing sibling tests in test/js/node/net/node-net.test.ts. The mechanism being removed was introduced in #34598 (85f22bb, 2026-07-24) and destroyed server-accepted sockets that received DATA+FIN before a reader attached — the opposite of Node's behavior, where buffered bytes stay available for a late .on('data').

Security risks

None. No auth/crypto/permissions surface; this is stream-lifecycle bookkeeping in the JS builtin layer. Removing an eager destroySoon() cannot introduce a resource leak that pins the process because finishSocketEnd already unref()s the native handle once the peer FIN arrives.

Level of scrutiny

Medium-high. node:net socket EOF handling is production-critical and this changes observable behavior for every server that accepts a write-then-half-close client. The change itself is small and well-argued (Node has no equivalent reclaim; the PR description traces the divergence to a specific commit and confirms the event-loop-pin concern is already handled by unref()), but it reverts a piece of a large maintainer-authored PR from five days ago whose commit message doesn't state why destroyAbandonedNT was added. The #34598 diff added no test that positively asserts the abandoned-socket destroy fires (only tests that it does not fire for clients / setImmediate-attached readers), so nothing in-tree obviously depended on it — but the original author should confirm.

Other factors

• Grep confirms no remaining references to destroyAbandonedNT or kReaderInterest anywhere under src/.
• The new test follows the exact structure of the three sibling tests in the same describe block (same cleanup pattern, same Promise.withResolvers + bounded-setImmediate poll on _readableState.ended); PR verification shows it fails on main and passes with the fix, and node-net-server/node-net-allowHalfOpen suites are unchanged.
• My earlier inline comment about the bare 50ms sleep was addressed in 867cc6e by switching to the same bounded poll; the one remaining nit (poll not exiting on client.destroyed) only affects failure-path diagnostics and is non-blocking.
• Two related open PRs (#28289, #34285) were flagged as potential overlaps; the author's rebuttal that they target different layers/tree states looks correct, but it's another reason a maintainer familiar with the area should sign off.

[claude]

No bugs found, and both earlier test-hygiene findings were addressed in 867cc6e and a8a8fd5. Deferring rather than approving because this deletes destroyAbandonedNT/kReaderInterest, which was intentionally added five days ago in #34598 (a large TLS PR) — the original author should confirm nothing on the TLS accepted-socket path was relying on this reclaim.

What was reviewed

• src/js/node/net.ts: confirmed all three kReaderInterest write sites and the SocketEmitEndNT scheduler are removed together; no dangling references remain.
• finishSocketEnd already unref()s the handle, so leaving the socket open with buffered data does not pin the event loop.
• #34598's own test additions in node-net.test.ts assert the reclaim doesn't fire (client-side, setImmediate-attach), so they remain valid after removal.
• New test's poll loops now exit on .destroyed so a regression fails fast instead of timing out.

Extended reasoning...

Overview

This PR removes the destroyAbandonedNT mechanism and its kReaderInterest bookkeeping from src/js/node/net.ts (27 lines deleted, three sites: symbol declaration, SocketEmitEndNT scheduler, ServerHandlers.handshake post-emit check, and onconnection post-emit check plus the function itself). It adds one regression test to test/js/node/net/node-net.test.ts and adjusts two neighboring tests' comments/wait mechanisms to drop references to the removed teardown.

Security risks

None. This is socket-lifecycle timing in the Node compat layer; no auth, crypto, or input-validation surface is touched.

Level of scrutiny

Medium-high. The diff is small and pure-deletion on the source side, and the PR description gives a clear repro, root-cause trace, and Node-behavior comparison. However, the deleted code was added deliberately in #34598 (85f22bb, 2026-07-24) — a large TLS PR authored by cirospaciari with Jarred as co-author. The #34598 commit message doesn't mention destroyAbandonedNT explicitly, and the tests #34598 added to node-net.test.ts assert the reclaim doesn't fire in specific cases rather than that it does, so removal doesn't break those. But REVIEW.md's "before deleting odd-looking code, git-blame why it was written" applies: the mechanism was scoped to server-accepted sockets and also armed on the TLS secureConnection path, so there may have been a TLS test-suite scenario (one of the 14 vendored upstream tests in #34598) that motivated it. That is worth a human confirming rather than a bot approving a reversal of a five-day-old maintainer decision.

Other factors

• The bug-hunting system found no issues this run. My two prior inline findings (bare 50ms sleep → bounded poll; poll loop lacking a .destroyed exit) were both applied.
• The github-actions duplicate-PR bot flagged #28289 and #34285 as related; robobun's response distinguishes them convincingly (different tree state / different layer), but it does mean this area has multiple in-flight approaches a human should reconcile.
• The behavioral argument is sound: destroyAbandonedNT destroyed exactly the sockets holding unread buffered data (readableLength === 0 was a bail-out), which is data loss, and Node has no equivalent. finishSocketEnd already unref()s the handle so keeping the socket open does not pin the loop.

[claude]

No issues found — the deletion is clean, finishSocketEnd already unref()s the handle so an unread-buffer socket won't pin the loop, and the new test proves the Node-compat gap. Deferring only because this reverts destroyAbandonedNT added deliberately in #34598 five days ago; worth a quick look from that PR's author to confirm it wasn't guarding a specific TLS abandoned-socket scenario there.

What was reviewed

• kReaderInterest had no remaining readers after the deletions (grep clean).
• finishSocketEnd at src/js/node/net.ts:575 unrefs the handle on FIN, so removing destroySoon() doesn't re-pin the event loop.
• Both bounded poll loops now exit on .destroyed (my earlier comments), so a regression fails fast instead of timing out.

Extended reasoning...

Overview

This PR deletes ~27 lines from src/js/node/net.ts: the destroyAbandonedNT function, the kReaderInterest symbol, and the three sites that set/read it (in SocketEmitEndNT, ServerHandlers.handshake, and onconnection). It adds one new test to test/js/node/net/node-net.test.ts and updates two existing sibling tests in the accepted-socket buffering describe block (comment cleanup + replacing a 50ms sleep with a bounded .ended poll per my earlier review).

The removed code was added in 85f22bb (#34598, merged 2026-07-24) and would destroySoon() a server-accepted socket one setImmediate after peer FIN if the app had not synchronously attached a reader — but only when readableLength > 0, i.e. it destroyed exactly the sockets holding unread inbound data. Node has no such reclaim; a paused socket with buffered data stays open until read or destroyed. I verified finishSocketEnd (src/js/node/net.ts:562-578) already unref()s the native handle on FIN, so leaving these sockets open does not pin the event loop, which was the only plausible non-compat rationale for the reclaim.

Security risks

None. This is a Node-compat lifecycle change in the JS net layer; no auth, crypto, parsing, or untrusted-input handling is touched.

Level of scrutiny

Medium-high. The diff is small and pure-deletion, and the reasoning is airtight (the removed guard's readableLength === 0 bail condition is inverted from any sensible resource-reclaim intent — it spares empty sockets and destroys the ones with data to lose). But node:net is a critical compat layer, and the deleted code was added deliberately five days ago as part of a large maintainer TLS PR (#34598). The #34598 commit message doesn't state a rationale for destroyAbandonedNT specifically, and the tests added alongside it (the two siblings this PR now updates) document around it rather than for it, so I can't rule out that it was covering a TLS-side scenario (abandoned post-handshake sockets in a vendored Node TLS test, for instance) that isn't exercised by the net suite.

Other factors

• All prior review feedback on this PR (two claude[bot] inline comments, two CodeRabbit nits) has been addressed and resolved.
• grep confirms no remaining references to kReaderInterest or destroyAbandonedNT anywhere in the tree.
• robobun confirmed the new test fails on main (destroyed: true) and passes with the fix; CI is green on all lanes that ran.
• The PR correctly notes it is not a duplicate of #28289 (stale, targets pre-#34598 tree) or #34285 (different layer — initial readableFlowing state).

Given the change reverts a very recent deliberate addition by a maintainer, I'd rather that author or another maintainer confirm the removal doesn't regress whatever motivated it in #34598 before this lands.