fix(test-isolation): close the async_hooks / tui::state / timer-queue split-lock gaps - #7710
Conversation
|
Warning Review limit reached
Next review available in: 11 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 ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change isolates async-hook and TUI test state with ChangesRuntime test isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/perry-runtime/src/per_test_global.rs`:
- Around line 123-156: Replace the raw usize contract in PerThread::shared_key
and PerThread::adopt with an opaque, type-parameterized SharedKey<T> whose
constructor is private and carries the originating table identity. Update the
per-thread slot mapping and instance lookup to use SharedKey<T>, and have adopt
validate that the key belongs to the receiving PerThread<T> before switching
mappings, rejecting zero, arbitrary addresses, and keys from other tables or
types.
In `@crates/perry-runtime/src/tui/state.rs`:
- Around line 217-256: Update state_slots_survive_a_foreign_clear to set the
primary thread’s STATE_DIRTY flag to true before spawning the clearing thread,
then assert STATE_DIRTY remains true after the foreign thread joins. Keep the
foreign thread’s false store and existing slot-survival assertions unchanged.
🪄 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: 9878057b-4100-494f-8e5f-e7f4308b68b1
📒 Files selected for processing (8)
changelog.d/7710-split-lock-sinks.mdcrates/perry-runtime/src/agent_dispatch_tests.rscrates/perry-runtime/src/async_hooks.rscrates/perry-runtime/src/gc/tests/runtime_roots.rscrates/perry-runtime/src/per_test_global.rscrates/perry-runtime/src/timer.rscrates/perry-runtime/src/timer/test_shared_queues.rscrates/perry-runtime/src/tui/state.rs
| /// This thread's instance of the table, as an opaque key a *different* | ||
| /// thread can hand to [`Self::adopt`] to observe the SAME instance | ||
| /// instead of materializing its own. | ||
| /// | ||
| /// Per-thread isolation exists to keep a table out of reach of an | ||
| /// UNRELATED test's noise (#7672's own guards, running on whatever | ||
| /// libtest thread happens to construct them) — it is not meant to hide a | ||
| /// test's own deliberately-spawned worker thread from data the spawning | ||
| /// thread just wrote. A test whose actual subject is cross-thread | ||
| /// visibility of the SAME table (`agent_dispatch_tests.rs`'s #6185 | ||
| /// coverage: an entry enqueued by one agent, read by a pump acting for | ||
| /// another) needs this escape hatch — without it, the per-thread split | ||
| /// makes such a test's assertions pass regardless of whether the | ||
| /// production filtering logic they exist to exercise is even still | ||
| /// there. Materializes this thread's instance if it has not touched the | ||
| /// table yet, so the key handed out is never dangling. | ||
| pub fn shared_key(&self) -> usize { | ||
| self.instance() as *const T as usize | ||
| } | ||
|
|
||
| /// Make this thread's future accesses to this table resolve to `key` | ||
| /// (obtained from [`Self::shared_key`] on another thread) instead of | ||
| /// materializing this thread's own instance. | ||
| /// | ||
| /// Must run before this thread's first access to the table. `instance()` | ||
| /// re-reads the mapping on every call, so calling this AFTER the table | ||
| /// has already materialized on this thread does not merge the two — it | ||
| /// just switches future lookups to `key`, silently abandoning whatever | ||
| /// this thread had already written to its own (now orphaned) instance. | ||
| pub fn adopt(&self, key: usize) { | ||
| let slot = self as *const Self as usize; | ||
| let _ = SLOTS.try_with(|slots| { | ||
| slots.borrow_mut().insert(slot, key); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 '\.(shared_key|adopt)\s*\(' crates/perry-runtime
rg -n -C 5 'pub fn (shared_key|adopt)' crates/perry-runtime/src/per_test_global.rsRepository: PerryTS/perry
Length of output: 3391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' crates/perry-runtime/src/per_test_global.rs
printf '\n--- tests/shared queue adoption ---\n'
sed -n '1,120p' crates/perry-runtime/src/timer/test_shared_queues.rs
printf '\n--- timer module exports/usages ---\n'
rg -n -C 2 'test_shared_queues|test_adopt_queues|test_shared_queue_keys|pub\(crate\) fn (TIMER_QUEUE|CALLBACK_TIMERS|INTERVAL_TIMERS)' crates/perry-runtime/src crates/perry-runtimeRepository: PerryTS/perry
Length of output: 23169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- per_writeback.rs outline ---'
ast-grep outline crates/perry-runtime/src/per_writeback.rs --match PerWriteback --view expanded || true
printf '%s\n' '--- PerWriteback usages ---'
rg -n -C 4 'pub struct PerWriteback|struct PerWriteback|PerWriteback|per_writeback!' crates/perry-runtime/src/per_writeback.rs crates/perry-runtime/src
printf '%s\n' '--- writeback tests ---'
fd -a '.*write.*|.*writeback.*' crates/perry-runtime/src | sed 's#^\./##' | head -50Repository: PerryTS/perry
Length of output: 401
Make per-thread adoption type-safe and table-specific.
adopt accepts any usize. A caller can pass 0, an arbitrary address, or a key from PerThread<U>, and instance() then treats that value as a *const T. Define an opaque SharedKey<T> whose constructor is private, store the source table identity in the key, and reject adoption unless the key came from the receiving PerThread<T>.
🤖 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/per_test_global.rs` around lines 123 - 156, Replace
the raw usize contract in PerThread::shared_key and PerThread::adopt with an
opaque, type-parameterized SharedKey<T> whose constructor is private and carries
the originating table identity. Update the per-thread slot mapping and instance
lookup to use SharedKey<T>, and have adopt validate that the key belongs to the
receiving PerThread<T> before switching mappings, rejecting zero, arbitrary
addresses, and keys from other tables or types.
| /// #7680: plants the #7672 shape directly — allocate a slot on THIS | ||
| /// thread, clear `SLOTS` (what all three pre-fix lock domains eventually | ||
| /// did) on ANOTHER thread, and assert the slot survived. Revert the | ||
| /// `per_test_global!` conversion above (back to a bare `static`) and this | ||
| /// fails: a foreign thread's clear empties the slot this thread just | ||
| /// allocated, and a subsequent `js_perry_tui_state_alloc` on this thread | ||
| /// hands out handle `0` again — the exact non-sequential-handle shape | ||
| /// `alloc_returns_sequential_handles` is named for. | ||
| #[test] | ||
| fn state_slots_survive_a_foreign_clear() { | ||
| reset(); | ||
| let h = js_perry_tui_state_alloc(7680.0); | ||
| assert_eq!( | ||
| js_perry_tui_state_get(h).to_bits(), | ||
| 7680.0_f64.to_bits(), | ||
| "the probe installed nothing, so survived-vs-wiped would be vacuous" | ||
| ); | ||
|
|
||
| std::thread::spawn(|| { | ||
| crate::gc::lock_gc_root_registry(&SLOTS).clear(); | ||
| STATE_DIRTY.store(false, Ordering::Release); | ||
| }) | ||
| .join() | ||
| .expect("the clearing thread panicked"); | ||
|
|
||
| assert_eq!( | ||
| js_perry_tui_state_get(h).to_bits(), | ||
| 7680.0_f64.to_bits(), | ||
| "a state slot written on this thread was destroyed by a foreign thread's \ | ||
| clear (#7680). Per-thread storage (`per_test_global!`) is what prevents \ | ||
| this." | ||
| ); | ||
| let h_next = js_perry_tui_state_alloc(1.0); | ||
| assert_eq!( | ||
| h_next, | ||
| h + 1, | ||
| "this thread's slot count must not have been reset by the foreign clear" | ||
| ); | ||
| reset(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert STATE_DIRTY isolation in this regression test.
The primary thread starts with STATE_DIRTY == false. The foreign thread also stores false. This test passes even if STATE_DIRTY remains process-global.
Set the flag to true on the primary thread before spawning the clearer. Assert that it remains true after the foreign clear.
🤖 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/tui/state.rs` around lines 217 - 256, Update
state_slots_survive_a_foreign_clear to set the primary thread’s STATE_DIRTY flag
to true before spawning the clearing thread, then assert STATE_DIRTY remains
true after the foreign thread joins. Keep the foreign thread’s false store and
existing slot-survival assertions unchanged.
… split-lock gaps (#7680) Follow-on to #7672/#7674: three more process-global sinks had the same split-lock-domain signature outside the GC guards' clear list, so #7674's gate never saw them. async_hooks (NEXT_ASYNC_ID, HOOKS_ACTIVE, HOOKS, RESOURCES, GC_DESTROY_QUEUE, NEXT_CONTEXT_SNAPSHOT_ID, CONTEXT_SNAPSHOTS, ASYNC_WRAP_PROVIDERS) and tui::state (SLOTS, STATE_DIRTY) sat behind four and three disjoint test lock domains respectively, with reads that took none of them. Both are converted to per_test_global!, which removes the need for any lock at all; the now-redundant private locks (this module's own async_hooks TEST_LOCK, gc/tests/runtime_roots.rs's ASYNC_HOOK_RUNTIME_TEST_LOCK, tui/state.rs's TEST_LOCK) are deleted. agent_dispatch_tests.rs is a different shape: its timer queues were already per_test_global (#7674), so its private TIMER_QUEUE_TESTS lock is unified with the GC guards' shared crate::gc::global_side_table_test_lock() per the issue's ask, but the more serious finding is that per-thread storage had silently made two of its five #6185 cross-agent tests vacuous — a spawned "worker" thread got its own empty queue by construction, so the tests passed whether or not crate::agent::owns filtering still worked. Confirmed by sabotaging owns() to always return true: only 2 of 5 tests caught it before this fix, 4 of 5 after. Fixed by adding PerThread::shared_key/adopt (per_test_global.rs) so a test whose actual subject is cross-thread visibility can opt a spawned thread into the SAME queue instance instead of its own isolated one. Each conversion carries a deterministic regression test (write on one thread, clear/read on another) verified to fail against the pre-fix shape.
403a44a to
1400d5a
Compare
Merging as v0.5.1411The best thing in this PR is not in the issue. The third sink was already fixed, and investigating it found something worse. Proved by sabotage rather than argued: forcing The fix is right too: rather than reverting the isolation, The two real sinks
Both got a failing-without-fix demonstration ( Two now-redundant private locks deleted. Gates
|
1 participant
Closes #7680.
Background
Follow-on to #7672/#7674. #7674 converted the ~20 statics on the GC test
guards' clear list (
reset_copying_nursery_runtime_test_state) toper_test_global!and gated the class withscripts/global_sink_isolation.py.#7680 named three more process-global sinks with the same split-lock-domain
signature, all outside that clear list, so the #7674 gate never saw them.
Per-sink verdict
1.
async_hooks(HOOKS,RESOURCES,NEXT_ASYNC_ID, and five morestatics
reset_for_tests()also clears). Real hazard, same shape as thethree flakes #7674 fixed.
reset_for_tests()cleared eight process-globalstatics from whatever thread ran it, under any of four disjoint lock
domains (a private module
TEST_LOCK,AsyncHookRuntimeTestGuard'sprivate
ASYNC_HOOK_RUNTIME_TEST_LOCK, the GC guards' shared lock, or —test_async_hooks_promise_alloc_remains_malloc_tracked— no lock at all).All eight are already reachable by
scan_async_hooks_roots_mut, a scannerregistered in the same file, so
scripts/gc_runtime_root_holders.py'scoverage is unaffected. Converted all eight to
per_test_global!, whichremoves the need for any of the four locks — deleted the now-redundant
TEST_LOCKandASYNC_HOOK_RUNTIME_TEST_LOCK.2.
tui::state::SLOTS/STATE_DIRTY. Same shape: cleared under threedifferent locks (a private
TEST_LOCK,gc/tests/roots.rs'slock_safe_runtime_scanner_test_guard(), and the GC guards' shared lock).SLOTSis already GC-scanned in the same file (scan_state_slot_roots_mut).Converted to
per_test_global!; deleted the privateTEST_LOCK.gc/tests/roots.rsstill takeslock_safe_runtime_scanner_test_guard()—that lock also protects
tui::hooks's still-unconverted slot pool andscanner-registration bookkeeping, unrelated to this table.
3.
agent_dispatch_tests.rs's timer-queue tests. Different shape thanthe issue's framing assumed.
TIMER_QUEUE/CALLBACK_TIMERS/INTERVAL_TIMERSwere alreadyper_test_global!(#7674 counted them amongits 37). Investigating why the issue still called the "unguarded clear" a
live problem turned up something more serious than a lock-domain mismatch:
per-thread storage had silently made two of the five #6185 cross-agent
dispatch tests vacuous. Those tests spawn a "worker" thread and assert it
can neither fire nor see a timer the "primary" thread scheduled — but under
plain
per_test_global!, a freshly spawned thread gets its own emptyqueue by construction, so the assertions hold no matter what
crate::agent::ownsdoes.Confirmed by sabotage: changed
owns()to always returntrue(i.e.disabled the owner-tag filtering these tests exist to cover) and re-ran —
only 2 of the 5 tests failed. The other 2 (
a_worker_neither_fires_nor_eats_a_primary_agent_timer,a_workers_gc_scan_never_visits_another_agents_timer_slots) passed anyway.Fixed with a new escape hatch rather than reverting the per-thread storage
(which would reintroduce the #7672 hazard for the ~180-reader population
elsewhere):
PerThread::shared_key()/PerThread::adopt()(
per_test_global.rs) let a test whose actual subject is cross-threadvisibility opt a deliberately-spawned worker thread into the SAME queue
instance the spawning thread already has, instead of the worker's own
isolated one.
timer/test_shared_queues.rsexposes this for the threetimer statics; the three affected tests capture
test_shared_queue_keys()before spawning and calltest_adopt_queues()as the first thing the worker does. Re-ran the
owns()sabotage after thefix: 4 of 5 tests now fail (the 5th,
a_worker_disowns_the_primary_agents_work,tests
owns()directly and always caught it).Also unified the private
TIMER_QUEUE_TESTSlock withcrate::gc::global_side_table_test_lock()per the issue's ask and theplugin::REGISTRYprecedent it names — the DATA hazard is already closedby
per_test_global!structurally (every libtest test runs on its ownthread), but the adoption mechanism means primary and worker genuinely
share a table for part of the test's run, so serializing against a
concurrent GC guard reset is no longer purely decorative.
Validation
cargo test -p perry-runtime --lib --no-fail-fast: 1946 passed, 0failed (one unrelated pre-existing timing test,
promise::keyed_table::tests::settling_many_keys_is_not_quadratic,flaked under this host's ~70+ load average from concurrent builds by
other agents; confirmed unrelated — untouched file, reproduces in
isolation, passes when run alone at lower load).
python3 scripts/gc_runtime_root_holders.pyand--self-test: clean,identical counts to the pre-change baseline (80 holders / 47 reached / 29
classified) — the conversions don't change what the gate sees.
python3 scripts/global_sink_isolation.pyand--self-test: clean, 0hazards, 93 statics classified, 15/15 self-test checks (these three
sinks are outside that gate's clear-list scope by design, per the
issue title).
cargo fmt --all -- --check: clean.scripts/check_file_size.sh: clean (timer.rsstays at 1999 lines bymoving the new test-only adoption helpers into a submodule rather than
inlining them).
by reverting the corresponding fix (temporarily, then restored) to
confirm it fails with the exact message named in code:
async_hooks::tests::async_hooks_state_survives_a_foreign_reset_for_teststui::state::tests::state_slots_survive_a_foreign_clearagent_dispatch_tests's three timer tests, via theowns()sabotagedescribed above (a repo-level A/B rather than an in-tree regression
test, since the fix is about which thread reads which data, not a
single reset call).
No version bump, no
CLAUDE.md/CHANGELOG.mdedits (external-stylechange per repo convention for this kind of fix).
Summary by CodeRabbit
Bug Fixes
Tests