Skip to content

process: skip uncaught-exception dispatch on a terminating worker - #35678

Closed
robobun wants to merge 11 commits into
mainfrom
farm/409c20b8/uncaught-exception-termination-guard
Closed

process: skip uncaught-exception dispatch on a terminating worker#35678
robobun wants to merge 11 commits into
mainfrom
farm/409c20b8/uncaught-exception-termination-guard

Conversation

@robobun

@robobun robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Repro

A worker whose module fails to load (MODULE_NOT_FOUND) reports its error via flush_logs while worker.terminate() and process.exit() on main land mid-dispatch. On the worker's thread, with has_requested_terminate() set:

  • flush_logs's vm_log.to_js / to_bun_string / WebWorker__dispatchError throws the TerminationException → Err(Thrown | Terminated)panic!("unhandled exception") aborts the process
  • if the JS-conversion step survives but WebWorker__dispatchError throws, report_uncaught_exceptionBun__handleUncaughtException lazily creates process and does process->get("_fatalException"), which trips ASSERTION FAILED: isCompilationThread() || Thread::mayBeGCThread() || object->structure() == this in JSC::Structure::storedPrototype

Both are debug-assert / debug-build only; the race is non-deterministic (~2/14 for the assert on release-asan-cov; the panic reproduced on the debian-13 x64-asan CI lane in build 81199). Looped driver:

for i in $(seq 1 40); do bun-asan worker-dataurl-importmeta-url-mangled.mjs >/dev/null 2>err || { head -2 err; break; }; done

Cause

flush_logs panicked on JsError::Thrown | Terminated instead of recognising termination, and Bun__handleUncaughtException (and its siblings Bun__handleUnhandledRejection, Bun__emitHandledPromiseEvent, Bun__promises__emitUnhandledRejectionWarning) had no termination guard. Once a worker's termination has been requested, running the uncaught-exception / unhandled-rejection machinery is wrong: the exception is either the TerminationException itself or was produced while it was pending, and the process->get / emit / call sequence walks the JS heap and may call user code on a VM that is being wound down concurrently with the parent's (or process-wide) teardown. The _fatalException check that landed in ab84aa2 is the first property walk in Bun__handleUncaughtException, which is why the assert is a new signature.

Fix

  • flush_logs: return early when has_requested_terminate() && has_termination_request() (keyed on the JSC-side trap so the same-thread configure_defines() self-signal, which sets only the atomic, still dispatches its error); on JsError::Terminated return; on a genuine JsError::Thrown report it via report_uncaught_exception instead of crashing; after WebWorker__dispatchError throws, re-check has_requested_terminate() before reporting.
  • Guard Bun__handleUncaughtException, Bun__handleUnhandledRejection, and Bun__emitHandledPromiseEvent on scriptExecutionStatus(), the same check used by MessagePort and the same class of guard as dispatchExitInternal in the same file. scriptExecutionStatus is Stopped exactly when a worker's has_requested_terminate() is set (or the VM is shutting down), so the main-thread node:vm watchdog is not caught by it.
  • VirtualMachine::uncaught_exception / unhandled_rejection / handled_promise now gate on script_execution_status() != Running (a strict superset of their previous is_shutting_down() check), which covers the isBunTest fast-path in uncaught_exception and all six --unhandled-rejections mode arms including the emitUnhandledRejectionWarning path that has its own reason.get(stack) prototype walk.
  • WebWorker::spin() gets a has_requested_terminate() && !exit_called checkpoint after the entry-promise status block, so an external terminate that lands during entrySettled / the status block goes straight to shutdown() rather than reaching dispatchOnline/fireEarlyMessages/tick(). The !exit_called carve-out preserves pre-PR 'online' ordering for a worker whose own uncaughtException handler calls process.exit(N).

Verification

