test(runtime): clear C5a per-key vetting sets in the inline-guard test reset#6805
Open
proggeramlug wants to merge 2 commits into
Open
test(runtime): clear C5a per-key vetting sets in the inline-guard test reset#6805proggeramlug wants to merge 2 commits into
proggeramlug wants to merge 2 commits into
Conversation
added 2 commits
July 22, 2026 21:49
… test reset (CodeRabbit on #6802) The sets are production-monotonic by design; without clearing them the cfg(test) reset left earlier tests' declared-field / installed-key state visible to later tests reusing a key name, making the disable decision order-dependent.
📝 WalkthroughWalkthroughThe class-field inline-guard test reset now clears C5a declared-field and prototype-descriptor hash sets, preventing state from persisting across tests. A changelog entry documents the reset behavior. ChangesC5a test reset cleanup
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 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 `@crates/perry-runtime/src/object/descriptor_state.rs`:
- Around line 154-159: Update the reset helper’s write-lock handling for
DECLARED_FIELD_NAME_HASHES and PROTO_DESCRIPTOR_KEY_HASHES so poisoned locks are
recovered with into_inner() or cause an explicit failure, rather than being
silently skipped. Ensure guard.take() runs for both hash sets even after a prior
test panic, preserving deterministic reset behavior.
🪄 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: b9d04e3b-fd95-4fb9-8332-5759652b2e7d
📒 Files selected for processing (2)
changelog.d/6805-test-reset-c5a-sets.mdcrates/perry-runtime/src/object/descriptor_state.rs
Comment on lines
+154
to
+159
| if let Ok(mut guard) = DECLARED_FIELD_NAME_HASHES.write() { | ||
| guard.take(); | ||
| } | ||
| if let Ok(mut guard) = PROTO_DESCRIPTOR_KEY_HASHES.write() { | ||
| guard.take(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not silently skip a poisoned reset lock.
If a test panics while holding either lock, write() returns Err and this helper leaves the corresponding hash set populated, reintroducing order-dependent test state. Recover the poisoned guard with into_inner() (or fail explicitly) so the reset remains deterministic.
🤖 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/object/descriptor_state.rs` around lines 154 - 159,
Update the reset helper’s write-lock handling for DECLARED_FIELD_NAME_HASHES and
PROTO_DESCRIPTOR_KEY_HASHES so poisoned locks are recovered with into_inner() or
cause an explicit failure, rather than being silently skipped. Ensure
guard.take() runs for both hash sets even after a prior test panic, preserving
deterministic reset behavior.
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.
Follow-up to CodeRabbit's post-merge review finding on #6802:
test_reset_class_field_inline_guard()reset only the sticky disable flag, leavingDECLARED_FIELD_NAME_HASHES/PROTO_DESCRIPTOR_KEY_HASHESaccumulated across tests in one process — a key name reused by a later test would inherit earlier state and make the disable decision order-dependent. The reset now clears both sets (cfg(test)-only change; the sets stay monotonic in production by design).Both
c5a_testsgreen with the fix. Includes the changelog.d fragment.Summary by CodeRabbit