install: make the debug-build stack dump on package install failure opt-in - #37335
install: make the debug-build stack dump on package install failure opt-in#37335robobun wants to merge 5 commits into
Conversation
…pt-in The install failure branch in PackageInstaller dumped the captured failure trace through bun_crash_handler::dump_stack_trace in debug builds. On Linux that spawns llvm-symbolizer over the debug binary, which takes about 5 seconds per failed package and writes the frames to stdout, in the middle of the install summary; without a symbolizer on PATH (and on macOS) the frames go to stderr instead. Every expected install failure paid for this, and bun-link.test.ts timed out on debug builds because of it. Gate the dump behind the hidden PackageInstaller debug scope, next to the "Installing x@y" log it complements, so BUN_DEBUG_PackageInstaller=1 still prints it. Tighten the test's stderr assertion so the stderr variants of the dump are caught as well.
|
Status: reproduced with |
WalkthroughThe installer emits failure stack traces only when ChangesInstall failure trace handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/install/bun-link.test.ts`:
- Around line 461-462: Update the comment near the debug-build stack-trace
assertion to state that bun_crash_handler::dump_stack_trace writes stack frames
to stderr on every platform, removing the incorrect Linux stdout distinction
while preserving the opt-in behavior description.
- Around line 463-466: Extend the install error test around the existing err4
assertion to also run a failing install with BUN_DEBUG_PackageInstaller=1 and
verify stderr retains stack frames. Update the adjacent comment to state that
dump_stack_trace writes to stderr on all platforms, while preserving the current
hidden-state assertion.
🪄 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: 9b9fba4d-3fc7-4962-bd70-5951994541d7
📒 Files selected for processing (2)
src/install/PackageInstaller.rstest/cli/install/bun-link.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — debug-build-only diagnostic gating; release binaries are unaffected.
What was reviewed:
- Confirmed
PackageInstalleris an existingdeclare_scope!(…, hidden)at src/install/PackageInstaller.rs:41 and.is_visible()matches the idiom used elsewhere (bundle_v2.rs, sys/lib.rs). #[cfg(bun_debug)]is retained, so release builds still compile the block out entirely.- Test tightening from
toContain→ exact-linetoEqualcatches the stderr variants (macOS, no-symbolizer) that the old assertion missed; ruled out that the exact match could flake on extra warn/resolve lines by checking sibling stderr assertions in the same file use the same shape.
Extended reasoning...
Overview
Two-line functional change in src/install/PackageInstaller.rs: a #[cfg(bun_debug)] block that unconditionally called bun_crash_handler::dump_stack_trace on every package install failure now checks PackageInstaller.is_visible() first, so the dump only fires when BUN_DEBUG_PackageInstaller=1 is set. The PackageInstaller scope is already declared at line 41 (declare_scope!(PackageInstaller, hidden)) and used at lines 1130/1420 for per-package logging, so this reuses the existing opt-in knob rather than inventing a new one. The accompanying test change in bun-link.test.ts tightens a toContain to an exact-line match so the stderr-side variants of the dump (macOS, missing symbolizer) also fail the test.
Security risks
None. The change only affects whether a debug diagnostic prints in a cfg(bun_debug) build. No user input flows through the changed lines; no allocation, syscall, or error-path change; release binaries compile the whole block out unchanged.
Level of scrutiny
Low. This is a debug-build-only gating change to developer diagnostics — it cannot affect production behavior. The .is_visible() idiom is used identically at four other sites in the tree (bundle_v2.rs:1981, sys/lib.rs:5043, jsc/lib.rs:1452/1461), so there's no novel pattern to audit. The one-line comment ("Opt-in: symbolizing the debug binary costs seconds per failed package") is exactly the kind of "why not what" note the repo guidance asks for.
Other factors
The PR description documents that the fix was verified both ways (bun bd test now passes at ~530ms vs a 5s timeout before; BUN_DEBUG_PackageInstaller=1 still produces the trace), and that USE_SYSTEM_BUN=1 passes before and after — which is expected and correct here since release builds never had the dump. The test tightening is a strict improvement per the review guidance ("assert exact values on normalized output … toBe over toContain"). No prior review comments to address; no CODEOWNERS on this path.
|
Replied to both review threads. Summary: the frames really do go to stdout on Linux debug builds, because |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — gates a debug-build-only diagnostic behind the existing PackageInstaller scope so ordinary install failures no longer pay for an llvm-symbolizer run.
What was reviewed:
- Verified
PackageInstalleris an existingScopedLoggerstatic (declare_scope!at line 41) withis_visible(), already used byscoped_log!in this file — the value/type namespaces don't collide withstruct PackageInstaller<'a>. - Release builds are unaffected: the block stays under
#[cfg(bun_debug)]. - The stderr assertion tightening from
toContainto exact split-line equality matches the pattern already used throughout this test file (e.g.expect(err.split(/\r?\n/)).toEqual([""])). - CodeRabbit's two findings were both withdrawn after author explanation; the comment-cop note was addressed in b3dded3.
Extended reasoning...
Overview
Two files touched. In src/install/PackageInstaller.rs, the #[cfg(bun_debug)] block that unconditionally called bun_crash_handler::dump_stack_trace on every install failure is now gated on PackageInstaller.is_visible() — the ScopedLogger static already declared at the top of this file and already consumed by scoped_log! for the per-package "Installing x@y" line. The intermediate let t = cause.debug_trace binding was inlined to &cause.debug_trace.trace(). In test/cli/install/bun-link.test.ts, the failing-install stderr assertion was tightened from toContain to an exact split-line array so the stderr variants of the dump (macOS in-process printer, Linux without symbolizer) would also fail the test.
Security risks
None. This is a debug-build-only diagnostic gate; the block is compiled out under #[cfg(bun_debug)] in release builds, exactly as before. No user input handling, no allocation, no FFI surface changes.
Level of scrutiny
Low. The Rust change is a 4-line edit inside a #[cfg(bun_debug)] block that only affects local debug binaries. It reuses the exact opt-in mechanism (ScopedLogger::is_visible(), driven by BUN_DEBUG_PackageInstaller=1 and honoring BUN_DEBUG_QUIET_LOGS) that the codebase uses everywhere for verbose debug output — verified against src/bun_core/output.rs:1459 and the declare_scope! macro at line 1520. The static and the generic struct sharing the name PackageInstaller live in separate Rust namespaces, and the static is already referenced this way at line 1130.
Other factors
The test tightening follows the repo review guidance (toBe over toContain) and matches the exact-stderr-array pattern used by every other spawn step in this file, so it does not introduce a new flake surface. Both CodeRabbit findings were investigated by the author and withdrawn; the github-actions comment-cop note about comment length was addressed in b3dded3 (comment is now one line). The PR description documents before/after timing, USE_SYSTEM_BUN=1 parity, and the opt-in path still working. No prior claude[bot] review exists on this PR.
There was a problem hiding this comment.
LGTM — debug-build-only change gating an expensive diagnostic behind the existing scoped logger.
What was reviewed:
- Confirmed
declare_scope!(PackageInstaller, hidden)at PackageInstaller.rs:41 produces theScopedLoggerstatic whose.is_visible()is called; it coexists with thestruct PackageInstaller(value vs type namespace). - Checked that
StoredTrace::trace(&self)borrows, so switching from move-then-borrow to&cause.debug_trace.trace()is sound and avoids a partial move. - The tightened stderr assertion matches the exact one-line error; the sibling stdout assertion already covers the Linux symbolizer-on-stdout variant.
- Release builds unaffected (
#[cfg(bun_debug)]still wraps the block).
Extended reasoning...
Overview
Two files: src/install/PackageInstaller.rs wraps the existing #[cfg(bun_debug)] stack-trace dump in the install-failure else-branch behind PackageInstaller.is_visible() (the file's already-declared bun_output::declare_scope! logger at line 41), and inlines the trace borrow. test/cli/install/bun-link.test.ts tightens one stderr assertion from toContain to an exact-match array so the stderr variants of the dump (no symbolizer / macOS) would also fail the test.
Security risks
None. Debug-build-only diagnostic gating; no input parsing, no auth, no user-facing surface change in release builds.
Level of scrutiny
Low. The entire Rust change is inside a #[cfg(bun_debug)] block, so release binaries are byte-identical. The mechanism (ScopedLogger::is_visible()) is the established pattern for opt-in debug diagnostics and is already used twice in this file via scoped_log!. I verified the macro expands to pub static PackageInstaller: ScopedLogger, that is_visible() reads BUN_DEBUG_PackageInstaller and respects BUN_DEBUG_QUIET_LOGS, and that the static-vs-struct name reuse is fine (separate Rust namespaces). The borrow simplification is correct: StoredTrace::trace() takes &self.
Other factors
All review-thread items are resolved: both CodeRabbit findings were withdrawn after the author explained the stdout/stderr split (llvm-symbolizer inherits stdio), and the comment-cop nit was addressed in b3dded3 by shortening the Rust comment to one line. The PR description documents verification on both debug and release builds, and the bug-hunting system found nothing. The test change strengthens rather than weakens an existing assertion.
|
Updated 6:45 PM PT - Aug 10th, 2026
✅ @robobun, your commit 21748577a252f902b799f7c19264f11ffff39080 passed in 🧪 To try this PR locally: bunx bun-pr 37335That installs a local version of the PR into your bun-37335 --bun |
There was a problem hiding this comment.
LGTM — debug-build-only gate using the existing PackageInstaller scope, plus a tightened stderr assertion. One inline nit on the test comment wording; not blocking.
Extended reasoning...
Overview
Two files touched: src/install/PackageInstaller.rs wraps the existing #[cfg(bun_debug)] stack-trace dump in if PackageInstaller.is_visible(), and test/cli/install/bun-link.test.ts tightens the failing-install stderr assertion from toContain to an exact two-element array match. The Rust change also drops the intermediate let t binding and borrows cause.debug_trace in place.
Security risks
None. The entire code change is inside #[cfg(bun_debug)], so release binaries are byte-identical. No user input, no new I/O, no allocation — it only adds a scope-visibility check before calling the existing crash-handler dumper.
Level of scrutiny
Low. This is a debug-diagnostics gate using the mechanism already declared at the top of the file (bun_output::declare_scope!(PackageInstaller, hidden), line 41) and used identically elsewhere (bundle_v2.rs, sys/lib.rs, jsc/lib.rs). The scope is hidden by default, so is_visible() returns false unless BUN_DEBUG_PackageInstaller=1 is set — which bunEnv does not set. The test change makes the assertion strictly stronger; the same file already uses expect(err.split(/\r?\n/)).toEqual([""]) on stderr for the earlier steps, so the exact-match pattern is established and not a new flake risk under ASAN.
Other factors
All prior review threads are resolved: CodeRabbit's two comments were withdrawn after the author correctly explained the llvm-symbolizer stdout inheritance, and the comment-cop bot's flag on the Rust comment was addressed in b3dded3. The one remaining nit (the 3-line test comment narrates the change past-tense; the Rust side was already trimmed to one line) is posted inline and is cosmetic — it does not affect behavior or block approval.
2 participants
Problem
bun bd test test/cli/install/bun-link.test.tsfails on a debug build at current main:The other three tests in the file pass, and all four pass with a release binary (
USE_SYSTEM_BUN=1).The last step of that test runs
bun installafter the linked package was unlinked and expects exit 1 with a one line error on stderr and a clean summary on stdout. Running the same steps by hand againstbuild/debug/bun-debug, the first three steps take about 100-130ms each and the failing install takes about 5.2s, with stdout looking like this:Cause
The
elsebranch of the install failure reporting insrc/install/PackageInstaller.rs(the branch that printsfailed <step> for package <name>) has a#[cfg(bun_debug)]block that passes the trace captured byInstallResult::failtobun_crash_handler::dump_stack_trace. That function is the crash path dumper: on Linux debug builds it spawnsllvm-symbolizeragainst the debug binary with inherited stdio, so it costs several seconds per failed package and the frames land on stdout, inside the install summary. When no symbolizer is onPATHit printsFailed to invoke command: llvm-symbolizer ...plus the WTF fallback trace to stderr, and on macOS it writes the trace to stderr, so the dump shows up on one stream or the other on every platform. The crash handler already moved its other non-fatal callers off this path for the same reason (see the comment ondump_current_stack_trace_from_core); this was the last non-crash caller.The branch is reached by ordinary user facing failures (a
link:dependency whose target was unlinked, as here, or copy, extract and patch failures), not by bugs in bun, so every such failure in a debug build paid for the symbolizer run. EveryInstallResult::failcall site passesNonefor the trace, so what gets dumped is the plain call stack at the failure site, which is developer diagnostics rather than something the default output needs.Fix
Gate the dump behind the
PackageInstallerdebug scope that is already declared in this file and used for the per packageInstalling x@ylog, so it is off by default (including under the test harness, which setsBUN_DEBUG_QUIET_LOGS=1) andBUN_DEBUG_PackageInstaller=1still prints it. The scope check is the mechanism the codebase already uses for verbose debug build diagnostics, and it keeps the full source line trace available when someone is actually debugging the installer. Release builds are unaffected: the block is still compiled out there.The test's stderr assertion for the failing install is tightened from
toContainto the exact line, so the stderr variants of the dump (no symbolizer onPATH, macOS) fail the test too; before this change only the Linux-with-symbolizer variant was caught, via the stdout assertion and the timeout.Verification
bun bd test test/cli/install/bun-link.test.ts: before,should link dependency without crashingtimes out at 5s; withPATHstripped ofllvm-symbolizerit instead fails the tightened stderr assertion with theFailed to invoke command: llvm-symbolizer ...lines and the fallback frames. After, all 4 tests pass and that test takes about 530ms.USE_SYSTEM_BUN=1 bun test test/cli/install/bun-link.test.ts: passes before and after (release builds never had the dump).BUN_DEBUG_PackageInstaller=1it prints[packageinstaller] Installing zzlinkpkg@link:zzlinkpkgfollowed by the same symbolized trace as before (5.2s). The opt-in path is not covered by an automated test: it is debug build only and its runtime is the symbolizer run this change takes off the default path.bun bd testonbun-install-hardlink-fallback.test.tsandsymlink-path-traversal.test.tsstill passes.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-link.test.ts