node:net: publish diagnostics_channel events (net.client.socket, net.server.socket, net.server.listen) by robobun · Pull Request #34209 · oven-sh/bun

robobun · 2026-07-15T04:18:12Z

Bun's node:net never published any of the built-in diagnostics channels, so connection-level instrumentation (per-connection context seeding via channel.bindStore on net.client.socket, connection counting, socket tagging, OpenTelemetry's documented net instrumentation pattern) was silently blind under Bun.

Repro

import dc from "node:diagnostics_channel";
import net from "node:net";

const GROUP = ["net.client.socket", "net.server.socket",
  "tracing:net.server.listen:asyncStart", "tracing:net.server.listen:asyncEnd"];
const hits = new Map();
for (const n of GROUP) dc.subscribe(n, () => hits.set(n, (hits.get(n) ?? 0) + 1));

const srv = net.createServer(s => s.end());
await new Promise(r => srv.listen(0, "127.0.0.1", r));
await new Promise(res => {
  const c = net.connect(srv.address().port, "127.0.0.1");
  c.on("close", res); c.on("error", res);
});
srv.close();
for (const n of GROUP) console.log(`${hits.get(n) ?? 0} ${n}`);

	node v26.3.0	bun 1.4.0 / main
`net.client.socket`	1	0
`net.server.socket`	1	0
`tracing:net.server.listen:asyncStart`	1	0
`tracing:net.server.listen:asyncEnd`	1	0

Fix

src/js/node/net.ts now publishes at the same points and with the same payloads as Node's lib/net.js:

Channel	Payload	Where
`net.client.socket`	`{ socket }`	`Socket.prototype.connect`, after args are normalized
`net.server.socket`	`{ socket }`	`onconnection`, right after `self.emit('connection', socket)`
`tracing:net.server.listen:asyncStart`	`{ server, options }`	`Server.prototype.listen`, after the `ERR_SERVER_ALREADY_LISTEN` check; `options` is `normalizeArgs(arguments)[0]`, the same value Node passes
`tracing:net.server.listen:asyncEnd`	`{ server }`	`kRealListen`, after a successful bind
`tracing:net.server.listen:error`	`{ server, error }`	`listen`'s catch, with the formatted listen error

Every publish is gated on the sub-channel's hasSubscribers so the no-subscriber path allocates nothing, matching the existing pattern in _http_client.ts, http2.ts and dgram.ts.

Verification

test/js/node/net/node-net.test.ts: two subprocess tests (isolated because dc subscriptions are process-global) pinning the exact payload shapes, instanceof checks, and ordering for a successful listen (asyncStart → asyncEnd → 3x client.socket → 3x server.socket) and a failing listen (asyncStart → error EADDRINUSE, no asyncEnd).
test/js/node/test/parallel/test-diagnostics-channel-net.js: Node's upstream test verbatim, which also covers all three client-socket entry points (net.connect, net.createConnection, new net.Socket().connect) and options.customOption passthrough on asyncStart.

Both fail on main (USE_SYSTEM_BUN=1) and pass with the change. node-net-server.test.ts and the existing test-diagnostics-channel-* parallel tests are unchanged.

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

….listen diagnostics channels Node's built-in diagnostics_channel channels for net were never published, so connection-level instrumentation (per-connection context via channel.bindStore on net.client.socket, connection counting, socket tagging) was silently blind under Bun. Publish at the same points and with the same payloads as Node's lib/net.js: - net.client.socket {socket} in Socket.prototype.connect - net.server.socket {socket} in onconnection, after the 'connection' emit - tracing:net.server.listen asyncStart {server, options} in Server.prototype.listen after the already-listening check - tracing:net.server.listen asyncEnd {server} in kRealListen after a successful bind - tracing:net.server.listen error {server, error} in listen's catch Every publish is gated on the sub-channel's hasSubscribers so the common no-subscriber path allocates nothing. Adds Node's upstream test-diagnostics-channel-net.js to the parallel suite and two subprocess tests in node-net.test.ts that pin the exact payload shapes and ordering.

