Skip to content

fix(regex): never memoize a never-match program beside a missing fancy fallback - #9799

Closed
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/regex-site-cache-incoherent-triple
Closed

fix(regex): never memoize a never-match program beside a missing fancy fallback#9799
proggeramlug wants to merge 1 commit into
PerryTS:mainfrom
proggeramlug:fix/regex-site-cache-incoherent-triple

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

The defect, on main today

REGEX_CACHE holds a never-match placeholder ([^\s\S]) for a pattern only
fancy-regex can match — a lookbehind, a backreference — with the real program
in FANCY_CACHE. The two maps are capped independently and each clear()s
wholesale on overflow (evict_regex_cache_if_full), so FANCY_CACHE can drop a
pattern that REGEX_CACHE still answers for.

When that has happened, nothing repopulates the fallback:

  • get_or_compile_regex returns early on a REGEX_CACHE hit;
  • compile_and_cache_regex_checked returns true immediately when the key is
    already in REGEX_CACHE.

So build_and_install_programs reads the pair (never-match, fancy: None)
which matches nothing.

On its own that state heals. The next REGEX_CACHE clear makes the pattern
recompile, and the recompile repopulates both maps. A site-cache entry does
not heal.
site_cache::install_programs remembers the triple against the
pattern text, and js_regexp_new then installs it eagerly on every later
construction of that text — regex_ptr non-null, so ensure_regex_compiled
short-circuits, lookup_fancy_regex returns None off the header, and the maps
are never consulted again. A lookbehind or backreference literal is then
permanently non-matching, silently, for the life of the thread.

The construction cache landed in the last merge train; this closes the window it
opened.

The desynchronisation is reachable, not theoretical

REGEX_CACHE also takes entries from ordinary patterns, so it can hit its cap
and clear on an ordinary insert while FANCY_CACHE keeps everything. From
there FANCY_CACHE reaches its own cap first, clears, and every fancy pattern
compiled since that earlier clear is left with a placeholder in REGEX_CACHE
and no fallback anywhere.

The change

  • The never-match placeholder becomes a process-wide singleton
    (never_match_program), so "is this entry a placeholder?" is an
    Arc::ptr_eq and not a pattern-text compare. Both places that used to build
    one now share it.
  • build_and_install_programs memoizes the triple only when it is coherent:
    never a placeholder with no fancy program beside it. Declining costs one
    recompile on a later construction and restores the self-healing behaviour.

Correctness

New test an_incoherent_program_pair_is_never_memoized_by_the_site_cache. It
builds a lookbehind, clears FANCY_CACHE the way the overflow guard does,
rebuilds, and asserts (a) the incoherent pair is not remembered against the text
and (b) the literal matches again once the maps heal.

Verified it can fail. With the guard's condition replaced by if true it
fails on its own named assertion — left: Some(true), right: Some(false)
and, without the first assertion, the recovered construction returns -1
instead of matching.

cargo test -p perry-runtime --release regex -- --test-threads=1: 99 passed,
0 failed.

Scope, and what this is NOT

This is the narrow guard: it stops the site cache making the breakage
permanent. It does not repair the underlying transient defect — a header built
during the incoherent window still carries a never-match program and no
fallback. #9796 item 2 fixes that properly, by rebuilding the missing fallback
beside the placeholder, and should land regardless of this PR. With it in place
this guard's condition can never be true and it degrades to a defensive
invariant, which is a fine thing for it to be.

One hole of the same shape is left open deliberately, and reported rather than
guessed at:
REPEAT_MATCHER_CACHE has the same independent cap. A cleared
repeat matcher is memoized as repeat: None beside a real std program, which
the placeholder test above cannot see, and lookup_repeat_matcher then answers
None for a pattern whose ECMA-262 RepeatMatcher capture semantics are
observable — wrong captures rather than no match. That belongs with #9796, which
already owns that function.

