fix(app-router): keep mounted-slot RSC MISS responses no-store - #2728
Open
NathanDrake2406 wants to merge 1 commit into
Open
fix(app-router): keep mounted-slot RSC MISS responses no-store#2728NathanDrake2406 wants to merge 1 commit into
NathanDrake2406 wants to merge 1 commit into
Conversation
finalizeAppPageRscCacheResponse derived "should I rewrite the client headers?" from the return value of scheduleAppPageRscCacheWrite. Those are independent decisions, and cloudflare#2497 made them disagree: mounted-slot variants now correctly skip the persistent write (their RSC key is slot-blind), but the early return took the pending-dynamic finalization with it. The result is that a fresh ISR-eligible RSC MISS carrying X-Vinext-Mounted-Slots leaves the origin with its initial `s-maxage=..., stale-while-revalidate` instead of being rewritten to `no-store, must-revalidate`. That header is what stops a shared cache from storing a stream that may still reach cookies()/headers() below a Suspense boundary after the cache policy was chosen, so a personalized payload can be stored and replayed for the URL/variant. Apps with named parallel routes send the header on essentially every client navigation; apps without slots never enter the path. Gate the header rewrite on preserveClientResponseHeaders alone, which is already `cacheState !== "MISS"` at the only production call site. This restores the client-facing behavior that shipped before cloudflare#2497 while keeping its cache-write change, and matches finalizeAppPageHtmlCacheResponse, which never coupled the two. Doing it structurally rather than adding a mountedSlotsHeader term means the next early return added to scheduleAppPageRscCacheWrite cannot silently reintroduce this.
commit: |
Contributor
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
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
1 participant
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.
What
finalizeAppPageRscCacheResponsederives "should I rewrite the client-facing headers?" from the return value ofscheduleAppPageRscCacheWrite. Those are independent decisions, and #2497 made them disagree.That PR correctly stopped persisting mounted-slot RSC variants — their RSC cache key is slot-blind (
isrRscKey(pathname, null, ...)), so writing a slot-specific payload under it would poison the store. But it expressed that as a new early return inscheduleAppPageRscCacheWrite, and the finalizer'sif (!didSchedule) return responsetook the pending-dynamic header rewrite down with it.Effect
A fresh ISR-eligible RSC MISS carrying
X-Vinext-Mounted-Slotskeeps thes-maxage=..., stale-while-revalidateit was built with, instead of being finalized tono-store, must-revalidate:no-store, must-revalidateslot:auth:/s-maxage=60, stale-while-revalidateno-store, must-revalidateconsumeDynamicUsage()runs atapp-page-render.ts:817, before the shell has flushed, so a MISS response has not yet proven its render was non-dynamic — that is what the provisionalno-storeis for. Skipping it for slot variants was not intended by #2497, which only meant to change the persistent write.Fix
Gate the header rewrite on
preserveClientResponseHeadersalone. At the only production call site that is alreadyrscResponsePolicy.cacheState !== "MISS"(app-page-render.ts:913), so this is a no-op for every non-MISS response and correct-by-construction for MISS.Structural rather than adding a
mountedSlotsHeaderterm to the condition:scheduleAppPageRscCacheWritealready has three early returns, and the next one added would silently reintroduce this.finalizeAppPageHtmlCacheResponsenever coupled the two — this brings the RSC path back in line with it.Restores the client-facing behavior that shipped before #2497 while keeping its cache-write change, so there is no CDN hit-rate tradeoff: those responses were already
no-storeon the client leg.Verification
tests/app-page-cache.test.tsasserts both halves — the slot variant is finalized tono-storeand still never reachesisrSet. Confirmed red without the source change (expected 's-maxage=60, stale-while-revalidate' to be 'no-store, must-revalidate'), green with it.tests/app-page-cache.test.ts(39),tests/app-page-dispatch.test.ts(67),tests/app-router-production-server.test.ts(73) pass.pnpm run checkclean.No changeset added — happy to add one if this warrants a release note.