fix(gc): Promise.all at scale read globalThis and its own combinator state from retired from-space (#7497) - #7516
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe runtime now roots and refreshes JavaScript values across moving-GC-sensitive promise, async, microtask, global-object, and array operations. A regression test covers large ChangesPromise GC rooting
Optimization gate update
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-runtime/src/object/class_registry/class_meta.rs (1)
320-335: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoot
func_valuetoo; the comparison still uses a pre-collection closure address.The loop allocates a key string on every iteration.
globalThisis now re-read across those allocations, butjvwas computed at line 139 and is never refreshed. If a copying minor evacuates the ClosureHeader during any key allocation, theglobalThisfield slot is rewritten to the new address whilejv.bits()still holds the from-space address. The equality test at line 332 then fails and the function returnsNone, which drops callers into the generic construct tail.Root the searched value in the same scope and compare against the re-read bits.
🛠️ Proposed fix
let scope = crate::gc::RuntimeHandleScope::new(); let global_handle = scope.root_nanbox_f64(js_get_global_this()); + let func_handle = scope.root_nanbox_f64(func_value); for name in GLOBAL_THIS_BUILTIN_CONSTRUCTORS.iter().copied() { let (key, global_this_f64) = global_handle.across_nanbox(|| { crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32) }); let global_obj = crate::value::js_nanbox_get_pointer(global_this_f64) as *const ObjectHeader; if global_obj.is_null() { return None; } let v = js_object_get_field_by_name(global_obj, key); - if v.bits() == jv.bits() { + if v.bits() == func_handle.get_nanbox_f64().to_bits() { return Some(name); } }🤖 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 `@crates/perry-runtime/src/object/class_registry/class_meta.rs` around lines 320 - 335, Refresh the searched globalThis value after each key-string allocation in the constructor lookup loop, using the rooted handle in the same RuntimeHandleScope rather than the stale pre-collection jv. Compare js_object_get_field_by_name against the re-read rooted value’s bits so a copying minor preserves the func_value match.crates/perry-runtime/src/promise/then.rs (1)
1397-1407: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe slow path hands its values to
perform_promise_then_with_cap, which still publishes pre-collection addresses.At lines 1212-1234 that helper allocates
ful_wrap, fills its captures, and then allocatesrej_wrap. Across that secondjs_closure_allocit holdspromiseandful_wrapin bare Rust locals, and it writeson_rejected,cap_resolve, andcap_rejectintorej_wrapafterwards. A copying minor at that allocation stores from-space addresses into the reject wrapper and passes a stale receiver tojs_promise_then. This is the same publishing shape fixed inmake_resolving_functionsandbuild_element_closure.🛠️ Proposed fix for `perform_promise_then_with_cap` (lines 1212-1234, outside this range)
let scope = crate::gc::RuntimeHandleScope::new(); let promise_h = scope.root_raw_mut_ptr(promise); let on_fulfilled_h = scope.root_nanbox_f64(on_fulfilled); let on_rejected_h = scope.root_nanbox_f64(on_rejected); let cap_resolve_h = scope.root_nanbox_f64(cap_resolve); let cap_reject_h = scope.root_nanbox_f64(cap_reject); let cap_promise_h = scope.root_nanbox_f64(cap_promise); let ful_wrap_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer( js_closure_alloc(then_cap_fulfill_fn as *const u8, 3) as i64, )); let rej_wrap_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer( js_closure_alloc(then_cap_reject_fn as *const u8, 3) as i64, )); // Both closures exist; every capture below is written at a post-collection address.🤖 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 `@crates/perry-runtime/src/promise/then.rs` around lines 1397 - 1407, Update perform_promise_then_with_cap to root promise, callback values, capability values, and both closure wrappers through RuntimeHandleScope before allocating the second closure. Allocate and root ful_wrap and rej_wrap before populating captures, then write all captures using their post-collection addresses and pass the rooted promise to js_promise_then.
🧹 Nitpick comments (1)
crates/perry-runtime/src/promise/spec_combinators.rs (1)
614-699: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold the three element-closure branches into one helper.
All,AllSettled, andAnyrepeat the same sequence: root a guard, build one or two element closures fromvalues_ptr()/state_ptr()/ the two capability handles, then bump the remaining count. A small helper that takes the elementfuncpointer and returns the rooted closure value would remove the duplication and keep the re-read order in one place. Behavior stays identical, so defer this if you prefer the explicit form.🤖 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 `@crates/perry-runtime/src/promise/spec_combinators.rs` around lines 614 - 699, Optionally refactor the repeated closure setup in the combinator loop into a shared helper used by CombinatorKind::All, CombinatorKind::AllSettled, and CombinatorKind::Any. Have it root the guard, build the requested element closure from values_ptr(), state_ptr(), and the capability handles, and preserve the existing state-count increment and callback behavior.
🤖 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 `@crates/perry-runtime/src/array/flat_clone.rs`:
- Around line 410-425: Update js_array_values to root arr with
RuntimeHandleScope before calling js_array_alloc, then re-read both arr and the
allocated result from their rooted handles before copying. Preserve the existing
length and value-copy behavior while eliminating use of the pre-allocation arr
address.
In `@crates/perry-runtime/src/promise/combinators.rs`:
- Around line 359-375: Root the incoming value at the start of the function and
use the rooted handle as the source for raw. In the GC_TYPE_OBJECT path,
recompute raw after the user [Symbol.iterator] lookup and again after allocating
the "next" key, before calling js_object_get_field_by_name or js_array_clone.
Apply the same allocation-safe rooted-value pattern already used in the array
branch.
---
Outside diff comments:
In `@crates/perry-runtime/src/object/class_registry/class_meta.rs`:
- Around line 320-335: Refresh the searched globalThis value after each
key-string allocation in the constructor lookup loop, using the rooted handle in
the same RuntimeHandleScope rather than the stale pre-collection jv. Compare
js_object_get_field_by_name against the re-read rooted value’s bits so a copying
minor preserves the func_value match.
In `@crates/perry-runtime/src/promise/then.rs`:
- Around line 1397-1407: Update perform_promise_then_with_cap to root promise,
callback values, capability values, and both closure wrappers through
RuntimeHandleScope before allocating the second closure. Allocate and root
ful_wrap and rej_wrap before populating captures, then write all captures using
their post-collection addresses and pass the rooted promise to js_promise_then.
---
Nitpick comments:
In `@crates/perry-runtime/src/promise/spec_combinators.rs`:
- Around line 614-699: Optionally refactor the repeated closure setup in the
combinator loop into a shared helper used by CombinatorKind::All,
CombinatorKind::AllSettled, and CombinatorKind::Any. Have it root the guard,
build the requested element closure from values_ptr(), state_ptr(), and the
capability handles, and preserve the existing state-count increment and callback
behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0577634c-621f-4f6c-b13a-45cea8d69b6f
📒 Files selected for processing (13)
.github/workflows/auto-opt-app-patterns.ymlchangelog.d/7516-promise-all-chains.mdcrates/perry-runtime/src/array/flat_clone.rscrates/perry-runtime/src/object/class_registry/class_meta.rscrates/perry-runtime/src/object/global_this/math_temporal.rscrates/perry-runtime/src/object/native_module_registry.rscrates/perry-runtime/src/object/object_ops/prototype.rscrates/perry-runtime/src/promise/combinators.rscrates/perry-runtime/src/promise/spec_combinators.rscrates/perry-runtime/src/promise/then.rsscripts/auto_opt_app_patterns.shtest-files/test_gap_gc_global_builtin_lookup_rooting.tstest-parity/gc_repsel_corpus.txt
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/promise/microtasks.rs (1)
435-444: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
v8::promise_hook_beforereceives a pointer captured beforeasync_hooks::beforeallocates. Both microtask arms reload the callback and the value across the two hook calls but leave the promise argument ofcrate::v8::promise_hook_beforereading the local captured beforecrate::async_hooks::before. The rooting handle is already live at both sites, so each fix is a one-line reload.
crates/perry-runtime/src/promise/microtasks.rs#L435-L444: passpromise_handle.get_raw_mut_ptr::<Promise>()tocrate::v8::promise_hook_beforeat line 438 instead of thepromiselocal.crates/perry-runtime/src/promise/microtasks.rs#L840-L847: reloadnextfromnext_handlebefore line 841 and pass the reloaded pointer tocrate::v8::promise_hook_before.🤖 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 `@crates/perry-runtime/src/promise/microtasks.rs` around lines 435 - 444, The promise pointer passed to promise_hook_before becomes stale after async_hooks::before allocates. In crates/perry-runtime/src/promise/microtasks.rs lines 435-444, pass a fresh pointer from promise_handle directly; in lines 840-847, reload next from next_handle immediately before promise_hook_before and pass that reloaded pointer.
🧹 Nitpick comments (2)
crates/perry-runtime/src/promise/async_step.rs (2)
338-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the nanbox encode/decode helpers instead of re-implementing them.
Lines 339-351 duplicate
boxed_closureandrooted_closurefromcrates/perry-runtime/src/promise/microtasks.rslines 144-171. Lines 461-475 duplicateboxed_promiseandrooted_promisefrom the same file, lines 136-161.Four copies of the same null/
TAG_UNDEFINEDencoding contract now exist across two files. If one copy changes its null sentinel, the mismatch is silent and produces a wrong pointer.Promote the four helpers to
crate::promise(for example inmod.rs, next to theClosurePtralias) and call them from both files.🤖 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 `@crates/perry-runtime/src/promise/async_step.rs` around lines 338 - 351, Promote the shared nanbox closure and promise encode/decode helpers to crate::promise, near the ClosurePtr alias, and replace the local implementations in async_step.rs and microtasks.rs with calls to those helpers. Preserve the existing null-to-TAG_UNDEFINED contract and pointer decoding behavior while removing all four duplicated helper pairs.
557-562: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead the target from
target_hat the call site.Line 561 passes the raw
trap.trap_nextcopy as the target, while the same line readsvaluefromvalue_hand line 562 reads the return value fromtarget_h. No allocation happens between line 559 and line 561, so the current behavior is correct.The mixed style is fragile. Any call inserted between the root and the use reintroduces the stale-pointer defect without a visible signal.
♻️ Proposed change
- resolve_trap_next_with_adoption(trap.trap_next, value_h.get_nanbox_f64()); + resolve_trap_next_with_adoption( + crate::value::js_nanbox_get_pointer(target_h.get_nanbox_f64()) as *mut Promise, + value_h.get_nanbox_f64(), + );🤖 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 `@crates/perry-runtime/src/promise/async_step.rs` around lines 557 - 562, Update the call to resolve_trap_next_with_adoption in the surrounding async-step logic to pass the rooted target value read from target_h rather than the raw trap.trap_next pointer. Keep the existing value_h argument and target_h-based return conversion unchanged.
🤖 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 `@crates/perry-runtime/src/promise/async_step.rs`:
- Around line 439-455: Apply the existing step_scope rooting and re-read pattern
to all early-return paths around build_async_step_thunks and
js_promise_resolved: root inner, fulfill, reject, and trap_next before
allocation, then re-read each rooted value before passing it to
then_backpatch_result. Cover both resolved-value branches and the pointer-tagged
thenable path, preserving their current control flow and return behavior.
In `@crates/perry-runtime/src/promise/microtasks.rs`:
- Around line 668-674: Update the step_closure handle initialization in the
affected microtask arm to use boxed_closure instead of directly calling
js_nanbox_pointer, matching the encoding expected by rooted_closure and the
sibling arms. Preserve the existing null step_closure behavior while ensuring
null is encoded as TAG_UNDEFINED rather than a nanboxed null pointer.
---
Outside diff comments:
In `@crates/perry-runtime/src/promise/microtasks.rs`:
- Around line 435-444: The promise pointer passed to promise_hook_before becomes
stale after async_hooks::before allocates. In
crates/perry-runtime/src/promise/microtasks.rs lines 435-444, pass a fresh
pointer from promise_handle directly; in lines 840-847, reload next from
next_handle immediately before promise_hook_before and pass that reloaded
pointer.
---
Nitpick comments:
In `@crates/perry-runtime/src/promise/async_step.rs`:
- Around line 338-351: Promote the shared nanbox closure and promise
encode/decode helpers to crate::promise, near the ClosurePtr alias, and replace
the local implementations in async_step.rs and microtasks.rs with calls to those
helpers. Preserve the existing null-to-TAG_UNDEFINED contract and pointer
decoding behavior while removing all four duplicated helper pairs.
- Around line 557-562: Update the call to resolve_trap_next_with_adoption in the
surrounding async-step logic to pass the rooted target value read from target_h
rather than the raw trap.trap_next pointer. Keep the existing value_h argument
and target_h-based return conversion unchanged.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62a8bec9-57d7-4ce9-8a42-95b8b825b6c6
📒 Files selected for processing (3)
changelog.d/7516-promise-all-chains.mdcrates/perry-runtime/src/promise/async_step.rscrates/perry-runtime/src/promise/microtasks.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- changelog.d/7516-promise-all-chains.md
|
@coderabbitai all six findings were valid and are fixed in a9e659f. They are
Re-verified after the fixes: One thing deliberately left open and called out in the PR body: a protected run |
|
Tip For best results, initiate chat on the files or code changes.
The six findings are addressed in The remaining 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
) js_get_global_this_builtin_value read the globalThis object out of its (correct, rewritten) root into a raw *const ObjectHeader and only then allocated the lookup key. The root is fine; the ORDER is not. The lookup interns nothing, so every call mints a fresh string, and any of those allocations can be the copying minor that evacuates globalThis -- after which js_object_get_field_by_name dereferences retired from-space. Promise.all is the shape that finds it: every element runs Promise.resolve, and js_promise_resolve_spec asks is_default_promise_constructor for globalThis.Promise through that helper, so one wide combinator call performs tens of thousands of these lookups and one straddles the collection. Three more callers of js_get_global_this() had the identical shape and are fixed the same way (audit, not reproduced): the builtin-constructor name walk in class_meta.rs (worse -- ~50 key allocations per call against one pre-loop address), js_globalthis_seed_async_local_storage, and the Temporal.<Type>.prototype walk.
…user-JS calls (#7497) With the globalThis lookup fixed, PERRY_GC_PROTECT_FROMSPACE moved the fault one frame out to run_combinator itself. perform() ran the per-element loop -- Call(promiseResolve, C, next) and Invoke(nextPromise, "then", ...), both of which run user JS -- while holding the elements snapshot, the shared values and remaining-count arrays, the capability's resolve/reject and the constructor in bare Rust locals. build_element_closure read all five of its GC arguments BEFORE js_closure_alloc and stored them after, publishing from-space addresses into capture slots. new_promise_capability, the allSettled element functions and build_settled_* had the same shape. Everything is rooted in a RuntimeHandleScope and re-read at its point of use. The per-element handles live in a scope inside the loop so a 50,000-element combinator does not push 50,000 handle-stack entries. Handles are NaN-boxed, so scripts/raw_handle_debt.py is unchanged at 999. Also empties the auto-opt gate's skip list. Every array expansion is guarded on ${#...[@]} because macOS ships bash 3.2, where set -u turns "${EMPTY[@]}" into an unbound-variable abort -- an empty skip list must leave the gate running.
… array fast path (#7497) make_resolving_functions held the promise across four allocations and then STORED it (and the shared already-resolved guard) into two closures' capture slots -- the from-space-publishing shape. combinator_iterable_to_array's array fast path carried the array being cloned across well_known_symbol and own_symbol_property, the latter of which can run a user getter. Both are on the Promise.all path the #7497 reproducer exercises.
…e globalThis.Promise probe (#7497) Every spec-entry that asks is_default_promise_constructor whether `this` is the intrinsic Promise pays a globalThis lookup that allocates a fresh key string, and each held its own arguments in registers across it. js_promise_resolve_spec is the one the instrument names: Promise.all calls it once per element, and for Promise.all([...promises]) the element IS a promise, so js_promise_resolved dereferenced a from-space GC_TYPE_PROMISE at minor #0. js_promise_reject_spec, js_promise_try_spec, js_promise_with_resolvers_spec and promise_prototype_then_thunk have the same shape and are rooted the same way.
…location (#7497) js_array_clone read the source array pointer, called js_array_alloc(len) -- which can trigger the copying minor and MOVE the source -- and then memcpy'd from the pre-collection address. That is every [...arr] / Array.from(arr) and every promise combinator's iterable snapshot: the clone could copy whatever the recycled from-space bytes now hold. PERRY_GC_PROTECT_FROMSPACE=1 faults inside js_array_clone on the Promise.all snapshot at minor #0.
…ok calls (#7497) Every dispatch arm of run_microtasks read its callback pointer (and the value it passes) out of the popped Task into a bare local, then called async_hooks::before and v8::promise_hook_before -- both of which allocate -- and only then loaded func_ptr out of that pointer. The CURRENT_MICROTASK_CALLBACK cell IS a scanned root, so evacuation rewrote the CELL and left the register copy naming from-space. Disassembly of the faulting site: promise_hook_before, then ldr x0,[sp,#0x70]; ldr x8,[x0] -- the closure header load. #1663 had already rooted promise and next in this arm; the callback was missed. The Inline, Microtask and AsyncStep arms have the same shape.
…#7497) The first attempt rooted the callback inside each arm, after enter_microtask_context had already run. A Task stops being a scanned root the instant it is popped off TASK_QUEUE, so that seeded the handle with an address the collection had already invalidated -- the instrument still faulted at call_async_step_direct's (*step_closure).func_ptr, on a value re-read from a handle. Disassembly showed the re-read was there and still wrong, which is what made the ordering the suspect. Every arm now roots its task's GC values as its first statement and re-reads them after the context switch.
…s its allocations (#7497) js_async_step_chain carried the step closure through adapt_foreign_promise_value, js_promise_new, build_async_step_thunks, js_promise_resolved and capture_context -- all allocating -- and then STORED it into a Task::AsyncStep. The task queue is a scanned root and the microtask runner now re-reads everything it pops, but neither helps when the pointer was already dead at the push: the runner faithfully dispatched it. That is why the fault kept reappearing at call_async_step_direct even after the consumer side was rooted.
…t performs (#7497) The reuse fast path settled trap_next and then RETURNED the pre-call copy of that pointer -- and settling enqueues jobs and allocates. The async state machine therefore received a from-space GC_TYPE_PROMISE as its own result. The adoption slow path had the same shape around js_promise_new / enqueue_native_adoption_job. Found by the protected run of the auto-optimize binary, which faulted at js_async_step_done AFTER printing the right answer.
All six were valid and are the shapes this PR is about: * class_meta.rs: the SEARCHED closure value was never refreshed, so a key allocation that evacuates it makes the equality test miss and the caller falls through to the generic construct tail. * then.rs perform_promise_then_with_cap: filled ful_wrap's captures and THEN allocated rej_wrap -- the publishing shape. Both wrappers are now allocated and rooted before any capture is written. * flat_clone.rs js_array_values: the same memcpy-from-a-moved-source shape js_array_clone had. * combinators.rs: the GC_TYPE_OBJECT arm of combinator_iterable_to_array carried its receiver across a user [Symbol.iterator] getter and a key allocation. * async_step.rs: the three early-return suspend paths carried the awaited promise, both fresh thunks and trap_next across build_async_step_thunks and js_promise_resolved; then_backpatch_result then STORES into those thunks. Factored into one rooted suspend_on_awaited helper. * microtasks.rs: the AsyncStep arm boxed its closure with js_nanbox_pointer but decoded with rooted_closure, which expects boxed_closure's null-as-undefined encoding.
e50a589 to
9768cf0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Fixes #7497.
Fixed
Promise.allat scale rejected with a resolution value, or withTypeError: value is not a function— eight stale from-space reads, none of themin the promise machinery's logic (#7497).
The app-pattern kernel
promise_all_chainsprintedUncaught (in promise) 0under
PERRY_NO_AUTO_OPTIMIZE=1and a rooting-shapedTypeErrorunder thedefault link, and was the last blocker on the public benchmark artifact. No
settle/reject/microtask decision changed. Every defect is the #7341 family —
a value read out of a root and held in a register across a call that allocates
is not rooted — and the fix is the same shape each time:
RuntimeHandleScopeplus a re-read at the point of use, so the pre-collection address is never
nameable.
They were found one at a time. Each
PERRY_GC_PROTECT_FROMSPACE=1fault named asite; fixing it moved the fault and exposed the next.
js_get_global_this_builtin_value— the canonicalglobalThis.<Builtin>read behind
instance.constructor, bareDate/Array/Objectidentifierresolution, and
is_default_promise_constructor:The root is fine —
THREAD_GLOBAL_THISis registered and evacuation rewritesit. The ORDER is not: this lookup interns nothing, so every call mints a fresh
string, and any of those allocations can be the copying minor that moves
globalThis.js_promise_resolve_spec—Promise.allcalls it once per element, andit asks
is_default_promise_constructor(i.e. 1) before touching its ownargument. For
Promise.all([...promises])that argument IS a promise, sojs_promise_resolveddereferenced a from-spaceGC_TYPE_PROMISE.js_promise_reject_spec,js_promise_try_spec,js_promise_with_resolvers_specandpromise_prototype_then_thunkshare theshape.
js_array_clone— the widest. It read the source array pointer, calledjs_array_alloc(len)for the destination (which can collect and MOVE thesource), then
copy_nonoverlapping'd from the pre-collection address. That isevery
[...arr], everyArray.from(arr)and every combinator's iterablesnapshot.
The spec combinators' own locals.
performran its per-element loop —Call(promiseResolve, C, «next»)andInvoke(nextPromise, "then", …), bothof which run USER JS — while holding the
elementssnapshot, the sharedvaluesand remaining-count arrays, the capability'sresolve/rejectandthe constructor in bare Rust locals.
Two publishers, worse than a stale read.
build_element_closureandmake_resolving_functionstake their GC arguments in registers, allocate, andthen store the pre-collection addresses into capture slots — putting
from-space into an object the collector goes on maintaining.
new_promise_capability, the twoPromise.allSettledelement functions,build_settled_{fulfilled,rejected}andcombinator_iterable_to_array'sarray fast path had shape (4) or (5).
The microtask runner's dispatch arms. Every arm read its callback pointer
(and the value it passes) out of the popped
Taskinto a bare local, ranasync_hooks::before/v8::promise_hook_before— both allocate — and onlythen loaded
func_ptrout of that pointer. SIGSEGV handling POST requests in compiled Fastify + @perryts/mysql service (0.5.1026 / 46b80d78); GET unaffected #1663 had already rootedpromiseand
nexthere; the callback was missed.…and rooting them inside the arm was still too late. A
Taskstops beinga scanned root the instant it is popped, and
enter_microtask_contextrunsbefore any of the arm's own bookkeeping. The first attempt seeded the handle
with an address the collection had already invalidated. Every arm now roots as
its first statement and re-reads after the context switch. Disassembling the
faulting instruction —
bl get_nanbox_u64; ldr x8,[x21]— is what showed there-read was present and still wrong, which is what made the ordering the
suspect rather than the rooting.
The producer side.
js_async_step_chaincarried the step closure throughadapt_foreign_promise_value,js_promise_new,build_async_step_thunks,js_promise_resolvedandcapture_contextand then STORED it into aTask::AsyncStep. The queue is a scanned root and the runner now re-readswhat it pops — neither helps when the pointer was already dead at the push.
js_async_step_donehad the mirror image: it settledtrap_next(whichallocates) and returned the pre-call copy as the async function's own result
promise.
All handles are NaN-boxed rather than
root_raw_*_ptr, soscripts/raw_handle_debt.pyis unchanged at 999. The per-element handles inperformlive in a scope INSIDE the loop, so a 50 000-element combinator does notpush 50 000 entries onto the handle stack.
Three more callers of
js_get_global_this()had (1)'s shape and are fixedthe same way. These come from auditing the callers, not from a reproducer, and
are called out as such:
class_meta.rs's builtin-constructor name walk (worsethan the proven site — a fresh key allocation inside a ~50-iteration loop against
one address read before the loop),
js_globalthis_seed_async_local_storage(
globalThisis the RECEIVER of a store that follows two allocations), and theTemporal.<Type>.prototypewalk. The four sites of this shape inerror.rs/with_env.rswere already fixed by #6943; these are the ones that sweep missed.Why it read as "a separate promise-rejection defect". A stale read returns
whatever from-space happens to hold, so
globalThis.Promisecame back as anon-callable — or, when the garbage was zero, as the resolution value
0arriving on the rejection path. The two link modes printed different messages for
the same defect. It was untouched by #7495 only because #7495 fixed a different
function.
Localisation, for the next person (each knob against the unfixed binary):
PERRY_GEN_GC=0andPERRY_WRITE_BARRIERS=0both make it pass whilePERRY_GC_MOVING_SAFEPOINT=0does not — the first two make the copying minorineligible, the third only disables the safepoint collection, and the one that
matters is the alloc-point direct minor (
trigger=ArenaBytes declared_safepoint=false).PERRY_GC_FROMSPACE_SCAN=1reportedcleaneverytime: no HEAP slot was ever stale, which is the signature of a holder in a native
frame rather than in a table.
What is still open, stated rather than papered over. A protected run of the
auto-optimize binary is not silent: it prints the correct checksum and then
faults inside
js_async_step_doneon another 72-byteGC_TYPE_PROMISE. That isa ninth site of the same family, after the program's observable output, and the
kernel matches the oracle byte for byte with and without the instrument. The
PERRY_NO_AUTO_OPTIMIZE=1binary and the new gap test are both silent under theinstrument. Separately,
test_gap_gc_iterator_drain_rootingandtest_gap_iterator_helpers_2874fail onorigin/maintoo — verified by buildingorigin/main's runtime from a cleangit archiveexport and running bothagainst it — and belong to #7498's
array_from_spread_valueprototype walk.Added
test-files/test_gap_gc_global_builtin_lookup_rooting.ts, registered intest-parity/gc_repsel_corpus.txtsogc-moving-witnessesruns it. One widePromise.all(50 000 elements) rather than the kernel's 1000 × 50: the singlewide call packs enough lookups between two collections to fail on the shipped
default in a fraction of a second, where the kernel needs ~20× the work for the
same window. Deterministic — 6/6 runs failing before, 5/5 passing after — and
byte-diffed against node 26.5.1.
Verified with the instrument asserted live rather than merely quiet: a protected
run prints
[gc-fromspace-protect] retired_set=#0and[gc-copy-minor] ran copied_objects=175416, so objects really did move, and nofault follows.
Changed
scripts/auto_opt_app_patterns.shno longer skipspromise_all_chains; the skiplist is empty and the gate is 12/12. Its rot check means the line had to come out
with the fix. Every array expansion is now guarded on
${#…[@]}: macOS shipsbash 3.2, where
set -uturns"${EMPTY[@]}"into an "unbound variable" abort,so an empty skip list would have stopped the gate before its first kernel —
CLAUDE.md hazard 4 wearing a different hat.
--self-teststill passes.Evidence
Release, node 26.5.1 oracle. The "before" column is
origin/main's runtime builtfrom a clean
git archiveexport into its own target dir — not this branch withthe patch reverted.
PERRY_NO_AUTO_OPTIMIZE=1Uncaught (in promise) 0checksum: 2500050000TypeErrorchecksum: 2500050000scripts/auto_opt_app_patterns.shTypeError: value is not a function(6/6)Promise.allover 50 000 promisesTypeErrorcargo test -p perry-runtimePERRY_GC_PROTECT_FROMSPACE=1 DEPTH=300retired_set=#0andcopied_objects=175416proving the instrument was liveThe first fault, before any of this landed:
Summary by CodeRabbit
Bug Fixes
Promise.all,Promise.allSettled,Promise.resolve,Promise.reject,Promise.withResolvers, andPromise.tryduring garbage collection.AsyncLocalStorageinitialization in memory-intensive scenarios.Tests