Skip to content

Commit 774421e

Browse files
committed
Fix npm test on Node 24+ and cover the teardown abort
Two things, both about being able to run the suite at all on current Node. The test script passed --experimental-async-context-frame unconditionally. Node 22/23 need it, but Node 24 turned AsyncContextFrame on by default and removed the flag, so from Node 24 on `npm test` failed before running anything: node: bad option: --experimental-async-context-frame Replace it with a small runner in the style of build.js, which computes the flag list from the running version. --disable-warning is conditional for the same reason: it only exists from Node 21.3. The computation lives in test/node-flags.js because the new test below needs it too, when spawning. Then add the regression test for the CtxWrap teardown abort fixed in the previous commit. It runs in a spawned process because the failure is a SIGABRT: `node --test` runs the file in-process, so an abort takes the whole file down rather than failing one test. Against the pre-fix addon the run reports `pass 0, fail 1` with the CtxWrap::~CtxWrap stack, not 48 passes and one failure — which is also why testing this inline was not an option. npm test now: 49 pass / 0 fail on Node 22, 24 and 26. Previously 24 and 26 could not run it.
1 parent 7800849 commit 774421e

5 files changed

Lines changed: 121 additions & 1 deletion

File tree

js/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
"description": "Node.js writer for the OpenTelemetry thread-context record (OTEP-4947), discoverable via AsyncLocalStorage / AsyncContextFrame.",
55
"main": "index.js",
66
"scripts": {
7-
"test": "node --experimental-async-context-frame --disable-warning=ExperimentalWarning --test test/test.js",
7+
"test": "node test/run.js",
88
"test:docker": "./test/run-in-docker.sh",
99
"install": "node build.js"
1010
},

js/test/node-flags.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
'use strict';
2+
3+
// Node flags needed to make AsyncContextFrame — the writer's discovery
4+
// substrate — available on the running Node.
5+
//
6+
// Node 22/23 need the --experimental-async-context-frame opt-in. Node 24
7+
// turned ACF on by default and *removed* the flag, so passing it there is a
8+
// hard `node: bad option` failure rather than a no-op. Anything that spawns a
9+
// Node process for these tests has to compute the list rather than hardcode
10+
// it; that includes `npm test` itself (see run.js).
11+
function acfFlags() {
12+
const major = Number(process.versions.node.split('.')[0]);
13+
return major >= 22 && major < 24 ? ['--experimental-async-context-frame'] : [];
14+
}
15+
16+
module.exports = { acfFlags };

js/test/run.js

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
'use strict';
2+
3+
// `npm test` entry point.
4+
//
5+
// The script used to pass --experimental-async-context-frame unconditionally,
6+
// which works on Node 22/23 and fails outright from Node 24 on, where the flag
7+
// was removed:
8+
//
9+
// node: bad option: --experimental-async-context-frame
10+
//
11+
// so `npm test` was broken on every current Node. Compute the flags instead.
12+
13+
const { spawnSync } = require('node:child_process');
14+
const path = require('node:path');
15+
16+
const { acfFlags } = require('./node-flags');
17+
18+
const major = Number(process.versions.node.split('.')[0]);
19+
const flags = [...acfFlags()];
20+
// --disable-warning landed in Node 21.3; on older Node it would itself be a
21+
// bad option. Nothing below 22 can run these tests anyway (test.js bails).
22+
if (major >= 22) {
23+
flags.push('--disable-warning=ExperimentalWarning');
24+
}
25+
26+
const res = spawnSync(
27+
process.execPath,
28+
[...flags, '--test', path.join(__dirname, 'test.js')],
29+
{ stdio: 'inherit' },
30+
);
31+
32+
if (res.error) {
33+
console.error(res.error);
34+
process.exit(1);
35+
}
36+
process.exit(res.status === null ? 1 : res.status);

js/test/teardown-child.js

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
'use strict';
2+
3+
// Spawned by the "contexts collected during isolate teardown" test. Runs in
4+
// its own process because the failure mode is a SIGABRT, which would take the
5+
// whole test run down with it.
6+
//
7+
// When CtxWrap derived from node::ObjectWrap, a CtxWrap collected during
8+
// isolate teardown ran ~ObjectWrap -> RemoveEnvironmentCleanupHook, which
9+
// CHECKs that an Environment is current. It is not, during teardown, so:
10+
//
11+
// Assertion failed: (env) != nullptr
12+
// 3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap()
13+
//
14+
// It needs enough instances that V8 still has some left to collect at
15+
// teardown — nothing below ~1000 reproduced it — hence the count.
16+
17+
const { ThreadContext } = require('..');
18+
19+
const N = Number(process.argv[2] || 3000);
20+
21+
function id(n, len) {
22+
const b = Buffer.alloc(len);
23+
b.writeUInt32BE(n >>> 0, 0);
24+
return b;
25+
}
26+
27+
const retained = [];
28+
29+
for (let i = 0; i < N; i++) {
30+
const ctx = new ThreadContext(id(i, 16), id(i, 8), ['k', String(i)]);
31+
if (i % 4 === 0) {
32+
// Still strongly reachable at exit.
33+
retained.push(ctx);
34+
} else {
35+
// Reachable only through the async context frame, so collectable
36+
// whenever V8 decides — including during teardown.
37+
ctx.enter();
38+
}
39+
}
40+
41+
if (retained.length > 0) {
42+
retained[0].enter();
43+
}
44+
globalThis.__retained = retained;
45+
46+
// Exit through the normal path so the Environment is torn down and the
47+
// isolate disposed; that is where the weak callbacks in question fire.
48+
console.log(`created ${N}, retained ${retained.length}`);

js/test/test.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ if (!isAsyncContextFrameAvailable()) {
3333
const path = require('node:path');
3434
const { spawnSync } = require('node:child_process');
3535

36+
const { acfFlags } = require('./node-flags');
37+
3638
const lib = require('..');
3739
const { ThreadContext, getContext, clearContext, getProcessContextAttributes, _currentRecordBytes } = lib;
3840

@@ -597,6 +599,24 @@ test('appendAttributes after invalidate mutates attrs_data but leaves valid=0',
597599
});
598600
});
599601

602+
// Regression test: CtxWrap used to derive from node::ObjectWrap, whose
603+
// destructor calls RemoveEnvironmentCleanupHook. A CtxWrap collected during
604+
// isolate teardown hit that function's CHECK that an Environment is current
605+
// and aborted the process. Spawned, because the failure is a SIGABRT rather
606+
// than an assertion failure.
607+
test('contexts collected during isolate teardown do not abort', () => {
608+
const child = path.join(__dirname, 'teardown-child.js');
609+
const r = spawnSync(process.execPath, [...acfFlags(), child, '3000'], {
610+
encoding: 'utf8',
611+
});
612+
assert.equal(
613+
r.status,
614+
0,
615+
`teardown-child exited with status=${r.status} signal=${r.signal}\n` +
616+
`${r.stdout}${r.stderr}`,
617+
);
618+
});
619+
600620
test('otel_thread_ctx_nodejs_v1 is exported as a TLS dynsym', (t) => {
601621
const addon = path.join(__dirname, '..', 'build', 'Release', 'customlabels.node');
602622
if (!require('node:fs').existsSync(addon)) {

0 commit comments

Comments
 (0)