fix(app-router): omit provisional RSC cache state for query renders - #2488
Conversation
commit: |
db0bd6e to
dbad535
Compare
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: omit provisional RSC cache state for query renders
The change is small, well-scoped, and correctly reasoned. Verified:
-
Behavioral delta is RSC-only, as documented. For HTML requests,
!options.isRscRequest && hasRequestSearchParamswas already equal tohasRequestSearchParams(HTML requests haveisRscRequest === false), so the HTML path atapp-page-render.ts:1123is genuinely unchanged. Only the RSC finalizer path atapp-page-cache-finalizer.ts:239-241gains the omission. -
Header semantics are coherent.
applyPendingDynamicCdnHeadersrunsapplyCdnResponseHeaders(..., { pendingDynamicCheck: true }), which the default CDN adapter maps toCache-Control: no-store, must-revalidate(shims/cdn-cache.ts:149-153). DeletingX-Vinext-Cache/X-Nextjs-CachewhenomitCacheStateis set leaves a consistent "don't store, no advertised state" response.X-Vinext-Cacheis a diagnostic/CDN-state header and is not consumed by client navigation logic, so omitting it is safe. -
The omission only fires where it matters. In
finalizeAppPageRscCacheResponse, the early return at lines 234-236 meansomitPendingDynamicCacheStateis only applied whenpreserveClientResponseHeaders !== true, i.e. whenrscResponsePolicy.cacheState === "MISS"(app-page-render.ts:866). So this narrows exactly the provisional-MISS case the PR targets and leaves HIT/STALE/skip-transport untouched. -
hasRequestSearchParamsis the right signal. It is guarded by!isForceStatic(app-page-dispatch.ts:551), so force-static routes are correctly excluded from being treated as query-dynamic. There is a single caller ofrenderAppPageLifecycle, so no other wiring needs updating. -
Tests are meaningful. Both new tests exercise the real path: the render-lifecycle test gates the stream and flips
dynamicUsedduring consumption to prove that (a) the provisional state is omitted up front and (b) the cache write is skipped once dynamic usage is discovered. I ranvp test run tests/app-page-cache.test.ts tests/app-page-render.test.tslocally: 86 passed.
No blocking issues. A couple of non-blocking observations below.
| const clientHeaders = new Headers(response.headers); | ||
| applyPendingDynamicCdnHeaders(clientHeaders, options.getPageTags()); | ||
| applyPendingDynamicCdnHeaders(clientHeaders, options.getPageTags(), { | ||
| omitCacheState: options.omitPendingDynamicCacheState === true, |
There was a problem hiding this comment.
Non-blocking: the omission is only meaningful here because this branch runs with pendingDynamicCheck: true (Cache-Control becomes no-store, must-revalidate). A one-line comment noting that invariant — that deleting the state headers is safe specifically in the pending-dynamic/no-store case — would help a future reader understand why this does not leave a stored-but-untagged response.
| handlerStart: options.handlerStart, | ||
| hasLoadingBoundary: hasActiveLoadingBoundary, | ||
| omitPendingDynamicCacheState: !options.isRscRequest && hasRequestSearchParams, | ||
| omitPendingDynamicCacheState: hasRequestSearchParams, |
There was a problem hiding this comment.
Confirmed this now covers RSC requests too. Since AGENTS.md requires verifying Next.js parity for cache-behavior changes and there is no .nextjs-ref clone available here, it would be worth confirming (and noting in the PR description) that Next.js also does not advertise a shared-cache state on query-bearing raw RSC (.rsc) responses whose searchParams usage is only discovered during Flight consumption. The reasoning in the PR is sound (searchParams access can happen during lazy stream consumption), but a link to the corresponding Next.js source/test would lock in the parity claim per the repo guidelines.
|
Review posted successfully to PR #2488. SummaryI reviewed PR #2488 ( Verdict: no blocking issues. The change is small, well-scoped, and correctly reasoned. Key findings from my analysis:
The two non-blocking comments suggest: (a) a clarifying comment about the pending-dynamic/no-store invariant, and (b) documenting the Next.js parity confirmation per AGENTS.md guidelines (no |
Overview
omitPendingDynamicCacheStatethrough RSC finalization and enable it whenever the request has search params.searchParamsusage only while the Flight stream is consumed, so the response should not expose a provisional cache state first.packages/vinext/src/server/app-page-cache-finalizer.ts,packages/vinext/src/server/app-page-dispatch.ts,packages/vinext/src/server/app-page-render.tsWhy
Cache headers should represent a completed cache decision, not a guess made before stream-time request API usage is known. Query-bearing raw RSC responses are especially sensitive because
searchParamsaccess may happen during lazy Flight consumption. This PR reuses the existing pending cache-state omission for RSC query requests so provisional state does not leak to the client.What changed
Maintainer review path
packages/vinext/src/server/app-page-dispatch.tsomitPendingDynamicCacheState.packages/vinext/src/server/app-page-render.tspackages/vinext/src/server/app-page-cache-finalizer.tstests/app-page-cache.test.tstests/app-page-render.test.tsValidation
vp test run tests/app-page-cache.test.ts tests/app-page-render.test.tsvp check packages/vinext/src/server/app-page-cache-finalizer.ts packages/vinext/src/server/app-page-dispatch.ts packages/vinext/src/server/app-page-render.ts tests/app-page-cache.test.ts tests/app-page-render.test.tsRisk / compatibility
Non-goals