Conversation
|
Updated 2:42 AM PT - Aug 23rd, 2026
❌ @robobun, your commit b805e57 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 39635That installs a local version of the PR into your bun-39635 --bun |
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 28 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 2 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Status: ready for review. Rebased on main (7f5a7a3), head b805e57. Reproduced with the new Rebase notes. First rebase: the only conflict was in CI. The crash handler tests passed on every lane in builds 101202, 104002, 104055 and 104068. Each build had one red lane that this diff does not touch: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused diagnostics improvement that hooks std's alloc error path so failed Vec/Box allocations report as OOM with a real stack instead of a one-frame abort() called.
What was reviewed:
- All 7
CrashReason::OutOfMemorymatch sites updated for the new struct variant; trace-string encoding still9so bun.report is unchanged. - Hook body and the new
write!OOM message path checked for allocation —core::fmtinteger formatting is stack-buffered and re-entrance is guarded byPANIC_STAGE. set_alloc_error_hookis registered alongside the existing panic hook ininstall_hooks; the crate already requires nightly features.- New
allocErrortest hook and three test cases cover the debug symbolized trace, terminal signal, and uploaded trace-string suffix; existingoutOfMemoryassertions tightened.
Extended reasoning...
Overview
Registers std::alloc::set_alloc_error_hook next to the existing panic hook so that infallible Rust allocations that return null (Vec growth, Box::new) route through crash_handler(CrashReason::OutOfMemory { requested_bytes: Some(n) }) while the allocating frames are still on the stack, instead of falling through to std's default abort() and yielding a single-frame abort() called report. CrashReason::OutOfMemory becomes a struct variant carrying the requested size, printed to stderr via a small RequestedBytes Display helper. A new allocError internal-for-testing hook triggers a real Vec::with_capacity(1 << 62) failure (or handle_alloc_error directly under ASAN, where the allocator is libc's and ASAN aborts on oversized requests).
Security risks
None. This is process-termination diagnostics code that runs only after an allocation has already failed. No user input parsing, no auth, no network. The trace-string wire format is unchanged (still encodes OOM as 9), so bun.report/Sentry classification is unaffected.
Level of scrutiny
Moderate. The crash handler is process-global infrastructure, but the change is narrow and additive: one hook registration mirroring the panic hook right above it, one enum variant reshape, and a formatter helper. All match sites for the reshaped variant are in src/crash_handler/lib.rs (verified by grep — 7 sites, all covered in the diff). The code path only executes when the process is already dying, so blast radius is limited to the quality of the crash report itself. The alloc-free constraint on the hook is satisfied (core::fmt integer formatting uses a stack buffer) and re-entrance is backstopped by the existing PANIC_STAGE guard. Adding #[feature(alloc_error_hook)] is fine — the crate already depends on nightly (core_intrinsics).
Other factors
Testing is thorough: three new cases (debug symbolized trace contains js_alloc_error, terminal signal is SIGABRT with the size in the message, uploaded URL ends in 9/ack), plus the existing outOfMemory case now asserts the exact message and trace-string suffix. The PR description documents fail-before with the hook line removed and USE_SYSTEM_BUN=1. The automatic crash reporter describe was refactored from a for-loop to test.each with per-case expected message and URL suffix — a strict tightening of the existing assertions. No outstanding reviewer comments.
da27bca to
a2892c2
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it touches process-startup crash-handler infrastructure (install_hooks, an unstable alloc_error_hook feature gate, and the OOM message path inside crash_handler itself), a human sign-off would still be worthwhile.
What was reviewed:
- All
CrashReason::OutOfMemorymatch sites updated for the newrequested_bytesfield; trace-string encoding still'9'so bun.report is unchanged. rust_alloc_error_hookand the newwrite!in the OOM message path do not heap-allocate (stackfmt::Arguments+ integer formatting).js_alloc_errortest hook:1 << 62is belowisize::MAXsoLayout::from_size_aligncannot panic; ASAN branch mirrors whatVecdoes after a null return.- Comment-cop feedback resolved; the one CI failure (
s3.test.ts) is unrelated.
Extended reasoning...
Overview
Registers std::alloc::set_alloc_error_hook alongside the existing panic hook so failed infallible Rust allocations (Vec::with_capacity, Box::new) route into Bun's crash reporter as OutOfMemory with the requested size, instead of std's default memory allocation of N bytes failed + abort() (which produced a useless one-frame ?? trace on Linux and nothing on Windows). CrashReason::OutOfMemory gains an Option<usize> payload; all seven match sites in src/crash_handler/lib.rs are updated. A new allocError internal-for-testing hook fails a real 1 << 62 allocation, exercised by three new test cases in run-crash-handler.test.ts (symbolized trace reaches js_alloc_error, terminal signal is SIGABRT with the sized message, uploaded trace string ends 9/ack).
Security risks
None. The change is diagnostic-only: it changes what is printed and uploaded when the process is already about to die from an allocation failure. No new user-controlled input, no privilege changes, no new syscalls on the happy path.
Level of scrutiny
High. install_hooks runs at startup for every bun invocation, and the crash handler is the last-chance diagnostic path — a regression there loses crash telemetry silently. The change also enables an unstable Rust feature (alloc_error_hook). The repo already builds on nightly with core_intrinsics, so the added feature gate is precedented, but a maintainer should confirm they're comfortable taking another unstable-feature dependency in this crate.
Other factors
The PR description demonstrates a thorough understanding: fail-before verified with the hook line removed, USE_SYSTEM_BUN=1 fails as expected, the hook-vs-GlobalAlloc-null-check trade-off is explained (a null return is also how try_reserve reports failure). I confirmed the new write! on the OOM message path does not allocate (stack-only fmt::Arguments), so it is safe to call while out of memory. The refactored "automatic crash reporter" test tightens the existing assertions (now checks the trace-string reason suffix) rather than weakening them. All comment-cop feedback is resolved. Deferring only because crash-handler + startup-path changes warrant a human maintainer's eyes; I have no correctness concerns.
A failed infallible allocation (Vec growth, Box::new, ...) reaches std's alloc error handler, which prints "memory allocation of N bytes failed" and calls abort(). The SIGABRT handler reported that as "abort() called" with the trace seeded inside libc, where the frame pointer walk stops after one frame. On Windows std aborts with __fastfail and nothing was reported. Register a std alloc error hook next to the panic hook. It reports CrashReason::OutOfMemory and captures the trace while the allocating frames are still on the stack. The report now also prints the size of the request that failed. The trace string encoding is unchanged. The hook is the right layer: a null return from the global allocator is also how try_reserve and the other fallible allocation paths report failure, so GlobalAlloc itself cannot treat null as fatal. Adds a crash_handler.allocError test hook that fails a real Vec::with_capacity (or calls handle_alloc_error directly under ASAN, whose allocator aborts on oversized requests instead of returning null).
59de7eb to
b805e57
Compare
There was a problem hiding this comment.
LGTM — registers std::alloc::set_alloc_error_hook alongside the existing panic hook so failed infallible Rust allocations report as OOM with a usable stack trace instead of a one-frame abort() called.
What was reviewed:
- All 7
CrashReason::OutOfMemorymatch sites updated; trace-string encoding still writes9so bun.report is unchanged. - The hook and the new
write!message paths don't heap-allocate (stack-basedcore::fmtinteger formatting), safe to run under OOM. - Test refactor of
automatic crash reporterpreserves prior assertions and strengthens them (exact message, URL suffix); comment-cop feedback resolved.
Extended reasoning...
Overview
Adds #[feature(alloc_error_hook)] and registers rust_alloc_error_hook in install_hooks() next to the existing panic hook. CrashReason::OutOfMemory gains an optional requested_bytes field printed in the stderr message via a small RequestedBytes Display helper. A new allocError test hook in crash_handler_jsc.rs triggers a real Vec::with_capacity(1 << 62) (or handle_alloc_error directly under ASAN, whose allocator aborts on oversized requests). Three new test cases in run-crash-handler.test.ts plus a refactor of the automatic crash reporter block from a for-loop to test.each with stronger assertions (exact message text, uploaded URL ending in 9/ack).
Security risks
None. This is diagnostic/crash-reporting plumbing. No new user-facing input surface; the requested-bytes value comes from Layout::size(). The trace-string wire format for OOM is unchanged (writer.write_byte(b'9')), so bun.report needs no coordination.
Level of scrutiny
Moderate. The crash handler is process-global infrastructure, but the change is additive and mirrors the existing out_of_memory() → crash_handler(OutOfMemory, …) path. The main hazard for an alloc-error hook — allocating inside it — is avoided: the hook constructs an enum on the stack and calls the same non-allocating crash_handler the existing OOM path already uses; the new write! calls format a usize via core::fmt, which uses a stack buffer. Verified all CrashReason::OutOfMemory pattern sites in the repo are inside src/crash_handler/lib.rs and every one is updated in this diff.
Other factors
- Nightly feature use is fine — the crate already gates
#[feature(core_intrinsics)]. - The
automatic crash reporterrefactor keeps every prior assertion (exit code, server URL in stderr, no "panic" for OOM) and addstoEndWith(expectedUrlSuffix)in place of the booleansent; nothing weakened. 1 << 62is safe on all Bun targets (64-bitusize) and sits belowisize::MAX, soVec::with_capacityreaches the allocator rather than hitting the capacity-overflow panic.- CI: crash-handler tests passed on all lanes across builds 101202/104002/104055 per the status comment; the comment-cop lints on long comments were addressed and the threads are resolved.
- No prior review from this bot on this PR.
This branch has not been deployed
2 participants
Problem
Vecgrowth,Box::new) ends in std, which printsmemory allocation of N bytes failedand aborts. On Linux the report isabort() calledwith one??frame (Sentry BUN-2QD1 and siblings). On Windows std uses__fastfail, so nothing is reported.src/crash_handler/lib.rs:1583) seeds the trace inside libc'sabort/raise, which have no frame pointers, so the walk stops after one frame.Fix
install_hooksregisters astd::allocerror hook next to the panic hook. It callscrash_handlerwithCrashReason::OutOfMemorywhile the allocating frames are still on the stack.GlobalAlloc: a null return is also howtry_reserveand the other fallible paths (about 97 sites) report failure.OutOfMemorycarries the requested size, which the stderr message prints. The trace string still encodes every OOM as9, so bun.report is unchanged. The hook does not allocate.test/cli/run/run-crash-handler.test.ts, with a newcrash_handler.allocErrorhook that fails a realVec::with_capacity(1 << 62). Alsocrash-report-command-char.test.tsand the source lints.Background
bun_alloc::out_of_memory()serves code that holds anAllocError. A plainVecorBoxcallshandle_alloc_error, which runs std's alloc error hook and then aborts. Bun's Rust code and the rebuilt std keep frame pointers, so a walk from the hook reaches the allocating frame.Notes
Before, on a debug Linux build (the SIGABRT handler is not installed under ASAN, so this is std's output alone):
After, with
--debug-crash-handler-use-trace-string:The symbolized debug trace starts at
std::alloc::rust_oom::{closure#0}and reachesjs_alloc_errorsix frames down (__rust_end_short_backtrace,rust_oom,__rust_alloc_error_handler,handle_alloc_error::rt_error,handle_alloc_error). The number of std frames differs between debug and release, so the trace is not trimmed past them. Sentry groups on the whole stack, so different allocation sites still become different issues.Tests. Three new cases: the debug symbolized trace contains
js_alloc_errorand not the capture machinery, the process dies with SIGABRT and prints the OOM message with the size, and the uploaded trace string ends in9/ack(the existingoutOfMemorycase now checks that too). Fail-before: with only theset_alloc_error_hookline removed, all three fail with std'smemory allocation of ... failedoutput.USE_SYSTEM_BUN=1fails the two non-debug cases and skips the debug one.Test hook. Under ASAN the global allocator is libc's and ASAN aborts on an oversized request instead of returning null, so the hook calls
handle_alloc_errordirectly there. That is the callVecmakes once the allocator has returned null.1 << 62as the size:mi_mallocof that size returns null on the release mimalloc objects inbuild/release(checked with a small C program linked againststatic.c.o). A merely large size is not reliable,mi_malloc(1 << 40)succeeded in the CI container because of overcommit.isize::MAXalso returns null but is the edge of whatLayoutaccepts.Sentry samples from the
??group (events 5b525e08, e419f388, 9873e4b3):1.4.0-canary, Linux,Abort/abort() called, one frame at a libc offset (0x9781b,0x969bb,0x8aeeb). The group also holds older Zig era events and single frame Windows segfaults, which this PR does not address.Not changed here: the SIGABRT walk itself. A real
abort()from C++ or an addon still yields one libc frame on Linux. Recovering the caller needs a stack scan from the signal context's sp, which is a separate change. Also not changed: on Linux,StackLine::from_addressencodes an address inside a shared library as an offset with no object name, which is why such a frame shows as??.no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/run-crash-handler.test.ts