Repository navigation
fix(loader): don't let a cached loader's inner invokes veto page caching - #1244
pedrofrxncx wants to merge 1 commit into
Conversation
A loader with stale-while-revalidate and a non-null cacheKey runs its handler with the request's shared ctx on a cache miss. Loaders it calls through ctx.invoke build their fn context from the same request state, so a no-store or null-key inner loader sets vary.shouldCache = false and the whole page goes no-store, but only on the outer loader's miss: on a hit the inner loaders never run. The veto never protected the cached data (it is served to every request on a hit regardless); it only made page caching depend on loader-cache state. Run callHandlerAndCache inside an AsyncLocalStorage scope carrying its own Vary (RequestContext.vary). fnContextFromHttpContext uses it for any loader resolved inside that scope, so inner keys and vetoes land in the scoped vary, while debug entries still go to the page. Loaders running outside the scope, including siblings that run concurrently, keep the request's vary, so their vetoes still apply. Non-cacheable paths are unchanged.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. Comment |
Tagging OptionsShould a new tag be published when this PR is merged?
|
End-to-end validation on a site previewI pinned miess-01 to Bots: Googlebot UA, cache-busted PDPs (deferred sections render inline)
The one PDP that stays Control set, browser UA: home, 10 PLPs, 5 PDPs, 3 searches and 3 legacy Deferred sections, human path: No regressions found. Still open: a real deploy, to watch the bot PDP BYPASS rate drop. |
This branch has not been deployed
1 participant
Problem
A loader with
cache = "stale-while-revalidate"and a non-nullcacheKeyrunshandler(props, req, ctx)with the request's shared context on a cache miss (callHandlerAndCache). Loaders it calls throughctx.invokebuild theirFnContextfrom the samestate.global, so they share the page'svary. If one of them isno-store, or returns anullkey, it setsvary.shouldCache = falseand the whole page goesno-store.This only happens on the outer loader's miss. On a hit the inner loaders never run, so page cacheability flips with loader-cache state.
Real case (a VTEX storefront): a site loader
relatedProductList(SWR, key = product slug) invokesvtex/loaders/legacy/productDetailsPage,intelligentSearch/productList(key isnullwhenctx.isInvoke) andlegacy/productList(nocacheexport, so it defaults tono-store). Bots render deferred sections inline, so every bot PDP whose related-products entry is cold comes backno-store: about 19k requests/day at ~1.3s each of origin time on that one site. For human visitors, the same veto hits that section's/deco/render.Change
RequestContextgets an optionalvary.callHandlerAndCacheruns the handler insideRequestContext.bind({ ...active, vary: scoped }), and also passes{ ...ctx, vary: scoped }to the handler itself.fnContextFromHttpContextusesRequestContext.active()?.varywhen present. Every loader resolved inside that async scope, invokes included, pushes keys and vetoes into the scoped vary. Debug entries still forward to the page'svary.debug, so?__dstill shows them.no-store,no-cache,null-key and cache-disabled paths are untouched.Is it correct to cache the outer result when an inner loader said no-store?
I think so. The outer loader's output is written to the loader cache and served to every request on a hit, and no veto runs on a hit. The inner veto therefore never protected the data; it only made the single miss request
no-store. The outercacheKeyalready owns correctness (it's what other requests are served by). This change makes a miss behave like a hit and adds no new exposure: anything an outer loader leaks is already leaked on hits today.I checked deco-cx/apps for cacheable loaders that invoke others (
vtex/loaders/options/productIdByTerm,wake/loaders/productDetailsPage,wake/loaders/productListingPage). The wake PLP returns anullkey when a partner cookie is present, so it stays on the uncached path, and itspartnerAssociatecall is an action, which never sets vary. Nothing in apps writesctx.varydirectly.Inner vary keys are also scoped now. They were already only present on misses, so dropping them makes the page vary stable across hit and miss; the outer key is pushed as before.
Tests
blocks/loader.test.ts, which goes throughloaderBlock.adaptwith fake HttpContexts that share request state, the way the resolver does for invokes:no-storeloader leaves pageshouldCachetrue. This fails on main.no-storeloader that vetoes while the cacheable loader's handler is in flight still vetoes the page.no-storeorno-cacheouter loader still propagates the inner veto.Full suite: 87 passed, 0 failed.
deno check live.tsis clean, anddeno fmt --check/deno lintare clean on the touched files. The 12 unformatted files in the repo were already unformatted on main.Draft until it has been checked end to end on a site preview; results to follow in a comment.
Summary by cubic
Fixes page caching being vetoed by loaders invoked inside a cacheable loader's handler, so a miss now behaves like a hit.
Previously, a cacheable loader (
stale-while-revalidatewith acacheKey) that invoked inner loaders viactx.invokeran them with the request's shared vary on a cache miss. If any inner loader wasno-storeor returned a null key, it set the page's vary to no-store—but only on that miss, because hits skip inner loaders. This made page cacheability depend on loader-cache state.This change scopes each cacheable loader's handler run to its own
VaryviaRequestContext, so inner loaders push keys and vetoes into that scoped vary instead of the page's. Siblings running concurrently on the same request are outside the scope and still veto the page. Non-cacheable loaders (no-store/no-cache) propagate vetoes as before, and debug entries still surface in the page's vary. Adds tests covering the scoped behavior, the sibling mid-flight veto, and the unchanged non-cacheable paths.Written for commit 1d6059f. Summary will update on new commits.