node:diagnostics_channel: sync with Node 26 + subsystem channels (+32 upstream tests) - #32628
cirospaciari wants to merge 68 commits into
Conversation
Port lib/diagnostics_channel.js from Node v26.3.0: adds boundedChannel()/ BoundedChannel, Channel.prototype.withStoreScope (store scopes built on DisposableStack), copy-on-write subscriber lists so unsubscribing during publish doesn't skip later subscribers, TracingChannel rebuilt on BoundedChannel with hasSubscribers, no-subscriber early exit, non-thenable warnings and custom-thenable passthrough, and a WeakReference that holds the channel strongly while it has active subscriptions so subscriptions survive GC. Add AsyncLocalStorage.prototype.withScope (RunScope), used by the new store-scope API. The scope updates the async context directly and restores the previous store on dispose instead of going through enterWith, so it does not arm the end-of-tick context cleanup. Publish the diagnostics channels Node provides in the corresponding subsystems: - worker_threads: 'worker_threads' on Worker construction - child_process: 'child_process' on ChildProcess construction and the 'child_process.spawn' tracing channel around spawning - net: 'net.client.socket', 'net.server.socket' and the 'net.server.listen' tracing channel - http server: 'http.server.request.start', 'http.server.response.created', 'http.server.response.finish' Sync all 67 test-diagnostics-channel-* tests from Node v26.3.0 (32 new, 8 updated). 59 pass; the remaining 7 newly-added tests depend on features Bun does not implement yet (module loader channels, Web Locks, v8.queryObjects, net.Socket-backed http server sockets) and are listed in test/expectations.txt with reasons.
|
Updated 9:52 PM PT - Sep 7th, 2026
⏳ @robobun, your commit c6c2296 is still building in |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Wrap the CommonJS require path in the "module.require" tracing channel the
way Node's wrapModuleLoad does: start/end/error events carry
{ parentFilename, id } (and result on success).
To keep require() free for processes that never use diagnostics_channel,
the channel lives in a small internal holder module
(internal/require_tracing) that node:diagnostics_channel fills in when it
is first loaded. overridableRequire only checks that holder; when nobody
subscribed it tail-calls the real implementation, which moved to a new
overridableRequireImpl builtin (registered as a lazy global builtin getter
like requireESM, so its source is still not parsed during global object
construction).
Enables test-diagnostics-channel-module-require(-error).js; their
expectations entries are removed.
Add v8.queryObjects(ctor[, options]) backed by a JSC heap scan: after a full synchronous GC, every live object whose prototype chain contains ctor.prototype is collected (walking prototypes structurally so no proxy traps or getters run during heap iteration). The JS wrapper mirrors Node's: validates the constructor and options, supports the 'count' and 'summary' formats, and emits the same one-time ExperimentalWarning. Vendors Node v26.3.0's test-v8-query-objects.js, which passes verbatim, and removes the expectations entry for test-diagnostics-channel-memory-leak.js, which now passes too.
Wrap dynamic import() in the "module.import" tracing channel like Node's
ESM loader: start/end fire synchronously around starting the import and
asyncStart/asyncEnd/error fire when the promise settles, with
{ parentURL, url } context.
The channel handling lives in JS: internal/require_tracing becomes
internal/module_tracing and now holds both the module.require and
module.import channels (installed by node:diagnostics_channel when it
loads) plus a traceImport() helper that runs the import through
TracingChannel.tracePromise. moduleLoaderImportModule moves its body into
a static helper and only diverts through traceImport when
node:diagnostics_channel has been loaded and the channel has subscribers,
so dynamic import is unchanged for everything else.
test-diagnostics-channel-module-import-error.js passes and its
expectations entry is removed. test-diagnostics-channel-module-import.js
still has an entry: the transpiler rewrites the statically-analyzable
import("http") literal to "node:http" before the loader sees it, so the
published url differs from Node's for that one case.
The tracing branch in moduleLoaderImportModule used a block-scoped ThrowScope; destroying it unreleased simulates a throw, so falling through to the real import (which declares its own scope) tripped JSC's exception check validation and aborted under BUN_JSC_validateExceptionChecks (the ASAN CI job). Declare the scope at function level and release it through RELEASE_AND_RETURN for the tail call instead, and drop the redundant check after toBoolean(), which cannot throw.
|
CI note: in build #64127 (the exception-scope fix commit), |
Create net.client.socket / net.server.socket / net.server.listen at module load like _http_client.ts (and Node's net.js) instead of lazily inside Socket.prototype.connect / listen / onconnection. This drops the per-call lazy-init branch and keeps diagnostics_channel's module evaluation out of the first connect() call, where the extra allocation work sat right in the window that GC-sensitive tests like test-net-connect-memleak.js measure.
|
Follow-up on the Alpine Digging into history: the test does not appear in the Alpine x64 shard logs of the last five main builds — the vendored-test sharding never places it on Alpine there. This PR adds 34 vendored test files, which shifts the shard boundaries so the test now runs on Alpine, where its single-
|
The vendored test asserts the implicit 'connect' once-listener closure is collected after a single gc(). On the Alpine (musl) CI runners that collection is intermittently not observed within one cycle, so the assertion fails; nothing holds the closure structurally and the test passes on glibc Linux, macOS, Windows and FreeBSD, where it keeps running. The test only started landing on the Alpine shards because this branch adds test files and shifts the shard boundaries. Scoped with the existing MUSL ABI modifier; tracked in #20627.
|
Resolution for the Alpine flake: e6ec58c adds a |
test_function and test_instance_data fail on the release-ASAN CI runners (the spawned bun exits 1 with no output, 4/4 in-job attempts) while passing on every other platform and on a local ASAN debug build. They only started running on the ASAN shards because the growing test list shifted the shard boundaries. Scoped to the existing ASAN modifier; tracked in #32642.
|
Two node-api suites (test_function, test_instance_data) turned out to fail consistently on the release-ASAN runners once the resharding placed them there — same exposure pattern as the Alpine memleak test, no involvement of this PR's code, and they pass locally on an ASAN debug build. Filed #32642 and quarantined them with |
…vative GC
Both tests create an object with a napi finalizer, null the JS reference,
call global.gc() once, and expect common.mustCall() to observe the
finalizer before process exit. JSC's GC conservatively scans the native
stack, so when require()'s call depth changes (as this PR's
overridableRequire wrapper does) a stale pointer to the just-created
object can survive in an unscrubbed stack slot on the release-ASAN
layout, the object is not collected by a single gc() and the mustCall
check fails at exit with 'Expected exactly 1, actual 0' on stdout.
Move the object creation into an IIFE so the creating frame is gone
before gc() runs, and retry gc() a bounded number of times. This mirrors
what test_finalizer/test.js in the same directory already does for the
same reason ('to be compatible with non-V8 JS engines').
Also capture the spawned test's stdout in the node-napi harness and
print it when the test fails, so the mustCall diagnostic is visible in
CI instead of presenting as a bare 'exit code 1'.
Removes the [ ASAN ] expectations entries for these suites; both pass
5/5 on a local release-ASAN build with the full CI environment.
Closes #32642
|
Pushed ee6a68d to address #32642: the two quarantined node-api suites weren't a pre-existing ASAN issue, they were tripped by the extra One optional follow-up left as a note on #32642: the no-subscriber fast path in |
|
CI status: every test job in build #64232 passed (285/286). The single failed job is |
|
Third retry of the darwin-26 aarch64 test job hit the same |
|
It'd be a huge W when this is done eventually 👀 |
…annel-node26 # Conflicts: # src/js/builtins/BunBuiltinNames.h # src/js/node/child_process.ts
…response.created
Remove the three [ FAIL ] expectations entries this branch added by deleting
the vendored tests behind them instead of shipping known-failing files:
- test-diagnostics-channel-web-locks.js: needs the Web Locks API
(navigator.locks), which Bun does not implement.
- test-diagnostics-channel-http.js: asserts the socket passed to the
http.server.* channels is a net.Socket; Bun's node:http server hands out a
Duplex wrapper. Fixing that belongs to the node:http compat work.
- test-diagnostics-channel-module-import.js: asserts the published url is the
literal specifier ('http'); Bun's transpiler rewrites import("http") to
"node:http" before the loader publishes module.import.
Also drop the unrelated [ MUSL ] quarantine of test-net-connect-memleak.js.
Deleting test-diagnostics-channel-http.js left http.server.response.created
with no coverage, so add a test for it to the diagnostics_channel suite.
Add the missing exception check after constructEmptyArray in queryObjects.
…le scope Requiring node:diagnostics_channel lazily from the Worker and ChildProcess constructors meant the module was evaluated after user code had already run. "postMessageToThread survives a tampered Map prototype" clobbers Map.prototype and then constructs a Worker, so diagnostics_channel loaded onto the tampered prototype and threw out of the constructor. Node creates both channels at module scope (lib/internal/worker.js, lib/internal/child_process.js), which is also what net.ts and _http_server.ts already do here. Do the same, so the channels are built while the module graph loads rather than on first use. Also route diagnostics_channel's own Map operations through the $-prefixed intrinsics instead of the user-overridable prototype methods.
…y, self-include - require(id) forwards 1 arg so the native $require keeps its skip-options gate (argumentCount distinguishes the arities again). - Drop the test-net-connect-memleak.js expectations entry; the file was removed on main and the entry matched nothing. - NodeDiagnosticsChannel.cpp includes its own header like NodeV8Module.cpp, so declaration and definition compile in one TU.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
connectionListenerHTTP1 (emit('connection', foreignSocket) and http2's
allowHTTP1 ALPN fallback) constructs req/res and emits 'request' without
publishing request.start/response.created/response.finish; in Node both
entry paths converge on parserOnIncoming which publishes all three.
dc.channel() returns the per-name singleton so these are the same channel
objects _http_server.ts uses. Accepted upgrades still return early before
any publish, matching the native dispatch path.
|
Closed #27881 ( |
…erver.socket
connect({ fd }) is Bun's equivalent of Node's new Socket({ handle })
(cluster-worker accepts, IPC-transferred handles, child stdio pipes), which
never reaches Node's connect() and so never publishes net.client.socket.
onClusterConnection is the cluster-worker analog of onconnection and now
publishes net.server.socket like it does.
… like Node Node publishes asyncEnd (and listen-failure error) from setupListenHandle, which both primary and worker listens reach. Bun's round-robin worker path ends in kClusterFauxListen and its shared-fd path catches kRealListen locally; neither published, so a worker listen left the span open.
robobun
changed the title
…annel-node26 # Conflicts: # src/js/node/_http_server.ts # src/js/node/v8.ts
…annel-node26 # Conflicts: # src/js/node/_http_server.ts # src/js/node/net.ts
…nale The previous wording attributed the GC survival to the conservative stack scanner without proof. Match test_finalizer/test.js's upstream comment instead; the test shape itself is unchanged.
|
Heads-up for the rebase: #40665 fixes the dropped-tick case this PR patches in |
…annel-node26 # Conflicts: # src/jsc/bindings/ZigGlobalObject.cpp # src/jsc/bindings/ZigGlobalObject.h
…e-net.test.ts The previous merge kept stale branch copies of these three files, dropping main's changes to them (WebKit upgrade signature changes, JSBufferList removal, StandaloneGlobalObject, ~180 lines of net tests). They are now main's versions plus only this PR's deltas: hasModuleImportSubscribers, tryTraceModuleImport and the moduleLoaderImportModule split, and the net diagnostics channels describe block.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Confirmed 8ee314d fixes both merge-regression blockers from my previous review: ZigGlobalObject.{cpp,h} now diff cleanly against main a3e0ab6 with only the hasModuleImportSubscribers flag + tryTraceModuleImport/moduleLoaderImportModule split, and node-net.test.ts now only adds the new net diagnostics channels and fd-attached sockets block — the seven pause/RST/pauseOnConnect tests are back.
Extended reasoning...
The last review flagged two 🔴 merge-regression blockers introduced by d9bf7f6; commit 8ee314d ("Fix bad merge d9bf7f6") addresses both. I re-diffed src/jsc/bindings/ZigGlobalObject.{cpp,h} and test/js/node/net/node-net.test.ts against base a3e0ab6: the former now carries only this PR's intended additions (the moduleLoaderImportModuleImpl extraction, tryTraceModuleImport, the wrapper moduleLoaderImportModule gated on hasModuleImportSubscribers, and the header's bool field) with no dropped main-side content — StandaloneGlobalObject, JSBufferList.h, the referrer parameter, and the *Mux* handlers are all present again. The test file's diff is purely additive. Nothing else new was ruled out this run beyond re-verifying those two files; the inline finding on child_process.ts covers the one new concern.
The child_process diagnostics channel publishes from the ChildProcess constructor (same placement as Node), so subscribers see the object before spawn() assigns #handle. The connected getter only handled the post-exit null and threw on the pre-spawn undefined; Node reports false for both.
This branch has not been deployed
5 participants
Summary
lib/diagnostics_channel.jsfrom Node v26.3.0:boundedChannel()/BoundedChannelexports,Channel.prototype.withStoreScope, copy-on-write subscriber lists (sync unsubscribe during publish no longer skips subscribers),TracingChannelrebuilt onBoundedChannel(hasSubscribers, early exit when unsubscribed, non-thenable warning, custom-thenable passthrough), and aWeakReferencethat keeps a channel alive while it has active subscriptions so they survive GC.worker_threads(Worker construction)child_process+child_process.spawntracing channel (spawn success/ENOENT/EACCES paths)net.client.socket,net.server.socket,net.server.listentracing channelhttp.server.request.start,http.server.response.created,http.server.response.finishtest-diagnostics-channel-*files from Node v26.3.0 (30 new, 8 updated to current upstream content). Now 30: diagnostics_channel: vendor the two http channel tests (+2) #34641 broughttest-diagnostics-channel-http.jsback into this branch and addedtest-diagnostic-channel-http-response-created.js, so withtest-v8-query-objects.jsbelow this diff adds 32 upstream test files and modifies 8. Every vendored file added or modified here is byte-identical to Node v26.3.0 upstream.Also implemented, because vendored tests in this sync depend on them:
module.requiretracing channel around CommonJSrequire()(test-diagnostics-channel-module-require(-error)pass)module.importtracing channel around dynamicimport()(test-diagnostics-channel-module-import-errorpasses)v8.queryObjects(), required bycommon/gc.js'scheckIfCollectableByCounting(Node'stest-v8-query-objects.jsis vendored and passes;test-diagnostics-channel-memory-leakpasses)cleanupAsyncHooksDatasoals.enterWith(x); process.nextTick(cb)did not dropcb; AsyncLocalStorage: drop enterWith() frames at the event-loop boundary, stop allocating per await/then #41190 on main has since replaced that whole mechanism, so the merge takes main's implementation and keeps only the regression test intest/js/node/async_hooks/AsyncLocalStorage.test.ts, which passes on it.)Rather than vendor known-failing tests behind
[ FAIL ]entries, the two upstream tests Bun cannot pass are not vendored at all. They are known gaps, to add when the blocking feature lands:test-diagnostics-channel-web-locksnavigator.locks) is not implementedtest-diagnostics-channel-module-importurlis the literal specifier ('http'); Bun's transpiler rewritesimport("http")to"node:http"before the loader publishesmodule.import, so Bun publishes'node:http'. The channel itself works;test-diagnostics-channel-module-import-errorcovers it.This PR also adds tests for the
http.server.response.createdchannel and for the upgrade-gating of all threehttp.server.*channels totest/js/node/diagnostics_channel/diagnostics_channel.test.ts. They fail underUSE_SYSTEM_BUN=1and pass on this build.http.server.request.startandhttp.server.response.finishstay covered by the vendoredtest-diagnostics-channel-http-server-start.js.Why the napi test changes are here
test_functionandtest_instance_datacreate an object with a napi finalizer, null the JS reference, callglobal.gc()once, and expect the finalizer before exit. They started failing on the release-ASAN lanes once this PR'srequire()changes landed. The tests now create the object inside an IIFE and retrygc()a bounded number of times, aligning them with the cross-engine conventiontest_finalizer/test.jsin the same directory already uses (its upstream comment: IIFE instead of{}for engines that do not support scoped variables). The napi harness also captures the spawned test's stdout on failure so themustCalldiagnostic is visible in CI instead of a bare exit code 1.Test plan
test-diagnostics-channel-*tests pass, run CI-style (bun-debug run --config=bunfig.node-test.toml,BUN_GARBAGE_COLLECTOR_LEVEL=1)test-async-local-storage-*,test-async-hooks-*,test-als-*andtest-asyncresource-*vendored suites passtest/js/node/diagnostics_channel/andtest/js/node/async_hooks/AsyncLocalStorage.test.tspass; the newhttp.server.response.createdtest and theenterWith+process.nextTicktest fail underUSE_SYSTEM_BUN=1test/js/node/test/parallel/diffs clean againstraw.githubusercontent.com/nodejs/node/v26.3.0Linked issues
Fixes #32472
Fixes #29586
Fixes #27805
Notes:
node:child_processAPI (spawn/fork/exec/execFile) publishes these channels, matching Node.Bun.spawnitself intentionally does not publish them. Overlaps with open PRs child_process: publish diagnostics_channel events for node:child_process spawn/fork/exec/execFile #30080 (child_process channels), http: publish to http.server.* diagnostics_channel channels #29588 (http.server.* channels) and fix: add hasSubscribers property to TracingChannel #27881 (TracingChannel.hasSubscribers), which each implement a slice of the same surface; this PR covers those slices as part of the full Node 26 sync.tracingChannel().hasSubscribersnow exists (covered bytest-diagnostics-channel-tracing-channel-has-subscribers.js).no test proof · iteration 35 · 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