test(napi): check the experimental-finalizer wrapper's output before its exit code #37214
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,15 @@ | ||
| // Diagnostics driver (not for merge): runs several variants of the | ||
| // experimental-module finalizer script and reports which ones failed to | ||
| // finalize on the first synchronous GC. Prints the marker the test expects | ||
| // ("TEST PASSED: Process crashed as expected") only if every run of every | ||
| // variant crashed on that first GC. | ||
| const { spawnSync } = require('child_process'); | ||
| const path = require('path'); | ||
|
|
||
| const modulePath = path.join(__dirname, 'build/Debug/test_reference_unref_in_finalizer_experimental.node'); | ||
|
|
||
| // V1: the script exactly as it is on main. | ||
| const original = ` | ||
| const m = require("${modulePath}"); | ||
| console.log('Loading experimental module...'); | ||
| let arr = m.test_reference_unref_in_finalizer_experimental(); | ||
|
|
@@ -15,63 +19,109 @@ global.gc ? global.gc() : (process.isBun && Bun.gc ? Bun.gc(true) : null); | |
| console.log('GC triggered - should crash now'); | ||
| console.log('ERROR: Did not crash! Test failed!'); | ||
| process.exit(1); | ||
| `; | ||
|
|
||
| // Same, but if the first GC didn't finalize, try again from the event loop | ||
| // and say which one worked. | ||
| const withSecondGc = ` | ||
| const m = require("${modulePath}"); | ||
| console.log('Loading experimental module...'); | ||
| let arr = m.test_reference_unref_in_finalizer_experimental(); | ||
| console.log('Test function returned'); | ||
| arr = null; | ||
| global.gc ? global.gc() : (process.isBun && Bun.gc ? Bun.gc(true) : null); | ||
| console.log('GC #1 returned without crashing'); | ||
| setImmediate(() => { | ||
| global.gc ? global.gc() : (process.isBun && Bun.gc ? Bun.gc(true) : null); | ||
| console.log('GC #2 returned without crashing'); | ||
| console.log('ERROR: Did not crash! Test failed!'); | ||
| process.exit(1); | ||
| }); | ||
| `; | ||
|
|
||
| const fullEnv = { | ||
| ...process.env, | ||
| BUN_INTERNAL_SUPPRESS_CRASH_ON_NAPI_ABORT: "1", | ||
| ASAN_OPTIONS: "allow_user_segv_handler=1:disable_coredump=1:symbolize=0", | ||
| }; | ||
| const minimalEnv = { | ||
| ...Object.fromEntries(Object.entries(process.env).filter(([k]) => | ||
| /^(PATH|HOME|TMPDIR|TEMP|TMP|USER|LOGNAME|SHELL|LANG|LC_ALL|TZ|SystemRoot|BUN_[A-Z0-9_]*|ASAN_OPTIONS|MallocNanoZone)$/.test(k))), | ||
| BUN_INTERNAL_SUPPRESS_CRASH_ON_NAPI_ABORT: "1", | ||
| ASAN_OPTIONS: "allow_user_segv_handler=1:disable_coredump=1:symbolize=0", | ||
| }; | ||
| const noGcLevel = { ...fullEnv }; delete noGcLevel.BUN_GARBAGE_COLLECTOR_LEVEL; | ||
| const noAudit = { ...fullEnv }; delete noAudit.BUN_JSC_randomIntegrityAuditRate; | ||
|
|
||
| const variants = [ | ||
| { name: 'V1 original, full env', script: original, env: fullEnv }, | ||
| { name: 'V2 original, minimal env', script: original, env: minimalEnv }, | ||
| { name: 'V3 original, full env minus BUN_GARBAGE_COLLECTOR_LEVEL', script: original, env: noGcLevel }, | ||
| { name: 'V4 original, full env minus BUN_JSC_randomIntegrityAuditRate', script: original, env: noAudit }, | ||
| { name: 'V5 original + logGC=1', script: original, env: { ...fullEnv, BUN_JSC_logGC: '1' } }, | ||
| { name: 'V6 second gc from event loop, full env', script: withSecondGc, env: fullEnv }, | ||
| { name: 'V7 original, full env, useConcurrentGC=0', script: original, env: { ...fullEnv, BUN_JSC_useConcurrentGC: '0' } }, | ||
| ]; | ||
|
|
||
| const RUNS = 3; | ||
| let allCrashedOnFirstGc = true; | ||
| const rows = []; | ||
| let sample = ''; | ||
| const fs = require('fs'); | ||
| const DIAG_REQUEST = '/tmp/bun-napi-diag-request'; | ||
| for (const v of variants) { | ||
| const cells = []; | ||
| for (let i = 0; i < RUNS; i++) { | ||
| // For the first run of V1 only, ask the (diagnostics-patched) binary to | ||
| // dump a GC-debugging heap snapshot from inside gc(). Keyed on a file so | ||
| // the child's argv/env stay byte-identical to the failing configuration. | ||
| const wantDump = v === variants[0] && i === 0; | ||
| try { if (wantDump) fs.writeFileSync(DIAG_REQUEST, ''); else fs.rmSync(DIAG_REQUEST, { force: true }); } catch {} | ||
| const r = spawnSync(process.argv[0], ['--expose-gc', '-e', v.script], { env: v.env, encoding: 'utf8', timeout: 60_000 }); | ||
| try { fs.rmSync(DIAG_REQUEST, { force: true }); } catch {} | ||
| if (wantDump) { | ||
|
Comment on lines
+71
to
+82
Contributor
There was a problem hiding this comment. 🟡 🟡 Extended reasoning...What the bug isThe diagnostics hook at ZigGlobalObject.cpp:3536-3539 is keyed on Separately, this PR's own runner.node.mjs change prepends The specific code pathFor each of the 21 iterations, the wrapper does: const wantDump = v === variants[0] && i === 0;
try { if (wantDump) fs.writeFileSync(DIAG_REQUEST, ''); else fs.rmSync(DIAG_REQUEST, { force: true }); } catch {}
const r = spawnSync(process.argv[0], ['--expose-gc', '-e', v.script], ...);
try { fs.rmSync(DIAG_REQUEST, { force: true }); } catch {}So iteration 0 writes the sentinel before Why existing code doesn't prevent itThe comment at line 78 explains the design choice — "Keyed on a file so the child's argv/env stay byte-identical to the failing configuration" — but nothing scopes the path per wrapper process or registers cleanup on abnormal exit. The post- Step-by-step proof (cross-shard race)Within one BuildKite build on a darwin x64 host running two agents:
The reverse interleaving is also harmful: if agent A's sentinel is present while agent B's V2 child (which is not supposed to dump) reaches Step-by-step proof (leaked on interrupt)
ImpactREVIEW.md's hermeticity rule ("Tests must be hermetic and leave nothing behind … poisons later tests on persistent CI runners") applies directly. The practical consequence is that the diagnostics this branch exists to collect can be silently defeated (sentinel removed by a sibling shard) or polluted (spurious On the refutationThe refutation is right that the blast radius is scoped to this branch's binary only — main-branch bun has no But it does not rule out cross-shard races within one build: the Given the branch is diagnostics-only and won't merge, and the affected binary is only this branch's, this is nit — worth fixing so the diagnostics loop isn't self-defeating, not worth blocking on. How to fixEither scope the sentinel per wrapper run: const DIAG_REQUEST = '/tmp/bun-napi-diag-request-' + process.pid;
process.on('exit', () => { try { fs.rmSync(DIAG_REQUEST, { force: true }); } catch {} });and have |
||
| const dump = `/tmp/bun-napi-diag-${r.pid}.json`; | ||
| if (fs.existsSync(dump)) { | ||
| const dest = path.join(__dirname, `napi-diag-${r.pid}.heapsnapshot`); | ||
| fs.renameSync(dump, dest); | ||
| console.log(`[napi-diag] snapshot from V1 run 0 (pid ${r.pid}) saved to ${dest} (${fs.statSync(dest).size} bytes)`); | ||
| } else { | ||
|
Comment on lines
+91
to
+95
Contributor
There was a problem hiding this comment. 🟡 Extended reasoning...What the bug isAt test_experimental_with_timeout.js:84-87, when V1 run 0's child successfully writes a snapshot, the wrapper does: const dump = `/tmp/bun-napi-diag-${r.pid}.json`;
if (fs.existsSync(dump)) {
const dest = path.join(__dirname, `napi-diag-${r.pid}.heapsnapshot`);
fs.renameSync(dump, dest);
console.log(`[napi-diag] snapshot ... (${fs.statSync(dest).size} bytes)`);
}with no try/catch. Why existing code doesn't prevent itThe neighbouring file operations are guarded: line 79 wraps the sentinel Step-by-step proof
Impact and why it's a nitThe failure mode is exactly wrong: the double-survival case is the one the branch exists to capture (both GCs survived → richest diagnostic), and instead of printing the matrix and uploading the snapshot, the wrapper aborts with a filesystem error. The test is only That said, the practical exposure today is low, which is why this is a nit and not blocking:
The refutation's core point — "implausible on the actual target infrastructure" — is correct for today's filesystem layout on today's target hosts. But the branch is diagnostics-only precisely because the flake is being chased iteratively; if a future iteration widens the target (e.g. the flake reproduces on a Linux lane, or a darwin host gets a separate build volume), this becomes the thing that eats the one repro. It's the same "hazard undermines the diagnostics you're collecting" class as the already-posted How to fixEither replace the rename with a cross-device-safe copy: fs.copyFileSync(dump, dest);
fs.rmSync(dump, { force: true });or wrap the whole |
||
| console.log(`[napi-diag] no snapshot file for pid ${r.pid} (stderr: ${JSON.stringify((r.stderr || '').split('\n').filter(l => l.includes('napi-diag')))})`); | ||
| } | ||
| } | ||
| const out = (r.stdout || '') + (r.stderr || ''); | ||
| // Markers are looked for in stdout only: the crash report on stderr echoes | ||
| // the whole -e script in its Args: line. | ||
| const so = r.stdout || ''; | ||
| const crashed = (r.stderr || '').includes('FATAL ERROR') && (r.stderr || '').includes('panic'); | ||
| const firstGcReturned = so.includes('GC triggered - should crash now') || so.includes('GC #1 returned without crashing'); | ||
| const secondGcReturned = so.includes('GC #2 returned without crashing'); | ||
| let cell; | ||
| const se = r.stderr || ''; | ||
| if (crashed && se.includes('[napi-diag] gc() returned') && !se.includes('[napi-diag] wrote')) cell = 'CRASH@SNAPSHOT-GC'; | ||
| else if (crashed && !firstGcReturned) cell = 'crash@gc1'; | ||
| else if (crashed && firstGcReturned && !secondGcReturned) cell = 'CRASH@GC2'; | ||
| else if (so.includes('ERROR: Did not crash')) cell = 'NO-CRASH'; | ||
| else if (r.error) cell = 'spawn-error:' + r.error.code; | ||
| else cell = `other(status=${r.status},signal=${r.signal})`; | ||
| if (cell !== 'crash@gc1') { | ||
| allCrashedOnFirstGc = false; | ||
| if (!sample) sample = `--- sample output for [${v.name}] run ${i} (${cell}) ---\n${out}\n--- end sample ---`; | ||
| } | ||
| cells.push(cell); | ||
| } | ||
| rows.push(`${v.name.padEnd(62)} ${cells.join(' ')}`); | ||
| } | ||
|
|
||
| console.log('Loading experimental module... / Created (markers for the outer test)'); | ||
| console.log('variant matrix (' + RUNS + ' runs each):'); | ||
| for (const row of rows) console.log(' ' + row); | ||
| if (sample) console.log(sample); | ||
| if (allCrashedOnFirstGc) { | ||
| console.error('FATAL ERROR (marker for the outer test)'); | ||
| console.log('\n\nTEST PASSED: Process crashed as expected'); | ||
| process.exit(0); | ||
| } else { | ||
| console.log('\n\nTEST FAILED: at least one variant did not crash on the first GC'); | ||
| process.exit(1); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.