Conversation
…e panic hook rust_panic_hook printed its own copy of the crash report instead of calling crash_handler() like panic_impl, Bun__crashHandler and the signal handlers do. The copy had drifted: it printed "panic: <msg>" without the thread tag that crash_handler() prints (and that scripts/runner.node.mjs matches to label a crash), did not count the thread in PANICKING, did not restore stdio, did not reset the fault handlers, and did not honor the crash auto-reload. It also reported an empty message when the payload exceeded its 1024-byte buffer. The hook now extracts the payload, caps it at 1024 bytes and calls crash_handler(). Nothing exercised the hook: the crash_handler.panic() test helper calls panic_impl directly. Add rustPanic() (a real panic!, &'static str payload) and rustUnwrap() (a real Result::unwrap() on an Err, String payload) and run them through the same assertions as panic(): report header and "panic(main thread): <message>" line, SIGABRT, upload of a trace string whose panic message decodes to the message, and (debug Linux) a trace that contains the panic site and not the crash handler's own frames.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughWalkthroughThe crash handler now carries borrowed panic and error messages. The JavaScript test API can trigger Rust panics and unwrap failures. Crash-handler tests validate message decoding, truncation, stack traces, signals, Windows behavior, and report uploads. ChangesCrash handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fixed, reviewed, ready for a maintainer.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the code path that every production Rust panic!/unwrap()/assert! goes through, a human look at the re-entrancy and lifetime reasoning would still be worthwhile.
What was reviewed:
rust_panic_hooknow delegates tocrash_handler()exactly aspanic_implandBun__crashHandleralready do; thedetach_lifetimeon the payload borrow mirrors those sites andcrash_handler()is-> !.- Checked that
crash_handler()'s ownPANIC_STAGEstaging (0 / 1|2 / 3 / _) covers the re-entrancy case the deleted hook-local guard handled; std aborts panic-in-hook so the only reachable re-entry is a panic during an in-progress signal/OOM report, which stage 1+ handles. floor_char_boundary(1024)keeps the slice on a UTF-8 boundary;payload_as_str()covers both the&'static strandStringpayload shapes the old downcasts handled.- New tests hold
panic/rustPanic/rustUnwrapto identical assertions (header,panic(main thread):line, SIGABRT, uploaded trace-string round-trip);fixture-crash.jsnow exits 1 on unknown approach so a missing helper fails instead of passing silently.
Extended reasoning...
Overview
This PR replaces the hand-copied crash-report body inside rust_panic_hook (the std::panic::set_hook handler) with a call to the shared crash_handler(), deleting ~130 lines of drifted duplication. It adds two bun:internal-for-testing helpers (rustPanic, rustUnwrap) that trigger a real panic! and a real Result::unwrap() on Err, and extends run-crash-handler.test.ts to hold all three panic entry points to the same report format, terminal signal, upload, and trace-string-encoded message. It also fixes fixture-crash.js to exit 1 on an unknown approach and updates a stale comment in crash-report-command-char.test.ts.
Security risks
None identified. The crash handler is diagnostic-only; the change routes panic reporting through an existing, more-complete path rather than adding new surface. The unsafe { detach_lifetime(...) } on the panic payload is the established idiom already used at three other CrashReason::Panic construction sites (panic_impl, Bun__crashHandler), and crash_handler() is -> ! so the borrow cannot outlive the payload's owning frames.
Level of scrutiny
High. rust_panic_hook is what runs for every .unwrap(), assert!, and unreachable!() in the production binary (panic = "abort", so the hook is the whole story). A regression here would affect every crash report Bun users see and upload. The change is a simplification that unifies with the already-tested panic_impl path, but the re-entrancy argument (dropping the hook-local PANIC_STAGE guard in favour of crash_handler()'s own staging, relying on std's abort-on-panic-in-hook) and the payload-lifetime soundness in the hook context are subtle enough that a maintainer should confirm them.
Other factors
- The PR description is unusually thorough: it explains why each piece of the deleted code is subsumed (
PANICKINGcount,HAS_PRINTED_MESSAGE,reset_segfault_handler(), auto-reload), documents the fail-before behaviour against both main and old-hook-with-new-helpers, and includes a probe table for the 1024-byte cap across message shapes. - Test coverage is comprehensive: both payload types, all-platforms report-format check, POSIX signal check, debug-Linux symbolized-trace check, and a trace-string decoder that verifies bun.report will show the right message.
- No prior human reviews on the timeline; only a CodeRabbit rate-limit notice.
- I verified
crash_handler()atsrc/crash_handler/lib.rs:791is-> !and has fullPANIC_STAGEstaging, and thatpanic = "abort"is set for all profiles inCargo.toml.
Deferring because the change, while clean and well-tested, rewires a critical production code path and the re-entrancy/lifetime reasoning deserves a maintainer's sign-off.
…sage cap CrashReason is only ever formatted and passed down to crash_handler()/crash(), never stored, so give it a lifetime instead of laundering every message to &'static with detach_lifetime. This removes the four unsafe blocks that built a Panic/ZigError reason, including the one in the panic hook, and lets cold_handle_error_return_trace (which returns normally) borrow its name. The cap the hook applies to the payload is now a named constant and is covered: rustPanic(message) panics with the given message, and the tests check a 1024-byte message is reported whole, a longer one is cut after exactly 1024 bytes, and the cut never splits a multi-byte char, on both the stderr line and the message decoded from the trace string. The report-format tests also decode the trace string for all three panic approaches.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewires the process-wide Rust panic hook and changes CrashReason's lifetime across the crash-handling path — with re-entrancy reasoning that leans on std's panic-in-hook abort and crash_handler()'s own PANIC_STAGE guard — a human look is still worthwhile.
What was reviewed:
rust_panic_hookdelegating tocrash_handler()(-> !): withpanic = "abort"no unwind follows the hook, andcrash_handler()'s existingPANIC_STAGEmatch covers the re-entrant cases the deleted guard did.CrashReason<'a>: all fourdetach_lifetimesites removed safely; the Windowsclassify_exception_windowspath returnsCrashReason<'static>so no borrowed data crosses the VEH.floor_char_boundaryon the 1024-byte cap keeps the truncated slice valid UTF-8; boundary tests cover exact, over, and split-char cases.
Extended reasoning...
Overview
The PR collapses rust_panic_hook's ~130-line hand-copied crash report into a 10-line delegate to crash_handler(), matching how panic_impl, Bun__crashHandler, OOM, and the signal handlers already enter it. To make the borrowed panic payload flow through without unsafe, CrashReason gains a lifetime parameter and four detach_lifetime sites are deleted. Two test-only host functions (rustPanic, rustUnwrap) are added so the std panic hook is reachable from tests, and run-crash-handler.test.ts gains ~100 lines of new coverage: report-format parity across all three panic entry points, the 1024-byte cap with a char-boundary split, symbolized-trace assertions, terminal-signal table entries, and crash-reporter upload decoding.
Security risks
None identified. The crash handler runs after the process is already terminating; the change removes unsafe rather than adding it. The new rustPanic(message) helper is gated behind bun:internal-for-testing and only reachable in debug builds or CI. No new external inputs reach the crash path.
Level of scrutiny
High. The crash handler is the last-resort diagnostic path for every unwrap(), assert!, and unreachable!() in the binary, and it runs while the process is in an undefined state. A regression here silently loses crash reports or hangs during termination. Two aspects in particular deserve a maintainer's eye: (1) removing the hook's own re-entrancy guard is argued correct because std aborts a panic-inside-hook and crash_handler()'s PANIC_STAGE handles panic-during-signal-report, but that reasoning is subtle enough to warrant confirmation; (2) CrashReason<'a> is a type change that ripples through crash_handler, crash, TraceString, and the Windows exception classifier — the author cross-checked x86_64-pc-windows-msvc and aarch64-apple-darwin builds, but a human should confirm nothing else in the tree constructs or stores a CrashReason.
Other factors
- The comment-cop bot fired eight times on an earlier revision; commit
da30a394shortened the flagged comments and the current diff's comments are one-liners, so those look addressed. - PR #38617 adds an overlapping
rustPanichelper; the description says the two rebase in either order, but a maintainer should decide the merge sequence. - The PR description is unusually thorough (fail-before output, symbolized traces, cross-target
cargo check, a long-message probe table) and the bug-hunting pass found nothing, which raises confidence — but per the approval guidelines, critical-path changes should not be auto-approved regardless of test coverage.
|
Updated 11:49 AM PT - Aug 15th, 2026
✅ @robobun, your commit 5a6acacba9ffa10b4ef8fb99a94485f7720cba2e passed in 🧪 To try this PR locally: bunx bun-pr 38927That installs a local version of the PR into your bun-38927 --bun |
Problem
panic!in the binary (unwrap()onNone/Err, failedassert!,unreachable!()) is reported byrust_panic_hookinsrc/crash_handler/lib.rs, which printed its own hand-copied crash report instead of callingcrash_handler()likepanic_impl,Bun__crashHandler, OOM and the signal handlers do.crash_handler():panic: <msg>instead ofpanic(main thread): <msg>;scripts/runner.node.mjs:1657extracts crash labels with/panic\(.*\): (.*)/, so a real Rust panic in CI lost its message while apanic_implcrash kept it;PANICKINGcount (soGlobal::exit()on another thread could cut the report short),HAS_PRINTED_MESSAGE,Output::source::stdio::restore(), the native-plugin / unsupported-uv-function notes,reset_segfault_handler()and the crash auto-reload;BoundedArrayappends are all-or-nothing).crash_handler.panic()helper inbun:internal-for-testingcallspanic_impldirectly (src/runtime/api/crash_handler_jsc.rs), sorun-crash-handler.test.tsandcrash-report-command-char.test.tsonly coveredpanic_impl. (Item 12 of the review of Rewrite Bun in Rust #30412.)Fix
rust_panic_hooknow takes the payload string (PanicHookInfo::payload_as_str, the same&'static str/Stringpair the old code downcast by hand), cuts it atMAX_MESSAGE_BYTES(1024) on a char boundary, and callscrash_handler(CrashReason::Panic(msg), TraceSeed::BeginAddr(return_address())). The duplicated report code is deleted.CrashReasongets a lifetime (CrashReason<'a>). A reason is only formatted and passed down tocrash_handler()/crash(), never stored (every use of the type is inlib.rs; the only constructors outside it are the unit/usizevariants incrash_handler_jsc.rs), so the four sites that laundered a message to&'staticwithunsafe { detach_lifetime(..) }(panic_impl, the hook,Bun__crashHandler,cold_handle_error_return_trace) become plain constructors and the hook contains nounsafe. Same shape as the existingTraceSeed<'a>.panic_impldoes with its message, so a Rust panic gets thepanic_implreport, upload, multi-thread handling and SIGABRT by construction;crash_handler()was already printing a signal/OOM report, andcrash_handler()'sPANIC_STAGEbranches already handle that case;return_address()is read in the hook's own frame, so the trace is trimmed where the old code trimmed it (verified below: the trace starts at the hook's caller in std and reaches the panicking function);crash_handler.rustPanic()is a realpanic!(a literal, so a&'static strpayload;rustPanic(message)panics with the given message, aStringpayload) andcrash_handler.rustUnwrap()is a realResult::unwrap()on anErr(std formats it, also aStringpayload).panic()remains thepanic_implcase. No-argumentrustPanic()has the same name and message as the helper Restore BUN_DUMP_STATE_ON_CRASH: dump the DevServer graph when bun crashes #38617 adds for its own test, so the two PRs rebase onto each other in either order (Restore BUN_DUMP_STATE_ON_CRASH: dump the DevServer graph when bun crashes #38617 also adds a call to the old hook body, which becomes unnecessary once the hook delegates).test/cli/run/run-crash-handler.test.ts:panic,rustPanicandrustUnwrapheld to the same assertions: report header, exactpanic(main thread): <message>line, theoh no: Bun has crashedline, and the message inflated back out of the trace string (all platforms); SIGABRT (POSIX table, which now carries the expected message per row); upload of the report with the same decode (reporter tests); on debug Linux, a symbolized trace containingjs_rust_panic/js_rust_unwrapand no capture machinery.fixture-crash.jsexits 1 on an unknown approach; the reporter test asserts on the report before awaiting the upload so a missing report fails fast; the stale note incrash-report-command-char.test.tssayingrun-crash-handler.test.tsis skip-listed is removed (test: stop quarantining whole files for one broken case #33952 un-skipped it).panic: ...lacking the thread tag (output below), and the over-cap cases would see an empty message.bun bd test test/cli/run/run-crash-handler.test.ts test/cli/run/crash-report-command-char.test.tsis 32 pass, 9 skip (Windows/macOS/non-ASAN-only cases), 0 fail;cargo clippyonbun_crash_handlerandbun_runtime,cargo fmt --check,cargo checkof the crate forx86_64-pc-windows-msvcandaarch64-apple-darwin(the Windows classifier returnsCrashReason<'static>), andtest/internal/source-lints/are clean.encode_trace_stringdrops the message from the trace string when its compressed form does not fit the 1024-bytetrace_str_buf(about 800+ bytes of high-entropy text). That affectspanic_implidentically and is handed off separately.Background
crash_handler()prints a header (version, kernel, CPU, args), apanic(<thread>): <reason>line, then either a symbolized backtrace (debug builds) or a trace string: a URL under bun.report (orBUN_CRASH_REPORT_URL) encoding the version, platform, running command, frame addresses and the crash reason. ForCrashReason::Panicthe reason is the tag0followed by the zlib-compressed, base64 message, which bun.report inflates to show the message; the tests inflate it the same way. With reporting enabled the URL is also POSTed byreport(); the tests observe that with a localBun.serve.panic_implvs the std panic hook:panic_implis the explicit entry point (thepanic()test helper,CrashHandler__unsupportedUVFunction) and forwards tocrash_handler(). Rust'spanic!machinery instead calls whateverstd::panic::set_hookinstalled. Bun builds withpanic = "abort", so the hook is all that runs for a panic, and every.unwrap()/assert!in production goes through it.panic!("literal")andOption::unwrap()hand the hook a&'static str;panic!with arguments andResult::unwrap()(std formats"{msg}: {err:?}") hand it aString.payload_as_str()covers both; anything else only comes frompanic_any. The payload lives in the std panic frames below the hook, which is why borrowing it for the (never returning)crash_handler()call is fine.TraceSeed::BeginAddr, so that address has to be read in a frame that is still live when the walk runs (see the comment inpanic_impl). Reading it in the hook makes the trace start at the hook's caller.Report-format tests against the old hook body, helpers built in
(Run before the cap tests and the trace-string decode were added.)
Symbolized trace of a real unwrap panic with the fix (debug build)
Long-message probe behind the 1024-byte cap
The first three rows of this table are what the new cap tests pin (whole at 1024, cut after 1024, char boundary).