test(napi): judge the experimental-finalizer wrapper on 'close', match markers on accumulated stderr - #37227
test(napi): judge the experimental-finalizer wrapper on 'close', match markers on accumulated stderr#37227dylan-conway wants to merge 2 commits into
Conversation
…lose', match markers on accumulated stderr The wrapper judged the child in its 'exit' handler and looked for "FATAL ERROR" / "panic(main thread)" in each stderr chunk separately. 'exit' can fire before the stderr pipe has been drained, so on a busy host the tail of the crash report (including the panic line) was sometimes not seen yet, and a marker split across two chunks was never seen; either way a child that crashed exactly as intended was reported as "terminated with code null" and the test failed. Wait for 'close' and match on the accumulated stderr. Also assert on the captured output before the exit code so a failure shows what the child printed. No-Verification-Needed: test-only change
WalkthroughThe experimental N-API crash tests now detect markers across stderr chunks, wait for stdio to drain before evaluating results, and assert combined output before separate diagnostics and the exit code. ChangesExperimental N-API crash tests
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only flake fix.
What was reviewed:
'exit'→'close'in the wrapper:'close'receives the same(code, signal)args and fires after stdio drains; SIGKILL from the early-kill and fallback-timeout paths still produces'close'.- Per-chunk → accumulated
stderr.includes(...): the flags are set-only, so rescanning the growing buffer is strictly more correct and catches split markers. - Assertion reorder in
napi.test.ts: no assertion removed or weakened; the added---- stderr ----separator can't create a falsetoContainmatch.
Extended reasoning...
Overview
Two test files touched, no production code. test/napi/napi-app/test_experimental_with_timeout.js switches the verdict handler from proc.on('exit') to proc.on('close') and checks the accumulated stderr buffer instead of the individual chunk when looking for the FATAL ERROR / panic(main thread) markers. test/napi/napi.test.ts reorders the assertions in the corresponding outer test so the captured stdout+stderr is asserted before the exit code, with a labelled separator so a future failure prints what the child actually wrote.
Security risks
None. Test-only changes to a fixture wrapper and assertion order; no runtime, no user-facing behavior, no external input handling.
Level of scrutiny
Low. This is a well-understood Node child_process idiom fix ('exit' can fire before stdio pipes drain; 'close' waits for both), applied to a test wrapper. The accumulated-buffer change is the standard fix for markers split across 'data' chunks — the buffer was already being accumulated into stderr, just not consulted. Both changes strictly tighten what the wrapper observes.
Other factors
I checked that 'close' still fires on both termination paths in this wrapper (the early SIGKILL after seeing both markers, and the 5s fallback SIGKILL): SIGKILL closes the child's pipes, so 'close' fires with signal === 'SIGKILL' and the existing timeout-branch logic still applies. The sawFatalError/sawPanic flags are only ever set to true, so rescanning the accumulated buffer each chunk is safe. In napi.test.ts the assertion set is identical to before — one toContain moved up and given a richer subject string, and expect(bunExitCode).toBe(0) moved to the end per the repo's "assert output before exit code" convention. Nothing was deleted or loosened. The PR description's diagnosis (darwin-x64 CI capture ending mid-report at Args: ") is consistent with the 'exit'-before-drain race the change addresses.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![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`:
- Line 1383: Update the assertion in the relevant NAPI test to validate that
“Loading experimental module” appears in bunStdout specifically, rather than in
the combined bunStdout + bunStderr output. Keep the existing fixture-output
expectation unchanged otherwise.
🪄 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: b26c2a40-1cd8-4581-b249-da294f686b49
📒 Files selected for processing (2)
test/napi/napi-app/test_experimental_with_timeout.jstest/napi/napi.test.ts
| expect(bunExitCode).toBe(0); | ||
| // Combined output first, so a failure shows what the wrapper and child wrote. | ||
| expect(bunStdout + "\n---- stderr ----\n" + bunStderr).toContain("TEST PASSED: Process crashed as expected"); | ||
| expect(bunStdout + bunStderr).toContain("Loading experimental module"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the fixture output on stdout.
The panic report can echo the full -e source into bunStderr, as documented on Lines [1387]-[1390]. That source contains "Loading experimental module" on Line [10]. Therefore, this assertion can pass even when the child did not write the message to stdout.
Proposed fix
- expect(bunStdout + bunStderr).toContain("Loading experimental module");
+ expect(bunStdout).toContain("Loading experimental module");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(bunStdout + bunStderr).toContain("Loading experimental module"); | |
| expect(bunStdout).toContain("Loading experimental module"); |
🤖 Prompt for 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.
In `@test/napi/napi.test.ts` at line 1383, Update the assertion in the relevant
NAPI test to validate that “Loading experimental module” appears in bunStdout
specifically, rather than in the combined bunStdout + bunStderr output. Keep the
existing fixture-output expectation unchanged otherwise.
There was a problem hiding this comment.
LGTM — test-only de-flake with a sound diagnosis.
What was reviewed:
'exit'→'close'so the verdict waits for stdio to drain; confirmed'close'receives the same(code, signal)args and still fires after the SIGKILL paths.- Matching markers against accumulated
stderrinstead of the per-chunk buffer — strictly safer, handles a marker split across chunks. - Assertion reorder in
napi.test.tsfollows the repo convention (output before exit code) and only improves failure diagnostics; no assertion was weakened or removed.
Extended reasoning...
Overview
Two test files touched, no production code. test_experimental_with_timeout.js is a wrapper that spawns a child expected to abort and judges pass/fail from the child's stderr. The PR (1) switches the verdict from proc.on('exit') to proc.on('close') so stdio is fully drained before deciding, and (2) tests the accumulated stderr string instead of each individual chunk so a marker split across two 'data' events is still matched. In napi.test.ts the outer test's assertions are reordered so the combined stdout/stderr is asserted before the exit code, improving failure output.
Security risks
None. Test-harness-only change; no user-facing or native code paths touched.
Level of scrutiny
Low. This is a targeted flake fix in a test wrapper script. The 'exit'-fires-before-stdio-drains race is a well-known Node child_process pitfall, and 'close' is the documented event that guarantees both process termination and stdio closure. The 'close' handler receives the same (code, signal) arguments as 'exit', so the existing branch logic (signal === 'SIGKILL' && !sawPanic, exit-code inversion) is unchanged in meaning. The early-SIGKILL path (kill once both markers are seen) and the fallback-timeout SIGKILL both still lead to 'close' firing, so there's no new hang risk.
Other factors
The accumulated-stderr check reuses the stderr variable that was already being built — the old code just wasn't consulting it. The napi.test.ts reorder keeps every existing assertion and adds a labelled stderr section to the first one for context; nothing is loosened. The PR description cites a concrete CI capture (darwin-x64 build 90725) matching the diagnosed race. The duplicate-PR bot flagged #37166/#37214 as overlapping; that's a merge-order decision for maintainers and doesn't affect the correctness of this diff.
1 participant
What does this PR do?
Fixes a low-rate flake in
test/napi/napi.test.ts › napi_reference_unref is blocked from finalizers in experimental modulesthat is independent of the GC-side cause of that test's recent failures (that one is addressed by oven-sh/WebKit#398 and needs no test change).test_experimental_with_timeout.jsspawns a child that is expected to abort, and decides pass/fail from what the child printed. It did that inproc.on('exit')and looked forFATAL ERROR/panic(main thread)in each stderr chunk.'exit'may fire before the stdio pipes are drained, so on a busy host the wrapper sometimes judged before the tail of the crash report had arrived; and a marker split across two chunks was never matched. In both cases a child that crashed exactly as intended was reported asTEST PASSED: Process terminated with code null signal SIGABRTinstead of…crashed as expected, and the outer assertion failed. Observed on darwin-x64 (build 90725): the captured stderr ended mid-report atArgs: "withFATAL ERRORpresent and the panic line missing.Changes: wait for
'close'(stdio drained) instead of'exit'; test the accumulated stderr rather than the chunk; and innapi.test.tsassert on the captured stdout+stderr before the exit code so any future failure prints what the child wrote instead ofExpected: 0 Received: 1.How did you verify your code works?
bun bd test napi/napi.test.ts -t napi_reference_unref→ 2 pass. The race itself isn't reproducible on demand; the diagnosis is from the CI capture described above.