Conversation
WalkthroughImplements worker resource limit parsing and exposure, heap-limit enforcement with OOM termination, worker termination propagation, and updated Changesworker_threads resourceLimits enforcement and OOM termination
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 4147d7b has some failures in 🧪 To try this PR locally: bunx bun-pr 32896That installs a local version of the PR into your bun-32896 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/worker_threads.ts (1)
491-553: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse one immutable
resourceLimitssnapshot for both JS and native.
makeResourceLimits(options?.resourceLimits)reads the user object beforenew WebWorker(...), but native construction reparsesoptions.resourceLimitsagain. Getter/proxy inputs can therefore run twice, andworker.resourceLimitscan drift from the limit the worker actually received. Returning the same#resourceLimitsobject also lets callers mutate future reads. Normalize once to a plain snapshot, feed that snapshot to native, and return a clone/snapshot from the getter.🤖 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/js/node/worker_threads.ts` around lines 491 - 553, The resource limits handling in the Worker constructor and resourceLimits getter should use a single immutable snapshot instead of re-reading the user input. Normalize options?.resourceLimits once in the Worker setup before calling new WebWorker, pass that same plain snapshot into native construction, and keep it in `#resourceLimits` so getters and native receive identical values. Also update resourceLimits to return a fresh clone/snapshot rather than the stored object, and reference the Worker constructor and resourceLimits getter when making the change.
🤖 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 633-697: Add a regression test around Worker.resourceLimits that
verifies it is read/snapshotted only once: use a getter/proxy-backed
resourceLimits option on Worker and assert the getter is observed a single time,
then mutate the original object and confirm worker.resourceLimits stays
unchanged. Place the new case alongside the existing resourceLimits tests in
worker_threads.test.ts, reusing the Worker and resourceLimits symbols to keep
the suite covering the snapshot contract.
---
Outside diff comments:
In `@src/js/node/worker_threads.ts`:
- Around line 491-553: The resource limits handling in the Worker constructor
and resourceLimits getter should use a single immutable snapshot instead of
re-reading the user input. Normalize options?.resourceLimits once in the Worker
setup before calling new WebWorker, pass that same plain snapshot into native
construction, and keep it in `#resourceLimits` so getters and native receive
identical values. Also update resourceLimits to return a fresh clone/snapshot
rather than the stored object, and reference the Worker constructor and
resourceLimits getter when making the change.
🪄 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: 39251612-2b5a-44ed-98f2-7e44d769406f
📒 Files selected for processing (12)
src/js/node/worker_threads.tssrc/jsc/ErrorCode.rssrc/jsc/bindings/ErrorCode.tssrc/jsc/bindings/NodeVMModule.cppsrc/jsc/bindings/NodeVMScript.cppsrc/jsc/bindings/webcore/JSWorker.cppsrc/jsc/bindings/webcore/Worker.cppsrc/jsc/bindings/webcore/Worker.hsrc/jsc/bindings/webcore/WorkerOptions.hsrc/jsc/web_worker.rstest/js/node/worker_threads/worker_threads.test.tstest/js/web/workers/worker.test.ts
|
Note Superseded by the rebase onto main. Both of these were fixed on main after this PR's old base commit (#33017 for the tsgo resolution, #33027 for the svelte CJS wrapper assertion), so they should now be green. The analysis below was accurate for the pre-rebase head. A note on the two red GitHub Actions checks, since they will stay red regardless of what this PR does: both are broken on main, at this PR's exact base commit (8706328), and neither involves any file this PR touches.
This PR's edit in that subtree is seven lines adding a |
There was a problem hiding this comment.
All prior review feedback has been addressed and no new issues found, but this touches worker VM lifecycle, cross-thread GC observer callbacks, and node:vm termination propagation across C++/Rust/JS — worth a human pass before merge.
Extended reasoning...
Overview
This PR implements resourceLimits for node:worker_threads: parsing the option in the Worker constructor, exposing worker.resourceLimits on both parent and worker sides, and enforcing maxOldGenerationSizeMb via a JSC::HeapObserver registered on the worker VM that terminates the worker with ERR_WORKER_OUT_OF_MEMORY when a Full GC ends over the limit. It also fixes a pre-existing process abort where worker.terminate() during a no-timeout vm.runInContext hit RELEASE_ASSERT_NOT_REACHED in checkForTermination. 13 files changed across C++ (Worker.cpp/h, JSWorker.cpp, WorkerOptions.h, NodeVMScript.cpp, NodeVMModule.cpp), Rust (web_worker.rs, ErrorCode.rs), TypeScript (worker_threads.ts, ErrorCode.ts), types, and tests.
Security risks
None identified. The feature adds a per-worker heap cap that restricts resource usage rather than expanding any surface. No auth, crypto, permissions, or untrusted-input parsing beyond reading numeric options (which are gated on isNumber() and clamped before casting).
Level of scrutiny
High. The implementation touches several genuinely tricky areas: a HeapObserver whose didGarbageCollect can fire on JSC's dedicated heap-collector thread (not the mutator), new pointer-passing FFI between Rust and C++ for cross-thread atomic flag access, modifications to the worker shutdown/teardown ordering (WebWorker__teardownJSCVM, dispatchExit), and changes to how node:vm's termination-exception handling distinguishes worker-level termination from script timeout/SIGINT. These are exactly the kinds of concurrency and lifecycle paths where subtle ordering bugs hide.
Other factors
The PR went through roughly seven review rounds; every issue raised (Watchdog/node:vm conflict → redesigned to HeapObserver; ErrorCode index misalignment; collector-thread thread-local UB; teardown-collection guard regression; terminate() exit-code-0 falsy check; non-object/non-number input handling; stale comments) was addressed with code changes and tests, and all inline threads are resolved. The bug-hunting pass on the current head (a16610f) found nothing new. Test coverage is thorough (normalization, defaults, ignore cases, single-read snapshot contract, end-to-end OOM enforcement, OOM-inside-vm, terminate-during-vm regression). The two red GitHub Actions checks are documented as broken on the base commit and unrelated to this PR's edits. Given the breadth and the cross-thread/VM-lifecycle subject matter, a human reviewer familiar with the worker subsystem should sign off.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/jsc/bindings/webcore/Worker.cpp (1)
113-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim or relocate the extended rationale comments.
These new comment blocks exceed the repository’s 3-line limit. Keep only the invariant needed at the callsite and move the longer design notes to docs or the PR description. As per coding guidelines, "Keep code comments to 3 lines max."
Also applies to: 162-183, 692-696, 707-710, 780-783
🤖 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/webcore/Worker.cpp` around lines 113 - 142, The comment blocks in Worker.cpp are too long and should be shortened or moved out of the callsite; keep only the essential invariant near the affected logic in Worker::didGarbageCollect and relocate the detailed threading/design rationale to docs or the PR description. Apply the same 3-line-max cleanup to the other flagged comment blocks so the code stays within the repository comment guideline.Source: Coding guidelines
src/jsc/bindings/webcore/JSWorker.cpp (1)
298-318: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch Node’s
typeof === "object"gate here.isObject()also accepts callable function objects in JSC, soresourceLimitscan be parsed from a function value instead of being ignored like Node does. Add a callable check alongside the object check.🤖 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/webcore/JSWorker.cpp` around lines 298 - 318, The resourceLimits parsing in JSWorker::create currently uses isObject(), which also accepts callable function values and diverges from Node’s typeof === "object" behavior. Update the resourceLimitsValue gate to exclude function/callable values before entering the WorkerResourceLimits parsing block, keeping the existing readLimit lambda and options.resourceLimits assignment unchanged.test/js/node/worker_threads/worker_threads.test.ts (1)
783-797: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude piped
stderrin the grouped subprocess assertions.These tests capture
stderrbut drop it from the result object. Add it back asexpect.any(String)so crash/debug output is preserved in failure diffs without constraining benign stderr.Proposed adjustment
expect({ parentLimits: jsonAfter(lines[0], "parentLimits "), workerLimits: jsonAfter(lines[1], "message "), error: lines[2], exit: lines[3], + stderr, exitCode, }).toEqual({ @@ error: "error ERR_WORKER_OUT_OF_MEMORY Worker terminated due to reaching memory limit: JS heap out of memory {}", exit: "exit 1", + stderr: expect.any(String), exitCode: 0, });- expect({ stdout: stdout.trim().split("\n"), exitCode }).toEqual({ + expect({ stdout: stdout.trim().split("\n"), stderr, exitCode }).toEqual({ stdout: ["error ERR_WORKER_OUT_OF_MEMORY", "exit 1"], + stderr: expect.any(String), exitCode: 0, });- expect({ stdout, exitCode }).toEqual({ stdout: "exit 1\n", exitCode: 0 }); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: "exit 1\n", + stderr: expect.any(String), + exitCode: 0, + });As per coding guidelines, subprocess tests should assert a combined
{ stdout, stderr, exitCode }object. Based on learnings,expect.any(String)is acceptable when stderr is intentionally unconstrained.Also applies to: 831-834, 867-868
🤖 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/node/worker_threads/worker_threads.test.ts` around lines 783 - 797, The grouped subprocess assertions in the worker_threads tests are omitting piped stderr, so update the expectation object used in these cases to include stderr alongside stdout and exitCode. In the affected assertions around the worker resource-limits and crash cases, keep the existing checks for parentLimits, workerLimits, error, exit, and exitCode, but add stderr back as expect.any(String) where it is intentionally unconstrained so failure diffs preserve debug output. Reuse the same pattern in the other matching subprocess assertions referenced by the worker_threads test cases.Sources: Coding guidelines, Learnings
🤖 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 `@src/js/node/worker_threads.ts`:
- Around line 528-532: Trim the inline comments in worker_threads.ts to the
repo’s 3-line limit by keeping only the durable invariant about reading back
from native once and dropping the extra explanatory prose. Update the comment
block near the resourceLimits handling and the similar comment at the other
flagged location so they stay concise while still referencing the behavior of
WebWorker constructor and kHandle.getResourceLimits().
In `@src/jsc/bindings/webcore/JSWorker.cpp`:
- Around line 302-306: Shorten the new explanatory comments in JSWorker.cpp so
they fit the repository’s 3-line limit. Update the comment block near the
WorkerOptions::resourceLimits handling to keep only the essential point, and do
the same for the other affected comment block referenced in the review, leaving
extended rationale for docs/PR notes instead.
In `@test/js/node/worker_threads/worker_threads.test.ts`:
- Around line 659-662: Shorten the new inline test comments in
worker_threads.test.ts so each stays within the 3-line limit and remove
bug-history/PR-context prose such as references to RELEASE_ASSERT_NOT_REACHED.
Update the comments around the resourceLimits getter/Proxy test blocks to be
brief, descriptive, and focused only on the test behavior; keep any historical
rationale out of the test body. Make the same cleanup in the related comment
block near the other affected assertions so the test matches the coding
guideline for test/js/node/**.
---
Outside diff comments:
In `@src/jsc/bindings/webcore/JSWorker.cpp`:
- Around line 298-318: The resourceLimits parsing in JSWorker::create currently
uses isObject(), which also accepts callable function values and diverges from
Node’s typeof === "object" behavior. Update the resourceLimitsValue gate to
exclude function/callable values before entering the WorkerResourceLimits
parsing block, keeping the existing readLimit lambda and options.resourceLimits
assignment unchanged.
In `@src/jsc/bindings/webcore/Worker.cpp`:
- Around line 113-142: The comment blocks in Worker.cpp are too long and should
be shortened or moved out of the callsite; keep only the essential invariant
near the affected logic in Worker::didGarbageCollect and relocate the detailed
threading/design rationale to docs or the PR description. Apply the same
3-line-max cleanup to the other flagged comment blocks so the code stays within
the repository comment guideline.
In `@test/js/node/worker_threads/worker_threads.test.ts`:
- Around line 783-797: The grouped subprocess assertions in the worker_threads
tests are omitting piped stderr, so update the expectation object used in these
cases to include stderr alongside stdout and exitCode. In the affected
assertions around the worker resource-limits and crash cases, keep the existing
checks for parentLimits, workerLimits, error, exit, and exitCode, but add stderr
back as expect.any(String) where it is intentionally unconstrained so failure
diffs preserve debug output. Reuse the same pattern in the other matching
subprocess assertions referenced by the worker_threads test cases.
🪄 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: ea2e31ea-8079-4299-b467-f2966337155e
📒 Files selected for processing (6)
packages/bun-types/bun.d.tssrc/js/node/worker_threads.tssrc/jsc/bindings/webcore/JSWorker.cppsrc/jsc/bindings/webcore/Worker.cppsrc/jsc/bindings/webcore/Worker.htest/js/node/worker_threads/worker_threads.test.ts
There was a problem hiding this comment.
All prior review feedback has been addressed and this round found nothing new, but the cross-thread GC observer / worker VM lifecycle changes here are subtle enough that a human pass is warranted before merge.
Extended reasoning...
Overview
This PR implements worker_threads resourceLimits end-to-end: parsing in JSWorker.cpp, a new WorkerResourceLimits struct in WorkerOptions.h, a JSC::HeapObserver in Worker.cpp that enforces the heap cap after Full collections, new Rust↔C++ FFI in web_worker.rs (WebWorker__setRequestedTerminate / hasRequestedTerminate / currentWorkerHasRequestedTerminate / installHeapLimitObserver), ERR_WORKER_OUT_OF_MEMORY plumbing in ErrorCode.ts/ErrorCode.rs, propagateWorkerTermination in NodeVMScript.cpp/NodeVMModule.cpp, the parent-side worker.resourceLimits getter, and a terminate()-after-exit-0 fix in worker_threads.ts. 13 files, ~500 net lines.
Security risks
None identified. The change is resource-limit enforcement and error propagation; it does not touch auth, crypto, network, or filesystem permissions. The new FFI surface takes opaque pointers captured at install time and only performs atomic reads/writes.
Level of scrutiny
High. This is production-critical runtime code with non-trivial threading: HeapObserver::didGarbageCollect can fire on JSC's dedicated heap-collector thread (not the mutator), and the PR went through eight review rounds where exactly that class of bug was found and fixed (thread-local VM read on the collector thread → UB; Watchdog slot conflict with node:vm; teardown-collection guard regression; ErrorCode index shift). The final design — pointer-captured-at-install FFI, dedicated disarm atomic, sizeAfterLastFullCollection() instead of Heap::size() — looks correct and is well-commented, but the iteration history itself demonstrates this is the kind of change where a second pair of human eyes on the threading model is valuable.
Other factors
- All 14 of my prior inline comments are resolved; the author engaged substantively with each (including correctly pushing back on two as unreachable-today, with empirical verification).
- One acknowledged pre-existing issue (nested no-timeout
vm.runInContextinside an outer{timeout}still hitsRELEASE_ASSERT_NOT_REACHED) was deliberately left for a follow-up as out of scope. - Test coverage is thorough (normalization, defaults, single-read/Proxy, non-number/non-object ignore, OOM enforcement, OOM-inside-vm, terminate-inside-vm, terminate-after-exit-0).
- The two red CI checks (
bun-typestsgo,bun-plugin-svelte) are documented as broken on the base commit and unrelated. - coderabbit suggested dylan-conway as reviewer; no human review has landed yet.
There was a problem hiding this comment.
All prior feedback is addressed and nothing new turned up on this pass, but given the scope — a new HeapObserver running on JSC's collector thread, cross-thread Rust/C++ FFI for termination, and changes to node:vm's termination handling — this should get a human sign-off.
Extended reasoning...
Overview
This PR implements node:worker_threads resourceLimits: parsing in JSWorker.cpp, a WorkerResourceLimits struct in WorkerOptions.h, a JSC::HeapObserver in Worker.cpp that terminates the worker on heap-limit breach, new pointer-taking Rust FFI exports in web_worker.rs, the ERR_WORKER_OUT_OF_MEMORY error code (appended to both ErrorCode.ts and the Rust mirror), a propagateWorkerTermination helper in NodeVMScript.cpp/NodeVMModule.cpp so worker termination during vm.runInContext no longer hits RELEASE_ASSERT_NOT_REACHED, and a parent-side worker.resourceLimits getter that reads back from native. It also fixes a pre-existing terminate() bug where exit code 0 fell through a truthiness check. 13 files across C++, Rust, and TypeScript, with ~240 lines of new tests.
Security risks
None identified. The new surface (resourceLimits) is opt-in and only constrains the worker further; the FFI additions take an explicit pointer captured at install time rather than reading thread-locals; and the error-code table change is append-only with the Rust mirror updated in lockstep. No auth, crypto, permissions, or untrusted-input parsing beyond Node-compatible typeof === 'number' gating.
Level of scrutiny
High. The enforcement path runs a HeapObserver::didGarbageCollect callback on JSC's dedicated heap-collector thread (not the mutator), reaches back into Rust via raw-pointer FFI, and interacts with worker shutdown ordering (disarmHeapLimitObserver before the teardown collectNow, the never-cleared requested_terminate latch, and the m_state >= Closing probe for the parent-side getter). The node:vm change alters how TerminationException is classified inside script evaluation. These are exactly the kind of cross-thread lifecycle invariants that benefit from a maintainer's mental model of the worker subsystem.
Other factors
The PR went through seven rounds of automated review on this thread, which surfaced and fixed several real issues (the original Watchdog/node:vm slot conflict, an ErrorCode index shift, collector-thread UB from a thread-local read, a teardown-collection guard regression, and the terminate() falsy-zero hang). Every inline comment is now resolved, the latest commits are comment-trimming only, and the bug-hunting pass on the final revision found nothing. Test coverage for the new behavior looks solid (parent/worker getter parity, single-observation contract, OOM termination including inside vm.runInContext, and the terminate()-during-vm regression). The two red CI checks are documented as broken on the base commit. None of that changes the fact that this is a substantial, concurrency-sensitive feature addition rather than a mechanical change.
|
Current head: What changed since the last status (
Reproduced how: on main, |
d56256e to
3025e88
Compare
There was a problem hiding this comment.
Nothing further from me — all prior review threads are resolved and this round found no new issues. Given the cross-thread HeapObserver design (collector-thread callbacks reaching the native WebWorker via captured pointer, disarm-before-teardown ordering, and the VMTraps-based termination path), a maintainer should still sign off on the enforcement approach.
Extended reasoning...
Overview
This PR implements node:worker_threads resourceLimits: parsing in the Worker constructor (JSWorker.cpp) into a new WorkerResourceLimits struct, a worker.resourceLimits prototype getter and in-worker export both fed by createResourceLimitsObject(), and enforcement of maxOldGenerationSizeMb via a new JSC::HeapObserver (WorkerHeapLimitObserver) that terminates the worker with ERR_WORKER_OUT_OF_MEMORY after a Full GC ends over the limit. It also adds propagateWorkerTermination() in NodeVMScript.cpp/NodeVMModule.cpp so a worker-level TerminationException no longer hits RELEASE_ASSERT_NOT_REACHED, fixes a pre-existing terminate() truthiness bug for exit code 0, adds pointer-taking FFI in web_worker.rs (WebWorker__setRequestedTerminate/hasRequestedTerminate/installHeapLimitObserver), appends ERR_WORKER_OUT_OF_MEMORY to both ErrorCode tables, and de-flakes four pre-existing worker tests. ~500 net lines across 14 files (C++, Rust, TS builtin, tests, .d.ts).
Security risks
None identified. The heap limit is a resource cap on the worker's own VM; input parsing gates on isNumber() and clamps against SIZE_MAX before casting; the observer is only registered when a limit is configured, so unconfigured workers pay nothing. No auth, crypto, filesystem-path, or network-input handling is touched.
Level of scrutiny
High. This is production-critical cross-thread native code: the observer runs on JSC's dedicated heap-collector thread (where Bun's per-thread VM is not installed), so the FFI must be pure atomics on a pointer captured race-free at install time; teardown ordering (disarm before the final collectNow) matters for correctness; and the enforcement mechanism is a novel design choice (post-Full-GC observer + VMTraps, deliberately not JSC::Watchdog). The PR went through ~10 review rounds that surfaced real bugs — a Watchdog slot conflict with node:vm, an ErrorCode index shift, collector-thread unwrap_unchecked() UB, a natural-drain teardown race, and the terminate() falsy-zero hang — all now resolved. That history itself signals this is not the kind of change to merge without maintainer sign-off.
Other factors
All prior inline threads (mine and CodeRabbit's) are marked resolved. The current bug-hunting pass found nothing. Test coverage is thorough (7 new tests covering echo/defaults/single-parse/non-number/non-object/OOM/OOM-inside-vm/terminate-inside-vm, plus the terminate()-after-exit-0 regression). The PR description accurately documents what is enforced vs. echoed-only (codeRangeSizeMb/stackSizeMb), the cooperative overshoot caveat, and the collector-thread threading contract. The one open architectural question a maintainer should confirm is whether the post-Full-GC HeapObserver approach (vs. an allocator-side hard cap) is the intended enforcement semantics for Bun.
92d41eb to
648b8b0
Compare
There was a problem hiding this comment.
No issues found on this revision, but this warrants a human look — it's ~620 lines across JSC GC internals (a HeapObserver that runs on the collector thread), cross-thread Rust/C++ FFI, worker termination lifecycle, and node:vm's checkForTermination.
What was reviewed across the 13 iterations:
- Collector-thread safety of the observer: the initial
JSC::Watchdogapproach and the later thread-local-VM reads were both replaced with pointer-taking atomic FFI; verified nothing indidGarbageCollectnow depends on the mutator thread. - Teardown ordering: the observer is disarmed via a dedicated release/acquire atomic before
WebWorker__teardownJSCVM's finalcollectNow, so a naturally-drained worker isn't relabelled OOM. ErrorCode.ts—ERR_WORKER_OUT_OF_MEMORYis appended (not inserted), so the generated Rust mirror stays index-aligned.propagateWorkerTerminationin node:vm — checked the on_exit misclassification window; not currently reachable from user JS. The pre-existing nested-vm-timeoutRELEASE_ASSERTis left for a follow-up.
Extended reasoning...
Overview
This PR implements node:worker_threads resourceLimits: parsing in JSWorkerDOMConstructor::construct, a WorkerResourceLimits struct on WorkerOptions, a JSC::HeapObserver (WorkerHeapLimitObserver) registered on the worker VM that terminates the worker with ERR_WORKER_OUT_OF_MEMORY when a Full GC ends over the configured heap limit, a parent-side worker.resourceLimits prototype getter and worker-side export (both fed by the single native parse via createResourceLimitsObject), and a propagateWorkerTermination helper in NodeVMScript.cpp/NodeVMModule.cpp so a worker-level TerminationException inside vm.runInContext unwinds instead of hitting RELEASE_ASSERT_NOT_REACHED. Rust-side FFI adds pointer-taking WebWorker__{set,has}RequestedTerminate and WebWorker__installHeapLimitObserver so the observer never touches the per-thread VM. ~250 lines of tests cover normalization, defaults, single-read semantics, non-object/non-number silent-ignore, OOM enforcement (including inside vm.runInContext), and terminate()-during-vm.
Security risks
None identified. resourceLimits is a defense-in-depth mechanism (bounds a worker's heap), not an attack surface. Input validation matches Node's silent-ignore semantics; heapLimitBytes() clamps in double before casting to size_t. The observer is opt-in (not registered without a configured limit), so there's no cost or new code path for workers that don't use it.
Level of scrutiny
High. This touches:
- JSC GC internals:
HeapObserver::didGarbageCollectcan run on JSC's dedicated collector thread, requiring every operation there to be atomic-only with no thread-local access. The review history shows this was a genuine source of UB (two separate collector-thread findings). - Cross-thread lifecycle: the observer holds
Worker&,VM&, and a rawWebWorker*across the worker's lifetime; disarming, teardown ordering, and them_terminatedDueToOOM/m_heapLimitDisarmedatomics all needed careful sequencing. - Worker termination paths:
dispatchExit,WebWorker__teardownJSCVM, and the interaction withshutdown()'sclear_has_termination_request()beforeon_exit(). - node:vm's
checkForTerminationand its twoNodeVMModule.cppsiblings, which previously assumed only two termination sources.
The PR reached its current state through 13 iterations, during which multiple genuine 🔴 bugs were found and fixed (Watchdog slot conflict with node:vm, ErrorCode index misalignment, collector-thread unwrap_unchecked() UB). That iteration count itself argues for a human pass on the final design.
Other factors
- All 15+ inline review threads are resolved; the author addressed each with either a fix or a reasoned won't-fix (the two unreachable-today
on_exit()windows). - Test coverage is thorough and follows repo conventions (subprocess spawns,
tempDir, combined-object assertions withstderr: expect.any(String), concurrent pipe drain). - The PR modifies two existing test files' timeouts/skips (
jest.setTimeout(15_000)under debug,skipIf(isDebug)on the eval-source-leak test, environmentData fixture depth 5→2) — these are justified in the description as post-#31216 debug-build timing, but a human should confirm the fixture-depth reduction doesn't weaken that test's invariant. - One acknowledged pre-existing bug (nested no-timeout
vm.runInContextinside an outer{timeout}still hitsRELEASE_ASSERT) is deliberately left for a follow-up; it's orthogonal to resourceLimits and unchanged by this PR.
aa6159b to
4ffdb99
Compare
There was a problem hiding this comment.
No issues found in this pass, but this touches worker VM lifecycle, JSC GC internals, and cross-thread FFI deeply enough that a maintainer should sign off on the final design.
Checked this round: the HeapObserver collector-thread path uses only pointer-taking atomic FFI (no per-thread VM reads remain); disarmHeapLimitObserver is set before teardown's final collectNow on all shutdown paths; ERR_WORKER_OUT_OF_MEMORY is appended (not inserted) in ErrorCode.ts so Rust discriminants stay aligned; the createNodeWorkerThreadsBinding array index (11) matches on both C++ and JS sides after the #31216 rebase.
Extended reasoning...
Overview
This PR implements node:worker_threads resourceLimits: parsing in JSWorkerDOMConstructor::construct, a JSC::HeapObserver that terminates the worker with ERR_WORKER_OUT_OF_MEMORY when the post-Full-GC live size exceeds maxOldGenerationSizeMb + maxYoungGenerationSizeMb, a parent-side worker.resourceLimits getter and worker-side export both fed by the single native parse, and a fix for node:vm's checkForTermination to propagate worker-level TerminationException instead of RELEASE_ASSERTing. It spans 13 files (~630 lines): C++ bindings (Worker.cpp/.h, JSWorker.cpp, WorkerOptions.h, NodeVMScript.cpp, NodeVMModule.cpp), Rust FFI (web_worker.rs), built-in JS (worker_threads.ts), types, and tests.
Security risks
The heap-limit observer reads user-supplied doubles and casts to size_t — the clamp-before-cast in heapLimitBytes() handles the out-of-range case. resourceLimits parsing uses getIfPropertyExists with RETURN_IF_EXCEPTION after each read, and gates on isNumber() rather than coercing, so hostile getters/Proxies are observed once and cannot desync enforcement from reporting. No auth/crypto/permissions surface. The observer's cross-thread reads are all through explicit atomics or the pointer-taking FFI shims added for this purpose.
Level of scrutiny
High. This introduces a new consumer of JSC's HeapObserver interface that runs on the dedicated heap-collector thread, with a lifetime tied to the worker VM and a disarm/teardown ordering that took several iterations to get right (the PR history includes fixes for a JSC::Watchdog slot conflict, collector-thread unwrap_unchecked() UB via the per-thread VM, and a natural-drain teardown race). It also modifies node:vm's termination classification and dispatchExit's close task. These are exactly the areas CLAUDE.md flags as most-blocked (memory safety, thread affinity, GC interaction), and the design choices (HeapObserver over Watchdog, pointer-passing FFI over thread-local, release/acquire disarm flag) each carry non-obvious rationale that a maintainer should confirm.
Other factors
The PR went through 13 review iterations with every raised issue addressed and tested. Test coverage is thorough (OOM enforcement, OOM-inside-vm, terminate-inside-vm, single-read/fresh-object contract, non-number/non-object handling, post-exit {}). All prior inline threads are resolved. The remaining CI reds on the last pre-rebase build were analyzed as infrastructure/known-flake unrelated to this PR's files. Still, per the approval guidelines this is not a simple/mechanical change — it is a new subsystem interacting with JSC's concurrent GC and worker teardown, and warrants a human maintainer's review before merge.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
24d62c3 to
94a515a
Compare
…ationSizeMb
Parses options.resourceLimits once, natively, in the JSWorker constructor
the way Node's parseResourceLimits does: a non-object or callable value and
non-number fields are ignored, maxOldGenerationSizeMb is floored at 2, and
a stackSizeMb that is not positive reads back as the 4 MB default.
worker.resourceLimits (a prototype getter) and the in-worker module export
(index 12 of the createNodeWorkerThreadsBinding array) both read that one
parse; the getter returns {} once the thread has exited, as in Node.
Enforcement lives on the WorkerMessagingProxy, which owns the options and
outlives both the thread and the script object. When a heap limit is
configured, start_vm() registers a JSC::HeapObserver on the worker's heap;
after each full collection it compares sizeAfterLastFullCollection() with
maxOldGenerationSizeMb (plus maxYoungGenerationSizeMb as headroom, since
JSC keeps young objects in the same heap) and stops the thread through
WebWorker__requestTermination, the path worker.terminate() uses. The
observer runs on whichever thread finished the collection, possibly JSC's
collector thread, so it only touches what it captured at install time and
atomics. requestTermination now reports whether it initiated the stop, and
the thread disarms the observer when it starts shutting down, so neither a
terminate() in flight nor the exit-time collections get reported as out of
memory. The proxy's exit path then exits with code 1 and dispatches an
ERR_WORKER_OUT_OF_MEMORY error event before the close event, so 'error'
precedes 'exit' as in Node.
A young generation size on its own configures no limit; codeRangeSizeMb
and stackSizeMb are reported back only.
Test-side, every nested worker in a debug+ASAN build is a full JSC VM
boot, so the environmentData fixture nests two levels instead of three,
the 100 MiB eval source leak test is skipped in debug builds (it cannot be
made smaller without dropping below its RSS noise floor), and
worker.test.ts loses two fixed 1 s hang timers.
94a515a to
4147d7b
Compare
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Because it adds a cross-thread JSC::HeapObserver with raw refs to the proxy/VM/thread object, changes the WebWorker__requestTermination FFI contract, and wires new atomics into the worker exit path, a human look at the lifetime and ordering arguments would still be worthwhile.
Checked: the observer's captured WorkerMessagingProxy& / void* workerThread outlive the heap (proxy holds the thread ref until releaseWorkerThread joins; observer is disarmed at shutdown() entry before VM teardown). Checked heapLimitBytes() for NaN/Infinity/overflow — guarded by isfinite and the size_t-range clamp. Checked the readLimit lambda's exception propagation (each getIfPropertyExists is followed by a scope check). Checked the request_termination bool return: the one pre-existing caller in terminateWorkerGlobalScope ignores it, which is fine.
Extended reasoning...
Overview
This PR implements resourceLimits for node:worker_threads: option parsing in JSWorker.cpp, a WorkerResourceLimits struct on WorkerOptions, a resourceLimits prototype getter, the in-worker module export (index 12 of createNodeWorkerThreadsBinding), and enforcement of maxOldGenerationSizeMb via a new WorkerHeapLimitObserver : JSC::HeapObserver on WorkerMessagingProxy. The observer compares sizeAfterLastFullCollection() against the limit after each full GC and, when exceeded, calls WebWorker__requestTermination (whose signature changes from void → bool) and sets an atomic flag the parent-side exit path reads to dispatch ERR_WORKER_OUT_OF_MEMORY before the close event. web_worker.rs installs the observer after publishing vm and disarms it at the top of shutdown(). dispatchCloseEvent is renamed dispatchExitEvent since it now also carries the OOM error event. ~15 new tests cover reporting, the 2 MB floor, the stackSizeMb default, non-object/non-number handling, {} after exit, young-only not capping, and two runaway-worker enforcement cases (plain and inside vm.runInContext). Two unrelated test edits reduce a fixture's worker chain depth (5→2) and drop 1 s hand-rolled timeouts from worker.test.ts.
Security risks
None identified. The option is a resource cap on the caller's own worker; no auth, crypto, path handling, or untrusted-input parsing beyond reading four numbers off a user object. heapLimitBytes() guards NaN/±Infinity via std::isfinite and clamps the double→size_t cast.
Level of scrutiny
High. This is native cross-thread code: the heap observer runs on JSC's collector thread with no Bun per-thread state, holds raw references to the proxy, the worker's JSC::VM, and the WebWorker*, and calls a mutex-taking FFI function from that thread. The correctness of the OOM report depends on ordering between m_heapLimitDisarmed, m_stoppedByHeapLimit, request_termination's new bool return under vm_lock, and the parent-side exit task. The design choice (HeapObserver rather than JSC's Watchdog, young-gen as headroom only, never unregistering the observer because the proxy outlives the heap) is well-argued in the description but is exactly the kind of architectural decision a maintainer should sign off on.
Other factors
The PR has been through several review rounds (Watchdog-collision bug, missing 2 MB floor, dead getter, callable-object handling, comment-length) all of which are addressed and resolved in the current revision. Test coverage is thorough and includes fail-fast bounds so an unenforced limit fails the test rather than hanging. The request_termination return-type change has one other caller (terminateWorkerGlobalScope) which safely discards it. No outstanding reviewer comments. Given the scope — new C++ class, FFI signature change, three new atomics on a shared object, and a rename touching the exit-event dispatch path — this warrants a human reviewer rather than auto-approval.
|
One follow-up from reviewing the re-ported design: |
This branch has not been deployed
2 participants
Fixes #31411
Problem
new Worker(file, { resourceLimits: { maxOldGenerationSizeMb: 32 } })silently drops the option: the worker grows until the whole process runs out of memory,worker.resourceLimitsisundefined, andrequire("node:worker_threads").resourceLimitsinside the worker is{}. Node terminates the worker, emits an'error'whosecodeisERR_WORKER_OUT_OF_MEMORY, exits it with code 1, and reports the limits through both properties.WorkerOptionshad no field for it,src/js/node/worker_threads.tsexported a placeholder{}, and the worker VM had no heap limit of any kind.Fix
JSWorker.cpp) parses the option once intoWorkerOptions::resourceLimits, following node'sparseResourceLimits: a non-object or callable value and non-number fields are ignored,maxOldGenerationSizeMbis floored at 2, and astackSizeMbthat is not positive reads back as node's 4 MB default.worker.resourceLimits(a getter on the prototype), the in-worker export (index 12 of thecreateNodeWorkerThreadsBindingarray) and enforcement all read that one parse, so a getter- or Proxy-backed option is observed exactly once and the reported limits cannot differ from the enforced ones. The getter returns{}once the thread has exited, as node does, which includes the out-of-memory'error'handler.WorkerMessagingProxy.cpp): whenmaxOldGenerationSizeMbis set,start_vm()(web_worker.rs) registers aJSC::HeapObserveron the worker's heap. After each full collection it comparessizeAfterLastFullCollection()with the limit (plusmaxYoungGenerationSizeMb, if given, as headroom, since JSC keeps young objects in the same heap) and, if exceeded, stops the thread throughWebWorker__requestTermination, the same pathworker.terminate()uses. That is why the exit looks exactly like node's: exit handlers do not run, the exit code is 1, and the proxy's existing exit task dispatches theERR_WORKER_OUT_OF_MEMORYerror event before the close event, so'error'precedes'exit'.node:vm'stimeoutoption owns the VM's single watchdog.WebWorker__requestTerminationnow returns whether it initiated the stop, so a collection that runs while the worker is already being terminated or exiting does not relabel that exit as out-of-memory; and the thread disarms the observer when it begins shutting down, so the collections run by exit handlers and teardown cannot either. The observer's flag is set before JSC resumes the mutator (Heap::runEndPhasenotifies observers first), so the exit task on the parent always sees it.codeRangeSizeMbandstackSizeMbare reported back but not enforced; an unset field reads back as-1(node fills in V8's computed defaults there, which JSC has no equivalent of).test/js/node/worker_threads/worker_threads.test.ts(describe("resourceLimits")): reporting of all four fields on both sides, the floor, the stack-size default, defaults when omitted, option read once and a fresh object per read, non-number and non-object values ignored,{}after a natural exit and afterterminate(),{}on the main thread, a young-only limit not terminating a worker that retains ~50 MB, a runaway worker terminated with the exact node error code, message, exit code and{}inside the handler, the same from inside a no-timeoutvm.runInContext, andworker.terminate()during a no-timeoutvm.runInContext. All of the reporting and enforcement cases fail on main (worker.resourceLimitsisundefined, the runaway workers are never stopped).bun bd test test/js/node/worker_threads/worker_threads.test.ts test/js/web/workers/worker.test.tsruns all 15 resourceLimits tests green and 167 of 174 tests overall; the remaining failures were 5 s timeouts in the Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075terminate() races and lifecycle edgesblock ofworker.test.ts, which this change does not touch (the set of timing-out tests changed from run to run on a machine with a load average above 100 and the same file passes on main's own CI).Background
Workeris three objects: the script-visibleWebCore::Workeron the parent thread, theWorkerMessagingProxyshared between the parent and the worker thread (it ownsWorkerOptionsand outlives both the thread and the script object, which is why the observer and the out-of-memory flag live there), and the native thread object insrc/jsc/web_worker.rs, which owns the thread's VM and is the only thing that can stop it (request_termination: raise a JSC termination exception at the next safepoint and wake the loop, from any thread).JSC::HeapObserver::didGarbageCollectis called at the end of every collection on whichever thread finished it, which for concurrent full collections is JSC's collector thread; that thread has none of bun's per-thread state, so the observer only uses what it captured when it was installed (the proxy, the VM and the thread object) and an any-thread entry point.sizeAfterLastFullCollection()is the heap size JSC computes for its own allocation limits at the end of a full collection, immediately before the observers are notified; it is the live set, whereas the size between collections includes garbage.WorkerMessagingProxy::workerGlobalScopeDestroyedInternalafter the thread has torn down its VM; it already sets the proxy toClosing(which is what makesthreadIdread-1and, now,resourceLimitsread{}) before dispatching the close event thatworker_threads.tsturns into'exit'.worker_threads.tsemits an error event'serrorobject unchanged when the event has no message, which is how thecodeproperty reaches the'error'listener.node:vmchecks the thread'sVmHandlewhen a script is interrupted, andrequest_terminationstops that handle; that is why a worker stopped for exceeding its limit insidevm.runInContextexits cleanly without thenode:vmchanges an earlier revision of this PR carried.Branch history
For about two hours on 2026-08-13 the branch head was a merge commit (24d62c3) whose parent order made GitHub diff the PR against a stale merge base and show ~3000 changed files; the comment-cop threads on files outside this change came from that. The branch is now a single commit on top of main with the same content (the only difference from the merge head is shorter comments), and those threads are resolved.