coderabbitai · 2026-07-15T04:18:20Z

Walkthrough

Changes

Net diagnostics channel tracing

Layer / File(s)	Summary
Server listen channel lifecycle `src/js/node/net.ts`	Defines diagnostics channels and emits server listen start, completion, and error events with server, options, and error payloads.
Socket channel publication `src/js/node/net.ts`	Publishes accepted server sockets and connected client sockets when subscribed.
Diagnostics channel integration tests `test/js/node/test/parallel/test-diagnostics-channel-net.js`, `test/js/node/net/node-net.test.ts`	Tests socket events, listen lifecycle payloads, port-collision errors, cleanup, and normalized snapshots.

Suggested reviewers: cirospaciari

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR improves node:net diagnostics, but the linked issue is about making build output run on Node.js and its listed blockers are not addressed.	Clarify the linked issue or retarget the PR to a matching Node.js build-compatibility task; otherwise add changes that address the issue's stated blockers.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	All changes stay within node:net diagnostics_channel support and associated tests, with no obvious unrelated code paths introduced.
Title check	✅ Passed	The title clearly summarizes the main change: adding diagnostics_channel events to node:net.
Description check	✅ Passed	The description covers what changed and how it was verified, even though it doesn't use the exact template headings.

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

robobun · 2026-07-15T04:18:22Z

^{Updated 12:30 AM PT - Jul 15th, 2026}

❌ @robobun, your commit 4fca107 has 3 failures in Build #73185 (All Failures):

test/js/third_party/grpc-js/test-tonic.test.ts - code 1 on 🍎 14 aarch64
test/cli/run/require-cache.test.ts - code 1 on 🍎 26 aarch64
test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT on 🐧 13 x64-asan

🧪 To try this PR locally:

bunx bun-pr 34209

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

bun-34209 --bun

robobun · 2026-07-15T04:18:22Z

Status: all four documented net diagnostics channels now fire, payloads and ordering pinned against Node v26.3.0.

Repro (fails on stock 1.4.0 with all 0s, passes with this branch):

bun -e 'import dc from "node:diagnostics_channel"; import net from "node:net"; const G=["net.client.socket","net.server.socket","tracing:net.server.listen:asyncStart","tracing:net.server.listen:asyncEnd"]; const h=new Map(); for(const n of G)dc.subscribe(n,()=>h.set(n,(h.get(n)??0)+1)); const s=net.createServer(c=>c.end()); await new Promise(r=>s.listen(0,"127.0.0.1",r)); await new Promise(r=>{const c=net.connect(s.address().port,"127.0.0.1");c.on("close",r);c.on("error",r)}); s.close(); let bad=0; for(const n of G){const c=h.get(n)??0;console.log((c?" ok":"BUG"),n,"x"+c);if(!c)bad++} process.exit(bad?86:0)'

CI on 4fca107 (build 73185): everything this PR adds or touches is green on every lane. The remaining reds are all [pre-existing] or [flaky] (passed on retry) and unrelated to this diff:

test	tag	lane	note
test-worker-message-port-transfer-terminate.js	pre-existing	x64-asan	JSC assertion in Worker
require-cache.test.ts	pre-existing
node-http-connect.test.ts	flaky	win x64
fetch-http3-adversarial.test.ts	flaky	win aarch64	HTTP3StreamReset
net-mongodb-pattern-leak.test.ts	flaky	x64	RSS noise (11MB vs 8MB bound), fixed module-load overhead, not a per-connection leak
multi-run.test.ts	flaky	darwin x64
s3.leak.test.ts	flaky	aarch64
proxy-stress-errors.test.ts	flaky
test-net-write-slow.js	flaky	darwin aarch64	ERR_STREAM_PUSH_AFTER_EOF at net.ts:531 (data handler), not touched by this diff

