Skip to content

test(gc): run remembered coverage sabotage in release - #11027

Closed
proggeramlug wants to merge 2 commits into
mainfrom
fix/10604-release-gc-sabotage
Closed

proggeramlug wants to merge 2 commits into
mainfrom
fix/10604-release-gc-sabotage

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Run the remembered-set coverage sabotage witness in release unit-test builds instead of silently ignoring it. The cross-check remains compiled out of production release binaries.

Changes

  • compile the skipped-object coverage audit and its read-only counting helper under cfg(test) as well as debug_assertions
  • remove the release-only ignore from the sabotage witness
  • update the surrounding comments to describe the production and test configurations

Related issue

Fixes #10604

Test plan

  • RUST_TEST_THREADS=1 LLVM_SYS_221_PREFIX=/usr/lib/llvm-22 cargo test --release -p perry-runtime --lib gc::tests::copy_slot_decode::sabotaged_remembering_arm_is_refused_by_the_coverage_cross_check -- --exact --nocapture (1 passed, 0 ignored; expected sabotage assertion observed)
  • RUST_TEST_THREADS=1 LLVM_SYS_221_PREFIX=/usr/lib/llvm-22 cargo test --release -p perry-runtime --lib (4,277 passed; 5 ignored)
  • production release compiler/runtime preflight build completed on perrymaster.skelpo.net
  • cargo fmt --all -- --check
  • python3 scripts/check_test_registration.py
  • scripts/check_file_size.sh

Checklist

  • I have NOT bumped the workspace version or edited CLAUDE.md / CHANGELOG.md (maintainer handles these at merge)
  • My commits follow the loose feat: / fix: / docs: / chore: prefix convention used in the log
  • I've read CONTRIBUTING.md and agree to the Code of Conduct

Summary by CodeRabbit

  • Tests
    • Release-mode unit tests now execute additional garbage-collection remembered-set coverage checks.
    • Cross-checks that were previously limited to debug builds now remain active during cargo test --release.
    • Test diagnostics provide clearer detection when remembered-set entries and restored dirty coverage disagree.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 87bdcb4b-b189-4440-b8a2-bc6cf51fee24

📥 Commits

Reviewing files that changed from the base of the PR and between f5cfbff and 40a0091.

📒 Files selected for processing (4)
  • changelog.d/11027-release-gc-sabotage.md
  • crates/perry-runtime/src/gc/sticky_remembered.rs
  • crates/perry-runtime/src/gc/tests/copy_slot_decode.rs
  • crates/perry-runtime/src/gc/verify.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

The GC remembered-set coverage cross-check now compiles in test builds, including release tests. The sabotage test no longer skips release configurations. Supporting names, comments, and the changelog describe this coverage.

Changes

GC release test coverage

Layer / File(s) Summary
Enable test-build dirty cross-checks
crates/perry-runtime/src/gc/sticky_remembered.rs, crates/perry-runtime/src/gc/verify.rs
The remembered-set coverage state, cross-check calls, and helper now compile under cfg(test) or debug_assertions. The helper is renamed to cross_check_covered_parent.
Run sabotage validation in release tests
crates/perry-runtime/src/gc/tests/copy_slot_decode.rs, changelog.d/11027-release-gc-sabotage.md
The sabotage test now runs in release test builds. The changelog records the updated coverage.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 40a00

The release sabotage witness is connected to the intended cross-check without changing production release behavior, so the change is mergeable.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: running the remembered coverage sabotage test in release builds.
Description check ✅ Passed The description includes all required sections, explains the implementation and purpose, links the related issue, and provides detailed test results and checklist confirmation.
Linked Issues check ✅ Passed Issue [#10604] requires a release-capable witness for the remembered-set coverage cross-check. The PR removes the release ignore from sabotaged_remembering_arm_is_refused_by_the_coverage_cross_check
Out of Scope Changes check ✅ Passed The changed runtime files implement the release-test coverage objective in [#10604]. The test comment, source comments, helper rename, and changelog fragment document or support that behavior. No unre…
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (1 skipped: 1 u…
✨ Finishing Touches 💡 1
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main in merge train 257 (#11039, v0.5.1640), main 990b3eeada.

Carried at head 40a00919ba. CI on the train head passed every job except the known public-baseline lint step: all 6 gap shards, cargo-test, e2e-scoped, gc-stress, check, warnings and security-audit green.

This train was split by blast radius after an earlier 35-PR assembly hit five gap regressions: it carries only PRs touching no lowering path. Trains rebase-merge, so commits get new SHAs and GitHub cannot mark this merged. Closed as landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(gc): #10491's sabotage twin cannot fail under cargo test --release — its observable is debug-only

1 participant