The added stress test in test/js/web/workers/worker-terminate-lifetime.test.ts loops the failing-worker + terminate + process.exit() shape 20 times under ASAN. Two levels only (main spawns the failing workers directly), so it does not also trip the nested-worker parent-VM UAF that #31951 addresses; an earlier three-level fixture caught that UAF on the debian-13 x64-asan lane in build 81404. The two-level fixture reproduced the flush_logs panic: unhandled exception SIGABRT on the debian-13 x64-asan lane at c298be5 (before the flush_logs change) and passes with the full diff applied. The storedPrototype assert itself did not fire on this machine's debug+ASAN build in 230 iterations; it was reported at ~2/14 on release-asan-cov.

Also verified unchanged:

  • test/js/node/test/parallel/test-worker-nested-uncaught.js, test-worker-nested-on-process-exit.js, test-worker-beforeexit-throw-exit.js, test-worker-abort-on-uncaught-exception.js, test-worker-exit-code.js (exercises the _fatalException → exit 6 path), test-worker-exit-from-uncaught-exception.js
  • test/js/node/test/parallel/test-promise-unhandled-{default,error,flag}.js, test-promise-handled-rejection-no-warning.js
  • test/js/node/process/process.test.js -t uncaught (including "throwing inside a worker runs that worker's uncaughtException handler", since beforeExit only fires when has_requested_terminate() is false)
  • test/js/node/worker_threads/worker_threads.test.ts (91 pass)
  • test/js/web/workers/worker.test.ts (25 pass)

Related

#31951 addresses the same flush_logs panic plus the nested-worker parent-VM UAF; this PR takes the minimal flush_logs termination handling and additionally guards the downstream handlers and Rust dispatchers so every caller is covered. The parent-VM UAF is left for #31951.


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

When a nested worker's module fails to load and it reports the error via
flush_logs while worker.terminate() + process.exit() land from another
thread, Bun__handleUncaughtException would still lazily create the
process object and walk process->get("_fatalException"). That read has
been observed to trip

  ASSERTION FAILED: isCompilationThread() || Thread::mayBeGCThread()
  || object->structure() == this

in Structure::storedPrototype on the worker thread.

Guard the handler with the worker's scriptExecutionStatus, matching
MessagePort and dispatchExitInternal: a worker that has been asked to
terminate does not run its uncaughtException / uncaughtExceptionMonitor /
capture-callback machinery. The main-thread node:vm watchdog path does
not set has_requested_terminate(), so main-thread behaviour is
unchanged.
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2455869a-de9f-479a-a640-26685161c7a4

📥 Commits

Reviewing files that changed from the base of the PR and between 482c8b9 and f4c12bc.

📒 Files selected for processing (5)
  • src/jsc/VM.rs
  • src/jsc/VirtualMachine.rs
  • src/jsc/bindings/BunProcess.cpp
  • src/jsc/web_worker.rs
  • test/js/web/workers/worker-terminate-lifetime.test.ts

Walkthrough

Changes

Worker termination lifecycle

Layer / File(s) Summary
Startup termination checkpoint
src/jsc/web_worker.rs
Worker startup now flushes logs and shuts down if termination is requested after entry-point loading.
Non-running event dispatch guards
src/jsc/bindings/BunProcess.cpp, test/js/web/workers/worker-terminate-lifetime.test.ts
Process exception and promise rejection events are skipped outside the Running state, with an ASAN regression test covering nested worker termination during module-load failures.

Possibly related PRs

Suggested reviewers: jarred-sumner, cirospaciari

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and directly describes the main change: skipping uncaught-exception dispatch for terminating workers.
Description check ✅ Passed It covers the bug, cause, fix, and verification, including a regression test, even though it doesn't use the exact template headings.

Comment @coderabbitai help to get the list of available commands.

Comment thread src/jsc/bindings/BunProcess.cpp
Comment thread test/js/web/workers/worker-terminate-lifetime.test.ts Outdated
Comment thread src/jsc/bindings/BunProcess.cpp
@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:05 PM PT - Jul 25th, 2026

@robobun, your commit f4c12bc has some failures in Build #81617 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 35678

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

