test(runtime): stop five process-globals leaking across libtest threads (#7946) - #7954
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe PR isolates runtime test state by using thread-local overrides and per-test globals, extracts deterministic scan-mode resolution, separates poll-counter logic from global atomics, and narrows incremental-mark veto checks to the current thread. It also adds promotion diagnostics and concurrency coverage. ChangesRuntime test isolation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ 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 |
This was referenced Aug 12, 2026
…ds (#7946) `cargo test -p perry-runtime` failed 24 runs in 100 on `main`, one or two tests per run and a different test each time. Every one of them was a PROCESS-global that one test moved and another read: libtest gives each test its own thread, so a `thread_local!` is per-test and a bare `static` is not, and the GC's test locks are opt-in on the writer side only. Measured on `origin/main` @ 1bd5eeb, release profile, 100 runs: 8 promote_in_place::an_untraced_promotion_indexes… cycles=0, objects=0 7 layout_trace::element_shape::…survives_a_copying_minor proof read back 0 3 promote_in_place::a_promoting_cycle_still_measures… should_promote_young_in_place() 2 layout_trace::element_shape::…keeps_growing_after… 2 root_words::bare_address_in_global_root_survives… step status 3 (SKIPPED) 2 poll_arm::arm_events_count_arms_and_the_word_is_reported 1 root_words::bare_address_in_shadow_slot_survives… 1 poll_arm::arm_and_disarm_are_a_counter_not_a_flag 1 promote_in_place::dead_byte_budget_stops_promotion… 1 promote_in_place::untraced_budget_forces_a_measuring_cycle… 1 runtime_roots::prototype_addr_cache::…forwarding_stub Five causes, each fixed where the codebase already had an answer for it: * `CLASS_SHAPE_GENERATION` / `ELEMENT_SHAPE_EPOCH` / `ELEMENT_SHAPE_PROOF_SEQ` become `per_test_global!`. `ELEMENT_SHAPES` was already thread-local, so a process-wide generation was only ever reachable by another TEST — and `invalidate_all_element_shapes()` is called by any prototype-method write anywhere in the crate. * `gc::tests::support::EnvVarGuard` is deleted. It took `std::env::set_var` under a mutex, which serialized the twelve tests that SET a knob and did nothing for the ~2 200 that read one; `PERRY_GC_FORCE_EVACUATE=1` is an input to `should_promote_young_in_place()`, so it turned in-place promotion off underneath the `promote_in_place` policy cases. `PERRY_GC_FORCE_EVACUATE` and `PERRY_GC_VERIFY_EVACUATION` get per-thread overrides next to their readers; the two `PERRY_CONSERVATIVE_STACK_SCAN` cases are about a precedence RULE, so the rule becomes a pure function taking the env answer as an argument. * `gc_blocked_by_unsafe_zone()` gets a per-thread test pin. `GC_UNSAFE_ZONES` is process-wide by design (a worker thread's unscannable stack must stop the main thread), which under `cargo test` made `store(1)` a global stop-the-collector: every concurrent budgeted cycle came back SKIPPED. * `poll_arm`'s arm/disarm algebra moves onto a `(&AtomicU32, &AtomicU64)` pair the tests own privately. `arm_poll()` is reached from `ScheduleGuard::set` on any thread, so exact-count assertions on the shipped word were assertions about the whole binary. A new read-only `the_statics_are_wired_to_the_shared_ algebra` keeps the shipped statics covered. * The untraced-promotion veto asks whether an incremental mark cycle is running ON THIS THREAD instead of "anywhere". The global count is the deliberately conservative cross-thread approximation the write barrier's fast path wants — a false positive there costs one call that returns; here it cost a policy decision, and it is unfounded, because arenas, birth flags and the barrier's `valid_ptrs` pointer are all per-thread. Single-threaded the two are identical; with `perry/thread` agents the old form let one agent's cycle disable another agent's promotion policy. `untraced_promotion_instrument_veto()` now returns the NAME of the armed instrument and the cycle records why it declined, so `cycles=0, objects=0` says which input moved. That is what identified the last cause: 21 of 21 recorded failures read `incremental_mark_in_progress`. Verification, same box and same load throughout, libtest default parallelism: baseline (origin/main @ 1bd5eeb) 100 runs 24 failed 11 tests baseline, --skip the 12 EnvVarGuard 100 runs 23 failed should_promote_ young_in_place() family 5 -> 0, rest unchanged first four fixes 200 runs 25 failed ONE test, and the new diagnostic read incremental_mark_in_ progress 21 of 21 all five fixes 300 runs 1 failed telemetry_verifier, a WALL-CLOCK bound The one remaining failure is `telemetry_verifier::allocation_heavy_arena_debt_ reports_budgeted_steps_and_debt` asserting `pause_steps[22] elapsed 4936us exceeded soft target 2000us` on a box at load 60. It fired 2 runs in 100 on the UNMODIFIED binary too, so it is pre-existing and orthogonal; filed as #7956. Excluding it: 0 failures in 300 runs, against 24 in 100 before. The new `another_agents_incremental_cycle_does_not_veto_this_threads_untraced_ promotion` is sabotage-verified: restoring the global read fails it with `left: Some("incremental_mark_in_progress"), right: None`. It asserts both directions, so a veto that never fires would fail it too. Not fixed, filed instead: `runtime_roots::prototype_addr_cache::prototype_addr_ reads_through_a_forwarding_stub` (1 run in 100 at baseline; did not reproduce in the 300-run arm, but the cause is structural), #7955. Its cache genuinely wants to be per-thread — it holds a raw address into a thread-local arena — but `per_test_global!` there deterministically broke two `dead_owner_side_tables` prune cases, 200 runs out of 200, and added four intermittent `promise::native_async` failures: materialisation is lazy, so the first read on a thread runs the whole `globalThis` bootstrap, and that read happens from inside a GC root scan. Reverted rather than shipped half-understood.
proggeramlug
force-pushed
the
test/7946-runtime-suite-flakes
branch
from
August 12, 2026 12:04
46bbb65 to
35cdf33
Compare
proggeramlug
marked this pull request as ready for review
August 12, 2026 12:05
proggeramlug
pushed a commit
that referenced
this pull request
Aug 12, 2026
This was referenced Aug 12, 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
1 participant
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.
Closes #7946.
cargo test -p perry-runtimefailed 24 runs in 100 onmain, one or two tests per run and a different test each time. CI runscargo test --release --workspace, so this landed on whoever was merging.The failure set (baseline,
origin/main@1bd5eeb6b, release, 100 runs)promote_in_place::an_untraced_promotion_indexes_the_objects_it_could_not_prove_livecycles=0, objects=0layout_trace::element_shape::…survives_a_copying_minor0promote_in_place::a_promoting_cycle_still_measures_so_the_predictor_cannot_go_staleassertion failed: should_promote_young_in_place()layout_trace::element_shape::…keeps_growing_after_a_copying_minor0root_words::bare_address_in_global_root_survives_a_real_collectionbudgeted GC cycle stopped before completion: status 3poll_arm::arm_events_count_arms_and_the_word_is_reportedleft: 3, right: 1root_words::bare_address_in_shadow_slot_survives_a_real_collectionpoll_arm::arm_and_disarm_are_a_counter_not_a_flagassertion failed: !poll_armed()promote_in_place::dead_byte_budget_stops_promotion_until_a_full_reclaimsshould_promote_young_in_place()promote_in_place::untraced_budget_forces_a_measuring_cycle_and_a_measurement_clears_itruntime_roots::prototype_addr_cache::prototype_addr_reads_through_a_forwarding_stubThe shared cause
Every one is a process-global that one test moves and another reads. libtest runs one thread per test, so a
thread_local!is per-test and a barestaticis not — and each of the GC's test locks (copying_nursery_isolation_lock,ELEMENT_SHAPE_TEST_LOCK,ENV_VAR_TEST_LOCK,LOCK_SAFE_RUNTIME_SCANNER_TEST_LOCK) is taken by the writer only. That is the defect classper_test_global!was introduced for in #7672; these are five more instances.CLASS_SHAPE_GENERATION/ELEMENT_SHAPE_EPOCHinvalidate_all_element_shapes()— any prototype-method write in the crateelement_shapecases (record.generation != class_shape_generation()retires a proof this thread just established)EnvVarGuard(12 sites, 3 knobs)PERRY_GC_FORCE_EVACUATE=1gc_force_evacuate_enabled()is an input toshould_promote_young_in_place()→ the threepromote_in_placepolicy casesGC_UNSAFE_ZONESgc::tests::roots'sGcUnsafeZoneResetGuardgc_budgeted_start_blocked()on every thread →complete_budgeted_gc_cycle()returnsSKIPPEDPERRY_GC_POLL_ARMED/ARM_EVENTSarm_poll(), fromScheduleGuard::seton any threadpoll_armcases, which assert exact countsPERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNTan_untraced_promotion…What changed
per_test_global!.ELEMENT_SHAPESwas already thread-local, so a process-wide generation was never reachable by a legitimate cross-thread read — only by another test. A product build gets the plainstaticback, byte for byte.EnvVarGuarddeleted. It tookstd::env::set_varunder a mutex, which serialized the twelve setters and did nothing for the ~2 200 readers.PERRY_GC_FORCE_EVACUATE/PERRY_GC_VERIFY_EVACUATIONget per-thread overrides next to their readers — the same shape asbarrier_arming'sTEST_ARMED_OVERRIDE,oldgen_defrag'sOLD_DEFRAG_TEST_OVERRIDE,roots'sCONSERVATIVE_STACK_SCAN_OVERRIDE. The twoPERRY_CONSERVATIVE_STACK_SCANcases are about a precedence rule, so the rule becomes a pure function taking the env answer as an argument (theparse_promote_in_placeidiom).gc_blocked_by_unsafe_zone()gets a per-thread test pin.GC_UNSAFE_ZONESis process-wide by design; undercargo teststore(1)was a global stop-the-collector.poll_arm's algebra moves onto a(&AtomicU32, &AtomicU64)pair the tests own privately, plus a new read-onlythe_statics_are_wired_to_the_shared_algebraso the shipped statics do not go dark.valid_ptrspointer are all per-thread. Single-threaded the two are identical (incremental_mark_barrier_active()short-circuits on the same global load); withperry/threadagents the old form let one agent's cycle disable another agent's promotion policy.untraced_promotion_instrument_veto()now returns the name of the armed instrument and the cycle records why it declined, socycles=0, objects=0says which input moved. That is what identified the last cause — 21 of 21 recorded failures readincremental_mark_in_progress.How it was verified
Same box, same load, same binary path, libtest default parallelism throughout.
origin/main@1bd5eeb6b)--skipthe twelveEnvVarGuardtestsshould_promote_young_in_place()family went 5 → 0; everything else unchanged — so the environment is exactly and only that groupincremental_mark_in_progresson 21 of 21 recorded failurestelemetry_verifier::allocation_heavy_arena_debt_reports_budgeted_steps_and_debtThat last one is a wall-clock assertion —
pause_steps[22] elapsed 4936us exceeded soft target 2000uson a box at load 60 — and it fired 2 runs in 100 on the unmodified binary too, so it is pre-existing and orthogonal (filed as #7956). Excluding it: 0 failures in 300 runs, against 24 in 100 before.A full
cargo test --release -p perry-runtimeis green (2 213 passed, 4 ignored, doctests included).Sabotage check. Restoring
!incremental_mark_barrier_globally_idle()fails the newanother_agents_incremental_cycle_does_not_veto_this_threads_untraced_promotionwithleft: Some("incremental_mark_in_progress"), right: None. The test asserts both directions — a foreign arm must not veto, this thread's own barrier must — so a veto that never fires would fail it too, and it asserts the global count actually reads as armed before concluding anything.cargo fmt --all -- --check,scripts/check_file_size.sh,scripts/global_sink_isolation.pyandscripts/gc_runtime_root_holders.pyall pass.Deliberately not in this PR
runtime_roots::prototype_addr_cache::prototype_addr_reads_through_a_forwarding_stub— 1 run in 100 at baseline; it did not reproduce in the 300-run arm, but the cause is structural rather than fixed, so it is filed as #7955 rather than declared gone. Its cache genuinely wants to be per-thread — it holds a raw address into a thread-local arena — butper_test_global!there deterministically broke twodead_owner_side_tablesprune cases (200 runs out of 200) and added four intermittentpromise::native_asyncfailures: materialisation is lazy, so the first read on a thread runs the wholeglobalThisbootstrap, and that read happens from inside a GC root scan. Reverted rather than shipped half-understood.Working notes, including the intermediate arms:
gc-handoff/FLAKY-NOTES.md.Summary by CodeRabbit
Bug Fixes
Documentation