fix(otel-thread-ctx): detect AsyncContextFrame by reading CPED natively - #398
Conversation
#397 replaced the execArgv inference with a feature detection, but the probe was indirect: it overrode `enterWith` on a throwaway AsyncLocalStorage and checked whether `run()` dispatched through it. That `run()` goes through the instance property is unspecified, and anything patching AsyncLocalStorage can break it — including dd-trace-js, which patches async-context machinery. The resulting false negative is the failure #397 set out to fix: `enter()` throwing inside a diagnostic-channel subscriber, in application code. Ask the question directly instead. `cpedMapContains(key, value)` reports whether the isolate's ContinuationPreservedEmbedderData binds a key to a value, so calling it from inside a `run()` with the probe storage and its own store observes the property the addon actually depends on. It is the same slot, and the same "is it a Map" question, that WallProfiler::SetContext asks before storing a context; the key is the one whose identity hash is published as otel_thread_ctx_nodejs_v1.als_identity_hash for the out-of-process reader to look up. Verified empirically that the frame is keyed by the storage instance with the store as value. Checking the key and value rather than just "CPED holds a Map" matters: CPED is a general embedder slot, so a Map another addon left there must not answer for us — that would resurrect the silent false positive, where the writer looks healthy from JS while readers see records nothing updates. Uses the public v8::Map::Get, not the raw OrderedHashMap walk in map-get.hh. Conflating "is ACF on" with "is our layout knowledge correct" would report a V8 layout change as ACF being unavailable; layout has its own coverage. Lives in binding.cc rather than wall.cc so it works on Windows: wall.cc's `#ifndef _WIN32` block is there for SIGPROF and the v8::base::TimeTicks symbol trick, neither of which a CPED read needs. Gated on NODE_MAJOR_VERSION >= 22, returning false below, which is the correct answer there rather than a missing export. Total by construction — no context, slot unset or not a Map, key absent, or a malformed call all yield false, never a throw, because the writer calls this from ensureHook(). Detection routes verified on both Node lines: 24 default-on, 24 off via command line, 24 off via NODE_OPTIONS, 22 off by default, 22 on via command line, 22 on via NODE_OPTIONS. Five new tests pin the key/value discrimination; mutating the helper to a bare IsMap check fails three of them and none of the pre-existing ones. 124 passing on macOS, 175 passing / 2 pending in test:docker.
Overall package sizeSelf size: 2.53 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.3.1 | 504.33 kB | 504.33 kB | | source-map | 0.8.0 | 185.66 kB | 185.66 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Review nits from #397. AsyncContextFrame landed in 22.7.0, not at the 22 boundary, so several places named the wrong version. Drop `&& satisfies(process.versions.node, '>=22.7.0')` from the four useCPED definitions. It is redundant against isAsyncContextFrameActive(): ACF cannot be active below 22.7.0, so the detection already answers false there. This leaves semver unused in test-get-value-from-map-profiler.ts, so the import goes too. Fix the cutoffs that were expressed as a bare major: the skip gates in test-async-context-frame.ts now use a semver check, and the prose in test-otel-thread-ctx.ts and in the async-context-frame doc comment names 22.7.0. asyncContextFrameHint() had the only user-visible instance of the bug: on Node 22.0 through 22.6 it advised passing --experimental-async-context-frame, a flag those versions do not have. Compared by major/minor rather than semver.satisfies because semver is a devDependency and this module ships. Boundary checked across 20.19.0, 22.6.0, 22.7.0, 22.23.2, 23.5.0 and 24.18.0. 124 passing on macOS, 175 passing / 2 pending in test:docker, unchanged.
There was a problem hiding this comment.
Pull request overview
This PR strengthens AsyncContextFrame (ACF) detection by switching from a JavaScript-level behavioral proxy (AsyncLocalStorage#run delegating to enterWith) to a native check that directly inspects V8’s ContinuationPreservedEmbedderData (CPED) for the expected ACF Map binding. It also corrects the version boundary for ACF support (22.7.0) and removes redundant >=22.7.0 gating where the new detection already answers correctly.
Changes:
- Add native addon helper
cpedMapContains(key, value)to validate that CPED holds a Map binding the given key to the given value (used as the new ACF detection substrate). - Update
isAsyncContextFrameActive()to run a probeAsyncLocalStorageand query CPED natively, avoiding reliance on unspecified JS dispatch behavior. - Adjust tests and test gating to reflect the correct ACF introduction point (Node 22.7.0) and remove redundant semver checks where appropriate.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ts/src/async-context-frame.ts | Replaces JS behavioral inference with native CPED-based detection; updates hint logic for Node 22.7.0 boundary. |
| bindings/binding.cc | Exports cpedMapContains to check CPED Map membership/binding in native code. |
| ts/test/test-async-context-frame.ts | Adds targeted tests for cpedMapContains semantics and updates version gating to >=22.7.0. |
| ts/test/test-time-profiler.ts | Removes redundant >=22.7.0 semver gate for CPED usage, relying on detection instead. |
| ts/test/test-get-value-from-map-profiler.ts | Removes unused semver import and redundant >=22.7.0 CPED gate. |
| ts/test/worker.ts | Removes redundant >=22.7.0 semver gate for CPED usage in worker tests. |
| ts/test/worker2.ts | Removes redundant >=22.7.0 semver gate for CPED usage in worker tests. |
| ts/test/test-otel-thread-ctx.ts | Corrects documentation comment to reflect ACF support starting at Node 22.7.0. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Follow-up to #397, implementing @nsavoire's suggestion from this comment: gather the evidence natively by reading CPED, rather than inferring it from
AsyncLocalStorage#run's behaviour.Of the three shapes I floated in r3776016667, this is the third —
cpedMapContains(key, value).#397's detection overrode
enterWithon a throwawayAsyncLocalStorageand checked whetherrun()dispatched through it:That
run()reaches the instance'senterWithproperty is unspecified. Anything patchingAsyncLocalStoragecan break it — dd-trace-js patches async-context machinery itself — and the resulting false negative is precisely the failure #397 existed to fix:ThreadContext#enter()throwing from an inline diagnostic-channel subscriber, surfacing in application code.We now replace this with:
With ACF active, Node installs an AsyncContextFrame — a JS Map keyed by the storage instance, valued by its store — as the running continuation's CPED. Nothing writes the slot otherwise.
Why key+value rather than
cpedIsMapCPED is a general embedder slot. A Map that some other native addon left there must not be able to answer for us — that would readmit the silent false positive #397 killed, where the writer looks healthy from JS while every reader sees records nothing updates.
This is load-bearing, not theoretical: mutating the helper to a bare
IsMapcheck fails three of the five new tests and none of the pre-existing ones. Without them, the weaker version would pass the whole suite.Why not
getCpedReturning the live frame to JS would hand any caller who can require the addon an object whose mutation corrupts async-context propagation process-wide. It also moves the check into JS, where
instanceof Mapis realm-sensitive (the frame is aSafeMapsubclass), so it'd need to beacf?.get?.(probe) === valueto be safe — same answer as this, larger blast radius.Implementation notes
binding.cc, notwall.cc, so it works on Windows.wall.cc's#ifndef _WIN32block is there for SIGPROF and thev8::base::TimeTickssymbol trick; a CPED read needs neither. This allows us to not have a JS fallback anywhere. This is largely theoretical as none of the features using CPED currently work on Windows, but one can hope.NODE_MAJOR_VERSION >= 22, returning false below — the correct answer there, rather than a missing export the JS side has to reason about.Second commit: the 22.7.0 cutoff
The
>=22.7.0nits from the same review. ACF landed in 22.7.0, not at the 22 boundary, and several places named the wrong version.The
&& satisfies(process.versions.node, '>=22.7.0')conjunct comes off all fouruseCPEDdefinitions: it is redundant againstisAsyncContextFrameActive(), since ACF cannot be active below 22.7.0, so the detection already answers false there. That leavessemverunused intest-get-value-from-map-profiler.ts, so its import goes too. The skip gates intest-async-context-frame.tsmove from a bare major to a semver check, and the prose cutoffs are corrected.asyncContextFrameHint()held the only user-visible instance: on 22.0 through 22.6 it advised passing--experimental-async-context-frame, a flag those versions don't have. Compared by major/minor rather thansemver.satisfies, since semver is a devDependency and this module ships. Boundary checked across 20.19.0, 22.6.0, 22.7.0, 22.23.2, 23.5.0 and 24.18.0.