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.

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

Draft

	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