-
Notifications
You must be signed in to change notification settings - Fork 4k
fix(diagnostics_channel): retain channels with active subscribers #26537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Updated 12:40 PM PT - Jan 28th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 26537That installs a local version of the PR into your bun-26537 --bun |
WalkthroughWeakReference in diagnostics_channel.ts is refactored to compose a private WeakRef with an explicit strong-reference cache and reference counting; regression tests added to ensure diagnostics_channel subscribers persist across Bun preload and main app contexts. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/js/node/diagnostics_channel.ts`:
- Around line 23-33: The private field `#strong` is currently unused which
triggers the no-unused-private-class-members lint rule; update the get() method
to use `#strong` by grabbing the dereferenced value from `#weak` (via
this.#weak.deref()), assign it to this.#strong (and optionally update
this.#refCount if you want to track strong refs), then return the value — this
ensures `#strong` is read/written and keeps the existing `#weak`, `#strong`, get(),
and `#refCount` semantics intact.
The WeakReference class in diagnostics_channel was allowing channels to be garbage collected even when they had active subscribers. This caused subscribers registered in preload scripts to be lost when the main application script ran, breaking OpenTelemetry instrumentation for frameworks like Fastify and Express. The fix aligns WeakReference behavior with Node.js by maintaining a strong reference when refCount > 0. When incRef() is called and refCount transitions from 0 to 1, a strong reference is stored. When decRef() returns refCount to 0, the strong reference is cleared, allowing GC. The get() method now returns the strong reference when available, falling back to the weak reference otherwise. Fixes #26536 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
7e5414b to
33b9a33
Compare
Summary
node:diagnostics_channelchannels being garbage collected even when they have active subscribersRoot Cause
The
WeakReferenceclass indiagnostics_channel.tsextendedWeakRefbut only tracked a reference count without actually preventing garbage collection. When channels had active subscribers, they could still be GC'd because there was no strong reference keeping them alive.Fix
Aligned the
WeakReferenceimplementation with Node.js by maintaining a strong reference whenrefCount > 0:incRef()is called and refCount transitions from 0 to 1, store a strong referencedecRef()returns refCount to 0, clear the strong reference to allow GCThis matches the behavior in Node.js's internal util.js.
Test plan
diagnostics_channel.test.tstests passFixes #26536
🤖 Generated with Claude Code