bun-35678 --bun

…uple fixture from data-URL quirk

- Apply the same scriptExecutionStatus guard to Bun__handleUnhandledRejection
  and Bun__emitHandledPromiseEvent (same processObject() + emit() shape; their
  Rust callers only check is_shutting_down, not has_requested_terminate).
- spin(): check has_requested_terminate() after the entry-promise status block
  so a terminate() that lands during load_entry_point_for_web_worker does not
  reach dispatchOnline/fireEarlyMessages/tick(). Mirrors the checkpoints at
  :1083 and the event-loop body, and makes the shutdown decision independent
  of what Bun__handleUncaughtException returns.
- Test fixture: spawn grandchildren from an absolute nonexistent path so
  MODULE_NOT_FOUND does not depend on import.meta.url being the mangled
  file:///data:... form inside a data-URL worker. Fail loudly (exit 1 on
  middle-worker error or timeout) instead of silently falling through.
Comment thread src/jsc/bindings/BunProcess.cpp Outdated
Comment thread src/jsc/bindings/BunProcess.cpp Outdated
Comment thread src/jsc/web_worker.rs Outdated

@coderabbitai coderabbitai 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.

Actionable comments posted: 2

🤖 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 124-140: Replace the multi-line regression explanation above the
test with a single comment containing the issue URL. Remove the
implementation-history and race-detail prose, and preserve the test declaration
and behavior unchanged.
- Around line 132-182: Extend the worker termination test around the existing
subprocess fixture to cover both promise-event guards: an unhandled rejection
during termination and a rejection that is handled later. Add separate race
cases for Bun__handleUnhandledRejection and Bun__emitHandledPromiseEvent,
preserving the existing worker-grandchild structure and assertions, and ensure
the complete termination-event variant matrix is exercised.
🪄 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: b22b3a29-34c6-40bb-9ae2-53e05050674e

📥 Commits

Reviewing files that changed from the base of the PR and between df6c7ee and 482c8b9.

📒 Files selected for processing (3)
  • src/jsc/bindings/BunProcess.cpp
  • src/jsc/web_worker.rs
  • test/js/web/workers/worker-terminate-lifetime.test.ts

Comment thread test/js/web/workers/worker-terminate-lifetime.test.ts Outdated
Comment thread test/js/web/workers/worker-terminate-lifetime.test.ts
Comment thread src/jsc/bindings/BunProcess.cpp Outdated
Comment thread src/jsc/web_worker.rs Outdated

