feat(gc): seeded GC-schedule fuzzing (PERRY_GC_SCHEDULE_SEED) — a failing schedule you can replay - #7317
feat(gc): seeded GC-schedule fuzzing (PERRY_GC_SCHEDULE_SEED) — a failing schedule you can replay#7317jdalton wants to merge 2 commits into
PERRY_GC_SCHEDULE_SEED) — a failing schedule you can replay#7317Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change replaces GC zeal with deterministic, rate-controlled seeded scheduling. It integrates scheduled safepoints with collection and evacuation policy, adds exit and signal diagnostics, provides tests and fuzzing tools, and updates GC reproduction documentation. ChangesSeeded GC schedule fuzzing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant Safepoint
participant Schedule
participant Collector
participant Reporter
Runtime->>Schedule: resolve seed and rate
Safepoint->>Schedule: advance handled safepoint
Schedule-->>Safepoint: return collection selection
Safepoint->>Collector: perform scheduled moving minor
Collector->>Reporter: record forced collection
Reporter-->>Runtime: report counters on exit or failure
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 |
2467132 to
5d8ce73
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
crates/perry-runtime/src/gc/schedule.rs (1)
244-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the "startup banner" claim with the actual announcement point.
The module documentation at Lines 331-334 describes layer 1 as "a startup banner, so the seed is in the log even if the failure mode is a hang or a
_exitthat runs no handler at all".resolved()runs the announcement lazily, at the first call site. For a mode-ON run, that is the first safepoint or the firstgc_force_evacuate_enabled()query. A hang or_exitbefore that point prints nothing, and no panic hook or signal handler is installed either.Consider resolving the configuration eagerly from GC initialization, or narrow the documentation claim to "the first safepoint" so an operator does not read a missing banner as "the seed was not set".
🤖 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/schedule.rs` around lines 244 - 252, The startup-banner documentation does not match the lazy announcement in resolved(). Either eagerly resolve the configuration during GC initialization so publish_seed and announce run before early hangs or _exit paths, or narrow the layer-1 documentation to state that the banner appears at the first safepoint or gc_force_evacuate_enabled() query; preserve the existing seed publication behavior.crates/perry-runtime/src/gc/tests/schedule.rs (1)
276-283: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRelease the GC root lock with a guard so a panic cannot leak the depth.
enter_gc_root_lock()andexit_gc_root_lock()are paired manually. Ifgc_safepoint_moving_minor()panics,exit_gc_root_lock()never runs, the root-lock depth stays non-zero for this thread, and every later collection on that thread is blocked. That converts one failure into a cascade of confusing failures in the same test binary.♻️ Proposed fix using a scope guard
let safepoints_before = gc_schedule_safepoints(); { let _schedule = ScheduleGuard::set(7, rate_threshold(1.0)); reset_thread_counter_for_test(); - super::super::roots::enter_gc_root_lock(); - gc_safepoint_moving_minor(); - super::super::roots::exit_gc_root_lock(); + struct RootLock; + impl RootLock { + fn enter() -> Self { + super::super::roots::enter_gc_root_lock(); + Self + } + } + impl Drop for RootLock { + fn drop(&mut self) { + super::super::roots::exit_gc_root_lock(); + } + } + let _lock = RootLock::enter(); + gc_safepoint_moving_minor(); }If the test support module already exposes a root-lock guard type, use it instead of the local shim.
🤖 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/schedule.rs` around lines 276 - 283, Update the test block around gc_safepoint_moving_minor to use the existing GC root-lock scope guard, if exposed by the test support module, instead of manually pairing enter_gc_root_lock and exit_gc_root_lock. Ensure the guard releases the lock during unwinding as well as normal completion, and remove the corresponding explicit exit call.docs/src/internals/gc-rooting-invariant.md (1)
271-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the
gc_schedule_fuzz.shargument syntax deterministic.The script accepts
<binary> [seed-count], butCLAUDE.mdstill says[seeds]. Update that line so the two docs use the actual positional argument semantics.🤖 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 `@docs/src/internals/gc-rooting-invariant.md` around lines 271 - 279, Update the gc_schedule_fuzz.sh usage text in CLAUDE.md to describe the second positional argument as seed-count, matching the script’s actual <binary> [seed-count] semantics. Also review the usage reference in docs/src/internals/gc-rooting-invariant.md and changelog.d/7307-seeded-gc-schedule-fuzzing.md at the specified ranges; update any remaining [seeds] wording there to [seed-count], with no other changes.
🤖 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 `@CLAUDE.md`:
- Around line 145-146: Add a required CI workflow arm for the seeded GC schedule
OFF state, using a compiled program to test both an unset PERRY_GC_SCHEDULE_SEED
and PERRY_GC_SCHEDULE_RATE set without a seed. Verify both remain schedule-inert
while pressure-driven collections still occur, reusing the existing
scripts/gc_schedule_fuzz.sh or schedule test infrastructure where appropriate.
In `@crates/perry-runtime/src/gc/mod.rs`:
- Around line 778-784: The exit summary is emitted during per-thread teardown,
so SUMMARY_EMITTED can capture counts before other threads finish. Update the
report_exit_summary call in the exit path to emit only after all worker threads
have completed teardown—prefer the existing main-thread or final-thread
coordination mechanism—and preserve once-only reporting with complete safepoints
and scheduled_collections totals.
In `@crates/perry-runtime/src/gc/schedule.rs`:
- Around line 240-243: Add a required CI workflow arm that runs the GC schedule
tests or relevant test suite with PERRY_GC_SCHEDULE_SEED and
PERRY_GC_SCHEDULE_RATE unset, verifying their default/OFF behavior alongside
existing CI coverage. Anchor the change to the workflow job invoking the tests
and preserve the current configured-knob coverage.
- Around line 584-608: In the signal-handler teardown around the previous
handler lookup, restore SIG_DFL before entering the previous > 1 chaining path,
so the default disposition is installed before invoking the chained handler.
Keep the existing chained-handler call and early return, but remove the
later-only restoration structure so schedule_fault_handler cannot loop when the
chained handler returns.
- Around line 493-502: Update the previous-handler storage in
reinstall_signal_reporter_after to check old.sa_flags for libc::SA_SIGINFO
before saving old.sa_sigaction. Store 0 for handlers without SA_SIGINFO, while
preserving the existing self-chain prevention and storing the handler value only
when the flag is present.
In `@docs/src/internals/gc-rooting-invariant.md`:
- Around line 281-285: Update the paragraph beginning “A rate is not a
substitute for a schedule” to qualify the ~3/N confidence bound as applying only
to independent trials. State that repeated runs with a fixed seed or
deterministic schedule are correlated, so 0/N failures provide no statistical
bound, while preserving the guidance to vary collection timing.
In `@docs/src/internals/memory-model.md`:
- Around line 138-139: Update the PERRY_GC_SCHEDULE_SEED and
PERRY_GC_SCHEDULE_RATE documentation to describe the configured rate as
additional schedule density for minor collections only, applied when
gc_budgeted_due_trigger() reports no pressure-driven collection is due. Clarify
that pressure-driven collections still occur independently, so the rate is not
the total fraction of safepoints that collect, and replace the current “iff”
wording with this behavior.
In `@scripts/gc_instrument_smoke.sh`:
- Around line 119-129: Replace the `run_arm ... | tail -1` command substitutions
for `sched_retired`, `sched_repeat`, and `sched_other` with output capture that
does not use a pipeline, then explicitly check each `run_arm` exit status and
abort on failure before comparing results. Apply the same status-preserving
change to the other arms in this script that use the pipeline pattern, while
retaining extraction of the final output line.
- Around line 150-164: The strict schedule-density checks in the smoke fixture
can fail on low safepoint counts without demonstrating a broken rate knob.
Update the fixture to generate enough handled GC safepoints for distinct
retirement counts, or revise both failure paths around sched_retired,
nozeal_retired, and zeal_retired to report all three counts before exiting.
In `@scripts/gc_schedule_fuzz.sh`:
- Around line 53-59: Validate SEED_COUNT immediately after argument parsing as a
positive integer, rejecting zero and non-numeric values with an error and
nonzero exit. In the final summary around FAILED_SEEDS and the PASS output,
track executed runs via passed plus failed seeds and exit nonzero with a failure
message when that total is zero; only report PASS after at least one seed ran.
---
Nitpick comments:
In `@crates/perry-runtime/src/gc/schedule.rs`:
- Around line 244-252: The startup-banner documentation does not match the lazy
announcement in resolved(). Either eagerly resolve the configuration during GC
initialization so publish_seed and announce run before early hangs or _exit
paths, or narrow the layer-1 documentation to state that the banner appears at
the first safepoint or gc_force_evacuate_enabled() query; preserve the existing
seed publication behavior.
In `@crates/perry-runtime/src/gc/tests/schedule.rs`:
- Around line 276-283: Update the test block around gc_safepoint_moving_minor to
use the existing GC root-lock scope guard, if exposed by the test support
module, instead of manually pairing enter_gc_root_lock and exit_gc_root_lock.
Ensure the guard releases the lock during unwinding as well as normal
completion, and remove the corresponding explicit exit call.
In `@docs/src/internals/gc-rooting-invariant.md`:
- Around line 271-279: Update the gc_schedule_fuzz.sh usage text in CLAUDE.md to
describe the second positional argument as seed-count, matching the script’s
actual <binary> [seed-count] semantics. Also review the usage reference in
docs/src/internals/gc-rooting-invariant.md and
changelog.d/7307-seeded-gc-schedule-fuzzing.md at the specified ranges; update
any remaining [seeds] wording there to [seed-count], with no other changes.
🪄 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: 77947a25-2bab-41b8-b287-31da05030b5f
📒 Files selected for processing (12)
CLAUDE.mdchangelog.d/7307-seeded-gc-schedule-fuzzing.mdcrates/perry-runtime/src/arena/quarantine.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/schedule.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/schedule.rsdocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mdscripts/gc_instrument_smoke.shscripts/gc_schedule_fuzz.sh
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CLAUDE.md`:
- Line 145: Update the PERRY_GC_SCHEDULE_SEED documentation to name Perry’s
process-exit teardown funnel as the source of seed reporting for _exit-based
exits, while describing atexit only as an additional reporting path. Preserve
the existing panic and signal-reporting paths and all other seed behavior.
- Around line 145-148: Condense the PERRY_GC_SCHEDULE_SEED and
PERRY_GC_SCHEDULE_RATE entries in CLAUDE.md to their concise runtime contract,
removing implementation rationale, reproduction guidance, and historical
context. Move that detailed narrative, including measurement guidance, to
changelog.d/7317-seeded-gc-schedule-fuzzing.md while preserving the documented
behavior and configuration semantics.
- Line 148: Update the documented invocation of scripts/gc_schedule_fuzz.sh to
use the optional argument name [seed-count] instead of [seeds], while preserving
the existing binary argument and surrounding guidance.
🪄 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: 91c7b636-1ffb-4ec0-ba19-591b2f2b1559
📒 Files selected for processing (12)
CLAUDE.mdchangelog.d/7317-seeded-gc-schedule-fuzzing.mdcrates/perry-runtime/src/arena/quarantine.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/schedule.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/schedule.rsdocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mdscripts/gc_instrument_smoke.shscripts/gc_schedule_fuzz.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/src/internals/gc-rooting-invariant.md
- crates/perry-runtime/src/arena/quarantine.rs
- crates/perry-runtime/src/gc/tests/mod.rs
- scripts/gc_instrument_smoke.sh
- crates/perry-runtime/src/gc/mod.rs
- docs/src/internals/memory-model.md
- crates/perry-runtime/src/gc/policy.rs
- scripts/gc_schedule_fuzz.sh
- crates/perry-runtime/src/gc/tests/schedule.rs
- crates/perry-runtime/src/gc/schedule.rs
|
The premise is right and it is the most useful framing anyone has put on this class:
That explains something we have been misreading. #7280's acceptance arms read 6, 8, 9 out of 30 across three runs of the same parent — we have been treating that as noise to work around, when it is really one schedule being sampled repeatedly. A seeded sweep is the right instrument, and it arrives at exactly the moment it is most needed: the owner has chosen to make statepoints the default and delete the shadow stack, and the soak deciding that is running now. Not merging yet, for two reasons:
What would make this land fast: the CI arm, and the two I have pointed the soak agent at this branch so it can use the sweep locally for schedule exploration without waiting on the merge — if it finds a failing seed on the statepoint arm, that is exactly the evidence the flip decision needs, and it would be a strong argument for landing this. |
5d8ce73 to
9c43d77
Compare
|
Thanks for the review — I pushed The fuzz script could report a vacuous PASS, and that was the most important catch. A sweep that never actually exercised a safepoint now reports The The two doc findings landed as suggested: The four I looked at and left alone, with the reasoningThe required OFF-state + live-subject CI arm. The required cargo-test path already carries both halves: Exit summary from any thread. The counters are process-global atomics, and the documented reproducibility scope is single-threaded, where the main thread teardown prints the final totals. Gating on a main-thread mark risks suppressing the summary on signal and exit paths where the mark was never set, which is a worse failure than the multi-threaded partial count it would fix. Left as-is deliberately. The claimed The |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@changelog.d/7317-seeded-gc-schedule-fuzzing.md`:
- Around line 45-48: Update the 0/16 statistical statement in the changelog to
identify the confidence level and interval method used for the ~19% upper bound,
specifically describing it as a 95% Wilson upper bound.
In `@CLAUDE.md`:
- Around line 145-146: Update the CI workflow coverage for the GC scheduling
configuration to add required arms for an unset PERRY_GC_SCHEDULE_SEED and for
PERRY_GC_SCHEDULE_RATE configured without a seed. In each arm, verify
pressure-driven collections remain active while schedule-triggered collections
stay disabled, matching the documented OFF-state behavior.
🪄 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: 3b65e0cf-c3cf-401f-8906-3df203dedc2b
📒 Files selected for processing (12)
CLAUDE.mdchangelog.d/7317-seeded-gc-schedule-fuzzing.mdcrates/perry-runtime/src/arena/quarantine.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/schedule.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/schedule.rsdocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mdscripts/gc_instrument_smoke.shscripts/gc_schedule_fuzz.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/perry-runtime/src/arena/quarantine.rs
- crates/perry-runtime/src/gc/policy.rs
- docs/src/internals/gc-rooting-invariant.md
- scripts/gc_schedule_fuzz.sh
- docs/src/internals/memory-model.md
- crates/perry-runtime/src/gc/mod.rs
- crates/perry-runtime/src/gc/tests/schedule.rs
- crates/perry-runtime/src/gc/schedule.rs
- crates/perry-runtime/src/gc/tests/mod.rs
- scripts/gc_instrument_smoke.sh
9c43d77 to
ca397ef
Compare
|
Follow-up in The signal-chain infinite loop was real, and my first pass was wrong to call it a false positive. Returning from a synchronous fault handler re-runs the faulting instruction, so if the chained quarantine handler also returned without resolving the fault, the disposition still pointed at us and we re-entered forever. The fix restores The The exit-summary gating I took as an improvement even though the original behavior was defensible. It now gates on a pure read, Where the OFF-state CI arm stands after thisThe required cargo-test coverage is complete as-is: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/native_handle.rs`:
- Around line 62-73: Update is_main_thread_or_unrecorded and its exit-summary
callers so an unrecorded MAIN_THREAD_ID does not allow a worker thread to claim
SUMMARY_EMITTED. Prefer ensuring js_native_handle_mark_main_thread() runs during
startup before workers begin, or make the teardown diagnostic skip callers while
MAIN_THREAD_ID remains 0; preserve acceptance only for the registered main
thread.
🪄 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: 5fe7a9c1-d4d9-4981-bddc-14cde04fae59
📒 Files selected for processing (13)
CLAUDE.mdchangelog.d/7317-seeded-gc-schedule-fuzzing.mdcrates/perry-runtime/src/arena/quarantine.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/schedule.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/schedule.rscrates/perry-runtime/src/native_handle.rsdocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mdscripts/gc_instrument_smoke.shscripts/gc_schedule_fuzz.sh
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/src/internals/gc-rooting-invariant.md
- crates/perry-runtime/src/gc/tests/mod.rs
- crates/perry-runtime/src/gc/policy.rs
- scripts/gc_instrument_smoke.sh
- docs/src/internals/memory-model.md
- crates/perry-runtime/src/arena/quarantine.rs
- crates/perry-runtime/src/gc/mod.rs
- crates/perry-runtime/src/gc/tests/schedule.rs
- scripts/gc_schedule_fuzz.sh
- crates/perry-runtime/src/gc/schedule.rs
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
changelog.d/7317-seeded-gc-schedule-fuzzing.md (1)
142-144: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReconcile the reported runtime test count before publishing this result.
This fragment reports
1670 passed, while the PR objectives report1687runtime library tests passing for the latest commit. Confirm whether the commands or feature sets differ. Update this fragment to the exact run that supports the current PR.🤖 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 `@changelog.d/7317-seeded-gc-schedule-fuzzing.md` around lines 142 - 144, Reconcile the runtime test-count claims in the changelog entry: verify the command and feature set used for the reported run against the latest PR result, then update the embedded test counts and run details to exactly match the current supporting execution. Remove or revise the stale comparison if it no longer reflects that run.docs/statepoint-gc-experiment.md (1)
933-942: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the stale platform limitation.
The bullet says scanning is “macOS/Mach-O-only,” but the same document reports x86-64 and AArch64 Linux verification at Lines 589-613. State the current platform support, or label this bullet as an earlier prototype limitation.
🤖 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 `@docs/statepoint-gc-experiment.md` around lines 933 - 942, The bullet point stating "scanning is macOS/Mach-O-only" contradicts the platform verification reported elsewhere in the document (x86-64 and AArch64 Linux support at lines 589-613). Update this bullet to accurately reflect the current platform support including Linux platforms, or if this limitation applies only to an earlier prototype version, explicitly label it as such to clarify the scope and timeline of the constraint.
♻️ Duplicate comments (1)
docs/src/internals/gc-rooting-invariant.md (1)
281-285: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQualify the
~3/Nbound.The paragraph says fixed-seed repetitions replay one schedule, but then applies the binomial
~3/Nbound to those repetitions. State that fixed-seed repetitions provide no statistical bound. Limit~3/Nto independent schedule or workload trials.🤖 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 `@docs/src/internals/gc-rooting-invariant.md` around lines 281 - 285, The paragraph currently applies the ~3/N statistical bound to fixed-seed repetitions without clarifying that this bound is invalid for that scenario. Revise the text to explicitly state that re-running the same binary with fixed seeds provides no statistical bound on the true bug rate. Then limit the ~3/N bound statement to apply only when runs use independent schedules or workloads, making clear the distinction between replayed runs (which don't accumulate statistical evidence) and varied runs (which do).
🤖 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 @.github/workflows/test.yml:
- Around line 1085-1091: Update the repository branch-protection required status
checks to include the existing GATING `gc-stress` context, ensuring failures
from the GC smoke-test workflow block merges. Do not alter the workflow test
logic.
In `@changelog.d/7219-registry-gc-unrooted-caches.md`:
- Around line 125-126: The `PERRY_GC_SCHEDULE_RATE` parameter is only effective
when `PERRY_GC_SCHEDULE_SEED` is set, so all documented test environments using
rate-1 scheduling must explicitly include the seed value. In
changelog.d/7219-registry-gc-unrooted-caches.md lines 125-126, add
`PERRY_GC_SCHEDULE_SEED=1` to both closure-call table rows. In
changelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.md lines 141-142,
add the seed to both regexp-receiver table rows. In
changelog.d/7253-gc-gate-main-line-run.md line 40, add the seed to the
14,373-minor verification environment. In
changelog.d/7270-rest-and-same-module-call-argument-rooting.md lines 55-56, add
the seed to both rest and same-module call rows. In
changelog.d/7280-optional-param-and-dynamic-construct-rooting.md line 67, add
the seed to the six-unit-reproducer environment. In
changelog.d/7317-seeded-gc-schedule-fuzzing.md lines 126-130, separate the
unseeded OFF-state trace from seeded rate-1 arms and explicitly name the seed
value for each ON-state trace environment.
In `@run_parity_tests.sh`:
- Around line 447-454: Update normalize_output’s sed filter to remove only the
known seeded GC schedule startup and exit diagnostic formats, rather than every
line beginning with “[gc-schedule]”. Preserve program output with that prefix
while continuing to strip the runtime-generated diagnostics before parity
comparison.
In `@scripts/gc_instrument_smoke.sh`:
- Line 3: Update the overview comment in gc_instrument_smoke.sh to include
PERRY_GC_SCHEDULE_RATE alongside PERRY_GC_SCHEDULE_SEED, so it documents both
scheduling controls exercised by the script.
- Line 214: The zero-probe diagnostic error message does not match the arm
number being tested. Locate the zero-probe error message that currently reports
"arm 4" and update it to report "arm 7" to align with the arm label shown in the
echo statement at line 214 for the quarantine test section, ensuring the failure
message correctly identifies which arm actually ran.
- Around line 252-253: Update the final summary near the retirement and
quarantine messages to avoid claiming zero retirements or program correctness
across all arms without corresponding assertions. Report the measured pressure
and rate retirement counts separately, and state the quarantine result
independently; alternatively, add explicit assertions for pressure_retired == 0
and Arm 7 probe-output correctness before making those claims.
In `@test-parity/gc_repsel_corpus.txt`:
- Around line 521-522: The measurement records in
test-parity/gc_repsel_corpus.txt document configuration requirements for
reproducing results but omit the PERRY_GC_SCHEDULE_SEED value that
PERRY_GC_SCHEDULE_RATE depends on. At lines 521-522 (the dynamic-construction
measurement record), add PERRY_GC_SCHEDULE_SEED=1 alongside POLLS=1 and RATE=1,
or explicitly note that the seed value is derived from the test harness. Apply
the same fix at lines 558-560 (the optional-parameter measurement record) to
ensure both documented scenarios include the complete seed specification needed
for reproducibility.
---
Outside diff comments:
In `@changelog.d/7317-seeded-gc-schedule-fuzzing.md`:
- Around line 142-144: Reconcile the runtime test-count claims in the changelog
entry: verify the command and feature set used for the reported run against the
latest PR result, then update the embedded test counts and run details to
exactly match the current supporting execution. Remove or revise the stale
comparison if it no longer reflects that run.
In `@docs/statepoint-gc-experiment.md`:
- Around line 933-942: The bullet point stating "scanning is macOS/Mach-O-only"
contradicts the platform verification reported elsewhere in the document (x86-64
and AArch64 Linux support at lines 589-613). Update this bullet to accurately
reflect the current platform support including Linux platforms, or if this
limitation applies only to an earlier prototype version, explicitly label it as
such to clarify the scope and timeline of the constraint.
---
Duplicate comments:
In `@docs/src/internals/gc-rooting-invariant.md`:
- Around line 281-285: The paragraph currently applies the ~3/N statistical
bound to fixed-seed repetitions without clarifying that this bound is invalid
for that scenario. Revise the text to explicitly state that re-running the same
binary with fixed seeds provides no statistical bound on the true bug rate. Then
limit the ~3/N bound statement to apply only when runs use independent schedules
or workloads, making clear the distinction between replayed runs (which don't
accumulate statistical evidence) and varied runs (which do).
🪄 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: f400a510-9796-4177-b65b-ed66f6000c5d
📒 Files selected for processing (30)
.github/workflows/test.ymlCLAUDE.mdchangelog.d/7196-gc-rooting-bug-instruments.mdchangelog.d/7219-registry-gc-unrooted-caches.mdchangelog.d/7227-regexp-receiver-rooting-and-alloc-re-audit.mdchangelog.d/7253-gc-gate-main-line-run.mdchangelog.d/7270-rest-and-same-module-call-argument-rooting.mdchangelog.d/7276-interned-string-cache-root-coverage.mdchangelog.d/7280-optional-param-and-dynamic-construct-rooting.mdchangelog.d/7311-dep-scale-corpus-and-root-reload.mdchangelog.d/7317-seeded-gc-schedule-fuzzing.mdcrates/perry-runtime/src/arena/quarantine.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/schedule.rscrates/perry-runtime/src/gc/tests/fromspace_protect.rscrates/perry-runtime/src/gc/tests/schedule.rscrates/perry-runtime/src/gc/zeal.rscrates/perry-runtime/src/object/class_registry/construct.rsdocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mddocs/src/internals/rfc-rooting-by-construction.mddocs/statepoint-gc-experiment.mdrun_parity_tests.shscripts/gc_instrument_smoke.shtest-files/test_gap_gc_call_argument_rooting.tstest-files/test_gap_gc_regexp_receiver_rooting.tstest-files/test_gap_gc_rest_argument_rooting.tstest-files/test_gap_gc_same_module_call_argument_rooting.tstest-parity/gc_repsel_corpus.txt
💤 Files with no reviewable changes (1)
- crates/perry-runtime/src/gc/zeal.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/perry-runtime/src/gc/policy.rs
- crates/perry-runtime/src/gc/mod.rs
- crates/perry-runtime/src/gc/tests/schedule.rs
- crates/perry-runtime/src/arena/quarantine.rs
- crates/perry-runtime/src/gc/schedule.rs
✅ Action performedComments resolved. Approval is disabled; enable |
fd1f4e2 to
4bbf0b6
Compare
bbaf38e to
e16f010
Compare
A rooting bug (PerryTS#7154 family) is a value live but not rooted across a collection point. Whether it is caught is decided by the GC schedule, not by the bug, so re-running one binary sixty times re-runs one schedule sixty times and explores almost nothing. This makes the schedule itself the knob, at any density from normal pacing up to a collection at every handled safepoint, and it hands back a reproducer. PERRY_GC_SCHEDULE_SEED=<u64> makes "should this safepoint collect?" a deterministic function of the seed and a per-thread safepoint ordinal, at a density set by PERRY_GC_SCHEDULE_RATE (default 0.05). It does exactly three things: js_gc_loop_safepoint stops requiring GC_SAFEPOINT_PENDING before it descends; past the entry guards a per-thread counter ticks once per handled safepoint and, when nothing is otherwise due, a minor runs iff splitmix64(splitmix64(seed) ^ counter) < threshold; and gc_force_evacuate_enabled() becomes true so a scheduled minor MOVES survivors. It does not bypass the entry guards, does not override PERRY_GEN_GC_EVACUATE=0, cannot emit loop polls codegen never produced, and never suppresses a pressure-driven collection. A value that does not parse as u64 reads as OFF, not as seed 0. scripts/gc_schedule_fuzz.sh <binary> [seed-count] sweeps seeds and prints a reproduce command per failure. On Socket Firewall's sfw-registry --help (loop polls compiled and run) the control failed 0/16; seeds 1..12 at rate 0.05 failed 6/12 in under two seconds, seed 1 reproducing 5/5 at the identical site. The schedule installer registers itself as the runtime main thread so it owns the once-only exit summary: while no main thread is recorded every thread passes is_main_thread_or_unrecorded and a worker tearing down first could claim the summary with non-final counts. The unrecorded fallback remains for paths where the schedule never activates. The seed is never lost: printed at startup, at exit (from the process-exit teardown funnel, since perry's _exit paths never reach atexit), on panic, and from an async-signal-safe handler for SIGSEGV/SIGBUS/SIGABRT/SIGILL/ SIGTRAP that chains rather than clobbers and that the from-space quarantine re-layers, so PERRY_GC_SCHEDULE_SEED + PERRY_GC_PROTECT_FROMSPACE reports both the seed and the precise fault site. Default off and proven inert: with no seed set, PERRY_GC_DIAG collector traces are byte-identical to the parent across five configurations on two fixtures. gc/tests/schedule.rs asserts both directions of both knobs (11 tests: parse, threshold endpoints, determinism, adjacent-seed divergence, realised density, collect/decline/blocked at a real safepoint, and the evacuation implication with its PERRY_GEN_GC_EVACUATE=0 precedence arm), and gc_instrument_smoke.sh gains three integrated arms gating that the rate knob spans a range (strictly between pressure-only and the rate-1 endpoint) and that the same seed retires exactly the same page-sets. cargo test -p perry-runtime --lib --test-threads=1: 1687 passed, 0 failed, 3 ignored. BREAKING CHANGE: PERRY_GC_ZEAL is removed. PERRY_GC_SCHEDULE_RATE=1 resolves to the always-threshold and selects every handled safepoint, so a seeded run at rate 1 forces an evacuating minor at exactly the points the retired knob did, at 100% density rather than an approximation of it. Keeping both meant two configurations to hold exercised under the GC knob kill-policy, and the pair had grown a precedence rule (zeal won when both were set) whose only job was to stop their live-subject counters from disagreeing. crate::gc::zeal_forced_collections() is replaced by crate::gc::gc_schedule_forced_collections().
e16f010 to
a8df637
Compare
2 participants
What
PERRY_GC_SCHEDULE_SEED=<u64>— collect at a safepoint iff a deterministic pseudo-random function of the seed and a per-thread safepoint ordinal says so, at a density set byPERRY_GC_SCHEDULE_RATE(default0.05). Plusscripts/gc_schedule_fuzz.sh <binary> [seeds], which sweeps seeds and prints a reproduce command per failure.Why
A #7154-class bug is a value live but not rooted across a collection point. Whether it is caught is a property of the GC schedule, not of the bug — so re-running one binary sixty times re-runs one schedule sixty times and explores almost nothing. With zero failures in
Nruns the 95% upper bound on the true rate is only ~3/N: 120 clean runs bound a 1.7% bug at 2.5%, i.e. no evidence at all.Two settings existed. Normal pacing puts collections tens of megabytes apart.
PERRY_GC_ZEAL=1collects at every safepoint — maximum pressure, but one fixed schedule, slow, and timing-distorting enough that it cannot be used on the registry at all (it dies innode-machine-idbefore the interesting code runs). This is the middle, and unlike either it hands back a reproducer.The result that matters
sfw-registry --help(#7291's tree,PERRY_FORCE_WELL_KNOWN=iovalkey, compiled and run withPERRY_GC_MOVING_LOOP_POLLS=1,--debug-symbols) fails ~1 run in 60 in the plain-polls configuration. Same binary, macOS arm64, four runs in parallel:1..12,RATE=0.05Two stable signatures:
The first is the signature the registry hunt has been chasing. Seed 1 was re-run 5/5 and failed every time at the identical site in ≤ 1 s:
The second signature is the
node-machine-idpath that makes zeal unusable here — at 5% density it is reachable without also losing the rest of the program.Cost: a seeded run is ~5–10× slower on this workload, which is why half the sweep is censored rather than passed. Failing seeds cost 1–2 s, so a sweep's wall clock is dominated entirely by the seeds that find nothing.
What the knobs gate, precisely — three effects, two non-effects
PERRY_GC_SCHEDULE_SEEDdoes exactly three things:js_gc_loop_safepointstops requiringGC_SAFEPOINT_PENDINGbefore descending intogc_safepoint_moving_minor— the bypass zeal performs, for the same reason: a schedule cannot select a safepoint the gate already returned from.gc_safepoint_moving_minor, past the entry guards, a per-thread counter advances once per handled safepoint; with nothing due, a minor runs anyway iffsplitmix64(splitmix64(seed) ^ counter) < threshold.gc_force_evacuate_enabled()becomes true, so a scheduled minor MOVES survivors — otherwise the mode would promise relocation stress and deliver sweep pressure (gc: no reachable configuration exercises an evacuating minor with unpinned runtime locals — the #6655/#6935 bug class is untestable #6942/GC testing: PERRY_GC_FORCE_EVACUATE is inert for gc()-driven tests (full mark-sweep + forced conservative scan) — stress claims may be unsupported #6946).It does not bypass the entry guards, and a blocked safepoint deliberately does not tick the counter, so the ordinal sequence tracks the program's safepoints rather than its allocation state. It does not override
PERRY_GEN_GC_EVACUATE=0. It cannot emit loop polls codegen never produced. It never suppresses a pressure-driven collection — the rate is additional density, never less. A value that does not parse as au64reads as OFF, not as seed 0.PERRY_GC_SCHEDULE_RATEgates only the comparison threshold, and is inert without a seed.0is an on-but-selects-nothing control;1is zeal's density.Determinism, precisely scoped — exact single-threaded, per-thread otherwise
The decision reads no wall clock, no address, no thread identity — so a single-threaded program replays a seed exactly. The counter is thread-local, so a
perry/threadprogram gets a deterministic schedule per thread given that thread's own safepoint sequence, but nothing makes the OS schedule that sequence identically twice. A global counter would be strictly worse: it would make even one thread's schedule depend on interleaving. Deterministic for single-threaded programs; per-thread but not run-to-run reproducible for multi-threaded ones.Default off, proven inert — byte-identical traces across five configurations
With no seed set,
PERRY_GC_DIAG=1collector traces are byte-identical to the branch parent across five configurations on two fixtures — 367-line traces under plain polls, 4941 under zeal, 6151 under zeal + from-space protection, plus the no-polls and forced-evacuation arms.The seed is never lost — printed at startup, exit, panic, and fatal signals
Printed at startup, at exit (
[gc-schedule] done: seed=… safepoints=… scheduled_collections=…, from the process-exit teardown funnel — perry's exits call_exit, soatexitalone would miss them), on panic, and from an async-signal-safe handler for SIGSEGV/SIGBUS/SIGABRT/SIGILL/SIGTRAP. That handler chains rather than clobbers, andarena/quarantine.rsre-layers it after installing its own, soPERRY_GC_SCHEDULE_SEED=… PERRY_GC_PROTECT_FROMSPACE=1reports both the seed and the precise fault site.Tests — 11 unit tests, three integrated smoke arms, full-suite runs
gc/tests/schedule.rs, 11 tests, both directions of both knobs: parse (includingu64::MAX + 1,-1,0x10→ OFF), threshold endpoints, 100k-ordinal determinism across five seeds, adjacent-seed divergence, realised density vs requested at four rates, collect / decline / blocked at a real safepoint, and the evacuation implication with itsPERRY_GEN_GC_EVACUATE=0precedence arm.scripts/gc_instrument_smoke.shgains three integrated arms that gate the three claims end to end. Measured on the fixture:pressure-only=0 < seeded(0.25)=989 < zeal=1230— a middle setting, not a second name for an endpoint — and the same seed twice retires989 == 989.cargo test -p perry-runtimeon this branch: 1670 passed, 0 failed (--test-threads=1, two consecutive runs). The branch parent, same machine, same conditions: 1658 passed, 1 failed (pty::…::js_pty_spawn_shell_data_and_exit, a 15 s pty wait that times out under load). The default parallel mode is flaky on both — threeobject::failures on the branch, a different four on the parent, none overlapping — a pre-existing isolation problem, not this change.No collector policy changed. Every scheduled collection runs at a point the collector already treats as a precise-root safepoint; only how often changes.
Summary by CodeRabbit
PERRY_GC_SCHEDULE_SEEDandPERRY_GC_SCHEDULE_RATE.