fix(test): isolate typed_feedback failures — a poisoned ENV_LOCK turned one failure into five (#7490) - #7492
Merged
Conversation
…ed one failure into five (#7490) `cargo test -p perry-codegen --test typed_feedback` reported five failures as a suite and a wobbling set under default parallelism. The suspected cause was process-global codegen state leaking between in-process compiles. It was not. Two assertions had genuinely drifted from intentional codegen changes, and both fail when run ALONE (contrary to the issue's premise): * `typed_feedback_guards_direct_class_field_specialization` matched the numeric coercion in the textual window between the `class_field_get_number.fallback` and `.merge` labels. #7430 split that arm: `.fallback` now holds only the nullish-receiver check, and the by-name load plus coercion moved to `.fallback_lookup` — a block RENDERED AFTER `.merge`. The window could never match again. * `typed_feedback_trace_dump_runs_before_entry_return` cut `main`'s body at the literal header `define i32 @main() {`. Since #7370 made native roots the default every emitted function carries `"frame-pointer"="non-leaf"`, so that header never matches. The first of those panics while holding `ENV_LOCK`, which poisons the mutex for the rest of the process; every later `ENV_LOCK.lock().unwrap()` then dies with `PoisonError` regardless of its own subject. That is the whole of the "order dependence": under `--test-threads=1` the alphabetically-early poisoner takes three healthy tests down with it, and under default parallelism the victim set shifts with the scheduler. Fix, in three parts: * `env_lock()` recovers a poisoned guard. Sound because each test declares its `EnvVarGuard` after the lock guard, so the env var is restored during unwind before the mutex is released — the protected state is already consistent at poison time. One test's failure must fail that test alone. * Both drifted assertions are re-pointed at the current IR and made STRONGER, not looser. The class-field one now proves the data flow the positional window stood in for, end to end: `.fallback_lookup` records the fallback, loads by name and coerces; its terminator branches to the numeric merge; and the merge phi's fallback incoming IS the coerced register. `entry_fn_body` matches the exact signature and cuts at that line's opening brace, so unrelated attribute changes can no longer fail the test. * A sabotage test plants the exact #7490 shape — an unwind out of a lock-holding test — asserts it really poisoned the mutex, and demands the accessor still hands out a guard. It fails against the pre-fix `.lock().unwrap()` (9 of 16 red), so a green run is evidence, not decoration. No production codegen state leaks between compiles: `PERRY_TYPED_FEEDBACK` is read live at each call site and `PERRY_FULL_OUTLINE_IC`'s decision is a thread-local set once per `compile_module` — both already correct.
|
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 (2)
📝 WalkthroughWalkthroughThe typed-feedback integration tests now recover from poisoned environment locks, parse generated entry functions with attributes, and validate class-field fallback IR through block-level assertions. The changelog and workspace version documentation are updated. ChangesTyped-feedback test robustness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 was referenced Aug 6, 2026
added 2 commits
August 6, 2026 09:52
This was referenced Aug 6, 2026
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 #7490.
What the order dependence actually was
The issue's premise — process-global codegen state leaking between in-process compiles — does not hold. There is no leaking codegen state, and two of the five reported failures also fail when run alone, contrary to the issue text:
typed_feedback_guards_direct_class_field_specializationtyped_feedback_trace_dump_runs_before_entry_returntyped_feedback_guards_direct_class_method_specializationPoisonErrorcascade onlytyped_feedback_instruments_property_and_method_boundariesPoisonErrorcascade onlytyped_feedback_marks_numeric_array_literalsPoisonErrorcascade onlyThe three cascade victims pass as a group when the poisoner is not in the run.
The two genuine assertion drifts
typed_feedback_guards_direct_class_field_specializationasserted that the numeric coercion appeared in the textual window between theclass_field_get_number.fallbackand.mergelabels. #7430 split that arm —.fallbacknow carries only the nullish-receiver check and branches to.throw_nullish/.fallback_lookup, and the by-name load plus coercion moved into.fallback_lookup, which is rendered after.merge:The window could never match again. The codegen is correct; the assertion was stale.
typed_feedback_trace_dump_runs_before_entry_returncutmain's body at the literal headerdefine i32 @main() {. Since #7370 made native roots the default, every emitted function carries"frame-pointer"="non-leaf"(and a stack-map-requesting function addsgc "statepoint-example"), so the header never matches and the.expectfires.How two failures became five, wobbling
typed_feedback_guards_direct_class_field_specializationpanics while holdingENV_LOCK. The unwind poisons the mutex for the rest of the process, so every laterENV_LOCK.lock().unwrap()dies withPoisonErrorregardless of whether its own subject is healthy. Under--test-threads=1the alphabetically-early poisoner deterministically takes three healthy tests down; under default parallelism the victim set shifts with the scheduler — which is exactly the reported "the failing set wobbles, andfull_outline_ic_*join and leave it".The fix
Three parts, none of which loosens an assertion.
Isolation.
env_lock()recovers a poisoned guard (unwrap_or_else(PoisonError::into_inner)). This is sound rather than a shrug: each test declares itsEnvVarGuardafter the lock guard, so Rust's reverse drop order restores the env var during unwind before the mutex is released — the protected state is already consistent at poison time. One test's failure must fail that test alone.Re-point the two drifted assertions — stronger, not looser. The class-field test now proves the data flow that the positional window only stood in for, end to end:
.fallback_lookuprecords the fallback call, loads by name and coerces; its terminator branches to the numeric merge; and the merge phi's fallback incoming is the coerced register. A newblock_body()helper reads a named block by stable label prefix, so per-function numeric label suffixes and block ordering stop being load-bearing.entry_fn_bodymatches the exact signature and cuts at that line's opening brace, so unrelated attribute changes can no longer fail the test.A sabotage test for the isolation itself.
env_lock_is_poison_tolerant_so_one_failure_cannot_cascadeplants the exact typed_feedback integration suite is order-dependent: 5 tests fail in-suite, pass alone #7490 shape — an unwind out of a lock-holding test — asserts it really did poison the mutex (the gate asserts its subject was live), then demands the accessor still hands out a usable guard. Against the pre-fix.lock().unwrap()it fails and reproduces the cascade: 9 of 16 red. Because it sorts first, every other test in the binary then runs under a genuinely poisoned lock, so the tolerance is exercised suite-wide rather than in one isolated case.Is this a production bug too?
No. Both process-global gates these tests touch are already right for the real compiler:
PERRY_TYPED_FEEDBACKis read live at each call site (crates/perry-codegen/src/expr/typed_feedback.rs:252), never cached.PERRY_FULL_OUTLINE_IC's decision is athread_local!set once percompile_module(crates/perry-codegen/src/codegen/helpers.rs:371), deliberately not a process-globalOnceLock, precisely so a multi-module build cannot pin the first module's decision.The defect was confined to the test binary's failure isolation.
Validation
cargo test -p perry-codegen --test typed_feedback— 16 passed, 0 failed, three consecutive runs at default parallelism and three at--test-threads=1.--list), so no test depends on another having run.cargo fmt --all -- --checkclean.env_lock()to.lock().unwrap()turns the suite red at 9/16 and restores the cascade — so the isolation is doing the work.Other suites checked (different root cause — not fixed here)
The sweep in the issue also named several suites. Every one of their failures reproduces when the test is run alone, so none is order-dependent, and this fix heals none of them. Full
--no-fail-fast -- --test-threads=1sweeps of-p perry-codegen --tests, default vs. the lowering override:They split into two causes, filed granularly rather than folded in:
shadow_slot_hygienegoing 0/12 → 11/12 underPERRY_RS4GC=0is the diagnosis; three ofnative_proof_regressions' four failures heal the same way. But that suite loses 13 tests underRS4GC=0— its wholeinvalidation::*family asserts the native-roots lowering and is correct today — so no single global setting satisfies the set. The pin has to be per-test, and the one that repaired the in-crate unit tests,NativeRootsPin::shadow(), is#[cfg(test)]and therefore unreachable fromtests/*.rs.PERRY_RS4GC=0):proven_buffer_and_typed_array_reads_are_numeric_operands,reassigned_typed_array_store_records_runtime_fallback,integer_modulo::i32_counter_mod_unsafe_or_nonliteral_divisors_keep_frem, andinteger_arithmetic_array_push_omits_inbounds_layout_note_and_barrier. Each needs a decision on whether the lowering changed on purpose or a proof regressed, so they are deliberately not swept into a test-only PR.CI
cargo-testandapi-docs-driftpass. The red jobs are pre-existing and reproduce identically on unrelated PR #7468:gc-ratchet,gc-root-dominance,security-audit,Warnings.lintfails only on its "Public benchmark evidence freshness" step (stale public baseline —run_public_baseline.shneeds a regen); its fingerprints coverCargo.tomlandbenchmarks/**only, and this PR touches neither.Summary by CodeRabbit
Bug Fixes
Documentation
Chores