Ready for review.

github-actions · 2026-07-15T04:22:23Z

This PR may be a duplicate of:

node:diagnostics_channel: sync with Node 26 + subsystem channels #32628 - Also adds net.client.socket, net.server.socket, and tracing:net.server.listen diagnostics_channel publishing to src/js/node/net.ts as part of a broader Node 26 diagnostics_channel sync

🤖 Generated with Claude Code

robobun · 2026-07-15T04:23:00Z

Overlaps the net.ts hunk of #32628 (draft, broader Node 26 diagnostics_channel sync across ~10 subsystems). This PR is the net-only slice and is ready to merge standalone; if #32628 lands first this becomes a no-op rebase.

Two small differences from #32628's net.ts changes:

asyncStart's options here is normalizeArgs(arguments)[0], which is exactly what Node passes (so listen(0, "127.0.0.1") publishes {port: 0, host: "127.0.0.1"}, matching Node). node:diagnostics_channel: sync with Node 26 + subsystem channels #32628 reconstructs {port, host} from already-parsed locals.
Each publish here is gated on the specific sub-channel (netServerListen.asyncStart.hasSubscribers) rather than netServerListen.hasSubscribers, so it works with the current TracingChannel which lacks that getter. node:diagnostics_channel: sync with Node 26 + subsystem channels #32628 adds the getter as part of its diagnostics_channel.ts rewrite.

coderabbitai

Actionable comments posted: 1

🤖 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 3409-3412: Move the netServerListen.asyncStart.publish call in the
listen flow to immediately after argument normalization and the _handle check,
before validating ports, paths, or option objects. Preserve the existing
publication payload and subscriber guard so rejected listen attempts still
notify asyncStart subscribers in the expected order.

🪄 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: f4222060-6927-49d4-9521-cf12b3203a50

📥 Commits

Reviewing files that changed from the base of the PR and between f12e9f9 and 7bbbbc6.

📒 Files selected for processing (3)

src/js/node/net.ts
test/js/node/net/node-net.test.ts
test/js/node/test/parallel/test-diagnostics-channel-net.js

Moves the ERR_SERVER_ALREADY_LISTEN check and the asyncStart publish to the top of Server.prototype.listen, before port/path/option validation, to match Node's event order: listen({port:-1}) publishes asyncStart first and then throws ERR_SOCKET_BAD_PORT; listen() on an already-listening server throws without publishing. A new subprocess test pins both orderings against Node v26.3.0 output.

…bust under conservative GC The napi common harness loads node:net at module init, so the new require('node:diagnostics_channel') in net.ts shifts stack layout during these tests' setup. JSC's conservative stack scan can then keep the tracked object alive across a single gc() call. Wrap the object creation in an IIFE and retry gc() up to 10 times, mirroring test_finalizer/test.js in the same directory. Same change as #32628; applied here so this PR is green standalone. Co-authored-by: Ciro Spaciari <6379399+cirospaciari@users.noreply.github.com>

claude

No bugs found this pass, but deferring to a human since this touches Server.prototype.listen / Socket.prototype.connect hot paths and reorders the _handle check ahead of port validation (changes error precedence to match Node), plus bundles two unrelated NAPI finalizer-test tweaks.

What was reviewed:

Publish sites match the _http_client.ts/http2.ts/dgram.ts pattern (hasSubscribers-gated, no allocation on the cold path).
normalizeArgs(arguments) at the top of listen() is pure and can't throw; the moved _handle check now matches Node's ordering and is pinned by the third subprocess test.
The cluster-worker listenOnPrimaryHandle gap I raised earlier was shown to be Node-parity for checkBindError; the remaining kRealListen-throws case is deferred to #34076.
The NAPI test_function/test_instance_data IIFE + gc-loop changes are the standard conservative-GC workaround and don't change what's asserted.

Extended reasoning...

Overview

