fix(gc): root the ten unrooted runtime-side caches, and the scanner that walked 1 of 3 sibling slots (#7231) - #7239
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe runtime now scans and rewrites cached heap values, callback pointers, and transient object state during moving garbage collection. Frame and worker callback handling was updated, and a ChangesMoving GC root coverage
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 |
…hat walked 1 of 3 sibling slots The #7231 enumeration, verified and closed. This is #7226's class -- a runtime table holding a GC pointer that is not a registered root -- which is strictly worse than #7154's stale-register class (it goes bad at collection #0 and stays bad, rather than needing a collection to land in a narrow window) and which no static instrument can find: `gc_root_dominance_check.py` reads emitted LLVM IR, and a runtime table is not in it. ★ `CACHED_ENV`, the `process.env` object, is the load-bearing one and it is a hard crash rather than a subtle wrong answer. `js_process_env_impl` builds it once with `js_object_alloc` -- the NURSERY -- and caches it in a thread-local `Cell<f64>` that is the ENTIRE reference graph: `process.env` is a `js_process_env()` call, not a field of the `process` object. So the first minor swept or evacuated it and every later `process.env.X = v` wrote through a dangling pointer. The sibling `PROCESS_FINALIZATION_OBJECT` uses the identical materialize-once idiom and was already rooted, which is what makes this an omission rather than a design. The observable is ENUMERATION -- `Object.keys(process.env)`, `for…in`, spread, which is how `@next/env` and `dotenv` consume it. A direct `process.env.KEY` read lowers to `js_getenv` and asks the OS, so a witness built on the read would be a gate that cannot fail. Also rooted: `CACHED_PERMISSION` and `CACHED_REPORT` (same shape; the `runtime_write_barrier_root_nanbox` beside the first is an incremental MARK barrier, not a root registration, and is now labelled as such); `ERROR_CONSTRUCTOR_PTR` (a raw duplicate of a `globalThis` closure, outside the object graph, stale after a move); `INPUT_HANDLER` (the inline `useInput` arrow, which nothing else refers to); `RESIZE_CALLBACK` (a native slot that bypasses the rooted EventEmitter listener array); `FRAME_CALLBACKS` (rooted only transiently during registration -- its `unsafe impl Send` SAFETY comment asserted the opposite and is corrected in place); `CURRENT_NEW_TARGET`; `ACCESSOR_RECEIVER_OVERRIDE`; and `PENDING_FETCH_SIGNAL`. Scanner gap, the shape #7230 found twice: `worker_threads.rs`'s `scan_parent_port_event_roots_mut` visited `MESSAGE_EVENT_CALLBACKS` and neither `MESSAGE_CALLBACK` nor `CLOSE_CALLBACK` -- three slots in the same `thread_local!` block holding the same raw `ClosureHeader*`. The box/visit/ unbox dance is factored into one helper, because three copies of it is how the fourth gets forgotten. Two further windows closed in `frame.rs` while rooting its queue. `js_frame_tick` drained into an unrooted local `Vec` and rooted each callback only as it invoked it, leaving #2..#N naked while #1 ran arbitrary user code (#7230's staging-buffer shape); one batch `RuntimeHandleScope` now covers the set. And `js_on_frame_callback` held the queue mutex across an allocating `capture_context()` -- harmless before, a self-deadlock once a scanner locks the same mutex. Verified against c9cd73b with `test_gap_gc_process_env_cache_rooting.ts`, registered in `test-parity/gc_repsel_corpus.txt`. Compiled AND run under `PERRY_GC_MOVING_LOOP_POLLS=1` with the evacuating base: SIGBUS (exit 138) 10/10 at base with no output at all, `bad 0` 10/10 after, byte-exact vs node 26.5.1. Clean 5/5 on the shipped default both sides, so this is a `requires=move` witness rather than a pre-existing failure. Under `ZEAL=1 PROTECT_FROMSPACE=1 DEPTH=800` the base arm faults at the stale dereference (`obj_type=2 size=416`, RETIRED FROM-SPACE) and this build is clean, so the instrument is a detector here and not a noise generator. `scripts/gc_repsel_matrix.sh --arms loop_polls --filter test_gap_gc_`: PASS=21 UNVER=0 FAIL=0, copy-minor live 21/21. Refs #7231, #7226, #7230, #7210, #7154, #7196.
cc9acf0 to
673751c
Compare
Gate results
The new cell
Both names appear verbatim in #7196's list, and both pass when run alone against the same test binary. Not asserted against a from-scratch base test binary — see the unmeasured list below.
|
![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)
crates/perry-runtime/src/frame.rs (1)
142-179: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKey
LAST_FIRE_BY_CLOSUREon the rewritten closure pointer.
batch_handle.get_raw_const_ptr::<ClosureHeader>()is the current root pointer after GC relocation;cb.callbackis a stale copy from the drainedpendingVec. If an earlier callback in the loop relocates a closure, using that stale key drops the previous-fire timestamp or reuses a stale address. Store the refreshed address and use it forjs_closure_call2andLAST_FIRE_BY_CLOSURE.🤖 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/frame.rs` around lines 142 - 179, Within the callback loop, derive the refreshed ClosureHeader pointer from batch_handle and use it consistently for both js_closure_call2 and the LAST_FIRE_BY_CLOSURE map key. Replace uses of the stale cb.callback pointer for timestamp tracking and invocation, while preserving the existing rooting and callback iteration flow.
🧹 Nitpick comments (1)
crates/perry-runtime/src/process/env_misc.rs (1)
1104-1111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "root + rewrite cached f64" logic into one helper. All three scanner functions repeat the identical five-line body that reads a
Cell<f64>, checks the zero sentinel, callsvisitor.visit_nanbox_f64_slot, and writes back on relocation. The root cause is the lack of a shared helper for this cache-rooting idiom.
crates/perry-runtime/src/process/env_misc.rs#L1104-L1111: extract the body ofscan_process_env_cache_roots_mutinto a shared helper and call it with&CACHED_ENVbound viawith.crates/perry-runtime/src/process/permission.rs#L204-L211: call the same shared helper forCACHED_PERMISSIONinstead of repeating the body.crates/perry-runtime/src/process/report.rs#L119-L126: call the same shared helper forCACHED_REPORTinstead of repeating the body.♻️ Proposed shared helper
pub(crate) fn scan_cached_f64_root( cell: &std::cell::Cell<f64>, visitor: &mut crate::gc::RuntimeRootVisitor<'_>, ) { let mut value = cell.get(); if value != 0.0 && visitor.visit_nanbox_f64_slot(&mut value) { cell.set(value); } }Each site then becomes, e.g.:
-pub(crate) fn scan_process_env_cache_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { - CACHED_ENV.with(|cell| { - let mut value = cell.get(); - if value != 0.0 && visitor.visit_nanbox_f64_slot(&mut value) { - cell.set(value); - } - }); -} +pub(crate) fn scan_process_env_cache_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + CACHED_ENV.with(|cell| scan_cached_f64_root(cell, visitor)); +}🤖 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/process/env_misc.rs` around lines 1104 - 1111, Extract the repeated cached-f64 rooting logic into a shared scan_cached_f64_root helper that reads the Cell<f64>, skips the zero sentinel, visits the value, and writes back relocations. Update scan_process_env_cache_roots_mut in crates/perry-runtime/src/process/env_misc.rs#L1104-L1111, and the corresponding scanners in crates/perry-runtime/src/process/permission.rs#L204-L211 and crates/perry-runtime/src/process/report.rs#L119-L126, to call this helper through each cache’s with binding.
🤖 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-stdlib/src/worker_threads.rs`:
- Around line 1899-1915: Update js_worker_threads_on to call
ensure_parent_port_event_gc_scanner() at its start, before storing
MESSAGE_CALLBACK or CLOSE_CALLBACK raw closure pointers. Leave
scan_parent_port_event_roots_mut unchanged.
---
Outside diff comments:
In `@crates/perry-runtime/src/frame.rs`:
- Around line 142-179: Within the callback loop, derive the refreshed
ClosureHeader pointer from batch_handle and use it consistently for both
js_closure_call2 and the LAST_FIRE_BY_CLOSURE map key. Replace uses of the stale
cb.callback pointer for timestamp tracking and invocation, while preserving the
existing rooting and callback iteration flow.
---
Nitpick comments:
In `@crates/perry-runtime/src/process/env_misc.rs`:
- Around line 1104-1111: Extract the repeated cached-f64 rooting logic into a
shared scan_cached_f64_root helper that reads the Cell<f64>, skips the zero
sentinel, visits the value, and writes back relocations. Update
scan_process_env_cache_roots_mut in
crates/perry-runtime/src/process/env_misc.rs#L1104-L1111, and the corresponding
scanners in crates/perry-runtime/src/process/permission.rs#L204-L211 and
crates/perry-runtime/src/process/report.rs#L119-L126, to call this helper
through each cache’s with binding.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d91bbe6a-2145-482b-a6ac-9d20eb27cb04
📒 Files selected for processing (20)
changelog.d/7239-gc-unrooted-runtime-caches.mdcrates/perry-runtime/src/frame.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_get_set/accessors.rscrates/perry-runtime/src/object/global_fetch.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/populate.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/env_misc.rscrates/perry-runtime/src/process/permission.rscrates/perry-runtime/src/process/report.rscrates/perry-runtime/src/tty.rscrates/perry-runtime/src/tui/input.rscrates/perry-stdlib/src/worker_threads.rstest-files/test_gap_gc_process_env_cache_rooting.tstest-parity/gc_repsel_corpus.txt
| fn scan_parent_port_event_roots_mut(visitor: &mut perry_runtime::gc::RuntimeRootVisitor<'_>) { | ||
| MESSAGE_EVENT_CALLBACKS.with(|cbs| { | ||
| for cb in cbs.borrow_mut().iter_mut() { | ||
| // Stored as a raw closure pointer (i64). Box it into a NaN-boxed | ||
| // pointer slot so the GC can visit + relocate it, then unbox. | ||
| let mut boxed = perry_runtime::value::js_nanbox_pointer(*cb).to_bits(); | ||
| visitor.visit_nanbox_u64_slot(&mut boxed); | ||
| *cb = perry_runtime::value::js_nanbox_get_pointer(f64::from_bits(boxed)); | ||
| visit_raw_closure_i64(visitor, cb); | ||
| } | ||
| }); | ||
| MESSAGE_CALLBACK.with(|cb| { | ||
| if let Some(ptr) = cb.borrow_mut().as_mut() { | ||
| visit_raw_closure_i64(visitor, ptr); | ||
| } | ||
| }); | ||
| CLOSE_CALLBACK.with(|cb| { | ||
| if let Some(ptr) = cb.borrow_mut().as_mut() { | ||
| visit_raw_closure_i64(visitor, ptr); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'ensure_parent_port_event_gc_scanner\s*\(' crates/perry-stdlib/src/worker_threads.rs
rg -n -C 20 'pub extern "C" fn js_worker_threads_on\b' crates/perry-stdlib/src/worker_threads.rsRepository: PerryTS/perry
Length of output: 2895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l crates/perry-stdlib/src/worker_threads.rs
sed -n '1748,1930p' crates/perry-stdlib/src/worker_threads.rs
rg -n 'MESSAGE_CALLBACK|CLOSE_CALLBACK|parent_port_event_listener|ensure_parent_port_event_gc_scanner|scan_parent_port_event_roots_mut|gc_register_mutable_root_scanner_named' crates/perry-stdlib/src/worker_threads.rsRepository: PerryTS/perry
Length of output: 9376
Register the parent-port event scanner before storing on() callbacks.
js_worker_threads_on saves MESSAGE_CALLBACK/CLOSE_CALLBACK as raw closure pointers, but that path does not call ensure_parent_port_event_gc_scanner(). These listeners can remain unscanned across GC collections. Add the guard at the start of js_worker_threads_on.
🤖 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-stdlib/src/worker_threads.rs` around lines 1899 - 1915, Update
js_worker_threads_on to call ensure_parent_port_event_gc_scanner() at its start,
before storing MESSAGE_CALLBACK or CLOSE_CALLBACK raw closure pointers. Leave
scan_parent_port_event_roots_mut unchanged.
1 participant
The #7231 enumeration, verified and closed. Ten unrooted runtime-side caches, plus one scanner that walked one of three sibling slots — the class #7226 established, which is strictly worse than the #7154 stale-register class and which no static checker can ever find, because
gc_root_dominance_check.pyreads emitted LLVM IR and a runtime table is not in it.★
CACHED_ENV—process.envThe headline, and it is a hard crash rather than a subtle wrong answer.
js_process_env_implbuildsprocess.envonce withcrate::object::js_object_alloc— the nursery — and stores it in a thread-localCell<f64>. That cell is the entire reference graph:process.envis not a field of theprocessobject, it is ajs_process_env()call that returns the cache. So the first minor collection swept or evacuated the object, and every laterprocess.env.X = vwrote through a dangling pointer into abandoned memory.The sibling
PROCESS_FINALIZATION_OBJECTuses the identical materialize-once-cache idiom and is rooted (scan_process_finalization_roots_mut), which is what makes this an omission rather than a design.The observable is ENUMERATION. A direct
process.env.KEYread lowers tojs_getenvand asks the OS, so it is correct whatever state the cached object is in. What walks the cached object isObject.keys(process.env),for…inand spread — which is exactly how@next/envanddotenvconsume it. A witness that tested the read would be a gate that cannot fail.obj_type=2is an object; 416 bytes is the header plus the inline env slots.The rest of the population
Each was taken to its declaration, its allocator, its dereference and a
grepofgc/for a scanner before being accepted — the campaign's record is roughly 1 diagnosis in 3 naming the wrong cause, and this round refuted several (below).process/env_misc.rsCACHED_ENVprocess/permission.rsCACHED_PERMISSIONruntime_write_barrier_root_nanboxnext to it is an incremental mark barrier, not a root registration — it does nothing for the sweep or the evacuation rewrite. Hoisted out of the fn body so a scanner can reach it.process/report.rsCACHED_REPORTobject/global_this/populate.rsERROR_CONSTRUCTOR_PTRusizeof ajs_closure_allocclosure. The canonicalErrorclosure is aglobalThisfield, so the structural trace keeps it alive and rewrites that reference; this duplicate is outside the object graph, soerror_prepare_stack_trace_overridereadsprepareStackTraceoff an abandoned closure after a move. Move hazard only.tui/input.rsINPUT_HANDLERuseInputarrow. Written inline in idiomatic ink style, so nothing else refers to it oncejs_perry_tui_use_inputreturns;drain_inputcalls through it on every keystroketty.rsRESIZE_CALLBACKprocess.stdout.on('resize', cb)stored in a native slot that bypasses the (rooted) EventEmitter listener arrayframe.rsFRAME_CALLBACKSonFrame(cb), rooted only transiently by aRuntimeHandleScopeduring registration. Theunsafe impl Send's SAFETY comment asserted the closures "point to global compiled code / GC-rooted data" — that premise was false and is corrected in placeobject/class_registry/construct.rsCURRENT_NEW_TARGETthis_binding.rs'sNEW_TARGETtracks correctly, held across a whole constructor bodyobject/field_get_set/accessors.rsACCESSOR_RECEIVER_OVERRIDEgettrap, i.e. arbitrary allocating user codeobject/global_fetch.rsPENDING_FETCH_SIGNALAbortSignal, sole reference across the fetch call's own argument loweringPlus one scanner gap of the shape #7230 found twice.
worker_threads.rs'sscan_parent_port_event_roots_mutvisitedMESSAGE_EVENT_CALLBACKSand notMESSAGE_CALLBACKorCLOSE_CALLBACK— three slots declared in the samethread_local!block, all holding the same thing (a rawClosureHeader*asi64, the only reference to a user callback). The Node-styleparentPort.on('message')/on('close')handlers were reclaimed by the next collection inside a worker. The box/visit/unbox dance is factored into one helper, because three copies of it is how the fourth gets forgotten.Two more windows closed in
frame.rs, both found while rooting the queuejs_frame_tickdrained the queue into an unrooted localVecand rooted each callback only as it invoked it — so callback Support custom menu bar items #1 running arbitrary user code left linux compilation and README #2..#N naked, in storage no root walk visits. That is fix(gc): root the callback and every staged argument of the timer family #7230's staging-buffer shape. One batchRuntimeHandleScopenow covers the whole set before the first call.js_on_frame_callbackheld the queue mutex acrosscapture_context(), which allocates. Harmless before; a self-deadlock the moment a root scanner locks the same mutex.capture_contextmoved above the lock, and the closure address re-read below it.Verification
Measured against
c9cd73ba5, both arms built from this worktree. The change is runtime-only, so the arms differ inlibperry_{runtime,stdlib}.a;PERRY_RUNTIME_DIRselects which is linked and the archives were hashed to prove they moved.test-files/test_gap_gc_process_env_cache_rooting.ts, registered intest-parity/gc_repsel_corpus.txt(so #7228'sgc-moving-witnessesarm gates it, and that arm rejectsUNVERas hard asFAIL, so an inert cell fails):PERRY_GC_MOVING_LOOP_POLLS=1(compile and run) +INCREMENTAL=0 CONSERVATIVE_STACK_SCAN=off HEAP_LIMIT=8bad 010/10+ ZEAL=1 PROTECT_FROMSPACE=1 DEPTH=800obj_type=2 size=416bad 05/5bad 05/5node --experimental-strip-types26.5.1The base arm being clean on the shipped default is what makes this a
requires=movewitness rather than a pre-existing failure this PR happens to sit next to.cargo fmt --all --checkclean.What this does NOT close, stated rather than implied
promise/rejection.rsinternally_handled— verified real and not fixed here. It is aHashSet<usize>of raw promise addresses consulted by identity (is_internally_handled), andscan_unhandled_rejection_roots_mutvisitsunhandled/reported/pending_handledand not it. Consequence is not a UAF: a stream's internalclosedpromise loses its marking after a move (spuriousunhandledRejection), and an address reused by a later promise inherits it (a real unhandled rejection swallowed). The fix is a rekey, not a root —visit_metadata_usize_slotplus the remove/insert loopscan_map_iterator_array_roots_mutalready uses — and it deserves its own witness. Note it is registered non-budgeted, so it is not a step-twin gap.CURRENT_NEW_TARGETandACCESSOR_RECEIVER_OVERRIDEboth park the displaced value in a bare Rust local across the window and republish it afterwards. Runtime frames are not covered by the precise scan, so those locals are fix(gc): root the interned typeof strings, the rawJSON key, and every saved implicitthis#7226'sprev_thisdefect in Rust rather than in codegen. Rooting the cell closes the armed value, not the saved one; the residual is written at each declaration and needsRuntimeHandleScopeplumbing.module_require.rsMODULE_PATH_REGISTRY— holdsmodule.exportsvalues, but it is a process-globalRwLockwhile arenas are per-thread, so a naive scanner would be unsound acrossperry/threadagents. Deliberately left.Refuted — the negative results are the point
(FULL, STEP)pairs were diffed field-by-field by reading both bodies (runtime_handles,temp_roots,promise×13 tables,timer×5, class side-table ×10, symbol ×5, tui hooks, tui state). No drift. fix(gc): root the callback and every staged argument of the timer family #7230'sIntervalTimer.argsfix reached both twins. Two symmetric oddities noted and left:CALLBACK_TIMERS.argsis visited twice on both paths (a merge artifact — harmless, but it doubles the incremental root budget), andMOCK_TIMERSskips thecrate::agent::ownsfilter its three siblings apply.buffer/header.rs's nine address registries are not root gaps.GC_TYPE_BUFFERandGC_TYPE_TYPED_ARRAYboth carrymovable: falseingc/types.rs, so the addresses cannot go stale, and the registries are identity sets rather than liveness references. What they do share with the pre-fixSYMBOL_POINTERSis address-reuse aliasing after death, which wants pruning (prune_dead_symbol_pointers's shape), not rooting.static_plugins.rsSTATIC_PLUGINSis unreachable.perry_register_static_pluginhas no caller anywhere in the workspace. Latent, not live — and by CLAUDE.md's kill-policy it should get a caller or be deleted rather than acquire a scanner nobody exercises.json/mod.rsSHAPE_CACHE's safety comment ("within one top-level stringify call no GC runs over the user object graph") is falsified bytoJSON(), but the consequence is a wrong shape template, and the fix is invalidation rather than rooting.CACHED_PERMISSION'sruntime_write_barrier_root_nanboxlooked like a root registration and is not — recorded at the declaration so the next reader does not have to re-derive it.Refs #7231, #7226, #7230, #7210, #7154, #7196.
Summary by CodeRabbit