From 10cd8c8d09c8ddec080f2c5df14b39412ec7eb82 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 14 Aug 2026 09:13:05 +0200 Subject: [PATCH 1/3] fix(otel-thread-ctx): detect AsyncContextFrame by reading CPED natively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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. --- bindings/binding.cc | 64 +++++++++++++++++++++++++ ts/src/async-context-frame.ts | 50 ++++++++++++++++---- ts/test/test-async-context-frame.ts | 73 +++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 9 deletions(-) diff --git a/bindings/binding.cc b/bindings/binding.cc index 68741dd8..6fdaff42 100644 --- a/bindings/binding.cc +++ b/bindings/binding.cc @@ -29,6 +29,69 @@ #include #endif +// Whether the isolate's ContinuationPreservedEmbedderData currently binds +// `key` to `value`. +// +// This exists for AsyncContextFrame feature detection. With ACF active, Node +// implements AsyncLocalStorage#run by installing an AsyncContextFrame — a JS +// Map keyed by the AsyncLocalStorage instance — as the CPED of the running +// continuation. Calling this from inside a run() with the storage and its +// store therefore observes the property this addon actually depends on, +// instead of inferring it from the Node version, process.execArgv, or whether +// run() happens to dispatch through the instance's enterWith. +// +// It is the same slot, and the same "is it a Map" question, that +// WallProfiler::SetContext asks before storing a context, and the identity +// hash published as otel_thread_ctx_nodejs_v1.als_identity_hash is the hash of +// this very key — so a false answer here means both consumers are broken. +// +// Deliberately total: no context entered, CPED unset or not a Map, or the key +// absent all yield false rather than throwing. The otel thread-ctx writer calls +// this from ensureHook(), which runs inside dd-trace-js diagnostic-channel +// subscribers, where an exception would surface in application code. +// +// Uses the public v8::Map::Get rather than the raw OrderedHashMap walk in +// map-get.hh on purpose: this answers "is ACF on", and conflating it with "is +// our map layout knowledge still correct" would report a V8 layout change as +// ACF being unavailable. Layout is covered separately, by the GetValueFromMap +// tests. +static NAN_METHOD(CpedMapContains) { +#if NODE_MAJOR_VERSION >= 22 + auto isolate = info.GetIsolate(); + + // A malformed call must not accidentally answer true by comparing an absent + // key's undefined against an undefined expected value. + if (info.Length() < 2) { + info.GetReturnValue().Set(false); + return; + } + + auto context = isolate->GetCurrentContext(); + if (context.IsEmpty()) { + info.GetReturnValue().Set(false); + return; + } + + auto cped = isolate->GetContinuationPreservedEmbedderData(); + if (cped.IsEmpty() || !cped->IsMap()) { + info.GetReturnValue().Set(false); + return; + } + + v8::Local found; + if (!cped.As()->Get(context, info[0]).ToLocal(&found)) { + info.GetReturnValue().Set(false); + return; + } + + info.GetReturnValue().Set(found->StrictEquals(info[1])); +#else + // No ContinuationPreservedEmbedderData, and no AsyncContextFrame to put in + // it, so false is the right answer rather than a missing export. + info.GetReturnValue().Set(false); +#endif +} + static NAN_METHOD(GetNativeThreadId) { #ifdef __APPLE__ uint64_t native_id; @@ -56,4 +119,5 @@ NODE_MODULE_INIT(/* exports, module, context */) { dd::WallProfiler::Init(exports); dd::OtelThreadCtx::Init(exports); Nan::SetMethod(exports, "getNativeThreadId", GetNativeThreadId); + Nan::SetMethod(exports, "cpedMapContains", CpedMapContains); } diff --git a/ts/src/async-context-frame.ts b/ts/src/async-context-frame.ts index fed0635f..389a4eb1 100644 --- a/ts/src/async-context-frame.ts +++ b/ts/src/async-context-frame.ts @@ -15,6 +15,23 @@ */ import {AsyncLocalStorage} from 'node:async_hooks'; +import {join} from 'path'; + +interface Addon { + cpedMapContains(key: unknown, value: unknown): boolean; +} + +let addon: Addon | undefined; + +// Required lazily so importing this module doesn't force the addon to load; +// memoized by isAsyncContextFrameActive, so this runs at most once per thread. +function bindings(): Addon { + if (!addon) { + const findBinding = require('node-gyp-build'); + addon = findBinding(join(__dirname, '..', '..')) as Addon; + } + return addon; +} let active: boolean | undefined; @@ -39,19 +56,34 @@ let active: boolean | undefined; * main thread's command line either, and tooling sometimes rewrites * `process.execArgv` outright. * - * With ACF, `run()` is implemented in terms of `enterWith()`; without it, it - * isn't. Memoized: the answer is fixed for the life of the thread. + * Detected by asking the addon what is in the CPED slot during a `run()`. With + * ACF, Node installs an AsyncContextFrame — a JS Map keyed by the + * `AsyncLocalStorage` instance, valued by its store — as the running + * continuation's CPED; without it, nothing writes the slot. So a probe storage + * whose own store is visible there is direct evidence, and it is evidence about + * the exact slot both consumers read: `WallProfiler::SetContext` requires that + * Map, and the thread-ctx reader looks this very key up by the identity hash + * published as `als_identity_hash`. + * + * Observing whether `run()` delegates to `enterWith()` would be an indirect + * proxy for the same thing: it holds today, but it depends on `run()` + * dispatching through the instance property, which is unspecified and which + * anything patching `AsyncLocalStorage` can break — and the failure would be + * silent and in the dangerous direction. + * + * Memoized: the answer is fixed for the life of the thread. */ export function isAsyncContextFrameActive(): boolean { if (active === undefined) { - const probe = new AsyncLocalStorage(); - let delegated = false; - probe.enterWith = () => { - delegated = true; - }; - probe.run(0, () => {}); + const probe = new AsyncLocalStorage(); + // Object identity, so a stray equal-valued binding can't answer for us. + const sentinel = {}; + let bound = false; + probe.run(sentinel, () => { + bound = bindings().cpedMapContains(probe, sentinel); + }); probe.disable(); - active = delegated; + active = bound; } return active; } diff --git a/ts/test/test-async-context-frame.ts b/ts/test/test-async-context-frame.ts index 4fe7e3de..6c8500bc 100644 --- a/ts/test/test-async-context-frame.ts +++ b/ts/test/test-async-context-frame.ts @@ -15,11 +15,16 @@ */ import {strict as assert} from 'assert'; +import {AsyncLocalStorage} from 'node:async_hooks'; import {fork} from 'node:child_process'; import {join} from 'node:path'; import {isAsyncContextFrameActive} from '../src/async-context-frame'; +const addon = require('node-gyp-build')(join(__dirname, '..', '..')) as { + cpedMapContains(key?: unknown, value?: unknown): boolean; +}; + const CHILD = join(__dirname, 'async-context-frame-child.js'); const major = Number(process.versions.node.split('.')[0]); @@ -119,3 +124,71 @@ describe('isAsyncContextFrameActive', () => { assert.equal(active, true); }); }); + +// The detection asks whether the running storage is bound to its own store, +// not merely whether the CPED slot holds a Map. These pin that difference: +// without them, weakening the helper to a bare IsMap check would still pass +// every test above. +describe('cpedMapContains', () => { + beforeEach(function () { + // With ACF off nothing writes the slot, so every answer here is false for + // an uninteresting reason. The routes that discriminate on/off are covered + // by the child-process cases above. + if (!isAsyncContextFrameActive()) this.skip(); + }); + + it('finds the running storage bound to its store', () => { + const als = new AsyncLocalStorage(); + const store = {}; + let found = false; + als.run(store, () => { + found = addon.cpedMapContains(als, store); + }); + als.disable(); + assert.equal(found, true); + }); + + it('does not match a foreign key', () => { + // CPED is a general embedder slot. Another native addon storing a Map there + // must not be able to answer for us, which is the false positive an IsMap + // check would admit. + const als = new AsyncLocalStorage(); + const store = {}; + let found = true; + als.run(store, () => { + found = addon.cpedMapContains(new AsyncLocalStorage(), store); + }); + als.disable(); + assert.equal(found, false); + }); + + it('does not match a different value for the right key', () => { + const als = new AsyncLocalStorage(); + let found = true; + als.run({}, () => { + found = addon.cpedMapContains(als, {}); + }); + als.disable(); + assert.equal(found, false); + }); + + it('is false outside any run', () => { + const als = new AsyncLocalStorage(); + const store = {}; + als.run(store, () => {}); + als.disable(); + assert.equal(addon.cpedMapContains(als, store), false); + }); + + it('is false when called without a key and value', () => { + // An absent key reads as undefined; so would a missing expected value, so + // a malformed call must not compare the two and report success. + const als = new AsyncLocalStorage(); + let found = true; + als.run({}, () => { + found = addon.cpedMapContains(); + }); + als.disable(); + assert.equal(found, false); + }); +}); From bbbe8a473f0779e643a4c276a0777e5501831e7c Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 14 Aug 2026 12:47:31 +0200 Subject: [PATCH 2/3] Rewrote it to be more compact --- bindings/binding.cc | 65 +++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 47 deletions(-) diff --git a/bindings/binding.cc b/bindings/binding.cc index 6fdaff42..ea46f95e 100644 --- a/bindings/binding.cc +++ b/bindings/binding.cc @@ -29,8 +29,8 @@ #include #endif -// Whether the isolate's ContinuationPreservedEmbedderData currently binds -// `key` to `value`. +// Whether the isolate's ContinuationPreservedEmbedderData is a JS Map that +// currently binds `key` to `value`. // // This exists for AsyncContextFrame feature detection. With ACF active, Node // implements AsyncLocalStorage#run by installing an AsyncContextFrame — a JS @@ -39,57 +39,28 @@ // store therefore observes the property this addon actually depends on, // instead of inferring it from the Node version, process.execArgv, or whether // run() happens to dispatch through the instance's enterWith. -// -// It is the same slot, and the same "is it a Map" question, that -// WallProfiler::SetContext asks before storing a context, and the identity -// hash published as otel_thread_ctx_nodejs_v1.als_identity_hash is the hash of -// this very key — so a false answer here means both consumers are broken. -// -// Deliberately total: no context entered, CPED unset or not a Map, or the key -// absent all yield false rather than throwing. The otel thread-ctx writer calls -// this from ensureHook(), which runs inside dd-trace-js diagnostic-channel -// subscribers, where an exception would surface in application code. -// -// Uses the public v8::Map::Get rather than the raw OrderedHashMap walk in -// map-get.hh on purpose: this answers "is ACF on", and conflating it with "is -// our map layout knowledge still correct" would report a V8 layout change as -// ACF being unavailable. Layout is covered separately, by the GetValueFromMap -// tests. static NAN_METHOD(CpedMapContains) { #if NODE_MAJOR_VERSION >= 22 - auto isolate = info.GetIsolate(); - // A malformed call must not accidentally answer true by comparing an absent // key's undefined against an undefined expected value. - if (info.Length() < 2) { - info.GetReturnValue().Set(false); - return; + if (info.Length() >= 2) { + auto isolate = info.GetIsolate(); + auto cped = isolate->GetContinuationPreservedEmbedderData(); + if (!cped.IsEmpty() && cped->IsMap()) { + auto context = isolate->GetCurrentContext(); + if (!context.IsEmpty()) { + v8::Local found; + if (cped.As()->Get(context, info[0]).ToLocal(&found)) { + info.GetReturnValue().Set(found->StrictEquals(info[1])); + return; + } + } + } } - - auto context = isolate->GetCurrentContext(); - if (context.IsEmpty()) { - info.GetReturnValue().Set(false); - return; - } - - auto cped = isolate->GetContinuationPreservedEmbedderData(); - if (cped.IsEmpty() || !cped->IsMap()) { - info.GetReturnValue().Set(false); - return; - } - - v8::Local found; - if (!cped.As()->Get(context, info[0]).ToLocal(&found)) { - info.GetReturnValue().Set(false); - return; - } - - info.GetReturnValue().Set(found->StrictEquals(info[1])); -#else - // No ContinuationPreservedEmbedderData, and no AsyncContextFrame to put in - // it, so false is the right answer rather than a missing export. - info.GetReturnValue().Set(false); #endif + // Either code above didn't reach the innermost if statement, or + // we're compiling for Node.js < 22. + info.GetReturnValue().Set(false); } static NAN_METHOD(GetNativeThreadId) { From 1a3a95477820bb6961dbf67b69d26546baa2edea Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 14 Aug 2026 12:58:49 +0200 Subject: [PATCH 3/3] test: use the real 22.7.0 AsyncContextFrame cutoff 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. --- ts/src/async-context-frame.ts | 13 ++++++++----- ts/test/test-async-context-frame.ts | 10 +++++++--- ts/test/test-get-value-from-map-profiler.ts | 4 +--- ts/test/test-otel-thread-ctx.ts | 5 +++-- ts/test/test-time-profiler.ts | 3 +-- ts/test/worker.ts | 5 +---- ts/test/worker2.ts | 5 +---- 7 files changed, 22 insertions(+), 23 deletions(-) diff --git a/ts/src/async-context-frame.ts b/ts/src/async-context-frame.ts index 389a4eb1..a9d6041b 100644 --- a/ts/src/async-context-frame.ts +++ b/ts/src/async-context-frame.ts @@ -44,9 +44,10 @@ let active: boolean | undefined; * `process.execArgv`, because the two disagree in both directions and each * combination is reachable today: * - * - `NODE_OPTIONS=--experimental-async-context-frame` is accepted on Node 22 - * and 23 and turns ACF on without appearing in `execArgv`. Inferring "off" - * there makes callers refuse to run in a process that would have worked. + * - `NODE_OPTIONS=--experimental-async-context-frame` is accepted from Node + * 22.7.0 through 23 and turns ACF on without appearing in `execArgv`. + * Inferring "off" there makes callers refuse to run in a process that would + * have worked. * - `NODE_OPTIONS=--no-async-context-frame` is accepted on Node 24 and turns * ACF off without appearing in `execArgv`. Inferring "on" there is the worse * error: the CPED slot is never written, so a writer that starts anyway keeps @@ -97,8 +98,10 @@ export function isAsyncContextFrameActive(): boolean { */ export function asyncContextFrameHint(): string { const version = process.versions.node; - const major = Number(version.split('.')[0]); - if (major < 22) { + const [major, minor] = version.split('.').map(Number); + // Hand-rolled rather than semver.satisfies: semver is a devDependency, and + // this module ships. + if (major < 22 || (major === 22 && minor < 7)) { return `Node ${version} does not support it at all; Node 24 and later enable it by default`; } if (major < 24) { diff --git a/ts/test/test-async-context-frame.ts b/ts/test/test-async-context-frame.ts index 6c8500bc..54a0e970 100644 --- a/ts/test/test-async-context-frame.ts +++ b/ts/test/test-async-context-frame.ts @@ -19,6 +19,8 @@ import {AsyncLocalStorage} from 'node:async_hooks'; import {fork} from 'node:child_process'; import {join} from 'node:path'; +import {satisfies} from 'semver'; + import {isAsyncContextFrameActive} from '../src/async-context-frame'; const addon = require('node-gyp-build')(join(__dirname, '..', '..')) as { @@ -28,6 +30,8 @@ const addon = require('node-gyp-build')(join(__dirname, '..', '..')) as { const CHILD = join(__dirname, 'async-context-frame-child.js'); const major = Number(process.versions.node.split('.')[0]); +// ACF landed in 22.7.0, so the opt-in routes are gated on that, not on major 22. +const hasAcfSupport = satisfies(process.versions.node, '>=22.7.0'); interface ChildReport { active: boolean; @@ -84,7 +88,7 @@ describe('isAsyncContextFrameActive', () => { }); it('reports it inactive when Node has no support for it', async function () { - if (major >= 22) return this.skip(); + if (hasAcfSupport) return this.skip(); const {active} = await probeChild(); assert.equal(active, false); }); @@ -112,11 +116,11 @@ describe('isAsyncContextFrameActive', () => { }); it('reports it active when NODE_OPTIONS turns it on', async function () { - // The mirror image, on the other Node line: 22 and 23 accept the flag in + // The mirror image, on the other Node line: 22.7.0 through 23 accept the flag in // NODE_OPTIONS (24 rejects it outright), again without it reaching execArgv, // so inferring from execArgv concludes ACF is off when it is on — and the // caller refuses to run in a process that would have worked. - if (major < 22 || major >= 24) return this.skip(); + if (!hasAcfSupport || major >= 24) return this.skip(); const {active, execArgv} = await probeChild({ nodeOptions: '--experimental-async-context-frame', }); diff --git a/ts/test/test-get-value-from-map-profiler.ts b/ts/test/test-get-value-from-map-profiler.ts index 1b926a25..6be2dc48 100644 --- a/ts/test/test-get-value-from-map-profiler.ts +++ b/ts/test/test-get-value-from-map-profiler.ts @@ -29,15 +29,13 @@ import assert from 'assert'; import {join} from 'path'; import {AsyncLocalStorage} from 'async_hooks'; -import {satisfies} from 'semver'; import {isAsyncContextFrameActive} from '../src/async-context-frame'; const findBinding = require('node-gyp-build'); const profiler = findBinding(join(__dirname, '..', '..')); -const useCPED = - isAsyncContextFrameActive() && satisfies(process.versions.node, '>=22.7.0'); +const useCPED = isAsyncContextFrameActive(); const supportedPlatform = process.platform === 'darwin' || process.platform === 'linux'; diff --git a/ts/test/test-otel-thread-ctx.ts b/ts/test/test-otel-thread-ctx.ts index bfb71118..e28a6f03 100644 --- a/ts/test/test-otel-thread-ctx.ts +++ b/ts/test/test-otel-thread-ctx.ts @@ -62,8 +62,9 @@ function tcIsTruncated(): boolean { } const isLinux = process.platform === 'linux'; -// AsyncContextFrame is the writer's discovery substrate: opt-in on Node 22/23 -// (via --experimental-async-context-frame) and on by default from Node 24 +// AsyncContextFrame is the writer's discovery substrate: opt-in from Node +// 22.7.0 through 23 (via --experimental-async-context-frame) and on by +// default from Node 24 // (disable-able via --no-async-context-frame). The TS layer refuses to install // the hook when it isn't active, so the entire describe block is skipped then. // Asks the same question the source side asks, the same way. diff --git a/ts/test/test-time-profiler.ts b/ts/test/test-time-profiler.ts index 0e249c7d..1738ee94 100644 --- a/ts/test/test-time-profiler.ts +++ b/ts/test/test-time-profiler.ts @@ -32,8 +32,7 @@ import {fork} from 'child_process'; import assert from 'assert'; -const useCPED = - isAsyncContextFrameActive() && satisfies(process.versions.node, '>=22.7.0'); +const useCPED = isAsyncContextFrameActive(); const collectAsyncId = satisfies(process.versions.node, '>=24.0.0'); diff --git a/ts/test/worker.ts b/ts/test/worker.ts index 5485196b..1da10334 100644 --- a/ts/test/worker.ts +++ b/ts/test/worker.ts @@ -12,10 +12,7 @@ const DURATION_MILLIS = 1000; const intervalMicros = 10000; const withContexts = process.platform === 'darwin' || process.platform === 'linux'; -const useCPED = - withContexts && - isAsyncContextFrameActive() && - satisfies(process.versions.node, '>=22.7.0'); +const useCPED = withContexts && isAsyncContextFrameActive(); const collectAsyncId = withContexts && satisfies(process.versions.node, '>=24.0.0'); diff --git a/ts/test/worker2.ts b/ts/test/worker2.ts index 0eed62c0..041a4461 100644 --- a/ts/test/worker2.ts +++ b/ts/test/worker2.ts @@ -10,10 +10,7 @@ const INTERVAL_MICROS = 10000; const withContexts = process.platform === 'darwin' || process.platform === 'linux'; -const useCPED = - withContexts && - isAsyncContextFrameActive() && - satisfies(process.versions.node, '>=22.7.0'); +const useCPED = withContexts && isAsyncContextFrameActive(); const collectAsyncId = withContexts && satisfies(process.versions.node, '>=24.0.0');