@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.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 src/jsc/bindings/BunProcess.cpp:1367-1368 — The follow-up commit guarded Bun__handleUnhandledRejection and Bun__emitHandledPromiseEvent, but missed Bun__promises__emitUnhandledRejectionWarning (line 1329, immediately above) — which, unlike those two, actually contains the exact JSValue::decode(reason).get(globalObject, vm.propertyNames->stack) prototype-chain walk named in the PR as tripping the assert, plus Process::emitWarning (user JS). It's reached from the same VirtualMachine::unhandled_rejection dispatcher (guards only on is_shutting_down(), not has_requested_terminate()) in Mode::Warn/WarnWithErrorCode/Strict, and now that Bun__handleUnhandledRejection returns false on a terminating worker, control flows into emit_warning() in those modes. Consider adding the same 2-line guard there — or gating unhandled_rejection() itself on has_requested_terminate() so all six mode arms are covered at once.

    Extended reasoning...

    What the finding is

    Commit 482c8b9 responded to the earlier 'fix the whole class' review by adding the scriptExecutionStatus() != Running guard to Bun__handleUnhandledRejection (BunProcess.cpp:1367) and Bun__emitHandledPromiseEvent (:1392). But it missed the third sibling in the same file that is called from the same Rust dispatcher and — unlike the two that were guarded — actually contains the exact ->get() prototype-chain walk that produced the observed assert.

    Bun__promises__emitUnhandledRejectionWarning (BunProcess.cpp:1329-1358, immediately above the newly-guarded Bun__handleUnhandledRejection) does:

    • JSC::createError(globalObject, ...) (:1333)
    • Bun__promises__isErrorLike (:1341) → objectPrototypeHasOwnProperty
    • JSValue::decode(reason).get(globalObject, vm.propertyNames->stack) (:1344) — the same JSObject::getgetPropertySlotStructure::storedPrototype path the PR description names as tripping ASSERT(object->structure() == this)
    • Process::emitWarning / Process::emitWarningErrorInstance (:1354/:1356) — lazy-creates process and runs user JS

    It has no scriptExecutionStatus() guard.

    Reachability on a terminating worker (step-by-step)

    VirtualMachine::unhandled_rejection (VirtualMachine.rs:3282) guards only on is_shutting_down() (:3290), not has_requested_terminate() — the same gap the PR closes for uncaught_exception. On a worker where terminate() has armed has_requested_terminate but is_shutting_down is still false:

    1. unhandled_rejection enters and dispatches on unhandled_rejections_mode().
    2. In Mode::Warn (:3333-3335): handle_unhandled()Bun__handleUnhandledRejectionnew guard returns falseemit_warning(self) is called unconditionally.
    3. In Mode::WarnWithErrorCode (:3339-3342): handle_unhandled() returns falseif !handled { emit_warning(self); ... }.
    4. In Mode::Strict (:3351-3357): uncaught_exception(...) (now guarded, returns true, discarded) → handle_unhandled() returns falseif !handled { emit_warning(self); }.
    5. emit_warningBun__promises__emitUnhandledRejectionWarning.get(globalObject, vm.propertyNames->stack) on a VM whose termination has been requested.

    So the newly-added guard on Bun__handleUnhandledRejection returning false steers control into the unguarded ->get() sibling in all three non-default modes.

    Why existing code doesn't prevent it

    The earlier (now-resolved) review comment noted that the two siblings it named 'lack the ->get() prototype walk that produced the specific observed assert' — that's precisely why they were lower-risk. Bun__promises__emitUnhandledRejectionWarning is the one that does have it, and it wasn't mentioned. The Rust caller's is_shutting_down() check is a different flag from has_requested_terminate() (VM teardown vs. worker terminate request), so it does not short-circuit in this window — the same distinction the PR description itself makes for Bun__handleUncaughtException.

    Impact

    Non-default --unhandled-rejections=warn/warn-with-error-code/strict mode; same debug-assert-only race window as the original bug; no crash proven on this specific path. For Warn/WarnWithErrorCode this is largely pre-existing (a fresh worker with no listeners already had handle_unhandled() return false pre-PR). Per REVIEW.md 'Fix the whole class in the same PR… If a site is intentionally excluded, say so in the PR', it's the one remaining sibling with the exact ->get() shape the PR exists to fix.

    Fix

    Same 2-line guard already applied twice in this commit, at the top of Bun__promises__emitUnhandledRejectionWarning:

    if (Zig::GlobalObject::scriptExecutionStatus(jsCast<Zig::GlobalObject*>(globalObject), globalObject) != JSC::ScriptExecutionStatus::Running) [[unlikely]]
        return;

    Or, more structurally: gate VirtualMachine::unhandled_rejection itself on has_requested_terminate() (alongside the existing is_shutting_down() check at :3290), which covers all six mode arms — including emit_warning, handle_unhandled, and the Mode::Throw arm — at once, and matches the 'prefer moving the guard into the shared helper' guidance.

Covers all six --unhandled-rejections mode arms (including
emitUnhandledRejectionWarning's reason.get(stack) prototype walk and
Process::emitWarning user JS) at the shared Rust dispatcher, rather than
guarding each C++ callee individually. script_execution_status() is
Stopped when is_shutting_down || (worker && has_requested_terminate),
so this is a strict superset of the previous is_shutting_down() check.
@robobun

robobun commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

