worker: re-throw TerminationException from Bun__reportUnhandledError so terminate() breaks a microtask-bound ReadableStream loop - #36806
Conversation
…so terminate() breaks a microtask-bound ReadableStream loop InternalMicrotask::BunPerformMicrotaskJob (the runner for queueMicrotask and the C++ stream start/pull reaction jobs) catches any exception from the job callback with an unconditional clearException() and then calls Bun__reportUnhandledError with the Exception*. When the caught exception is the TerminationException that clear consumes the one shot the NeedTermination trap fired, so the microtask drain never observes termination and a worker in a microtask-bound loop that constructs a JS-source ReadableStream (or calls queueMicrotask) each turn spins forever with terminate() never resolving. Bun__reportUnhandledError already skipped reporting a TerminationException; now it re-throws it via VM::throwTerminationException so MicrotaskQueue::runMicrotask sees it on return and breaks out of the drain. Also: check_body_stream_ref (called from Response::constructor / Request::construct_into after the native object has been heap-allocated) was re-tagging the stored stream via ReadableStream::from_js, whose trap-handling exception check is a VMTraps safepoint. A TerminationException thrown there left the generated construct holding a non-null ptr with an exception pending, tripping its 'Memory leak detected: new Response()' assertion. check_body_stream_ref only needs the raw JSValue, so read it directly from the Strong's slot.
|
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)
WalkthroughWorker termination exceptions now propagate through JSC VM, event listener, and error reporting paths. ReadableStream migration avoids JavaScript conversion during cleanup. Regression tests cover workers trapped in microtask loops. ChangesWorker termination handling
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:39 PM PT - Aug 2nd, 2026
❌ @robobun, your commit c5e47d4 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36806That installs a local version of the PR into your bun-36806 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Re the find-issues suggestions: this PR does not fix #34690 or #34095. Those are different termination races (ExceptionScope::assertNoException during worker bootstrap and |
…est subprocess after PASS innerInvokeEventListeners uses the NakedPtr<Exception>& call() overload which clears the exception unconditionally. Now that Bun__reportUnhandledError re-throws a TerminationException, continuing the loop into the next listener's call() would trip executeCallImpl's assertNoException(). Break out of the loop when the caught exception is the TerminationException. Also add process.exit(0) after PASS in the new test's subprocess so the losing Bun.sleep() timers don't hold the event loop open for the full deadline.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/workers/worker-terminate-lifetime.test.ts`:
- Around line 217-220: Update the worker startup flow around the Worker instance
and message wait so the startup promise rejects on the first error event instead
of discarding it. Use one-shot message and error handlers, remove the alternate
listener when either settles, and preserve visibility of any worker errors
occurring after successful startup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5542a50-8303-4424-949e-a3897f6cf92e
📒 Files selected for processing (6)
src/jsc/bindings/bindings.cppsrc/jsc/bindings/webcore/EventEmitter.cppsrc/jsc/virtual_machine_exports.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ReadableStream.rstest/js/web/workers/worker-terminate-lifetime.test.ts
|
CI on build 87992: the new tests in |
There was a problem hiding this comment.
The two issues I raised earlier are addressed and I didn't find anything new, but this changes Bun__reportUnhandledError's post-condition (may now return with a pending TerminationException) across every caller, so it's worth a human look before merge.
What was reviewed:
- Audited every
Bun__reportUnhandledErrorcaller for the new re-throw: napi/BunProcess/EventEmitter:193/NodeTimerObject:40 pass non-TerminationException values; NodeTimerObject:65 usestryClearExceptionso the guard makes the re-throw a no-op;jsFunctionPerformMicrotaskVariadicandreportUncaughtExceptionAtEventLoopreturn immediately after, where propagating is the intent. check_body_stream_ref:Strong::value()returns the sameJSValuethe oldstream.valuecarried, minus thefrom_js_host_call_genericsafepoint;ensure_still_aliveanddowngrade()ordering preserved.- Test: startup promise now rejects on error/early-exit;
process.exit(0)after PASS drops the idle sleep tail.
Extended reasoning...
Overview
The PR fixes worker.terminate() never resolving when the worker is in a microtask-bound loop that goes through BunPerformMicrotaskJob (queueMicrotask, C++ stream start/pull reactions). It touches: bindings.cpp (new JSC__VM__rethrowTerminationException, guarded on hasTerminationRequest() && !hasPendingTerminationException()), virtual_machine_exports.rs (report_unhandled_error now re-throws instead of silently skipping a TerminationException), EventEmitter.cpp (breaks the listener loop on TerminationException — added in response to my earlier review), Body.rs/ReadableStream.rs (avoid the from_js VMTraps safepoint in check_body_stream_ref post-allocation), and a new 5-variant describe block in worker-terminate-lifetime.test.ts.
Security risks
None. This is termination-exception propagation inside a worker VM being torn down; no auth, crypto, or untrusted-input parsing is involved.
Level of scrutiny
High. TerminationException handling is one of the most fragile areas in the JSC integration — the timeline references at least four prior PRs (#35976, #36331, #36579, #36581) touching adjacent paths, and my first-pass review of this PR found a real regression (EventEmitter listener loop re-entering call() with the exception pending). The core change alters a function's post-condition for every caller. I re-audited each caller after the EventEmitter fix and they look correct now, but a maintainer familiar with the BunPerformMicrotaskJob / MicrotaskQueue::runMicrotask interaction should confirm the re-throw is the right layer (the PR itself notes the JSC-side clearException() should eventually become clearExceptionExceptTermination()).
Other factors
All prior feedback (mine, CodeRabbit's startup-rejection point, comment-cop) is resolved. The test covers five loop shapes with a timing sweep and asserts {stderr, stdout, exitCode} in the right order. CI build #87992 was still running at review time. The Body.rs change is a semantics-preserving simplification: it reads the same stored JSValue without the ReadableStreamTag__tagged round-trip, so no source-tag reload happens — but the caller only ever needed the value for stream_set_cached, never the tag.
Problem
worker.terminate()never resolves when the worker is in a microtask-bound loop that constructs a JS-sourceReadableStream(or callsqueueMicrotask) each turn. The worker thread keeps spinning at ~0.5-0.7 core forever, noexitevent fires, and a secondterminate()also never resolves.Reproduces on main since the C++ streams rewrite (#33193); Node terminates the identical worker in a few ms.
Cause
InternalMicrotask::BunPerformMicrotaskJob(the runner forqueueMicrotaskand the C++ stream start/pull reaction jobs added in #33193) catches any exception from the job callback with an unconditionalclearException()and then callsBun__reportUnhandledErrorwith theException*:When the caught exception is the
TerminationException, that clear consumes the one shot theNeedTerminationtrap fired (the trap bit was already cleared byVMTraps::handleTraps), soMicrotaskQueue::runMicrotask'sclearExceptionExceptTermination()check never trips and the drain keeps processing microtasks forever.A plain
for(;;){ await 0 }loop terminates because the promise-reaction microtask path goes throughrunMicrotask's termination-aware clear; it's onlyBunPerformMicrotaskJobwith its own catch scope that swallows it.Separately, under debug/ASAN the
new Response(new ReadableStream({...}))shape aborts with:because
check_body_stream_ref(called fromResponse::constructor/Request::construct_intoafter the native object has been heap-allocated) re-tagged the stored stream viaReadableStream::from_js, whose Rust-side post-call exception check is a VMTraps safepoint. ATerminationExceptionthrown there left the generatedconstructholding a non-nullptrwith an exception pending.Fix
Bun__reportUnhandledErroralready skipped reporting aTerminationException; it now re-throws it viaVM::throwTerminationException(guarded onhasTerminationRequest() && !hasPendingTerminationException()) soMicrotaskQueue::runMicrotasksees it on return and breaks out of the drain. This covers everyBunPerformMicrotaskJobcaller (streams reactions,queueMicrotask, promise-reject deferrals).check_body_stream_refnow reads the storedJSValuedirectly from theStrong's slot instead of going throughReadableStream::from_js; it only needed the value, never the source tag.The
BunPerformMicrotaskJobhandler itself lives in JavaScriptCore's prebuiltJSMicrotask.cppand should eventually useclearExceptionExceptTermination(); this change makesterminate()work correctly regardless.Verification
New
describeblock intest/js/web/workers/worker-terminate-lifetime.test.tsspawns a worker in each of five microtask-bound loop shapes (ReadableStreampull/start,Response/Requestwrapping a stream,queueMicrotask), sweeps a fewterminate()timing offsets, and assertsterminate()resolves. On main the ReadableStream/Request shapes printHUNG round 0and fail; with this change all five pass (144/144 terminations across 3 local stress runs).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