Bun.sleepSync: make worker.terminate() interrupt a worker blocked in sleepSync - #35103
Bun.sleepSync: make worker.terminate() interrupt a worker blocked in sleepSync#35103robobun wants to merge 1 commit into
Conversation
…can interrupt it A worker blocked in Bun.sleepSync(N) could not be terminated until N elapsed: sleep_sync was a single std::thread::sleep, and worker.terminate()'s only wakeups are a JSC VMTrap (checked at JS safepoints) and an event-loop poke, neither of which unblocks a parked nanosleep/Sleep. In a worker, slice the sleep and check WebWorker::has_requested_terminate() between slices; terminate() now takes effect within one slice. Main-thread sleepSync is unchanged.
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 5:57 AM PT - Jul 22nd, 2026
❌ @robobun, your commit da0b4ac has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35103That installs a local version of the PR into your bun-35103 --bun |
There was a problem hiding this comment.
LGTM — focused fix that slices sleepSync in workers so terminate() can take effect within 100 ms.
What was reviewed
millisecondsis a non-negativei32before theas u64cast, so the switch fromtry_from().expect()is behavior-preserving;Instant + durationcannot overflow at ≤ i32 ms.worker_ref()lifetime is documented to outlive the VM andhas_requested_terminate()is an atomic Acquire load, so the cross-thread poll is safe.- Deadline-based loop preserves total sleep accuracy; terminate-before-sleep race is handled by the top-of-loop flag check.
- Test follows the file's existing spawn/drain/assert pattern, is hermetic, and uses the shared
timeout.
Extended reasoning...
Overview
Two-file change: src/runtime/api/BunObject.rs replaces the single std::thread::sleep in sleep_sync with a deadline-based loop when running on a worker thread, polling WebWorker::has_requested_terminate() between 100 ms slices. Main-thread behavior is unchanged. A regression test is added to test/js/web/workers/worker-terminate-lifetime.test.ts that verifies worker.terminate() unblocks a worker parked in Bun.sleepSync(600000).
Security risks
None. No untrusted input handling changes — argument validation (number check, negative check) is untouched. The only new state read is an existing AtomicBool via an Acquire load, which is the intended cross-thread contract for requested_terminate. worker_ref() returns a &WebWorker whose backing allocation is documented to outlive the VM (src/jsc/VirtualMachine.rs:910-917), and this call happens on the worker's own thread, so there is no lifetime hazard.
Level of scrutiny
Medium. It touches worker-thread coordination, but the change is additive and narrow: it reads one existing atomic flag between sleep slices. I checked the as u64 cast (safe: milliseconds: i32 is validated >= 0 immediately above), Instant::now() + duration overflow (max i32 ms ≈ 24.8 days, well within platform limits), and the terminate-races-sleep-entry case (the flag is checked before each slice, so an early terminate returns immediately).
Other factors
The test mirrors the four existing tests in the same file exactly (subprocess spawn, concurrent stdout/stderr/exited drain, exact-string asserts, shared timeout). The PR description reports it fails under USE_SYSTEM_BUN=1 and passes on the debug build, and that test/js/bun/util/sleepSync.test.ts still passes. The deadline loop (vs. accumulating fixed slices) preserves total sleep duration accuracy, which the description also verified empirically.
|
CI on build #77671: 191 lanes passed, 3 failed, 2 darwin lanes still queued. The new test in The three failed lanes are debian 13 x64 and two ubuntu 25.04 x64 shards, each on one of these GC-timing tests:
Both fail intermittently on This diff is ready for review. |
Problem
worker.terminate()never completes while the worker thread is parked inBun.sleepSync(N). Thecloseevent does not fire until the fullNmilliseconds elapse, so a supervisor that relies onterminate()to bound hostile code cannot reclaim a worker that callsBun.sleepSyncwith a large argument.Cause
sleep_syncis a singlestd::thread::sleep. The parent-threadWebWorker::notify_need_terminationonly setsrequested_terminate, fires the JSCNeedTerminationVMTrap (serviced at JS safepoints), and wakes the uws/uv event-loop poll. None of those can unblock a thread parked innanosleep/Sleep, so the worker stays asleep for the full duration.Fix
When
sleepSyncruns on a worker thread, slice the sleep against anInstantdeadline and pollWebWorker::has_requested_terminate()between slices. Onceterminate()sets the flag,sleepSyncreturns at the next slice boundary and the already-fired VMTrap throws theTerminationExceptionat the next JS safepoint. Main-threadsleepSynckeeps the single uninterruptible sleep (there is nothing to poll).The slice is 100 ms: termination latency is bounded by that, and the extra wakeups are negligible (a 1 h sleep wakes ~36000 times, microseconds each).
Verification
New test in
test/js/web/workers/worker-terminate-lifetime.test.ts:USE_SYSTEM_BUN=1 bun test -t "Bun.sleepSync"HUNG)bun bd test -t "Bun.sleepSync"test/js/bun/util/sleepSync.test.ts(5 tests) passes unchanged, andsleepSync(250)in a worker still measures 250 ms (deadline loop preserves accuracy).Related
This came out of a supervisor kill-switch audit that also flagged
Atomics.wait(#32802) and nested-worker orphans. Those are independent changes.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker-terminate-lifetime.test.ts