Re the Bun__promises__emitUnhandledRejectionWarning finding: addressed in d260627 by gating VirtualMachine::unhandled_rejection itself on script_execution_status() != Running (a strict superset of its previous is_shutting_down() check). That covers all six --unhandled-rejections mode arms including emit_warning at the shared dispatcher, so no per-callee C++ guard is needed for that one. PR body updated.

Comment thread src/jsc/bindings/BunProcess.cpp

@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.

d260627 also resolves my earlier note on BunProcess.cpp:1355 — with VirtualMachine::unhandled_rejection now gated on script_execution_status() != Running, the Rust dispatcher returns before ever reaching Bun__handleUnhandledRejection / emitUnhandledRejectionWarning on a terminating worker, so the return false choice there no longer matters. The remaining gap is the uncaught_exception / isBunTest sibling noted inline.

Extended reasoning...

d260627 widened the unhandled_rejection guard at VirtualMachine.rs:3290 to script_execution_status() != Running, and Bun__handleUnhandledRejection has no other Rust caller — so my prior inline comment on BunProcess.cpp:1355 (return-value inconsistency falling through to emitUnhandledRejectionWarning) is moot: the Rust caller short-circuits first. The one place the widened check was not applied is uncaught_exception at :1351, whose isBunTest fast-path at :1355 runs before the guarded FFI call; that's covered by the new inline nit. Still deferring to a human given this is cross-thread worker-VM teardown.

Comment thread src/jsc/VirtualMachine.rs
…tatus

