docs: perry-runtime's tests must run single-threaded locally, as CI already does - #7791
Conversation
15a5163 to
1d06cbf
Compare
|
Warning Review limit reached
Next review available in: 13 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 (2)
📝 WalkthroughWalkthroughThe test instructions now run ChangesRuntime test isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@CLAUDE.md`:
- Around line 85-86: Remove the added test commands and runtime guidance from
CLAUDE.md, including the content around the shown cargo test commands and the
related section near line 241. Keep CLAUDE.md limited to the permitted Current
Version update, or move the guidance to an approved documentation file if it
must be retained.
🪄 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: 4b9a8904-e50e-4dc0-806c-8e74aabd94c8
📒 Files selected for processing (2)
CLAUDE.mdchangelog.d/7791-perry-runtime-tests-single-threaded.md
| RUST_TEST_THREADS=1 cargo test --release -p perry-runtime # MUST be single-threaded (see below) | ||
| cargo test --release --workspace --exclude perry-runtime \ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CLAUDE.md line count and relevant lines =="
wc -l CLAUDE.md
sed -n '75,90p' CLAUDE.md
sed -n '235,245p' CLAUDE.md
echo
echo "== Check for current guideline in repository/config files =="
rg -n "supplied coding guideline|CLAUDE.md|Current Version|changes only|allowed documentation" -S .Repository: PerryTS/perry
Length of output: 50372
Move CLAUDE.md guidance out of the change.
Coding guidelines allow CLAUDE.md to update only **Current Version:**. Lines 85-86 and 241 add test commands and runtime guidance instead, so get an approved exception or move this content to an allowed documentation file.
🤖 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 `@CLAUDE.md` around lines 85 - 86, Remove the added test commands and runtime
guidance from CLAUDE.md, including the content around the shown cargo test
commands and the related section near line 241. Keep CLAUDE.md limited to the
permitted Current Version update, or move the guidance to an approved
documentation file if it must be retained.
Source: Coding guidelines
1d06cbf to
2603ebf
Compare
…lready does `gc::tests::root_words::bare_address_in_shadow_slot_survives_a_real_collection` was reported failing on a clean c2a96b6 at load ~111 and investigated as a possible load-dependent GC bug. It is not one. perry-runtime's tests share process-global side tables. test.yml has pinned RUST_TEST_THREADS=1 for this crate since #1444; global_sink_isolation's header records ~180 readers that still do not take the clearing lock (#7672 converts them one at a time). CLAUDE.md documented `cargo test --release --workspace`, which does not pin the thread count and therefore runs this crate in exactly the configuration CI exists to avoid. Measured against 2e5bf44 in release: 600/600 full-suite runs clean at --test-threads=16 (load ~90); 2 failures in 320 at --test-threads=64 (load ~115), in proxy::...numeric_write_guard... and element_shape matrix_delete_revokes. Both are the crate's own vacuity guards firing on a destroyed fixture precondition, not a collector invariant. root_words itself never failed across 920 parallel full-suite runs, 1500 targeted pairings, or a 40-seed PERRY_GC_SCHEDULE_SEED sweep. No test is ignored and no retry is added; the unsupported configuration simply stops being documented. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
2603ebf to
2365dfc
Compare
|
Audit complete — merging. Facts verified against the tree: This session hit the same class four times today (gate reds at load 26–45 that re-ran clean), so the operational value is immediate. Bumped to 0.5.1461; gate 19/19 green (docs-only diff). |
1 participant
What
CLAUDE.md documented
cargo test --release --workspace …for local runs. That command does not pinRUST_TEST_THREADS, so it runsperry-runtimemulti-threaded — the exact configuration every CI path deliberately avoids. This splits the documented command soperry-runtimerunsRUST_TEST_THREADS=1, and records the hazard under "CI gates that surprise people".Why — the investigation behind it
gc::tests::root_words::bare_address_in_shadow_slot_survives_a_real_collectionwas reported failing on a cleanc2a96b638with the box at load ~111, passing in isolation and passing three full-suite reruns on a lightly loaded box. It was handed to me as "either make it deterministic, or it's a real load-dependent GC bug — and if so that outranks everything."It is not a GC bug.
perry-runtime's tests share process-global side tables.test.ymlhas pinnedRUST_TEST_THREADS=1for this crate since #1444 (its comment: the default pool "races the GC/threading tests into intermittent SIGSEGV"), andgc::tests::global_sink_isolation's header records that ~180 readers still do not take the clearing lock, with #7672 converting them toper_test_global!one table at a time. The local command was the one place that still walked into it.Evidence
Against
2e5bf4434, release, on a tree that includes #7317:--test-threads=16, load ~90--test-threads=64, load ~115The two failures were
proxy::tests::object_array_numeric_write_guard_requires_complete_uniform_proofandarray::element_shape::matrix_tests::matrix_delete_revokes— neither isroot_words. The failing test is simply whichever one loses the race.The decisive detail is what they assert when they fail. Both are the crate's own vacuity guards:
fixture must start proven, or every verdict below is vacuousone-field loops should publish one non-zero 16-bit laneThat is a test discovering its own precondition was destroyed by a concurrent thread — not a collector invariant being violated. A real GC bug would surface as a swept-while-live object, not as a fixture that never armed.
root_wordsitself did not fail once across:root_words+global_sink_isolationpairings at--test-threads=16,PERRY_GC_SCHEDULE_SEEDsweep at rate 25% — it never appears among that sweep's failures.Deliberately not done
No
#[ignore], no retry wrapper. Both would hide which of the two possibilities this was. The fix is to stop documenting a configuration the repo already knows is unsupported.Side note recorded in the changelog fragment
PERRY_GC_SCHEDULE_SEEDis a whole-process env var andscripts/gc_schedule_fuzz.shtakes a compiled binary, not the unit-test suite. Setting it acrosscargo testfails 8 GC tests at every seed, several of which assert the unset behaviour outright (schedule::unset_is_inert_for_evacuation_policy). That is the tool used outside its contract, not a defect — written down so the next person doesn't re-derive it.Validation
cargo fmt --all -- --checkandscripts/check_file_size.shboth clean. Docs-only change; no code paths touched.Summary by CodeRabbit