Conversation
While one thread is inside execve(2), the kernel fails every clone(2) made by the other threads of the process with EAGAIN. A collection that starts JSC's marker threads in that window aborts in WTF::Thread::create. on_before_reload_process_posix reset SIGABRT to SIG_DFL before the execve, so that abort killed the process instead of the reload going through. Install a handler for SIGABRT, SIGILL, SIGTRAP and SIGFPE for the rest of the reload. It parks every thread other than the reloading one, and execve tears the parked threads down with the old image. A crash on the reloading thread itself still terminates the process. The test makes the race deterministic with an LD_PRELOAD shim that fails pthread_create from the moment bun calls execve() and lets the script run its first collection at that point.
|
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 30 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 1 minute), 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 (2)
Comment |
|
Updated 4:13 AM PT - Aug 21st, 2026
✅ @robobun, your commit 33209164701bc21fb6ea430c9f97c2f30c1d6a7d passed in 🧪 To try this PR locally: bunx bun-pr 39868That installs a local version of the PR into your bun-39868 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it installs a new process-wide signal handler on the reload path (threading + signals + execve across all POSIX targets), a human look would still be worthwhile.
What was reviewed:
- Handler body is async-signal-safe (
pthread_self/pthread_equal/sigaction/raise/pauseare all on the POSIX AS-safe list);reloading_threadis written beforesigactioninstalls the handler, and signal delivery goes through the kernel, so other threads observe the write. - The reset loop correctly skips the four parked signals via
std::find, so the new handler isn't immediately clobbered; SIGSEGV/SIGBUS are left to JSC as before. - On execve failure the handler stays installed, but that matches the existing
RELOAD_IN_PROGRESSnever being cleared — a crashing non-reload thread already exits viamaybe_handle_panic_during_process_reload, so this is not a regression. - Test: shim state resets in the post-exec image (
in_execveis a fresh static), markers live outside the watched dir, stderr is drained so a full pipe can't block the aborting thread, and the finalfailed-createassert proves the failure path was exercised rather than skipped.
Extended reasoning...
Overview
The PR fixes a CI flake where bun --watch dies with SIGABRT during a reload on alpine/aarch64. Root cause: while the watcher thread is inside execve, the kernel fails clone() from other threads with EAGAIN (fs->in_exec in copy_fs), and JSC's WTF::Thread::create RELEASE_ASSERTs on that when the JS thread starts GC marker threads. #34660 made this fatal by resetting SIGABRT to SIG_DFL before execve. The fix installs a handler for SIGABRT/SIGILL/SIGTRAP/SIGFPE in on_before_reload_process_posix that parks non-reloading threads in pause() (execve tears them down anyway) and re-raises with SIG_DFL on the reloading thread. Two files: ~40 lines in c-bindings.cpp, ~130 lines of test in watch.test.ts.
Security risks
None. This is internal signal-handling glue on the --watch reload path. No user input flows into it, no auth/crypto/network surface is touched. The handler only runs in the narrow window between on_before_reload_process_posix() and execve().
Level of scrutiny
High. Signal handlers + multi-threading + execve is one of the most subtle areas in POSIX programming, and this code runs on every --watch reload across Linux, macOS, and FreeBSD. That said, the diff is small, the reasoning in the PR description is unusually thorough (kernel behavior verified with a standalone C repro, CI history correlated to the triggering merges, both handler branches manually exercised), and every function called from the handler is on the POSIX async-signal-safe list. The placement is correct: installing the handler inside on_before_reload_process_posix and excluding those signals from its reset loop is the only spot that survives #34660's reset.
Other factors
- The test follows the existing LD_PRELOAD-shim pattern already in this file, is Linux-gated with
skipIf(!isLinux || !cc), keeps marker files outside the watched directory to avoid spurious file events, drains stderr to prevent pipe-full deadlock, and asserts thefailed-createmarker exists so the test can't pass by routing around the failure. The 30s timeout comfortably covers the shim's ~11s worst-case wait. - I checked the execve-failure path:
reloading_threadand the park handler stay live if execve returns, but so does the pre-existingRELOAD_IN_PROGRESSflag — the process was already in a "park anything that crashes" mode viamaybe_handle_panic_during_process_reload, so this is consistent, not a new hazard. - Cross-thread visibility of
reloading_thread: written before thesigactioncalls that install the handler; both the syscall and kernel signal delivery act as full barriers, so a thread entering the handler observes the write. - Deferring rather than approving because this is a design decision about process-level signal disposition that a maintainer should sign off on — particularly the set of signals chosen (parking SIGABRT/SIGILL/SIGTRAP/SIGFPE while leaving SIGSEGV/SIGBUS to JSC's chained handlers).
|
On the choice of signals, for whoever takes the human look: the four parked signals are the ones the crash handler installs (update_posix_segfault_handler in src/crash_handler/lib.rs) minus SIGSEGV and SIGBUS. Those two already stayed out of the reset before this PR because JSC's own handlers sit on them, and a non-JSC fault chains from there to the crash handler, which steps aside during a reload. So after this PR every crash signal behaves the same way in the window: the thread gets out of the way and the execve goes through. The only difference is that the four parked ones now also do so in ASAN builds and for more than one thread. |
The shim now also starts one thread that calls abort() and one that traps (SIGILL on x64, SIGTRAP on aarch64) before it execs for real, and the test checks that the reload went through all three crashes. A handler that parks one thread only (SA_RESETHAND), or one that parks SIGABRT only, fails the test. The previous version passed with both.
![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. Because it changes process-wide signal dispositions on the --watch reload path — async-signal-safe handler code, a new parked-thread state, and interaction with the existing SIG_DFL reset loop — a human look is still worthwhile.
Checked: the handler uses only async-signal-safe calls (pthread_equal/pthread_self/sigaction/raise/pause); reloading_thread is written before the handler is installed; the raise(sig) on the reloading thread is correctly deferred by the in-handler mask; and the reset loop's new std::find skip keeps the park handler in place. The LD_PRELOAD shim test drives all three crash paths and asserts the marker files, so it can't pass by skipping the race.
Extended reasoning...
Overview
The PR fixes a race in --watch reloads: while the watcher thread is inside execve, the kernel fails clone() on other threads with EAGAIN, and WTF::Thread::create aborts on that. Since #34660 resets SIGABRT to SIG_DFL before execve, that abort is now fatal instead of being absorbed by the crash handler. The fix installs a small handler for SIGABRT/SIGILL/SIGTRAP/SIGFPE in on_before_reload_process_posix (src/jsc/bindings/c-bindings.cpp) that parks non-reloading threads in pause() and re-raises with SIG_DFL on the reloading thread. The reset loop is updated to skip these four signals. A new Linux-only LD_PRELOAD shim test in test/cli/watch/watch.test.ts deterministically reproduces the race with three concurrent crashing threads.
Security risks
None identified. The change is confined to the pre-execve window on a process that is about to replace its own image; no new external input is parsed and no privilege boundary is crossed.
Level of scrutiny
High. This is native signal-handling code on a process-lifecycle path that already has a history of subtle interactions (#34660, #36825, #39541). The correctness argument depends on kernel in_exec behavior, POSIX signal-mask semantics inside a handler, JSC's lazy marker-thread creation, and the existing crash handler's chaining for SIGSEGV/SIGBUS. Getting it wrong could turn a crash into a hang (parked reloading thread) or silently swallow a real crash on the watcher thread. The PR description is unusually thorough — kernel repro, CI history, manual verification of both handler branches — which raises confidence, but a maintainer familiar with reload_process and the crash-handler chain should confirm the design choice (park vs. some other approach) and the signal set.
Other factors
- The comment-cop bot fired several times on the handler's comment; the author iterated and the current comment is short and states the invariant rather than justifying a workaround. Whether the last fire (line 307) is now satisfied is a mechanical check that will re-run on the current head.
- The test is well-constructed: markers live outside the watched directory, stderr is drained to avoid a pipe-full deadlock,
RLIMIT_COREis zeroed so an unfixed build's abort doesn't trip CI's crash scan, and the finalreaddirSyncassertion proves the reload went through all three crashes rather than around them. It follows the existing LD_PRELOAD shim pattern already in this file. - No prior review from me on this PR.
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Because it changes process-wide signal dispositions on the --watch reload path — parking crashing threads across the execve window — a human sign-off on the design is still worthwhile.
What was reviewed:
- Handler is async-signal-safe (
pthread_equal/pthread_self/sigaction/sigemptyset/raise/pauseare all on the POSIX list). parked_crash_signalsmatchesupdate_posix_segfault_handler's set minus SIGSEGV/SIGBUS, and the reset loop correctly skips them.- Re-raise on the reloading thread: the signal is masked during its own handler (no
SA_NODEFER), soraisequeues and delivers withSIG_DFLon return — no hang. - Test follows the existing LD_PRELOAD-shim pattern in this file; markers dir is outside the watched dir; stderr is drained;
afterEachSIGKILLs the watchee.
Extended reasoning...
Overview
The PR modifies on_before_reload_process_posix in src/jsc/bindings/c-bindings.cpp to install a signal handler for SIGABRT/SIGILL/SIGTRAP/SIGFPE just before the --watch reload's execve. On any thread other than the one performing the reload, the handler parks in pause() so the thread stops interfering and is torn down by the execve; on the reloading thread itself it restores SIG_DFL and re-raises so a crash there is still fatal. The reset loop that follows now skips these four signals so it does not overwrite the handler with SIG_DFL. A new Linux-only LD_PRELOAD-shim test in test/cli/watch/watch.test.ts deterministically reproduces the race (failed pthread_create → WTF::Thread::create abort on the JS thread, plus two extra crashing threads) and asserts the reload still goes through.
Security risks
None identified. The handler is only installed immediately before execve on the watcher thread's reload path, is process-local, and does not touch any external input. The parked signals are the crash-handler set (verified against src/crash_handler/lib.rs:1630-1638) minus SIGSEGV/SIGBUS, which the existing code already leaves for JSC.
Level of scrutiny
High. This is process-wide POSIX signal disposition on a path that affects every --watch user, and the correctness argument depends on kernel behavior (Linux failing clone(CLONE_FS) with EAGAIN while another thread is in execve), async-signal-safety, and the interaction with JSC's own SIGSEGV/SIGBUS handlers and the crash handler's SA_RESETHAND. The reasoning in the PR description is thorough and the mechanism is well-argued, but this is not a simple or mechanical change — a maintainer should confirm the design choice (park-in-pause() vs. alternatives) and the signal set.
Other factors
The comment-cop bot flagged comment length; the author shortened to two lines and all threads are resolved. The test mirrors an existing LD_PRELOAD-shim test in the same file, uses tempDir, keeps markers outside the watched dir, drains stderr, and relies on the file-level afterEach SIGKILL for cleanup. I verified the handler uses only async-signal-safe calls, that the re-raise-on-reloading-thread branch cannot hang (signal masked during its own handler, delivered on return), and that the four parked signals exactly match the crash handler's set minus SIGSEGV/SIGBUS. No prior claude[bot] reviews on this PR.
…reloading thread The shim interposes pause(), which is where bun parks a crashing thread, and execs as soon as the three crashing threads have called it instead of sleeping for a second. A second case checks that an abort on the watcher thread itself, after the handler is installed, still ends the process. The shim compile is shared by the three shim tests.
|
Self-review came back with two points on the test, both about the fixed 1 s sleep in the shim. 3320916 replaces it: the shim interposes pause(), which is where bun parks a crashing thread, and execs as soon as the three crashing threads have called it. That takes about 1 s off the test. The same commit adds a case for the other branch of the handler (an abort on the watcher thread itself still ends the process) and shares the shim compile between the three shim tests. No change to the fix itself since 965b296. |
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. The analysis is thorough and the fix is small, but it changes process-wide signal dispositions in the reload path — a human look at the signal-handling semantics would still be worthwhile.
What was reviewed:
- The park handler uses only async-signal-safe calls (
pthread_self,pthread_equal,sigaction,raise,pause) and the reset loop correctly skips the four parked signals. - The reloading-thread branch re-raises with
SIG_DFLso a crash on the watcher thread itself stays fatal — covered by the second new test. - The shim test awaits observable conditions (interposed
pause()+ marker files) rather than sleeping, drains stderr, and asserts all four markers so the reload can't pass by going around the crashes.
Extended reasoning...
Overview
The PR modifies on_before_reload_process_posix in src/jsc/bindings/c-bindings.cpp to install a signal handler for SIGABRT/SIGILL/SIGTRAP/SIGFPE before the existing disposition-reset loop. The handler parks any non-reloading thread in pause() and re-raises with SIG_DFL on the reloading thread. The reset loop is updated to skip these four signals. Two new Linux-only LD_PRELOAD shim tests are added to test/cli/watch/watch.test.ts, and the existing shim-compile logic is factored into a shared compileShim helper.
Security risks
None identified. The change is scoped to the --watch reload window and only affects how crash signals are handled between the disposition reset and execve. It does not touch auth, crypto, network input, or user-controlled data.
Level of scrutiny
High. This is low-level POSIX signal handling on a production-critical path (process reload). The handler runs in async-signal context on arbitrary threads, interacts with JSC's own SIGSEGV/SIGBUS handlers, and changes the behavior of every crash signal in the reload window across all POSIX platforms (Linux, macOS, FreeBSD). While the fix itself is ~25 lines and the reasoning in the PR description is exceptionally detailed (kernel source references, CI history, empirical verification with a C reproducer), the correctness depends on subtle invariants: that execve will always follow shortly after this function, that parking a thread mid-abort cannot deadlock the reloading thread (e.g. via a lock held by the parked thread that execve setup needs), and that the four-signal set matches the crash handler's set going forward.
Other factors
- The author's own timeline comment explicitly flags this "for whoever takes the human look", indicating a human review is expected.
- The tests are elaborate and well-constructed (deterministic via LD_PRELOAD, await real conditions, cover both handler branches, verified to fail on unfixed builds), but they only run on Linux with a C compiler present — the macOS/FreeBSD paths are covered by the same code but not exercised by the new tests.
- The comment-cop bot flagged the code comments repeatedly; the author shortened them and the remaining comments are now two lines each, which seems reasonable given the non-obvious kernel behavior being documented.
- No prior human review on the PR.
|
On the one open question in the latest review, whether a parked thread can hold a lock that the reloading thread still needs: after on_before_reload_process_posix the reloading thread only duplicates argv and environ (malloc), reads the cached exe path and calls execve (src/bun_core/util.rs, reload_process). The thread that crashes in WTF::Thread::create holds no allocator lock at that point (pthread_create has returned), and mimalloc allocates from per-thread heaps, so the duplication does not take a lock the parked thread can own. The lock abort() holds on newer glibc only blocks other callers of abort() and sigaction(SIGABRT), and the reloading thread calls neither afterwards. The exposure is the same one the crash handler's pthread_exit had before #34660. The kernel then kills the parked threads inside execve, so nothing they hold matters after that point. |
|
#40978 takes a different route to the same crash: on Linux it links with |
|
Superseded by #40978. |
1 participant
Problem
test-changed.test.ts(--changed --watch) is red on the alpine lanes and flaky on the aarch64 lanes. Thebun test --watchchild dies during a reload:SIGABRTinWTF::Thread::create(Threading.cpp:330,RELEASE_ASSERT(success)), under the idle GC inBun__JSC_onBeforeWait.execvewhile the JS thread keeps running. While one thread is insideexecve, the kernel fails everyclonefrom the other threads withEAGAIN. The JS thread was creating the GC marker threads right then. WTF aborts on that.on_before_reload_process_posix(src/jsc/bindings/c-bindings.cpp) resetsSIGABRTtoSIG_DFLbefore theexecve. Until then the crash handler ended only the thread. Don't request a GC before waiting on the entry point #39541 made the race frequent: the marker threads now appear at the first collection, which here coincides with the next touch.Fix
on_before_reload_process_posixinstalls a handler forSIGABRT,SIGILL,SIGTRAPandSIGFPE, outside the reset loop. It parks every thread except the reloading one inpause(). On the reloading thread it restoresSIG_DFLand raises again.execvereplaces. Theexecvedoes not need it and tears it down with the rest.SIGSEGVandSIGBUSstay with JSC's handlers, as before.test/cli/watch/watch.test.ts. In the first, the JS thread aborts on the failedpthread_create, two more threads abort and trap in the same window, and the reload has to go through all three. It fails on the canary and on an unfixed debug build and passes 24/24 with the fix. The second checks that an abort on the watcher thread itself still ends the process. Other suites: see the notes.Background
--watchrestarts by callingexecveon its own binary from the watcher thread (reload_process,src/bun_core/util.rs). The other threads run until theexecvekills them.SA_RESETHANDlimits it to one thread. The new handler has neither limit.Notes
test/cli/watch/,test/cli/test/test-changed.test.ts,test/cli/hot/watch.test.ts,test/cli/run/run-crash-handler.test.ts, the nodetest-watch-mode-kill-signal-*tests. All pass.reload_process, before node compat batch: callback-throw dispatch, Assert class + native deep-equality parity, Intl gate + URL/buffer fallout, compile cache, watch kill-signal, profilers (+98 tests) #34660 landed. Installed there, the handler is removed again by the reset loop that node compat batch: callback-throw dispatch, Assert class + native deep-equality parity, Intl gate + URL/buffer fallout, compile cache, watch kill-signal, profilers (+98 tests) #34660 added toon_before_reload_process_posix, and the branch is based on the oldreload_process. This PR installs the handler in the function that does the reset.execve, another callspthread_createin a loop):pthread_createreturnedEAGAINin 26 of 30 runs, up to 7 times in a row, and theexecvewent through each time. Linux setsfs->in_execincheck_unsafe_execandcopy_fsrejectsclone(CLONE_FS)while it is set.abort()underWTFCrashWithInfowith no crash handler frames. Other alpine cores from the same days (builds 101261, 101857, 101866) do showcrash_handlerframes. That is how the reset of the dispositions was identified as the reason the abort is fatal.BUN_JSC_numberOfGCMarkers=64. The shim test drives the same path:Bun.gc->collectInMutatorThread->ParallelHelperPool->WTF::Thread::create->EAGAIN->abort()on the JS thread, between the reset and theexecve. Unfixed release and debug builds both die withSIGABRTthere, and the failingpthread_createruns on the thread whose tid is the pid.SIGABRTalone, or one withSA_RESETHAND, fails the test: the shim's extra threads raiseSIGABRTandSIGILL(SIGTRAPon aarch64) after the JS thread has already been parked.__builtin_trap()on the watcher thread ends the process withSIGILLthe same way.pause(), the shim interposes that too, and it execs once the three crashing threads have called it (about 20 ms after theexecve()call on the debug build). Without the fix the process is dead within a few ms of the failedpthread_create. The first version of the test slept 1 s here instead, which the self-review flagged.maybe_handle_panic_during_process_reloadis unchanged. It still covers Rust panics during a reload, which do not go through a signal.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/test-changed.test.ts test/cli/watch/watch.test.ts