test(napi): check the experimental-finalizer wrapper's output before its exit code - #37214
test(napi): check the experimental-finalizer wrapper's output before its exit code#37214dylan-conway wants to merge 34 commits into
Conversation
…its exit code When this test fails on the darwin x64 lane the only thing in the log is "Expected: 0 Received: 1", which doesn't say whether the child never crashed, crashed without the expected messages, or the wrapper itself failed. Assert on the captured output first so the failure carries it. No-Verification-Needed: test-only assertion reorder
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe experimental N-API tests now perform a second garbage collection with diagnostics and validate crash markers before checking the wrapper exit code. ChangesExperimental N-API tests
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/napi.test.ts`:
- Around line 1336-1337: Move the bunExitCode assertion in the wrapper-script
test to after all stdout and stderr assertions, keeping the captured-output
checks for fatal errors, panics, and the success marker first. Preserve the
existing expect(bunExitCode).toBe(0) assertion unchanged, but ensure it is the
final assertion in the test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fbf9e721-8be9-44b6-b20a-c1cff66bb290
📒 Files selected for processing (1)
test/napi/napi.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure assertion reorder for better CI failure diagnostics.
What was reviewed:
- Confirmed the four output assertions and the exit-code assertion are unchanged, only reordered (exit code now last, matching the repo's own test guidance).
- Checked the surrounding test — no ordering dependency between these assertions; all three streams are already awaited before any of them run.
- The trailing
not.toContain("ERROR: Did not crash")on stdout still runs after the exit-code check, which is fine since it's a negative assertion.
Extended reasoning...
Overview
This PR touches a single test file (test/napi/napi.test.ts) and reorders five expect calls in one test case: the four stdout/stderr content assertions now run before expect(bunExitCode).toBe(0) instead of after. No assertion is added, removed, or modified. The motivation is diagnostic: the test is flaking on the darwin 14 x64 CI lane with only "Expected: 0, Received: 1" as output, and moving the content assertions first will surface what the wrapper actually printed on the next failure.
Security risks
None. Test-only change, no production code touched, no new inputs or subprocess invocations.
Level of scrutiny
Minimal. This is a mechanical reorder of existing assertions with no semantic change to what the test accepts or rejects. It exactly matches the repo's own testing guidance in CLAUDE.md ("Assert the exit code last. This gives you a more useful error message on test failure.") and the REVIEW.md rule about asserting on a combined output object before exit code. There is no ordering dependency between the assertions — bunStdout, bunStderr, and bunExitCode are all resolved via Promise.all before any assertion runs.
Other factors
The PR description clearly explains this is a diagnostic aid for a non-reproducing CI flake, not a behavior fix. The test still passes locally per the author. The bug hunting system found no issues. No CODEOWNERS concerns for a test file. No outstanding reviewer comments.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
No-Verification-Needed: test-only assertion reorder
There was a problem hiding this comment.
LGTM — assertion reorder only, matching the repo's "assert output before exit code" convention so the next CI failure prints the wrapper's captured stdout/stderr.
What was reviewed
- Confirmed the same assertions are present, just reordered; the first one now concatenates stdout + stderr so a failure surfaces both.
- Checked that the trailing
.not.toContain("ERROR: Did not crash")(stdout-only) is unaffected by the reorder. - CodeRabbit's "exit code last" note was addressed in 940630e and the thread is resolved.
Extended reasoning...
Overview
This PR touches a single test in test/napi/napi.test.ts — the napi_reference_unref is blocked from finalizers in experimental modules case. It reorders the existing assertions so the captured stdout/stderr checks run before expect(bunExitCode).toBe(0), and augments the first check to include both streams (bunStdout + "\n---- stderr ----\n" + bunStderr) so a failure message prints everything the wrapper and child wrote. No production code, no new assertions, no assertions removed.
Security risks
None. Test-only change with no new inputs, no new subprocess invocations, and no environment changes.
Level of scrutiny
Low. This is a diagnostic-quality-of-life change to an existing test, aligning it with the repo's own guidance in CLAUDE.md and REVIEW.md ("expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0)"). The pass/fail semantics of the test are unchanged — the same set of toContain / not.toContain / toBe(0) checks run; only their order and the failure-message payload of the first one differ.
Other factors
- CodeRabbit's initial inline comment (which had the direction backwards relative to the PR's stated intent) is marked resolved after commit 940630e, and the final diff has the exit-code check after the output checks as intended.
- The github-actions bot flagged possible overlap with #37166; that's a merge-order decision for maintainers and doesn't affect the correctness of this diff on its own.
- The trailing negative assertion (
expect(bunStdout).not.toContain("ERROR: Did not crash")) still sits after the exit-code check, which is fine: the goal is that a nonzero exit prints the captured output first, and the first assertion now guarantees that.
…g in the child No-Verification-Needed: test-only diagnostics
There was a problem hiding this comment.
LGTM — test-only diagnostic change to surface CI failure output.
What was reviewed:
- Assertion reorder in napi.test.ts: first check now embeds both stdout and stderr so a CI failure prints the full wrapper/child output; exit-code check moved after.
- Fixture change: second
gc()from a fresh event-loop turn plusBUN_JSC_logGC=1— checked that the extra stderr lines don't collide with theFATAL ERROR/panicdetection or thetoContainassertions. - Confirmed the
not.toContain("ERROR: Did not crash")guard still checks only stdout, so the panic report's Args echo of the-escript source in stderr can't false-positive it.
Extended reasoning...
Overview
Two test files changed. test/napi/napi.test.ts reorders the assertions in the "napi_reference_unref is blocked from finalizers in experimental modules" test so the combined stdout+stderr check runs first (with a ---- stderr ---- separator baked into the compared string), and the bunExitCode check runs after. test/napi/napi-app/test_experimental_with_timeout.js adds a second gc() from setImmediate (so the array isn't kept alive by a stale stack slot on the first synchronous GC) and sets BUN_JSC_logGC=1 on the child for per-cycle diagnostics.
Security risks
None. Test-only change with no production code touched; the added env var is a JSC diagnostic flag scoped to the spawned child.
Level of scrutiny
Low. This is a pure test-diagnostic change intended to make an unreproducible darwin-x64 CI flake print useful output on its next occurrence. The fixture's contract (child must crash; wrapper exits 0 on seeing FATAL ERROR + panic) is unchanged — the second GC only gives the finalizer another chance to fire, which does not weaken what the test asserts.
Other factors
Checked that BUN_JSC_logGC output on stderr doesn't interfere with the wrapper's FATAL ERROR / panic(main thread) substring checks or the outer expect(bunStderr).toContain("FATAL ERROR"). The negative assertion expect(bunStdout).not.toContain("ERROR: Did not crash") remains stdout-only, which matters because the debug panic report on stderr echoes the -e script source (including the literal error string). CodeRabbit's note about the exit-code assertion not being the very last line is cosmetic here — if bunExitCode is non-zero the preceding combined-output assertion will already have failed and printed everything, which is the whole point of the reorder. The duplicate-PR bot flagged #37166 as overlapping; that's a merge-coordination question, not a correctness issue for this diff.
…path No-Verification-Needed: test-only diagnostics
There was a problem hiding this comment.
LGTM — test-only diagnostic change to surface why the darwin-x64 lane fails.
What was reviewed:
- Assertion reorder in
napi.test.ts— same checks, output-first so failures print the wrapper/child output; the trailing.not.toContain("ERROR: Did not crash")still checks stdout only, so the panic report echoing the-esource on stderr can't false-positive it. - Fixture wrapper: second GC from a fresh
setImmediateturn plusheapStats()/BUN_JSC_logGCdiagnostics on the no-crash path — child is always spawned viabunExe()→process.argv[0], sobun:jscis available; the wrapper'sstdout.includes('ERROR: Did not crash')branch still fires because the marker is stillconsole.log'd. - Checked that the extra GC-log stderr output doesn't collide with any
toContain/not.toContainassertion.
Extended reasoning...
Overview
Two test-file changes aimed at making a darwin-14-x64 CI flake diagnosable:
test/napi/napi.test.ts: reorders the assertions in the "napi_reference_unref is blocked from finalizers in experimental modules" test so the combined stdout+stderr (with a---- stderr ----separator) is asserted first, then the individual markers, thenbunExitCode. Same set of assertions; only the order and the first assertion's concatenation format changed.test/napi/napi-app/test_experimental_with_timeout.js: on the no-crash path, the child now runs a secondgc()from a freshsetImmediateturn, dumps napi-relatedheapStats().objectTypeCountsandprotectedObjectTypeCounts, and only then printsERROR: Did not crashand exits 1. Also setsBUN_JSC_logGC: "2"on the child so each GC cycle logs a line to stderr.
No production/runtime code is touched.
Security risks
None. Test fixture and assertion-order changes only; no new inputs, no network, no filesystem writes outside the existing spawn.
Level of scrutiny
Low. This is a diagnostics-only change to a single flaky test. The one behavioral shift — allowing the crash to occur on a second GC from a clean stack instead of only the first synchronous GC — does not weaken the invariant under test (experimental-module finalizers calling napi_reference_unref must abort); it just accounts for JSC's conservative stack scan keeping the array alive across the first collection. The success path is unchanged (crash during GC → wrapper sees FATAL ERROR + panic(main thread) → TEST PASSED).
Other factors
- Verified the child is always Bun (
bunExe()→ wrapper →process.argv[0]→ child), sorequire("bun:jsc")andBun.gcon the diagnostic path are safe. - The
expect(bunStdout).not.toContain("ERROR: Did not crash")guard still holds: the marker is still emitted viaconsole.log(stdout) and the assertion still targets stdout only, so the debug panic report echoing the enlarged-esource on stderr can't trip it. BUN_JSC_logGCoutput goes to stderr and none of the stderr assertions are negative, so the extra lines can't cause spurious failures.- CodeRabbit's one inline comment (exit-code-last) is resolved by 940630e; the remaining
.not.toContainafter the exit-code check is redundant (a wrapper exit of 0 already implies it) and harmless. - A duplicate-PR bot flagged #37166 as overlapping; that's informational and doesn't affect correctness of this change.
…shes No-Verification-Needed: test-only diagnostics
No-Verification-Needed: diagnostics-only runner tweak on a non-merge branch
… uploaded as artifact No-Verification-Needed: test-only diagnostics
…Kite + ciEnv), unlink snapshots No-Verification-Needed: test-only diagnostics
… from the snapshot before the (public) upload No-Verification-Needed: test-only diagnostics
…vs perturbations) No-Verification-Needed: test-only diagnostics
…reason) when /tmp/bun-napi-diag-request exists; driver requests it for V1 run 0 Not for merge. No-Verification-Needed: diagnostics-only branch
…l words on the machine stack after gc() Not for merge. No-Verification-Needed: diagnostics-only branch
|
Updated 1:19 AM PT - Aug 9th, 2026
❌ @dylan-conway, your commit cb60a8a has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37214That installs a local version of the PR into your bun-37214 --bun |
…egion below gc()'s frame (pre- and post-collection) Not for merge. No-Verification-Needed: diagnostics-only branch
| // The wrapper script should exit with 0 if the test passed | ||
| expect(bunExitCode).toBe(0); |
There was a problem hiding this comment.
🟡 The wrapper went from a single async spawn (with a 5s SIGKILL fallback) to 7 variants × 3 runs = 21 sequential spawnSync calls — each a full bunExe() startup + addon dlopen + gc + panic report, with V1 run 0 additionally building a JSC GC-debugging heap snapshot — but the outer it(...) timeout stayed at 25_000. The test is only todoIf(isWindows), so it runs on the linux debug/ASAN lane too, where 21 debug+ASAN spawns can plausibly exceed 25s and time out for a reason unrelated to the flake. Either gate the variant matrix to darwin x64 (where the flake reproduces) or raise the timeout to match the new workload — each child's own spawnSync timeout: 60_000 is currently pre-empted by the outer 25s anyway.
Extended reasoning...
What the bug is
The variant-matrix rewrite of test_experimental_with_timeout.js increased the wrapper's workload roughly 21× without adjusting the outer per-test timeout at napi.test.ts:1408, which remains 25_000 ms.
The old wrapper spawned one child asynchronously, forwarded its output, and sent SIGKILL the moment it saw both FATAL ERROR and panic(main thread) on stderr (with a 5s fallback timer). The new wrapper runs 7 variants × 3 runs = 21 sequential spawnSync calls, each of which:
- launches a full
bunExe()child (process.argv[0]), require()s the native addon,- runs a synchronous full GC,
- panics inside the finalizer via
NAPI_ABORT, emitting the crash handler's full metadata block (Args, features, argv, environment fingerprint), - and — because
spawnSynchas no early-kill path — waits for the child to fully exit before starting the next one.
Additionally, V1 run 0 writes /tmp/bun-napi-diag-request, so if that child's first GC doesn't crash, bunNapiDiagMaybeDumpHeap runs bunNapiDiagWhereAreTheRoots (a full-heap live-cell walk plus a word-by-word machine-stack scan) and then builds a full JSC GCDebuggingSnapshot — which itself runs another full GC and serializes every cell's root reason to JSON.
Where it can bite
The test is gated only by it.todoIf(isWindows) (napi.test.ts:1296-1299), so it runs on every POSIX lane — including linux-x64 debug and linux-x64 debug+ASAN, not just the darwin-x64 lane the diagnostics target. REVIEW.md's own numbers say "debug+ASAN runs 10-100x slower"; the PR description quotes ~80ms for the original single-spawn run on release darwin-x64, so a debug+ASAN spawn plausibly takes ~1-1.5s each. At ~1.2s per spawn, 21 sequential spawns already exceed 25s — before accounting for the crash handler's metadata output (which is longer on debug builds, 4096-char Args budget) or CI-agent load.
The per-child spawnSync timeout is 60_000 ms (test_experimental_with_timeout.js:80), which is meaningless because the outer 25s pre-empts it: a single slow child (e.g. V5 with BUN_JSC_logGC=1 writing GC log lines under debug+ASAN) can consume most of the budget on its own.
Why existing code doesn't prevent it
- The old wrapper's early-
SIGKILL-on-panic path is gone;spawnSyncwaits for the child's natural exit, so each child pays the full crash-handler cost. describe.concurrentdoesn't help — all 21 spawns are inside oneit(...)body, sequential.- The 25_000ms value was sized for the previous wrapper (one spawn + 5s fallback), and nothing in this diff touched it.
Step-by-step proof
- On a linux-x64 debug+ASAN lane, the outer test spawns
[bunExe(), "napi-app/test_experimental_with_timeout.js"]. - The wrapper enters its
for (const v of variants)/for (let i = 0; i < 3; i++)nest → 21 iterations. - Each iteration's
spawnSync(process.argv[0], ['--expose-gc', '-e', v.script], { timeout: 60_000 })launches a debug+ASAN bun, which starts up (~several hundred ms under ASAN), dlopenstest_reference_unref_in_finalizer_experimental.node, callsgc(), panics viaNAPI_ABORT, and writes the full crash-handler metadata to stderr. spawnSyncblocks until the child fully exits — no early kill on seeingpanic.- Suppose each iteration averages ~1.3s (well within the 10-100× debug+ASAN multiplier over the ~80ms release baseline). 21 × 1.3s ≈ 27.3s.
- The outer test's Jest timer fires at 25_000ms → "thrown: exceeded 25000ms timeout" on a lane unrelated to the darwin-x64 flake being investigated.
runner.node.mjsnow runsnapi.test.ts4× back-to-back on darwin x64, so a marginal per-run duration compounds there too — though darwin-x64 release is the fast lane and the more likely victim is linux debug/ASAN.
Impact
Unrelated timeout noise on non-darwin lanes while iterating on this diagnostics branch. That's the exact opposite of what the branch wants: clean CI signal so a genuine TEST FAILED: at least one variant did not crash on the first GC stands out. Not a correctness/data-loss issue, and the author has stated the branch won't merge, so this is a nit.
How to fix
Either is fine (the second is closer to REVIEW.md's "shrink the workload" preference):
- Raise the timeout to match the new workload, e.g.
21 * 60_000or a conservative180_000, so the per-childspawnSynctimeout is the operative bound. - Gate the matrix to the target lane: wrap the variant loop (or the whole test) in
if (process.platform === 'darwin' && process.arch === 'x64')(orit.todoIf(!isMacOS || !isIntelMacOS || isWindows)), so non-darwin lanes keep roughly the original single-spawn cost.
…()'s frame un-inflated; deeper below-sp capture Not for merge. No-Verification-Needed: diagnostics-only branch
Not for merge. No-Verification-Needed: diagnostics-only branch
…erior + just-past-the-end) and print offsets Not for merge. No-Verification-Needed: diagnostics-only branch
… dladdr), show neighbours and pointee state Not for merge. No-Verification-Needed: diagnostics-only branch
| 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) { |
There was a problem hiding this comment.
🟡 🟡 /tmp/bun-napi-diag-request is a machine-global path with no per-run scoping, and runner.node.mjs now makes every darwin x64 shard run this wrapper — so concurrent shards on the same host race on it: shard B's else fs.rmSync(DIAG_REQUEST) (20 of 21 iterations) can remove shard A's sentinel mid-V1-run-0 (defeating the snapshot), or shard A's sentinel makes shard B's V2-V7 children dump [napi-diag] and be misclassified as CRASH@SNAPSHOT-GC. The sentinel also survives in /tmp if the wrapper is hard-killed during V1 run 0. Scope it per-run (e.g. /tmp/bun-napi-diag-request-${process.pid}, with the child reading the path from a BUN_NAPI_DIAG_REQUEST env var — an env var the crash path never touches doesn't perturb the repro), or add a process.on('exit') unlink. Nit: only this branch's binary has the access() check, and the branch is stated as diagnostics-only — but the cross-shard race directly undermines the diagnostics you're collecting.
Extended reasoning...
What the bug is
The diagnostics hook at ZigGlobalObject.cpp:3536-3539 is keyed on access("/tmp/bun-napi-diag-request", F_OK), and test_experimental_with_timeout.js:71-82 creates that file with a fixed machine-global path for the duration of V1 run 0's spawnSync. There is no PID, job, or shard scoping on either side.
Separately, this PR's own runner.node.mjs change prepends napi.test.ts four times to every darwin x64 shard's test list. On persistent hosts running multiple BuildKite agents (cornbread/bagel/pretzel per the PR description), concurrent shards of the same build share /tmp.
The specific code path
For 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 spawnSync and removes it after; iterations 1-20 each remove it before spawnSync. Inside the child, functionJsGc calls bunNapiDiagCaptureBelowSp and then bunNapiDiagMaybeDumpHeap iff the sentinel exists at the moment of the access() call.
Why existing code doesn't prevent it
The 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-spawnSync rmSync runs only if control returns from spawnSync; a SIGKILL of the wrapper process (runner's per-file timeout, job cancellation, or Ctrl-C locally) during V1 run 0 skips it. And the pre-spawnSync rmSync on iterations 1-20 is the very thing that races with a different wrapper's V1 run 0 on the same host.
Step-by-step proof (cross-shard race)
Within one BuildKite build on a darwin x64 host running two agents:
- Agent A (shard 3) starts
napi.test.tscopy Fix ?? operator #1 → wrapper reaches V1 run 0 →writeFileSync('/tmp/bun-napi-diag-request', '')→ entersspawnSync. The child begins startup + addon dlopen. - Agent B (shard 7) starts
napi.test.tscopy Fix ?? operator #1 a few hundred ms later → wrapper reaches V1 run 1 → executeselse fs.rmSync('/tmp/bun-napi-diag-request', { force: true }). - Agent A's child now reaches
functionJsGc→access("/tmp/bun-napi-diag-request", F_OK)returns-1→bunNapiDiagMaybeDumpHeapreturns immediately. No snapshot, no[napi-diag]output — even if this was the flaking run the whole branch exists to capture. - The wrapper reports
[napi-diag] no snapshot file for pid …and moves on.
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 gc(), that child emits [napi-diag] gc() returned to stderr and — if it happens to survive GC #1 — builds a full GCDebuggingSnapshot. The wrapper then classifies it as CRASH@SNAPSHOT-GC (line 101: se.includes('[napi-diag] gc() returned') && !se.includes('[napi-diag] wrote')), and the /tmp/bun-napi-diag-<pid>.json it wrote is never cleaned up (only V1 run 0's dump is renameSync'd).
Step-by-step proof (leaked on interrupt)
- The outer
it(...)'s 25 s timeout (already flagged as too short in the open comment on line 1400) or the runner's per-file timeout hard-kills the wrapper's process tree while V1 run 0 is inspawnSync— plausible precisely on the flaking run, where the child is building a fullGCDebuggingSnapshot. /tmp/bun-napi-diag-requestsurvives.- Every subsequent
global.gc()call in this branch's binary — e.g. later tests in the same shard that spawn children with--expose-gc, or the next of the 4× napi.test.ts runs' V2-V7 children before their pre-spawnrmSync— hits theaccess()check and dumps.
Impact
REVIEW.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 CRASH@SNAPSHOT-GC cells and orphaned /tmp/bun-napi-diag-<pid>.json files) — which wastes the CI iterations the author is explicitly waiting on.
On the refutation
The refutation is right that the blast radius is scoped to this branch's binary only — main-branch bun has no access() check, so a leaked sentinel is inert to other branches on the same host. It's also right that the leak-on-interrupt window is narrow (V1 run 0 is the first iteration) and self-healing (the next iteration's else fs.rmSync removes it). And "every push cancels the in-flight darwin-x64 jobs" does rule out cross-build races.
But it does not rule out cross-shard races within one build: the runner.node.mjs change deliberately runs the wrapper on every shard, and multi-agent darwin hosts run shards concurrently. The refutation's point (4) about Bun.gc() vs functionJsGc addresses in-process siblings inside the test-runner harness; it doesn't apply to the wrapper's own spawnSync children, which are launched with --expose-gc and call global.gc() → functionJsGc directly. So the cross-shard mechanism stands.
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 fix
Either 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 functionJsGc read the path from an env var (getenv("BUN_NAPI_DIAG_REQUEST")) that the wrapper sets on all 21 children — an env var the crash path never inspects doesn't perturb the byte-identical-argv goal. Or, minimally, keep the fixed path but register the process.on('exit') unlink so an interrupted wrapper doesn't leave it behind (this doesn't fix the cross-shard race).
|
|
||
| // 4'. Native frame layout above (and including) this frame, via the frame | ||
| // pointer chain, with return addresses symbolized, so a hit can be | ||
| // attributed to the frame that owns the slot. Then re-list the hits with | ||
| // the owning frame and a few neighbouring words. | ||
| { | ||
| struct NativeFrame { | ||
| uintptr_t* fp; | ||
| const char* name; | ||
| uintptr_t ret; | ||
| }; | ||
| WTF::Vector<NativeFrame, 128> chain; | ||
| uintptr_t* fp = static_cast<uintptr_t*>(__builtin_frame_address(0)); | ||
| while (fp && fp < origin && chain.size() < 120) { | ||
| uintptr_t ret = fp[1]; | ||
| Dl_info info; | ||
| const char* name = (dladdr(reinterpret_cast<void*>(ret), &info) && info.dli_sname) ? info.dli_sname : "?"; | ||
| chain.append({ fp, name, ret }); | ||
| uintptr_t* next = reinterpret_cast<uintptr_t*>(fp[0]); | ||
| if (next <= fp) break; | ||
| fp = next; |
There was a problem hiding this comment.
🔴 Commit 85de86a added section 4' (native fp-chain) to bunNapiDiagWhereAreTheRoots, which uses Dl_info/dladdr() at lines 3423-3424 with no platform guard — but <dlfcn.h> is included only under #if !OS(WINDOWS) (lines 241-243), so on Windows these are undeclared identifiers and the TU fails to compile (robobun confirms 🪟 x64/aarch64 build-cpp are still red on bb4430b). This is the same class as the earlier <unistd.h> break fixed in b8743ea; wrap the bunNapiDiagWhereAreTheRoots body (and bunNapiDiagCaptureBelowSp / the BUN_NAPI_DIAG_CAPTURE_CALLEE_SAVED inline-asm) in #if !OS(WINDOWS) — they're only reached from the POSIX branch of bunNapiDiagMaybeDumpHeap anyway.
Extended reasoning...
What the bug is
Commit b8743ea fixed the earlier Windows break by guarding #include <unistd.h> and giving bunNapiDiagMaybeDumpHeap an early return on Windows. But commit 85de86a ("attribute stack hits to a named native frame (fp chain + dladdr)") landed after that fix and added section 4' to bunNapiDiagWhereAreTheRoots, which at ZigGlobalObject.cpp:3423-3424 does:
Dl_info info;
const char* name = (dladdr(reinterpret_cast<void*>(ret), &info) && info.dli_sname) ? info.dli_sname : "?";Dl_info and dladdr are declared in <dlfcn.h>, which this file includes only under #if !OS(WINDOWS) at lines 241-243. root.h provides no shim. bunNapiDiagWhereAreTheRoots itself has no #if !OS(WINDOWS) around its body — it is defined at file scope on every platform.
Why the earlier fix doesn't cover it
b8743ea guarded only bunNapiDiagMaybeDumpHeap: on Windows that function returns immediately and never calls bunNapiDiagWhereAreTheRoots. But not being called doesn't help — C++ still compiles the callee's body as part of the translation unit. A static function with an undeclared identifier in its body is a compile error whether or not anything calls it. The same applies to bunNapiDiagCaptureBelowSp (though that one happens to use only WTF types), and to the BUN_NAPI_DIAG_CAPTURE_CALLEE_SAVED() macro's GCC-syntax asm volatile on the x86_64 branch if the Windows toolchain isn't clang-cl.
Step-by-step proof
- BuildKite spawns the
🪟 x64 - build-cppjob for commit bb4430b. - The Windows compiler compiles
src/jsc/bindings/ZigGlobalObject.cpp(as it does onmain— the file's own#if OS(WINDOWS)blocks at lines 232/287/598/1437 prove it's part of the Windows TU set). - The preprocessor reaches line 241:
#if !OS(WINDOWS)is false →<dlfcn.h>is not included. - Compilation reaches line 3423 inside
bunNapiDiagWhereAreTheRoots:Dl_info info;—Dl_infois undeclared → error: use of undeclared identifier 'Dl_info' (clang-cl) / C2065 (MSVC). - Line 3424:
dladdr(...)— also undeclared. - Compilation stops; the lane goes red.
The robobun comment on this PR independently confirms both 🪟 x64 - build-cpp and 🪟 aarch64 - build-cpp are "build failed" on bb4430b (the latest commit, which is the autofix formatting pass on top of 85de86a).
Impact
Every push to this branch reds every Windows CI lane. The branch is stated as diagnostics-only and won't merge, but the diagnostics loop is pushing repeatedly and reading CI results — a Windows lane that's red for a compile error is noise to filter on every build, and blocks every downstream Windows test lane so it can't say whether Windows exhibits the flake. The author already fixed the identical-class <unistd.h> finding in b8743ea when it was flagged, so they evidently want Windows green during the diagnostics loop; this is a fresh regression of that same class from the very next commit.
How to fix
Wrap the whole bunNapiDiagWhereAreTheRoots body in #if !OS(WINDOWS) (matching bunNapiDiagMaybeDumpHeap), and do the same for bunNapiDiagCaptureBelowSp and the x86_64 asm volatile arm of BUN_NAPI_DIAG_CAPTURE_CALLEE_SAVED for good measure:
__attribute__((no_sanitize("address"), noinline)) static void bunNapiDiagWhereAreTheRoots(JSC::VM& vm, JSC::CallFrame* callFrame)
{
#if !OS(WINDOWS)
// ... entire body ...
#endif
}(or #if OS(DARWIN), since the diagnostics target darwin x64 exclusively). The functions are only ever reached from the POSIX #else branch of bunNapiDiagMaybeDumpHeap, so no call-site change is needed.
No-Verification-Needed: diagnostics-only branch
…the PreciseAllocation half of oven-sh/WebKit#398) No-Verification-Needed: dependency pin bump; built in CI
…ve JSArray Not for merge. No-Verification-Needed: diagnostics-only branch
…) even when everything passes No-Verification-Needed: diagnostics-only branch
| bunNapiDiagPreGcLiveScan(JSC::getVM(global)); | ||
| bunNapiDiagCaptureBelowSp(JSC::getVM(global)); |
There was a problem hiding this comment.
🟡 c17e393 inserts bunNapiDiagPreGcLiveScan before bunNapiDiagCaptureBelowSp, so the 256KB region CaptureBelowSp then copies is the freshly-dead frames of PreGcLiveScan's own forEachLiveCell walk (whose lambda's HeapCell* cell parameter cycled through every live JSArray) — section 5's "pre-gc below-sp" hits are now the diagnostic's own residue, not what was there before gc() was entered. The same residue sits in the region Bun__gc's conservative scan reads next, so it may also seed a root on V1 run 0 (frame-layout dependent, unobserved yet — but check section 5 before trusting a V1-run-0-only CRASH@SNAPSHOT-GC/NO-CRASH; V1 runs 1-2 have no sentinel and stay uncontaminated). Swap the two calls so CaptureBelowSp runs first — that fixes section 5 outright; if V1 run 0 then diverges from runs 1-2 after this commit, the perturbation is self-evident from the matrix row.
Extended reasoning...
What the bug is
Commit c17e393 added bunNapiDiagPreGcLiveScan and inserted it at ZigGlobalObject.cpp:3630 so it runs before bunNapiDiagCaptureBelowSp at :3631 and before Bun__gc. Both are noinline siblings called from functionJsGc, so their frames occupy overlapping stack depth: CaptureBelowSp's &marker sits at roughly the same depth as PreGcLiveScan's frame top, and the 256KB it memcpy's below that point contains the freshly-dead frames of PreGcLiveScan's callees — HeapIterationScope ctor, MarkedSpace::forEachLiveCell, the block-walk loop, and the lambda body — every one of which held raw HeapCell* values (the lambda's cell parameter cycles through every live cell including the target JSArray, and arrays.add(cell) spills through HashSet::add).
Two consequences
(1) Section 5 is contaminated. bunNapiDiagWhereAreTheRoots section 5 ("pre-gc words BELOW gc()'s frame pointing at Array/Object/Napi* cells") reads bunNapiDiagBelowSpCopy and reports those addresses as if they were left there by whatever ran before gc() — the very hypothesis section 5 exists to test. Its own comment ("The dead region below gc()'s frame as it was BEFORE the collection") is now false: it's the dead region as it was after the diagnostic's own heap walk. Before c17e393, CaptureBelowSp was the only call inside the sentinel branch and captured the actual leftover state from module-load / addon-call frames.
(2) V1 run 0's crash bit may be perturbed. The same residue sits in [PreGcLiveScan-deepest, functionJsGc-sp), and Bun__gc's gatherFromCurrentThread scans [collector-sp, origin) — which spans that region. sanitizeStackForVM last ran at interpreter entry (before functionJsGc), not between the pre-scan and the collect, so it does not clear this residue. If any word survives un-overwritten by the collector's own frames, the conservative scan roots the JSArray, GC #1 does not finalize, and V1 run 0 is classified CRASH@SNAPSHOT-GC → allCrashedOnFirstGc = false → the test fails on a run where the underlying flake did not reproduce.
Step-by-step proof (section-5 pollution)
- On V1 run 0, the sentinel exists →
functionJsGcenters theaccess(...) == 0branch. - Line 3630:
bunNapiDiagPreGcLiveScan(vm)runs. ItsforEachLiveCelllambda receives every live cell asHeapCell* cell; for each JSArray it callsarrays.add(reinterpret_cast<uintptr_t>(cell)). The lambda invocation, itscellparameter, andHashSet::add's internals are all laid out belowbunNapiDiagPreGcLiveScan's own frame — i.e., belowfunctionJsGc's sp. - If the stack-scan loop finds a hit, its 8-arg
fprintf(stderr, "...PRE-GC stack...", ..., (void*)base, ...)passesbase(the JSArray address) as a stack vararg (SysV x64 passes only 6 integer args in registers), guaranteeing the address is written below the frame. PreGcLiveScanreturns. Its frame and callee frames are dead but not zeroed (no_sanitize("address")andnoinlinedon't zero dead frames).- Line 3631:
bunNapiDiagCaptureBelowSp(vm)runs. Its&markeris at ~the same depth as step 2's frame top; the loop copies[sp - 256KB, sp)intobunNapiDiagBelowSpCopy— including the JSArray addresses from step 2-3. - After
Bun__gcreturns without crashing,bunNapiDiagWhereAreTheRootssection 5 iteratesbunNapiDiagBelowSpCopy, matches those addresses viainteresting(), and prints them aspre-gc below-sp[...] -> Array @...— falsely attributing them to pre-gc()state.
Why existing code doesn't prevent it
CaptureBelowSp has no way to distinguish diagnostic-deposited words from genuine leftovers, and PreGcLiveScan doesn't wipe its callee stack before returning. The comment at test_experimental_with_timeout.js:78 ("Keyed on a file so the child's argv/env stay byte-identical") shows the author is deliberately avoiding perturbation — but this changes the child's stack contents right before both the capture and the conservative scan.
On the refutation of consequence (2)
One reviewer argued the GC-perturbation half is too speculative to file separately: everything in [collector-sp, functionJsGc-sp) is the collector's ~8-10-frame live call chain at capture time, so a specific cell address would have to land in an uninitialized hole of every frame; PreGcLiveScan's trailing unconditional fprintf+fflush is itself a deep libc/syscall chain that overwrites much of the shallower residue with non-cell values; and V1 runs 1-2 (no sentinel → no pre-scan) provide an immediate control — a run-0-only divergence is self-evident from the matrix row. All fair points, and consequence (2) is accordingly hedged as "may" / "frame-layout dependent" rather than a definite failure. But frames don't densely overwrite every slot (padding, unused callee-saved spill slots, uninitialized locals), the forEachLiveCell block-iteration chain plausibly goes deeper than fprintf→write, and — critically — the section-5 half is not speculative: CaptureBelowSp deterministically copies whatever PreGcLiveScan left. The two share one root cause and one fix, so they're filed together; the actionable takeaway for (2) is "look at the section-5 output and the V1 run-1/2 columns before trusting a V1-run-0-only non-crash@gc1", not "this will definitely fire".
How to fix
Swap the two calls so bunNapiDiagCaptureBelowSp runs first:
if (access("/tmp/bun-napi-diag-request", F_OK) == 0) {
bunNapiDiagCaptureBelowSp(JSC::getVM(global));
bunNapiDiagPreGcLiveScan(JSC::getVM(global));
}This restores section 5's meaning (CaptureBelowSp reads only [sp-256KB, sp) and vm.lastStackTop(), neither of which PreGcLiveScan needs pristine — PreGcLiveScan scans [sp, origin) upward). It does not address consequence (2) — PreGcLiveScan still runs immediately before Bun__gc — but that half is speculative and self-diagnosing via the matrix's run-1/2 control columns; if it turns out to matter, PreGcLiveScan could memset-zero [sp - kBelowSpBytes, sp) on return, or run from a fixed-depth trampoline whose depth exceeds the collector's.
Nit because this is diagnostics-only code that won't merge (per the author's own comments on this PR), section 5 is supplementary to the primary PRE-GC / section-4 / 4' output which is unaffected, and the author has already accepted similar methodology observations here as "will fix if another iteration is needed" — but it directly undermines section 5's signal on exactly the below-SP-garbage hypothesis being tested, so worth swapping before reading build #90799.
There was a problem hiding this comment.
True, and worth stating the direction of the bias: residue from the pre-GC walk can only add roots (make V1 run 0 survive), never remove them. The run in question showed the +24 word present pre-GC and the child then SIGABRT on GC #1 (finalized) — so if the walk contaminated anything, it did so against the result we observed. Section 5's below-sp numbers from that run are indeed not trustworthy; not relying on them. Leaving the order as is since this branch won't get further diagnostic rounds after the final repin.
🤖 Addressed by Claude Code
| 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 { |
There was a problem hiding this comment.
🟡 fs.renameSync(dump, dest) moves /tmp/bun-napi-diag-<pid>.json into the checkout with no try/catch — on any host where /tmp is a separate filesystem from the checkout (tmpfs /tmp is the systemd default on Linux), rename(2) fails with EXDEV and the uncaught throw aborts the wrapper at V1 run 0 before the matrix prints. Use fs.copyFileSync(dump, dest); fs.rmSync(dump, { force: true }), or wrap the if (wantDump) body in try/catch so a snapshot-collection failure degrades to the "no snapshot file" message. Nit: the darwin-x64 target hosts have /private/tmp on the same APFS volume as the checkout so it works there today, and the double-survival trigger hasn't been seen on Linux — but the test runs on every POSIX lane and this is the same "diagnostics hazard undermines the diagnostics" class as the sentinel-race comment above.
Extended reasoning...
What the bug is
At 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. rename(2) fails with EXDEV when source and destination are on different filesystems, and Node/Bun's fs.renameSync does not fall back to copy+unlink — it throws. On any host where /tmp is a separate filesystem from the checkout (tmpfs /tmp is the systemd default on many Linux distros; a macOS host with the BuildKite working directory on a separate APFS volume would also qualify), this throws an uncaught exception that terminates the wrapper at top level — with 20 of the 21 spawns still unrun and no matrix printed.
Why existing code doesn't prevent it
The neighbouring file operations are guarded: line 79 wraps the sentinel writeFileSync/rmSync in try { ... } catch {}, and line 81 does the same for the post-spawnSync rmSync. But the if (wantDump) block at lines 82-90 has no try/catch around renameSync or statSync. The else branch at line 88-89 ("no snapshot file for pid ...") already exists as a graceful degradation path, but an EXDEV throw never reaches it.
Step-by-step proof
- On a Linux host with tmpfs
/tmp(or any host wherestat -c %d /tmp≠stat -c %d <checkout>), V1 run 0's child hits the double-survival case:Bun__gc()returns without finalizing the wrapped object, thenbunNapiDiagMaybeDumpHeaprunsHeapSnapshotBuilder::buildSnapshot()whose GC also returns without finalizing, so control reachesfwriteat ZigGlobalObject.cpp:3554-3556 and writes/tmp/bun-napi-diag-<pid>.json. - Back in the wrapper,
fs.existsSync(dump)→true. fs.renameSync('/tmp/bun-napi-diag-<pid>.json', '<checkout>/test/napi/napi-app/napi-diag-<pid>.heapsnapshot')→ kernel returnsEXDEV, Bun throwsError: EXDEV: cross-device link not permitted, rename ....- The throw is at the script's top level (inside the
for (const v of variants)loop, not inside anytry) → the wrapper exits non-zero with the stack trace on stderr;rowsis never printed, V1 runs 1-2 and V2-V7 never run. - The outer test's
expect(bunStdout + ... + bunStderr).toContain("TEST PASSED: Process crashed as expected")at napi.test.ts:1393 fails, printing the EXDEV stack trace instead of the variant matrix. - The snapshot stays behind in
/tmpon the persistent host (nothing cleans it up on this path).
Impact and why it's a nit
The 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 todoIf(isWindows), so it runs on every POSIX lane including linux-x64, linux-aarch64, and their debug/ASAN variants.
That said, the practical exposure today is low, which is why this is a nit and not blocking:
- On the darwin-x64 target hosts (cornbread/bagel/pretzel per the PR description), macOS's
/tmpis a symlink to/private/tmpon the boot APFS Data volume, and BuildKite checkouts live on the same volume — sorenameSyncsucceeds. The author's resolved comment on the HashSet-zero-key thread ("it already produced full output there") confirms the snapshot→redact→upload path has run end-to-end on those hosts. - On Linux (where tmpfs
/tmpwould trigger EXDEV), the flake being investigated has only been observed on darwin-x64. If GC Fix ?? operator #1 crashes on V1 run 0 as it does in the non-flaking case,existsSync(dump)is false andrenameSyncis never reached. - If it did fire, the failure is self-diagnosing: the outer test's
toContainassertion printsbunStdout + bunStderr, which would include the EXDEV stack trace, so the cause would be immediately visible.
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 /tmp/bun-napi-diag-request cross-shard-race comment.
How to fix
Either replace the rename with a cross-device-safe copy:
fs.copyFileSync(dump, dest);
fs.rmSync(dump, { force: true });or wrap the whole if (wantDump) body in try { ... } catch (e) { console.log([napi-diag] snapshot move failed: ${e}); } so a failure degrades to a logged message and the matrix still prints.
…t exact one-past-the-end for non-butterfly precise cells) No-Verification-Needed: dependency pin bump; built in CI
2 participants
What does this PR do?
napi > napi_reference_unref is blocked from finalizers in experimental moduleshas been failing on the darwin 14 x64 lane (e.g. builds 90590, 90595, 90602, 90622, 90629, 90644 — hosts cornbread/bagel/pretzel), and every failure reads only:because the exit-code assertion runs before the ones on the captured stdout/stderr. It does not reproduce outside the agent job on those same hosts (0/300+ runs of the wrapper, the whole file, and the CI runner itself, idle and loaded, with the CI binary and CI-built addons), so the next CI failure needs to say what happened. This moves the output assertions ahead of the exit-code check — same assertions, reordered — so a failure shows whether the child never crashed ("ERROR: Did not crash"), crashed without the expected text, or the wrapper failed.
How did you verify your code works?
Assertion reorder only; the test still passes locally (
bun bd test test/napi/napi.test.ts -t "blocked from finalizers").