Reported by the regex lane (#9796 item 2); confirmed against this code before
being fixed.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an issue where certain lookbehind and backreference patterns could become permanently non-matching after internal cache eviction.
    • Improved cache consistency so affected regular expressions continue to recover and match correctly.
  • Tests

    • Added coverage for cache eviction scenarios to verify patterns are not incorrectly memoized as never matching.

…y fallback

Found by the regex lane (PerryTS#9796 item 2) against the construction cache added
earlier on this branch.

`REGEX_CACHE` holds a never-match placeholder for a lookbehind / backreference
pattern whose real program lives in `FANCY_CACHE`, and the two maps have
independent clear-on-overflow caps. `FANCY_CACHE` can therefore drop a pattern
`REGEX_CACHE` still answers for, and `build_and_install_programs` then reads
the pair (never-match, no fancy) — which matches nothing.

Left to the maps alone that state heals: the next `REGEX_CACHE` clear makes the
pattern recompile and repopulate both. A site-cache entry never heals — a
construction that hits it is born built and never consults the maps again — so
memoizing the incoherent pair makes a lookbehind literal PERMANENTLY
non-matching, silently, for the life of the thread. That permanence is this
branch's regression, and this is its fix: the triple is only remembered against
the text when it is coherent.

The placeholder becomes a process-wide singleton so the question is an
`Arc::ptr_eq` rather than a pattern-text compare.

This is the narrow guard, not the whole repair: PerryTS#9796 item 2 also REBUILDS the
missing fallback so the header itself stops mis-matching, which fixes the
underlying transient defect (present on main) that this only declines to make
permanent. Keep both; this one becomes a defensive invariant once that lands.
The same hole exists for `REPEAT_MATCHER_CACHE` — a cleared repeat matcher is
memoized as `repeat: None` beside a real std program, changing capture
semantics — and is reported to that lane rather than guessed at here.

Test: `an_incoherent_program_pair_is_never_memoized_by_the_site_cache` reads
`Some(true)` and then `-1` without the guard.
@coderabbitai

coderabbitai Bot commented Sep 5, 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: b28fec9d-6618-4603-9a0b-9daea765c17d

📥 Commits

Reviewing files that changed from the base of the PR and between 1d63fa9 and 54f69b1.

📒 Files selected for processing (4)
  • changelog.d/9764-regex-site-cache-coherence.md
  • crates/perry-runtime/src/regex.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; 5 remain after this review.


📝 Walkthrough

Walkthrough

The regex runtime now shares its never-match program and detects it by identity. Site-cache installation skips incoherent standard/fancy program pairs. Tests cover cache eviction, non-memoization, and recovery.

Changes

Regex cache coherence

Layer / File(s) Summary
Shared never-match program
crates/perry-runtime/src/regex.rs
The standard-engine regex uses Arc<Regex>. The never-match program is built once and shared. Pointer identity detects the placeholder, and test helpers clear each cache.
Coherent site-cache installation
crates/perry-runtime/src/regex/lazy.rs, crates/perry-runtime/src/regex/tests.rs, changelog.d/9764-regex-site-cache-coherence.md
Site-cache installation skips a never-match standard program when no fancy program exists. The regression test covers cache eviction and recovery. The changelog records the fix.

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

Merge Risk: ⚪ Minimal · up to 54f69

This change prevents lookbehind and backreference patterns from being permanently cached as non-matching after cache desynchronization, with regression coverage for recovery. It is ready to merge.

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preventing the site cache from memoizing a never-match regex program without its fancy-regex fallback.
Description check ✅ Passed The description provides a detailed summary, explains the concrete changes, identifies related issue #9796, and documents the regression test and verification command. It does not use the template hea…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files. (1 skipped: 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Same bug, and the diagnosis matches mine exactly — but this fix leaves half of it standing, and I think the half it leaves is the visible one.

Declining to memoize stops the incoherent triple from being inherited by later constructions. It does not stop the header that is being built right now from getting regex_ptr = <never-match placeholder> and fancy_ptr = null — and that header is not repaired later, because ensure_regex_compiled returns early as soon as regex_ptr is non-null. So in the bad window a /(?<=foo)bar/ object is born matching nothing and stays that way for its whole lifetime; only the next evaluation of the literal gets a working one. "The maps heal" is true for future compiles, not for the object in hand.

Two more things it does not cover:

  • REPEAT_MATCHER_CACHE has the identical shape. There the standard program is a real program, not the placeholder, so is_never_match_program never fires — the header is published with repeat_matcher_ptr = null and the regex matches with the linear engine's capture assignment instead of ECMA-262's RepeatMatcher semantics. Quieter symptom, same cause, and it is memoized too.
  • A pattern both engines rejected also gets the placeholder (get_or_compile_regex's final fallback), so is_never_match_program(std) && fancy.is_none() is true for it as well and its triple is never memoized — harmless, but it means the guard is not only firing on the case it names.

#9801 fixes it the other way round: repair the missing program in build_and_install_programs before the header is published and before the triple is memoized, for both caches. That establishes the invariant the header-authoritative lookups and the construction cache both already assume — "a built header carries every program its pattern needs" — and makes the guard here unnecessary, because an incoherent triple can no longer exist to be memoized.

Its test is the discriminator if you want to check this locally: a_single_program_cache_clear_cannot_disarm_a_lookbehind_literal asserts (*header).fancy_ptr is non-null on the header built in the bad window, not just that a later construction is fine. I also tried the narrower "clear the three caches as a group" fix and dropped it for the same reason yours has: it closes one route to the bad state without repairing the state, so that assertion still fails under it.

Happy either way as long as the header is repaired — if you would rather keep this PR as the landing vehicle, take the repair block from #9801's lazy.rs and I will close mine.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Closing in favour of #9801, which is strictly stronger and which I would rather have.

Same defect, same attribution — I reached it from #9764's side after the regex lane reported it, and my analysis agreed with theirs on the point that matters: the incoherence pre-dates ddbe0b126, which removed the FANCY_CACHE fallback in lookup_fancy_regex (correctly — it was a full pattern copy plus a SipHash per exec) and added install_programs. It did not open the hole; it stopped the hole healing itself.

What #9801 does that this does not:

  • it repairs the header being built rather than only declining to memoize it, so the RegExp object constructed inside the bad window matches correctly instead of being wrong for its whole lifetime — this PR leaves exactly that case broken;
  • it covers REPEAT_MATCHER_CACHE as well. I found that hole while writing this PR and reported it rather than fixing it (the placeholder test here cannot see it, because the standard program is genuine) — fix(regex): a built header must carry every program its pattern needs #9801 re-derives the repeat matcher, and its observation that repeat_matcher::compile returns immediately unless a capture sits under a quantifier is what makes that affordable.

Nothing here is worth carrying separately once #9801 lands. Its test covers the same lookbehind-after-a-FANCY_CACHE-clear case as mine and fails without the change, which is the property I cared about.

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.

1 participant