fix(test-isolation): the GC guards' clear can no longer reach another test's data (#7672) - #7674
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (19)
📝 WalkthroughWalkthroughThe runtime adds test-only per-thread storage for GC-cleared global side tables. Runtime registries use the new declaration macro. Cross-thread isolation tests and a source audit validate coverage. CI runs both checks. ChangesGlobal sink isolation
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TestThread
participant RuntimeTable
participant ClearThread
participant GuardReset
TestThread->>RuntimeTable: install test state
ClearThread->>GuardReset: reset GC test state
GuardReset->>RuntimeTable: clear ClearThread table
TestThread->>RuntimeTable: verify state survives
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/7674-guard-cleared-global-sinks.md`:
- Around line 37-40: Reconcile the “23 tables converted” headline with the
category counts in the listed breakdown, which sum to 32. Update either the
headline or the individual category counts so the documented total and breakdown
agree.
In `@crates/perry-runtime/src/gc/tests/global_sink_isolation.rs`:
- Around line 293-301: Update the coverage assertions around
COVERED_CLEAR_HELPERS to include the console-reset isolation probe before
validating helper coverage, since reset_copying_nursery_runtime_test_state
already invokes builtins::test_set_console_log_singleton(0). Adjust the
assertion wording or filtering so it covers this non-test_clear helper without
claiming the check is limited to test_clear_* helpers.
In `@crates/perry-runtime/src/gc/tests/support.rs`:
- Line 355: Keep reset_copying_nursery_runtime_test_state private and add a
sibling-visible wrapper that acquires copying_nursery_isolation_lock() before
invoking it. Update foreign_guard_clear to call the locked wrapper, while
ensuring the probe thread remains unlocked before spawning so it continues
testing an unlocked reader.
In `@scripts/global_sink_isolation.py`:
- Around line 196-205: Update the helper expansion around find_fn_body to
recursively follow same-file helper calls until no new bodies are found,
tracking visited function names to prevent cycles and retaining the existing
accessor filtering as appropriate. Ensure identifiers from every reachable
wrapper are included in idents, and add a self-test covering BARE_TABLE accessed
through a two-hop wrapper.
🪄 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: aebe5c2c-d9f9-462b-9fac-da6c6160fbaa
📒 Files selected for processing (19)
.github/workflows/test.ymlchangelog.d/7674-guard-cleared-global-sinks.mdcrates/perry-runtime/src/builtins/console.rscrates/perry-runtime/src/closure/dynamic_props.rscrates/perry-runtime/src/gc/tests/global_sink_isolation.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/support.rscrates/perry-runtime/src/geisterhand_registry.rscrates/perry-runtime/src/guard_cleared_global.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/object/class_registry/prototype_methods.rscrates/perry-runtime/src/object/class_registry/prototype_objects.rscrates/perry-runtime/src/object/class_registry/state.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/symbol.rscrates/perry-runtime/src/symbol/accessors.rscrates/perry-runtime/src/timer.rscrates/perry-runtime/src/ui_text_registry.rsscripts/global_sink_isolation.py
| const COVERED_CLEAR_HELPERS: &[&str] = &[ | ||
| "test_clear_closure_side_tables", | ||
| "test_clear_symbol_side_table_roots", | ||
| "test_clear_all_timer_scanner_roots", | ||
| "test_clear_class_side_table_roots", | ||
| "test_clear_object_cache_roots", | ||
| "test_clear_geisterhand_roots", | ||
| "test_clear_ui_text_registry_roots", | ||
| ]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/perry-runtime/src/builtins/console.rs --items all
rg -n -C 4 'test_set_console_log_singleton|test_.*console|CONSOLE.*SINGLETON|console.*singleton' \
crates/perry-runtime/src/builtins/console.rs \
crates/perry-runtime/src/gc/testsRepository: PerryTS/perry
Length of output: 22422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the coverage list and the reset helper it is meant to cover.
sed -n '260,305p' crates/perry-runtime/src/gc/tests/global_sink_isolation.rs
sed -n '340,390p' crates/perry-runtime/src/gc/tests/support.rs
# Check whether global_sink_isolation test has a console-related foreign-clear test
rg -n -C 4 'console|console_log_singleton|COVERED_CLEAR_HELPERS|covered_clear' crates/perry-runtime/src/gc/tests/global_sink_isolation.rs crates/perry-runtime/src/gc/tests/support.rsRepository: PerryTS/perry
Length of output: 8024
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the actual probe implementation for covered clear helpers.
fd -a 'global_sink_isolation.rs|support.rs' crates/perry-runtime/src/gc/tests | sed 's#^\./##'
rg -n 'survives_a_foreign_clear|test_clear_|FOREIGN_CLEAR|covered' crates/perry-runtime/src/gc/tests/global_sink_isolation.rs crates/perry-runtime/src/gc/tests/support.rsRepository: PerryTS/perry
Length of output: 5298
Move the console-reset isolation check into the covered helper assertions.
COVERED_CLEAR_HELPERS now documents the helpers this file claims to cover, but builtins::test_set_console_log_singleton(0) is already called by reset_copying_nursery_runtime_test_state. Add the console probe before asserting it and adjust the check text if it is no longer limited to test_clear_* helpers.
🤖 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/global_sink_isolation.rs` around lines 293
- 301, Update the coverage assertions around COVERED_CLEAR_HELPERS to include
the console-reset isolation probe before validating helper coverage, since
reset_copying_nursery_runtime_test_state already invokes
builtins::test_set_console_log_singleton(0). Adjust the assertion wording or
filtering so it covers this non-test_clear helper without claiming the check is
limited to test_clear_* helpers.
| } | ||
|
|
||
| fn reset_copying_nursery_runtime_test_state() { | ||
| pub(super) fn reset_copying_nursery_runtime_test_state() { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the reset lock precondition inside the exported API.
This function assumes that the caller owns copying_nursery_isolation_lock(). crates/perry-runtime/src/gc/tests/global_sink_isolation.rs calls it from foreign_guard_clear without that lock. The direct call can race a real guard while resetting process-global test state.
Keep this reset function private. Expose a sibling-visible wrapper that takes the same lock before it calls this function. Use that wrapper from foreign_guard_clear. The probe thread must not take the lock before spawning, so the test still exercises an unlocked reader.
Proposed fix
-fn reset_copying_nursery_runtime_test_state() {
+fn reset_copying_nursery_runtime_test_state() {
// existing reset body
}
+
+pub(super) fn reset_copying_nursery_runtime_test_state_with_lock() {
+ let _lock = copying_nursery_isolation_lock();
+ reset_copying_nursery_runtime_test_state();
+}Based on supplied test context, foreign_guard_clear invokes this function without the guard lock.
🤖 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/support.rs` at line 355, Keep
reset_copying_nursery_runtime_test_state private and add a sibling-visible
wrapper that acquires copying_nursery_isolation_lock() before invoking it.
Update foreign_guard_clear to call the locked wrapper, while ensuring the probe
thread remains unlocked before spawning so it continues testing an unlocked
reader.
… side tables (#7672) Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
… test's data (#7672) Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…the macro is renamed for what it does (#7672) A 22-run soak of the first fix produced two reds, both the same class and both diagnosed from the wrong VALUE rather than the timing: * `gc::barrier::GENERATED_WRITE_BARRIERS_EMITTED` is owned by TWO guards under TWO DIFFERENT locks and read by tests holding neither, so `GeneratedWriteBarrierTestGuard::inactive()` on one thread silences another thread's runtime barrier. `sabotaged_parent_gate_strands_a_ young_child_the_shipped_gate_keeps` failed with `missing_edges=1 ... slot_page_ever_dirty=false` — the barrier did not fire. * `tui::tree`'s `NEXT_HANDLE` / `REGISTRY` have no clear and no lock, and `register_increments_handle` asserts `h2 == h1 + 1`. Neither is reached by a `test_clear_*` helper, so the macro is renamed `per_test_global!` for what it does rather than for the sharpest instance of the hazard, and the lint gate now audits the guards' own module as well as the clear list. Two ways the gate could have gone quiet, both closed and both self-tested: the `per_test_global!(...)` paren form was invisible to a `{`-only matcher (silently taking the three timer tables to "no static storage"), and a matcher that stops matching now trips a classified-statics floor instead of reporting a clean tree. A `Mutex<()>` serializer is classified as a lock, never a hazard — making one per-thread would turn it into a no-op. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
61cf886 to
7993f67
Compare
Audit — merging as v0.5.1387You were right and my recommendation was wrong, for the reason you gave: "readers get a cheap assertion" cannot satisfy my own second requirement, because the assertion is an opt-in on the same unbounded reader population. I proposed detection where prevention was available. Partitioning the storage removes the class; observing the clear only tells you after the fact, and only for readers who remembered to ask. The three properties that make it the right shape:
Sabotage verified independently: reverting the Finding five instances, two of them by soaking your own fix
The intermittency evidence is the part I'd hold up as the model25/25 green — "a false all-clear" — then 20/22, which is what found the two extra instances. Naming the first soak as a false all-clear rather than banking it is the whole discipline. ~102 runs across four soaks. And the honesty on #7683: 1 in ~102 on the branch vs 0 in 25 on Two more#7018 closed by refutation — doesn't reproduce in 20 runs, and structurally cannot: All four arms in Your harness nearly lying twice — counting an external SIGTERM as a product flake, then SIGSEGV as an external kill — and your lint gate silently losing three tables to the Gates: 24/24 lint (the extraction is 24 now, up from 22), fmt clean, |
1 participant
Closes #7672.
The class, and why the three fixes so far did not close it
gc::tests::support::reset_copying_nursery_runtime_test_state()runs fromGcTestIsolationGuardandCopyingNurseryTestGuard, on whatever libtest thread happens to construct one, and calls 20test_clear_*helpers that empty process-global side tables. The guards serialize against each other, and against the handful of tests that remember to takecrate::gc::global_side_table_test_lock(). Nothing requires a reader to take it, so the defence is opt-in and the opt-in is invisible at the read site.opt_report's row sinkrows.len() == 2failing at 3ext_registry'sUSED_PROVIDERSioredispresentclosure'sCLOSURE_PROPSTAG_UNDEFINEDEach was exposed by an unrelated PR that changed the parallel schedule, never introduced by it, so the exposing author paid the diagnosis and the natural conclusion ("my change broke something") was wrong.
What this changes: storage, not locking
A lock cannot close this. The damage window is "between this test's write and this test's read", and only the test knows that span. An accessor that takes a shared lock for one call does not cover it. A lock that covers it has to be taken by the test — which is the opt-in the class is made of, on a reader population the issue counts at ~180 and growing.
So the storage moves.
per_test_global!(crates/perry-runtime/src/per_test_global.rs) expands to:staticit replaced, byte for byte.test_clear_*is#[cfg(test)]and never runs in a shipped runtime, so the hazard and the fix are both confined to tests.PerThread<T>that derefs to a per-thread instance. libtest runs one thread per test, so "per thread" and "per test" coincide, and a guard on thread U empties U's instance, which already holds only U's entries.That is exactly the isolation the clear was reaching for. Call sites do not change —
SYMBOL_REGISTRY.lock()andCLASS_PROTOTYPE_OBJECTS.read()keep working throughDeref, so this is a declaration-site change and not a 300-site rewrite.37 statics converted across 13 files (26 at first, plus the class-registry and soak-found ones), covering every family the guards clear: closure dynamic props (3), symbol side tables (5), the class-registry
RwLocks (7), the timer queues (3), the object-constant caches andGLOBAL_THIS_PTR(9),geisterhand(2),ui_text_registry(2), theconsole.logsingleton.What holds the line, and how each half is shown able to fail
scripts/global_sink_isolation.py, inlint(a required context; ~1s, no compiler). It derives the clear list fromreset_copying_nursery_runtime_test_state's own source, follows one level of same-file accessors (test_clear_closure_side_tablesnames no static at all — it goes throughget_closure_props()), resolves each identifier in its own module first, and classifies the storage asthread_local!/per_test_global!/Mutex<()>lock / barestatic. A bare static fails unless allowlisted with an issue, and an allowlist entry that matches nothing also fails. The burden lands on the ~20 table authors, who are finite and gated, instead of on the readers, who are not.Its
--self-testhas 9 checks: a bare table is rejected, a converted one and a thread-local one are not, allowlisting silences exactly one entry, a stale entry fails, a renamed or empty reset function raises instead of passing vacuously, and the parsers are run against the real tree (≥10 helpers parsed,CLOSURE_PROPSclassifiedper_test,ARGUMENTS_OBJECTSclassifiedthread_local) so a regex that stops matching cannot make the gate green.gc::tests::global_sink_isolation, 9 tests. Each plants the #7671 shape — write on this thread, run the guards' real clear on another thread, read back — for onetest_clear_*helper. They deliberately do not take the global lock: taking it would test the opt-in rather than the isolation. Every probe asserts its subject was live before the clear, so "survived" is never confused with "was never installed".the_probe_catches_a_bare_process_global_sinkruns the identical procedure against a canary declared as a plainstaticand requires the wipe to be observed. A green file means the detector works, not that nothing was tried.every_covered_clear_helper_is_still_called_by_the_guardsinclude_str!ssupport.rsat compile time so a probe for a helper that no longer runs cannot sit green forever.Sabotage, measured, not asserted. With the macro's
#[cfg(test)]arm reverted to the pre-#7672 bare static — one edit, every table at once — 7 of 9 tests fail, each with the message naming the cause:(
error[count 0 andRunning unittestspresent on that run — the sabotage compiled and executed rather than failing to build.)Allowlist: 4 entries, each with a reason
YOUNG_PIN_EVER(perf(gc): the copying minor traverses the young graph twice — the eligibility preflight is 22% of json_pipeline's hot phase #7645) — deliberately process-wide and monotone; the skip path is masked if it is not. No reader outside the guard.SYMBOL_REGISTRY,WELL_KNOWN_SYMBOLS(Process-global side tables are cleared by test guards that readers are not required to take — three flakes in two days #7672) — read, never written by the clear: they are the process-lifetime registries the per-threadSYMBOL_POINTERSrebuild is derived from, and their symbols areBox::leaked, so process-wide identity is the correct one (Symbol.for("x") === Symbol.for("x")depends on it).plugin::REGISTRY(Process-global side tables are cleared by test guards that readers are not required to take — three flakes in two days #7672) — the one table already defended soundly, and the model the survey pointed at: the guard takesPLUGIN_REGISTRY_TEST_LOCKand all 9 plugin tests take the same lock. The split lock domain is the root cause in all three fixed flakes; this domain is not split.Validation
cargo test -p perry-runtime --lib --no-fail-fast— 25 consecutive runs, 25 green, 0 vacuous (each run checked forRunning unittestsand forerror[before its result was counted). 1926 passed / 0 failed / 4 ignored, ~8.5 s per run; the conversion costs no measurable wall time.cargo check --all-targets— clean.lintgate commands green, including the new one.rustup run stable cargo fmt --all -- --check— clean.timer.rssat three lines under the 2000-line cap, so the macro takes an optional trailing;and the three timer tables are declared on one line each — the file is unchanged at 1997 lines.Also found while surveying (not fixed here)
The same architecture exists outside the guards' clear list, with the same split-lock-domain signature:
async_hooksHOOKS/RESOURCES/NEXT_ASYNC_IDsit under four disjoint serialization domains, andgc/tests/alloc.rs:836takes none of them.resource_ids_are_monotonic_even_without_hooksassertsb.async_id == a.async_id + 1— a wrong-VALUE flake waiting for a schedule change.tui::state::SLOTSis cleared under three different locks;alloc_returns_sequential_handlesassertsh0 == 0, h1 == 1, h2 == 2.agent_dispatch_tests.rs's privateTIMER_QUEUE_TESTSlock protects the timer queues the guards also clear — theopt_report/ext_registryshape exactly.These are candidates for the same treatment; they are called out rather than folded in because none of them is on the guards' clear path, which is what this PR's gate covers.
Update: two more instances, found by soaking this very fix
The first 25-run soak was 25/25 green. A second, 22-run soak produced two reds — which is exactly why the ≥20-run bar exists, and why the first all-clear was not one. Both are the same class, neither is reached by any
test_clear_*helper, and both were diagnosed from the wrong VALUE rather than the timing:gc::barrier::GENERATED_WRITE_BARRIERS_EMITTED— owned by two guards under two different locks (CopyingNurseryTestGuardunder the copying-nursery isolation lock,GeneratedWriteBarrierTestGuardunderGENERATED_BARRIER_TEST_LOCK) and read by every runtime write barrier holding neither.sabotaged_parent_gate_strands_a_young_child_the_shipped_gate_keepsfailed withmissing_edges=1 ... slot_page_ever_dirty=false— the barrier did not fire, because another thread'sGeneratedWriteBarrierTestGuard::inactive()had zeroed the flag mid-test.tui::tree'sNEXT_HANDLE/REGISTRY— no clear, no lock, andregister_increments_handleassertsh2 == h1 + 1plus an exact registry length.Both are converted, both have a dedicated regression test, and both fail under the same one-edit sabotage (now 9 of 11 tests red).
The macro is renamed
per_test_global!— for what it does, not for the sharpest instance of what it defends against, since two residents are cleared by nothing. Naming it after the clear would have made these two look like misfits rather than the same defect. The gate now audits the guards' own module alongside the clear list, which is what makes the write-barrier flag visible to it at all.Two ways this gate could itself have gone quiet — both closed, both self-tested
per_test_global!(...)paren form (used intimer.rsto stay under the 2000-line cap) was invisible to a{-only matcher, silently taking the three timer tables to(no static storage). A gate that stops matching reports zero hazards and exits 0 — indistinguishable from a clean tree. Reverting the delimiter fix now fails a self-test case by name.Mutex<()>serializer is classified as a lock, never a hazard: making one per-thread would turn it into a no-op — the opposite of the fix.Self-test is now 15 checks, allowlist 5 entries each with a reason.