diagnostics_channel: publish tracing:module.require and tracing:module.import - #34212
diagnostics_channel: publish tracing:module.require and tracing:module.import#34212robobun wants to merge 5 commits into
Conversation
…e.import
Node.js publishes on the built-in TracingChannels tracing:module.require:*
around every CJS require() (context {id, parentFilename}) and
tracing:module.import:* around every dynamic import() (context {parentURL,
url}). Instrumentation loaders (OpenTelemetry, APM module patchers) subscribe
to these to hook module loads. Bun never published on any of the eight
channels.
For require(): the bound require() wrapper in builtins/CommonJS.ts now
consults a lazily-created tracingChannel('module.require') and runs the load
through traceSync when subscribed, matching Node's wrapModuleLoad. The
unsubscribed fast path adds only a cached internal-module read plus a
hasSubscribers getter call.
For dynamic import(): moduleLoaderImportModule is native, so subscribing to
any tracing:module.* channel now flips a one-way de-opt flag on the global
(set from markActive() via a new host function). When set, the hook routes
its result promise through a JS traceDynamicImport helper which wraps it with
tracePromise. No subscriber means one extra boolean read.
Also adds the TracingChannel.prototype.hasSubscribers getter (Node v22.0.0),
which the module loader uses to gate the slow path.
|
Updated 12:57 AM PT - Jul 15th, 2026
❌ @robobun, your commit bdbd151 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34212That installs a local version of the PR into your bun-34212 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
#32628 is a draft superset of this: it implements This PR is the standalone slice (just the two module channels, plus |
WalkthroughChanges
Module tracing
N-API garbage-collection test reliability
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/ZigGlobalObject.cpp (1)
3572-3579: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute NodeVM imports through the tracer before returning.
NodeVM::importModulereturns its promise directly, while its exception path exits throughRETURN_IF_EXCEPTION. These imports therefore emit nostart/end/async*/errorevents despite tracing being active. Convert any pending exception to a rejected promise and pass every non-null result throughtraceDynamicImport.🤖 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 `@src/jsc/bindings/ZigGlobalObject.cpp` around lines 3572 - 3579, Update the NodeVM::importModule handling in the needsTracing flow to convert any pending exception into a rejected promise instead of returning through RETURN_IF_EXCEPTION, then pass every non-null result to traceDynamicImport before returning it. Preserve the existing direct-return behavior when tracing is not active.
🤖 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/diagnostics_channel.ts`:
- Around line 68-69: In the ActiveChannel activation logic, replace the userland
`channel.name.startsWith` call with the primordial
`StringPrototypeStartsWith.$call` using the same channel name and prefix.
Preserve the existing conditional and module-tracing initialization behavior
while ensuring the prefix check cannot invoke mutable userland code.
In `@test/js/node/diagnostics_channel/diagnostics_channel.test.ts`:
- Around line 412-414: Reorder assertions in the affected subprocess test blocks
around the stdout parsing and event expectations: validate parsed stdout and its
expected result first, then assert stderr, and check exitCode last. Apply this
consistently to the blocks near the existing Promise.all assertions, including
the additionally referenced cases.
---
Outside diff comments:
In `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 3572-3579: Update the NodeVM::importModule handling in the
needsTracing flow to convert any pending exception into a rejected promise
instead of returning through RETURN_IF_EXCEPTION, then pass every non-null
result to traceDynamicImport before returning it. Preserve the existing
direct-return behavior when tracing is not active.
🪄 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: 6b759b5d-93bf-4a2b-a33e-92b37e984ff2
📒 Files selected for processing (8)
src/js/builtins/CommonJS.tssrc/js/internal/module_tracing.tssrc/js/node/diagnostics_channel.tssrc/jsc/bindings/NodeDiagnosticsChannel.cppsrc/jsc/bindings/NodeDiagnosticsChannel.hsrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigGlobalObject.htest/js/node/diagnostics_channel/diagnostics_channel.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/diagnostics_channel/diagnostics_channel.test.ts (1)
348-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise every subchannel and overlapping subscriptions.
Checking only
asyncEndanderrorindependently would miss a getter that omitsstart,end, orasyncStart, or incorrectly becomes false when one of several active subscriptions is removed.🤖 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/diagnostics_channel/diagnostics_channel.test.ts` around lines 348 - 361, Expand the “hasSubscribers reflects sub-channel state” test to cover subscriptions on every tracing subchannel: start, end, asyncStart, asyncEnd, and error. Add overlapping-subscription assertions so hasSubscribers remains true while any subscription is still active, including after unsubscribing one of multiple subscribed handlers, and returns false only after all are removed.
🤖 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/diagnostics_channel/diagnostics_channel.test.ts`:
- Around line 469-522: Update the expected payloads in the diagnostics channel
test to assert the Node shape: use the emitted `id` field instead of `url`, and
expect `parentURL` as a URL-valued value rather than a plain string. Apply this
consistently to every `tracing:module.import:*` event in the expectation while
preserving the existing event ordering and result/error flags.
---
Outside diff comments:
In `@test/js/node/diagnostics_channel/diagnostics_channel.test.ts`:
- Around line 348-361: Expand the “hasSubscribers reflects sub-channel state”
test to cover subscriptions on every tracing subchannel: start, end, asyncStart,
asyncEnd, and error. Add overlapping-subscription assertions so hasSubscribers
remains true while any subscription is still active, including after
unsubscribing one of multiple subscribed handlers, and returns false only after
all are removed.
🪄 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: 052c30d7-9395-4cde-851a-fe5652ebcbaf
📒 Files selected for processing (2)
src/js/node/diagnostics_channel.tstest/js/node/diagnostics_channel/diagnostics_channel.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The require() wrapper added in this PR changes the CJS module load call depth, which on release-ASAN layouts can leave a stale pointer to the finalizer target in an unscrubbed stack slot that JSC's conservative stack scan keeps alive past a single gc(). Create the target inside an IIFE so its frame is gone before gc() runs, and retry gc() a bounded number of times, matching the pattern already used by test_finalizer/test.js for the same reason. Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
|
CI at bdbd151 (build #73197): the two napi finalizer tests that failed in the previous build now pass.
Ready for review. |
There was a problem hiding this comment.
No bugs found, and the earlier primordial-startsWith nit is addressed in 9142a5b — but this touches the per-call require() builtin wrapper and the native moduleLoaderImportModule hook plus new ZigGlobalObject state, so it's worth a human look for the hot-path cost and the #32628 overlap decision.
Checked: exception-scope handling in the new traceDynamicImport C++ helper and its three call sites (rejected-promise paths covered); the LazyProperty initializer for m_traceDynamicImportFunction (visited via FOR_EACH_GLOBALOBJECT_GC_MEMBER); the require() fast path still forwards arguments unchanged when unsubscribed; hasSubscribers getter matches Node's five-channel OR. The NAPI test IIFE/gc-retry changes look like standard conservative-stack-scan hardening.
Extended reasoning...
Overview
This PR wires Bun's CJS require() and dynamic import() into Node's tracing:module.require / tracing:module.import diagnostics channels. It touches: the require() builtin wrapper in src/js/builtins/CommonJS.ts (runs on every require), GlobalObject::moduleLoaderImportModule in ZigGlobalObject.cpp (native dynamic-import hook), a new LazyProperty<JSFunction> and bool de-opt flag on ZigGlobalObject, a new C++ host function, a new internal/module_tracing JS module, and the TracingChannel.prototype.hasSubscribers getter. It also bundles unrelated NAPI test hardening (IIFE-scoping finalizer targets + gc retry loop) in bdbd151.
Security risks
None identified. The change is observability-only; the new code paths run user-provided subscriber callbacks, but that is the existing diagnostics_channel contract and errors are already caught by ActiveChannel.publish. No new external input parsing, auth, or filesystem/permission surface.
Level of scrutiny
High. src/js/builtins/CommonJS.ts:require() is bound to every CommonJSModule instance and runs on every require() call — the unsubscribed path now adds an internal-module-registry lookup plus a getter call per invocation. moduleLoaderImportModule is the sole native entry point for dynamic import() and the new code adds a JS re-entry (traceDynamicImport) at three return sites with fresh throw scopes. ZigGlobalObject.h is explicitly flagged in CLAUDE.md as a place to avoid new fields. These are not mechanical changes; a maintainer should sign off on the per-require overhead being acceptable and confirm the C++ exception-scope discipline.
Other factors
- The author explicitly flags overlap with draft #32628 (a superset that also covers
module.requireviaoverridableRequiresoModule.prototype.requireis traced) and #33449 (hasSubscribers), and asks whether to close in favor of the larger PR — that's a maintainer call, not something to auto-approve past. - My earlier inline nit (primordial
startsWith) was addressed; the currentmarkActiveusesStringPrototypeStartsWith.$call. - Tests are thorough (event ordering, context shape, result identity, error paths, unsubscribed path) and were verified to fail on release / pass with fix under ASAN and
BUN_JSC_validateExceptionChecks=1. - The bundled NAPI test edits are logically separate from the diagnostics_channel change; they look correct but widen the PR's scope.
1 participant
Node.js publishes on the built-in TracingChannels
tracing:module.require:{start,end,error}around every CJSrequire()andtracing:module.import:{start,end,asyncStart,asyncEnd,error}around every dynamicimport(). Instrumentation loaders (OpenTelemetry, APM module patchers) subscribe to these to hook module loads. Bun never published on any of the eight channels.Repro
Node v26: all eight channels fire (
requirestart/end x2, error x1;importstart/end/asyncStart/asyncEnd x2, error x1). Bun before this change fires zero.Fix
require(): the bound
require()wrapper insrc/js/builtins/CommonJS.tsnow consults a lazily-createdtracingChannel('module.require')(cached in the newinternal/module_tracingmodule) and runs the load through itsstart/end/errorchannels when subscribed, matching Node'swrapModuleLoad. The context is{id, parentFilename}withresult/erroradded. The unsubscribed fast path adds only a cached internal-module field read plus ahasSubscribersgetter call.import():
GlobalObject::moduleLoaderImportModuleis native C++ with no JS layer. Subscribing to anytracing:module.*channel now flips a one-wayhasModuleTracingSubscribersde-opt flag on the global (set frommarkActive()via a new host function, same pattern ashasOverriddenModuleResolveFilenameFunction). When set, each return site routes its result promise through a JStraceDynamicImporthelper which wraps it withtracePromise, matching Node'sModuleLoader#import. The context is{parentURL, url}. No subscriber means one extra boolean read perimport().Also adds the
TracingChannel.prototype.hasSubscribersgetter (Node v22.0.0), which the module loader uses to gate the slow path. This overlaps with #33449; whichever lands second just drops the duplicate getter.Verification
bun bd test test/js/node/diagnostics_channel/diagnostics_channel.test.tscovers the full event sequence for successful and failingrequire()/import()(all eight channels, context shape, result/error propagation), the identity between the:endcontext'sresultand therequire()return value, the unsubscribed path, and the newTracingChannel.hasSubscribersgetter. All new tests fail on the release build and pass with the fix. Verified clean underBUN_JSC_validateExceptionChecks=1.[review] gate passed · iteration 2 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 2
evidence per changed file