RuntimeTranspilerStore: don't report TerminationException as uncaught on worker.terminate() - #36581
RuntimeTranspilerStore: don't report TerminationException as uncaught on worker.terminate()#36581robobun wants to merge 2 commits into
Conversation
… on worker.terminate()
A worker dynamic-importing in a loop and terminated mid-iteration has an
in-flight TranspilerJob whose JS-thread completion (AsyncModule::fulfill)
raises the TerminationException at the from_js_host_call_generic trap check.
run_from_js_thread handed that to report_uncaught_exception_from_error, which
has no termination filter, so Bun__handleUncaughtException ran
process->get("_fatalException") on a terminating VM. The static-hash-table
reification inside that lookup transitions process's Structure mid-walk and
trips ASSERT(object->structure() == this) in Structure::storedPrototype
(debug-build SIGABRT).
Route the error through report_error_or_terminate instead (the same helper the
AnyTask/ManagedTask/CppTask dispatch arms use), which recognises both
JsError::Terminated and a pending TerminationException and propagates
JsTerminated to unwind the tick loop. run_from_js_thread now returns
Result<(), JsTerminated> so dispatch can short-circuit.
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughRuntime transpiler jobs now propagate JavaScript termination errors through event-loop dispatch. A debug-only worker regression test covers repeated termination during invalid dynamic imports and verifies clean process completion. ChangesWorker termination handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — routes TranspilerJob completion errors through the termination-aware report_error_or_terminate helper, matching the AnyTask/ManagedTask/CppTask arms.
What was reviewed:
- Confirmed
report_error_or_terminatefilters bothJsError::Terminatedandis_termination_exception()before falling through to the samereport_uncaught_exceptionthe old path used, so non-termination errors are still reported. dispatch::run_taskis the only caller ofRuntimeTranspilerStore::run_from_js_thread;?-propagation there matches every sibling arm.- Early-return on termination mid-batch mirrors the pre-existing
drain_microtasks_with_globalearly-return, so no new job-leak path. - Test follows the file's existing debug-gated stress-test pattern (tempDir, concurrent pipe drain, combined
{stderr, stdout, exitCode, signalCode}assert).
Extended reasoning...
Overview
Two-line Rust change plus a return-type widening: RuntimeTranspilerStore::run_from_js_thread now hands JsError from TranspilerJob::run_from_js_thread() to crate::task::report_error_or_terminate (instead of report_uncaught_exception_from_error) and returns Result<(), JsTerminated>. dispatch::run_task ?-propagates it. A debug-gated stress test is added to worker-terminate-lifetime.test.ts.
Security risks
None. This is error-routing on the worker-termination path; no untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-medium. The change is mechanical — swapping one error-reporting helper for its termination-aware sibling — and copies the exact pattern already used by the AnyTask, ManagedTask, and CppTask arms in the same match (dispatch.rs:270-285). I verified report_error_or_terminate (Task.rs:43-65): it short-circuits on JsError::Terminated and on is_termination_exception(), otherwise reaches report_uncaught_exception — identical to the old path for genuine exceptions. The drain_microtasks_with_global .is_err() → return was already returning Result<(), JsTerminated>, so ? is a straight rewrite. The new early-return on termination after the first job leaves later batch jobs un-run, but that matches the pre-existing mid-loop drain_microtasks early-return and the VM is tearing down anyway.
Other factors
- Single caller of the changed signature (grep-verified), updated in the same PR.
- The PR body demonstrates a traced root cause (static-hash-table reification transitioning
processStructure mid-prototype-walk while the TerminationException is armed) and verified fails-before/passes-after (3/3 abort → 5/5 survive). - Test uses
tempDir/bunEnv/bunExe, drains stdout/stderr/exited concurrently, assertssignalCode: null, and isskipIf(!isDebug)(the assert is debug-only). ThesetTimeoutdelays are the deliberate race-window sweep, documented in the comment;timeout * 2(120 s on debug) has headroom over the ~8-25 s expected wall-clock for 2 lanes × 100 rounds. - The fix sits at the layer that owns the invariant (the dispatch site should recognise termination), consistent with REVIEW.md guidance and the note about #35678 in the PR body.
Addresses review: the previous version started the terminate timer at
new Worker() with delays capped at 139ms, while debug+ASAN worker startup
measured 112-137ms, so on a slower host no round would reach the import
loop. Waiting for the ready postMessage guarantees every round is inside
the while(true){await import()} when terminate lands, which lets the round
count drop (100 -> 60) and the delay sweep shrink (20-139 -> 0-29). Also
moves w.mjs and main.mjs into the tempDir object with relative ./modN
imports instead of writeFileSync from inside the -e child.
|
Self-review addressed in 0b45bbe:
Re-verified: 5/5 abort with the CI build 86443: the new test passes on all lanes. Remaining failures are unrelated:
Diff is green; ready for a maintainer. |
There was a problem hiding this comment.
LGTM — routes the transpiler-job error through the same report_error_or_terminate helper the sibling AnyTask/ManagedTask/CppTask arms already use, so TerminationException now propagates JsTerminated instead of reaching Bun__handleUncaughtException.
What was reviewed:
report_error_or_terminatestill callsreport_uncaught_exceptionfor genuine non-termination errors, so the observable behavior for real exceptions is unchanged.dispatch.rsis the only caller ofRuntimeTranspilerStore::run_from_js_thread; the return-type change is fully covered.- Mid-batch early-return on
JsTerminatedalready existed via thedrain_microtasks_with_global.is_err()path — no new leak of un-drainedTranspilerJobs. - Test keys terminate off a
postMessage("ready")signal and assertssignalCode: null, so it fails on the abort rather than timing out.
Extended reasoning...
Overview
Two-line Rust change in RuntimeTranspilerStore::run_from_js_thread: swap report_uncaught_exception_from_error for crate::task::report_error_or_terminate (×2 call sites), change the return type from () to Result<(), JsTerminated>, and ?-propagate at the one caller in dispatch::run_task. The drain_microtasks_with_global result, previously mapped to a bare return, is now ?-propagated too. A new debug-gated stress test in worker-terminate-lifetime.test.ts reproduces the Structure::storedPrototype assert.
Security risks
None. This is worker-termination unwind plumbing; no untrusted-input parsing, auth, or crypto touched.
Level of scrutiny
Low-to-moderate. The fix is a mechanical alignment with the pattern already used by three adjacent dispatch arms (AnyTask, ManagedTask, CppTask) in the same match. I confirmed report_error_or_terminate (src/jsc/Task.rs:43) checks both JsError::Terminated and is_termination_exception() before falling through to report_uncaught_exception, so a real thrown exception from AsyncModule::fulfill still surfaces. The signature change has exactly one caller, updated in the same diff.
Other factors
- The mid-batch early-return concern (jobs left in
batch.iterator()when?fires) is not new: the old code already early-returned ondrain_microtasks_with_global().is_err(), and this only happens during VM teardown. - The test was re-verified after self-review (0b45bbe) to abort 5/5 unfixed and survive 5/5 fixed, with terminate keyed off the worker's ready signal to avoid the debug+ASAN startup-latency race.
- The PR description acknowledges overlap with #35678 (guard in
Bun__handleUncaughtException) and correctly argues this fixes the dispatch site rather than the symptom.
|
Update from a fresh investigation on current main (9008ae7, which includes the Worker lifetimes rework from #37075): this door is still open and its crash signature has changed. The same workload (workers looping dynamic import() of fresh query-string specifiers, terminated 20-200ms in) now dies with Captured stack (first one for this door, debug build): Mechanism: after terminate(), the worker's TerminationException stays pending across the FFI boundary (tryClearException refuses to clear it), while JSC's entry-scope exit clears hasTerminationRequest once the NeedTermination trap has been serviced (VM.cpp, executeEntryScopeServicesOnExit). Any DeferTermination scope opened in that state asserts. The transpiler completion's error path is what walks into one: it reports the termination as an uncaught exception, and Bun__handleUncaughtException materializes the lazy process object, whose LazyProperty initializer opens DeferTerminationForAWhile. I verified two things on current main:
One small note on the diff: on the early-return paths the jobs already popped from the queue but not yet run are stranded until VM teardown (their slots, Strong promise handles and poll refs are only reclaimed when the hive drops). release_queued_jobs_for_teardown only drains the queue, and these jobs are no longer in it. Releasing the remainder the same way that function does (promise.deinit, reset_for_pool, store.put) would keep the terminate path tidy. Related: #35678 fixes the second half of this (the guard ordering inside Bun__handleUncaughtException itself); each fix independently stops the crash above, and both are worth landing. |
1 participant
Repro
A worker dynamic-importing in a loop and terminated mid-iteration aborts the process on debug builds:
Stack (raw-stack symbolized; gdb DWARF is broken on
bun-debug):Cause
When
worker.terminate()lands between aTranspilerJobdispatching back to the JS thread and itsAsyncModule::fulfillcall completing,from_js_host_call_generic's post-FFI trap check raises the TerminationException and returnsErr(JsError::Thrown).RuntimeTranspilerStore::run_from_js_threadhanded that straight toreport_uncaught_exception_from_error, which has no termination filter (unlikereport_active_exception_as_unhandled/report_error_or_terminate), soBun__handleUncaughtExceptionran itsprocess->get("_fatalException")prototype walk with the TerminationException still armed._fatalExceptionis aFunctionentry in Process's static hash table.getOwnNonIndexPropertySlot→getOwnStaticPropertySlot→setUpStaticFunctionSlotreifies it viaputDirectNativeFunction, which transitionsprocess's Structure. TheStructure*loaded at the top ofgetPropertySlot's loop is then stale whenstructure->storedPrototype(object)checksobject->structure() == this, tripping the assert.Fix
Route the error through
report_error_or_terminate(the same helper theAnyTask/ManagedTask/CppTaskdispatch arms already use), which recognises bothJsError::Terminatedand a pendingTerminationExceptionvalue and propagatesJsTerminatedto unwind the tick loop instead of reporting.run_from_js_threadnow returnsResult<(), JsTerminated>sodispatch::run_taskcan?-propagate it like the other arms.A genuine non-termination exception from
AsyncModule::fulfillstill reachesreport_uncaught_exceptioninsidereport_error_or_terminate.Verification
New debug-gated stress test in
test/js/web/workers/worker-terminate-lifetime.test.tsspawns 2 lanes × 100 workers, each dynamic-importing syntax-error modules in a loop and terminated after 20-140 ms. On an unfixed debug build: 3/3 abort with thestoredPrototypeassert within 3-5 s. With the fix: 5/5 survive.The imported modules intentionally have a syntax error so the fetch promise rejects and the
ModuleLoadTopSettledmicrotask takes its Rejected branch (noloadModule→hostLoadImportedModule→resolve()), avoiding the separate WebKit-levelscope.assertNoException()termination bug incontinueDynamicImport(tracked separately). The test uses the global WebWorkerfor the same reason:node:worker_threadsinjects a"node:worker_threads"preload whoseload_preloadsspin uses the non-termination-awarewait_for_promiseand independently hits that WebKit assert.Related
TranspilerJobUAF (VM freed while the pool-side parse is still reading it). That PR's body notes thisstoredPrototypeassert as out-of-scope; this is the follow-up.scriptExecutionStatus() != Runningguard inBun__handleUncaughtExceptionitself, which would also happen to stop this symptom; this PR fixes it at the source (the dispatch site that should not be reporting termination as uncaught in the first place) and matches the existingreport_error_or_terminatepattern.continueDynamicImportassertNoException()bug (also surfaced by the original multi-lane repro once this assert no longer fires first) lives inJSModuleLoader.cppin the prebuilt WebKit and is tracked separately.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