Skip to content

bundler: fail the build instead of panicking when the bundle thread cannot create its waker - #37769

Open
robobun wants to merge 2 commits into
farm/efaf8ee8/bundle-thread-spawn-failurefrom
farm/27408989/bundle-thread-waker-failure
Open

bundler: fail the build instead of panicking when the bundle thread cannot create its waker#37769
robobun wants to merge 2 commits into
farm/efaf8ee8/bundle-thread-spawn-failurefrom
farm/27408989/bundle-thread-waker-failure

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • At the open-files limit (EMFILE/ENFILE), the first Bun.build() in a process, or the first non-HMR HTML route request in Bun.serve, aborts bun with panic: Failed to create waker / oh no: Bun has crashed. Reproduced on 1.4.0 and on bundler: fail the build instead of panicking when the bundle thread cannot be started #37753's branch.
  • The bundle thread creates the handle it sleeps on only after it has started, and a failure there was unwrapped with a panic while the thread that requested the build was still blocked waiting for it to come up, so no error could reach the build.
  • Running out of descriptors is an OS resource condition, and every other EMFILE a build or a server hits (opening a source file, accept) is already reported as an error; this was the one spot that took the process down.
  • Stacked on bundler: fail the build instead of panicking when the bundle thread cannot be started #37753, which does the same for a failed pthread_create of this thread; the diff here is only this PR's commits.

Fix

  • The bundle thread stores the waker error in the shared struct and signals ready as usual. The spawner reads it, joins the thread, frees the struct, and fails the build through the exact path bundler: fail the build instead of panicking when the bundle thread cannot be started #37753 uses for a failed spawn: spawn() returns it as a std::io::Error (an errno round-trips through SystemErrno::from_io_error into the named message, the macOS mach-port failure takes the OS-description path), so singleton::get() has a single error path for both failure kinds.
  • The message is Failed to start the bundler thread: EMFILE. plus an open-files hint for EMFILE/ENFILE, next to bundler: fail the build instead of panicking when the bundle thread cannot be started #37753's EAGAIN thread-limit hint.
  • Why it is right: after a failure nothing of the attempt survives (thread joined, allocation freed, singleton slot still empty), so the next build starts the thread from scratch, and a build has an error channel every other resource failure already uses.
  • The waker is still created on the bundle thread, because on Windows it binds the calling thread's loop. The Windows waker's init() now returns the same Result type as the POSIX ones so the shared call site compiles on every target; on Windows it cannot fail and the new branch is never taken.
  • Verification: the LD_PRELOAD test from bundler: fail the build instead of panicking when the bundle thread cannot be started #37753 now also fails eventfd() on the "Bundler" thread and runs its seven scenarios for both failure kinds (plus bundler: fail the build instead of panicking when the bundle thread cannot be started #37753's unnamed-errno case, which only the spawn can hit). The waker cases hit the panic without this change and pass with it on a debug (ASAN) build. Linux only; Windows and macOS targets are cargo check only.

