Skip to content

fix(runtime): release compiled regex programs during GC - #9684

Closed
proggeramlug wants to merge 2 commits into
mainfrom
fix/9678-regex-arc-leak
Closed

fix(runtime): release compiled regex programs during GC#9684
proggeramlug wants to merge 2 commits into
mainfrom
fix/9678-regex-arc-leak

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Release the compiled regex programs owned by a RegExpHeader when the GC finalizes that header. This closes the permanent per-pattern leak reported in #9678.

Changes

  • Reconstruct and drop the header-owned standard, fancy, and RepeatMatcher Arc references during true GC finalization.
  • Null the raw program pointers before dropping them so overlapping cleanup remains idempotent.
  • Keep moved arena-stub cleanup metadata-only, transferring ownership to the relocated live header instead of freeing its programs.
  • Add a regression test that verifies all three owned references lose exactly one strong count and repeated finalization does not double-release them.

Related issue

Fixes #9678

Test plan

  • cargo build --release clean
  • cargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windows passes
  • CARGO_BUILD_JOBS=1 RUST_TEST_THREADS=1 cargo test --lib -p perry-runtime (3,069 passed, 4 ignored)
  • cargo test -p perry-runtime regex::tests:: (60 passed)
  • cargo test -p perry-runtime gc::tests:: (930 passed, 1 ignored)
  • cargo check -p perry-runtime --no-default-features
  • ./scripts/pre-tag-check.sh --quick
  • Added a #[test] in the affected crate
  • Docs updated (not applicable: no user-facing API change)
  • Platform UI backend built (not applicable)

./scripts/test_affected_crates.sh --base origin/main completed the entire single-threaded runtime phase successfully, then encountered an existing failure in the unchanged perry crate: PERRY_CONCAT_SITE_CACHE is missing from the build-cache environment-variable inventory. The same focused test fails on origin/main, and this branch has no diff under crates/perry.

Screenshots / output

Not applicable.

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

  • Bug Fixes

    • Regular expression programs are now properly released when their JavaScript owners are collected by garbage collection.
    • Improved cleanup across standard, fallback, and RepeatMatcher regular expression engines.
    • Preserved regular expression program ownership during live memory moves.
  • Tests

    • Added coverage verifying complete cleanup and safe repeated finalization of regular expression programs.

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 93fdb7c1-9cd5-4842-863f-ee4a028a8234

📥 Commits

Reviewing files that changed from the base of the PR and between f102c61 and c3a7dab.

📒 Files selected for processing (6)
  • changelog.d/9684-regex-program-finalization.md
  • crates/perry-runtime/src/gc/types.rs
  • crates/perry-runtime/src/regex.rs
  • crates/perry-runtime/src/regex/compile.rs
  • crates/perry-runtime/src/regex/lazy.rs
  • crates/perry-runtime/src/regex/tests.rs

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


📝 Walkthrough

Walkthrough

RegExp GC finalization now releases header-owned compiled programs for standard, fancy-regex, and RepeatMatcher implementations. The finalizer clears raw pointers and dead-header metadata. Tests verify release counts and prevent double release.

Changes

RegExp GC cleanup

Layer / File(s) Summary
RegExp finalizer and GC wiring
crates/perry-runtime/src/regex.rs, crates/perry-runtime/src/gc/types.rs, crates/perry-runtime/src/regex/compile.rs, crates/perry-runtime/src/regex/lazy.rs, changelog.d/9684-regex-program-finalization.md
The GC hook now invokes regex_header_finalize_for_gc. The finalizer releases standard, fancy-regex, and RepeatMatcher Arc references, nulls their pointers, and clears dead-header metadata. Comments and the changelog describe the updated ownership.
Finalizer release validation
crates/perry-runtime/src/regex/tests.rs
Tests verify Arc strong-count decreases, pointer nulling, support for all three engines, and safe repeated finalization.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to c3a7d

RegExp objects now release their compiled standard, fancy, and RepeatMatcher programs when collected, preventing retained program memory after their JavaScript owners die. The implementation includes coverage for release and repeated finalization, with no active merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: releasing compiled regex programs during GC.
Description check ✅ Passed The description includes the required summary, changes, related issue, test plan, output section, and checklist. It also documents the unrelated existing test failure and marks non-applicable items ap…
Linked Issues check ✅ Passed The PR implements the primary coding objective in [#9678] by releasing the standard, fancy, and RepeatMatcher Arc references during true GC finalization. It also preserves ownership during arena mov…
Out of Scope Changes check ✅ Passed The changes are within scope for [#9678]. The implementation, documentation updates, changelog fragment, and regression tests directly support GC finalization of compiled regex programs.
Full details: Docstring Coverage

Explanation

Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 5 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/9678-regex-arc-leak

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 via merge train #9687 (rebase-merge, authorship preserved). Your test-plan boxes were unchecked so I validated independently, including a PERRY_GC_FORCE_EVACUATE=1 regex arm (96/0) to exercise the moved-stub path — the metadata-only stub handling and null-before-drop both hold up.

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

Labels

None yet

Projects

None yet

1 participant