process: throw from a lazy property whose builder fails instead of storing undefined - #39436
process: throw from a lazy property whose builder fails instead of storing undefined#39436robobun wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: fixed; rebased onto main after #29642 landed (head 09056d8: this PR no longer touches Reproduced with the released binary and with a debug build of main by reading Open point for whoever lands this: the engine-side assertion described in the body (#37001) is hit by assertion builds when |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes exception-propagation semantics across the process lazy-property builders and depends on specific JSC-side handling (reifyStaticProperty / setUpStaticFunctionSlot behavior with a pending exception, TopExceptionScope vs ThrowScope), a human look would still be worthwhile.
Checked: each changed builder still uses TopExceptionScope so the JSC vm.exceptionForInspection() caller contract holds; the new getIndex checks in constructStdioWriteStream cover both reads; the getConsoleStdout/getConsoleStderr fix correctly replaces the dead if (!value) guard (JSObject::get returns undefined, not empty); the forEachProperty reorder clears the exception before the continue so the next static-table lookup doesn't run with one pending. The four new tests each assert the specific broken behavior (undefined property, uncaught-exception report, exit 1) and are stated to fail on the unfixed build.
Extended reasoning...
Overview
This PR changes how eight lazy PropertyCallback builders on the process object (callLazyProcessBuilder, constructStdioWriteStream, constructStdin, constructNextTickFn, constructEnv, constructProcessConfigObject, constructMainModuleProperty, Process_stubEmptyArray) respond when the underlying JS they call throws: instead of clearing the exception, reporting it via reportUncaughtExceptionAtEventLoop, and returning jsUndefined() (which reifyStaticProperty then permanently stores), they now RETURN_IF_EXCEPTION(scope, {}) so the exception propagates, the property stays un-reified, and the next read retries the builder. It also fixes getConsoleStdout/getConsoleStderr to propagate rather than cache undefined, and reorders CLEAR_IF_EXCEPTION in forEachProperty so a failed lookup's exception is cleared before the next static-table lookup runs. Four subprocess tests cover the stack-overflow path, Bun.inspect(process) during unwind, a non-stack-overflow builder failure (Set removed), and the bulk-reification (delete process.x) path.
Security risks
None identified. This is error-handling plumbing; no user input parsing, no auth/crypto, no new external surface.
Level of scrutiny
High. Per the repo's review rules, exception-check placement in JSC bindings is the most-blocked category. The correctness of this change hinges on JSC-side invariants the PR description cites (oven-sh/WebKit#282/#306 landed via #34669): that reifyStaticProperty stores nothing on an empty result, that setUpStaticFunctionSlot reports not-found so the caller throws, and that reifyAllStaticProperties stops at the failing entry. The PR keeps TopExceptionScope in the builders because JSC's caller checks via vm.exceptionForInspection() rather than a scope check — that reasoning is sound but is exactly the kind of subtle contract a maintainer should confirm against the pinned WebKit. The change also affects the node:worker_threads preload's bulk reification path.
Other factors
The PR description is unusually thorough — it names the mechanism, the JSC PRs that enabled it, related/superseded PRs (#30612, #37258, #38821, #29642), and lists what was run under BUN_JSC_validateExceptionChecks=1. The tests are well-constructed (subprocess-isolated, assert stderr empty and exit 0, exercise both single-read and bulk-reify paths, and include a non-stack-overflow trigger so the test isn't only about the RangeError case). The bug-hunting pass found nothing. Still, this is a semantic change to error handling across a widely-used object, not a mechanical fix, so it should not be auto-approved.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes exception-propagation semantics for the process lazy property builders and relies on a specific contract in the pinned JSC fork (reifyStaticProperty / setUpStaticFunctionSlot handling an empty builder result), a human familiar with the WebKit fork should confirm before merge.
What was reviewed:
- The eight builder sites in
BunProcess.cppnow match the pattern already used byconstructVersions/constructReportand theBun.*builders —RETURN_IF_EXCEPTION(scope, {})under aTopExceptionScope. getConsoleStdout/getConsoleStderr: the oldif (!value)guard was dead becauseJSObject::getreturnsundefinedon failure, not empty; the newThrowScope+RETURN_IF_EXCEPTIONis the right check and prevents cachingundefinedintoconsole._stdout/_stderr.forEachPropertyinbindings.cpp: movingCLEAR_IF_EXCEPTIONbefore thecontinueis correct — with a pending exception the nextgetPropertySlotmisbehaves; ruled out that this drops the not-found check (it doesn't,hasPropertyis captured first).- Callers of
constructNextTickFnviaqueueNextTickalreadyRETURN_IF_EXCEPTIONafter theget, so the new propagation is handled there.
Extended reasoning...
Overview
The PR replaces the clear-and-report-and-store-undefined pattern in eight process lazy property builders (callLazyProcessBuilder, constructStdioWriteStream, constructStdin, constructNextTickFn, constructEnv, constructProcessConfigObject, constructMainModuleProperty, Process_stubEmptyArray) with RETURN_IF_EXCEPTION(scope, {}), so a builder failure surfaces as an ordinary throw at the read site and the property stays lazy for the next read. It also fixes getConsoleStdout/getConsoleStderr to propagate instead of caching undefined, moves the exception-clear in forEachProperty so a failed lookup doesn't poison the next one, adds two getIndex exception checks in constructStdioWriteStream, updates a fixture comment, and adds four subprocess tests.
Security risks
None identified. This is error-propagation plumbing; no new inputs are parsed, no auth/crypto/permissions touched.
Level of scrutiny
High. This is C++ JSC bindings code on a very hot object (process), and the correctness of returning empty from a PropertyCallback builder depends on the pinned WebKit fork's reifyStaticProperty / setUpStaticFunctionSlot / reifyAllStaticProperties handling that case (oven-sh/WebKit#282, #306 per the description). I could not verify those files locally (the WebKit vendor tree is not present in this checkout), so the contract is taken on the PR description's word. The change is consistent with existing builders (constructVersions, constructReport) that already use this pattern, which is strong circumstantial evidence, but a maintainer who knows the fork should confirm — especially the bulk-reification path used by the node:worker_threads preload.
Other factors
- The mechanical transformation is uniform and net-negative in lines; each site keeps its
TopExceptionScope(comment explains why: JSC checks withvm.exceptionForInspection(), which doesn't satisfy aThrowScope). queueNextTick(BunProcess.cpp:4291) readsnextTickviagetunder aThrowScopeand already hasRETURN_IF_EXCEPTIONafter it, so callers of the changedconstructNextTickFnare covered.- The
forEachPropertychange is a two-line reorder that only widens when the clear runs; behaviour whenhasPropertyis true is identical. - Tests are subprocess-based, drain both pipes concurrently, assert stderr empty and exit code 0, cover the RangeError path, a non-stack-overflow builder failure, bulk reification via
delete, andBun.inspect(process)(Windows-skipped with a stated reason). The author verified they fail on an unfixed build. - The comment-cop bot's remarks about long comments were addressed in 4e52164 and all threads are resolved.
- CI was still noted as pending in the timeline.
Given the reliance on JSC fork internals and the breadth of behaviour touched (every lazy process property, console._stdout/_stderr, Bun.inspect), this warrants a human sign-off rather than an automated approval.
|
This PR supersedes #30612 and #37258; I am closing both in favour of it. What I checked against a debug build of this branch (4e52164):
One suggestion: the four tests here reach the properties through JS reads, while #30612 was about the internal let atLimit;
function recurse() {
try {
recurse();
} catch {
if (atLimit !== undefined) return;
try {
atLimit = typeof process.nextTick;
} catch (e) {
atLimit = "threw " + e.constructor.name;
}
}
}
recurse();
process.on("worker", () => {
console.log(JSON.stringify({ atLimit, workerEvent: true, nextTick: typeof process.nextTick }));
w.terminate();
});
let w;
try {
w = new Worker("data:text/javascript,");
} catch (e) {
console.log(JSON.stringify({ atLimit, constructorThrew: String(e) }));
} |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes exception-propagation semantics across the process lazy-property builders and forEachProperty, relies on specific pinned-WebKit behavior in reifyStaticProperty/setUpStaticFunctionSlot, and has a stated assertion-build abort window until #37001 lands, a human look would still be worthwhile.
What was reviewed:
- The eight builder sites in
BunProcess.cpp— each keepsTopExceptionScopeand swaps clear+report+jsUndefined()forRETURN_IF_EXCEPTION(scope, {}); the two newgetIndexchecks inconstructStdioWriteStreamcomplete the contract. getConsoleStdout/getConsoleStderr—DECLARE_THROW_SCOPEis correct for a custom getter (matchesgetConsoleConstructor);get()returnsundefinedon failure so the oldif (!value)was dead.forEachPropertyImpl— clearing before the!hasPropertycontinue matches the sibling loop at ~line 5810 that already does this.- Tests spawn subprocesses, drain stdout/stderr/exited concurrently, gate the stdio unwind cases on
!isDebug && !isASAN, and skip theBun.inspect(process)case on Windows with a stated reason.
Extended reasoning...
Overview
The PR changes how eight lazy PropertyCallback builders on process (BunProcess.cpp) respond to a JS exception during construction: instead of clearing the exception, reporting it as uncaught, and returning jsUndefined() (which reifyStaticProperty then permanently stores), they now return empty with the exception pending via RETURN_IF_EXCEPTION(scope, {}). The pinned JSC handles this by not storing the property and reporting the slot as not found, so the read throws and the next read retries the builder. The same pattern is applied to getConsoleStdout/getConsoleStderr in ZigGlobalObject.cpp (which were caching undefined because JSObject::get returns undefined rather than empty on failure), and forEachPropertyImpl in bindings.cpp is reordered so a pending exception from one property lookup is cleared before the next iteration. ~270 lines of new tests in process.test.js cover stack-overflow-at-first-read, retry-after-unwind, non-stack builder failures (Set deleted), bulk reification via delete, the Bun.inspect property walk, and the internal queueNextTick path via emitWarning.
Security risks
None identified. This is exception-propagation plumbing; no auth, crypto, permissions, or untrusted-input parsing is touched.
Level of scrutiny
High. The change is small in line count but sits at the JSC/Bun boundary and changes user-observable semantics: what was previously an uncaught-exception report plus a permanently-undefined property is now a throw at the read site with retry-on-next-read. Correctness depends on specific behavior in the pinned WebKit fork (reifyStaticProperty not storing on empty return, setUpStaticFunctionSlot checking vm.exceptionForInspection(), reifyAllStaticProperties stopping without setting staticPropertiesReified) — behavior the PR body traces to oven-sh/WebKit#282/#306 landed via #34669. The PR also documents a known assertion-build abort window (stdio builders that partially reify process.nextTick before overflowing) that remains until #37001's getPropertySlot change is in the pin, and gates the affected tests to release builds accordingly. That kind of engine-contract dependency and cross-PR ordering is exactly what a maintainer should sign off on.
Other factors
- The change is consistent with existing builders (
versions,report,Bun.*) that already use this pattern, and thebindings.cppreorder matches the sibling loop in the same file. - The comment-cop bot flagged long comments; the author trimmed them in 4e52164 and all four threads are resolved.
- The PR supersedes #30612 and #37258 (author is closing both) and overlaps with #38821 (
constructEnv) and #29642 (identicalbindings.cpphunk) — merge-order coordination is a human decision. - Tests are thorough and follow harness conventions (
describe.concurrent,bunEnv/bunExe,Promise.allon stdout/stderr/exited, exact-value assertions on JSON output, exit code asserted last, per-case skips with stated reasons). The author states every case fails on the unfixed build and ranBUN_JSC_validateExceptionChecks=1on the repro scripts. - No prior
claude[bot]review on this PR.
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. The change itself is coherent and well-tested, but it is a deliberate policy reversal for how process lazy-property builder failures propagate (the prior clear-and-report was kept intentionally per #31831), it relies on specific behavior in the pinned WebKit's reifyStaticProperty/setUpStaticFunctionSlot, and it has an explicit merge-ordering question with #37001 (assertion builds can abort in a narrow window if this lands first). A human should sign off on the policy and the landing order.
What was reviewed:
- The eight
BunProcess.cppbuilders nowRETURN_IF_EXCEPTION(scope, {})— matches the contract described forreifyStaticPropertyreturning withoutputDirecton empty. getConsoleStdout/getConsoleStderr: replacingif (!value)withRETURN_IF_EXCEPTIONis correct sinceJSObject::getreturnsundefined(not empty) on throw.forEachPropertyinbindings.cpp: reorderingCLEAR_IF_EXCEPTIONbefore the!hasPropertycontinue is safe; the slot is only used whenhasPropertyis true.- New tests use
describe.concurrentwith per-case subprocesses, drain stdout/stderr/exited together, and gate the stdio unwind cases on!isDebug && !isASANfor the stated #37001 reason.
Extended reasoning...
Overview
The PR changes the failure policy of the lazy process.* property builders in src/jsc/bindings/BunProcess.cpp (callLazyProcessBuilder, constructStdioWriteStream, constructStdin, constructNextTickFn, constructEnv, constructProcessConfigObject, constructMainModuleProperty, Process_stubEmptyArray) from clear-exception + reportUncaughtExceptionAtEventLoop + return jsUndefined() to RETURN_IF_EXCEPTION(scope, {}). It also fixes getConsoleStdout/getConsoleStderr in ZigGlobalObject.cpp to propagate the exception instead of caching undefined, reorders the exception clear in forEachProperty (bindings.cpp), updates a fixture comment, and adds a ~270-line test suite.
Security risks
None identified. This is exception-propagation plumbing in the JSC bindings; no auth, crypto, untrusted-input parsing, or privilege boundaries are touched.
Level of scrutiny
High. This is not a mechanical fix — it reverses a policy that #31831 explicitly kept ("out of caution about the worker preload's bulk reification"), and correctness depends on the pinned WebKit fork's reifyStaticProperty / setUpStaticFunctionSlot / reifyAllStaticProperties handling empty builder results the way the PR body describes. The PR also documents a known assertion-build abort window that exists until #37001 lands, and interacts with four other open PRs (#29642, #38821, #38987, #37001) plus two it supersedes (#30612, #37258). A maintainer should confirm the policy is the one they want and decide the landing order relative to #37001.
Other factors
CI is green on all 179 lanes per the robobun status comment. The comment-cop bot feedback about paragraph-long comments was addressed (all threads resolved). Tests are thorough — they cover direct reads at the stack limit, console._stdout/_stderr, Bun.inspect(process), bulk reification via spread, the internal queueNextTick path via emitWarning, and retry-while-unwinding — and the author verified 0/8 (debug) and 0/11 (release) fail on the unfixed build. The TopExceptionScope choice is justified (JSC checks via vm.exceptionForInspection(), which does not satisfy a ThrowScope). Nothing looks wrong to me; the deferral is purely because this is a cross-cutting policy change with an open engine-side dependency and an explicit "open point for whoever lands this" from the author.
…oring undefined The PropertyCallback builders behind process.stdout, stderr, stdin, nextTick, finalization, allowedNodeEnvironmentFlags, channel, env, config, mainModule, _preload_modules and moduleLoadList cleared the exception thrown while building the value, reported it as an uncaught exception and returned undefined, which reifyStaticProperty stored for good. A builder that failed once (for example because it was first read from the catch block of a stack overflow) left the property undefined for the rest of the process and made it exit with code 1. Return empty with the exception pending instead, like the other process and Bun builders: JSC stores nothing, the read throws, and the next read runs the builder again. console._stdout / console._stderr check for that exception instead of caching the undefined that JSObject::get returns on failure, and forEachProperty clears the exception a failed lookup left before looking up the next property, since a pending exception makes the next static table lookup report its property as missing after reifying it.
On Windows process.env is built by a JS builtin and carries a JS custom inspect function; both still abort the process when run at the stack limit (LazyProperty initializers), which this test would hit before reaching what it covers.
…ile unwinding The property walk case makes one builder fail without involving the stack (Set removed), so it observes the forEachProperty change on every lane: the properties declared after the failed one must still be listed. process.emitWarning covers Process::queueNextTick, the internal consumer of the nextTick builder. The unwinding cases read the property again every 64 frames until a read has enough stack; the stdio ones only run on builds without assertions, since those builders add a property to process before they can still fail, which the pinned engine's getPropertySlot asserts on. The inspect-at-the-limit case now inspects once, at the deepest frame.
…ugProcess Spreading process reifies the table the same way and does not depend on a particular stub staying a table entry.
5b7f858 to
09056d8
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the failure policy of the process lazy-property builders in JSC bindings and carries an explicit landing-order dependency (#37001, plus overlaps with #38821/#38987), a human should decide the landing order and confirm the assertion-build abort window described in the body is acceptable.
What was reviewed:
- The eight builder sites in
BunProcess.cpp— each swaps clear-and-report +jsUndefined()forRETURN_IF_EXCEPTION(scope, {}); theTopExceptionScopeis retained so JSC'svm.exceptionForInspection()check is satisfied. getConsoleStdout/getConsoleStderr— theif (!value)guard was dead (JSObject::getreturnsundefined, not empty), so the newRETURN_IF_EXCEPTIONis the correct replacement.- New tests: assertions are non-vacuous, subprocess pipes are drained concurrently, release-only cases are gated on
!isDebug && !isASAN, and the Windows skip has a stated reason.
Extended reasoning...
Overview
The PR touches src/jsc/bindings/BunProcess.cpp (8 lazy PropertyCallback builders), src/jsc/bindings/ZigGlobalObject.cpp (console._stdout/_stderr custom getters), a test fixture comment, and adds a ~270-line describe.concurrent block to process.test.js. Each builder site is a mechanical swap: the clear-exception → reportUncaughtExceptionAtEventLoop → return jsUndefined() block becomes RETURN_IF_EXCEPTION(scope, {}), so a failed builder leaves the exception pending and returns empty. JSC's reifyStaticProperty then stores nothing, the read throws, and the next read reruns the builder. constructStdioWriteStream also gains exception checks after its two getIndex calls so the whole function honours the same contract. The console getters replace a dead if (!value) check (JSObject::get never returns empty) with a proper ThrowScope + RETURN_IF_EXCEPTION.
Security risks
None identified. This is error-propagation plumbing; no new inputs are parsed and no trust boundary is crossed.
Level of scrutiny
High. Although each hunk is small and repetitive, this is a policy change in core JSC bindings that reverses a deliberate prior choice (#31831 kept clear-and-report out of caution about worker-preload bulk reification). The correctness depends on engine behaviour in the pinned WebKit (reifyStaticProperty / setUpStaticFunctionSlot / reifyAllStaticProperties all handling an empty builder result), which the PR body cites but which a maintainer should confirm against the current pin. The PR also names an open engine-side issue (#37001) that makes assertion builds abort in a specific stack-depth window if this lands first — that landing-order call is a human decision.
Other factors
CI was reported green on an earlier commit (build 100130, 179/179) with only a test-trigger tweak since. The tests are thorough (direct read, bulk reification via spread, Bun.inspect walk, internal queueNextTick path via emitWarning, retry-while-unwinding) and the author verified 0/8 → 8/8 on debug and 0/11 → 11/11 on release. The comment-cop bot's flags were addressed (comments trimmed to one line). The PR body explicitly maps overlaps with four other open PRs and proposes a landing order — coordinating that is outside what automated review should decide.
1 participant
Problem
process.stdout,stderr,stdin,nextTick,finalizationandallowedNodeEnvironmentFlagsstayundefinedfor the rest of the process when they are first read at a moment their builder cannot run, and the process exits with code 1 although nothing was left uncaught. Repro: read one of them for the first time from thecatchof aRangeError: Maximum call stack size exceeded.(logging from such a catch block does that), orconsole.log(process)there; stderr shows oneRangeError: Maximum call stack size exceeded.uncaught-exception report per builder. Internal users break the same way: after one failed build everyprocess.emitWarning(),new Worker(), ... fails withFailed to call nextTick.src/jsc/bindings/BunProcess.cpp(callLazyProcessBuilder,constructStdioWriteStream,constructStdin,Process::constructNextTickFn, and with the same codeconstructEnv,constructProcessConfigObject,constructMainModuleProperty,Process_stubEmptyArray) clear the exception, pass it toreportUncaughtExceptionAtEventLoopand returnjsUndefined(), whichreifyStaticPropertystores as the property's value, so the builder never runs again.getConsoleStdout/getConsoleStderr(src/jsc/bindings/ZigGlobalObject.cpp) have the same bug one level up: when theprocess.stdoutread throws,JSObject::getreturnsundefined(not an empty value, so the existingif (!value)never fires) and the getter caches it inconsole._stdout.Fix
RETURN_IF_EXCEPTION(scope, {})), like the otherprocessbuilders (versions,report) and theBun.*builders. The pinned JSC is built for this:reifyStaticPropertystores nothing for an empty result,setUpStaticFunctionSlotreports the slot as not found so the read throws the builder's error, and the next read runs the builder again.constructStdioWriteStreamalso checks its twogetIndexreads so the whole function has that contract.reifyAllStaticProperties, used bydelete process.xand spread): JSC stops at the failing builder, leaves the rest lazy and the triggering operation throws. Thenode:worker_threadspreload, the one internal user, wraps eachdeleteintry/catch, and nothing can make these builders fail there in the normal case. The one-line comment oncallLazyProcessBuilderrecords the remaining constraint: builders keep aTopExceptionScope, because JSC'svm.exceptionForInspection()check does not satisfy aThrowScope.console._stdout/console._stderrpropagate the exception instead of cachingundefined.Bun.inspect/console.logof an object with a failed builder needed the property walk to clear the failed lookup's exception before the next lookup (otherwise the lazy properties declared after the failed one are dropped, and builds with assertions abort withASSERTION FAILED: isCompilationThread() || Thread::mayBeGCThread() || object->structure() == thisinStructure::storedPrototype). That landed separately in Fix crash and dropped properties in Bun.inspect when a property lookup or getPrototypeOf throws during the property walk #29642; this PR originally carried the same two lines and dropped them when rebasing onto it. The two inspect cases below exercise that onprocess, which with this change is the first object whose builders fail in ordinary programs.processand then throws. The stdio builders do that when they get far enough to load the stream modules (which readprocess.nextTick) and then run out of stack, i.e. whenprocess.stdoutis first read a few hundred frames above the limit rather than at it. Release builds are unaffected (the stale structure has the same prototype, the read throws and the next one works; on main the property is lost for good instead), builds with assertions abort there until Fix stale-structure abort when a lazy Bun property builder throws mid-lookup #37001'sgetPropertySlotchange is in the pin. Landing Fix stale-structure abort when a lazy Bun property builder throws mid-lookup #37001 first avoids that window; if this lands first, that scenario is a known assertion-build abort until then (the tests below that reach it are limited to release builds for that reason).test/js/node/process/process.test.js>lazy process properties whose builder throws. Every case fails on the unfixed build (0 pass / 8 fail on a debug build without thesrc/changes, 0 / 11 on the release binary, where the three release-only cases run too):console._stdout/_stderrfrom a stack overflow catch block throwsRangeError; the next read builds them,console._stdout === process.stdout;process.emitWarning()at the limit throws the builder'sRangeError(the internalProcess::queueNextTickpath, which process: retry nextTick initialization in queueNextTick after failed first reification #30612 patched separately) and a later warning is delivered;process.nextTickandprocess.allowedNodeEnvironmentFlags, and on release buildsstdout/stderr/stdin, read again every 64 frames while unwinding: the first reads fail, a later one builds the property;Bun.inspect(process)at the limit leaves the same undefined properties as a process that never overflowed. Skipped on Windows: thereprocess.envis built by a JS builtin and has a JS custom inspect function, and both building and inspecting it at the stack limit abort the current canary too (0xC0000409, them_processEnvObject/m_utilInspectFunctionLazyPropertyinitializers, process.env: make a failed env build throw instead of aborting (Windows 0xC0000409 on Bun.$ / Bun.sql / process.env near the stack limit) #38821 / Make a failed util.inspect load throw from custom inspect functions instead of aborting #39382);Setremoved;allowedNodeEnvironmentFlagsbuilds one) throwsTypeError, does not call anuncaughtExceptionlistener, and the read works onceSetis back;{ ...process }(bulk reification, as adeletewould do) throws from the spread; afterwards a spread copies the property and the remaining properties work;Bun.inspect(process)hides exactly that one property (Fix crash and dropped properties in Bun.inspect when a property lookup or getPrototypeOf throws during the property walk #29642's walk behaviour onprocess: before it,loadEnvFile,finalizationandarchdisappeared as well, or the assertion above fired; verified against a build without those two lines before the rebase). Runs on every platform, nothing stack related in it.process.test.jsand theprocess-*tests (the existing stack overflow fixture's comment is updated; one pre-existing failure becauseUSERis unset in this container),worker_threads.test.ts,worker-transfer-terminate-stress.test.ts,worker-terminate-lifetime.test.ts(its dns case reports anode_fs_bindingleak identically without this change),test-worker-message-port-transfer-terminate.jsx3,inspect.test.js,BunObject.test.ts,console.test.ts. The repro scripts, plus a variant that removesSetbefore the stdio builders run (their module loading fails, they rebuild correctly afterwards), are clean underBUN_JSC_validateExceptionChecks=1.Overlap with open PRs
This PR is the one carrying the failure policy of the
processbuilders (BunProcess.cpp) and the dependent console getters. For the hunks it shares with other open PRs:Bun.sqlbuilders) has landed; the rebase onto it dropped this PR's copy of the walk change (the only conflict:bindings.cpp, resolved by taking main), so this PR is nowBunProcess.cpp, the console getters and the tests.BunProcess.cppin places this PR rewrites: process.env: make a failed env build throw instead of aborting (Windows 0xC0000409 on Bun.$ / Bun.sql / process.env near the stack limit) #38821'sconstructEnvhunk and its comments describing the clear-and-report policy, worker_threads: don't build the process object's lazy properties during worker startup #38987's rewrite of the comment abovecallLazyProcessBuilder. Suggested order: this PR first; those hunks then disappear from both on rebase and the rest of them is unaffected (process.env: make a failed env build throw instead of aborting (Windows 0xC0000409 on Bun.$ / Bun.sql / process.env near the stack limit) #38821'sprocess.reporthunks, the env map and initializer work; worker_threads: don't build the process object's lazy properties during worker startup #38987's table and preload changes). In the other order this PR drops theconstructEnvhunk and takes the one-line comment, nothing else. The bulk reification test here uses a spread rather than deleting_debugProcess, which worker_threads: don't build the process object's lazy properties during worker startup #38987 removes from the table. A note with this is on both PRs.nextTickcase insidequeueNextTick) are closed in favour of this PR; theemitWarningcase here is the regression test for process: retry nextTick initialization in queueNextTick after failed first reification #30612's internal path.Background
process's properties come from a static hash table (processObjectTable). APropertyCallbackentry is not built whenprocessis created: the first read calls its builder and stores the result as an ordinary own property (reifyStaticProperty). Several builders run a JS builtin (getStdioWriteStream,initializeNextTickQueue, ...), so they can throw, for instance when there is no stack left to enter JS.eeab0404,scripts/build/deps/webkit.ts; same lines as in the previous pin, andgetPropertySlotstill uses the structure it read before the lookup, so the Fix stale-structure abort when a lazy Bun property builder throws mid-lookup #37001 point is unchanged):runtime/Lookup.h, thePropertyCallbackbranch ofreifyStaticProperty, returns withoutputDirectwhen the builder returned empty;runtime/Lookup.cpp,setUpStaticFunctionSlot, runs the reify underDeferTerminationForAWhileand returns false (slot not found) whenvm.exceptionForInspection()is set afterwards;runtime/JSObject.cpp,reifyAllStaticProperties, does the same per entry and returns without settingstaticPropertiesReified, so the next bulk operation continues with the entries that are left.RETURN_IF_EXCEPTION). A function propagates a failure by returning an empty value with the flag set; once control is back in JS the exception is thrown there.JSObject::getis the exception to the empty-value convention: it returnsundefinedon failure.ThrowScopevsTopExceptionScope: in exception-checking builds aThrowScopepretends its function threw whenever it returns, so a caller without a check is flagged; aTopExceptionScopedoes not. JSC checks a builder's result withvm.exceptionForInspection(), which is not such a check, henceTopExceptionScopein the builders.reifyAllStaticPropertiesbuilds every remaining entry of the table at once; JSC does this when an operation cannot be answered from the table, such as deleting one of its properties or spreading the object.Structure; adding a property moves the object to a new one.JSObject::getPropertySlotreads the structure once per object in the prototype chain and, when the own lookup misses, asks that structure for the prototype. A builder that added a property and then failed makes that read stale, which assertion builds check for. Fix stale-structure abort when a lazy Bun property builder throws mid-lookup #37001 makes it re-read the structure.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/node/process/process.test.js