Background

  • Bundle thread: Bun.build() and HTML routes in Bun.serve run on one lazily started, process-wide "Bundler" thread. The first build starts it and later builds enqueue work onto it; starting it needs two OS resources, the thread (bundler: fail the build instead of panicking when the bundle thread cannot be started #37753) and its waker (this PR).
  • Waker: the handle the bundle thread sleeps on until work is queued. It is an eventfd on Linux and a kqueue plus a mach port on macOS, so creating it costs a file descriptor and can fail with EMFILE/ENFILE; on Windows it wraps the thread's libuv loop and cannot fail.
  • Ready event: the spawner blocks on a one-shot event until the bundle thread sets it. Anything the bundle thread writes to the shared struct before set() is visible to the spawner after wait(); that is the channel the error travels through.
  • Windows loops are per thread: uWS::Loop::get() is thread_local and the loop dies with its thread, so a waker created on the JS thread before the spawn would bind the wrong loop. That is why the waker is created on the bundle thread and the failure reported back.
  • Test shim: an LD_PRELOAD library overriding pthread_create and eventfd, driven by a plan string with one letter per intercepted call (f fails with the limit errno, n with ENOMEM, u with a code bun has no errno name for, s lets it through). It recognises the bundle thread by a RUST_MIN_STACK stack size nothing else requests for the spawn, and by the thread name "Bundler" (set before the waker is created) for the eventfd.

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-thread-spawn-failure.test.ts

Earlier iterations (superseded by rebases onto #37753's branch)

Two shapes this PR had before #37753's branch moved under it:

  • The waker error was first carried as a new bun_bundler::Error::Io(bun_io::Error) variant. bundler: fail the build instead of panicking when the bundle thread cannot be started #37753 later switched its own reporting to std::io::Error end to end (so an OS code bun has no errno name for is reported as the OS describes it); this PR now converts the waker's bun_io::Error into a std::io::Error inside spawn() instead, and the extra enum variant and the bun_io::Error::name() visibility change are gone.
  • The test shim originally identified the bundle thread by Rust's default 2 MiB stack request, which on ASAN agents collided with JSC's threads (caught in CI), then by an arm/disarm marker file around the calls that start the thread. bundler: fail the build instead of panicking when the bundle thread cannot be started #37753 replaced that with running the fixtures under a RUST_MIN_STACK value nothing else asks for; the eventfd override was keyed on the thread name from the start and keeps working unchanged.

Original description:

Stacked on #37753 (this PR's base is that branch, so the diff here is only this PR's own commits). #37753 makes a failed pthread_create for the "Bundler" thread fail the build; this does the same for the other resource the thread needs to start.

What

Once the bundle thread is running it creates the handle it sleeps on (eventfd on Linux, kqueue plus a mach port on macOS). When that fails, which is what happens at the open-files limit (EMFILE/ENFILE), the first Bun.build() (or the first non-HMR HTML route request in Bun.serve) in the process aborts bun:

panic: Failed to create waker
oh no: Bun has crashed. This indicates a bug in Bun, not your code.

Repro (Linux): an LD_PRELOAD shim that makes eventfd() return EMFILE on the thread named "Bundler" (the name is set before the waker is created, and no other thread with that name creates an eventfd), then

try {
  await Bun.build({ entrypoints: ["./entry.js"] });
} catch (e) {
  console.log("rejected:", e.errors[0].message);
}

exits 134 with the panic above on 1.4.0 and on a debug build of #37753's branch. The test in this PR is that shim.

Cause

BundleThread::thread_main unwrapped Waker::init() with a panic. At that point the spawning thread is blocked in ready_event.wait() inside singleton::get(), so nothing could turn the failure into an error.

Why this should be a build error rather than stay an abort: running out of descriptors is an OS resource condition, not a broken invariant, and Bun.build() has an error channel that every other resource failure inside a build already uses (a source file that cannot be opened at the same fd limit is a build error; #37738 and #37753 route the pool and the thread itself through it). The same holds for a server bundling an HTML route on demand: every other EMFILE it hits (accept, open) is an error it recovers from once descriptors are freed, and this one spot took the process down. It is the same shape as #35185 (a helper thread that cannot start fails the operation that asked for it), and unlike #33850 it does not try to keep going without process-level infrastructure: nothing of the failed attempt is kept and the next build starts from scratch.

Fix (original shape)

  • BundleThread gets a waker_error slot. When Waker::init() fails, thread_main writes the error there, sets ready_event as usual and returns; that set() is its last access to the struct.
  • spawn() reads the slot after ready_event.wait(). On success it detaches the thread as before; on failure it joins the thread (so the allocation can be freed) and returns the error, so singleton::get() has a single error path: free the allocation, leave the slot empty so the next build tries again, and hand the error to singleton::enqueue(), which fails the build through its normal completion exactly as bundler: fail the build instead of panicking when the bundle thread cannot be started #37753 does for a failed spawn.
  • WindowsWaker::init() returned a different Result alias than the Linux and macOS wakers (bun_core::Error instead of bun_io::Error), which nothing noticed while the result was only ever unwrapped. It now returns the same type, so the shared call site compiles on every target. Its docs and SAFETY comments (and WindowsLoop::get()'s) also called the WindowsLoop a process-global or default loop; it is per thread (uWS::Loop::get() is thread_local, wrapping the thread's libuv loop, and it is freed when that thread exits), so those comments now say that and derive the pointer's lifetime from the bundle thread never exiting. The per-thread loop is also why the waker cannot simply be created on the spawning thread before the spawn: on Windows it would bind the JS thread's loop. Creating it on the bundle thread and reporting back keeps one code path for all platforms.

Not changed: when the mach port cannot be created on macOS, KEventWaker::init_with_file_descriptor leaks the kqueue it had already opened. That is pre-existing (until now the process died right after) and #33845 fixes it in bun_io.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: fix and tests pushed, stacked on #37753 (this PR's base branch; rebased onto its current tip, so the diff here is only this PR's own commits). Reproduced by failing eventfd() on the "Bundler" thread through an LD_PRELOAD shim: Bun.build() aborts with panic: Failed to create waker (exit 134) without the fix, and rejects with Failed to start the bundler thread: EMFILE. ... with it; the next build starts the thread again. The waker half of the test file fails on the base branch alone and passes with this change. Once #37753 lands this gets rebased onto main.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it reworks cross-thread lifetime handling in unsafe Rust (the waker_error handshake, join-before-free, and the spawn() return-type change) and touches every platform's Waker::init, a human look would still be worthwhile.

Checked: ResetEvent provides the release/acquire so the waker_error write in thread_main happens-before the read in spawn(); on the success path the field is never written after init so the post-wait() read does not race with the running loop; thread.join() precedes heap::destroy so no use-after-free; bun_io::Error and bun_bundler::Error both derive Copy so the new Io variant and the by-value Option read are sound; the placeholder waker owns nothing on any platform so dropping it in destroy is harmless.

Extended reasoning...

Overview

Stacked on #37753. Replaces the panic!("Failed to create waker") in BundleThread::thread_main with an error handed back to the spawning thread through a new waker_error: Option<bun_io::Error> slot published by the existing ready_event. spawn() now owns both failure kinds (thread creation and waker creation), joins the failed thread before returning Err, and singleton::get() frees the allocation and leaves the singleton empty for a retry. bun_bundler::Error gains an Io variant, bun_io::Error::name() becomes pub, and WindowsWaker::init()'s return type is aligned with the POSIX wakers so the shared call site compiles everywhere. The existing LD_PRELOAD test is extended via describe.each to also fail eventfd() on the "Bundler" thread.

Security risks

None. This is error-path plumbing for an OS resource exhaustion condition; no user input reaches the changed code, no auth/crypto/permissions.

Level of scrutiny

High. The core of the change is a cross-thread handshake in unsafe Rust with raw-pointer field projections, followed by freeing an allocation the other thread was just using. That is exactly the class REVIEW.md flags most heavily (memory safety, thread affinity, ref lifetime). I traced the synchronization: thread_main writes waker_error before ready_event.set() (release), spawn() reads it after wait() (acquire), and on the error branch set() is the bundle thread's last access to *instance before it returns, so join()destroy() is sound. On the success branch waker_error is only ever the None written by uninitialized() before the spawn, so reading it while the bundle thread runs its loop touches a disjoint, quiescent field via raw projection — no &Self is materialized. Async::Error is Copy, so the by-value Option read and the new #[from]/Copy derive on bun_bundler::Error hold.

Other factors

  • The Windows return-type fix (crate::Resultcrate::error::Result) is a real cross-target compile fix that only surfaces now that the result is pattern-matched instead of unwrapped; the PR description says cargo check was run for Windows/macOS/FreeBSD.
  • Test coverage is thorough (both entry points, both throw modes, retry-after-failure, repeated failure, HTML route) and reuses the #37753 harness via describe.each, which is the right shape.
  • The pre-existing kqueue leak on macOS mach-port failure is called out and deferred to #33845, and IoRequestLoop::load()'s own Waker::init() panic is a separate subsystem — both reasonable scope boundaries.
  • No prior reviews on the timeline.

The change looks correct to me, but unsafe cross-thread lifetime code plus a four-platform Waker surface is not something I'd approve without a human pass.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:28 PM PT - Aug 16th, 2026

@robobun, your commit b4f211fc7e0ce20436e747c112cab9c903af4560 passed in Build #99440! 🎉


🧪   To try this PR locally:

bunx bun-pr 37769

That installs a local version of the PR into your bun-37769 executable, so you can run:

bun-37769 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it touches the cross-thread raw-pointer handshake in BundleThread::spawn/thread_main (unsafe field writes synchronized by ready_event, then join-before-free), a human look would still be worthwhile.

What was reviewed:

  • The waker_error write-before-set() / read-after-wait() ordering, and that set() is the failing thread's last access to *instance before join()destroy().
  • bun_io::Error is Copy, so the by-value read of (*instance).waker_error is sound; the placeholder waker owns nothing, so dropping it in destroy() is harmless.
  • WindowsWaker::init() return-type unification (crate::Resultcrate::error::Result) so the shared match compiles on Windows; the per-thread-loop comment corrections match uws_get_loop_with_native.
  • The shim's eventfd hook keys on prctl(PR_GET_NAME) == "Bundler", which is set (via both Builder::name and configure_named_thread) before Waker::init() runs.
Extended reasoning...

Overview

Stacked on #37753. Replaces the panic!("Failed to create waker") in BundleThread::thread_main with an error path: a new waker_error: Option<bun_io::Error> slot is written by the bundle thread before it set()s ready_event and returns; spawn() reads it after wait(), joins the thread, and returns Err. spawn() now returns Result<(), bun_bundler::Error> and absorbs the detach/join and errno-mapping that #37753 had put in singleton::get(), so the singleton has one uniform error path (free the allocation, leave the slot empty, fail the build via enqueue()'s existing completion hand-back). bun_bundler::Error gains an Io(bun_io::Error) variant; bun_io::Error::name() becomes pub; WindowsWaker::init()'s return type is unified with the POSIX wakers; and several comments describing WindowsLoop as process-global are corrected to per-thread. The test file is extended via describe.each to also intercept eventfd() on the "Bundler" thread.

Security risks

None. This is error-path plumbing for OS resource exhaustion (EMFILE/ENFILE) during Bun.build(); no untrusted input handling, auth, or crypto is touched.

Level of scrutiny

High. The core change is a cross-thread handshake over a heap allocation via raw pointers under unsafe: thread_main writes waker_error and set()s the event, spawn() wait()s and reads it, then join()s so the caller can free the allocation. I traced the ordering (ResetEvent is futex-backed and provides the happens-before; set() is the failing thread's last access; join() precedes destroy()), confirmed bun_io::Error derives Copy so the by-value read is a plain copy, and checked that the placeholder waker on every platform owns nothing whose drop would be unsound (Linux: sentinel fd; macOS: empty Box<[u8]> = no allocation; Windows: None). The ptr::write on the success path overwrites the placeholder without dropping it, which is also fine for the same reason. All of this looks correct, but it is exactly the memory-safety category the repo's review guidelines flag as most-blocked, and it is stacked on an unmerged PR, so a maintainer should confirm the design.

Other factors

The Windows init() return-type change from crate::Result (the write module's alias) to crate::error::Result (bun_io::Error) is required for the shared match Async::Waker::init() to typecheck across targets — the PR description says cargo check passes on Windows/macOS/FreeBSD. The comment-only edits in src/io/lib.rs and src/uws_sys/Loop.rs correct a factual inaccuracy (the uws loop is thread_local) that also justifies why the waker must be created on the bundle thread. The test extension reuses the existing five scenarios via describe.each, and the new eventfd hook is correctly scoped by thread name (set before Waker::init() runs).

@robobun
robobun force-pushed the farm/27408989/bundle-thread-waker-failure branch from b86ad2e to 82ebc6f Compare August 12, 2026 13:21
@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

The x64-asan lane caught the shim intercepting JSC's own 2 MiB threads (WTF::Thread::create asserts on failure, so the process aborted). The base branch #37753 fixed that by only arming the shim around the calls that start the bundle thread, so I rebased onto its current tip and folded the waker mode into that structure: eventfd() is now intercepted only while armed and only on the thread named "Bundler".

The rebase also picks up SystemErrno::from_io_error from the base instead of this branch's local errno mapping, and both modes now cover the ENOMEM path without the resource-limit hint. All 12 tests pass locally on the ASAN debug build.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it touches unsafe concurrent Rust (raw-pointer field projection, cross-thread handoff of waker_error via ResetEvent, thread join before freeing the allocation) and cross-platform waker code, a human look at the memory-safety reasoning would still be worthwhile.

What was reviewed:

  • waker_error handoff: write before ready_event.set() / read after wait()ResetEvent is acquire/release (src/threading/ResetEvent.rs), and bun_io::Error is Copy, so the raw read is sound.
  • Success path: bundle thread never touches waker_error, so spawn() reads the spawner's own initial None — no race.
  • thread.join() before Err return, so singleton::get()'s destroy cannot race the exiting thread.
  • WindowsWaker::init() return type now matches the POSIX wakers so the shared match in thread_main type-checks on all targets.
Extended reasoning...

Overview

Stacked on #37753. Converts the panic!("Failed to create waker") in BundleThread::thread_main (when eventfd/kqueue fails at the fd limit) into a build error surfaced through the same channel #37753 established for pthread_create failure. Adds a waker_error: Option<bun_io::Error> slot on BundleThread, changes spawn() to return Result<(), bun_bundler::Error> and to join the failed thread before returning, unifies the error path in singleton::get()/enqueue(), adds Error::Io(bun_io::Error) to the bundler error enum, aligns WindowsWaker::init()'s return type with the POSIX wakers, and corrects several comments describing the Windows loop as process-global (it is per-thread). The test file gains an eventfd() interposer keyed on the thread name and runs the existing 6 scenarios × 2 failure modes via describe.each.

Security risks

None. This is error-path plumbing for OS resource exhaustion; no untrusted input parsing, auth, or crypto.

Level of scrutiny

High. The core of the change is unsafe concurrent Rust: a value is written on the bundle thread via raw-pointer projection, published through a futex-backed ResetEvent, read on the spawning thread via raw-pointer deref, and then the allocation is freed after thread.join(). I verified the pieces (ResetEvent uses acquire/release; bun_io::Error and bun_bundler::Error derive Copy; on the success path the bundle thread never writes waker_error so the spawner's read of its own initial None cannot race; set() is the failing thread's last access to *instance and join() precedes the caller's free), and the SAFETY comments accurately describe each of these. But per the review guidelines, memory-safety-sensitive concurrent code with raw-pointer lifetime reasoning is exactly where a human should confirm the argument rather than rely on automated review alone.

Other factors

  • Cross-platform: the WindowsWaker::init() return-type change (crate::Resultcrate::error::Result) is needed for the shared call site to compile on Windows; the PR description says cargo check was run for Windows/macOS/FreeBSD. The comment corrections about the per-thread WindowsLoop are accurate (uws_get_loop_with_native wraps a thread-local libuv loop).
  • Tests are thorough — both failure kinds × {reject, throw:false, ENOMEM without hint, retry succeeds, retry keeps failing, HTML route 500→200}, using an LD_PRELOAD shim that attributes eventfd() by prctl(PR_GET_NAME) (the thread name is set before Waker::init()).
  • The PR explicitly calls out the pre-existing kqueue leak on macOS mach-port failure as out of scope (#33845), which is the right boundary.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Nothing changed since this review; it covers the rebased tip (82ebc6f), and CI for it is in flight. Agreed that the waker_error handshake in BundleThread::spawn/thread_main is the part that deserves a maintainer look; the PR body walks through the ordering argument (write before set(), read after wait(), join() before the allocation is freed).

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

CI on 82ebc6f: no test failures in 192 jobs (the x64-asan lane that caught the earlier shim issue is green). The build shows red only because two "darwin 26 aarch64 - test-bun" jobs expired waiting for an agent. This will re-run anyway when #37753 lands and this branch is rebased onto main.

…annot create its waker

The bundle thread creates its wakeup handle (eventfd on Linux, kqueue plus
mach port on macOS) on the thread itself and panicked when that failed, which
happens at the open-files limit. The spawning thread was blocked in
ready_event.wait() at that point, so the first Bun.build() (or HTML route) in
the process took bun down with "panic: Failed to create waker".

thread_main now records the error in BundleThread::waker_error before setting
ready_event and returns. spawn() reads it after the handshake, joins the
thread and returns the error, so singleton::get() frees the allocation and
reports it the same way it already reports a failed pthread_create:
singleton::enqueue() fails the build with the reason in its log, and the next
build tries to start the thread again.

The waker still has to be created on the bundle thread because on Windows it
binds the calling thread's libuv loop; WindowsWaker::init() now returns the
same Result type as the POSIX wakers so the shared call site compiles on all
targets. bun_bundler::Error gains an Io variant to carry the bun_io error.
…ing comments

The field doc already says the waker holds the calling thread's loop; the
SAFETY comments in wait()/wake(), the uv_loop() doc and WindowsLoop::get()
still called it a process-global or default loop.
@robobun
robobun force-pushed the farm/27408989/bundle-thread-waker-failure branch from 82ebc6f to b4f211f Compare August 16, 2026 19:01
@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto the updated tip of #37753 (this PR is stacked on that branch, so that is where the conflicts came from, not main). The base had reworked two things this branch touched, and the resolution adopts both:

  • bundler: fail the build instead of panicking when the bundle thread cannot be started #37753 now reports a failed start as a std::io::Error end to end, so an OS code bun has no errno name for is described by the OS instead of being collapsed into EAGAIN. The waker error follows the same path now: spawn() converts the bun_io::Error into a std::io::Error (an errno round-trips into the named message), and the bundler error enum no longer needs the Io variant an earlier iteration added.
  • The test shim now singles out the bundle thread by a RUST_MIN_STACK value nothing else requests, replacing the arm/disarm marker file. The eventfd override keeps keying on the thread name. Both failure modes run all seven scenarios, including bundler: fail the build instead of panicking when the bundle thread cannot be started #37753's new VM-teardown case; the unnamed-errno case stays spawn-only since eventfd errors always carry an errno.

All 15 tests pass locally on the ASAN debug build; bun_bundler cargo check passes for the Windows and macOS targets. PR description updated to match.

@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Checked the conflict report: this branch merges cleanly into current main (verified with git merge-tree against 8326d1b), and so does the base branch, so there is nothing to resolve right now. The PR stays stacked on #37753; rebasing it onto main directly would fold everything main has gained into this PR's diff. When #37753 lands, this branch gets rebased onto main and CI re-runs.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and the automated bug-hunting pass found no issues. The waker_error handshake in BundleThread::spawn/thread_main looks sound to me (write before set(), read after wait(), join() before the allocation is freed, Async::Error is Copy so the by-value read is fine), but since it adds new unsafe cross-thread synchronization and changes spawn()'s ownership contract, a maintainer look is still worthwhile — as the author already noted.

Also checked: singleton::get() is the only caller of BundleThread::spawn so the return-type change is fully covered; the WindowsWaker::init() return-type fix (crate::Result at the io crate root is write::Result<_, bun_core::Error>, not error::Result) correctly unifies it with the POSIX wakers so waker_error_to_io's match compiles on Windows; the describe.each refactor keeps every existing pthread_create test intact and adds the waker mode alongside it.

Extended reasoning...

Overview

Stacked on #37753. Turns a panic (Failed to create waker) in BundleThread::thread_main into a build error when the bundle thread's waker (eventfd on Linux, kqueue+mach port on macOS) can't be created, typically at the open-files limit. Adds a waker_error: Option<bun_io::Error> slot published via the existing ready_event; spawn() now handles both detach-on-success and join-on-failure internally and returns std::io::Result<()>. Also fixes WindowsWaker::init()'s return type to match the POSIX wakers (crate::error::Result instead of crate::Result, which at the bun_io crate root is actually write::Result<_, bun_core::Error>), and corrects several comments describing WindowsLoop as process-global when it is per-thread. The test file gains an eventfd() interception in the LD_PRELOAD shim (keyed by thread name "Bundler") and reruns the six scenarios for both failure kinds via describe.each.

Security risks

None. This is error-path plumbing for an OS resource-exhaustion condition; no user-controlled input is parsed and no privilege boundary is crossed.

Level of scrutiny

High. The core change is new unsafe code implementing a cross-thread handshake over a raw *mut BundleThread: the bundle thread writes waker_error via addr_of_mut!, calls ready_event.set(), and returns; the spawner reads it after wait(), join()s, then the caller frees the allocation. The reasoning depends on ResetEvent providing acquire-release (it's futex-backed per the file's re-export comment), on Async::Error being Copy (verified in src/io/error.rs — it derives Copy), on the by-value (*instance).waker_error read not forming a &Self (it's a raw place read of a Copy field), and on set() being the failed thread's last access to *instance. All of this checks out, but per REVIEW.md this is squarely the "native code: memory safety" category that warrants maintainer sign-off. spawn()'s safety contract also changes (after Err, either no thread was started or one was started and joined), which singleton::get() — the only caller — handles correctly.

Other factors

  • The test refactor was checked line-by-line: every pre-existing pthread_create test case is preserved inside the new describe.each, and the one non-parametrized test (unnamed errno) correctly stays pthread_create-only with a comment explaining why.
  • waker_error_to_io maps Sys(errno) via from_raw_os_error(errno as i32) so SystemErrno::from_io_error in singleton::enqueue recovers it for the named message + hint; MachportCreationFailed/Unexpected fall through to std::io::Error::other and take the OS-description arm. The Windows note (its waker cannot fail, and its raw_os_error space is Win32 codes) is accurate.
  • The author already flagged the handshake as "the part that deserves a maintainer look" and reports CI green on 82ebc6f (192 jobs, x64-asan included); the tip b4f211f only adds the remaining per-thread comment fix in src/io/lib.rs.
  • The PR description mentions bun_bundler::Error gaining an Io variant and bun_io::Error::name() becoming pub, neither of which is in the diff — those appear to be from an earlier iteration before waker_error_to_io converted to std::io::Error instead. Not a concern for the code, just stale prose in the collapsed "Original description".

@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Nothing changed since this review; it matches the current tip (b4f211f). On the one remark: the Error::Io variant and the bun_io::Error::name() visibility change are mentioned only inside the collapsed "earlier iterations" block, which is labeled as the superseded shape; the visible Fix section describes the current std::io::Error conversion. The handshake ordering (write before set(), read after wait(), join() before the free) remains the part for a maintainer to confirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant