process: don't hand an empty JSValue to reifyStaticProperty on builder exception

[robobun]

Problem

Six lazy PropertyCallback builders in BunProcess.cpp (constructVersions, constructProcessReleaseObject, constructProcessReportObject, the tail of constructProcessConfigObject, Process_stubEmptySet, constructFeatures) declare a TopExceptionScope but bail with RETURN_IF_EXCEPTION(scope, {}).

reifyStaticProperty stores a PropertyCallback's result via thisObj.putDirect(vm, name, result, attrs) with no exception check (Lookup.h), so that {} lands in putDirectInternal:

ASSERT(value);   // JSObjectInlines.h:497

which crashes the debug build, and in release stores an empty value the next reader dereferences. The exception itself also escapes reifyStaticProperty unchecked.

RETURN_IF_EXCEPTION additionally handles VM traps, so a worker terminate() that arms the termination trap between the scope declaration and the check is enough for the macro to convert it into a pending TerminationException and take the return {} branch. In practice these six builders do not enter JS, so the window is very narrow; the remaining trigger is OOM inside constructEmptyObject / constructEmptyArray / JSSet::create.

Fix

Replace each RETURN_IF_EXCEPTION(scope, {}) with the same clear-and-report pattern Process_stubEmptyArray and constructProcessConfigObject already use for this exact reason (the latter's comment: "Lazy property builder: exceptions must not propagate into reifyStaticProperty, which performs no exception check"). Extract that pattern into a small file-local helper and migrate the nine existing inline copies of it in the same file:

static JSValue clearAndReportLazyPropertyException(JSC::TopExceptionScope& scope, JSC::JSGlobalObject* globalObject)
{
    auto* exception = scope.exception();
    (void)scope.tryClearException();
    Zig::GlobalObject::reportUncaughtExceptionAtEventLoop(globalObject, exception);
    return JSC::jsUndefined();
}

jsUndefined() is a valid value for putDirect, the exception is reported instead of silently propagating past the caller, and because the check is scope.exception() rather than RETURN_IF_EXCEPTION it no longer handles the termination trap mid-builder, so a terminate() landing in that window is observed at the next real safepoint instead of producing an empty property value.

Swapping the scope type to DECLARE_THROW_SCOPE (the literal inverse of ab84aa2) would not help: it still returns {} on exception, and reifyStaticProperty still has no check.

Why no fail-before test

None of the six builders run any JS that user code can cause to throw; the only reachable exception is OOM (or the sub-instruction termination race above). There is no allocation-failure injection knob, so the defect cannot be reproduced deterministically without instrumenting src/. The new test in process.test.js reifies every touched lazy property under BUN_JSC_validateExceptionChecks=1 as a scope-discipline guard and asserts the expected types, and BUN_JSC_validateExceptionChecks=1 BUN_JSC_dumpSimulatedThrows=1 over all of them plus process.report.getReport() / process.stdin / process.stdout / process.stderr / process.nextTick is clean.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 5 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: fab1c064-484b-408d-878a-047588aafa3c

📥 Commits

Reviewing files that changed from the base of the PR and between 13f5dec and 396238d.

📒 Files selected for processing (2)

• src/jsc/bindings/BunProcess.cpp
• test/js/node/process/process.test.js

---

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

[robobun]

^{Updated 9:18 PM PT - Jul 20th, 2026}

@robobun, your commit 396238d is building: #76636

[github-actions]

Found 2 issues this PR may fix:

1. ASAN CI: JSC assertion in JSObject::getOwnPropertyDescriptor during worker terminate (test-worker-message-port-transfer-terminate) #34095 - Fixes the JSC assertion failure (!scope.exception() || !result) during worker terminate by ensuring lazy property builders in BunProcess.cpp no longer return empty JSValues on exception
2. ASAN CI: ExceptionScope::assertNoException during worker terminate (worker-transfer-terminate-stress, separate from #34095) #34690 - Completes the fix started in process: defer termination across lazy PropertyCallback builders #33966 by covering the six additional lazy property builders that still used RETURN_IF_EXCEPTION(scope, {}), which could trigger assertNoException during worker-transfer-terminate stress

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #34095
Fixes #34690

🤖 Generated with Claude Code

[robobun]

Closing: this was already fixed at the WebKit layer in oven-sh/WebKit#282 and oven-sh/WebKit#306, which landed in bun's pinned WebKit via #34373.

reifyStaticProperty now returns early when a PropertyCallback yields an empty value, and setUpStaticFunctionSlot wraps the call in DeferTerminationForAWhile and treats a pending exception as slot-not-found. So RETURN_IF_EXCEPTION(scope, {}) in these builders is the supported pattern and the exception propagates correctly without touching putDirect.

The local vendor/WebKit checkout I was reading from was at an older revision than the one the build actually uses, which is what prompted the change.

[github-actions]

This PR may be a duplicate of:

1. process: defer termination across lazy PropertyCallback builders #33966 - Also modifies the same BunProcess.cpp PropertyCallback builders to prevent crashes when exceptions occur during lazy property reification, using DeferTerminationForAWhile instead of clearAndReportLazyPropertyException
2. worker_threads: don't abort when terminate() interrupts a lazy property builder #33418 - Modifies a subset of the same BunProcess.cpp PropertyCallback builders (stdio, channel, mainModule, nextTick) with DeferTerminationForAWhile to prevent crashes when worker termination interrupts lazy property builders
3. process: retry nextTick initialization in queueNextTick after failed first reification #30612 - Modifies constructNextTickFn exception handling in BunProcess.cpp to retry after failed first reification (narrower scope, same code area)

🤖 Generated with Claude Code

[claude]

LGTM — mechanical extraction of an existing clear-and-report pattern into a file-local helper, applied to six RETURN_IF_EXCEPTION(scope, {}) sites and nine identical inline copies. The one inline finding is pre-existing and unrelated to this change.

Extended reasoning...

Overview

Two files touched: src/jsc/bindings/BunProcess.cpp gains a small static helper clearAndReportLazyPropertyException and 15 call sites are rewritten — six previously used RETURN_IF_EXCEPTION(scope, {}) (which would hand an empty JSValue to reifyStaticProperty's unchecked putDirect), and nine were already-correct inline copies of the exact clear-and-report pattern now deduplicated into the helper. test/js/node/process/process.test.js gains a scope-discipline guard that reifies every touched lazy property under BUN_JSC_validateExceptionChecks=1.

Security risks

None. No user-controlled input, no new API surface, no auth/crypto/permissions. The change only affects the OOM / termination-trap error path inside lazy process.* property builders.

Level of scrutiny

Low-to-medium. The helper is a byte-for-byte extraction of the pattern that already existed at nine sites in this file (e.g. constructEnv, Process_stubEmptyArray, constructStdin), so the nine migrations are pure dedup with no behavior change. The six RETURN_IF_EXCEPTION conversions change behavior only on the exception path (OOM in constructEmptyObject/constructEmptyArray/JSSet::create, or a termination trap landing in a sub-instruction window), where returning jsUndefined() and reporting is strictly better than returning {} into putDirectInternal's ASSERT(value). I verified each migrated site passes the same globalObject it previously used and that the double-read of scope.exception() (guard + helper) is harmless.

Other factors

• REVIEW.md's "never clearException()" concern doesn't apply: this is tryClearException() followed by reportUncaughtExceptionAtEventLoop, the established pattern for lazy-property builders whose caller performs no exception check, and it was already in use at nine sites in this file before this PR.
• The PR description honestly explains why no fail-before test is possible (no allocation-failure injection knob; the builders don't enter user JS). The added test is a reasonable validateExceptionChecks guard and follows the file's existing subprocess conventions.
• The one inline finding (duplicate excludeEnv key that should be signal at BunProcess.cpp:2528-2529) is pre-existing, sits in unchanged context lines, and is unrelated to this PR's exception-handling change — noted as a drive-by opportunity, not a blocker.

[claude]

🟣 Pre-existing (not introduced by this PR), but visible in the touched hunk: lines 2528-2529 both putDirect the key "excludeEnv" — the second, whose value is "SIGUSR2", was almost certainly meant to be "signal". As-is, process.report.signal is undefined and process.report.excludeEnv is the string "SIGUSR2" instead of boolean false. Worth fixing while you're in this function, but shouldn't block the PR.

Extended reasoning...

What the bug is

In constructProcessReportObject (BunProcess.cpp:2528-2529), two consecutive putDirect calls use the identical key "excludeEnv":

report->putDirect(vm, JSC::Identifier::fromString(vm, "excludeEnv"_s), JSC::jsBoolean(false), 0);
report->putDirect(vm, JSC::Identifier::fromString(vm, "excludeEnv"_s), JSC::jsString(vm, String("SIGUSR2"_s)), 0);

The second call overwrites the first. Node's process.report exposes a signal property that defaults to 'SIGUSR2' and an excludeEnv boolean that defaults to false, so the second line's key was clearly meant to be "signal" — this is a copy-paste slip.

Step-by-step

1. User evaluates process.report → the lazy PropertyCallback constructProcessReportObject runs.
2. Line 2528 sets report.excludeEnv = false.
3. Line 2529 sets report.excludeEnv = "SIGUSR2", overwriting step 2.
4. No line ever sets report.signal.
5. Observable result: process.report.excludeEnv === "SIGUSR2" (string, wrong type) and process.report.signal === undefined (missing).

Why nothing prevents it

putDirect on an existing own property silently replaces the slot; it doesn't warn or throw on duplicate keys. There is no test asserting the type of process.report.excludeEnv or the presence of process.report.signal — the existing process.report test just calls JSON.stringify(process.report.getReport()), and this PR's new test only checks typeof process.report === "object".

Impact

Node-compat divergence on process.report: code that reads process.report.signal (e.g., to know which signal triggers report generation) gets undefined, and code that branches on the boolean process.report.excludeEnv sees a truthy string instead of false. Low practical impact since process.report is largely a stub in Bun, but it's a straightforward correctness bug.

Fix

Change the second key to "signal":

report->putDirect(vm, JSC::Identifier::fromString(vm, "excludeEnv"_s), JSC::jsBoolean(false), 0);
report->putDirect(vm, JSC::Identifier::fromString(vm, "signal"_s), JSC::jsString(vm, String("SIGUSR2"_s)), 0);

Relation to this PR

These lines are unchanged context in the diff — git blame attributes them to a commit predating this PR. This PR only replaced the RETURN_IF_EXCEPTION(scope, {}) on the line immediately below with the new clearAndReportLazyPropertyException helper. The bug is unrelated to the exception-handling change; it's flagged only because it sits inside the touched hunk and is trivial to fix while here.