Skip to content

Expire memoized template digests when a component registers - #2709

Merged
joelhawksley merged 1 commit into
ViewComponent:mainfrom
erikaxel:vc-digest-registration-invalidates-cache
Sep 24, 2026
Merged

joelhawksley merged 1 commit into
ViewComponent:mainfrom
erikaxel:vc-digest-registration-invalidates-cache

Conversation

@erikaxel

@erikaxel erikaxel commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem

ViewComponent::CacheDigest.enabled? is !registry.empty?, and dependencies_in returns [] while it's false. The registry only fills as components are autoloaded. Under lazy loading — development, and any test environment without eager_load — a digest computed before the first component loads silently omits every component dependency, and ActionView::Digestor memoizes it in DetailsKey.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/views partial with a cache block around render SomeComponent.new, where SomeComponent includes ExperimentallyCacheable:

finder = -> { ActionView::LookupContext.new(ActionController::Base.view_paths) }
digest = ->(p) { ActionView::Digestor.digest(name: p, format: :html, finder: finder.call) }

digest.call("vc_cache_probe/_host")
# => "5d2c78aeba543f6517a00d9f27a4d790"   registry empty

SomeComponent                                 # referencing it registers the component
ActionView::LookupContext::DetailsKey.clear
digest.call("vc_cache_probe/_host")
# => "0190488ed202a6ee3b0a6227a1b6f9cc"   registry size 1

No files changed between those two calls.

Impact

Production is mostly safe, because eager_load = true loads 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.rb sets config.eager_load = true.

Fix

CacheDigest.register now 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_caches is public API in every supported Action View (7.1 through main). DetailsKey.clear also 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 on main.
  • test_registering_an_unchanged_component_leaves_memoized_digests_alone and test_registering_a_new_component_expires_memoized_digests (unit) — cover both branches of the new guard.

bundle exec rake passes on Rails 7.1, 7.2, 8.0 and 8.1. The only failures seen were pre-existing and unrelated: allocation-count assertions in RenderingAllocationsTest on 8.0, and the Ruby/Rails version-matrix fixture on 7.1 when run under a Ruby other than the pinned one.

@erikaxel

erikaxel commented Sep 2, 2026

Copy link
Copy Markdown
Contributor 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.
We are running around 1000 view components, and use them all they way, from layout and out.
This is the first of a couple of fixes that we will suggest based on implementing and testing cache in our project.

@erikaxel

erikaxel commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 High severity

Open (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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@erikaxel
erikaxel force-pushed the vc-digest-registration-invalidates-cache branch from 91e547e to 187f292 Compare September 23, 2026 07:10
@joelhawksley
joelhawksley merged commit 23ceae2 into ViewComponent:main Sep 24, 2026
19 checks passed
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.

3 participants