Adds diagnostics_channel publishing to src/js/node/net.ts at five sites: net.client.socket in Socket.prototype.connect, net.server.socket in onconnection, and the tracing:net.server.listen asyncStart/asyncEnd/error sub-channels in Server.prototype.listen and kRealListen. Also moves the ERR_SERVER_ALREADY_LISTEN check to the top of listen() (before port/path validation) so asyncStart fires ahead of option validation, matching Node v26.3.0. Ships three subprocess tests in node-net.test.ts plus Node's upstream test-diagnostics-channel-net.js verbatim. Separately, two NAPI tests (test_function, test_instance_data) are wrapped in IIFEs with a 10× global.gc() loop to survive JSC's conservative stack scan.

Security risks

None. Diagnostics-channel publishes are observability-only; payloads are the socket/server objects the caller already holds. No new input parsing, no auth/crypto/permissions surface.

Level of scrutiny

Medium-high. The publish calls themselves are mechanical and mirror the existing pattern in _http_client.ts, http2.ts, and dgram.ts — that part I'd approve on its own. What warrants a human look is the _handle check reordering in Server.prototype.listen: it changes which error wins when an already-listening server is passed an invalid port (now ERR_SERVER_ALREADY_LISTEN instead of ERR_SOCKET_BAD_PORT). This is the Node-correct order and is snapshot-pinned, but it's a user-observable behavior change in a core module and worth a maintainer glance.

Other factors

CodeRabbit's ordering concern was addressed in ffd2593 and confirmed resolved.
My earlier note about the cluster-worker listenOnPrimaryHandle path not reaching netServerListen.error.publish was checked empirically against Node v26.3.0 by the author — Node also publishes only asyncStart on that path, so it's parity as-is; the residual kRealListen-throws-in-worker case is behind a pre-existing bug tracked by #34076.
The two NAPI test edits are unrelated to the PR title (CI-flake hardening for conservative GC). They're benign — same assertions, just IIFE-scoped so the tracked object's stack slot is off-frame before gc() — but their presence is another reason a human should skim the diff.
Overlaps draft #32628; the author documented the two intentional deltas (per-sub-channel hasSubscribers gating, and passing normalizeArgs(arguments)[0] as options).
Tests use expect(stderr).toBe(""), which the file's existing subprocess tests also do; finder agents flagged this and verifiers ruled it out.

github-actions Bot added the claude label Jul 15, 2026

[autofix.ci] apply automated fixes

7bbbbc6

coderabbitai Bot reviewed Jul 15, 2026

View reviewed changes

Comment thread src/js/node/net.ts Outdated

claude Bot reviewed Jul 15, 2026

View reviewed changes

Comment thread src/js/node/net.ts

claude Bot reviewed Jul 15, 2026

View reviewed changes

This was referenced Jul 16, 2026

node:diagnostics_channel: sync with Node 26 + subsystem channels #32628

Open

diagnostics_channel: add BoundedChannel/boundedChannel, withStoreScope; make TracingChannel props non-enumerable #34604

Open

Navigation Menu

Uh oh!

node:net: publish diagnostics_channel events (net.client.socket, net.server.socket, net.server.listen) - #34209

node:net: publish diagnostics_channel events (net.client.socket, net.server.socket, net.server.listen)#34209
robobun wants to merge 4 commits into
mainfrom
farm/080071de/net-diagnostics-channel

robobun commented Jul 15, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 15, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

robobun commented Jul 15, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 15, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 15, 2026

Uh oh!

robobun commented Jul 15, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robobun commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Repro

Fix

Verification

Uh oh!

coderabbitai Bot commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

robobun commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 15, 2026

Uh oh!

robobun commented Jul 15, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jul 15, 2026 •

edited

Loading

coderabbitai Bot commented Jul 15, 2026 •

edited

Loading

robobun commented Jul 15, 2026 •

edited

Loading

robobun commented Jul 15, 2026 •

edited

Loading