diagnostics_channel: add BoundedChannel/boundedChannel, withStoreScope; make TracingChannel props non-enumerable - #34604
Conversation
…Channel props non-enumerable Close four gaps against Node.js v26: - Export boundedChannel() and the BoundedChannel class, which bundles start/end Channels with subscribe/unsubscribe/withScope/run helpers. - Add Channel.prototype.withStoreScope(data) on both the inactive and active prototypes. The active variant returns a disposable that enters every bound store via AsyncLocalStorage#withScope, publishes the data, and restores the stores on [Symbol.dispose]. - Back TracingChannel with two private BoundedChannel instances and expose start/end/asyncStart/asyncEnd via prototype accessors, with error defined as a non-enumerable own property. Object.keys(tc) now yields [] as it does on Node.js, so spread/for-in/serialisation of a TracingChannel agree across runtimes. - Add TracingChannel.prototype.hasSubscribers delegating to the underlying windows and error channel. traceSync/tracePromise/traceCallback continue to use the existing runStores path via the new accessors, so the existing node parallel tests are unchanged. Imports Node's test-diagnostics-channel-bounded-channel.js unmodified.
WalkthroughChangesDiagnostics channel enhancements
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:05 AM PT - Jul 18th, 2026
❌ @robobun, your commit 7da3742 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34604That installs a local version of the PR into your bun-34604 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
This overlaps with #32628, which already contains the This PR is intentionally the narrow subset covering just those four surface gaps, in case a smaller change is easier to land independently. All existing CI: build 75307 finished 284/286 green. The two red lanes are unrelated to this diff: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…fault for withScope
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/diagnostics_channel.ts`:
- Around line 407-488: Update TracingChannel.traceSync, tracePromise, and
traceCallback to execute their traced work through BoundedChannel.withScope(),
using `#callWindow` for synchronous calls and `#continuationWindow` for promise
continuations and callbacks. Ensure asyncStart/asyncEnd execution is scoped
through the continuation window rather than published directly. Add an
AsyncLocalStorage regression test confirming stores bound to the continuation
window are active during promise continuation tracing.
In `@test/js/node/diagnostics_channel/diagnostics_channel.test.ts`:
- Around line 510-519: Update the test “hasSubscribers reflects any of the five
channels” to exercise each tracingChannel subscriber property: start, end,
asyncStart, asyncEnd, and error. Parameterize or iterate the subscription,
assertion, and unsubscribe steps so every channel independently changes
hasSubscribers to true and back to false, preserving the existing cleanup
behavior.
- Around line 445-475: Extend the diagnostics channel tests around
boundedChannel.run to cover a throwing callback with subscribers: bind or
establish a store value, make the callback throw, assert the same original error
propagates, verify the end event still publishes, and confirm the bound store is
restored afterward. Keep the existing successful run and subscriber-free
withScope tests unchanged.
🪄 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: 1722352d-9a48-497f-a24b-7da383457659
📒 Files selected for processing (3)
src/js/node/diagnostics_channel.tstest/js/node/diagnostics_channel/diagnostics_channel.test.tstest/js/node/test/parallel/test-diagnostics-channel-bounded-channel.js
….hasSubscribers branches
There was a problem hiding this comment.
No bugs found in this pass — the earlier three inline findings were addressed in eeca4be and the two CodeRabbit test-coverage asks in 7da3742. Deferring to a maintainer because this adds new public API surface (BoundedChannel/boundedChannel/withStoreScope) and restructures TracingChannel's internals, and because it overlaps with #32628 / #33449 / #27881 — someone needs to pick which of those lands.
What was reviewed:
RunStoresScopeconstruction/dispose ordering — thetakenflag +stack.move()correctly cleans up on a mid-construction throw, andBoundedChannelScope[Symbol.dispose]guards the no-subscriber path.TracingChannelrefactor —traceSync/tracePromise/traceCallbackstill readstart/end/etc. via the new getters and hit the samerunStores/publishpath as before, so existing behavior is preserved.subscribe/unsubscriberewrite — the{start: asyncStart, end: asyncEnd}remapping into#continuationWindowround-trips correctly;unsubscribestill returnsfalseon any miss.
Extended reasoning...
Overview
This PR closes four Node.js v26 surface gaps in node:diagnostics_channel: it adds the BoundedChannel class and boundedChannel() factory, adds Channel.prototype.withStoreScope / ActiveChannel.prototype.withStoreScope (returning a disposable that enters bound ALS stores and publishes), and restructures TracingChannel so its five event channels are exposed via non-enumerable accessors backed by two private BoundedChannel instances plus a non-enumerable own error property. It also adds a hasSubscribers getter on TracingChannel. ~200 net new lines in src/js/node/diagnostics_channel.ts, 13 new tests in the module's test file, and one Node parallel test imported unmodified.
Security risks
None identified. This is a pure-JS observability/tracing module with no I/O, network, or filesystem surface. The tamper-resistance concern (capturing DisposableStack at load) was addressed in eeca4be.
Level of scrutiny
Medium-high — this is new user-facing API surface in a node: builtin, and the repo's landing-PRs guidance flags API design changes as needing maintainer sign-off. The TracingChannel restructuring is not purely additive: subscribe/unsubscribe are rewritten to route through the new BoundedChannel instances, and the five channel properties change from own data properties to prototype getters. The PR description asserts existing test-diagnostics-channel-* parallel tests still pass, which mitigates regression risk, but I cannot verify CI status from here (build #75307 was still running per the last robobun update).
Other factors
- Duplicate PRs: github-actions flagged overlap with #32628 (a broader Node 26 sync containing all of this), #33449, and #27881 (both add
TracingChannel.hasSubscribers). The author acknowledged this is an intentional narrow subset. A maintainer needs to decide whether to land this independently or fold it into #32628. - Prior review cycle: three findings from an earlier pass (frozen
kEmptyObjectdefault, uncapturedDisposableStackglobal, shared mutablenoopDisposablesingleton) were all fixed in eeca4be. Two CodeRabbit test-coverage requests were addressed in 7da3742. One CodeRabbit finding (routingtracePromisecontinuations throughwithScope) was declined as out-of-scope pre-existing behavior, which is a reasonable scoping call but is another thing a maintainer should ratify. - Test coverage: solid — new tests cover shape, subscribe/unsubscribe,
runon both success and throw paths, ALS store enter/exit viawithStoreScope, and all fivehasSubscribersbranches viatest.each.
1 participant
What does this PR do?
Closes four surface gaps in
node:diagnostics_channelagainst Node.js v26:dc.boundedChannelundefinedfunctiondc.BoundedChannelundefinedfunctionChannel.prototype.withStoreScopeundefinedfunctionObject.keys(tracingChannel("x"))["start","end","asyncStart","asyncEnd","error"][]Repro
Before: exit 1 (all four checks fail). After: exit 0.
How
BoundedChannel/boundedChannel: new class bundlingstart/endChannels withsubscribe,unsubscribe,hasSubscribers,withScope, andrun.start/endare installed as non-enumerable own properties viaObject.defineProperty, matching Node.withStoreScope(data): added to bothChannel.prototype(returns a no-op disposable) andActiveChannel.prototype(returns aRunStoresScopethat enters every boundAsyncLocalStorageviawithScope, publishesdata, and restores the stores on[Symbol.dispose]).TracingChannelshape: the five event channels are now backed by two privateBoundedChannelinstances.start/end/asyncStart/asyncEndare exposed via prototype getters anderroris defined as a non-enumerable own property, soObject.keys/spread/for-inover aTracingChannelreturn nothing, as on Node. AhasSubscribersgetter is added that delegates to the underlying channels.traceSync/tracePromise/traceCallbackcontinue to read the channels via the new accessors and call the existingrunStorespath, so their behaviour is unchanged and all existingtest/js/node/test/parallel/test-diagnostics-channel-*tests continue to pass.Tests
test/js/node/diagnostics_channel/diagnostics_channel.test.ts: addeddescribeblocks coveringwithStoreScope,BoundedChannel, and theTracingChannelshape/accessors (13 new tests, all failing on the released binary).test/js/node/test/parallel/test-diagnostics-channel-bounded-channel.js: imported unmodified from Node v26.3.0.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file