test: stop deliberate crash tests from uploading to CI's crash-report server - #34024
Conversation
run-crash-handler.test.ts and native-plugin.test.ts spawn processes that crash on purpose. They used env: bunEnv, which in CI inherits BUN_CRASH_REPORT_URL pointing at runner.node.mjs's remap server. The tests then exit 0, so the runner never drains /traces. Those five crash reports sit on the server until the next test that exits non-zero, which gets them attributed as "crash reported" (isAlwaysFailure) and loses its retries. In build 72106 a transient npm hang on one macOS 26 tart agent made dev-server-ssr-100.test.ts time out once; it inherited the five deliberate crashes (crashByPanic x2, outOfMemory, 0xDEADBEEF, native_plugin segfault at 0x0) and went straight to RED with no retry. Builds 72095 and 72085 show the same five reports pinned on node-http-backpressure-max.test.ts and fetch-leak.test.ts. Set BUN_CRASH_REPORT_URL="" (and BUN_ENABLE_CRASH_REPORTING=0 for the fall-through) on every spawn that crashes on purpose but does not itself assert on reporting. The "automatic crash reporter" tests already point at their own server and are unchanged.
WalkthroughChangesCrash test reporting isolation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:59 AM PT - Jul 12th, 2026
✅ @robobun, your commit 99e87749e2dfc1b06f1df05e695e687b16965f04 passed in 🧪 To try this PR locally: bunx bun-pr 34024That installs a local version of the PR into your bun-34024 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I grepped for other deliberate-crash spawns (fixture-crash.js callers, crash_handler.panic()) and found none outside this PR's two files and the tls sibling. Also confirmed is_reporting_enabled() returns false for an empty BUN_CRASH_REPORT_URL (src/crash_handler/lib.rs:2704-2705), so the empty-string override is effective.
Extended reasoning...
The env overrides in the diff are correct and mechanical; the only gap is the missed same-class sibling in tls-syscall-fault.test.ts, which the inline comment covers. I checked for further siblings and the empty-string semantics of the env var so a follow-up pass doesn't re-explore those.
…xture Same class as the other two files in this PR: the OOM fixture crashed on purpose with env: bunEnv, so on socket_fault_injection lanes it uploaded to CI's remap server and the report lingered for the next failing test. With reporting suppressed the OOM phrasing is now deterministic, so drop the both-phrasings matcher.
There was a problem hiding this comment.
LGTM — test-only env overrides to stop deliberate-crash fixtures from polluting CI's remap server. The inline nit on the OOM phrasing matcher only affects a release+non-ASAN+--socket-fault-injection=on build that no CI lane uses; fine to address in a follow-up or ignore.
Extended reasoning...
Overview
Test-only change touching three files: run-crash-handler.test.ts, native-plugin.test.ts, and tls-syscall-fault.test.ts. Each spawn that deliberately crashes now sets BUN_CRASH_REPORT_URL="" / BUN_ENABLE_CRASH_REPORTING=0 so the crash handler doesn't POST to CI's remap server, which was causing unrelated later-failing tests to inherit "crash reported" and lose their retries. No src/ changes.
Security risks
None. Pure test-environment configuration; no auth, crypto, or user-facing surface touched.
Level of scrutiny
Low. The mechanism is well-documented in the PR description with concrete build-number evidence, the fix is a two-env-var override applied identically at each site, and the author verified upload counts (5→0 and 1→0) against a simulated remap server. The two tests that do assert on upload behaviour (automatic crash reporter, raise ignoring panic handler) already point at their own local server and are correctly left untouched.
Other factors
My earlier review flagged the missed tls-syscall-fault.test.ts sibling; it was applied in 46639f6 and the thread is resolved. The remaining inline nit is about the single-phrasing OOM matcher being fragile on a release non-ASAN build with socket_fault_injection force-enabled — a config no CI job runs (the flag defaults to the asan setting per scripts/build/config.ts). On every lane that actually runs this test, SHOW_CRASH_TRACE is compile-time true and the Display phrasing is stable, so the assertion holds. Not blocking; the load-bearing env override is correct regardless.
SHOW_CRASH_TRACE is a compile-time constant (IS_DEBUG || IS_TEST || ENABLE_ASAN); on a release non-ASAN build with --socket-fault-injection=on the OOM phrasing differs. Match the shared substring like run-crash-handler.test.ts does.
There was a problem hiding this comment.
LGTM — both earlier review points are addressed in af952f5.
What was reviewed:
- Env overrides on the five deliberate-crash spawns:
BUN_CRASH_REPORT_URL: ""reachesis_reporting_enabled()as empty → disabled, andBUN_ENABLE_CRASH_REPORTING: "0"covers the fall-through branch. - The two tests that assert on upload behaviour ("automatic crash reporter", "raise ignoring panic handler") still point at their own local server — unchanged.
tls-syscall-fault.test.tsOOM matcher now uses the phrasing-agnostic.toLowerCase().includes("out of memory"), so it holds regardless ofSHOW_CRASH_TRACE.
Extended reasoning...
Overview
Test-only change touching three files: test/cli/run/run-crash-handler.test.ts, test/bundler/native-plugin.test.ts, and test/js/node/tls/tls-syscall-fault.test.ts. Each adds BUN_CRASH_REPORT_URL: "" / BUN_ENABLE_CRASH_REPORTING: "0" to subprocess spawns that crash on purpose, so their crash reports don't land on CI's remap server and get misattributed to the next unrelated failing test. The tls test also swaps a two-phrasing OOM matcher for a shared-substring match. No src/ changes.
Security risks
None. This only overrides two env vars on test-spawned child processes to suppress an outbound HTTP POST to a localhost CI server. No new network, filesystem, or auth surface.
Level of scrutiny
Low. The change is mechanical (env-var overrides on Bun.spawn/Bun.$ calls), test-infrastructure-only, and its worst-case failure mode is that a test still uploads a crash report (status quo) or a test assertion on stderr phrasing changes — both would show up immediately in CI, not ship to users.
Other factors
Two prior review rounds were fully addressed: the missed sibling in tls-syscall-fault.test.ts was added in 46639f6, and the phrasing-dependent matcher was made build-config-agnostic in af952f5. Both threads are resolved. The bug-hunting pass on the latest revision found nothing. No CODEOWNERS cover these paths. The PR description documents verification against a simulated remap server (5 uploads → 0) and bun bd test passing.
|
Another instance: build 72121 reports |
There was a problem hiding this comment.
LGTM — both earlier review notes (the tls-syscall-fault.test.ts sibling and the OOM phrasing matcher) are addressed; test-only env overrides on deliberate-crash spawns.
Checked: the three env: bunEnv → noReportEnv swaps in run-crash-handler.test.ts cover exactly the spawns that don't self-host BUN_CRASH_REPORT_URL; the two upload-asserting tests still point at their own server. The Bun.$ inline-env form in native-plugin.test.ts reaches the child as an empty string, which is_reporting_enabled() treats as disabled. The .toLowerCase().includes("out of memory") matcher covers both SHOW_CRASH_TRACE phrasings.
Extended reasoning...
Overview
Test-only change touching three files: test/cli/run/run-crash-handler.test.ts, test/bundler/native-plugin.test.ts, and test/js/node/tls/tls-syscall-fault.test.ts. Each file has spawns that deliberately crash a child process; the change sets BUN_CRASH_REPORT_URL="" and BUN_ENABLE_CRASH_REPORTING=0 on those spawns so they no longer POST to CI's per-job remap server. The TLS file also swaps a two-string OOM matcher for a phrasing-agnostic .toLowerCase().includes("out of memory"). No src/ changes.
Security risks
None. Env-var overrides on test subprocesses; no auth, crypto, network exposure, or user-input parsing is touched.
Level of scrutiny
Low. This is CI-hygiene: the mechanism (runner exports BUN_CRASH_REPORT_URL, drains /traces only on non-zero exit, deliberate crashes leak reports onto the next failing test) is well-documented in the PR description with three concrete build numbers showing the misattribution. The fix is the minimal, targeted one — clear the URL at each deliberate-crash spawn — and each existing assertion is preserved.
Other factors
I raised two points on earlier revisions and both were applied: the tls-syscall-fault.test.ts sibling now has the same env override (46639f6), and the OOM assertion was made SHOW_CRASH_TRACE-independent (af952f5). Both threads are resolved. The automatic crash reporter and raise ignoring panic handler tests correctly remain untouched since they self-host the report URL. The bug-hunting pass found nothing on the current revision. Latest commit is a no-op CI retrigger.
|
CI status on the three files in this diff: green on every executed lane across build 72166 and build 72190, including Build 72190 surfaced a real gap in this PR: the inline Remaining CI red on 72166/72190 is unrelated to this diff and already owned elsewhere:
|
…-report suppression
Build 72190 shows the inline shell-syntax override did not reach the
child on macOS release (the crash still printed the CI remap server
URL). Switch to .env({...bunEnv, ...}) which sets the spawn env
directly and matches this file's existing convention.
There was a problem hiding this comment.
LGTM — both earlier review rounds are addressed (tls-syscall-fault sibling now suppresses reporting, OOM matcher is phrasing-agnostic), and 99e8774 moves the native-plugin override to .env() so it actually reaches the child.
What was reviewed:
- Confirmed the three
noReportEnvswaps only touch spawns that don't assert on upload; the two tests that self-host a report server still set their ownBUN_CRASH_REPORT_URL. .env({...bunEnv, ...})on theBun.$call matches the file's existing convention and preservesBUN_TEST_TEMP_DIR; no cwd or other env was dropped vs. the original inline form.bunEnvis a plain object literal in harness, so the module-scope{ ...bunEnv, ... }spread is fine.
Extended reasoning...
Overview
Test-only env-var overrides on five subprocess spawns across three files, so deliberate-crash fixtures stop POSTing to CI's crash-remap server (which was pinning spurious "crash reported" errors on unrelated later tests). run-crash-handler.test.ts swaps three env: bunEnv for a shared noReportEnv; native-plugin.test.ts moves one Bun.$ spawn to .env({...bunEnv, BUN_CRASH_REPORT_URL: "", BUN_ENABLE_CRASH_REPORTING: "0"}); tls-syscall-fault.test.ts gets the same override plus a phrasing-agnostic OOM matcher. No src/ changes.
Security risks
None. Adds two env vars to child-process spawns inside the test suite; no new inputs, no external network, no auth/crypto surface.
Level of scrutiny
Low. Mechanical test-harness change with a clearly documented CI-attribution root cause. Worst-case regression is a test-file failure, which CI would catch immediately — and per the thread, the three touched files are green on builds 72166/72190 including the originally failing lane.
Other factors
This is my third pass on the PR. Both prior comments (missed tls-syscall-fault sibling; over-specific OOM phrasing after that fix) were applied in 46639f6 and af952f5 and are marked resolved. The final commit 99e8774 fixes a real gap surfaced by CI (inline shell VAR= prefix didn't reach the grandchild on macOS release) by switching to .env(), which is the pattern the rest of the file already uses. I verified bunEnv is a plain module-scope object so spreading it at module load in noReportEnv is safe, and that the two report-server tests in run-crash-handler.test.ts still point at their own local server. No outstanding reviewer comments.
|
Another instance: build 72253 flagged |
|
Another instance: build 72281 flagged |
2 participants
test/integration/next-pages/test/dev-server-ssr-100.test.tswas reported RED in build 72106 on:darwin: 26 aarch64with5 crashes reported during this test. The test itself is fine on main (last four completed main builds 72110/72020/71943/71860 have no mention of it). The RED was manufactured by CI's crash-report attribution.Cause
scripts/runner.node.mjsexportsBUN_CRASH_REPORT_URL=http://localhost:<remapPort>to every test so real crashes are captured. It only drains/traceswhen a test exits non-zero (the known caveat is documented in the runner at the drain site).run-crash-handler.test.tsspawns processes that crash on purpose withenv: bunEnv, which inherits that URL, andnative-plugin.test.ts's "prints name when plugin crashes" does the same viaBun.$. Both files pass (exit 0), so their five crash reports stay on the remap server:In build 72106 a transient npm-registry hang on one tart agent (
66790-tart-26) madedev-server-ssr-100'sbun iblock for 100 s and time out. That non-zero exit drained/traces, inherited the five deliberate crashes, and becameerror = "crash reported";isAlwaysFailure("crash reported")blocks retries, so one transient timeout became a hard RED.next-auth.test.tshit the same npm hang on the same agent minutes later, got normal retries, and passed on attempt #4.Same five reports pinned on unrelated tests in other recent PR builds:
test/js/node/http/node-http-backpressure-max.test.ts("5 crashes reported", identical list)test/js/web/fetch/fetch-leak.test.ts(segfault at0x0fromnative-plugin)Fix
Set
BUN_CRASH_REPORT_URL=""(andBUN_ENABLE_CRASH_REPORTING=0for the fall-through branch inis_reporting_enabled()) on every spawn that crashes on purpose but is not asserting on upload behaviour:run-crash-handler.test.ts: the threeenv: bunEnvspawns now use a sharednoReportEnv.native-plugin.test.ts: the "prints name when plugin crashes"Bun.$command sets the two vars inline.The "automatic crash reporter" and "raise ignoring panic handler" tests already point
BUN_CRASH_REPORT_URLat their own local server and are unchanged.Verification
Simulated CI's remap server and ran the test file against it:
bun bd test test/cli/run/run-crash-handler.test.tspasses.native-plugin.test.ts"prints name when plugin crashes" isskipIf(isASAN)so it is skipped under the debug build; a neighbouring case in the same file still loads, and the inline-env override was verified separately (Bun.$\VAR="" ...`reaches the child as an empty string, whichis_reporting_enabled()` treats as disabled).Test-only change; no
src/diff because the behaviour being fixed lives in the CI runner and the child's env, not in bun itself.no test proof · iteration 3 · Platform-specific test-only change; deferring to CI.