Matches unhandled_rejection (d260627): script_execution_status() is a
strict superset of is_shutting_down() that also returns Stopped for a
worker with has_requested_terminate() set. Covers the isBunTest
fast-path in uncaught_exception (fires before the guarded C++ call;
isBunTest is a process-global static, true on every worker under
bun test).
Comment thread src/jsc/web_worker.rs Outdated
WebWorker::exit() (the worker's own process.exit()) sets both exit_called
and requested_terminate; the checkpoint should only fire for an external
terminate(), so a worker whose uncaughtException handler calls
process.exit(N) still reaches dispatchOnline as before. Verified
test-worker-exit-from-uncaught-exception.js unchanged.

@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.

All three of my earlier findings are now addressed (the emitUnhandledRejectionWarning fall-through, the uncaught_exception/handled_promise sibling widening, and the !exit_called self-exit carve-out in the new spin() checkpoint). No further issues found, but this touches worker-VM-lifecycle and cross-thread termination ordering, so worth a human look.

What was reviewed:

  • script_execution_status() is a strict superset of is_shutting_down() (adds only the worker.has_requested_terminate() case), so main-thread dispatch is unchanged.
  • The C++ guards match the existing MessagePort.cpp pattern; the return false at :1354/:1379 is now moot since the Rust callers short-circuit first.
  • The new spin() checkpoint with && !exit_called preserves dispatchOnline for the entry-throws-then-process.exit()-in-handler case.
Extended reasoning...

Overview

Four files: three scriptExecutionStatus() != Running guards in BunProcess.cpp (Bun__handleUncaughtException, Bun__handleUnhandledRejection, Bun__emitHandledPromiseEvent); three Rust dispatchers in VirtualMachine.rs widened from is_shutting_down() to script_execution_status() != Running; a new post-entry-promise terminate checkpoint in WebWorker::spin(); and an ASAN-gated stress test.

Security risks

None. This is defensive hardening of internal error-dispatch machinery on a terminating worker VM; no user-controllable input paths, auth, or serialization touched.

Level of scrutiny

High. Worker VM lifecycle and cross-thread termination is one of the more delicate areas in the runtime — the file header of web_worker.rs alone documents three distinct UAF windows. The fix is for a non-deterministic race (~2/14 on release-asan-cov, 0/230 on the author's debug+ASAN build), so the test is best-effort regression coverage rather than a proof. The spin() checkpoint changes when dispatchOnline fires, which is observable from the parent.

Other factors

This PR has been through three review rounds, each of which found a real edge case that was then fixed (d260627, e5390ce, c298be5). All are now addressed and the current diff is internally consistent: script_execution_status() at VirtualMachine.rs:986-998 returns Stopped only for is_shutting_down or a worker with has_requested_terminate(), so the Rust widening is behavior-preserving on the main thread; the C++ guard pattern matches existing MessagePort.cpp usage; and the && !exit_called carve-out preserves pre-PR 'online' ordering for self-exit. The author verified the vendored test-worker-* / test-promise-unhandled-* suites. Given the criticality of the code path and the number of iterations it took to converge, a maintainer sign-off is appropriate.

A terminate() that lands while flush_logs is converting the log to a JS
error / dispatching it makes vm_log.to_js / to_bun_string /
WebWorker__dispatchError throw the TerminationException; the old
Err(Thrown | Terminated) => panic! aborted the whole process. The new
stress test reproduced this on CI's release-asan lane (panic: unhandled
exception, SIGABRT). Skip dispatching a late 'error' for a worker that
is shutting down anyway, and report (don't crash) on a genuine throw.
Comment thread src/jsc/web_worker.rs Outdated
Comment thread src/jsc/web_worker.rs Outdated
…t_thrown

- The blanket has_requested_terminate() early-return swallowed the
  configure_defines() failure dispatch (start_vm self-signals via
  set_requested_terminate() alone, without arming the JSC trap, and
  relies on spin()'s first checkpoint to flush_logs the error). Gate on
  has_termination_request() too so only an external terminate (which
  arms the trap via notify_need_termination) skips the dispatch.
- Extract the terminate-check/take_exception/report sequence into a
  report_thrown closure used by both error arms (was duplicated with
  inconsistent None handling).
- Drop the dead flush_logs call in the new spin() checkpoint.
- Add VM::has_termination_request() (the FFI already existed).
- Restructure the stress test to two levels (main spawns failing
  workers directly, no middle worker) so it does not also trip the
  nested-worker parent-VM UAF that #31951 addresses. The earlier
  three-level fixture caught that UAF on the debian-13 x64-asan lane in
  build 81404.
Comment thread src/jsc/web_worker.rs Outdated
Comment thread src/jsc/web_worker.rs Outdated
Comment thread src/jsc/web_worker.rs

@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.

No further findings after f4c12bc — all six prior rounds of feedback have been addressed. Deferring to a human for the final call given this is worker-termination concurrency (the interplay of has_requested_terminate() vs the JSC has_termination_request() trap vs exit_called vs script_execution_status() took several iterations to get consistent).

What was reviewed:

  • flush_logs termination handling: outer guard and report_thrown now both key on the JSC trap so the configure_defines self-signal still dispatches; Terminated/Thrown split replaces the panic.
  • spin() post-entry checkpoint: && !exit_called carve-out preserves pre-PR 'online' ordering when an uncaughtException handler self-exits.
  • Rust dispatchers (uncaught_exception/unhandled_rejection/handled_promise) all widened to script_execution_status() != Running, covering the isBunTest fast-path; verified this is a strict superset of is_shutting_down() and does not catch the main-thread node:vm watchdog.
  • C++ scriptExecutionStatus guards on the three Bun__* handlers; JSC__VM__hasTerminationRequest binding exists in bindings.cpp.
Extended reasoning...

Overview

The PR fixes a debug-build crash / assert when worker.terminate() + process.exit() race a worker's load-error dispatch. It touches five files: src/jsc/web_worker.rs (the flush_logs rewrite and a new spin() checkpoint), src/jsc/VirtualMachine.rs (three dispatchers widened from is_shutting_down() to script_execution_status() != Running), src/jsc/bindings/BunProcess.cpp (three scriptExecutionStatus guards), src/jsc/VM.rs (new has_termination_request() accessor), and a stress test.

Security risks

None identified. This is internal error-dispatch / lifecycle plumbing; no user-controlled input reaches new parsing or allocation, and the guards fail closed (skip dispatch on a terminating VM rather than run more JS).

Level of scrutiny

High. Worker VM lifecycle is exactly the area where a wrong predicate turns into a UAF or a swallowed error, and this PR distinguishes four related-but-distinct termination signals. The review history bears that out: six rounds each surfaced a real edge case (self-exit conflated with external terminate; configure_defines self-signal swallowed; isBunTest fast-path bypassing the C++ guard; Bun__handleUnhandledRejection return-value inversion; report_thrown predicate mismatch). All were addressed, and the current diff is internally consistent.

Other factors

The added test is ASAN-gated and loops the race 20× in a subprocess with combined {stderr, stdout, exitCode, signalCode} assertion — it proved the flush_logs panic on the debian-13 x64-asan lane per the PR body, though the storedPrototype assert did not reproduce locally. The author verified the vendored test-worker-* / test-promise-unhandled-* suites, worker_threads.test.ts, and process.test.js -t uncaught unchanged. Given the number of subtle interactions already found and fixed here, and the overlap with #31951's nested-worker UAF work, a maintainer sign-off on the final predicate choices is warranted.

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

CI on f4c12bc (build 81617): the added worker-terminate-lifetime.test.ts test passed on all lanes including debian-13 x64-asan. Remaining failures are all marked flaky and unrelated to this diff (jsc-stress Wasm OMG timeout on windows-aarch64, fastutf8stream-reopen on ubuntu-aarch64, no-orphans/multi-run/20144 on darwin). Ready for review.

@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Update from a fresh investigation on current main (9008ae7, after the Worker lifetimes rework in #37075): the guard-ordering hole this PR fixes in Bun__handleUncaughtException still exists there, with a new crash signature. #37075 added its own termination check to this function, but placed it after the processObject() call:

auto* process = globalObject->processObject();   // LazyProperty init runs here
auto& wrapped = process->wrapped();
auto& vm = JSC::getVM(globalObject);
if (vm.hasPendingTerminationException()) [[unlikely]]   // too late
    return true;

On a worker terminated during dynamic import() churn, the completion's error path reaches this function with the TerminationException still pending while hasTerminationRequest has already been cleared by JSC's entry-scope exit. Materializing the lazy process object opens a DeferTermination scope (LazyPropertyInlines.h), which dies with

ASSERTION FAILED: vm.hasTerminationRequest()
vendor/WebKit/Source/JavaScriptCore/runtime/VMTraps.cpp(539) : void JSC::VMTraps::deferTerminationSlow(DeferAction)

so the pre-#37075 Structure::storedPrototype signature this PR describes has moved, but the fix (check before touching processObject()) is exactly what is still needed. I verified on current main that hoisting the termination check above processObject() alone stops the crash: a 25s three-lane churn repro (terminate keyed off the worker's first postMessage, batches of 8 concurrent imports) goes from SIGABRT within seconds to passing, 9 of 9 runs.

Heads up for the rebase: this function's body changed in #37075 (it now probes process._fatalException under a TOP exception scope with its own termination checks), so the BunProcess.cpp hunks will conflict textually. A minimal current-main version of the hoist plus a gate-proven test for the new signature is on branch farm/6e8834d2/worker-terminate-dynimport-deferassert (test/js/web/workers/worker-terminate-lifetime.test.ts) if that saves time.

Related: #36581 reroutes the transpiler-store completion errors through report_error_or_terminate, which closes the same door from the caller side; each fix independently stops the crash above, and both are worth landing.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: both faces described here are handled on current main. Bun__handleUncaughtException returns early on a pending termination and tolerates a termination thrown by the _fatalException lookup since #36579, and the rewritten flush_logs in #37075 returns without reporting once the worker is being stopped and maps JsError::Terminated to a plain return instead of panicking (src/jsc/web_worker.rs). The stress test this PR adds to test/js/web/workers/worker-terminate-lifetime.test.ts passes unmodified against an ASAN debug build of main at 04148c8, three runs in a row.

@robobun robobun closed this Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants