From e07e0a63104352bccab8f6b673af3944bb93dceb Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Wed, 12 Aug 2026 13:36:59 +0200 Subject: [PATCH 1/2] fix(otel-thread-ctx): feature-detect AsyncContextFrame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The writer inferred whether AsyncContextFrame was available from the Node version plus `process.execArgv`, and threw from `enter()` when it concluded it was not. That inference is wrong in both directions, and each way is reachable with a flag Node itself accepts: # Node 22.23.2 — ACF on, execArgv empty: inference says "unavailable" $ NODE_OPTIONS=--experimental-async-context-frame node probe.js {"isACFActive":true,"execArgv":[]} # Node 24.18.0 — ACF off, execArgv empty: inference says "available" $ NODE_OPTIONS=--no-async-context-frame node probe.js {"isACFActive":false,"execArgv":[]} Node 22 and 23 accept --experimental-async-context-frame in NODE_OPTIONS (Node 24 rejects it, and does not need it); Node 24 accepts --no-async-context-frame there (Node 22 has no such flag). Neither reaches execArgv. A worker thread created with an explicit execArgv doesn't inherit the main thread's command line either, and tooling sometimes rewrites process.execArgv outright. The false-negative makes the writer refuse to run in a process where it would have worked. The false-positive is worse and silent: the CPED slot the addon reads is only written when ACF is on, so the writer installs its hook, keeps looking healthy from JS — getStore() still works — and every out-of-process reader sees a record that nothing ever updates. Ask the question directly instead: with ACF, AsyncLocalStorage#run is implemented in terms of #enterWith, and without it, it isn't. The version and execArgv are still used, but only to word the error message. Five test-side copies of the same inference decided whether to exercise the CPED paths, so they mis-skipped in exactly the same processes; they now share the one detection. Their >=22.7.0 floor for time-profiler CPED support is unchanged. --- bindings/otel-thread-ctx.cc | 2 +- ts/src/async-context-frame.ts | 76 ++++++++++++ ts/src/otel-thread-ctx.ts | 31 +++-- ts/test/async-context-frame-child.ts | 26 +++++ ts/test/test-async-context-frame.ts | 121 ++++++++++++++++++++ ts/test/test-get-value-from-map-profiler.ts | 7 +- ts/test/test-otel-thread-ctx.ts | 22 ++-- ts/test/test-time-profiler.ts | 6 +- ts/test/worker.ts | 7 +- ts/test/worker2.ts | 7 +- 10 files changed, 255 insertions(+), 50 deletions(-) create mode 100644 ts/src/async-context-frame.ts create mode 100644 ts/test/async-context-frame-child.ts create mode 100644 ts/test/test-async-context-frame.ts diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index cbe34d51..3d061891 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -756,7 +756,7 @@ void StoreAls(const FunctionCallbackInfo& args) { #else // Node < 22 lacks ContinuationPreservedEmbedderData entirely (and the // associated V8 internal offset). The TS layer refuses to install the - // hook on these versions via asyncContextFrameError, so StoreAls is + // hook on these versions via isAsyncContextFrameActive, so StoreAls is // never called from JS — this null assignment is just here so the // addon compiles on the older Node versions the package supports. otel_thread_ctx_nodejs_v1.cped_slot = nullptr; diff --git a/ts/src/async-context-frame.ts b/ts/src/async-context-frame.ts new file mode 100644 index 00000000..fed0635f --- /dev/null +++ b/ts/src/async-context-frame.ts @@ -0,0 +1,76 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {AsyncLocalStorage} from 'node:async_hooks'; + +let active: boolean | undefined; + +/** + * Whether this process's `AsyncLocalStorage` is backed by AsyncContextFrame, + * which is what puts the active value in the isolate's + * ContinuationPreservedEmbedderData slot that this addon reads. + * + * Feature-detected rather than inferred from the Node version plus + * `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=--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 + * looking healthy from JS — `getStore()` still works — while every + * out-of-process reader sees a record that nothing ever updates. + * - A worker thread created with an explicit `execArgv` doesn't inherit the + * 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. + */ +export function isAsyncContextFrameActive(): boolean { + if (active === undefined) { + const probe = new AsyncLocalStorage(); + let delegated = false; + probe.enterWith = () => { + delegated = true; + }; + probe.run(0, () => {}); + probe.disable(); + active = delegated; + } + return active; +} + +/** + * How to turn AsyncContextFrame on, for the error message of whatever declined + * to run without it. + * + * Advisory text only — never decide availability from this. That is what + * {@link isAsyncContextFrameActive} is for. + */ +export function asyncContextFrameHint(): string { + const version = process.versions.node; + const major = Number(version.split('.')[0]); + if (major < 22) { + return `Node ${version} does not support it at all; Node 24 and later enable it by default`; + } + if (major < 24) { + return `Node ${version} needs --experimental-async-context-frame, on the command line or in NODE_OPTIONS; Node 24 and later enable it by default`; + } + return `Node ${version} enables it by default, so something turned it off — look for --no-async-context-frame on the command line, in NODE_OPTIONS, or in this worker's execArgv`; +} diff --git a/ts/src/otel-thread-ctx.ts b/ts/src/otel-thread-ctx.ts index f59a976b..6ee061ed 100644 --- a/ts/src/otel-thread-ctx.ts +++ b/ts/src/otel-thread-ctx.ts @@ -19,6 +19,11 @@ // as a near-verbatim copy: edits should ideally land upstream first and // be ported here, so the two stay in sync. We plan to drop this vendored // copy once the upstream package is suitable to depend on directly. +// +// Known divergence from upstream: AsyncContextFrame availability is +// feature-detected via ./async-context-frame instead of being inferred from +// `process.execArgv`, which is wrong in both directions — see that module. Keep +// the divergence across re-syncs until upstream does the same. // Node.js writer for the OpenTelemetry Thread Local Context Record // (OTEP-4947), discoverable from an out-of-process reader via the @@ -30,6 +35,11 @@ import {join} from 'path'; import {AsyncLocalStorage} from 'node:async_hooks'; +import { + asyncContextFrameHint, + isAsyncContextFrameActive, +} from './async-context-frame'; + /** * OTEP-4719 process-context attributes corresponding to a particular * key list. Spread this into whatever attribute map the application @@ -171,27 +181,12 @@ if (process.platform === 'linux') { let als: AsyncLocalStorage | undefined; - function asyncContextFrameError(): string | undefined { - const [major] = process.versions.node.split('.').map(Number); - if (process.execArgv.includes('--no-async-context-frame')) { - return 'Node explicitly launched with --no-async-context-frame'; - } - if (major >= 24) return undefined; - if (process.execArgv.includes('--experimental-async-context-frame')) { - return undefined; - } - if (major >= 22) { - return 'Node versions prior to v24 must be launched with --experimental-async-context-frame'; - } - return 'Node major versions prior to v22 do not support the feature at all'; - } - function ensureHook(): AsyncLocalStorage { if (als) return als; - const err = asyncContextFrameError(); - if (err) { + if (!isAsyncContextFrameActive()) { throw new Error( - `otel thread-ctx writer requires async_context_frame support, which is unavailable: ${err}.`, + 'otel thread-ctx writer requires async_context_frame support, which is ' + + `unavailable: ${asyncContextFrameHint()}.`, ); } als = new AsyncLocalStorage(); diff --git a/ts/test/async-context-frame-child.ts b/ts/test/async-context-frame-child.ts new file mode 100644 index 00000000..ededfbe9 --- /dev/null +++ b/ts/test/async-context-frame-child.ts @@ -0,0 +1,26 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Reports how this process sees AsyncContextFrame, for test-async-context-frame +// to compare against how the flags reached it. Also reports execArgv, so a +// failure shows whether the flag was visible there at all. + +import {isAsyncContextFrameActive} from '../src/async-context-frame'; + +process.send?.({ + active: isAsyncContextFrameActive(), + execArgv: process.execArgv, +}); diff --git a/ts/test/test-async-context-frame.ts b/ts/test/test-async-context-frame.ts new file mode 100644 index 00000000..4fe7e3de --- /dev/null +++ b/ts/test/test-async-context-frame.ts @@ -0,0 +1,121 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {strict as assert} from 'assert'; +import {fork} from 'node:child_process'; +import {join} from 'node:path'; + +import {isAsyncContextFrameActive} from '../src/async-context-frame'; + +const CHILD = join(__dirname, 'async-context-frame-child.js'); + +const major = Number(process.versions.node.split('.')[0]); + +interface ChildReport { + active: boolean; + execArgv: string[]; +} + +// Runs the probe in a child process configured the way the test wants, since +// AsyncContextFrame is decided at process start and can't be toggled in-process. +function probeChild( + options: {execArgv?: string[]; nodeOptions?: string} = {}, +): Promise { + return new Promise((resolve, reject) => { + const child = fork(CHILD, [], { + execArgv: options.execArgv ?? [], + env: options.nodeOptions + ? {...process.env, NODE_OPTIONS: options.nodeOptions} + : {...process.env, NODE_OPTIONS: ''}, + stdio: ['ignore', 'ignore', 'pipe', 'ipc'], + }); + let report: ChildReport | undefined; + let stderr = ''; + child.stderr?.on('data', chunk => { + stderr += chunk; + }); + child.on('message', message => { + report = message as ChildReport; + }); + child.on('error', reject); + child.on('exit', code => { + if (report === undefined) { + reject( + new Error( + `child exited with ${code} and no report; stderr: ${stderr}`, + ), + ); + return; + } + resolve(report); + }); + }); +} + +describe('isAsyncContextFrameActive', () => { + it('gives the same answer on every call', () => { + const first = isAsyncContextFrameActive(); + assert.equal(typeof first, 'boolean'); + assert.equal(isAsyncContextFrameActive(), first); + }); + + it('reports it active when Node enables it by default', async function () { + if (major < 24) return this.skip(); + const {active} = await probeChild(); + assert.equal(active, true); + }); + + it('reports it inactive when Node has no support for it', async function () { + if (major >= 22) return this.skip(); + const {active} = await probeChild(); + assert.equal(active, false); + }); + + it('reports it inactive when the command line turns it off', async function () { + // The flag only exists from Node 24, where ACF is the default. + if (major < 24) return this.skip(); + const {active} = await probeChild({ + execArgv: ['--no-async-context-frame'], + }); + assert.equal(active, false); + }); + + it('reports it inactive when NODE_OPTIONS turns it off', async function () { + // The regression this detection exists for: Node 24 accepts the flag in + // NODE_OPTIONS, where it does not reach execArgv, so inferring from execArgv + // concludes ACF is on. It is off, the CPED slot is never written, and a + // caller that trusted the inference would emit records nothing updates. + if (major < 24) return this.skip(); + const {active, execArgv} = await probeChild({ + nodeOptions: '--no-async-context-frame', + }); + assert.deepEqual(execArgv, []); + assert.equal(active, false); + }); + + 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 + // 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(); + const {active, execArgv} = await probeChild({ + nodeOptions: '--experimental-async-context-frame', + }); + assert.deepEqual(execArgv, []); + assert.equal(active, true); + }); +}); diff --git a/ts/test/test-get-value-from-map-profiler.ts b/ts/test/test-get-value-from-map-profiler.ts index 432dac5c..1b926a25 100644 --- a/ts/test/test-get-value-from-map-profiler.ts +++ b/ts/test/test-get-value-from-map-profiler.ts @@ -31,14 +31,13 @@ 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 = - (satisfies(process.versions.node, '>=24.0.0') && - !process.execArgv.includes('--no-async-context-frame')) || - (satisfies(process.versions.node, '>=22.7.0') && - process.execArgv.includes('--experimental-async-context-frame')); + isAsyncContextFrameActive() && satisfies(process.versions.node, '>=22.7.0'); 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 f4d6683b..bfb71118 100644 --- a/ts/test/test-otel-thread-ctx.ts +++ b/ts/test/test-otel-thread-ctx.ts @@ -30,6 +30,7 @@ import {fork, spawnSync} from 'node:child_process'; import {existsSync} from 'node:fs'; import {join} from 'node:path'; +import {isAsyncContextFrameActive} from '../src/async-context-frame'; import { ThreadContext, getContext, @@ -61,21 +62,12 @@ function tcIsTruncated(): boolean { } const isLinux = process.platform === 'linux'; -// AsyncContextFrame (the writer's discovery substrate) is opt-in on Node -// 22/23 (via --experimental-async-context-frame) and on by default in -// Node 24+ (disable-able via --no-async-context-frame). The TS layer -// refuses to install the hook when ACF isn't available, so the entire -// describe block is skipped in that case. Mirrors the source-side -// asyncContextFrameError logic. -const isAsyncContextFrameAvailable = (() => { - if (process.execArgv.includes('--no-async-context-frame')) return false; - const major = Number(process.versions.node.split('.')[0]); - if (major >= 24) return true; - if (major >= 22) { - return process.execArgv.includes('--experimental-async-context-frame'); - } - return false; -})(); +// 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 +// (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. +const isAsyncContextFrameAvailable = isAsyncContextFrameActive(); // Returns a plain Uint8Array (not a Buffer) so assert.deepStrictEqual against // other Uint8Arrays — including the one the addon returns — succeeds. diff --git a/ts/test/test-time-profiler.ts b/ts/test/test-time-profiler.ts index ede45f7a..0e249c7d 100644 --- a/ts/test/test-time-profiler.ts +++ b/ts/test/test-time-profiler.ts @@ -15,6 +15,7 @@ */ import * as sinon from 'sinon'; +import {isAsyncContextFrameActive} from '../src/async-context-frame'; import {time, getNativeThreadId} from '../src'; import {profileV2, stopV2} from '../src/time-profiler'; import * as v8TimeProfiler from '../src/time-profiler-bindings'; @@ -32,10 +33,7 @@ import {fork} from 'child_process'; import assert from 'assert'; const useCPED = - (satisfies(process.versions.node, '>=24.0.0') && - !process.execArgv.includes('--no-async-context-frame')) || - (satisfies(process.versions.node, '>=22.7.0') && - process.execArgv.includes('--experimental-async-context-frame')); + isAsyncContextFrameActive() && satisfies(process.versions.node, '>=22.7.0'); const collectAsyncId = satisfies(process.versions.node, '>=24.0.0'); diff --git a/ts/test/worker.ts b/ts/test/worker.ts index 5b4240af..5485196b 100644 --- a/ts/test/worker.ts +++ b/ts/test/worker.ts @@ -4,6 +4,7 @@ import {time} from '../src/index'; import {Profile, ValueType} from 'pprof-format'; import {getAndVerifyPresence, getAndVerifyString} from './profiles-for-tests'; import {satisfies} from 'semver'; +import {isAsyncContextFrameActive} from '../src/async-context-frame'; import assert from 'assert'; @@ -13,10 +14,8 @@ const withContexts = process.platform === 'darwin' || process.platform === 'linux'; const useCPED = withContexts && - ((satisfies(process.versions.node, '>=24.0.0') && - !process.execArgv.includes('--no-async-context-frame')) || - (satisfies(process.versions.node, '>=22.7.0') && - process.execArgv.includes('--experimental-async-context-frame'))); + isAsyncContextFrameActive() && + satisfies(process.versions.node, '>=22.7.0'); const collectAsyncId = withContexts && satisfies(process.versions.node, '>=24.0.0'); diff --git a/ts/test/worker2.ts b/ts/test/worker2.ts index 2a1e4b13..0eed62c0 100644 --- a/ts/test/worker2.ts +++ b/ts/test/worker2.ts @@ -1,6 +1,7 @@ import {parentPort} from 'node:worker_threads'; import {time} from '../src/index'; import {satisfies} from 'semver'; +import {isAsyncContextFrameActive} from '../src/async-context-frame'; const delay = (ms: number) => new Promise(res => setTimeout(res, ms)); @@ -11,10 +12,8 @@ const withContexts = const useCPED = withContexts && - ((satisfies(process.versions.node, '>=24.0.0') && - !process.execArgv.includes('--no-async-context-frame')) || - (satisfies(process.versions.node, '>=22.7.0') && - process.execArgv.includes('--experimental-async-context-frame'))); + isAsyncContextFrameActive() && + satisfies(process.versions.node, '>=22.7.0'); const collectAsyncId = withContexts && satisfies(process.versions.node, '>=24.0.0'); From 85c5618926d4e6ed4030ba6a5042e4f65d76160b Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Wed, 12 Aug 2026 13:37:00 +0200 Subject: [PATCH 2/2] test(docker): stage the tree without tsconfig.tsbuildinfo The runner deletes the host's node_modules, build and out before building inside the container, but copies in tsconfig.tsbuildinfo, which is gitignored and present on any host where `npm run compile` has been run. tsc then trusts that incremental state, emits nothing for the deleted out/, and the run ends in Error: No test files found: "out/test/test-*.js" having tested nothing at all. --- scripts/docker/run-in-docker.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/docker/run-in-docker.sh b/scripts/docker/run-in-docker.sh index 099549da..7772cd83 100755 --- a/scripts/docker/run-in-docker.sh +++ b/scripts/docker/run-in-docker.sh @@ -31,7 +31,11 @@ exec docker run --rm \ set -euo pipefail cp -R /work/. /tmp/work/ # Drop any host-built artifacts so we get a clean build inside. - rm -rf /tmp/work/node_modules /tmp/work/build /tmp/work/out + # tsconfig.tsbuildinfo has to go with out/: left behind, tsc trusts it, + # emits nothing for the deleted out/, and the run ends in "No test files + # found" having tested nothing. + rm -rf /tmp/work/node_modules /tmp/work/build /tmp/work/out \ + /tmp/work/tsconfig.tsbuildinfo npm install --no-audit --no-fund npm test '