Expire memoized template digests when a component registers - #2709
joelhawksley merged 1 commit into
Conversation
|
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. |
|
@reeganviljoen Are you willing to take a look at this? (since you were involved in the original cache PRs). I also have some other PRs up with fixes or improvements for caching, and would love your thoughts on those as well, but I think this is the first one that should be merged. @joelhawksley, please let me know if there is anything else you need. The PR adds some tests, but do you also need a more concrete example in a sample repo? The fix itself is quite straight forward. I wondered if perhaps it should be gated to only run when lazy loading is on, since this is not a problem for eager loading. Would love to hear your opinion on that. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The critical registration/invalidation race can leave stale digests memoized.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Fixes stale Action View template digests when cacheable components load lazily.
Changes:
- Invalidates memoized digests when new components register.
- Preserves caches for unchanged registrations.
- Adds tests and changelog coverage.
A critical concurrency race remains: an in-flight digest can be memoized after invalidation using the old registry state.
| File | Description |
|---|---|
test/sandbox/test/experimentally_cacheable_test.rb |
Tests registration and cache invalidation. |
test/sandbox/test/experimentally_cacheable_integration_test.rb |
Tests load-order-independent digests. |
lib/view_component/cache_digest.rb |
Adds guarded invalidation; concurrent digest computation remains unsafe. |
docs/CHANGELOG.md |
Documents the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # caches alone: no template changed, only the set of dependencies the | ||
| # Digestor can see. | ||
| def expire_digests | ||
| ActionView::LookupContext::DetailsKey.digest_caches.each(&:clear) |
There was a problem hiding this comment.
Hi, I don't think this should be fixed:
- It does not affect production, since it eager-loads
- For development the remaining race needs two concurrent requests during the very first cacheable-component load. A stale entry then lives until Action View next clears DetailsKey (a change under a view path) or the process restarts.
Below is a longer explanation of what we would need to do to fix it. Let me know what you decide.
When it can produce a wrong digest. Only while the feature is switching on: the first cacheable component registering while another thread is mid-digest. Once the registry is non-empty, DependencyTracking scans every template, and constantize_component autoloads any component it finds before returning its path. By the time a path is in the tree its component has registered, so an in-flight digest that straddles a later registration is already correct, and memoizing it after the clear is harmless.
Why not take the Digestor’s lock. register runs from included/inherited, during an autoload, while Ruby holds the require lock for that file. A digesting thread holds the digest mutex and, once the tracker is installed, may safe_constantize the same component and block on that require lock, so the two threads wait on each other. On a single thread, the scan’s own autoload re-enters register while the mutex is already held, which raises ThreadError. The mutex is also private and has changed shape (@@digest_mutex through 8.1, a Ractor-local digest_mutex on main).
Closing it fully would mean wrapping ActionView::Digestor.digest with a registry generation check and retry, installed at boot for every app rather than on the first include. A thread already inside digest when the first component loads would never pass through a wrapper installed later. That runs against the current design, where nothing is installed until a component opts in, so I’ve left it out. Happy to add it if you’d prefer the stronger guarantee.
- 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
91e547e to
187f292
Compare

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.