fix(otel-thread-ctx): feature-detect AsyncContextFrame by szegedi · Pull Request #397 · DataDog/pprof-nodejs

szegedi · 2026-08-12T11:44:56Z

What does this PR do?:

Replaces the process.execArgv-based inference of AsyncContextFrame availability with a direct feature detection, in a new ts/src/async-context-frame.ts shared by the writer and the tests.

Motivation:

The 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 — and because ThreadContext#enter() is called from inline diagnostic-channel subscribers in dd-trace-js, that surfaces as an exception in application code on the first span activation (found while reviewing DataDog/dd-trace-js#9210).

The false-positive is worse and silent: StoreAls records the CPED slot, which 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.

Asking the question directly avoids both: with ACF, AsyncLocalStorage#run is implemented in terms of #enterWith, and without it, it isn't. The Node version and execArgv are still used, but only to word the error message.

Additional Notes:

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.
ts/src/otel-thread-ctx.ts is a near-verbatim vendored copy of the upstream polarsignals writer, so the header now records this as a deliberate divergence to preserve across re-syncs.
Second commit is unrelated to the fix but is how it got verified: scripts/docker/run-in-docker.sh staged tsconfig.tsbuildinfo along with the tree while deleting out/, so tsc trusted the incremental state, emitted nothing, and the run ended in Error: No test files found having tested nothing. That happens on any host where npm run compile has been run.

How to test the change?:

ts/test/test-async-context-frame.ts pins the discrimination using forked children, one case per route (default-on, unsupported version, command line off, NODE_OPTIONS off, NODE_OPTIONS on). Each case is version-gated to the Node line where its flag is accepted, so the CI matrix covers both halves.

Mutation-checked by restoring the old inference: it fails NODE_OPTIONS turns it off on Node 24 and NODE_OPTIONS turns it on on Node 22 — one Node line per bug.

Full suites run green: 119 passing on macOS, and 170 passing / 2 pending in npm run test:docker, which is the run that actually exercises the Linux-gated otel-thread-ctx suite against the real addon.

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.

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.

github-actions · 2026-08-12T11:45:51Z

Overall package size

Self size: 2.51 MB
Deduped: 3.22 MB
No deduping: 3.22 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}

datadog-official · 2026-08-12T11:53:23Z

Tests

🔄 Datadog auto-retried 1 job - 1 passed on retry

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 85c5618 | Docs | Datadog PR Page | Give us feedback!}

nsavoire · 2026-08-12T12:54:46Z

@codex review

chatgpt-codex-connector · 2026-08-12T12:57:59Z

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 85c5618926

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

nsavoire · 2026-08-13T09:59:17Z

