fix(cache): match segment cache staleness semantics - #2860
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(cache): match segment cache staleness semantics
I reviewed all 25 changed files, ran the targeted suites, and type/lint-checked the changed source. No blocking issues found — the change is well-structured and the test coverage is strong (ports the relevant Next.js E2E scenarios and adds focused unit tests for each new helper).
Verification
vp test run tests/shims.test.ts tests/slot.test.ts tests/app-page-dispatch.test.ts tests/app-visited-response-cache.test.ts→ 1401 passedvp test run tests/link-navigation.test.ts tests/app-page-route-wiring.test.ts→ 161 passedvp checkon all six substantial source files → clean (format + lint + types)
What I looked at closely
1. app-page-dispatch.ts — warmup isolation via runWithUnifiedStateMutation. Replacing discardAppPageRenderState() (warm in the shared context, then consume/clear afterward) with a scoped child context that resets the reference-typed slices before warm() is a genuine improvement. The reset set (cacheableFetchUrls, currentRequestTags, dynamicFetchUrls, dynamicUsageDetected, invalidDynamicUsageError, renderRequestApiUsage, requestScopedCacheLife, unstableCacheObservations) is a superset of what the old discard consumed, so no observation can leak from warmup into the final render. It also correctly follows the replace-not-mutate contract documented on the ALS helper.
2. cache-runtime.ts — PPR warmup cache result reuse. The dynamic-life branch intentionally suspends without publishing tags/cache-life (excluding short-lived Cache Components holes from the route prefetch expiry), and the non-dynamic branch propagates tags + records cache-life. The isDynamicCacheLife/cacheControlToCacheLife extraction matches the existing inline revalidate === 0 || expire < DYNAMIC_EXPIRE checks. Suspensions are kept outside the decode try/catch so an abort rejection is not mistaken for a corrupt entry. cacheLife() no longer eagerly publishing to the request scope is the correct pairing for this — the wrapper publishes the retained minimum instead.
3. link.tsx / link-prefetch.ts / app-prefetch-fetch-queue.ts — traversal-safe prefetch. The re-checking of destinationIsCurrent() at each post-await boundary (it now recomputes against live window.location.href instead of a captured target) and the epoch-guarded viewport drain correctly avoid a superseded setup fetching the page a traversal just reached. The pendingLinkPrefetchFetches Set is drained via the new onRelease hook in all three lifecycle paths (high-priority release, low-priority complete/error, and cancel), so its lifetime tracks the existing appPrefetchFetchControls WeakMap rather than introducing a new leak.
4. app-browser-entry.ts — provisional visited entry. Publishing an elements-only entry with a zero-length body before await cacheBufferPromise is safe: it passes committedElements (enabling elements-only BFCache reuse during the await window) and seedPrefetchCache=false (so no zero-body snapshot leaks into the prefetch cache), then replaces it with the full snapshot. This closes the window where a back navigation would otherwise find the destination absent from both caches.
5. app-ssr-entry.ts — renderer selection. Dropping the NODE_ENV short-circuit in isReactDevelopmentRuntime() and choosing the renderer from the actual React implementation is the right fix for the "prerender sets NODE_ENV=production after React loaded" case; production React still resolves to the ESM prerender path.
6. app-page-route-wiring.tsx — new ordered tree positions. Adding every route-segment position to orderedTreePositions unconditionally is safe because the BfcacheSegmentBoundary wrapping only occurs for primaryBfcacheSegments, which are only populated when __NEXT_CACHE_COMPONENTS === "true". The default-mode route-wiring suite (80 tests) still passes, confirming no regression in the non-cache-components path.
Minor observation (non-blocking)
In cache-runtime.ts, the non-dynamic warmup-result HIT path (the early readPprFallbackShellWarmupCacheResult branch) calls recordRequestScopedCacheLife(...) but not recordRequestScopedCacheControl(...), so it skips the enclosing-cache-scope parentCtx.lifeConfigs.push that the existing HIT path performs. For a non-dynamic result the NestedDynamicUseCacheError propagation is a no-op regardless, and warmup results are keyed per pprCacheKey, so this is unlikely to matter in practice. Worth a quick sanity check that a nested use cache scope during the final render still bubbles its life config to the outer scope on this path — but I could not construct a failing case and the suite is green.
Overall: LGTM.
|
Review posted to PR #2860. Summary of my findings: Review outcome: LGTM (no blocking issues)I reviewed all 25 changed files, ran the targeted test suites, and type/lint-checked the six substantial source files. Verification:
Key areas examined:
One minor non-blocking observation: the non-dynamic warmup-result HIT path calls |
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: fix(cache): match segment cache staleness semantics
I reviewed all 25 changed files, ran the targeted suites, and type/lint-checked the substantial source files. The change is well-structured and the fixes are individually sound. No blocking issues. One testability gap and a couple of intentional-but-worth-flagging behavioral changes below.
Verification
vp test run tests/link-navigation.test.ts tests/app-visited-response-cache.test.ts→ 101 passedvp test run tests/shims.test.ts tests/slot.test.ts tests/app-page-dispatch.test.ts tests/app-page-route-wiring.test.ts→ 1463 passedvp checkon the six substantial source files → clean (format + lint + types)git diff --check→ clean
What I verified
-
app-page-dispatch.ts—runWithUnifiedStateMutationwarmup isolation. The reset field set (cacheableFetchUrls,currentRequestTags,dynamicFetchUrls,dynamicUsageDetected,invalidDynamicUsageError,renderRequestApiUsage,requestScopedCacheLife,unstableCacheObservations) is a superset of what the removeddiscardAppPageRenderState()consumed, and each field is replaced (not mutated in place), correctly honoring the replace-not-mutate contract documented on the ALS helper. No observation leaks from warmup into the final render. -
cache-runtime.ts— PPR warmup cache-result reuse. The dynamic-life branch suspends without publishing tags/cache-life (excluding short-lived Cache Components holes from route prefetch expiry), and the non-dynamic branch propagates tags + records cache-life.isDynamicCacheLife/cacheControlToCacheLifematch the pre-existing inlinerevalidate === 0 || expire < DYNAMIC_EXPIREchecks. Suspensions are kept outside the decodetry/catchso an abort rejection is not mistaken for a corrupt entry.Regarding the prior review's minor observation about
recordRequestScopedCacheControlvsrecordCacheHitLife: this is resolved — the warmup-result HIT path callsrecordCacheHitLife, which itself performs theparentCtx.lifeConfigs.pushand theNestedDynamicUseCacheErrorcapture. So nesteduse cachelife bubbling is preserved on that path. -
link.tsx/link-prefetch.ts/app-prefetch-fetch-queue.ts— traversal-safe prefetch.destinationIsCurrent()recomputes against livewindow.location.hrefat each post-await boundary; the epoch-guarded viewport drain andnavigate/popstatecancellation avoid a superseded setup fetching a page a traversal just reached. The newpendingLinkPrefetchFetchesSetis drained viaonReleasein all three lifecycle paths (high-priority release, low-priority complete/error, cancel). Deleting the current element from theSetduringfor...ofincancelLinkPrefetchSetups()is spec-safe. -
app-browser-entry.ts— provisional zero-body visited entry. Publishing an elements-only entry (committedElements,seedPrefetchCache=false) beforeawait cacheBufferPromiseis safe and closes the back-navigation window.hasVisitedResponseForPrefetchis only invoked forpriority === "high", so its internal cache-eviction side effect only runs against thetraverse(>30 min) freshness bound — a benign eviction of truly-expired entries. -
app-page-route-wiring.tsx/app-page-segment-plan.ts— primary bfcache segments. Unconditionally adding route-segment positions toorderedTreePositionsis safe becauseBfcacheSegmentBoundarywrapping only fires forprimaryBfcacheSegments, which are gated by__NEXT_CACHE_COMPONENTS === "true"viaincludePrimarySegment. Default-mode route-wiring suite still green. -
slot.tsx—isSlotIdguard. Skipping non-slot (layout-id) parents in thepreservedParentIdsidentity-flatten loop is correct: primary children-spine boundaries use layout ids as synthetic parents and must not go through slot-flattening there. Covered byslot.test.ts.
Non-blocking findings
1. (Testability) The core new helpers lack direct unit tests. rememberPprFallbackShellWarmupCacheResult / readPprFallbackShellWarmupCacheResult (new in ppr-fallback-shell.ts) and the warmup-result reuse branches in cache-runtime.ts are the heart of this PR, but they have no direct test. tests/ppr-fallback-shell.test.ts was not touched, and the cache-runtime warmup-reuse path (dynamic-suspend vs non-dynamic tag/life propagation, phase-gating of remember/read) is only reachable indirectly. Per AGENTS.md ("unit test the runtime helpers directly"), a focused test on these helpers — phase gating, the dynamic-vs-non-dynamic branch, and tag/cacheLife copying — would guard the exact staleness semantics this PR is fixing. Recommend adding before merge or as an immediate follow-up.
2. (Intentional, flagging for awareness) app-ssr-entry.ts onError now swallows all errors once pprFallbackShellSignal.aborted. Previously only isPprFallbackShellAbortError(error) was suppressed. Same change applied to app-page-render.ts's onRscRenderError. This is a reasonable "post-abort errors are cascade noise" choice and keeps dev/prod parity, but it is a genuine widening of error suppression during the fallback-shell prerender. Confirm this matches the App Router prerender-abort semantics you ported from (it appears to), since a real error raised in the exact turn after abort would now be dropped silently.
3. (Intentional, flagging) app-ssr-entry.ts loadStaticPrerender no longer falls back to the ESM entry in dev. When isReactDevelopmentRuntime() is true it now only attempts the CJS dev renderer and throws on failure, rather than trying react-dom/static.edge as before. This is the correct fix for the "prerender sets NODE_ENV=production after React loaded" bug (mixing dev React with the production renderer), but a future React dev build that exports prerender() from the ESM entry would route through CJS instead. Fine for current React; noting the removed fallback.
Overall: solid, matches the intended Next.js staleness behavior, and the E2E ports back it up. The only thing I'd want before/right-after merge is direct unit coverage for the two new PPR warmup-cache-result helpers and the cache-runtime reuse branch (finding 1).
|
Review posted to PR #2860. Review outcome: LGTM (no blocking issues), one testability gap flaggedI reviewed all 25 changed files, ran the targeted suites (1463 + 101 passed), and type/lint-checked the six substantial source files (clean). Verified sound:
Findings (all non-blocking):
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): match segment cache staleness semantics
Reviewed all 26 changed files, focusing on the substantial source changes. Ran the targeted suites and type/lint-checked the six substantial source files. No blocking issues — LGTM.
The two earlier /bigbonk runs (10:03, 10:24) were against ceb8bdb, before the third commit 3e9ebe2 test(cache): cover fallback shell reuse. That commit resolves the testability gap flagged as finding #1 in the prior review, so this pass focuses on confirming that and on independent correctness checks.
Verification
vp test run tests/ppr-fallback-shell.test.ts tests/app-page-dispatch.test.ts tests/app-visited-response-cache.test.ts tests/slot.test.ts→ 135 passedvp test run tests/link-navigation.test.ts tests/app-page-route-wiring.test.ts→ 162 passedvp checkon the six substantial source files → clean (format + lint + types)
Prior testability gap is now resolved
The heart of this PR — the PPR warmup-cache-result reuse — now has direct unit coverage:
tests/ppr-fallback-shell.test.ts:rememberPprFallbackShellWarmupCacheResult/readPprFallbackShellWarmupCacheResultphase-gating (warmup-only write, final-only read), the defensive clone (mutating the sourcecacheLife/tagsafterrememberdoes not affect the stored value), andshouldIgnorePprFallbackShellRenderError.tests/shims.test.ts"omits short-lived use cache entries from a Cache Components prerender": exercises bothcache-runtime.tsreuse branches throughregisterCachedFunction— the non-dynamic (minutes) path propagates tags (getCollectedFetchTags()→["long-lived"]) and records cache-life (_peekRequestScopedCacheLife()→ the minutes profile) without re-executing, while the dynamic (seconds) path suspends indefinitely (stays pending, then rejects on abort) and is never re-run.
What I verified independently
-
cache-runtime.tswarmup reuse — the early warmup-HIT branch (L586) usesrecordCacheHitLife, which performs the enclosing-scopeparentCtx.lifeConfigs.push+NestedDynamicUseCacheErrorcapture, so the earlier minor observation aboutrecordRequestScopedCacheControlis fully addressed. Suspensions are kept outside the decodetry/catch(L617-624) so an abort rejection is not misread as a corrupt entry.cacheControlToCacheLife/isDynamicCacheLifematch the pre-existing inlinerevalidate === 0 || expire < DYNAMIC_EXPIREchecks. -
app-page-dispatch.tsrunWithUnifiedStateMutationisolation — the reset field set is a superset of what the removeddiscardAppPageRenderState()consumed, and each field is replaced (not mutated in place), honoring the replace-not-mutate contract on the ALS helper. The child context is discarded afterwarm(), so warmup observations cannot leak into the final render. -
link.tsx/link-prefetch.ts/app-prefetch-fetch-queue.ts—destinationIsCurrent()recomputes against livewindow.location.hrefat each post-await boundary; the epoch-guarded viewport drain plusnavigate(traverse)/popstatecancellation avoid a superseded setup fetching a page a traversal just reached. The newpendingLinkPrefetchFetchesSetis drained viaonReleasein all three queue lifecycle paths, so it tracks the existingappPrefetchFetchControlsWeakMap lifetime and introduces no leak. -
app-browser-entry.ts— the provisional zero-body visited entry (committedElements,seedPrefetchCache=false) is safe and closes the back-navigation window;hasVisitedResponseForPrefetchruns only forpriority === "high"and itstraversefreshness bound only evicts truly-expired entries. -
app-page-segment-plan.ts/app-page-route-wiring.tsx/slot.tsx—primaryBfcacheSegmentsare gated by__NEXT_CACHE_COMPONENTS === "true"viaincludePrimarySegment; the unconditionalorderedTreePositionsadditions are inert outside cache-components mode (default route-wiring suite green). TheisSlotIdguard inmergeElementscorrectly skips layout-id synthetic parents. -
app-rsc-handler.ts/app-page-dispatch.ts— the newkind: "cache-components-prerender"discriminant correctly keepsplaceGeneratedMetadataInBodyfalse for a real prerender whilefallback-shellstill places metadata in the body.
Non-blocking observations
-
(Pre-existing, not a regression) Shared reference fields not reset by the warmup
mutate.runWithUnifiedStateMutation's docstring enumerates all reference-typed slices that amutatemust replace to avoid the parent observing in-place writes. The dispatchmutateresets the observation-relevant subset but notpendingSetCookies,afterContext.callbacks,pendingRevalidatedTags, orserverInsertedHTMLCallbacks. If a fallback-shell warmup render performs acookies().set(),after(), orrevalidateTag()in place, those would bleed into the parent scope. The removeddiscardAppPageRenderState()had the same gap, so this is not introduced here — but since warmup now runs in a scoped child, resetting those slices too would be strictly safer and match the documented contract. Worth a follow-up if a warmup can reach those APIs. -
(Intentional, flagging for awareness)
onErrornow swallows all errors oncepprFallbackShellSignal.abortedin bothapp-ssr-entry.tsandapp-page-render.ts(shouldIgnorePprFallbackShellRenderErrorreturns true wheneversignal.aborted, not only for abort-shaped errors). This matches Next.js's "post-abort errors are cascade noise" prerender semantics and keeps dev/prod parity, but it is a genuine widening of suppression during the fallback-shell prerender. Now covered by a unit test.
Overall: solid, matches the intended Next.js staleness behavior, the E2E ports back it up, and the third commit closes the earlier coverage gap.
|
Review posted to PR #2860. Review outcome: LGTM (no blocking issues)I reviewed all 26 changed files, ran the targeted suites (135 + 162 passed), and Key finding vs. prior runs: the two earlier
Verified independently: the Two non-blocking observations:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): match segment cache staleness semantics
Independent pass focused on the parts the prior /bigbonk runs covered less — the build/prerender/seed pipeline, the new /__vinext/prefetch-policy endpoint and its client consumer, cache-key parity across the KV/seed/runtime paths, and the new bfcache "segment" descriptor. No blocking issues. The core App Router cache-runtime/dispatch/link changes were already covered thoroughly in the three earlier reviews and I independently re-confirmed them.
Verification
vitest run tests/ppr-fallback-shell.test.ts tests/app-page-dispatch.test.ts tests/seed-cache.test.ts tests/prerender.test.ts tests/inject-pregenerated-paths.test.ts tests/pregenerated-concrete-paths.test.ts→ 234 passedvitest run tests/shims.test.ts tests/link-navigation.test.ts tests/app-visited-response-cache.test.ts tests/slot.test.ts tests/prefetch-cache.test.ts→ 1491 passed
Cache-key parity — verified consistent across all three producers
The new prefetch-loading-shell RSC variant is keyed identically everywhere:
- Runtime read —
appIsrRscKey(pathname, null, "prefetch-loading-shell")→...:rsc:prefetch-loading-shell - Seed (Node) — entry's
buildAppPageRscKey(pathname, renderMode)=__isrRscKey(pathname, null, renderMode)(same), fallback =baseKey + ":rsc:prefetch-loading-shell" - Cloudflare KV populate —
appIsrCacheKey(cachePathname, "rsc:prefetch-loading-shell", buildId)
All three resolve to the same string, so seeded/populated shells are actually reachable at runtime. Good.
Partial-prerender path normalization — agrees for canonical inputs
Server stores normalizePath(normalizePathnameForRouteMatch(pathname)) (addPartialPrerenderPath); the client hasPartialPrerenderForRoute looks up with only normalizePathnameForRouteMatch(...). normalizePath is a no-op fast-path for already-canonical single-slash pathnames, which is what prerendered route.path/route.route values always are, so the sets agree in practice. Only a pathological (double-slash / dot-segment) prerender path would diverge, and those don't occur here. Fine.
/__vinext/prefetch-policy endpoint — placement is correct
Handled early in handleAppRscRequest on pathname === "/__vinext/prefetch-policy", matching the established /__vinext/prerender/* internal-endpoint pattern (matches on pathname, bypasses middleware/routing — appropriate for an internal manifest endpoint). clearRequestContext() is called before responding. Cache-Control: public, max-age=0, must-revalidate is reasonable for a build-static manifest.
- Non-blocking nit: the two existing sibling endpoints use exported constants (
VINEXT_PRERENDER_STATIC_PARAMS_PATH), whereas this one hard-codes the"/__vinext/prefetch-policy"literal in both the handler and the client fetch (app-route-prefetch-policy.ts). A shared constant would keep the two ends from drifting. Cosmetic only.
New hasPartialPrerenderForRoute await in the Link prefetch path — traversal-safe
The new await hasPartialPrerenderForRoute(...) at link.tsx:547 adds an async boundary before the fetch, but it only selects the renderMode request header; the actual fetch is still gated by scheduleGuardedLinkPrefetchFetch → prefetchWasSuperseded() (re-checks navigationEpoch + live destinationIsCurrent()), so a traversal that lands during the await still cancels the fetch. The module-level partialPrerenderPathsPromise memoizes the manifest fetch (one network call) and .catch(() => new Set()) fails open to "no partial prerender", so a policy-endpoint failure degrades to normal prefetch rather than breaking it. Sound.
New bfcache "segment" descriptor — collision-free
deriveBfcacheSegmentIdentity's new "segment" case serializes to ["segment", ownerLayoutGraphId, boundSegmentKey] — a distinct leading kind tag and 3-tuple that cannot alias the other kinds. The primary segment ids are minted via createNestedBfcacheSlotSegmentId(layoutEntry.id, level) with a layout id as the encoded parent, so isNestedBfcacheSlotSegmentIdFor(id, <slotId>) is false for them — which is exactly why the slot.tsx isSlotId(parentId) guard is needed to skip them in the identity-flatten loop. Consistent, and gated behind __NEXT_CACHE_COMPONENTS === "true" via includePrimarySegment (default-mode suites green).
One pre-existing gap re-confirmed (non-blocking, matches prior finding #1)
The warmup mutate in app-page-dispatch.ts (the runWithUnifiedStateMutation child) resets 8 observation slices but not afterContext, pendingSetCookies, serverInsertedHTMLCallbacks, pendingRevalidatedTags, pendingRevalidations, or ssrHeadChildren — all of which the runWithUnifiedStateMutation docstring explicitly enumerates as reference-typed slices a mutate must replace. Because warm() renders the full page element (warmupBuildResult.element), a page/layout that calls after(), cookies().set(), revalidateTag(), or inserts server HTML during warmup would bleed those into the parent scope via the shared reference (e.g. double-run after() callbacks in the final render).
- Not introduced by this PR — the removed
discardAppPageRenderState()had the same gap. - Only reachable under
VINEXT_PRERENDER === "1"(build-time prerender), never on a live production request. - Since warmup now runs in a scoped child anyway, adding those field resets to the
mutatewould be strictly safer and would match the documented ALS contract. Good follow-up candidate.
Intentional widenings (already flagged in prior reviews, re-confirmed)
shouldIgnorePprFallbackShellRenderError(signal, error)returns true wheneversignal.aborted(not just for abort-shaped errors), now applied in bothapp-ssr-entry.tsandapp-page-render.ts. Matches Next.js's "post-abort errors are cascade noise" prerender semantics; unit-tested.loadStaticPrerenderno longer falls back to the ESM entry whenisReactDevelopmentRuntime()— the correct fix for the "prerender setsNODE_ENV=productionafter React loaded" dev/prod dispatcher mismatch.
Overall: solid, well-tested, and the build→seed→runtime cache-key path lines up. LGTM.
|
Review posted to PR #2860. Review outcome: LGTM (no blocking issues)I ran an independent pass focused on the areas the three prior Verification:
Key checks that came back clean:
Non-blocking findings:
|
…he-staleness-current # Conflicts: # packages/vinext/src/shims/internal/app-route-prefetch-policy.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): match segment cache staleness semantics
Independent pass at 6e70303. I re-derived the correctness of each substantial source change from scratch, verified cache-key/render-mode parity across the build→seed→KV→runtime paths, and ran the targeted suites. No blocking issues — LGTM. The three earlier /bigbonk runs are consistent with what I found; I confirmed their prior concerns are either resolved or accurately characterized as non-blocking.
Verification
vitest run tests/ppr-fallback-shell.test.ts tests/shims.test.ts tests/app-page-dispatch.test.ts tests/seed-cache.test.ts tests/prerender.test.ts→ 1498 passedvitest run tests/link-navigation.test.ts tests/app-visited-response-cache.test.ts tests/slot.test.ts tests/prefetch-cache.test.ts tests/app-page-route-wiring.test.ts tests/app-rsc-handler.test.ts→ 460 passedvp check packages/vinext/src/shims/cache-runtime.ts server/app-page-dispatch.ts shims/link.tsx server/app-page-cache.ts→ clean (format + lint + types)
Independently confirmed correct
-
cache-runtime.tswarmup reuse + life bubbling. The warmup-HIT branch (L586–596) usesrecordCacheHitLife, which at L808/L821/L827 does all three of:parentCtx.lifeConfigs.push, theNestedDynamicUseCacheErrorcapture, and_setRequestScopedCacheLife. So the request-scope publish and the enclosing-use cache-scope bubbling are both preserved on the reuse path — the earliest review'srecordRequestScopedCacheControlconcern is genuinely resolved (I traced it line-by-line rather than taking it on faith). Dynamic holes suspend before publishing tags/life; suspensions stay outside the decodetry/catch.cacheControlToCacheLife/isDynamicCacheLifereproduce the pre-existing inlinerevalidate === 0 || expire < DYNAMIC_EXPIREcheck exactly. -
cache.ts—cacheLife()no longer eagerly publishing is the correct pairing for #1: the wrapper now publishes the retained minimum once the value is kept, so a short-lived hole omitted from a Cache Components prerender no longer constrains the enclosing page. -
app-page-cache.tsbypassStaleResponse. A fresh (non-stale)prefetch-loading-shellentry is still served on HIT (L409–435); only a stale one is bypassed to force a fresh navigation render (L446–455). Returningnullthere deliberately skips background regeneration for that request, which is correct — an ordinary navigation must render the missing dynamic data, not replay/refresh the partial shell. -
app-page-dispatch.tsisPrerenderPartialCacheRead. Correctly narrowed to RSC + navigation render-mode + whole-route prefetch (Next-Router-Prefetch: 1, no segment header), so only a whole-route prefetch reads the partial artifact; a real navigation renders through. -
link.tsxtraversal-safety.destinationIsCurrent()recomputes against livewindow.location.hrefat every post-await boundary; the newhasPartialPrerenderForRouteawait and the RSC-URL hashing are both followed by epoch + current-URL re-checks, and the actual fetch is gated byscheduleGuardedLinkPrefetchFetch → prefetchWasSuperseded(). The newnavigate(traverse)/popstatelisteners abort in-flight setups viacancelLinkPrefetchSetups. ThependingLinkPrefetchFetchesSetis drained viaonReleasein all three queue lifecycle paths, tracking the existingappPrefetchFetchControlsWeakMap lifetime (no new leak). -
Cache-key/render-mode parity across producers. The
prefetch-loading-shellRSC variant keys identically at runtime (__isrRscKey(pathname, null, renderMode)), in the Node seed path (buildAppPageRscKey(pathname, renderMode)and its:rsc:<variant>fallback), and in the Cloudflare KV populate path (getRscRenderModeCacheVariant→rsc:<variant>).parseAppRscRenderMode/getRscRenderModeCacheVariantare shared, so seeded and populated shells are reachable at runtime.writePrerenderIndexonly recordspartialPrerenderPathsforrendered && app && prefetch-loading-shell && !fallback, and the same predicate seedsaddPartialPrerenderPath— the/__vinext/prefetch-policyendpoint therefore advertises exactly the routes that were captured. -
app-ssr-entry.tsrenderer selection. Dropping theNODE_ENVshort-circuit and choosing the renderer from the actual React implementation (Function.prototype.toString(...).includes("getOwner")) is the right fix for "prerender setsNODE_ENV=productionafter React was loaded"; production React still resolves to the ESMprerenderpath (the ESM attempt now runs after the dev-CJS branch). -
slot.tsxisSlotIdguard correctly skips layout-id synthetic parents in the identity-flatten loop;bfcache-identity.ts"segment"serializes to a collision-free 3-tuple; both are gated behind__NEXT_CACHE_COMPONENTS === "true"viaincludePrimarySegment(default suites green).
Non-blocking observations
-
(Pre-existing, build-prerender-only) The warmup
mutateinapp-page-dispatch.ts(L1069) resets 8 observation slices but not the other reference-typed slices therunWithUnifiedStateMutationdocstring enumerates —afterContext,pendingSetCookies,serverInsertedHTMLCallbacks,pendingRevalidatedTags,pendingRevalidations,ssrHeadChildren. I re-confirmed the warmup is still strictly gated byprocess.env.VINEXT_PRERENDER === "1"(L1064), so this is build-time only and never reachable on a live request; the removeddiscardAppPageRenderState()had the same gap, so it is not a regression. But sincewarm()now renders in a scoped child, a page/layout that callsafter()/cookies().set()/revalidateTag()/ inserts server HTML during warmup would bleed those into the parent via the shared reference. Adding those resets to themutatewould match the documented contract and cost nothing. Good follow-up candidate. -
(Naming clarity)
notifyLinkNavigationStartAndCancelPrefetchSetuponly bumps the epoch; it does not abort in-flight fetches — only the newcancelLinkPrefetchSetups(bound tonavigate/traverse +popstate) both bumps the epoch and callscancelAppPrefetchFetch. That asymmetry looks intentional (a forwardrouter.push/click may be navigating to the in-flight prefetch's destination, so aborting it would be wasteful), but the "…AndCancelPrefetchSetup" name now overstates what it does relative to the new sibling. Cosmetic. -
(Cosmetic, re-confirming prior finding)
/__vinext/prefetch-policyhard-codes the path literal in both the handler (app-rsc-handler.ts) and the client fetch (app-route-prefetch-policy.ts) instead of a shared exported constant like itsVINEXT_PRERENDER_STATIC_PARAMS_PATHsiblings. A shared constant would prevent the two ends from drifting.
Overall: the change is well-structured, matches the intended Next.js staleness semantics, the E2E ports and new unit tests back it up (ppr-fallback-shell.test.ts + the "omits short-lived use cache entries" shim test cover both cache-runtime reuse branches), and the build→seed→runtime cache-key path lines up. Ship it.
|
Review posted to PR #2860. Review outcome: LGTM (no blocking issues)I did an independent pass at Independently confirmed correct:
Non-blocking findings:
|
…he-staleness-current # Conflicts: # packages/vinext/src/shims/cache-runtime.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): match segment cache staleness semantics
Independent pass at 58cfe92 (post-merge with main). I re-derived each substantial source change from scratch, verified cache-key/render-mode parity across the build→seed→KV→runtime paths, traced the warmup-cache-result key derivation, and ran the targeted suites. No blocking issues — LGTM. My findings match the five prior /bigbonk runs.
Verification
vp test run tests/shims.test.ts tests/ppr-fallback-shell.test.ts tests/app-page-dispatch.test.ts tests/link-navigation.test.ts→ 1461 passedvp test run tests/app-visited-response-cache.test.ts tests/app-page-route-wiring.test.ts tests/slot.test.ts tests/prefetch-cache.test.ts tests/seed-cache.test.ts tests/prerender.test.ts tests/app-rsc-handler.test.ts→ 500 passed
Independently confirmed correct
-
cache-runtime.tswarmup reuse — key derivation is consistent. The final-phase read uses the earlypprCacheKey(cacheVariant\0+ root-param-suffixedcacheKey, L669/L679), while the warmup writes key offresultCacheKey(L794–798). Because warmup runs first andaddKnownRootParamNames(id, …)populatesknownRootParamsByFunctionIdduring warmup, the early key in the final phase reconstructs the same suffix — so remembered results are actually found. The dynamic branch (L683) intentionally suspends without publishing tags/cache-life; the non-dynamic branch propagates tags +recordCacheHitLife(which does theparentCtx.lifeConfigs.push+NestedDynamicUseCacheErrorcapture +_setRequestScopedCacheLife), so the earliest review'srecordRequestScopedCacheControlconcern is genuinely resolved.cacheControlToCacheLife/isDynamicCacheLifereproduce the pre-existing inlinerevalidate === 0 || expire < DYNAMIC_EXPIREcheck exactly. Suspensions are kept outside the decodetry/catch. -
cache.ts—cacheLife()no longer eagerly publishing is the correct pairing: the wrapper publishes the retained minimum only after the value is kept, so a short-lived hole omitted from a Cache Components prerender no longer constrains the enclosing page. -
app-page-dispatch.tsisPrerenderPartialCacheRead— correctly narrowed to RSC + navigation render-mode + whole-route prefetch (Next-Router-Prefetch: 1, no segment header), andbypassStaleResponseonly fires for the loading-shell read path.app-page-cache.tsbypassStaleResponsereturnsnullon a stale partial shell only (fresh shells still HIT), forcing a fresh navigation render — correct. -
app-browser-entry.tsprovisional zero-body visited entry —createCachedRscResponseSnapshot(navResponse, new ArrayBuffer(0), …)only reads headers/the passed buffer, neverresponse.body, so it does not disturb the response;extractRscCompletionMetadataon an empty buffer short-circuits (indexOf === -1). Published withseedPrefetchCache=falseand elements-only, then replaced with the buffered snapshot. Safe, and it closes the back-navigation window. -
link.tsxtraversal-safety —destinationIsCurrent()recomputes against livewindow.location.hrefat every post-await boundary; the newhasPartialPrerenderForRouteawait and RSC-URL hashing are both followed by epoch + current-URL re-checks; the actual fetch is gated byscheduleGuardedLinkPrefetchFetch → prefetchWasSuperseded(). ThependingLinkPrefetchFetchesSetdrains viaonReleasein all three queue lifecycle paths (Set-delete-during-iteration is spec-safe). -
navigation.tsrouter.prefetch— the addedhasPartialPrerenderForRouteawait sets an independent header (VINEXT_RSC_RENDER_MODE_HEADER) from the reusable-prefetch block; ordering is irrelevant, andsetup.cancelledis still re-checked after all awaits before the fetch. -
Cache-key/render-mode parity — the
prefetch-loading-shellRSC variant keys identically at runtime (__isrRscKey(pathname, null, renderMode)), in the Node seed path, and in the Cloudflare KV populate path (getRscRenderModeCacheVariant→rsc:<variant>).writePrerenderIndexrecordspartialPrerenderPathsforrendered && app && prefetch-loading-shell && !fallback, and the same predicate seedsaddPartialPrerenderPath, so/__vinext/prefetch-policyadvertises exactly the captured routes. -
app-ssr-entry.tsrenderer selection,slot.tsxisSlotIdguard, andbfcache-identity.ts"segment"(collision-free 3-tuple) all sound; the primary-segment path is gated behind__NEXT_CACHE_COMPONENTS === "true"viaincludePrimarySegment(default suites green).
Non-blocking observations (all consistent with prior runs)
-
(Pre-existing, build-prerender-only) The warmup
mutate(L1069) resets 8 observation slices but not the remaining reference-typed slices therunWithUnifiedStateMutationdocstring enumerates —afterContext,pendingSetCookies,serverInsertedHTMLCallbacks,pendingRevalidatedTags,pendingRevalidations,ssrHeadChildren. I confirmed these APIs write in-place (ctx.afterContext.callbacks.push,pendingSetCookies.push,pendingRevalidatedTags.add), so a page/layout that callsafter()/cookies().set()/revalidateTag()/inserts server HTML during warmup would bleed into the parent via the shared reference (e.g. double-runafter()in the final render). It is strictly gated toprocess.env.VINEXT_PRERENDER === "1"(L1064) — build-time only, never a live request — and the removeddiscardAppPageRenderState()had the same gap, so this is not a regression. Sincewarm()now runs in a scoped child, adding those resets to themutatewould match the documented contract and cost nothing. Good follow-up candidate. -
(Intentional)
onErrornow swallows all errors oncepprFallbackShellSignal.aborted(shouldIgnorePprFallbackShellRenderError), in bothapp-ssr-entry.tsandapp-page-render.ts. Matches Next.js's "post-abort errors are cascade noise" prerender semantics; now unit-tested. -
(Cosmetic)
/__vinext/prefetch-policyhard-codes the path literal in both the handler (app-rsc-handler.ts:610) and the client fetch (app-route-prefetch-policy.ts), whereas the siblingVINEXT_PRERENDER_STATIC_PARAMS_PATHendpoints use a shared exported constant. A shared constant would prevent the two ends from drifting. -
(Naming clarity)
notifyLinkNavigationStartAndCancelPrefetchSetuponly bumps the epoch — it does not abort in-flight fetches; only the newcancelLinkPrefetchSetups(bound tonavigate/traverse +popstate) both bumps the epoch and callscancelAppPrefetchFetch. The asymmetry looks intentional but the name overstates it relative to the new sibling.
Overall: well-structured, matches the intended Next.js staleness semantics, the E2E ports and new unit tests (ppr-fallback-shell.test.ts + the "omits short-lived use cache entries" shim test cover both cache-runtime reuse branches) back it up, and the build→seed→runtime cache-key path lines up. Ship it.
|
Review posted to PR #2860. Review outcome: LGTM (no blocking issues)I did an independent pass at Verified correct:
Non-blocking findings (all consistent with prior runs):
The PR is well-structured, well-tested (both cache-runtime reuse branches now have direct unit coverage), and matches Next.js staleness semantics. |
# Conflicts: # packages/vinext/src/build/prerender.ts # packages/vinext/src/server/app-rsc-handler.ts # packages/vinext/src/server/headers.ts # packages/vinext/src/server/prerender-manifest.ts # packages/vinext/src/server/seed-cache.ts # packages/vinext/src/shims/link.tsx # packages/vinext/src/shims/navigation.ts # tests/prefetch-cache.test.ts # tests/seed-cache.test.ts
1 participant
Summary
Upstream coverage
Ports and fixes the current Next.js staleness behavior exercised by:
test/e2e/app-dir/segment-cache/staleness/segment-cache-stale-time.test.tstest/e2e/app-dir/segment-cache/staleness/segment-cache-per-page-dynamic-stale-time.test.tsValidation
vp check: 25/25 files cleangit diff --checkvp run knipvp run vinext#buildIndependent cumulative review: no findings.