diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index b96de691..2ff29356 100644 --- a/.claude/skills/release/SKILL.md +++ b/.claude/skills/release/SKILL.md @@ -15,6 +15,13 @@ The `branch-diff` tool must be installed globally: npm install branch-diff -g ``` +Fetch and fast-forward **both** branches before doing anything else. Comparing a +stale `v5.x` against a stale `main` silently produces a wrong commit list: + +``` +git fetch origin && git checkout v5.x && git pull && git checkout main && git pull +``` + ## Steps ### 1. Identify commits to cherry-pick @@ -25,9 +32,36 @@ Use the `branch-diff` tool to list commits on `main` not yet applied to `v5.x`: branch-diff v5.x main ``` -Review the output with the user. Skip: -- Version bump commits (e.g. "Bump package version on to 6.0.0-pre") -- Commits that would result in empty cherry-picks (already applied or superseded) +Its GitHub issue-lookup errors go to stderr; the commit list is on stdout. PR numbers +appear in the trailing URL (`.../pull/393`), *not* as `(#393)` — parsing the `(#NNN)` +form instead picks up PR references that happen to appear in commit titles. + +`branch-diff` matches commits, not content, so it reports a substantial number of +**false positives** — commits whose changes are already on `v5.x`. Do not cherry-pick +these. They fall into three classes: + +**a. Squash-merged releases.** Releases 5.14.2, 5.14.3 and 5.14.4 were squash-merged +rather than rebased, so every commit they contained lost its identity and is reported +forever. This set is closed and will not grow — treat all of these as already released: + +| Release | Proposal | PRs subsumed | +|---|---|---| +| 5.14.2 | #331 | 284, 310, 311, 315, 316, 317, 320, 323, 324, 325, 326, 327, 329 | +| 5.14.3 | #334 | 328, 332 | +| 5.14.4 | #337 | 333, 335, 336 | + +**b. Superseded dependency bumps.** A Dependabot bump that never landed on `v5.x`, which +later picked up an equal-or-newer version of the same package directly. Cherry-picking one +would *downgrade* the branch. Recognise these by comparing the package version in +`v5.x:package.json` against the bump's target — skip when `v5.x` is at or ahead of it. +(Examples seen so far: #140, #344, #348, #349, #350.) + +**c. The `main`-only version bump.** #154 moved `main` to `6.0.0-pre`. It must never be +cherry-picked onto a 5.x release branch. + +Anything left after removing those three classes is a genuine candidate. Note that being +old is *not* by itself evidence of a false positive: #352 sat below all of these and was a +real, unapplied commit. Classify by the rules above, not by age. Confirm the list of commits with the user before proceeding. @@ -54,6 +88,13 @@ Create a git worktree from the current repo, checking out a new branch `v$VERSIO git worktree add ../pprof-nodejs-v5 -b v$VERSION-proposal v5.x ``` +The path is usually still occupied by the previous release's worktree. Once that +proposal's PR is merged, it is safe to clear — verify it is clean and merged first, then: + +``` +git worktree remove ../pprof-nodejs-v5 && git branch -D v-proposal +``` + All subsequent steps run in the worktree directory. ### 4. Cherry-pick commits @@ -66,7 +107,30 @@ git cherry-pick ... If a cherry-pick has conflicts, stop and resolve with the user. -### 5. Create the version bump commit +### 5. Verify the selection against `main` + +Before bumping the version, diff the worktree against `main`: + +``` +git diff --stat main -- . +``` + +The goal is **minimal divergence**: ideally this reports nothing but `package.json` and +`package-lock.json` (the version, plus any dev-dep bump this release includes). + +This is the check that validates step 1, and it is worth doing carefully — it is how #352 +was caught, a genuinely unapplied commit that a plausible-looking age heuristic had +written off as a false positive. Any *other* file appearing here means one of two things: + +- a real commit was wrongly classified as a false positive — cherry-pick it, or +- the divergence is deliberate — say so explicitly in the PR body rather than leaving it + silently unexplained. + +Note that a class-(b) superseded bump correctly shows up as a `package.json` / +`package-lock.json` difference where `v5.x` is *ahead* of `main`. That is expected and +should be left alone. + +### 6. Create the version bump commit Bump the version in package.json and package-lock.json using npm, then commit: @@ -76,7 +140,11 @@ git add package.json package-lock.json git commit -m "v$VERSION" ``` -### 6. Push and create a PR +Keep this commit last on the branch. If a further cherry-pick turns out to be needed after +this point, drop the version commit (`git reset --hard HEAD~1`), apply the cherry-pick, +then re-run the bump — rather than stacking the new commit on top of the release commit. + +### 7. Push and create a PR Push the branch and create a PR targeting `v5.x`: diff --git a/bindings/binding.cc b/bindings/binding.cc index 68741dd8..ea46f95e 100644 --- a/bindings/binding.cc +++ b/bindings/binding.cc @@ -29,6 +29,40 @@ #include #endif +// 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 +// 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. +static NAN_METHOD(CpedMapContains) { +#if NODE_MAJOR_VERSION >= 22 + // A malformed call must not accidentally answer true by comparing an absent + // key's undefined against an undefined expected value. + 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; + } + } + } + } +#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) { #ifdef __APPLE__ uint64_t native_id; @@ -56,4 +90,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/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index cbe34d51..ddee80cf 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -301,7 +301,12 @@ thread_local CtxWrap* g_live_ctx_wraps = nullptr; // fires exactly once, at teardown, while the Environment is still alive. void DrainLiveCtxWraps(void* arg) { auto* isolate = static_cast(arg); + // We must allocate our own HandleScope here as node::FreeEnvironment wraps + // RunCleanup in a SealHandleScope, so handle_.Get() below has to allocate + // inside a scope of our own or V8 aborts with "Cannot create a handle without + // a HandleScope". v8::HandleScope scope(isolate); + CtxWrap* p = g_live_ctx_wraps; while (p != nullptr) { CtxWrap* next = p->next_; @@ -756,7 +761,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/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index fe85c876..0187af47 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -700,15 +700,19 @@ WallProfiler::~WallProfiler() { // unlink. (~PCP still resets its weak handle during delete, so the dangling // internal-field pointer in the wrap object stays inert even if V8 later // GCs the wrap.) + // + // While it'd be tempting to do the same "zero out internal field logic" here + // as in otel-thread-ctx.cc's DrainLiveCtxWraps, we shouldn't. That one only + // ever runs as an environment cleanup hook, while this can also get here from + // Nan::ObjectWrap's weak callback, and V8 forbids the API in a first-pass + // weak callback. The holders' internal fields therefore keep pointing at the + // PCPs we free, but since they are only ever read back through our own + // cpedKey_ that dies with us it is not an issue. auto* p = liveContextPtrHead_; - auto isolate = Isolate::GetCurrent(); while (p != nullptr) { auto* next = p->next_; p->pprev_ = nullptr; p->next_ = nullptr; - if (isolate != nullptr && !p->handle_.IsEmpty()) { - SetAlignedPointerInInternalField(p->handle_.Get(isolate), 0, nullptr); - } delete p; p = next; } diff --git a/package-lock.json b/package-lock.json index bbfc19ac..972bf4cc 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@datadog/pprof", - "version": "5.18.0", + "version": "5.18.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@datadog/pprof", - "version": "5.18.0", + "version": "5.18.1", "license": "Apache-2.0", "dependencies": { "node-gyp-build": "^4.8.4", @@ -15,17 +15,17 @@ }, "devDependencies": { "@types/mocha": "^10.0.1", - "@types/node": "26.1.2", - "@types/semver": "^7.5.8", + "@types/node": "26.2.0", + "@types/semver": "^7.8.0", "@types/sinon": "^22.0.0", "@types/tmp": "^0.2.3", "clang-format": "^1.8.0", "codecov": "^3.8.3", "deep-copy": "^1.4.2", - "eslint-plugin-n": "^18.2.2", + "eslint-plugin-n": "^18.3.0", "gts": "^7.0.0", "js-green-licenses": "^4.0.0", - "mocha": "^11.7.6", + "mocha": "^11.8.0", "nan": "^2.28.0", "nyc": "^18.0.0", "semver": "^7.8.5", @@ -962,9 +962,9 @@ "license": "MIT" }, "node_modules/@types/node": { - "version": "26.1.2", - "resolved": "https://registry.npmjs.org/@types/node/-/node-26.1.2.tgz", - "integrity": "sha512-Vu4a5UFA9rIIFJ7rB/Vaafh9lrCQszopTCx6KjFboXTGQbPNasehVR5TEiithSDGyd1DEiUByggTZsg8jukeIg==", + "version": "26.2.0", + "resolved": "https://registry.npmjs.org/@types/node/-/node-26.2.0.tgz", + "integrity": "sha512-5IviulTZeRNp2vAJ514cc/HUlY5nZ9fCbq9DMyC52BrhFZACo3nI0R7qBxhQmo/d27NFe96ur/b7Wwxklda+kg==", "dev": true, "license": "MIT", "dependencies": { @@ -989,9 +989,9 @@ } }, "node_modules/@types/semver": { - "version": "7.7.1", - "resolved": "https://registry.npmjs.org/@types/semver/-/semver-7.7.1.tgz", - "integrity": "sha512-FmgJfu+MOcQ370SD0ev7EI8TlCAfKYU+B4m5T3yXc1CiRN94g/SZPtsCkk506aUDtlMnFZvasDwHHUcZUEaYuA==", + "version": "7.8.0", + "resolved": "https://registry.npmjs.org/@types/semver/-/semver-7.8.0.tgz", + "integrity": "sha512-1mAINjtQCXXeLkJ9ehXkwOcBpqtLxiVtKhpUf83DdRNdQKV0iXZpaHYqRr7nj+wvxuJzoAmAwXI+sCNMv1CzLQ==", "dev": true, "license": "MIT" }, @@ -2245,9 +2245,9 @@ } }, "node_modules/eslint-plugin-n": { - "version": "18.2.2", - "resolved": "https://registry.npmjs.org/eslint-plugin-n/-/eslint-plugin-n-18.2.2.tgz", - "integrity": "sha512-gOO0lIqwEjZ750kv9/SptCWArUoAZXJoBr0vYWTO2dCBxctHUXlBIigiC8xuxxr/NKqgIT6Ehz1xRcilj8a5cA==", + "version": "18.3.0", + "resolved": "https://registry.npmjs.org/eslint-plugin-n/-/eslint-plugin-n-18.3.0.tgz", + "integrity": "sha512-cPVguuDe6DrIPb/qUXHf8P89MaVTUmiYWwpt5gX5AILsvRIiZAxMFXcFR6QHYBksqKJpjfUBlL/RleCJUWcD7w==", "dev": true, "license": "MIT", "dependencies": { @@ -4105,9 +4105,9 @@ } }, "node_modules/mocha": { - "version": "11.7.6", - "resolved": "https://registry.npmjs.org/mocha/-/mocha-11.7.6.tgz", - "integrity": "sha512-nS9xOGbw2I3cjCpxwZAEJ9xK9lmJ08vEkQvLtz4du9ZrF9UrjRpeJGiIgl2Z+Qs++pmB4ecDe48Fwsh+j+j7xA==", + "version": "11.8.0", + "resolved": "https://registry.npmjs.org/mocha/-/mocha-11.8.0.tgz", + "integrity": "sha512-VyCeUdGN3A9lmCTTgG4yuvY9ixxaDk+xt2R/7/+1AP6EqNG+G9OKkzBwhVtVYoNX8YsxNSgAl8mOv3IAeOpFbw==", "dev": true, "license": "MIT", "dependencies": { diff --git a/package.json b/package.json index 9092580a..85062239 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@datadog/pprof", - "version": "5.18.0", + "version": "5.18.1", "description": "pprof support for Node.js", "repository": { "type": "git", @@ -43,17 +43,17 @@ }, "devDependencies": { "@types/mocha": "^10.0.1", - "@types/node": "26.1.2", - "@types/semver": "^7.5.8", + "@types/node": "26.2.0", + "@types/semver": "^7.8.0", "@types/sinon": "^22.0.0", "@types/tmp": "^0.2.3", "clang-format": "^1.8.0", "codecov": "^3.8.3", "deep-copy": "^1.4.2", - "eslint-plugin-n": "^18.2.2", + "eslint-plugin-n": "^18.3.0", "gts": "^7.0.0", "js-green-licenses": "^4.0.0", - "mocha": "^11.7.6", + "mocha": "^11.8.0", "nan": "^2.28.0", "nyc": "^18.0.0", "semver": "^7.8.5", 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 ' diff --git a/ts/src/async-context-frame.ts b/ts/src/async-context-frame.ts new file mode 100644 index 00000000..a9d6041b --- /dev/null +++ b/ts/src/async-context-frame.ts @@ -0,0 +1,111 @@ +/* + * 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'; +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; + +/** + * 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 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 + * 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. + * + * 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(); + // 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 = bound; + } + 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, 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) { + 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..54a0e970 --- /dev/null +++ b/ts/test/test-async-context-frame.ts @@ -0,0 +1,198 @@ +/* + * 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 {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 { + cpedMapContains(key?: unknown, value?: unknown): boolean; +}; + +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; + 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 (hasAcfSupport) 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.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 (!hasAcfSupport || major >= 24) return this.skip(); + const {active, execArgv} = await probeChild({ + nodeOptions: '--experimental-async-context-frame', + }); + assert.deepEqual(execArgv, []); + 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); + }); +}); diff --git a/ts/test/test-get-value-from-map-profiler.ts b/ts/test/test-get-value-from-map-profiler.ts index 432dac5c..6be2dc48 100644 --- a/ts/test/test-get-value-from-map-profiler.ts +++ b/ts/test/test-get-value-from-map-profiler.ts @@ -29,16 +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 = - (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')); +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 f4d6683b..e28a6f03 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,13 @@ 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 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. +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..1738ee94 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'; @@ -31,11 +32,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')); +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 5b4240af..1da10334 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'; @@ -11,12 +12,7 @@ const DURATION_MILLIS = 1000; const intervalMicros = 10000; 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'))); +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 2a1e4b13..284811b8 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)); @@ -9,12 +10,7 @@ const INTERVAL_MICROS = 10000; 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'))); +const useCPED = withContexts && isAsyncContextFrameActive(); const collectAsyncId = withContexts && satisfies(process.versions.node, '>=24.0.0'); @@ -28,8 +24,22 @@ time.start({ useCPED: useCPED, }); -parentPort?.on('message', () => { - void delay(50).then(() => { - parentPort?.postMessage('hello'); +function listen() { + parentPort?.on('message', () => { + void delay(50).then(() => { + parentPort?.postMessage('hello'); + }); }); -}); +} + +// Establish a sample context, and do it around the listener registration so +// the async context frame holding it stays reachable until we are terminated. +// That leaves a live PersistentContextPtr for ~WallProfiler to walk when it +// runs from the environment cleanup hook; with an empty list the walk is a +// no-op and the teardown path goes untested. +if (useCPED) { + time.runWithContext({worker: 'worker2'}, listen); +} else { + time.setContext({worker: 'worker2'}); + listen(); +}