fix(app-router): limit mounted slot cache variants - #2497
Merged
Conversation
Member
Author
|
/bigbonk review for issues |
commit: |
Contributor
|
Contributor
|
The review is complete. Let me summarize my findings. The changes are correct and well-tested; I found no blocking issues, only minor observations. Review SummaryI reviewed PR #2497 (
What the change does (correctly)
Minor observations (non-blocking)
No correctness, parity, or security concerns found. LGTM. |
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 |
james-elicx
force-pushed
the
codex/limit-mounted-slot-cache-variants
branch
from
July 2, 2026 22:30
75107ae to
e081409
Compare
Member
Author
|
/bigbonk review for issues |
Contributor
Bonk review — PR #2497
|
Contributor
|
Review posted to PR #2497. SummaryReviewed Verified:
Minor (non-blocking): one dead |
james-elicx
force-pushed
the
codex/limit-mounted-slot-cache-variants
branch
from
July 2, 2026 22:35
e081409 to
b5d10cb
Compare
Member
Author
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Bonk review — PR #2497 fix(app-router): limit mounted slot cache variants
Traced both cache paths to their callers, verified header/normalization invariants, checked dev/prod parity, and ran the touched suites plus related response/ISR tests. No blocking issues — LGTM.
Verification
vp test run tests/app-page-cache.test.ts tests/app-page-dispatch.test.ts tests/app-rsc-cache-busting.test.ts tests/app-rsc-request-normalization.test.ts→ 196 passed.vp test run tests/app-page-response.test.ts tests/app-rsc-response-finalizer.test.ts tests/isr-cache.test.ts→ 110 passed.vp checkclean (format + lint + types) on all three modified source files.
What the change does correctly
- Read guard (
app-page-cache.ts:333) short-circuits mounted-slot RSC requests toMISSbeforeisrGet/isrRscKey, and falls through to a fresh render. The early return skipsclearRequestContext(), which is correct and consistent with the otherMISSreturns in this function — the request context must stay live for the fresh render downstream. - Write guard (
app-page-cache-finalizer.ts:254) bails mounted-slot variants out of persistent RSC writes. Verified parity: the other caller offinalizeAppPageRscCacheResponse(app-page-render.ts:874, the fresh-render path) passesmountedSlotsHeaderthrough (:903), so it inherits the same guard — there is no unguarded RSC write path. - Canonical HTML-triggered regen (
app-page-cache.ts:439) now writes the RSC key withnullmounted slots. Real hardening: a forgedX-Vinext-Mounted-Slotsheader on an HTML request can no longer poison the canonical RSC entry. Matches the pre-existing HTML finalizer path (app-page-cache-finalizer.ts:132), so both HTML→RSC write paths agree. - Vary header drops
VINEXT_MOUNTED_SLOTS_HEADER. Safe: the header still feeds the_rsccache-busting hash (app-rsc-cache-busting.ts:185), so slot state still differentiates the request URL; theVaryentry was redundant now that variants are never persistently cached and can't be cross-served by a URL-keyed CDN.
Invariant checks (verified consistent)
- Truthiness vs. normalization: the guards test raw
options.mountedSlotsHeadertruthiness whileappIsrRscKeyrunsnormalizeMountedSlotsHeader. These can't diverge because the pipeline normalizes the header at request-normalization (app-rsc-request-normalization.ts:133→normalizeMountedSlotsHeader, which returnsnullfor empty/malformed input,app-mounted-slots-header.ts:53) before it reaches either helper. By the time the guards run, the value is already a valid normalized string ornull. - CDN header behavior change (intended, verified by tests): before this PR, a production mounted-slot RSC
MISSrender hadcapturedRscDataPromiseset (shouldCaptureRscForCacheMetadatadoes not gate on slots), soscheduleAppPageRscCacheWritereturnedtrueandapplyPendingDynamicCdnHeaderswas applied (CDN cache-tags +X-Vinext-Cache: MISS). Now the mounted-slots guard makesdidSchedulefalse, sofinalizeAppPageRscCacheResponsereturns the response untouched and those shared-cache CDN headers are no longer emitted. This is correct — mounted-slot responses are per-client and must not carry shared-cache tag/state headers — and is exactly what the dispatch test now asserts (x-vinext-cache→null).
Minor (non-blocking)
readAppPageCacheResponsestill passesoptions.mountedSlotsHeaderintoisrRscKeyat:341, but the early return at:333guarantees it's falsy for RSC requests by then. Harmless no-op; could benullto make the invariant explicit and match thenullalready passed at:439and in the write path. Pure readability.- The dispatch test change (fresh-rendered slot variants now carry request search params, e.g.
tab=popular) is a user-visible correctness improvement beyond caching — worth a one-line PR-body mention.
No correctness, parity, or security concerns.
Contributor
|
Review posted to PR #2497. SummaryReviewed Verified:
Notable finding I dug into: a real (intended) behavioral change — mounted-slot RSC Minor (non-blocking): the dead |
james-elicx
marked this pull request as ready for review
July 2, 2026 22:41
Member
Author
|
/bigbonk review for issues |
Contributor
There was a problem hiding this comment.
Bonk review — PR #2497 fix(app-router): limit mounted slot cache variants
Traced both cache paths to all callers, verified the header-normalization and Vary/cache-busting invariants, checked dev/prod parity across every RSC cache entry point, and ran the touched suites. No blocking issues — LGTM.
Verification
vp test run tests/app-page-cache.test.ts tests/app-page-dispatch.test.ts tests/app-rsc-cache-busting.test.ts tests/app-rsc-request-normalization.test.ts→ 196 passed.vp checkclean (format + lint + types) on all three modified source files.- Confirmed there is no unguarded RSC cache read/write path:
readAppPageCacheResponse,scheduleAppPageRscCacheWrite, andfinalizeAppPageRscCacheResponselive only in the sharedserver/app-page-*helpers, and every caller (app-page-dispatch.ts,app-page-render.ts:874fresh-render path,app-server-action-execution.tsaction re-render viabuildPageElement) routes through them and forwardsmountedSlotsHeader. App Router prod/worker entries delegate to the built RSC entry, so they inherit the guards.
What the change does correctly
- Read guard (
app-page-cache.ts:333) short-circuits mounted-slot RSC requests toMISSbeforeisrGet/isrRscKey, then falls through to a fresh render. The early return correctly skipsclearRequestContext()(the request context must stay live for the downstream fresh render), consistent with the otherMISSreturns in this function. - Write guard (
app-page-cache-finalizer.ts:254) bails mounted-slot variants out of persistent RSC writes; the sole other caller (app-page-render.ts:903) inherits it. - Canonical HTML-triggered regen (
app-page-cache.ts:439) now writes the RSC key withnullmounted slots. Real hardening: theslot:forged:/test (tests/app-page-cache.test.ts:655) confirms the write lands onrsc:/stale-html:none, so a forgedX-Vinext-Mounted-Slotsheader on an HTML request can no longer poison the canonical RSC entry. Matches the pre-existing HTML finalizer path (app-page-cache-finalizer.ts:132), so both HTML→RSC write paths agree. - Vary header drops
VINEXT_MOUNTED_SLOTS_HEADER. Safe: the header still feeds the_rsccache-busting hash (app-rsc-cache-busting.ts:185), so slot state still differentiates the request URL; theVaryentry was redundant now that variants are never persistently cached. TheVINEXT_MOUNTED_SLOTS_HEADERimport remains used (lines 185, 299), so no dead import.
Invariant checks (verified consistent)
- Truthiness vs. normalization: the guards test raw
options.mountedSlotsHeadertruthiness whileappIsrRscKeyrunsnormalizeMountedSlotsHeader. These can't diverge — the pipeline normalizes the header once at request-normalization (app-rsc-request-normalization.ts:133→normalizeMountedSlotsHeader, which returnsnullfor empty/malformed input) before it reaches any of these helpers. By the time the guards run, the value is already a valid normalized string ornull. - CDN header behavior change (intended, verified by tests): a mounted-slot RSC
MISSrender now returns untouched fromfinalizeAppPageRscCacheResponse(didScheduleisfalse), so it no longer emitsapplyPendingDynamicCdnHeaders(CDN cache-tags +X-Vinext-Cache: MISS). This is correct — those responses are per-client and must not carry shared-cache tag/state headers — and is exactly what the updated dispatch test asserts (x-vinext-cache→null).
Minor (non-blocking)
readAppPageCacheResponse:341still passesoptions.mountedSlotsHeaderintoisrRscKey, but the early return at:333guarantees it's falsy for RSC requests by then. Harmless no-op; could benullto make the invariant explicit and match thenullalready passed at:439and in the write path. Pure readability.- The dispatch test change (fresh-rendered slot variants now carry request search params, e.g.
tab=popular, instead of serving a stale empty-search variant) is a user-visible correctness improvement beyond caching — worth a one-line PR-body mention.
No correctness, parity, or security concerns.
Contributor
|
Review posted to PR #2497. SummaryReviewed Verified:
Minor (non-blocking): one guaranteed-no-op |
Member
Author
|
Follow-up on the latest Bonk minor notes: current head already passes |
james-elicx
force-pushed
the
codex/limit-mounted-slot-cache-variants
branch
from
July 2, 2026 23:01
b5d10cb to
ee367b5
Compare
Member
Author
|
/bigbonk review for issues |
Contributor
|
The test changes are meaningful and strengthen the assertions:
These are proper positive assertions, not weakened. The review is complete. Review SummaryI reviewed PR #2497 ( Verified:
One documentation discrepancy (non-blocking, no code impact):
No correctness, parity, or security concerns. LGTM. |
This was referenced Jul 27, 2026
james-elicx
added a commit
that referenced
this pull request
Aug 3, 2026
* fix(app-router): keep mounted-slot RSC MISS responses no-store finalizeAppPageRscCacheResponse derived "should I rewrite the client headers?" from the return value of scheduleAppPageRscCacheWrite. Those are independent decisions, and #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 #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. * fix(app-router): keep mounted slots out of edge caches * fix(cache): clear mounted-slot CDN overrides * docs(cache): explain mounted-slot no-store scope * fix(cache): clear pending CDN overrides * docs(cache): clarify pending header policy * test(cache): cover dynamic mounted-slot headers * test(cache): cover pending HTML CDN overrides --------- Co-authored-by: James <james@eli.cx>
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.
Summary
Tests
Review