fix(gc): disable evacuating minor by default pending #7154 (use-after-free on dynamically-added fields) - #7161
Merged
proggeramlug merged 1 commit intoAug 1, 2026
Conversation
The evacuating (moving-loop) minor GC that PerryTS#7019 flipped default-on has a use-after-free: a young closure referenced from a dynamically-added object field (field[1], holders built in proxy::create_or_update_receiver_property from zod's $constructor) is reclaimed while still live, so the field dangles and a later call dies with `TypeError: value is not a function`. A live-trace evacuation probe confirmed the edge IS enumerated on the evac path but classify() correctly rejects the target because it already points at reclaimed/garbage NurseryEden memory (dangling nanboxed pointer) — the closure died in an earlier cycle. This reproduces in the DEFAULT config (no env), so the shipped binary can corrupt the heap. Flip both the runtime gate (gc_moving_loop_polls_enabled) and the coherent codegen gate (moving_safepoint_polls_enabled) to DEFAULT OFF, restoring the previously-correct non-moving minor. The moving-loop path is unchanged and still reachable behind an explicit PERRY_GC_MOVING_LOOP_POLLS=1 opt-in (compile + run). Stopgap only; root cause tracked in PerryTS#7154. Verification: - default `./bin-registry --help` (no env): clean 10/10 (was crashing) - compiled+run with PERRY_GC_MOVING_LOOP_POLLS=1: still crashes (path retained) - cargo test -p perry-runtime: added test_moving_loop_minor_off_by_default_7154; only pre-existing order-dependent / toolchain failures remain (confirmed identical on the clean base).
📝 WalkthroughWalkthroughThe moving-loop minor GC changes from default enabled to explicit opt-in. Runtime and codegen accept ChangesMoving-loop minor GC configuration
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/tests/triggers.rs (1)
432-453: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the codegen side of this gate.
This test validates only
moving_loop_polls_enabled_from_env. The codegen path incrates/perry-codegen/src/stmt/loops.rsLines 5236-5241 uses a separate cached predicate. Add a codegen test or generated-output assertion for unset, invalid, and accepted values. Otherwise the runtime and codegen gates can diverge while this test remains green.🤖 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/gc/tests/triggers.rs` around lines 432 - 453, Add coverage for the separate cached moving-loop predicate used by the loop codegen path in loops.rs, verifying unset and invalid values disable the moving path while accepted opt-in values enable it. Reuse the same value cases as test_moving_loop_minor_off_by_default_7154 and assert the resulting codegen or generated output, ensuring runtime and codegen gates remain consistent.
🤖 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.
Nitpick comments:
In `@crates/perry-runtime/src/gc/tests/triggers.rs`:
- Around line 432-453: Add coverage for the separate cached moving-loop
predicate used by the loop codegen path in loops.rs, verifying unset and invalid
values disable the moving path while accepted opt-in values enable it. Reuse the
same value cases as test_moving_loop_minor_off_by_default_7154 and assert the
resulting codegen or generated output, ensuring runtime and codegen gates remain
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a31b390-29f4-4b01-8a3c-7cb9d621d5ec
📒 Files selected for processing (4)
changelog.d/7154-gc-disable-evacuating-minor-default.mdcrates/perry-codegen/src/stmt/loops.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/tests/triggers.rs
proggeramlug
pushed a commit
that referenced
this pull request
Aug 1, 2026
The full release suite caught the design error: deferring the allocation-point old-gen reclaim breaks #5476's regression test, which asserts that a SINGLE gc_check_trigger call completes the reclaim. That guarantee exists because the workload it was filed for is a compute-only loop reaching no host step, and the bug title was 'RSS climbs unbounded'. Two reasons the deferral was wrong here: 1. The headline RSS argument does not apply. The +364%..+5371% figure is the cost of the copying minor becoming ineligible; this arm runs a FULL mark-sweep, non-moving either way. The scan costs conservative retention for one cycle, not evacuation -- much smaller, and unmeasured. 2. It trades a measured RSS guarantee for an unmeasured one. So the alloc-point arm is unchanged and counted, and gc_safepoint_moving_minor keeps the precise full mark-sweep it gained -- programs reaching a safepoint get the reclaim precisely and NO LATER than before. The fallback is attacked by adding a competing earlier precise path, not by postponing the collection. Removes the old-gen deferral state and the test-only slack override with it. Default-robustness for #7161 (proposed PERRY_GC_MOVING_LOOP_POLLS flip to OFF), now tested in both directions: - old-gen alloc-point arm: does not read the gate at all -> identical behaviour and identical census under either default. - host pressure precise inline collection: does not read the gate -> unaffected. - host pressure deferral: js_gc_loop_safepoint goes no-op, but the lowered arena trigger still makes the next allocation-point check collect the owed cycle; test asserts that backstop directly. - nursery-churn valve: pre-existing code already skips the deferral when polls are off, so every nursery collection scans conservatively -- inert and sound, but the census now makes that cost visible.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 1, 2026
proggeramlug
added a commit
that referenced
this pull request
Aug 1, 2026
…n OS memory pressure, and census every conservative-scan fallback (#7166) * fix(gc): defer old-gen reclaim and host pressure to precise safepoints; census every conservative-scan fallback #7148. Six force_full_scan() sites forced the conservative native-stack scan, four of them without any user calling gc(). A scanning cycle is not merely slower: it makes the copying minor ineligible, so it runs no copying minor at all (gc_ratchet README: heap_used_bytes +364%..+5371%, minor_cycles -> 0 on all eight probes). Per-site disposition: - OldReclaim alloc point -> DEFER. gc_safepoint_moving_minor now drains the old-gen reclaim as the same full mark-sweep with precise roots. The alloc-point arm keeps a slack valve bounded in the trigger's OWN unit (old-gen in-use + external side bytes, one 32 MB growth quantum), because an arena-unit slack would never expire for the >16 KB old-arena workload #5476 exists for -- #7024's bug inverted. - Host pressure -> DEFER when a generated frame is live (arm the deferral, return the already-documented code 1); collect PRECISELY, no scan, when the shadow stack is empty -- the run-loop-boundary case the module's own docs call typical, and the case where deferring would shed nothing because an idle process reaches no safepoint. - Nursery-churn valve, emergency reclaim, gc(), perry/gc minor() -> KEEP, now counted. Emergency reclaim provably cannot defer: it runs after an allocation has already failed and the caller's next act is to panic, so there is no next safepoint. Every site is now recorded in gc/scan_fallback.rs with a PERRY_GC_DIAG line, and the two deferral paths record a drain counter -- so a gate can assert its subject was live (CLAUDE.md four-ways-a-gate-cannot-fail #4) instead of only that nothing crashed. Also: PERRY_CONSERVATIVE_STACK_SCAN=full failed 134/1574 runtime tests. In the test build a pinned per-thread override now beats an env request for Full -- the env may make the scan less aggressive than a test declared, never more. Kept rather than deleted because =full is the gc_ratchet's validated sensitivity arm, the only end-to-end proof that gate can fail. * test(gc): red-then-green coverage for the #7148 safepoint deferrals Every deferral test asserts BOTH that the conservative valve did not fire and that the precise safepoint collection which replaced it DID run (a drain counter). Checking only the first half would pass on a tree where the trigger never armed at all -- the largest possible regression reported as a pass (CLAUDE.md, four ways a gate cannot fail, #4). Adds two test-only scoped overrides next to the existing force_legacy_gc_pacing pattern: pinned moving pacing (so a deferral test cannot silently run under legacy pacing and pass for the wrong reason) and a shrinkable old-gen deferral slack (the shipped slack is 32 MB; crossing it for real would mean committing 32 MB of old-gen inside a unit test). * docs(gc-ratchet): record why PERRY_CONSERVATIVE_STACK_SCAN=full is kept and how it was verified * changelog: fragment for PR 7166 * refactor(gc): split roots/scan_mode.rs out; delete the host-pressure conservative arm outright Two follow-ups the compiler found. 1. gc/roots.rs crossed the 2000-line file-size gate (1992 -> 2035). The conservative-scan mode block is topically self-contained -- the mode enum, its env/override precedence, ManualGcScanGuard, and the decision the mark phase consults -- so it moves to roots/scan_mode.rs. roots.rs is now 1851. 2. ConservativeScanSite::HostPressure was never constructed, because after the deferral js_gc_memory_pressure has NO conservative arm left: it either collects with precise roots or defers. So the variant is deleted rather than kept 'for completeness'. An enum arm nothing can produce is a claim no test can check -- the host-pressure tests now assert automatic_scan_fallback_total == 0 instead, which catches reintroduction at any automatic site. The counter read APIs are #[cfg(test)]; ops observability is the PERRY_GC_DIAG line, which is unconditional. * docs(gc): correct SafepointDrainKind::HostPressure — it is a synchronous inline collection, not a deferred drain (CodeRabbit) * fix(gc): site 1 is 'keep + add a competing precise path', not 'defer' The full release suite caught the design error: deferring the allocation-point old-gen reclaim breaks #5476's regression test, which asserts that a SINGLE gc_check_trigger call completes the reclaim. That guarantee exists because the workload it was filed for is a compute-only loop reaching no host step, and the bug title was 'RSS climbs unbounded'. Two reasons the deferral was wrong here: 1. The headline RSS argument does not apply. The +364%..+5371% figure is the cost of the copying minor becoming ineligible; this arm runs a FULL mark-sweep, non-moving either way. The scan costs conservative retention for one cycle, not evacuation -- much smaller, and unmeasured. 2. It trades a measured RSS guarantee for an unmeasured one. So the alloc-point arm is unchanged and counted, and gc_safepoint_moving_minor keeps the precise full mark-sweep it gained -- programs reaching a safepoint get the reclaim precisely and NO LATER than before. The fallback is attacked by adding a competing earlier precise path, not by postponing the collection. Removes the old-gen deferral state and the test-only slack override with it. Default-robustness for #7161 (proposed PERRY_GC_MOVING_LOOP_POLLS flip to OFF), now tested in both directions: - old-gen alloc-point arm: does not read the gate at all -> identical behaviour and identical census under either default. - host pressure precise inline collection: does not read the gate -> unaffected. - host pressure deferral: js_gc_loop_safepoint goes no-op, but the lowered arena trigger still makes the next allocation-point check collect the owed cycle; test asserts that backstop directly. - nursery-churn valve: pre-existing code already skips the deferral when polls are off, so every nursery collection scans conservatively -- inert and sound, but the census now makes that cost visible. * changelog: update fragment for the reworked site-1 disposition and the #7161 interaction --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Contributor
|
Working #7154. PR #7179 fixes one traced root cause — two raw, unbarriered pointer stores into GC-traced payload slots ( It does not make this stopgap unnecessary. On my reproducer (zod 4.4.3 compiled natively) #7179 takes dangling closure-capture edges from 294/cycle to 0, but the binary still fails in the default configuration and is still clean under |
proggeramlug
pushed a commit
that referenced
this pull request
Aug 1, 2026
proggeramlug
added a commit
that referenced
this pull request
Aug 1, 2026
Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 1, 2026
…traced payload slots (#7179) * fix(gc): record layout+barrier on raw pointer publishes into traced payload slots Refs #7154 * test(gc): #7154 pointer-publish invariant regression tests * test(gc): strengthen #7154 weakmap barrier test (old entry -> young value, live canary) * docs: changelog fragment for #7154 * docs: key changelog fragment to PR 7179 * test(gc): register #7154 pointer-publish gap test in the GC x repsel corpus * fix(gc): root the receiver across the barriered this-capture store (#7179 review) * docs(gc): record that #7154's corpus member is live only on the moving arms after #7161 * docs: changelog follow-up for the #7179 review fixes --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stopgap for #7154. The evacuating (moving-loop) minor GC that #7019 flipped default-on has a use-after-free that corrupts the heap in the default configuration — plain
sfw-registry --help(no env) exits 1 withTypeError: value is not a function. This flips the default back to the previously-correct non-moving minor while the root cause is fixed under #7154. This does not close #7154 (the collector bug is still open); it stops shipping a heap-corrupting default.The bug (diagnosis, tracked in #7154)
A young closure referenced from a dynamically-added object field (
field[1], holders built inproxy::create_or_update_receiver_propertyfrom zod's$constructor) is reclaimed by the evacuating minor while still live, so the field dangles and a later call dies withTypeError: value is not a function.A live-trace probe on the evacuation path (temporary, reverted) established:
visit_value_bits → classifyreturnsNone, so the slot is never rewritten.0x7ffd…) into NurseryEden whose target header is garbage in 100% of samples (plausible_arena=false). It is a dangling pointer — the closure died in an earlier cycle. A "teach classify/move_youngto accept it" fix would evacuate garbage → SIGSEGV, which is why the deep fix is left to GC: evacuating minor drops an old-to-young field[1] edge, crashing with 'value is not a function' #7154 rather than attempted here.Full probe write-up: #7154 (comment)
The change
Flip both coherent gates from default-ON to default-OFF, keeping the flag as an opt-in:
gc_moving_loop_polls_enabled()(runtime,gc/policy.rs)moving_safepoint_polls_enabled()(codegen,stmt/loops.rs)Both read the same
PERRY_GC_MOVING_LOOP_POLLSenv and must agree (a runtime/codegen default mismatch would defer collections that never drain). Now only an explicitPERRY_GC_MOVING_LOOP_POLLS=1/on/trueenables the moving-loop path (at compile and run time). No collector code is removed — this is a pure default flip. Trade-off: reverts #7019's minor-GC RSS/throughput win, not correctness.Verification
sfw-registry --help(no env), rebuilt on this branchPERRY_GC_MOVING_LOOP_POLLS=1cargo test -p perry-runtimenew testtest_moving_loop_minor_off_by_default_7154cargo test -p perry-runtime(rest)gc::tests::teardown::map_set_*(order-dependent),object::global_this::global_this_webassembly::…(order-dependent), and theshadow_stack_ops::out_of_range_frame_pop_is_ignoreddebug-nounwind abort — all confirmed present identically on the clean baseRebuild note for reviewers:
rm target/perry-auto-*/release/libperry_stdlib.athenPERRY_FORCE_WELL_KNOWN=iovalkey perry compile src/sfw-registry/cli.ts -o bin-registry(orPERRY_NO_CACHE=1) to pick up the runtime-lib change.Refs #7154
Summary by CodeRabbit
Bug Fixes
Documentation
Tests