worker_threads: don't abort when terminate() interrupts a lazy property builder - #33418
worker_threads: don't abort when terminate() interrupts a lazy property builder#33418robobun wants to merge 3 commits into
Conversation
JSC's reifyStaticProperty calls a PropertyCallback builder and stores the result with putDirect, performing no exception check, so a builder must never return with a pending exception. The process.* and Bun.* builders relied on TopExceptionScope::tryClearException(), which is a no-op for a TerminationException. A worker.terminate() that landed while one of them was entering JS therefore left the exception pending and tripped EXCEPTION_ASSERT(!scope.exception() || !hasSlot) in JSValue::get. Wrap the builders that enter JS in DeferTerminationForAWhile, the same guard JSC::LazyProperty::callFunc uses: the builder runs to completion and the trap refires afterwards, so the unwind happens at the next safepoint instead.
|
Updated 3:17 AM PT - Jul 6th, 2026
❌ @robobun, your commit 4cd43cd has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33418That installs a local version of the PR into your bun-33418 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
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)
WalkthroughAdds termination deferral guards around lazy/static property reification paths in BunObject and BunProcess, updates the generated lazy getter wrapper, and adds a worker regression test that terminates during lazy property access. ChangesTermination deferral during lazy property reification
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/BunObject.cpp (1)
318-341: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd the termination guard to both SQL property builders
src/jsc/bindings/BunObject.cpp:318-341—defaultBunSQLObjectandconstructBunSQLObjectfollow the same lazyPropertyCallbackpath asconstructBunShell, and both can enter JS viarequireId(...)/.get(). AddJSC::DeferTerminationForAWhilehere too soreifyStaticPropertycan’t observe aTerminationException.🤖 Prompt for 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. In `@src/jsc/bindings/BunObject.cpp` around lines 318 - 341, Add the termination guard to both SQL property builders, since `defaultBunSQLObject` and `constructBunSQLObject` follow the same lazy `PropertyCallback` path as `constructBunShell` and can enter JS through `requireId(...)` and `.get()`. Update `BunObject.cpp` so each function wraps the JS-facing work with `JSC::DeferTerminationForAWhile`, ensuring `reifyStaticProperty` cannot observe a `TerminationException`. Keep the change localized to these two helpers and preserve their existing exception handling flow.
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/BunObject.cpp`:
- Around line 318-341: Add the termination guard to both SQL property builders,
since `defaultBunSQLObject` and `constructBunSQLObject` follow the same lazy
`PropertyCallback` path as `constructBunShell` and can enter JS through
`requireId(...)` and `.get()`. Update `BunObject.cpp` so each function wraps the
JS-facing work with `JSC::DeferTerminationForAWhile`, ensuring
`reifyStaticProperty` cannot observe a `TerminationException`. Keep the change
localized to these two helpers and preserve their existing exception handling
flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6f09c10-d06c-4cfc-9fc8-7cb5b7075cc1
📒 Files selected for processing (3)
src/jsc/bindings/BunObject.cppsrc/jsc/bindings/BunProcess.cpptest/js/web/workers/worker-terminate-lifetime.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
defaultBunSQLObject / constructBunSQLObject evaluate the bun:sql internal module through requireId(), and every Rust-backed getter behind DEFINE_ZIG_BUN_OBJECT_GETTER_WRAPPER can leave an exception pending too. Without the defer they hand the empty JSValue to putDirect instead of tripping the assertion, but it is the same contract violation. Sweeping all 46 lazy properties through the repro now reports none crashing.
|
Thanks — the review caught a real gap. Both
|
|
Tip For best results, initiate chat on the files or code changes.
The clarification on why this isn't a duplicate of ( •ᴗ• )b ✏️ Learnings added
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/web/workers/worker-terminate-lifetime.test.ts (2)
147-153: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert
stderrbeforestdoutin the combined object, per this file's established ASAN-diagnosability convention.The combined assertion currently orders
stdoutbeforestderr. For this specific file, a prior review established that stderr must be checked first (and unconditionally) since ASAN can print a heap-use-after-free report to stderr even when the process exits 0 — putting stderr first surfaces that signal prominently in the failure diff.🔧 Suggested reorder
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect({ stdout, stderr, exitCode, signalCode: proc.signalCode }).toEqual({ - stdout: `terminated ${lazyProperties.length}\n`, + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stderr, stdout, exitCode, signalCode: proc.signalCode }).toEqual({ stderr: "", + stdout: `terminated ${lazyProperties.length}\n`, exitCode: 0, signalCode: null, });Based on learnings, "assert stderr first (before stdout) and do so unconditionally... canonical ordering for this pattern: assert stderr → assert stdout → assert exitCode" for
test/js/web/workers/worker-terminate-lifetime.test.ts.🤖 Prompt for 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. In `@test/js/web/workers/worker-terminate-lifetime.test.ts` around lines 147 - 153, The combined assertion in the worker lifetime test is using the wrong field order for this file’s ASAN-diagnosability convention. Update the expect object in the `worker-terminate-lifetime` test so `stderr` is asserted before `stdout`, and keep that ordering unconditionally alongside `exitCode` and `signalCode` to match the established pattern in this file.Source: Learnings
121-138: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWire the worker's
errorevent to fail fast instead of relying on the outer timeout.
closedonly resolves on thecloseevent; there's noerror(or similar failure) listener rejecting the promise if a worker throws for a reason unrelated to termination (e.g. a bug in the property-access script itself). If that happens, the test will hang until the whole-testtimeout(20s/60s) rather than fail immediately with a useful error.🛠️ Suggested fix
- const closed = new Promise(resolve => w.addEventListener("close", resolve, { once: true })); + const { promise: closed, resolve, reject } = Promise.withResolvers(); + w.addEventListener("close", resolve, { once: true }); + w.addEventListener("error", reject, { once: true }); w.addEventListener("message", () => w.terminate(), { once: true }); return closed;As per coding guidelines, "Wire EVERY failure event (
error,close,abort, process exit) to reject the awaited promise — never throw inside event callbacks."🤖 Prompt for 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. In `@test/js/web/workers/worker-terminate-lifetime.test.ts` around lines 121 - 138, The worker promise in worker-terminate-lifetime.test.ts only waits for the close event, so failures from the Worker can hang until the test timeout instead of failing immediately. Update the closed promise in the properties.map block to also listen for the worker’s error event (and reject on it) using the same Worker instance that is already created and terminated via w.terminate(). Keep the close listener resolving the promise, but make any non-termination worker failure reject fast with the underlying error so the test surfaces the real issue.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/web/workers/worker-terminate-lifetime.test.ts`:
- Around line 147-153: The combined assertion in the worker lifetime test is
using the wrong field order for this file’s ASAN-diagnosability convention.
Update the expect object in the `worker-terminate-lifetime` test so `stderr` is
asserted before `stdout`, and keep that ordering unconditionally alongside
`exitCode` and `signalCode` to match the established pattern in this file.
- Around line 121-138: The worker promise in worker-terminate-lifetime.test.ts
only waits for the close event, so failures from the Worker can hang until the
test timeout instead of failing immediately. Update the closed promise in the
properties.map block to also listen for the worker’s error event (and reject on
it) using the same Worker instance that is already created and terminated via
w.terminate(). Keep the close listener resolving the promise, but make any
non-termination worker failure reject fast with the underlying error so the test
surfaces the real issue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 912ed2fb-2780-4abf-a345-2230c6311c7c
📒 Files selected for processing (3)
src/jsc/bindings/BunObject+exports.hsrc/jsc/bindings/BunObject.cpptest/js/web/workers/worker-terminate-lifetime.test.ts
There was a problem hiding this comment.
Thanks for covering the SQL builders and the Rust-backed wrapper macro — that closes the gap I flagged. Deferring to a human for sign-off given this is JSC exception/termination handling in the bindings and needs merge-order coordination with #33211.
Extended reasoning...
Overview
Adds JSC::DeferTerminationForAWhile RAII guards to every PropertyCallback lazy builder that can enter JS (process.nextTick/mainModule/stdin/stdout/stderr/channel, Bun.$, Bun.sql/SQL/postgres, and — via the shared wrapper macro in BunObject+exports.h — all ~40 Rust-backed Bun.* getters). This mirrors what JSC's own LazyProperty::callFunc does, so a TerminationException armed by worker.terminate() can no longer escape into reifyStaticProperty (which has no exception check) and trip EXCEPTION_ASSERT(!scope.exception() || !hasSlot). A deterministic regression test parks each worker in Bun.sleepSync, terminates it, then touches one lazy property per builder shape.
My earlier inline comment (SQL builders also enter JS via requireId → profiledCall) has been addressed in commit ff8ad9e: both defaultBunSQLObject and constructBunSQLObject now carry the guard, and Bun.sql / Bun.SQL / Bun.argv were added to the test's lazyProperties array.
Security risks
None. This is purely worker-termination timing inside the runtime; no user-controlled input, auth, or data-exposure surface is touched.
Level of scrutiny
Medium-high. The change is mechanically small (one RAII line per function), but it lives in JSC C++ bindings and alters exception/termination-scope interaction across a wide fan-out — including a macro that now applies the defer to every Rust-backed Bun.* getter. That broad application looks correct (JSC does the same for its own lazy properties, and DeferTerminationForAWhile refires the trap on scope exit so the worker still unwinds promptly), but a maintainer familiar with the worker termination lifecycle should confirm.
Other factors
- The PR description explicitly calls out a textual conflict with #33211 and specifies how the two should compose ("whichever lands second should put the defer inside that helper"). A human needs to decide merge order and reconcile.
- The new test asserts
stderr: "", which the repo guidelines warn against for ASAN/debug noise, but every other test in this file already does the same — it's the local convention, not new risk introduced here. - No bugs found by the automated bug hunt on this revision.
…tion order Without an error listener a worker that dies before it signals still fires close, so the test would report every property as terminated while having exercised nothing.
|
Both test findings applied in 4cd43cd.
|
CI status: the diff is green, the three red jobs are infra + a known flakeBuild 68785 at
The three failures:
I checked whether my own commits introduced it rather than assuming: build 68762 ( Not re-triggering. With that napi flake hitting two thirds of failed builds, a re-roll would very likely land in the same place; it would burn a CI run for nothing. Happy to push a retrigger if a maintainer would rather see a clean board, otherwise this is ready for review on the merits. The one open question for a human is still merge order against #33211, as described in the PR body: the two guards compose, and whichever lands second should fold |
|
#33966 fixes the I tried the The |
|
Heads up: #32488 (merged 2026-07-16) added The stress test's worker body is |
|
This was fixed at the JSC layer instead: oven-sh/WebKit#282 wraps the two reifyStaticProperty call sites (setUpStaticFunctionSlot and reifyAllStaticProperties) in DeferTerminationForAWhile and reports the slot as not found when a builder throws, rather than storing an empty value. That covers every PropertyCallback builder at once, including the Bun.* getter wrapper. Bun picked it up with the WebKit bump in #34669. This PR's test case passes unmodified against a debug ASAN build of current main (bdb7382): 3 runs, 4 pass / 0 fail each. The change is no longer needed. Closing. |
1 participant
Deflakes
test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js, which has been aborting the whole test process on thex64-asanlane (5 of the last 120 failed builds, e.g. builds 68677, 68637, 68564, 68435, 68343):Repro
The test creates 10 workers and terminates each one on the next
setImmediate, soterminate()lands while the worker is still evaluating its entry module. Deterministically, with no racing:Aborts every time on a debug build.
process.mainModule,process.stdinandBun.$abort the same way (Bun.$viaJSCJSValueCell.h:67:34: member call on null pointer of type 'JSC::JSCell'under UBSAN, because its builder hands the empty JSValue toputDirect).Cause
process.nextTick,process.mainModule,process.stdin,process.stdout,process.stderr,process.channelandBun.$arePropertyCallbackentries in a static hash table. JSC reifies them inreifyStaticProperty(Lookup.h):No exception check, and
getPropertySlotthen reports the slot as found. So a builder must never return with an exception pending — the builders know that and say so in a comment:ExceptionScope::tryClearException()refuses to clear aTerminationExceptionand returnsfalse, which nothing checks. Whenworker.terminate()has armed the termination trap,profiledCallthrows theTerminationExceptionat the initializer's entry, the builder leaves it pending,getPropertySlotreturnstrue, andJSValue::get'sEXCEPTION_ASSERT(!scope.exception() || !hasSlot)fires. (JSObject::gettolerates this exact state —!scope.exception() || vm.hasPendingTerminationException() || !hasProperty— but the LLInt'sget_by_idslow path goes throughJSValue::get, which does not.)The window in the vendored test is the straight-line code between the last trap check and the first
process.nextTickread inside an internal module.Fix
Wrap the builders that enter JS in
JSC::DeferTerminationForAWhile, which is whatJSC::LazyProperty::callFuncalready does for JSC's own lazy properties (LazyPropertyInlines.h). The builder runs to completion, no exception is left pending, and the trap refires on scope exit so the worker unwinds at the next safepoint exactly as before.Applied to every
PropertyCallbackbuilder that can enter JS:constructNextTickFn,constructStdioWriteStream(process.stdout/process.stderr),constructStdin,constructProcessChannel,constructMainModulePropertyconstructBunShell(Bun.$),defaultBunSQLObject(Bun.sql/Bun.postgres),constructBunSQLObject(Bun.SQL) — these evaluate a builtin or thebun:sqlinternal moduleDEFINE_ZIG_BUN_OBJECT_GETTER_WRAPPER, the one wrapper behind all 38 Rust-backedBun.*gettersThe
Bun.*builders fail differently today:RETURN_IF_EXCEPTION(scope, {})hands the empty JSValue toputDirect, so the symptom isJSCJSValueCell.h:67:34: member call on null pointer of type 'JSC::JSCell'rather than the assertion. Same contract violation, same fix. (Bun.argvandBun.embeddedFilesreach it through the Rust wrapper.)Verification
test/js/web/workers/worker-terminate-lifetime.test.tsgains a case that terminates one worker per builder shape, each parked inBun.sleepSyncsoterminate()is requested before its first touch of the property.git stash push -- src/ && bun bd test test/js/web/workers/worker-terminate-lifetime.test.ts→ fails withASSERTION FAILED: !scope.exception() || !hasSlotgit stash pop && bun bd test test/js/web/workers/worker-terminate-lifetime.test.ts→ 4 passSweeping the repro over all 46 lazy properties on the
Bunandprocessobjects (Bun.$,Bun.sql,Bun.SQL,Bun.postgres,Bun.argv,Bun.embeddedFiles, the 38 Rust-backed getters,process.nextTick/mainModule/stdin/stdout/stderr/channel) reports none crashing; before the change, nine of them did.The vendored node test also passes, and the previously-crashing timing window (
terminate()at 400-720 ms into a debug build's worker startup, where it reproduced 2 of 3 runs) is now clean across 16 runs.Adjacent: #33211
#33211 fixes a different bug in the same
reifyStaticPropertycontract — aBun.*lazy getter that throws returns the empty JSValue, whichputDirectthen stores. It touchesconstructBunShell, so these two conflict textually. ThelazyPropertyResult()guard it introduces usestryClearException()too, so it still needs theDeferTerminationForAWhilefrom this PR for the termination case; whichever lands second should put the defer inside that helper.