fix(test-isolation): two process-global sinks recorded across test boundaries - #7665
Merged
Conversation
added 4 commits
August 8, 2026 23:17
opt_report's gate (FORCED) and sink are process-global, but Session's lock only serialises tests that TAKE a Session. A concurrently-running test that lowers code without one therefore emitted into the holder's sink, and the holder's snapshot saw a neighbour's rows. Observed, not theoretical: only_the_return_position_is_marked_served asserts rows.len() == 2 and failed at 3 about one run in six once #7662 added lowering tests, which changed the parallel schedule. 0/14 on main before that PR, 2/18 on it -- the extra row is what identified the mechanism, not the timing. Recording is now additionally narrowed to the thread that opened the Session. Production is unaffected: the thread check is inside the #[cfg(test)] forced branch, and the env-var path is untouched. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…lock USED_PROVIDERS is process-wide but PROVIDER_TEST_LOCK only serialises tests that TAKE it, so a concurrently-running test that lowers any code touching an ext symbol added providers to the holder's set. Observed: ext_prefix_net_does_not_over_match asserts the set is empty after an unlisted symbol and failed 1 run in 20 once #7662 added child_process lowering tests, which call record_ffi_call. ProviderTestGuard takes the lock AND claims the thread; record_ffi_call skips the global set when another thread holds a provider test. Production is unaffected -- PROVIDER_TEST_THREAD is #[cfg(test)] and the predicate returns true whenever no provider test is running. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
|
Caution Review failedThe pull request is closed. ℹ️ 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 (5)
📝 WalkthroughWalkthroughThe change isolates ChangesTest isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: ✨ 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 |
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
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.
fix(test-isolation): two process-global sinks recorded across test boundaries.Both follow one pattern: a process-global collector paired with a lock that only serialises the tests which take it. Any concurrently-running test that exercises the recording path — without knowing the collector exists — wrote into the lock-holder's snapshot.
opt_report—FORCEDand theMutex<Vec<Entry>>sink are global;Sessiondrains on entry and drop but cannot stop a neighbour emitting mid-test.only_the_return_position_is_marked_servedassertsrows.len() == 2and failed at 3.ext_registry—USED_PROVIDERSis a process-wideMutex<HashSet>;record_ffi_callfires from any lowering of an ext symbol.ext_prefix_net_does_not_over_matchasserts the set is empty after an unlisted symbol and failed withioredisstill in it.Both are pre-existing and both were surfaced by #7662 (Layer 1 slice 7), which added
child_processand Proxy/Reflect lowering tests and so changed the parallel schedule:only_the_return_position_is_marked_servedwas 0/14 onmainand 2/18 on that branch;ext_prefix_net_does_not_over_matchwas 1/20.Diagnosed from the extra row, not from the timing. A 1-in-6 flake invites a retry; the value 3-instead-of-2 says a neighbour's entry is in the snapshot, which names the mechanism directly.
Recording is now narrowed to the thread holding the guard —
test_support::recording_thread_is_current()foropt_report,ProviderTestGuard+provider_recording_permitted()forext_registry. Production is untouched in both: theopt_reportcheck sits inside the#[cfg(test)]forced branch and the env-var path is unchanged, andPROVIDER_TEST_THREADis#[cfg(test)]with the predicate returningtruewhenever no provider test is running.Verified 0 failures in 25 consecutive
cargo test -p perry-codegen --lib --no-fail-fastruns on top of #7662, where the pair reproduced at 3/38 before.Summary by CodeRabbit
Bug Fixes
Documentation
Chores