gc: enumerate the runtime-side GC-pointer holders, with a gate (#7231) - #7695
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 ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded a runtime GC-pointer holder audit. The audit discovers holders, evaluates scanner coverage, reconciles classifications, runs self-tests, and fails lint for missing or stale inventory entries. ChangesRuntime GC custody audit
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Lint
participant gc_runtime_root_holders.py
participant RuntimeSources
participant HolderInventory
Lint->>gc_runtime_root_holders.py: Run self-tests and audit
gc_runtime_root_holders.py->>RuntimeSources: Discover holders and scanners
RuntimeSources-->>gc_runtime_root_holders.py: Return declarations and scanner reachability
gc_runtime_root_holders.py->>HolderInventory: Load and reconcile classifications
HolderInventory-->>gc_runtime_root_holders.py: Return missing and stale entries
gc_runtime_root_holders.py-->>Lint: Report success or fail lint
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
CLAUDE.md (1)
254-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this guidance concise.
This bullet includes implementation rules, current inventory counts, historical checker defects, and test details. Keep a short instruction to run
scripts/gc_runtime_root_holders.pyand maintain its inventory. Keep the detailed history inchangelog.d/7695-runtime-root-holder-gate.md.As per coding guidelines, "
CLAUDE.md" must stay concise and detailed change history belongs inchangelog.d/fragments.🤖 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` at line 254, Condense the runtime-side cache guidance in CLAUDE.md to a brief instruction to run scripts/gc_runtime_root_holders.py and keep its inventory current, while retaining the essential requirement to register new heap-pointer caches with gc_register_mutable_root_scanner. Move implementation rules, checker history, counts, and test details into changelog.d/7695-runtime-root-holder-gate.md.Source: Coding guidelines
🤖 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 `@scripts/gc_runtime_root_holders.py`:
- Around line 350-371: Update apply_inventory to validate each matching
inventory entry before marking a holder as covered: require verdict and why,
accept only recognized verdicts, reject unverified, and resolve
covered_elsewhere through the registered scanner definitions. Preserve
stale-entry detection and ensure every relevant holder has scanner coverage or
an explicit valid verdict. Extend the --self-test fixtures with incomplete,
unknown, unverified, and unresolved-scanner inventory entries.
- Around line 282-319: Update the call-graph construction around REGISTER_CALL,
function_bodies, and reachable_text_by_file to preserve module-qualified
function identities instead of collapsing names with rsplit. Store bodies and
registered entries under qualified keys, resolve each IDENT call relative to the
defining module, and only mark the matching function reachable. Add a self-test
covering a registered scanner and an unrelated same-named function that
references an uncovered holder, ensuring the holder remains uncovered.
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 254: Condense the runtime-side cache guidance in CLAUDE.md to a brief
instruction to run scripts/gc_runtime_root_holders.py and keep its inventory
current, while retaining the essential requirement to register new heap-pointer
caches with gc_register_mutable_root_scanner. Move implementation rules, checker
history, counts, and test details into
changelog.d/7695-runtime-root-holder-gate.md.
🪄 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: 1b264112-ef4d-46b1-94d4-3f5ea20acc81
📒 Files selected for processing (5)
.github/workflows/test.ymlCLAUDE.mdchangelog.d/7695-runtime-root-holder-gate.mdscripts/gc_runtime_root_holders.jsonscripts/gc_runtime_root_holders.py
added 5 commits
August 9, 2026 15:50
A `thread_local!` or `static` in perry-runtime / perry-stdlib that stores a pointer into the GC heap IS a GC root, and the collector only knows that if something registers it. Nothing static could find the class: the one static checker this repo has reads emitted LLVM IR, and a runtime table is not in it. #7226, #7239, #7268 and #7274 were all found by hand, each re-deriving the same sweep. This is that sweep as something that can fail. `scripts/gc_runtime_root_holders.py`: 1. Enumerates every static-shaped declaration in the two crates whose type can hold a GC pointer. Rule A: the type names a heap header or `JSValue`. Rule B: an integer/`f64` cell that some function in its own file both names and allocates in — which is the only way to catch `CACHED_ENV: Cell<f64>`, the highest-impact holder in the issue's original report. 2. COMPUTES coverage instead of trusting names. Registered scanners are read from every `gc_register_*root_scanner*(...)` call; a call graph over both crates is walked from them, and a holder counts as covered when its name appears in a reachable function DEFINED IN THE SAME FILE. `REGISTRY`, `SLOTS`, `ROOTS`, `STATES` and `CACHED` each name several different holders here, so a name-only match certifies the wrong one; the graph walk is what finds holders a scanner reaches through an accessor (`cp_live_lock()`, `get_closure_props()`, `buffer_props()`). 3. Requires a written verdict for the rest, in `scripts/gc_runtime_root_holders.json`. An unclassified holder fails, and so does an entry that no longer matches — which is what makes a fix delete its own exemption. Current state: 81 holders, 47 reached by a registered scanner, 30 classified. The inventory records 11 `covered_elsewhere` (the gate's known false positives, each naming the scanner that covers it), 15 `not_a_gc_pointer`, 1 `test_only`, 1 `unverified`, and two `open_gap`s the sweep found and nothing tracked: * `json/mod.rs` `PARSE_KEY_RING` — a hot-key mirror of the ROOTED `PARSE_KEY_CACHE`. A move rewrites one copy and not the other. Narrow: the keys are longlived/old-gen, so only old-gen defrag can move them. * `perf_hooks.rs` `PERF_ENTRY_KEYS_ARRAY` — a nursery `keys_array` address compared by identity and never rewritten. Stale ⇒ a silent slow path, or a match against a newly-allocated array recycled into the address. Three bugs found while building it, all in the checker rather than the tree, and all of the "green because it matched nothing" shape: * string literals were not stripped, so brace counting swallowed `scan_raw_json_key_root_mut` and reported `RAW_JSON_KEY` — which that scanner visits three lines below its declaration — as uncovered; * the registration regex captured only the FIRST argument, so `gc_register_mutable_root_scanner_named("name", scanner)` registered nothing and six worker_threads holders read as uncovered; * rule B keyed on the file rather than the function and reported 544 holders, four fifths of them counters — a gate nobody would read. `--self-test` plants a covered holder, one reached only through an accessor, one uncovered per rule, and a same-named decoy in another file, and asserts each classification; then asserts the verdict machinery can go red (empty inventory ⇒ everything unclassified; an entry matching nothing ⇒ stale; an entry for a COVERED holder ⇒ stale). Live sabotage: planting an unrooted `Cell<*mut ObjectHeader>` into `regex.rs` fails the real scan. The docstring names what the gate CANNOT see — `RuntimeState`'s fields (not declarations; a field-count floor makes growth loud), integer holders whose file never allocates, cross-file scanners, and whether a "covered" holder is covered CORRECTLY (the #7239 three-of-four-slots shape). It bounds the population; it does not audit semantics.
Both are the hazard this script exists to catch, in the script itself.
1. Bare-name reachability could certify the WRONG module's holder.
`bodies` is keyed on the bare function name, so two modules defining
`scan_roots_mut` share a key — and registering one made the other's body
reachable, marking a holder in that module covered when nothing scans it.
Not hypothetical: `scan_tls_roots_mut` is defined in BOTH perry-runtime and
perry-stdlib, and `worker_threads` has several `scan_*_roots_mut` siblings.
The registration text carries the module path
(`crate::json::raw_json::scan_raw_json_key_root_mut`), so the ROOT set is now
resolved to a defining file; a name that resolves to several definitions must
match the path. Deeper hops stay bare-name — nothing in the text says which
module a call resolved to — and the docstring's "cannot see" section now says
that out loud rather than leaving it as an assumption.
Self-test: two modules define `scan_dup_roots_mut`, only one is registered,
and the unregistered module's holder must read UNCOVERED. Sabotage-verified —
reverting to bare-name reachability fails that case and only that case.
2. `apply_inventory` accepted any object carrying a matching (file, name).
No `verdict`, an invented `verdict`, an empty `why`, a `covered_elsewhere`
naming no scanner, an `open_gap` citing no issue — each silenced a holder,
and a suppression whose justification cannot be read or checked is a mute
button rather than a decision record.
`inventory_problems` now validates the vocabulary, requires a `why` long
enough to be a reason, requires `scanner` on `covered_elsewhere` and `issue`
on `open_gap`, rejects duplicates, and caps `unverified` at 2 so the one
verdict that classifies nothing cannot quietly become the whole inventory.
The self-test plants one malformed entry per rule and requires each to be
rejected.
It found seven of my own entries with reasons too thin to check
("Monotonic counter.", "Same six-slot loop.") on its first run. Those are
rewritten.
Gate output is unchanged: 81 holders, 47 reached by a registered scanner, 30
classified.
The gate said so itself: an entry that no longer matches an open gap is stale and fails the build, which is what makes the inventory shrink-only. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
proggeramlug
force-pushed
the
gc/7231-runtime-root-holder-gate
branch
from
August 9, 2026 13:52
339db36 to
377d33c
Compare
Contributor
Author
Audit — merging as v0.5.1402, with the stale entry deletedThe gate predicted its own first failure and was right. Its
#7694 landed, and both the gate and its self-test went red on exactly that entry. I deleted it: holders 30 → 29, both green. That is the shrink-only property working on its first real test rather than being asserted. 80 holder declarations scanned, 47 reached by a registered scanner, 29 classified, 122 registered scanners. The thing that makes this a gate rather than a list is that coverage is computed — registered scanners → call-graph walk → holder named in a reachable function in the same file — instead of declared. Sabotage-verified independently: I planted The three checker bugs you found in it are the report's best contentAll three were "green because it matched nothing", which is the failure this whole campaign is about, appearing inside the tool built to detect it:
And CodeRabbit's two more are the same shape: bare-name module collisions could certify the wrong module's holder (real — The two previously-untracked Gates: 26/26, |
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.
Addresses #7231 — the enumeration, with a mechanical gate rather than a list in
a comment.
Why a gate and not a sweep
A runtime-side table holding a GC pointer is a root, and nothing static
could find the class:
scripts/gc_root_dominance_check.pyreads emitted LLVMIR, and a
thread_local!is not in it. That is not a gap in that tool, it isoutside its subject. #7226, #7239, #7268 and #7274 were each found by hand, and
each one re-derived the same sweep. The issue's own closing line is the point:
a list nobody checks is how this class got here.
What the gate does
scripts/gc_runtime_root_holders.py, inlint(already a required context —hazard 2 of CLAUDE.md's four is the promotion step people forget, so the gate is
placed where that step does not exist).
Enumerate. Every static-shaped declaration in
perry-runtime/perry-stdlibwhose type can hold a GC pointer. Rule A: the type names aheap header or
JSValue. Rule B: an integer/f64cell that some function inits own file both names and allocates in — the only way to catch
CACHED_ENV: Cell<f64>, the highest-impact holder in the issue's report.Compute coverage, don't trust names. Registered scanners are read from
every
gc_register_*root_scanner*(...)call site; a call graph over bothcrates is walked from them to depth 3; a holder counts as covered when its
identifier appears in a reachable function defined in the same file as the
declaration.
Both halves are load-bearing. The graph walk is what finds holders a scanner
reaches through an accessor rather than by name (
cp_live_lock(),get_closure_props(),buffer_props()). The same-file requirement is whatstops
REGISTRY,SLOTS,ROOTS,STATESandCACHED— each of whichnames several different holders in this tree — from certifying the wrong
one.
Require a verdict for the rest, in
scripts/gc_runtime_root_holders.json.An unclassified holder fails. An entry that no longer matches also fails,
which is what makes a fix delete its own exemption (the
gc_root_dominance_allowlist.jsonrule).Current state
81 holder declarations scanned, 47 reached by a registered scanner, 30 classified in the inventory (122 registered scanners).The inventory records 11
covered_elsewhere— the gate's known false positives,each naming the scanner that actually covers it, so they are documented rather
than silenced — 15
not_a_gc_pointer, 1test_only, 1unverified, and twoopen_gaps the sweep found that nothing tracked:json/mod.rsPARSE_KEY_RING— a hot-key mirror of the rootedPARSE_KEY_CACHE, holding the same*const StringHeadervalues. A moverewrites one copy and not the other, so a ring hit hands out a pre-move
address. Narrow: the keys are
js_string_from_bytes_longlived, i.e. old-gen,so only old-gen defrag can move them — not the copying minor.
perf_hooks.rsPERF_ENTRY_KEYS_ARRAY— a nurserykeys_arrayaddresscompared by identity at
perf_hooks.rs:119and never rewritten. Stale ⇒ asilent slow path, or — the sharper half — a match against a newly-allocated
array recycled into the recycled address. The neighbouring
PERF_ENTRIESiscovered; this one slot is not.
SHAPE_CACHEis also listed, as anopen_gappointing at #7268 / #7694. If#7694 lands first, delete that one entry — the gate will say so, which is the
mechanism working.
Three bugs found while building it — all in the checker, all "green because it matched nothing"
Worth recording, because this is the failure mode the repo keeps paying for:
function body swallowed
scan_raw_json_key_root_mut— andRAW_JSON_KEY,which that scanner visits three lines below its own declaration, reported as
uncovered.
gc_register_mutable_root_scanner_named("stdlib:worker_threads:workers", scan_worker_roots_mut)registered nothing and sixworker_threadsholdersread as uncovered.
holders, four fifths of them counters and ids — a gate nobody would read.
Each would have produced a plausible-looking inventory full of entries that were
simply wrong.
How it is built to be able to fail
--self-testplants a covered holder, one reachable only through anaccessor, one uncovered per rule, and a same-named decoy in another file, and
asserts every classification. Then it asserts the verdict machinery can go
red: an empty inventory must leave every uncovered holder unclassified; an
entry matching nothing must be stale; an entry for a covered holder must
be stale.
MIN_HOLDERSdeclarations orMIN_REGISTEREDscannersexits 2, because a broken regex would otherwise report a clean, empty,
green run — and a broken registration regex would make everything read as
uncovered, which is noise, not a gate.
Cell<*mut ObjectHeader>intocrates/perry-runtime/src/regex.rsfails thereal scan, naming the file, line, type and rule. Restored; the scan is green
again.
What it CANNOT see — stated in the docstring, because an unstated limit is how a gate gets trusted past its subject
RuntimeState's fields.state.rsabsorbed roughly a dozen formerthread_local!s; they are struct fields reached throughstate(), so nodeclaration-site scan sees them. All are covered today. A field-count floor
makes the struct growing at least loud.
covered_elsewhereentry naming the scanner.of a table's four slots — the shape fix(gc): root the ten unrooted runtime-side caches, and the scanner that walked 1 of 3 sibling slots (#7231) #7239 found in
scan_parent_port_event_roots_mut. This gate bounds the population; it doesnot audit semantics.
CLAUDE.md's runtime-cache bullet gains one clause pointing at the gate. No
version bump.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation