fix(ci): unbreak the thread-local policy gate, and stop counting #[cfg(test)] declarations - #7824
Merged
Merged
Conversation
…declarations, stop counting `#[cfg(test)]` ones `scripts/check_thread_locals.py` has been red on `main` (exit 1). Because it is deliberately not yet a required context, nothing surfaced it — and that red state is exactly what blocks `tls-budget`'s documented promotion step, so the gate stays advisory indefinitely. This is the shape CLAUDE.md calls gate theatre: it runs, it reports failure, and it cannot block anything. TWO OF THE EIGHT RAW BLOCKS ARE REAL, AND THEY ARE NOT THE ONES REPORTED. #7814 names `array/indexing.rs`, `map.rs`, `set.rs` and `registry_latch_probes.rs` as "hot path" violations. All four are `#[cfg(test)]`: they do not exist in a shipping build and cannot cost a single `_tlv_get_addr`. The genuinely live declarations are in `gc/schedule.rs`, which the ticket does not mention — `SAFEPOINT_COUNTER` (bumped on every safepoint) and `SCHEDULE_NEXT_CANDIDATE_BYTES` (read on every poll). Those two are converted to `crate::perry_thread_local!`. `#[cfg(test)]` DECLARATIONS ARE OUT OF SCOPE BY CONSTRUCTION. Recording a test-only declaration as "cold" records the wrong fact — it is not cold, it is absent — and spends the allowlist's credibility on entries nobody can ever act on. The scan now skips three statically-visible gated shapes: the attribute directly above a block, an inline `#[cfg(test)] mod … { … }`, and a file whose whole module is declared `#[cfg(test)] mod <stem>;` (closed transitively, so `gc/tests/mod.rs` carries its subtree). That is the direction that can go wrong quietly — an over-broad exclusion makes the gate pass by seeing less — so `--self-test` grew two cases per shape: gated is not counted, AND removing the gate makes the same declaration fail again. It now proves the checker can fail in six directions, not four. The allowlist regeneration is provably one-way: 101 -> 91 entries, 157 -> 129 blocks, with nothing added and no entry's count increased. Three of the dropped entries were already stale (`gc/zeal.rs` from #7741's removal of `PERRY_GC_ZEAL`, plus `arena/quarantine.rs` and `gc/oldgen_defrag.rs`). `_hot_declarations` also moves 157 -> 160, and only two of those three are mine: the recorded value was already one behind the tree, since nothing compares it except the capacity ceiling. Still a maintainer action, and the point of the exercise: promote `tls-budget` to a required context after its first green run on `main`. Refs #7814
📝 WalkthroughWalkthroughThe thread-local policy checker now excludes test-only declarations across module-tree forms. GC scheduling declarations use ChangesThread-local policy gate
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PolicyCheck as check_thread_locals.py
participant ModuleTree as Rust module tree
participant Sources as Rust source files
participant Allowlist as thread_local_cold_allowlist.json
PolicyCheck->>ModuleTree: Resolve cfg(test) module descendants
ModuleTree-->>PolicyCheck: Return test-only files
PolicyCheck->>Sources: Count shipping raw thread_local! blocks
Sources-->>PolicyCheck: Return production declarations
PolicyCheck->>Allowlist: Compare declaration counts and entries
Allowlist-->>PolicyCheck: Return policy result
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 |
Contributor
Author
|
|
proggeramlug
marked this pull request as ready for review
August 11, 2026 01:44
![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 `@scripts/check_thread_locals.py`:
- Around line 129-142: The brace_span scanner must ignore braces inside Rust
comments and literals when finding block boundaries. Replace its
character-by-character matching with Rust-aware lexical scanning while
preserving the returned opening and closing offsets, and add a --self-test case
covering braces in a string or comment so declarations outside the intended
block are classified correctly.
🪄 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: ddcfcafd-a5da-43d5-934e-0ce6aaa48287
📒 Files selected for processing (4)
changelog.d/7824-thread-local-policy-gate.mdcrates/perry-runtime/src/gc/schedule.rsscripts/check_thread_locals.pyscripts/thread_local_cold_allowlist.json
Comment on lines
+129
to
+142
| def brace_span(src: str, start: int) -> tuple[int, int]: | ||
| """`(open, close)` offsets of the brace-matched block opening at/after `start`.""" | ||
| i = src.index("{", start) | ||
| depth = 0 | ||
| j = i | ||
| while j < len(src): | ||
| if src[j] == "{": | ||
| depth += 1 | ||
| elif src[j] == "}": | ||
| depth -= 1 | ||
| if depth == 0: | ||
| break | ||
| j += 1 | ||
| return i, j |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse Rust block boundaries without counting braces in literals or comments.
brace_span treats every { and } as syntax. A } in a string before a test-only thread_local! ends the inline module span early. The scanner then counts a declaration that cannot ship. A { can also extend the span and hide a shipping declaration.
Use a Rust-aware lexical scan that ignores comments and literals before matching braces. Add a --self-test case with braces inside a string or comment.
Also applies to: 191-201, 363-395
🤖 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 `@scripts/check_thread_locals.py` around lines 129 - 142, The brace_span
scanner must ignore braces inside Rust comments and literals when finding block
boundaries. Replace its character-by-character matching with Rust-aware lexical
scanning while preserving the returned opening and closing offsets, and add a
--self-test case covering braces in a string or comment so declarations outside
the intended block are classified correctly.
pull Bot
pushed a commit
to Mu-L/perry
that referenced
this pull request
Aug 11, 2026
Covers the 32 PRs admin-merged in one pass (audited in principle at the maintainer's direction): PerryTS#7768 PerryTS#7772 PerryTS#7779 PerryTS#7784 PerryTS#7785 PerryTS#7786 PerryTS#7788 PerryTS#7789 PerryTS#7797 PerryTS#7798 PerryTS#7801 PerryTS#7802 PerryTS#7804 PerryTS#7805 PerryTS#7806 PerryTS#7807 PerryTS#7808 PerryTS#7810 PerryTS#7811 PerryTS#7815 PerryTS#7816 PerryTS#7818 PerryTS#7819 PerryTS#7820 PerryTS#7821 PerryTS#7822 PerryTS#7823 PerryTS#7824 PerryTS#7825 PerryTS#7826 PerryTS#7827 PerryTS#7828. (PerryTS#7787 closed as already-landed via the PerryTS#7786 stack.) Per-change history lives in each PR's changelog.d fragment as usual. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
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.
Fixes #7814.
scripts/check_thread_locals.pyis red onmain(exit 1). Because it isdeliberately not yet in branch protection's required contexts, nothing surfaces
it — and that red state is exactly what blocks
tls-budget's documentedpromotion step, so the gate stays advisory indefinitely. It is the shape
CLAUDE.md calls gate theatre: it runs, it reports failure, and it cannot block
anything.
Two of the eight raw blocks are real — and they are not the ones reported
The ticket names
array/indexing.rs,map.rs,set.rsandregistry_latch_probes.rsas violations "in hot paths". All four are#[cfg(test)]. They do not exist in a shipping build and cannot cost a single_tlv_get_addr. (registry_latch_probes.rsis not even compiled outside tests —lib.rs:143declares it#[cfg(test)] mod.)The genuinely live declarations are in
gc/schedule.rs, which the ticket doesnot mention:
SAFEPOINT_COUNTERSCHEDULE_NEXT_CANDIDATE_BYTESSCHEDULE_OVERRIDE#[cfg(test)]SCHEDULE_STRIDE_OVERRIDE#[cfg(test)]The two ungated ones are converted to
crate::perry_thread_local!.#[cfg(test)]declarations are out of scope by constructionRecording a test-only declaration as "cold" records the wrong fact — it is not
cold, it is absent — and spends the allowlist's credibility on entries nobody
can ever act on. The scan now skips three statically-visible gated shapes:
#[cfg(test)]directly above the block,#[cfg(test)] mod … { … }enclosing it,#[cfg(test)] mod <stem>;, closedtransitively so
gc/tests/mod.rscarries its subtree.Unlike hot-vs-cold, this is a static fact the scan can actually see.
The new rule is the kind that fails quietly, so it is sabotage-tested
An over-broad exclusion makes the gate pass by seeing less, which looks
identical to a fix.
--self-testtherefore grew two cases per shape: gated isnot counted, and removing the gate makes the same declaration fail again. It
now proves the checker can fail in six directions rather than four, and the
whole-file rule is checked in both directions too (
#[cfg(test)] mod probes;excluded, plain
mod probes;back in scope).The regeneration is provably one-way
Asserted mechanically against the pre-image, because
--updateabsorbing a realviolation is the failure mode the ticket warns about. Three dropped entries were
already stale:
gc/zeal.rs(left by #7741's removal ofPERRY_GC_ZEAL),arena/quarantine.rsandgc/oldgen_defrag.rs._hot_declarationsmoves 157 -> 160, and only two of those three are mine: therecorded value was already one behind the tree, since nothing compares it
except the capacity ceiling (160 vs 768).
Validation
scripts/check_thread_locals.py→thread-local policy OK: 160 hot declarations, 129 raw blocks in 91 recorded cold files, capacity 768, exit 0.scripts/check_thread_locals.py --self-test→ passes in all six directions.cargo build -p perry-runtimeclean;cargo test -p perry-runtime -- --test-threads=1— see the comment below.Left for the maintainer, and the point of the exercise
Promote
tls-budgetto a required context after its first green run onmain.The workflow's own header specifies this step, and CLAUDE.md's corollary is that
leaving it undone is how
gc-root-dominancesat red for weeks.Summary by CodeRabbit
Bug Fixes
Tests