Repository navigation
Conversation
bm1549
added
semver-patch
AI Generated
Contributor
Overall package sizeSelf size: 5.8 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.1 | 82.56 kB | 817.39 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
This comment has been minimized.
This comment has been minimized.
BenchmarksBenchmark execution time: 2026-05-08 13:52:02 Comparing candidate commit c381672 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1735 metrics, 109 unstable metrics. |
10 parallel runners × 25 iterations to confirm the ws plugin fix holds under repeated test:plugins:ci execution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bm1549
force-pushed
the
brian.marks/fix-ws-plugin-flake
branch
from
May 8, 2026 00:06
7f363ba to
5139166
Compare
bm1549
changed the title
bm1549
changed the title
Adds a tiny pre-test step that patches node_modules/dc-polyfill to memoize dc.channel(name) results by name. Locally this makes the ws-stress repro pass on all 3 iterations. Pushing to verify the same fix works in CI. NOT a permanent fix — the proper home for this workaround is dc-polyfill itself; the underlying bug is in Node 18's diagnostics_channel returning different Channel objects across calls for the same name.
bm1549
added a commit
to DataDog/dc-polyfill
that referenced
this pull request
May 8, 2026
) * fix: memoize channel(name) to make Channel identity stable per name The phony AVOID_GARBAGE_COLLECTION subscriber installed by this patch keeps a created Channel alive, but Node's underlying channel(name) can still return a brand-new Channel object for the same name on a subsequent call (observed on Node 18 under certain workloads — e.g. heavy proxyquire reload + plugin teardown/recreate cycles). Because the existing anti-GC tracking is a WeakSet keyed by Channel-object identity, each new object is treated as new and gets its own phony subscriber. The two objects are never unified. The visible failure mode for callers: code that captures a Channel in a module-level closure (a common pattern for instrumentation publishers) ends up publishing to a different Channel than later subscribers attach to. Publishes go nowhere; subscribers see nothing. hasSubscribers reports false (only the phony is on the publish-side Channel) and traceSync/traceCallback fast-path-bypass the publish. This fix memoizes dc.channel(name) by name and short-circuits before delegating to channel() on subsequent calls. Channel identity is now stable per name regardless of what the underlying channel() returns — the contract callers actually depend on. Empirically verified end-to-end against dd-trace-js's ws plugin stress workflow (10 runners × 25 iterations of plugin tests on Node 18.20.8, 250 runs total): 0/10 runners pass without this patch, 10/10 pass with it. dd-trace-js PR for context: DataDog/dd-trace-js#8297. All existing dc-polyfill tests pass (26/26 suites). * test: regression test for memoized channel(name) identity Adds a unit test that injects a mock unpatched module whose channel(name) returns a different Channel object on every call, simulating the Node 18 bug. Without the memoization in patch-garbage-collection-bug.js, this test fails 6/8 assertions (channel identity diverges, subscribers attach to a different Channel than later publishers, the underlying channel() is re-invoked on every memoizable lookup). With the patch, all 8 pass. This pins the contract the patch is meant to enforce — dc.channel(name) returns a stable Channel object per name regardless of what the underlying registry hands back — without requiring a real Node 18 bug to be triggered to exercise the patch. * Apply suggestion from @bm1549
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR contains
Master + the
ws-stressworkflow (10 runners × 25 iterations ofnpm run test:plugins:ciwithPLUGINS=ws). Nothing else.What it reproduces
ws-stressis deterministically red on master: 14 passing / 32 failing per iteration. Every failing test returns a Promise fromagent.assertSomeTraces(...); every passing test uses adone()callback. Spans never reach the mock agent.Locally on macOS arm64 / Node 18.20.8, the bug surfaces from iteration 2 (iteration 1 passes — the wall-clock cost of the fresh nyc transform must keep something pinned). On Linux x64 in CI, iteration 1 already fails.
Where the bug actually is
Node 18's
diagnostics_channel.channel(name)returns differentChannelobjects across calls for the same name. Verified by tracingrequire('diagnostics_channel').channel('ws:send')(bypassing dc-polyfill entirely) inside packages/datadog-instrumentations/src/ws.js — across the test run, the native call returns object ids1 → 2 → 3 → 5 → 6for the same name. That's the underlying defect; everything downstream is fallout.dc-polyfill tries to work around this in patch-garbage-collection-bug.js: on each
channel(name)call it keeps the result alive via a phony subscriber and records the object in aWeakSet. The gap is that theWeakSetcheck is keyed by the returned Channel object — when Node returns a brand-new Channel for the same name,channels.has(ch)is false, dc-polyfill treats it as new, and you end up with multiple Channel instances for one name. The phony-subscriber strategy keeps each of them alive but doesn't unify them.Why iter 1 passes and iter 2 fails
Both iterations see Node hand out multiple Channel objects for the same name. The difference is which one gets the dd-trace plugin's subscription. In iter 1, by chance, the plugin subscribes to the same Channel object that the publish-side closure (in the wrapped
ws.prototype.send) has captured. In iter 2, Node hands out enough different objects that the publish-side and subscribe-side end up on different Channels and the publish goes nowhere as far as the plugin is concerned.assertSomeTraceswaits 5 s and times out.What changes between the two iterations is an indirect side effect of nyc's cache-hit path skipping work that the cache-miss path does — likely some retention pattern that pins the original Channel object. Confirmed by removing any one of
ws.js,helpers/hook.js,helpers/register.js, ordatadog-instrumentations/index.jsfromnode_modules/.cache/nyc/between iterations: iter 2 then passes. The cached files are byte-identical to fresh transforms (diff→ 0 lines), so the cache-load path itself isn't producing wrong code; it's that running through the fresh-transform path leaves the system in a state where Node's broken registry doesn't bite.Fix surfaces, in order of where the bug lives
diagnostics_channel.channel(name)return a stable Channel object per name. Real fix; out of scope here.npm run test:plugins:cipass. It papers over Node's bug rather than fixing it, but dc-polyfill is already the workaround layer.producerCh.startetc. in module-level closures; resolvetracingChannel(name)inside each wrapped method (or behind a memoizing getter). Higher per-call cost; pushes the workaround into every integration.What I tried but couldn't isolate
A minimal standalone reproducer outside dd-trace-js.
dc.channel('foo')× 2 undernyc node, then with multiple module loads viadelete require.cache, then with explicit--expose-gcand a sub/unsub cycle — channels stay identical in all three. Whatever scales the bug into visibility is something specific to dd-trace's test setup (proxyquire-heavy reload, plugin-manager destroy-and-recreate cycles, channel count). For now the stress workflow on this branch is the most compact reliable repro.Reproducer recipe
CI: just push to this branch —
ws-stressjobs will go red. Locally on Node 18.20.8: