install: gate the exit-callback cache teardown to the main thread#31376
Conversation
… the main thread in ASAN builds
|
Updated 2:02 AM PT - May 25th, 2026
❌ @Jarred-Sumner, your commit 191db10 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31376That installs a local version of the PR into your bun-31376 --bun |
|
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 (1)
WalkthroughExit-time cache cleanup registration is now conditional on ASAN, and the exit callback returns early when run on non-main threads while keeping initialization and singleton checks. A new CLI test asserts ChangesExit-time cache cleanup robustness with thread-safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, defensive fix gating an ASAN-only exit callback behind #[cfg(bun_asan)] and adding a main-thread guard, with a regression test mirroring the existing cafile tests.
Extended reasoning...
Overview
This PR touches two files: src/install/PackageManager.rs (gating deinit_caches_at_exit registration and definition behind #[cfg(bun_asan)] and adding an is_main_thread() early-return) and test/cli/install/bun-install-registry.test.ts (a new test that runs bun install --cafile /does/not/exist in a workspace project and asserts exit code 1 with the expected error message). The function is only referenced from allocate_package_manager, and both the registration and the definition are now under the same cfg, so non-ASAN builds compile cleanly with the callback elided entirely.
Security risks
None. The change strictly reduces what runs at process exit: release builds no longer register the callback at all, and ASAN builds bail out off the main thread before dereferencing main-thread-owned MimallocArena heaps. No new inputs, parsing, auth, or network surface.
Level of scrutiny
Low-to-moderate. The fix is defensive (adding guards, not changing happy-path logic), the cfg flag bun_asan is already used pervasively in src/bun_alloc/, and bun_crash_handler::cli_state::is_main_thread() is an existing thread-ID comparison that does not depend on VM initialization. The only behavioral change in production builds is removing a redundant exit-time teardown, which is always safe since the OS reclaims process memory anyway.
Other factors
The new test follows the exact structure of the adjacent "non-existent --cafile" test, just with a workspace layout to populate the cache before the HTTP thread fails. The initial CI build failure on commit 70e962b was addressed by the follow-up commit bbb769e (moving from a runtime ENABLE_ASAN check to #[cfg(bun_asan)] so the unused function doesn't trip dead-code lints in non-ASAN builds). The duplicate-PR bot flagged #31357 as overlapping; that's a merge-order decision for the maintainer and doesn't affect this PR's correctness. No prior reviews from me, no outstanding human reviewer comments, and the bug hunter found nothing.
There was a problem hiding this comment.
LGTM — strictly narrows when the exit-time teardown runs, so it can only make things safer.
Extended reasoning...
Overview
This PR touches two files: a 5-line guard in src/install/PackageManager.rs and a new regression test in test/cli/install/bun-install-registry.test.ts. The Rust change (1) gates registration of the deinit_caches_at_exit exit callback behind #[cfg(bun_asan)] so it only exists in ASAN builds, and (2) adds an early return when the callback fires on a non-main thread. The test reproduces the workspace + invalid --cafile scenario that was intermittently dumping core in CI and asserts a clean exit code 1.
Security risks
None. This is exit-time memory cleanup for LeakSanitizer bookkeeping; no auth, crypto, input parsing, or network surface is touched.
Level of scrutiny
Low. The change is purely subtractive with respect to when teardown runs: in release builds the callback is no longer registered at all (OS reclaims the memory, which is strictly safer than the previous behavior), and in ASAN builds it now bails out off-main-thread instead of touching main-thread-owned arenas. There is no path on which this adds new behavior — it only removes a known-unsound one. I verified bun_crash_handler::cli_state::is_main_thread() exists (src/crash_handler/lib.rs:559) and that #[cfg(bun_asan)] is an established cfg used elsewhere in the tree, and that the function's only reference (the add_exit_callback call) is gated by the same cfg so non-ASAN builds compile cleanly.
Other factors
The new test mirrors the adjacent "non-existent --cafile" test's assertions exactly (same error string and exit code), just with a workspace layout to populate the cache first. The Windows transpiler.test.js CI failure is unrelated to these files. A bot flagged #31357 as a possible duplicate fix — that's a process question for the maintainers but doesn't affect the correctness of this diff. No CODEOWNERS entries cover these paths.
Jarred-Sumner
deleted the
claude/hardening-fix-pre-111-exit-callbacks-on-http-thread
branch
* oven/main (10 new commits): Optimize TextEncoder.encode: restore SIMD ASCII fast paths lost in the Rust port (oven-sh#31385) js_parser: sanitize auto-generated default export name for digit-named modules (oven-sh#31403) fetch: run checkServerIdentity before writing the request (oven-sh#31325) ffi: avoid copying the threadsafe callback wrapper on the calling thread (oven-sh#31332) install: gate the exit-callback cache teardown to the main thread (oven-sh#31376) fix(node:module): don't register native helpers as their own constructors (oven-sh#31393) css: escape custom pseudo-class/element names when printing (oven-sh#31404) Deepen the lots-of-for-loop fixture so the transpiler stack-overflow tests throw on Windows (oven-sh#31382) Hardening: input validation and bounds tightening across 36 subsystems (round 4) (oven-sh#31339) Speed up FormData multipart serialization (oven-sh#31379) Auto-merged: src/install/PackageManager.rs, src/runtime/cli/upgrade_command.rs, src/runtime/webcore/Blob.rs, src/sys/lib.rs
2 participants
The package manager registers an exit callback that tears down its workspace package.json cache so LeakSanitizer does not report it as still reachable at exit. Exit callbacks run on whichever thread drives process exit, but the cache's per-entry arenas are created by and owned by the main thread, so running that teardown from another thread (e.g. the HTTP client thread when CA-file validation fails) is unsound and can abort the process instead of exiting with code 1.
This change registers the callback only in ASAN builds (it is wasted work otherwise) and makes the callback a no-op when invoked off the main thread, letting the OS reclaim the memory on that path.
Adds a test that runs
bun install --cafile /does/not/existin a workspace project, which populates the cache on the main thread before the HTTP thread fails initialization and drives exit; it asserts the error message and exit code 1. Because the underlying failure is timing-dependent, the unfixed binary does not fail this test on every run, but the test pins the exact configuration that was intermittently dumping core in CI ("certificate authority > non-existent --cafile" on the alpine x64 lane). The existing cafile tests continue to pass.