Repository navigation
Worker / console / --hot: remove unsafe from web_worker, ConsoleObject, hot_reloader - #40420
Jarred-Sumner wants to merge 7 commits into
Conversation
|
Updated 4:43 PM PT - Sep 6th, 2026
❌ @Jarred-Sumner, your commit 010bb5b has 3 failures in
🧪 To try this PR locally: bunx bun-pr 40420That installs a local version of the PR into your bun-40420 --bun |
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues (the earlier dead-code nit was addressed in 88575b9). Given the scope — worker lifecycle/refcounting rework, the deliberate move of dir-cache busts to the JS thread, and the new BoxedTask/WorkerVm ownership patterns — a human look is still warranted.
Checked: Task::enqueue no longer resets self.count after posting, but the only post-enqueue caller either resets it explicitly (append) or drops the task (on_file_update tail); the VisitedRemove → post-match map.remove conversion preserves error-path behavior since ? above the guard was already unguarded; IoWriterAdapter::interface keeps whole-struct provenance via from_mut(self).cast where the old &mut self.head did not.
Extended reasoning...
Overview
26 files across bun_jsc, bun_runtime, bun_event_loop, bun_io, bun_watcher, bun_libuv_sys, and one C++ binding. The core work is removing ~130 unsafe blocks from four modules by restructuring ownership: WebWorker moves from ThreadSafeRefCounted + raw pointers + thread-confined Cells to Arc + ThreadBound + Guarded with worker-thread state hoisted into a stack-owned WorkerThread; ConsoleObject replaces raw-pointer Restore/Decrement/VisitedRemove RAII guards with a single Scoped deref guard and typed ForEachContext trampolines; hot_reloader becomes a WatcherHandler that never dereferences its Ctx off-thread, posting boxed tasks via a new unsafe trait BoxedTask. Supporting infrastructure: WorkerVm (#[must_use] + asserting Drop), IoWriterAdapter, Timer::one_shot, FileUpdateBatch, and codegen changes to accept explicit lifetimes on rust-ABI HOST_EXPORTs.
Security risks
None identified. No user-input parsing, auth, or crypto paths are touched. The C++ ABI change (WorkerMessagingProxy.cpp now passes BunString arrays that are copied before return) narrows the trust surface — the worker no longer holds borrowed StringImpl* into parent-thread storage.
Level of scrutiny
High. This is production-critical concurrency code: worker thread lifecycle, VM teardown ordering, cross-thread refcounting, and hot-reload task posting. The PR contains at least three design decisions a maintainer should sign off on: (1) the cpp_ref: Guarded<Option<Arc<WebWorker>>> self-reference pattern for the C++ proxy's ref (vs. the previous derive-based refcount), (2) the deliberate behavioral change moving directory-cache busts from the watcher thread to the owning JS thread — with busts dropped entirely for bun build --watch, and (3) the BoxedTask unsafe-trait + boxed_task! macro as the tag↔type pairing mechanism. It is also stacked on an unlanded PR (#40200).
Other factors
The prior automated finding (orphaned BundleV2::bust_dir_cache) was addressed. Test coverage per the description is thorough (worker/console/hot suites, ASAN, hand-driven stress), and perf is measured byte-identical. CI on the latest commit shows two failures whose relation to this change is unclear (Windows test-cpu-prof-dir-relative.js segfault, aarch64 bun-serve-html-build-holds-server). The print_as refactor drops the RAII-on-error-path guarantee for map.remove / map.insert restoration, but I traced the affected sites and the pre-existing code already had unguarded ? returns above those guards, so behavior is preserved.
79b151e to
3c7b365
Compare
3996d35 to
6af78a4
Compare
There was a problem hiding this comment.
Re-reviewed after the latest pushes and didn't find any bugs; the orphaned BundleV2::bust_dir_cache forwarder I flagged earlier is now deleted. Given the size of the ownership/thread-affinity rework across web_worker/hot_reloader/VmHandle, a human pass is still worthwhile.
What was reviewed:
Watcher::dispatch_file_updatesswapping the RAIIlock_guard()for manuallock()/unlock()aroundhandler.take()— the watcher thread is the only mutex holder here, so a panic doesn't strand another thread; ruled out.Timer::one_shot's(*handle).close(free)for narrowed-provenance autoref (theTty::closefootgun) —handleis the leakedBox's raw pointer, not a field reborrow, so provenance covers the allocation; ruled out.- Confirmed no remaining
bust_dir_cachecallers onBundleV2after the forwarder removal.
Extended reasoning...
Overview
This PR is a ~5,000-line safety refactor removing unsafe blocks from web_worker.rs, ConsoleObject.rs, hot_reloader.rs, and Debugger.rs by introducing typed cross-thread task abstractions (BoxedTask/TaskHop in ConcurrentTask.rs, post_with/post_hop/post_boxed in VmHandle.rs), a safe WorkerMessagingProxy FFI wrapper module, an owned WatcherHandler trait on bun_watcher, and assorted call-site updates across ~30 files. Since the prior review, six commits landed: the dead BundleV2::bust_dir_cache forwarder was removed (addressing my earlier inline), the BoxedTask/TaskHop shape was rebased onto the napi-lifecycle form with a refused hook, the source-lint inventory was regenerated, and a stray Windows import was dropped.
Security risks
No direct attack-surface changes — this is internal ownership plumbing. The risk profile is memory safety: cross-thread task posting, worker VM lifecycle (Arc<WebWorker> + ThreadBound<VirtualMachine> + C++ cpp_ref slot), FFI signature changes in WorkerMessagingProxy.cpp, and the deliberate behavioral change moving directory-cache busts from the watcher thread to a posted task on the owning JS thread (with busts dropped for bun build --watch, which has no event loop). REVIEW.md's memory-safety section applies heavily here — refcount balance on every terminal path, thread affinity of every line, and JS-heap operations only on the JS thread.
Level of scrutiny
High. The change touches exactly the categories REVIEW.md calls the most-blocked: cross-thread lifetime, refcounting across FFI, and refactors that must be proven behavior-preserving (worker shutdown order is stated as unchanged but that is a claim to verify). The Watcher::dispatch_file_updates rewrite trades an RAII guard for manual lock/unlock to allow handler.take() + &mut Watcher reborrow — reasonable given the borrow-checker constraint, and panic-safety was examined and ruled out (single-threaded mutex holder). The Timer::one_shot provenance concern was also checked against the documented Tty::close autoref footgun and does not apply.
Other factors
The PR description reports debug+ASAN and release test parity with main across worker/console/hot/watch suites, plus rust-check-all on windows-msvc and aarch64-darwin. No CHANGES_REQUESTED reviews are outstanding; the only resolved thread was my own inline, now addressed by a commit. This is far too large and lifetime-sensitive to auto-approve, but nothing new blocks it from this run.
29b8012 to
5a008e9
Compare
4d65960 to
d8bb409
Compare
102af55 to
ed34be2
Compare
WebWorker is an Arc shared by the C++ proxy (typed cpp_ref slot), its thread and the parent's child_workers; worker-thread state lives in a WorkerThread owned by thread_main and the VM in a WorkerVm handle. The console object is a Box on the VM; formatter scope state is restored by a Deref guard instead of raw-pointer guards; forEach callbacks go through typed ForEachContext/ForEachPropertyContext trampolines; C++ entry points are HOST_EXPORTs. The hot reloader is the watcher's WatcherHandler and never touches its context off-thread: reloads and directory-cache busts are posted to the owning thread as boxed tasks (BoxedTask) and coalesced there.
…refused + ConcurrentTask::create_boxed), FSWatchTask and the reload tasks go through it; EncodedSlice/Utf8Bytes spellings
…anges; no dead_code escape in the ConcurrentTask test
… impl (napi-lifecycle shape); FSWatchTask and the reload task converted
d8bb409 to
010bb5b
Compare
What
Same programme as #40055 … #40411. Stacked on #40200 (base branch
claude/watchers-zero-unsafe, forThreadBound/VmHandle::post_boxed); retarget to main once that lands. Sharesbun_watcher::WatcherHandlerwith #40255 (same hunk).src/jsc/web_worker.rs44 → 1,ConsoleObject.rs47 → 1,hot_reloader.rs35 → 1,Debugger.rs14 → 4 — every residual is an all-safe fnextern block. Collateral:jsc_hooks.rs−4,run_command/test_command/hw_exports−6,VirtualMachine.rs−2,shell/interpreter.rs−2.WebWorkerisArc-shared andSend + Syncby construction (BackRef<WorkerMessagingProxy>,ThreadBound<VirtualMachine>, atomics,Guarded); the C++ proxy's ref is a typedcpp_refslot released byWebWorker__deref(ThisPtr); worker-thread-only state (WorkerVm, arena, env loader, status) lives in aWorkerThreadowned bythread_main;bun_jsc::virtual_machine::WorkerVm(#[must_use], assertingDrop,teardown_and_free) owns the worker VM; argv/execArgv are copied atcreate()(C++ passesBunStringarrays); allWebWorker__*areHOST_EXPORTs; the one thread-affine proxy call goes throughThreadBound::get(). Shutdown order unchanged.Bun__ConsoleObject__*areHOST_EXPORTs over&[u8]/&[JSValue]; the raw-pointer Restore/Decrement/VisitedRemove guards become oneScopedderef guard;JSValue::for_each_*_ctxtyped trampolines replace*mut c_voidctx;with_console(|c| ..)/console_writer()instead of an unbounded&mut ConsoleObject;bun_io::IoWriterAdapter.WatcherHandlerand never touches itsCtxoff-thread; the reloadTaskcarriesThreadBound<Ctx>+Arc<Pending { count, dirs }>and is posted withVmHandle::post_boxed;bun_event_loop::BoxedTaskis anunsafe traitwhose tag↔type pairing lives inboxed_task!, and only theVirtualMachineinstantiation gets a dispatch tag. Deliberate change: directory-cache busts now run on the owning JS thread inside the posted task (coalesced, drained before the reload; a bust-only task is posted when no watched file changed) instead of racing from the watcher thread; forbun build --watch(no event loop) busts are dropped.bun_libuv_sys::Timer::one_shot(Windows debugger wait),generate-host-exports.tsaccepts explicit lifetimes onrust-ABI exports.Pre-existing, unchanged: worker
process.argv[1]is the worker file rather than the parent's main (two tests expect Node's behaviour and fail on main too).Perf (release,
taskset -c 56-63,perf stat -e instructions:u, 5 interleaved, medians; output byte-identical): 1e5×console.log({a:1,b:[1,2,3],c:"str"})+0.05 %; 100× 100×100 nested array −0.10 %; 2000× depth-50console.dir−0.06 %.Testing
Debug+ASAN:
test/js/web/workers+test/js/node/worker_threads606 pass / 5 red = 2 argv/execArgv (identical on main), 1 debug-timing message-flood, 2 LSAN cases needing a symbolizer; release build: base 507/3, this 507/3 (same three). Console/inspect suites pass (inspect-error-leakdebug timeout passes on release).test/cli/hot+test/cli/watch30/30. Hand-driven: 50 workers × 10 create/terminate (RSS profile matches main),process.exit(42)inside a worker, terminate during a postMessage flood, 3-deep nested terminate, unref lets the process exit; console circular/Proxy/throwing-getter/%s %d %o/table/group/count/time byte-identical to main;bun --hot30 rapid edits → 30 generations; new-file-then-import under--hot×5;--watch5 edits → 6 runs — no ASAN output. clippy clean on bun_jsc/bun_runtime/bun_event_loop/bun_io/bun_libuv_sys/bun_watcher/bun_bundler;rust-check-allwindows-msvc + aarch64-darwin pass.