Skip to content

Commit 10cd8c8

Browse files
committed
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. 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.
1 parent 9ac10de commit 10cd8c8

3 files changed

Lines changed: 178 additions & 9 deletions

File tree

bindings/binding.cc

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,69 @@
2929
#include <unistd.h>
3030
#endif
3131

32+
// Whether the isolate's ContinuationPreservedEmbedderData currently binds
33+
// `key` to `value`.
34+
//
35+
// This exists for AsyncContextFrame feature detection. With ACF active, Node
36+
// implements AsyncLocalStorage#run by installing an AsyncContextFrame — a JS
37+
// Map keyed by the AsyncLocalStorage instance — as the CPED of the running
38+
// continuation. Calling this from inside a run() with the storage and its
39+
// store therefore observes the property this addon actually depends on,
40+
// instead of inferring it from the Node version, process.execArgv, or whether
41+
// run() happens to dispatch through the instance's enterWith.
42+
//
43+
// It is the same slot, and the same "is it a Map" question, that
44+
// WallProfiler::SetContext asks before storing a context, and the identity
45+
// hash published as otel_thread_ctx_nodejs_v1.als_identity_hash is the hash of
46+
// this very key — so a false answer here means both consumers are broken.
47+
//
48+
// Deliberately total: no context entered, CPED unset or not a Map, or the key
49+
// absent all yield false rather than throwing. The otel thread-ctx writer calls
50+
// this from ensureHook(), which runs inside dd-trace-js diagnostic-channel
51+
// subscribers, where an exception would surface in application code.
52+
//
53+
// Uses the public v8::Map::Get rather than the raw OrderedHashMap walk in
54+
// map-get.hh on purpose: this answers "is ACF on", and conflating it with "is
55+
// our map layout knowledge still correct" would report a V8 layout change as
56+
// ACF being unavailable. Layout is covered separately, by the GetValueFromMap
57+
// tests.
58+
static NAN_METHOD(CpedMapContains) {
59+
#if NODE_MAJOR_VERSION >= 22
60+
auto isolate = info.GetIsolate();
61+
62+
// A malformed call must not accidentally answer true by comparing an absent
63+
// key's undefined against an undefined expected value.
64+
if (info.Length() < 2) {
65+
info.GetReturnValue().Set(false);
66+
return;
67+
}
68+
69+
auto context = isolate->GetCurrentContext();
70+
if (context.IsEmpty()) {
71+
info.GetReturnValue().Set(false);
72+
return;
73+
}
74+
75+
auto cped = isolate->GetContinuationPreservedEmbedderData();
76+
if (cped.IsEmpty() || !cped->IsMap()) {
77+
info.GetReturnValue().Set(false);
78+
return;
79+
}
80+
81+
v8::Local<v8::Value> found;
82+
if (!cped.As<v8::Map>()->Get(context, info[0]).ToLocal(&found)) {
83+
info.GetReturnValue().Set(false);
84+
return;
85+
}
86+
87+
info.GetReturnValue().Set(found->StrictEquals(info[1]));
88+
#else
89+
// No ContinuationPreservedEmbedderData, and no AsyncContextFrame to put in
90+
// it, so false is the right answer rather than a missing export.
91+
info.GetReturnValue().Set(false);
92+
#endif
93+
}
94+
3295
static NAN_METHOD(GetNativeThreadId) {
3396
#ifdef __APPLE__
3497
uint64_t native_id;
@@ -56,4 +119,5 @@ NODE_MODULE_INIT(/* exports, module, context */) {
56119
dd::WallProfiler::Init(exports);
57120
dd::OtelThreadCtx::Init(exports);
58121
Nan::SetMethod(exports, "getNativeThreadId", GetNativeThreadId);
122+
Nan::SetMethod(exports, "cpedMapContains", CpedMapContains);
59123
}

ts/src/async-context-frame.ts

Lines changed: 41 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,23 @@
1515
*/
1616

1717
import {AsyncLocalStorage} from 'node:async_hooks';
18+
import {join} from 'path';
19+
20+
interface Addon {
21+
cpedMapContains(key: unknown, value: unknown): boolean;
22+
}
23+
24+
let addon: Addon | undefined;
25+
26+
// Required lazily so importing this module doesn't force the addon to load;
27+
// memoized by isAsyncContextFrameActive, so this runs at most once per thread.
28+
function bindings(): Addon {
29+
if (!addon) {
30+
const findBinding = require('node-gyp-build');
31+
addon = findBinding(join(__dirname, '..', '..')) as Addon;
32+
}
33+
return addon;
34+
}
1835

1936
let active: boolean | undefined;
2037

@@ -39,19 +56,34 @@ let active: boolean | undefined;
3956
* main thread's command line either, and tooling sometimes rewrites
4057
* `process.execArgv` outright.
4158
*
42-
* With ACF, `run()` is implemented in terms of `enterWith()`; without it, it
43-
* isn't. Memoized: the answer is fixed for the life of the thread.
59+
* Detected by asking the addon what is in the CPED slot during a `run()`. With
60+
* ACF, Node installs an AsyncContextFrame — a JS Map keyed by the
61+
* `AsyncLocalStorage` instance, valued by its store — as the running
62+
* continuation's CPED; without it, nothing writes the slot. So a probe storage
63+
* whose own store is visible there is direct evidence, and it is evidence about
64+
* the exact slot both consumers read: `WallProfiler::SetContext` requires that
65+
* Map, and the thread-ctx reader looks this very key up by the identity hash
66+
* published as `als_identity_hash`.
67+
*
68+
* Observing whether `run()` delegates to `enterWith()` would be an indirect
69+
* proxy for the same thing: it holds today, but it depends on `run()`
70+
* dispatching through the instance property, which is unspecified and which
71+
* anything patching `AsyncLocalStorage` can break — and the failure would be
72+
* silent and in the dangerous direction.
73+
*
74+
* Memoized: the answer is fixed for the life of the thread.
4475
*/
4576
export function isAsyncContextFrameActive(): boolean {
4677
if (active === undefined) {
47-
const probe = new AsyncLocalStorage<number>();
48-
let delegated = false;
49-
probe.enterWith = () => {
50-
delegated = true;
51-
};
52-
probe.run(0, () => {});
78+
const probe = new AsyncLocalStorage<object>();
79+
// Object identity, so a stray equal-valued binding can't answer for us.
80+
const sentinel = {};
81+
let bound = false;
82+
probe.run(sentinel, () => {
83+
bound = bindings().cpedMapContains(probe, sentinel);
84+
});
5385
probe.disable();
54-
active = delegated;
86+
active = bound;
5587
}
5688
return active;
5789
}

ts/test/test-async-context-frame.ts

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,16 @@
1515
*/
1616

1717
import {strict as assert} from 'assert';
18+
import {AsyncLocalStorage} from 'node:async_hooks';
1819
import {fork} from 'node:child_process';
1920
import {join} from 'node:path';
2021

2122
import {isAsyncContextFrameActive} from '../src/async-context-frame';
2223

24+
const addon = require('node-gyp-build')(join(__dirname, '..', '..')) as {
25+
cpedMapContains(key?: unknown, value?: unknown): boolean;
26+
};
27+
2328
const CHILD = join(__dirname, 'async-context-frame-child.js');
2429

2530
const major = Number(process.versions.node.split('.')[0]);
@@ -119,3 +124,71 @@ describe('isAsyncContextFrameActive', () => {
119124
assert.equal(active, true);
120125
});
121126
});
127+
128+
// The detection asks whether the running storage is bound to its own store,
129+
// not merely whether the CPED slot holds a Map. These pin that difference:
130+
// without them, weakening the helper to a bare IsMap check would still pass
131+
// every test above.
132+
describe('cpedMapContains', () => {
133+
beforeEach(function () {
134+
// With ACF off nothing writes the slot, so every answer here is false for
135+
// an uninteresting reason. The routes that discriminate on/off are covered
136+
// by the child-process cases above.
137+
if (!isAsyncContextFrameActive()) this.skip();
138+
});
139+
140+
it('finds the running storage bound to its store', () => {
141+
const als = new AsyncLocalStorage<object>();
142+
const store = {};
143+
let found = false;
144+
als.run(store, () => {
145+
found = addon.cpedMapContains(als, store);
146+
});
147+
als.disable();
148+
assert.equal(found, true);
149+
});
150+
151+
it('does not match a foreign key', () => {
152+
// CPED is a general embedder slot. Another native addon storing a Map there
153+
// must not be able to answer for us, which is the false positive an IsMap
154+
// check would admit.
155+
const als = new AsyncLocalStorage<object>();
156+
const store = {};
157+
let found = true;
158+
als.run(store, () => {
159+
found = addon.cpedMapContains(new AsyncLocalStorage<object>(), store);
160+
});
161+
als.disable();
162+
assert.equal(found, false);
163+
});
164+
165+
it('does not match a different value for the right key', () => {
166+
const als = new AsyncLocalStorage<object>();
167+
let found = true;
168+
als.run({}, () => {
169+
found = addon.cpedMapContains(als, {});
170+
});
171+
als.disable();
172+
assert.equal(found, false);
173+
});
174+
175+
it('is false outside any run', () => {
176+
const als = new AsyncLocalStorage<object>();
177+
const store = {};
178+
als.run(store, () => {});
179+
als.disable();
180+
assert.equal(addon.cpedMapContains(als, store), false);
181+
});
182+
183+
it('is false when called without a key and value', () => {
184+
// An absent key reads as undefined; so would a missing expected value, so
185+
// a malformed call must not compare the two and report success.
186+
const als = new AsyncLocalStorage<object>();
187+
let found = true;
188+
als.run({}, () => {
189+
found = addon.cpedMapContains();
190+
});
191+
als.disable();
192+
assert.equal(found, false);
193+
});
194+
});

0 commit comments

Comments
 (0)