+export function asyncContextFrameHint(): string {
+  const version = process.versions.node;
+  const major = Number(version.split('.')[0]);
+  if (major < 22) {


nit: cut off is < 22.7.0

nsavoire · 2026-08-13T10:12:54Z

+  });
+
+  it('reports it inactive when Node has no support for it', async function () {
+    if (major >= 22) return this.skip();


nit: cut off is >= 22.7.0

nsavoire · 2026-08-13T10:13:05Z

+    // 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();


nit: cut off is < 22.7.0

nsavoire · 2026-08-13T10:16:04Z

+ * `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


cut off is Node 22.7.0

nsavoire · 2026-08-13T10:16:57Z

+  });
+
+  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


22 -> 22.7.0

nsavoire · 2026-08-13T10:20:03Z

-    !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');


Suggested change

isAsyncContextFrameActive() && satisfies(process.versions.node, '>=22.7.0');

isAsyncContextFrameActive();

nsavoire · 2026-08-13T10:20:27Z

-    !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');


Suggested change

isAsyncContextFrameActive() && satisfies(process.versions.node, '>=22.7.0');

isAsyncContextFrameActive();

nsavoire · 2026-08-13T10:20:48Z

+  isAsyncContextFrameActive() &&
+  satisfies(process.versions.node, '>=22.7.0');


Suggested change

isAsyncContextFrameActive() &&

satisfies(process.versions.node, '>=22.7.0');

isAsyncContextFrameActive();

nsavoire · 2026-08-13T10:21:05Z

+  isAsyncContextFrameActive() &&
+  satisfies(process.versions.node, '>=22.7.0');


Suggested change

isAsyncContextFrameActive() &&

satisfies(process.versions.node, '>=22.7.0');

isAsyncContextFrameActive();

nsavoire · 2026-08-13T11:27:46Z

+ */
+export function isAsyncContextFrameActive(): boolean {
+  if (active === undefined) {
+    const probe = new AsyncLocalStorage<number>();


nit: why not retrieve CPED natively from addon and then check it from JS instead of relying on run() calling enterWith() only when ACF is enabled ? This would directly check if store landed in CPED, rather than a proxy for it.

I just lifted the approach used in dd-trace-js, which has no native add-ons so it uses the best it can observe from pure JS.

You're right that here we could do a native check for the real behavior. I might follow up with a change for it.

@nsavoire let me pick your brains about approaches I'm thinking of. We already have a check in the setter path that's:

auto cped = isolate->GetContinuationPreservedEmbedderData(); // No Node AsyncContextFrame in this continuation yet if (!cped->IsMap()) return;

Do you think "is CPED a Map" would be sufficient to reproduce as a separate little native helper? Then the check would look like:

const probe = new AsyncLocalStorage<number>(); let cpedIsMap = false; probe.run(0, () => { cpedIsMap = pprof.cpedIsMap(); }); probe.disable(); active = cpedIsMap;

or do we want to have to check that the right value gets bound? We could have a native method that returns CPED so we can check if it's a map and also check the values.

const probe = new AsyncLocalStorage<number>(); let acfWorks = false; const value = {}; probe.run(value, () => { const acf = pprof.getCped(); acfWorks = acf instanceof Map && acf.get(probe) === value; }); probe.disable(); active = acfWorks;

If that looks too dangerous of a method to expose, we can also do a more constrained pure query cpedMapContains(key, value) and instead do:

const probe = new AsyncLocalStorage<number>(); let acfWorks = false; const value = {}; probe.run(value, () => { acfWorks = pprof.cpedMapContains(probe, value); }); probe.disable(); active = acfWorks;

So basically, the choice is between cpedIsMap, getCped and cpedMapContains native methods.

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.

…ly (#398) * fix(otel-thread-ctx): detect AsyncContextFrame by reading CPED natively #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. * test: use the real 22.7.0 AsyncContextFrame cutoff

szegedi added 2 commits August 12, 2026 13:36

szegedi requested review from IlyasShabi, nsavoire and r1viollet as code owners August 12, 2026 11:44

IlyasShabi approved these changes Aug 12, 2026

View reviewed changes

szegedi added the semver-patch Bug or security fixes, mainly label Aug 12, 2026

szegedi merged commit 9ac10de into main Aug 13, 2026
70 of 71 checks passed

szegedi deleted the szegedi/acf-feature-detection branch August 13, 2026 07:49

nsavoire reviewed Aug 13, 2026

View reviewed changes

szegedi mentioned this pull request Aug 14, 2026

fix(otel-thread-ctx): detect AsyncContextFrame by reading CPED natively #398

Merged

	isAsyncContextFrameActive() && satisfies(process.versions.node, '>=22.7.0');
	isAsyncContextFrameActive();

		isAsyncContextFrameActive() &&
		satisfies(process.versions.node, '>=22.7.0');

Uh oh!

Conversation

szegedi commented Aug 12, 2026

Uh oh!

github-actions Bot commented Aug 12, 2026

Overall package size

Uh oh!

datadog-official Bot commented Aug 12, 2026 • edited by datadog-prod-us1-6 Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nsavoire commented Aug 12, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 12, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

datadog-official Bot commented Aug 12, 2026 •

edited by datadog-prod-us1-6 Bot

Loading