Conversation
|
PR changed again? Review this PR in Change Stack to compare snapshots and stay oriented. Warning Review limit reached
More reviews will be available in 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a Rust allocation error hook that catches infallible allocation failures and routes them through Bun's crash-handler system. It enables the nightly ChangesAllocation Error Hook and Testing
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the capacity-hint nit. The change looks correct and well-tested, but since it installs a process-wide alloc_error_hook (nightly API) that alters how every infallible allocation failure terminates, I'd like a human to sign off on the approach.
Extended reasoning...
Overview
This PR registers std::alloc::set_alloc_error_hook in crash_handler::install_hooks() so that ordinary Vec/Box/String allocation failures route into the existing CrashReason::OutOfMemory crash-report path instead of std's default "memory allocation of N bytes failed" + abort(). It adds the #![feature(alloc_error_hook)] nightly gate, a 4-line hook that calls out_of_memory(), a doc-comment update in handle_oom.rs, a new test-only infallibleOutOfMemory binding in crash_handler_jsc.rs (plus the ENTRIES.len() fix from my earlier nit), and wires it into the existing run-crash-handler.test.ts matrix with appropriate ASAN_OPTIONS handling. The Cargo.lock hunk is an unrelated lockfile resync.
Security risks
None apparent. No new attack surface — the hook only fires on allocation failure and routes into pre-existing crash-report machinery. The new JS binding is gated behind bun:internal-for-testing (debug/CI only). Crash report uploads already existed for the explicit OOM path; this just makes the infallible path produce them too.
Level of scrutiny
Moderate-to-high. While the diff is small and the implementation looks correct, it changes process-global behavior: every infallible allocation failure across the entire binary now flows through out_of_memory() → crash_handler() instead of std's default abort. The hook runs under genuine memory pressure, so the no-allocation invariant of the downstream crash path matters (the description argues it holds via stack buffers + raw stderr + the PANIC_STAGE re-entry guard, which seems sound). It also adds a new nightly feature dependency. The PR description explicitly weighs and rejects alternatives (-Zoom=panic, SIGABRT handler), which is exactly the kind of design decision a human maintainer should ratify.
Other factors
The earlier nit (capacity hint vs. ENTRIES.len()) was addressed in 87f67ac. The bug-hunting pass found nothing on the latest revision. Test coverage is solid: the new case exercises the real allocator → handle_alloc_error → hook chain end-to-end and asserts the OOM message, report URL, and POST receipt. Given the global scope of the behavior change and the nightly-feature addition, I'm deferring rather than auto-approving.
There was a problem hiding this comment.
Thanks for addressing the capacity-hint nit. I didn't find any further issues, but since this registers a process-global alloc_error_hook (new nightly feature gate) that changes how every infallible allocation failure terminates, it's worth a human sign-off on the approach.
Extended reasoning...
Overview
This PR routes infallible allocation failures (Vec/Box/String growth → handle_alloc_error) into Bun's crash reporter by registering std::alloc::set_alloc_error_hook alongside the existing panic hook in install_hooks(). It adds the #![feature(alloc_error_hook)] nightly gate to src/crash_handler/lib.rs, a new alloc_error_hook fn that diverges into out_of_memory(), an updated doc comment in handle_oom.rs, a new infallibleOutOfMemory test-only JS binding (crash_handler_jsc.rs / internal-for-testing.ts), and a new case in the existing crash-reporter test matrix. The Cargo.lock hunk is an unrelated lockfile sync for bstr. My earlier nit (the create_empty_object capacity hint drifting to 8 vs 9 entries) was addressed in 87f67ac by hoisting ENTRIES and passing ENTRIES.len().
Security risks
None apparent. The hook only changes the termination path on allocation failure (already a fatal condition); it doesn't expose new inputs, parse untrusted data, or affect auth/crypto/permissions. The crash report path itself is pre-existing and unchanged.
Level of scrutiny
Moderate-to-high. While the diff is small and well-reasoned, it changes process-global behavior for every Bun build: a new nightly feature gate is enabled crate-wide, and a global hook is installed that runs under memory pressure on the abort path. The PR description argues the hook is allocation-free and that PANIC_STAGE guards re-entry, which looks correct from the code, but this is exactly the kind of subtle, runs-once-per-process, hard-to-debug-if-wrong infrastructure where a maintainer should confirm the design choice (vs. -Zoom=panic or a SIGABRT handler, both of which the description considered and rejected).
Other factors
The change is well-tested: a real Vec::with_capacity(1<<61) exercises the genuine RawVec → handle_alloc_error path end-to-end, with ASAN accommodations. The test-only binding additions follow the established pattern in crash_handler_jsc.rs. No outstanding reviewer comments remain — my prior nit is resolved. Given the global/infrastructural nature of the change rather than its size, I'm deferring rather than auto-approving.
|
CI status summary — the diff is green; the remaining red is macOS agent capacity, unrelated to this change. Two full builds (59786 and its re-roll 59893): every job that obtained an agent passed — 280+ jobs each, all build lanes, all Linux/Windows test lanes. The only failures ever recorded:
Verification of the change itself was done locally on the ASAN debug build: without I've used my one CI re-roll; not pushing further retriggers. Ready for maintainer review/merge. Update: the re-rolled build's darwin-14-x64 shard landed on the same broken runner ( Final state of build 59893: darwin-26-aarch64 passed once it drew a healthy agent. darwin-14-x64 failed only on Build 60935 (post-rebase): the debian-13 x64-asan shard fails only in Build 60935 final: the |
Only explicit Result<_, AllocError> paths reached the crash handler's CrashReason::OutOfMemory report via bun_core::handle_oom. Ordinary Vec/Box/String allocation failure went to the global allocator's handle_alloc_error, whose std default hook prints "memory allocation of N bytes failed" and aborts with SIGABRT: no OOM message, no trace string, no crash report. Install a std::alloc::set_alloc_error_hook in install_hooks() that routes handle_alloc_error into the existing out_of_memory() path, so every OOM produces the same "Bun ran out of memory" report. The hook body is a single diverging call and performs no allocation; a second OOM inside the report path hits the PANIC_STAGE re-entry guard. Adds a crash_handler.infallibleOutOfMemory testing binding that fails a real Vec growth through the global allocator and wires it into the automatic crash reporter tests. Cargo.lock picks up the bstr dependency bun_bin gained in #31668.
The inline-capacity hint was a literal 8, which desynced when infallibleOutOfMemory became the 9th entry. Derive it from the entry list so it cannot drift.
7317ea0 to
3a4863d
Compare
![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/cli/run/fixture-crash.js`:
- Around line 14-15: The invalid-approach branch in fixture-crash.js currently
only prints the usage string and returns success; update that branch so it fails
loudly by calling process.exit with a non-zero code after the console.error call
(i.e., when the approach is unrecognized in the invalid-approach / default
branch that prints "usage: bun fixture-crash.js ..."). Ensure the exit code is
non-zero (e.g., 1) so misconfigured tests do not pass silently.
🪄 Autofix (Beta)
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: 20ae8ee1-49ef-4d74-a2e7-e783430e4d1e
📒 Files selected for processing (6)
src/crash_handler/handle_oom.rssrc/crash_handler/lib.rssrc/js/internal-for-testing.tssrc/runtime/api/crash_handler_jsc.rstest/cli/run/fixture-crash.jstest/cli/run/run-crash-handler.test.ts
An unrecognized approach printed usage and exited 0, which could mask a miswired test as a successful run.
|
Superseded by #38888, which installs the same alloc error hook and also removes the dead OOM wrapping around the collection APIs that the hook makes unnecessary (the follow-on cleanup requested by the maintainers). Closing this one in its favor. |
1 participant
Problem
Out-of-memory crashes from ordinary (infallible) allocations are invisible: zero
OutOfMemory-typed crash reports come from Rust builds, while Zig stable builds report thousands. In Zig, every failed allocation returnederror.OutOfMemoryand bubbled tobun.outOfMemory()→CrashReason::OutOfMemory→ auto-report. In the Rust port, only explicitResult<_, AllocError>paths go throughbun_core::handle_oom; plainVec/Box/Stringgrowth failure goes to the global allocator'shandle_alloc_error, whose std default hook prints:and calls
abort()— SIGABRT (exit 134), no "Bun ran out of memory" message, no trace string, no report URL, no upload. Since the vast majority of allocations are the infallible kind, effectively all real-world OOMs vanish from telemetry. The POSIX crash handler only registersSIGSEGV/SIGILL/SIGBUS/SIGFPE, so nothing downstream catches it either (and by SIGABRT time the OOM context would be gone anyway).Fix
install_hooks()— wherestd::panic::set_hookis already wired — now also registersstd::alloc::set_alloc_error_hook(nightly API; the workspace is pinned to nightly). The hook routes into the existingout_of_memory()→crash_handler(CrashReason::OutOfMemory, …)path, so infallible allocation failure produces the same report as the explicitAllocErrorpath and Zig:The hook runs under memory pressure, so it must not allocate: its body is a single diverging call, and the crash handler itself writes through stack buffers and raw stderr. A second OOM inside the report path hits the existing
PANIC_STAGEre-entry guard and aborts instead of looping. This also covers the in-tree directhandle_alloc_errorcallers (VirtualMachine.rs,analyze_transpiled_module.rs).(
-Zoom=panicwas considered and rejected: it would type these asPanicinstead ofOutOfMemory; registering SIGABRT was rejected because the OOM context is gone by signal time.)Test
New
crash_handler.infallibleOutOfMemorybinding inbun:internal-for-testingperforms a realVec::with_capacity(1 << 61)— a reservation no 64-bit address space can satisfy — so the failure travels the genuine route: global allocator returns null →RawVec→handle_alloc_error→ hook → report. Wired into the existingautomatic crash reportermatrix intest/cli/run/run-crash-handler.test.ts, which asserts the OOM message, the report URL, and the POST arriving at a self-hosted report server. Under ASAN the child getsASAN_OPTIONS=allocator_may_return_null=1so the interceptor returns null instead of hard-erroring before Rust sees the failure.Verified on the ASAN debug build: test fails without the src changes (fixture reports the approach as unknown, process exits 0 with no report) and passes with them; the other crash-reporter approaches (
panic,segfault,outOfMemory) are unaffected.Cargo.lock: syncs thebstrdependencybun_bingained in #31668 (lockfile wasn't regenerated there; any cargo invocation reproduces this hunk).