test(gc): cover every interned string-cache root cell, and trim CLAUDE.md - #7276
Merged
proggeramlug merged 1 commit intoAug 3, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR adds test helpers and GC-root tests for cached ChangesInterned cache GC coverage
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 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 |
jdalton
force-pushed
the
fix/7226-7227-7214-review-followups
branch
from
August 2, 2026 19:55
b2cd049 to
69756be
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: 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 `@CLAUDE.md`:
- Around line 253-254: Shorten the two GC guidance bullets in CLAUDE.md to
retain only the root-store dominance invariant, the requirement to register
runtime raw-pointer caches with gc_register_mutable_root_scanner, and links to
the detailed documentation or changelog fragments. Remove the incident history,
issue numbers, diagnostic commands, worked examples, and other detailed lists,
without modifying the frozen CHANGELOG.md.
In `@crates/perry-runtime/src/gc/tests/runtime_roots/interned_string_caches.rs`:
- Around line 50-58: Update evacuate_string so the from object is rooted before
calling arena_alloc_gc_old and reloaded afterward before set_forwarding_address,
preserving the correct pointer across any collection; alternatively, document
and enforce that this allocation path cannot collect in this test mode.
🪄 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: 942f932f-3a34-410e-b329-830a1f3b6066
📒 Files selected for processing (7)
CLAUDE.mdchangelog.d/7214-closure-calln-stale-registers.mdcrates/perry-runtime/src/builtins/arithmetic.rscrates/perry-runtime/src/gc/tests/runtime_roots.rscrates/perry-runtime/src/gc/tests/runtime_roots/interned_string_caches.rscrates/perry-runtime/src/json/raw_json.rstest-files/test_gap_gc_typeof_string_cache_rooting.ts
…E.md Review follow-ups from PerryTS#7226, PerryTS#7227 and PerryTS#7214, all merged, so they land together against main. The interned `typeof` strings and the `JSON.rawJSON` key are registered GC roots since PerryTS#7226, but nothing tested the registration from the Rust side and the `.ts` gap test drove only six of the eight `typeof` cells. `scan_typeof_string_roots_mut` is eight hand-written `visit(...)` calls, so `bigint` and `symbol` could have lost theirs without a red test. - `gc/tests/runtime_roots/interned_string_caches.rs`: mark, rewrite and registration tests for both scanners. The rewrite test forwards all eight `typeof` cells, so a dropped `visit(...)` line fails there. Marking alone is not enough and is asserted separately: a marked but un-rewritten cell still hands out a pre-move address after a copying minor, which is the whole PerryTS#7211 failure. - `test_gap_gc_typeof_string_cache_rooting.ts` now drives all eight cells. Unregistering the scanner takes it from `bad 0` to `bad 592` 5/5 under a genuine `POLLS=1` build; the six-cell version reported `bad 444`, and 592/8 == 444/6 == 74, so the two added cells fail at the same collection as the rest rather than being decorative. - `reset_typeof_string_cache_for_test` had no callers and its doc described an arena-reset teardown that does not exist. It is now driven by the tests above, its eight-cell list is shared with the new populate/peek helpers instead of being written out twice, and `raw_json.rs` gets the matching trio. - CLAUDE.md: fold PerryTS#7226's additions back toward the length of the entries around them. The file's own opening note says to keep it concise and put detail in `changelog.d/`, and the incident narrative is already in `changelog.d/7219-registry-gc-unrooted-caches.md`. The detector knobs it re-listed are documented in full two sections above. - `changelog.d/7214-...md`: rewrap so the line does not open with `PerryTS#7161`, which markdownlint reads as a malformed ATX heading (MD018). No runtime behavior changes: every new Rust symbol is `#[cfg(test)]`.
jdalton
force-pushed
the
fix/7226-7227-7214-review-followups
branch
from
August 2, 2026 20:29
69756be to
9ac8046
Compare
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
2 participants
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 out the last unresolved review feedback on #7226, #7227 and #7214. All three are merged, so the fixes group here rather than in three PRs.
Five findings, four dispositions.
Fixed
The
typeofcache test drove six of eight cells.scan_typeof_string_roots_mutis eight hand-writtenvisit(...)calls, soTYPEOF_BIGINTandTYPEOF_SYMBOLcould have lost theirs without a red test.test_gap_gc_typeof_string_cache_rooting.tsnow drives all eight.Measured, not assumed — unregistering the scanner and rebuilding:
POLLS=1compiled and runbad 0bad 05/5bad 5925/5The six-cell version reported
bad 444.592/8 == 444/6 == 74, so the two added cells go bad at the same collection as the other six — they are really covered, not decoration.Nothing tested either scanner from the Rust side. New
gc/tests/runtime_roots/interned_string_caches.rs: mark, rewrite and registration tests for bothscan_typeof_string_roots_mutandscan_raw_json_key_root_mut. Mark and rewrite are asserted separately on purpose — a marked but un-rewritten cell still hands out a pre-move address after a copying minor, which is the whole #7211 failure.Sabotage-tested, per the repo's own "four ways a gate can be unable to fail" rule:
visit(&TYPEOF_BIGINT, visitor)cell 6gc_initregistration ofscan_raw_json_key_root_mutreset_typeof_string_cache_for_testwas dead code. It had no callers, and its doc described a shared arena-reset teardown that does not exist in this repo. It is now driven by the tests above, its eight-cell list is shared with the new populate/peek helpers rather than written out twice, andraw_json.rsgets the matching trio.CLAUDE.md. #7226 grew one bullet to 2006 characters and added another at 1358, in a section whose other entries run 242-355. The file's own line 5 says to keep it concise and put detail in
changelog.d/; the incident narrative is already inchangelog.d/7219-registry-gc-unrooted-caches.md, and the detector knobs the new bullet re-listed are documented in full two sections above. Trimmed to 1722 and 903, keeping every operational fact.MD018.
changelog.d/7214-...mdline 117 opened with#7161, which markdownlint reads as a malformed ATX heading. Rewrapped.Not changed
SYMBOL_ROOTSdoes not need theperry-ext-*crates. The audit is a liveness check onALLOC_RE's alternatives, so widening the symbol corpus can only make it more permissive, never less. Measured: 3775 symbols under the current roots, 394 more that are ext-only, and the dead-alternative verdict is the empty list either way. No alternative is kept alive only by an ext symbol. The 26 ext-only allocating symbols already matchALLOC_REvia the_new/_createconventions, so detection coverage is identical. Left alone, andgc_root_dominance_check.pyis in-flight elsewhere anyway.Verification
cargo test -p perry-runtime --lib -- --test-threads=1: 1651 passed, 0 failed. The parent measures 1640 passed, 0 failed at the same setting, and the delta is exactly the 6 new tests plus the 5 that landed in the rebase.Run it multi-threaded and the suite is order-flaky on the parent too — 7 distinct names across 3 parent runs, including
gc::tests::teardown::map_set_side_allocations_release_on_thread_exitandjs_pty_spawn_shell_data_and_exit. That is pre-existing and--test-threads=1is clean on both sides, so the comparison is quoted there.No runtime behavior changes: every new Rust symbol is
#[cfg(test)].Summary by CodeRabbit
Bug Fixes
typeofandJSON.rawJSONvalues.typeofresult types, includingbigintandsymbol.Tests
Documentation