process: defer termination across lazy PropertyCallback builders - #33966
process: defer termination across lazy PropertyCallback builders#33966robobun wants to merge 9 commits into
Conversation
A worker.terminate() that lands while a lazy process.* PropertyCallback builder (constructStdout/constructStdin/constructNextTickFn/...) is entering JS left the TerminationException pending: the builders called scope.tryClearException(), which refuses to clear a termination. JSC's reifyStaticProperty/setUpStaticFunctionSlot don't check for exceptions, so getOwnPropertySlot returned true with the exception still pending and tripped EXCEPTION_ASSERT in JSValue::get / getOwnPropertyDescriptor. Since #31216, every node:worker_threads Worker preloads the module and runs setupWorkerStdio(), which does Object.defineProperty(process, "stdout", ...) and triggers constructStdioWriteStream via the getOwnPropertyDescriptor path on every worker bootstrap. That turned a rare flake into test-worker-message-port-transfer-terminate.js aborting on the x64-asan lane. Replace the tryClearException()/RETURN_IF_EXCEPTION({}) pattern in every process PropertyCallback builder with a shared helper that fully clears the exception (TopExceptionScope::clearException, which the VM trap re-arms at the next safepoint) and skips reportUncaughtExceptionAtEventLoop for the termination case.
|
Updated 2:49 AM PT - Jul 11th, 2026
❌ @robobun, your commit b36f4ed has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33966That installs a local version of the PR into your bun-33966 --bun |
|
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)
WalkthroughChangesProcess lazy-property callbacks now defer termination while constructing process properties. A slow-gated worker test exercises termination during these callbacks and verifies clean subprocess completion. Process lazy-property termination deferral
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/node/worker_threads/worker_threads.test.ts`:
- Around line 1308-1314: Replace the seven-line regression comment near the
amplified worker termination test with one concise, behavior-focused comment of
no more than three lines, or remove it entirely; do not include bug history or
JSC implementation details.
🪄 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: afd2d3ac-a0ca-4255-aa82-e4a590e520d3
📒 Files selected for processing (2)
src/jsc/bindings/BunProcess.cpptest/js/node/worker_threads/worker_threads.test.ts
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate flag for #33418: the PR body covers the relationship under Related. #33418 uses Re #30421: that's a release-build SIGABRT on macOS arm64 with no assertion text. The assertions fixed here are |
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/node/worker_threads/worker_threads.test.ts`:
- Around line 1333-1341: Update the worker-thread regression test’s process
configuration to use diagnostic-only stderr, preferably "inherit", and remove
stderr from the expected result and assertions. Keep stdout, exitCode, and
signalCode as the pass/fail contract.
🪄 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: c2e3310e-408a-4859-a937-a202f9c04e41
📒 Files selected for processing (1)
test/js/node/worker_threads/worker_threads.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Use DeferTerminationScope. Do not clear termination exceptions.
There was a problem hiding this comment.
No issues found; the earlier stderr nit is addressed in 5a3f766. Deferring to a maintainer because the core choice — unconditionally clearing the TerminationException via TopExceptionScope::clearException() and relying on the VM trap to re-throw — is a JSC-semantics call that overlaps with #33418's DeferTerminationForAWhile approach, and someone with JSC context should confirm which idiom they want for the process.* builders.
What was reviewed:
clearLazyPropertyCallbackException: skipsreportUncaughtExceptionAtEventLoopfor termination and re-clears after reporting non-termination — both look correct.- All ~14 call sites: each was previously either
tryClearException+ report orRETURN_IF_EXCEPTION(scope, {}); the latter's contract violation (returning empty JSValue with exception pending intoreifyStaticProperty) is fixed uniformly. - New test: stderr is now diagnostic-only, pipes drained concurrently, gated to ASAN/debug where the assertion fires.
Extended reasoning...
Overview
Two files changed. src/jsc/bindings/BunProcess.cpp adds a static helper clearLazyPropertyCallbackException and routes every PropertyCallback lazy builder's exception path through it (14 sites: constructVersions, constructProcessReleaseObject, constructProcessReportObject, constructProcessConfigObject ×2, constructStdioWriteStream, constructStdin, constructProcessChannel, constructEnv, Process_stubEmptyArray, Process_stubEmptySet, constructMainModuleProperty ×2, Process::constructNextTickFn, constructFeatures). The test file adds one ASAN/debug-gated regression test spawning 60 workers and terminating each mid-bootstrap.
Security risks
None. This is exception-scope hygiene in lazy property builders; no untrusted input parsing, auth, or crypto is touched.
Level of scrutiny
Moderate-to-high. The mechanical application is straightforward and consistent, but the semantic decision — clearing a TerminationException outright rather than deferring it (DeferTerminationForAWhile, JSC's own LazyProperty idiom, used by #33418 for the Bun.* builders) — depends on the invariant that the VM trap re-arms and re-throws at the next safepoint. The PR body argues this convincingly (deferring across constructStdioWriteStream loading all of node:stream shifted the race into a rarer assertNoException path at ~1/4000 workers; clearing stress-tests clean at 0/7200), and the helper correctly skips reportUncaughtExceptionAtEventLoop for termination (not a user error) and re-clears after reporting non-termination in case that re-enters JS. But this is exactly the kind of JSC-internals tradeoff a maintainer should sign off on, especially with a competing open PR taking the other approach.
Other factors
- The sites that previously used
RETURN_IF_EXCEPTION(scope, {})were returning an emptyJSValueintoreifyStaticProperty→putDirectwith the exception still pending — the PR fixes those to returnjsUndefined()(or the partially-built object) with the exception cleared, which is strictly better regardless of the termination question. - My prior inline comment (asserting
stderr: ""on ASAN/debug lanes) was addressed in 5a3f766 with theexitCode === 0 ? "" : stderrpattern matching the file's convention at line ~1371. - The PR author (robobun) already acknowledged the #33418 relationship in-thread and left a note there; the two PRs are complementary (different builder sets) but the idiom divergence is worth a human confirming.
|
Switching to |
|
yes thats fine. remove your current code. |
…nistic Per review: use JSC::DeferTerminationForAWhile (matching LazyProperty::callFunc) instead of clearing the termination exception. The builder runs to completion and the trap re-fires on scope exit. Regression test switched from a 60-worker stress to a deterministic sleepSync + property read per builder, which reliably fails before the fix and doesn't depend on termination landing in a timing window.
robobun
changed the title
|
Switched to |
worker_threads.test.ts has several pre-existing 5s timeouts under debug+ASAN that are unrelated to this fix.
There was a problem hiding this comment.
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 133-137: The worker-termination test’s fixed Bun.sleepSync(300)
timing window is nondeterministic. Update the worker script and coordination
around the existing Promise.all/property loop to use an explicit handshake
confirming w.terminate() is armed before executing process[p], ensuring the test
reliably exercises the pending-termination path without wall-clock delays.
- Around line 146-153: Preserve the captured stderr in the worker termination
assertion instead of replacing it with an empty string when exitCode is 0.
Update the expectation in the worker lifetime test to validate stderr against
the explicitly allowed diagnostic contract, including benign ASAN/debug output,
while still failing on unexpected diagnostics.
🪄 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: 3b89f567-86ab-4b8d-a0c9-171c552122a2
📒 Files selected for processing (2)
src/jsc/bindings/BunProcess.cpptest/js/web/workers/worker-terminate-lifetime.test.ts
|
Opened oven-sh/WebKit#282 to move the |
There was a problem hiding this comment.
No issues found, but deferring to a human reviewer given this touches JSC termination/exception handling across 14 lazy builders and changes worker teardown latency (deferring across constructStdioWriteStream now runs the full node:stream load before unwind).
What was reviewed:
- Verified
DEFER_TERMINATION_FOR_LAZY_PROPERTYis placed before eachTopExceptionScopeso the RAII guard outlives it; matches JSC's ownLazyProperty::callFuncidiom and #33418. - Checked the uncovered PropertyCallback builders (
hrtime,memoryUsage,send,disconnect,argv0,execPath) — none declare an exception scope or enter JS, so exclusion is correct. - The earlier stderr-assertion nit is addressed; test now uses the diagnostic-only pattern and is gated on debug/ASAN.
Extended reasoning...
Overview
The PR adds JSC::DeferTerminationForAWhile (via a local macro) to 14 lazy PropertyCallback builders in src/jsc/bindings/BunProcess.cpp, and adds a deterministic regression test to test/js/web/workers/worker-terminate-lifetime.test.ts. The fix prevents a TerminationException from being left pending when getOwnPropertySlot returns true, which trips EXCEPTION_ASSERT in debug/ASAN builds.
Security risks
None identified. This is exception-scope hygiene in lazy property reification; no auth, crypto, or untrusted-input parsing is involved.
Level of scrutiny
Moderate-to-high. The change is mechanically simple (one RAII guard per builder), follows an established precedent (#33418 for Bun.* builders, and JSC's own LazyProperty::callFunc), and the maintainer approved the approach mid-thread. However, it sits squarely in JSC exception/termination handling — one of the most-blocked categories per repo guidance — and has a stated behavioral side effect: workers terminated during bootstrap now complete the full stdio stream initializer before unwinding. The author also notes a separate ~1/4000 assertNoException path that this PR intentionally does not address. A human should confirm the final 14-site application and the teardown-latency trade-off.
Other factors
All prior review threads (mine and CodeRabbit's) are resolved. The test is deterministic (one worker per builder via sleepSync + property read), verified to fail 3/3 on the unfixed build, and follows the file's existing subprocess-assertion conventions. I spot-checked the PropertyCallback table against the diff: builders without an exception scope are correctly excluded.
…Value (#34104) `computeErrorInfoWrapperToJSValue` is Bun's `vm.onComputeErrorInfoJSValue` hook, called from `ErrorInstance::materializeErrorInfoIfNeeded` when a lazy error property (`stack`/`line`/`column`/`sourceURL`) is first read. When the hook throws before the default stack string has been computed (e.g. a throwing `.message` getter while `Error.prepareStackTrace` is set), `computeErrorInfoToJSValue` returns `{}`. `materializeErrorInfoIfNeeded` then `putDirect`s that empty value into the error's `stack` slot, and the next read segfaults: ```js Error.prepareStackTrace = (e, s) => "custom"; const e = new Error("x"); Object.defineProperty(e, "message", { get() { throw new TypeError("boom"); } }); e.stack; // Segmentation fault at address 0x5 (release), UBSan null deref (debug) ``` Fall back to `jsUndefined()` so the stored value is always valid. The `.message` throw still propagates to the caller. ### About the `getOwnPropertyDescriptor` assertion in #34095 `ErrorInstance::getOwnPropertySlot` in WebKit does not check for an exception after `materializeErrorInfoIfNeeded` (unlike its siblings `defineOwnProperty`/`put`, which do). When the hook leaves one pending, `JSObject::getOwnPropertyDescriptor` trips: ``` ASSERTION FAILED: !scope.exception() || !result vendor/WebKit/Source/JavaScriptCore/runtime/JSObject.cpp(3936) ``` An earlier revision of this PR wrapped the hook in `DeferTerminationForAWhile` to keep a `TerminationException` from reaching that assertion, but that scope would have covered the `profiledCall` into user `Error.prepareStackTrace`, making an infinite loop there uninterruptible by `worker.terminate()`. Dropped in 3dd57f6. A proper fix for the termination case is a `RETURN_IF_EXCEPTION` in `ErrorInstance::getOwnPropertySlot` on the WebKit side. ### Related - #33966 applies `DeferTerminationForAWhile` to the `process.*` lazy property builders (bounded C++ initializers, no unbounded user JS), which is the path `test-worker-message-port-transfer-terminate.js` actually hits during worker bootstrap. - #30823 takes the alternative approach of clearing every exception in this hook via `tryClearException`, which covers a throwing `Error.prepareStackTrace` reaching `getOwnPropertyDescriptor` on debug/ASAN, at the cost of `e.stack` no longer propagating the throw. ### Verification New test in `test/js/node/v8/capture-stack-trace.test.js` spawns the `.message`-throws repro and asserts the first `.stack` read throws `msg-boom` and a subsequent read returns `undefined` instead of crashing. Segfaults on the unfixed build, passes with this change. The full test file (41 tests, including the existing `e.stack`-throws and `prepareStackTrace`-propagation tests) passes. Refs #34095 <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/v8/capture-stack-trace.test.js" info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (3dd57f6) test/js/node/v8/capture-stack-trace.test.js: (pass) Regular .stack [12.67ms] (pass) throw inside Error.prepareStackTrace doesnt crash [6.81ms] (pass) capture stack trace [6.22ms] (pass) capture stack trace with message [6.94ms] (pass) capture stack trace with constructor [4.93ms] (pass) capture stack trace limit [21.63ms] (pass) prepare stack trace [10.24ms] (pass) capture stack trace second argument [17.05ms] (pass) capture stack trace edge cases [11.07ms] (pass) prepare stack trace call sites [12.77ms] (pass) sanity check [13.28ms] (pass) CallFrame isEval works as expected [6.69ms] (pass) CallFrame isTopLevel returns false for Function constructor [7.85ms] (pass) CallFrame.p.getThisgetFunction: strict/slopp ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (3dd57f6) test/js/node/v8/capture-stack-trace.test.js: (pass) Regular .stack [0.39ms] (pass) throw inside Error.prepareStackTrace doesnt crash [0.11ms] (pass) capture stack trace [0.07ms] (pass) capture stack trace with message [0.09ms] (pass) capture stack trace with constructor [0.06ms] (pass) capture stack trace limit [0.22ms] (pass) prepare stack trace [0.12ms] (pass) capture stack trace second argument [0.18ms] (pass) capture stack trace edge cases [0.10ms] (pass) prepare stack trace call sites [0.11ms] (pass) sanity check [0.11ms] (pass) CallFrame isEval works as expected [0.14ms] (pass) CallFrame isTopLevel returns false for Function constructor [0.12ms] (pass) CallFrame.p.getThisgetFunction: strict/sloppy mode interaction [0.11ms] (pass) CallFrame.p.isConstructor [0.04ms] (pass) CallFrame.p.isNative [0.04ms] (pass) return non-strings from Error.prepareStackTrace [0.03ms] (pass) CallFrame.p.toString [0.03ms] (pass) err.stack should invoke prepareStackTrace [0.30ms] (pass) Error.prepareStackTrace inside a node:vm works [4.77ms] (pass) Error.captureStackTrace inside error constructor works [0.10ms] (pass) Error.prepareStackTrace has ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/v8/capture-stack-trace.test.js" info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (3dd57f6) test/js/node/v8/capture-stack-trace.test.js: (pass) Regular .stack [12.34ms] (pass) throw inside Error.prepareStackTrace doesnt crash [6.56ms] (pass) capture stack trace [6.06ms] (pass) capture stack trace with message [6.90ms] (pass) capture stack trace with constructor [4.93ms] (pass) capture stack trace limit [21.98ms] (pass) prepare stack trace [10.25ms] (pass) capture stack trace second argument [17.39ms] (pass) capture stack trace edge cases [11.23ms] (pass) prepare stack trace call sites [12.74ms] (pass) sanity check [12.78ms] (pass) CallFrame isEval works as expected [6.62ms] (pass) CallFrame isTopLevel returns false for Function constructor [7.84ms] (pass) CallFrame.p.getThisgetFunction: strict/slopp ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) [configured] bun-profile → bun (stripped) in 643ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/26] gen generated_host_exports.rs generated_host_exports.rs: 91 exports (host=3, lazy=10, generic=78, rust=0); 243 extern-C blocks audited [2/26] gen cpp.rs (cppbind) [3/26] gen JS modules (bundle-modules) Preprocess modules (6586ms) Bundle modules (26ms) Postprocesss modules (26ms) Bundle Functions (664ms) Generate Code (74ms) [7.39s] Bundled "src/js" for production 1912 kb 162 internal modules 12 native modules 90 internal functions across 19 files [3/17] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: component rust ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/bindings/FormatStackTraceForJS.cpp | 4 ++++ test/js/node/v8/capture-stack-trace.test.js | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/bindings/FormatStackTraceForJS.cpp 9 9 0 test/js/node/v8/capture-stack-trace.test.js 2 3 0 ``` </details> <!-- robobun:evidence:end -->
… for darwin noise Self-review surfaced two factual corrections: - The stress test's CI crashes are all ExceptionScope.h:61 assertNoException (6/6 observed), not the JSObject.cpp:3936 path tracked in #34095. PR #33966's Verification section reports that path still reproducing at ~1/4000 workers AFTER its lazy-builder fix. Give the stress-test entry its own tracker (#34690) and stop claiming it is removable with the vendored-test entry. - The error-only annotation sweep missed a style=warning (retry-passed) hit on darwin 26 aarch64 at delta=14 in build 75589, the very build the previous comment cited. Raise the threshold to 20 so the margin above the observed noise ceiling matches the margin below the +25 leak signal, and correct the comment.
|
This was fixed at the JSC layer instead of per builder. oven-sh/WebKit#282 (with the follow-up oven-sh/WebKit#306) wraps both Checked by running this PR's version of Nothing left for this PR to add, so closing it. |
2 participants
Fixes
test/js/node/test/parallel/test-worker-message-port-transfer-terminate.jsaborting on the x64-asan lane since #31216 (e.g. builds 71759, 71693):Repro
Deterministic, one worker per builder:
Aborts on every debug build. The vendored Node test hits the same path via
setupWorkerStdioracingsetImmediate-then-terminate().Cause
process.stdout/stderr/stdin/nextTick/mainModule/channel/env(and the callbacks that only allocate) arePropertyCallbackentries in the process static hash table. JSC reifies them viasetUpStaticFunctionSlot→reifyStaticProperty, neither of which check for exceptions after calling the builder:The builders that enter JS try to clear any exception before returning:
but
ExceptionScope::tryClearException()refuses to clear aTerminationException. So whenworker.terminate()arms the trap while the builder is entering JS, the builder returns with the termination still pending,getOwnPropertySlotreturnstrue, andJSValue::get/getOwnPropertyDescriptorassert.#31216 turned this from a rare flake into a per-run abort by preloading
node:worker_threadsin everyworker_threadsWorker, whosesetupWorkerStdio()doesObject.defineProperty(process, "stdout", ...)during bootstrap, right in thesetImmediate-then-terminate()window.Fix
Wrap every process
PropertyCallbackbuilder inJSC::DeferTerminationForAWhile, matching what JSC's ownLazyProperty::callFuncdoes: the builder runs to completion with no exception pending, and the trap re-fires on scope exit so the worker unwinds at the next safepoint.Related: #33418 applies the same idiom to the
Bun.*lazy builders.Verification
New deterministic test in
test/js/node/worker_threads/worker_threads.test.ts(one worker perprocess.stdout/stderr/stdin/nextTick/mainModule, debug/ASAN-gated).git stash push -- src/ && bun bd test ... -t "lazy process.* builder"→ fails 3/3 withASSERTION FAILED: !scope.exception() || !hasSlot/ SIGABRTgit stash pop && bun bd test ...→ passes 5/5test-worker-message-port-transfer-terminate.js× 30 runs → cleanUnder a 60-worker × 80-run bootstrap stress, a separate pre-existing
assertNoException()atExceptionScope.h:61still surfaces at about 1 in 4000 workers (termination landing later in the bootstrap, after the stdio builders have completed). That path is unrelated to the lazy-builder contract and not in scope here; the deterministic test in this PR does not reach it.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 2
evidence per changed file