diagnostics_channel: snapshot the subscriber list per publish

[robobun]

Repro

A subscriber that unsubscribes itself (the one-shot / trace-once idiom) silently prevents a later subscriber from seeing the publish it is in the middle of:

const dc = require("node:diagnostics_channel");
const ch = dc.channel("app");
const got = [];

const f1 = () => { got.push("f1"); ch.unsubscribe(f1); };
const f2 = () => got.push("f2");
const f3 = () => got.push("f3");

ch.subscribe(f1);
ch.subscribe(f2);
ch.subscribe(f3);
ch.publish({ x: 1 });

console.log(got.join(","));
// node: f1,f2,f3
// bun:  f1,f3     <- f2 never sees the publish

Other shapes of the same bug, all confirmed against node v26.3.0:

case	node	bun 1.4.0
subscriber unsubscribes itself	f1,f2,f3	f1,f3
subscriber unsubscribes a later subscriber	f1,f2	f1
subscriber unsubscribes everyone	f1,f2	f1
subscriber subscribes during publish	f1	f1,late
same function subscribed twice, unsubscribes itself	f1,f2,f1	f1,f1
channel emptied mid-publish, re-subscribed by a later subscriber	f1,f2, then f3	f1, then nothing (channel left with no subscribers)
unsubscribe during runStores	f1,f2,fn	f1,fn

Cause

ActiveChannel.publish() re-read this._subscribers on every iteration, while ActiveChannel.unsubscribe() spliced that same array in place:

publish(data) {
  for (let i = 0; i < (this._subscribers?.length || 0); i++) {
    const onMessage = this._subscribers[i];
    onMessage(data, this.name);
  }
}

So when a subscriber unsubscribed during dispatch the live array shifted left under the loop index and the next subscriber was skipped, and when it subscribed during dispatch the new function was appended to the in-flight publish. Unsubscribing the last subscriber also set _subscribers to undefined via maybeMarkInactive, which ended the publish early.

The same function subscribed twice is the worst shape: removing the first registration shifts the second one onto the current index, so the one-shot subscriber runs a second time while the subscriber between them is skipped.

Fix

Match node (nodejs/node#55116): make subscribe()/unsubscribe() copy on write, and have publish() capture the array once. A publish then dispatches to exactly the subscribers registered when it started, and the mutation takes effect from the next publish on.

Copy-on-write rather than copying inside publish() keeps the hot path allocation-free: publish() is what runs per request (http2.ts, _http_client.ts, dgram.ts only ever publish), while subscribe/unsubscribe are setup-time. publish() is also marginally cheaper now, since the property load is hoisted out of the loop.

Verification

test/js/node/diagnostics_channel/diagnostics_channel.test.ts gains 8 tests covering every row of the table above, each asserting the exact dispatch order plus that the mutation did take effect on the following publish. does not throw when unsubscribed is also brought in line with the upstream test, which node extended in the same commit to assert the second subscriber still runs.

All 9 differential cases now match node v26.3.0 byte for byte, and node's test-diagnostics-channel-* ports under test/js/node/test/parallel/ still pass. (test-diagnostics-channel-http2-server-stream-close.js fails identically before and after this change, for an unrelated http2 stream.destroyed lifecycle reason.)

[robobun]

^{Updated 8:21 PM PT - Jul 2nd, 2026}

✅ @robobun, your commit 220a66733b33442c0e20f9878980a684c702f94b passed in Build #68135! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 33288

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

bun-33288 --bun

[github-actions]

This PR may be a duplicate of:

1. node:diagnostics_channel: sync with Node 26 + subsystem channels #32628 - Also implements copy-on-write subscriber lists in node:diagnostics_channel to fix subscribers being skipped during publish() mutation, as part of a broader Node 26 sync

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fb144363-6273-49f5-96a0-dccf479e5f1d

📥 Commits

Reviewing files that changed from the base of the PR and between 85f82af and 4a61705.

📒 Files selected for processing (1)

• test/js/node/diagnostics_channel/diagnostics_channel.test.ts

---

Walkthrough

ActiveChannel now uses copy-on-write subscriber updates and snapshot iteration during publish(). The test suite adds scenarios covering subscriber list mutations during in-flight publish and runStores() dispatch.

Changes

Diagnostics Channel Subscriber Mutation Safety

Layer / File(s)	Summary
Copy-on-write unsubscribe and publish snapshotting src/js/node/diagnostics_channel.ts	Adds an ArrayPrototypeSlice alias; unsubscribe() now copies _subscribers before removing an entry and reassigns the array, and publish() iterates over a local snapshot.
Mutation-during-publish test coverage test/js/node/diagnostics_channel/diagnostics_channel.test.ts	Updates one existing test to pass an inline mustCall subscriber, and adds a mutating subscribers during publish suite covering mid-dispatch unsubscribe, late subscribe, duplicate registrations, and runStores() snapshot behavior.

Related Issues: None specified.
Related PRs: None specified.
Suggested labels: node.js, diagnostics_channel, tests
Suggested reviewers: None specified.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: snapshotting the subscriber list during publish.
Description check	✅ Passed	The description covers the PR purpose and verification steps in detail, even though it uses Repro/Cause/Fix/Verification instead of the template headings.
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.

---

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

[robobun]

Confirmed, the overlap is real: #32628 does carry the same copy-on-write change to subscribe/unsubscribe/publish, as one piece of its Node 26 sync.

Two things make me think this is still worth landing separately, though I'm happy either way.

The tests don't collide. #32628 doesn't touch test/js/node/diagnostics_channel/diagnostics_channel.test.ts. Its only coverage for this bug class is node's upstream one-liner in test-diagnostics-channel-sync-unsubscribe.js (a second mustCall() subscriber), which this PR adds too. The other eight cases here are additive to either branch, and they cover shapes that one-liner doesn't catch:

• the same function subscribed twice: removing the first registration shifts the second onto the current loop index, so the one-shot subscriber fires a second time and the subscriber between them is skipped (f1,f1 instead of f1,f2,f1)
• a channel emptied mid-publish is left with zero subscribers permanently, because the subscriber that would have re-subscribed never ran
• subscribe() from inside a subscriber appends to the in-flight publish
• all of the above again through runStores()

Scope. #32628 is a draft at 58 files, +3305/-185, last commit Jun 23. This is 2 files, and the behavior fix is independent of the rest of that sync.

So: happy to land this and let #32628 drop the hunk when it rebases (it would already be applied), or to close this and move the test block over to #32628 if @cirospaciari would rather keep it together. Either works, your call.

[claude]

LGTM — clean copy-on-write fix matching Node's approach, with thorough test coverage.

Extended reasoning...

Overview

This PR fixes node:diagnostics_channel so that mutating the subscriber list during a publish() (subscribing or unsubscribing from within a subscriber callback) does not skip or duplicate deliveries in the in-flight dispatch. The source change is ~15 lines in src/js/node/diagnostics_channel.ts: subscribe() and unsubscribe() now copy-on-write the _subscribers array via a captured ArrayPrototypeSlice, and publish() captures this._subscribers into a local before iterating. The test file gains 8 new tests covering self-unsubscribe, unsubscribing later/all subscribers, subscribing mid-publish, duplicate registrations, channel emptied then re-subscribed mid-publish, and runStores().

Security risks

None. This is pure JS iteration-order semantics for an observability API — no I/O, auth, crypto, parsing of untrusted input, or memory management is touched.

Level of scrutiny

Low-to-moderate. The change is small, mechanical, and directly mirrors the upstream Node.js fix (nodejs/node#55116). It follows the file's existing primordial-safe conventions (captured Array.prototype methods invoked via .$call). I traced the trickiest case (mutate6: channel emptied via maybeMarkInactive swapping the prototype to Channel, then re-subscribed by a still-in-flight subscriber) and it works correctly because the loop iterates the captured local array, and the later subscribe re-activates the channel via Channel.prototype.subscribe → markActive. The retained subscribers?.length guard is now technically unnecessary (the local is always an array on ActiveChannel) but harmless.

Other factors

The bug-hunting system found no issues. There is no CODEOWNERS entry for this path. The PR description is exceptionally detailed with a per-case behavior table verified against Node v26.3.0, and each row maps to a dedicated test asserting exact dispatch order plus post-publish state. A bot flagged possible overlap with PR #32628 (a broader Node 26 sync) — that's a merge-coordination concern for maintainers, not a correctness issue with this change.

[robobun]

Thanks. No changes needed, but one correction on the note about the subscribers?.length guard being unnecessary, since that's the kind of thing that looks safe to delete later.

It's still reachable. publish can be detached from a channel while it's active and then called after the channel has gone inactive, at which point _subscribers is undefined:

const ch = dc.channel("guard");
const f = () => {};
ch.subscribe(f);

const pub = ch.publish; // ActiveChannel.prototype.publish
ch.unsubscribe(f);      // maybeMarkInactive -> _subscribers = undefined

pub.call(ch, "data");   // without the `?.` this is undefined.length

node v26.3.0 and this branch both run that without throwing, and node keeps the same guard upstream for the same reason, so I've left it as is.

[claude]

LGTM — small, well-tested copy-on-write fix that mirrors upstream Node (nodejs/node#55116).

Extended reasoning...

Overview

This PR fixes node:diagnostics_channel so that mutating the subscriber list from inside a subscriber (the common one-shot/trace-once idiom) does not skip or double-invoke other subscribers during the same publish(). The implementation change is ~15 lines in src/js/node/diagnostics_channel.ts: subscribe()/unsubscribe() now copy-on-write the _subscribers array, and publish() captures the array reference once before iterating. This is a direct port of the upstream fix in nodejs/node#55116. The test file gains 8 targeted cases (self-unsubscribe, unsubscribe-later, unsubscribe-all, subscribe-during-publish, duplicate registration, empty-then-resubscribe, and runStores) plus one existing test is extended to match Node's updated upstream test.

Security risks

None. This is pure JS in a Node-compat module with no auth, crypto, filesystem, or network surface. The change uses captured primordials (ArrayPrototypeSlice, ``, $arrayPush) consistent with the rest of the file, so no new tampering surface is introduced.

Level of scrutiny

Low-to-moderate. The fix is mechanical, matches the referenced upstream Node commit exactly in approach, and the affected code path is self-contained (only ActiveChannel.subscribe/unsubscribe/publish). I traced the trickiest new test (mutate6, where the channel is emptied mid-publish via maybeMarkInactive and then re-subscribed by a later snapshot subscriber through the Channel → markActive path) and the control flow is correct. The retained subscribers?.length guard was already justified in the thread. No CODEOWNERS cover this path.

Other factors

The bug-hunting system found nothing. There is an open sequencing note in the thread about overlap with draft PR #32628 (a larger Node 26 sync), but the author has laid out both options and the code here is correct and independently landable either way — that's a maintainer merge-order decision, not a correctness concern. Test coverage is thorough and asserts exact dispatch order plus post-publish state, so regressions would be caught.