Expire memoized template digests when a component registers - #2709
Open
erikaxel wants to merge 1 commit into
Open
Expire memoized template digests when a component registers#2709erikaxel wants to merge 1 commit into
erikaxel wants to merge 1 commit into
Conversation
- Return early from CacheDigest.register when the entry is unchanged, so reloads and re-registrations don't churn - Clear ActionView's digest caches on a new registration, leaving resolver caches alone - Add a regression test asserting a fragment digest is the same whether the component registered before or after the template was digested
Author
|
Hi, as probably can be deduced from the PR description itself, this was created with the help of Claude. The background is that we are testing the new experimental cache support landed in #2685 and released in 4.15.0. |
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.
Problem
ViewComponent::CacheDigest.enabled?is!registry.empty?, anddependencies_inreturns[]while it's false. The registry only fills as components are autoloaded. Under lazy loading — development, and any test environment withouteager_load— a digest computed before the first component loads silently omits every component dependency, andActionView::Digestormemoizes it inDetailsKey.digest_cache, so the wrong value sticks for the life of the process.The digest is therefore not a pure function of the source: same files, same code, different answer depending on load order.
Reproduction
An
app/viewspartial with acacheblock aroundrender SomeComponent.new, whereSomeComponentincludesExperimentallyCacheable:No files changed between those two calls.
Impact
Production is mostly safe, because
eager_load = trueloads every component at boot. Development and test are not: the same fragment cache block can key differently between two boots, and a fragment digested early in a process silently loses its component dependencies. It also makes the feature confusing to evaluate — this is what made our first attempt to verify the advertised behaviour look like the feature simply did not work.The sandbox suite doesn't catch it because
test/sandbox/config/environments/test.rbsetsconfig.eager_load = true.Fix
CacheDigest.registernow expires Action View's memoized digests when it adds a new entry, so a digest computed before a component registered is recomputed rather than served from memory. Re-registering an unchanged component returns early, so reloads and repeated registrations don't churn.The reset is deliberately narrow:
DetailsKey.digest_caches.each(&:clear)drops memoized digests only and leaves resolver caches alone, since no template changed — only the set of dependencies the Digestor can see.DetailsKey.digest_cachesis public API in every supported Action View (7.1 throughmain).DetailsKey.clearalso works but throws away resolver caches for no reason.Registration only happens on class load, so the churn is bounded and stops once everything has loaded.
Alternative considered
Eagerly registering every component at boot instead. That is harder to do correctly under lazy loading, since it means discovering components independently of the autoloader, which is why invalidating on register looks like the better trade.
Tests
test_digest_does_not_depend_on_when_the_component_registered(integration) — a fragment digest is identical whether the component registered before or after the template was first digested. Fails onmain.test_registering_an_unchanged_component_leaves_memoized_digests_aloneandtest_registering_a_new_component_expires_memoized_digests(unit) — cover both branches of the new guard.bundle exec rakepasses on Rails 7.1, 7.2, 8.0 and 8.1. The only failures seen were pre-existing and unrelated: allocation-count assertions inRenderingAllocationsTeston 8.0, and the Ruby/Rails version-matrix fixture on 7.1 when run under a Ruby other than the pinned one.