fix(app-router): vary segment prefetches by accessed params - #2861
fix(app-router): vary segment prefetches by accessed params#2861james-elicx wants to merge 13 commits into
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 5 regressed · 1 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review — PR #2861 (vary segment prefetches by accessed params)
Reviewed all 40 changed files against main. The feature is well-contained behind the experimental triple-gate (cacheComponents && experimental.varyParams && experimental.optimisticRouting), so it defaults off and cannot affect existing users. Tests pass locally (vp check clean; app-prefetch-vary-analysis, app-page-probe, app-optimistic-routing, app-layout-param-observation, app-page-element-builder, app-rsc-cache-busting, thenable-params, rsc-completion-metadata, link, next-config, entry-templates — 610 tests green).
A few issues worth addressing before merge:
1. analyzeAppPrefetchCapabilities runs unconditionally at entry-generation time, even when the feature is disabled (perf)
toLinkPrefetchRoute (packages/vinext/src/entries/app-browser-entry.ts:119) calls analyzeAppPrefetchCapabilities(route) for every app route regardless of whether prefetch-vary is enabled. That helper does synchronous fs.readFileSync on the page source and every layout source (app-prefetch-vary-analysis.ts:38-52). For a large app this is repeated blocking file I/O on every dev rebuild and every production build, for output that is only consumed when the experimental flags are on.
generateBrowserEntry already threads prefetchVaryEnabled, but toLinkPrefetchRoute/toLinkPrefetchRoutes don't receive it (and index.ts:1522 calls toLinkPrefetchRoutes without it for the manifest). Consider gating the source analysis behind the flag so the default path stays free of extra I/O — this aligns with the AGENTS.md "keep the common request/build path lightweight" guidance.
2. Vary metadata can exceed the 4096-byte footer limit and error the RSC stream mid-flight
getPrefetchVaryMetadata() is serialized into the RSC completion footer via encodeFooter, which throws "RSC completion metadata exceeded its framing limit" when the payload exceeds MAX_FOOTER_BYTES (now 4096) — and this throw happens inside appendRscCompletionMetadata's pull() at stream end (rsc-completion-metadata.ts:96-98, 240-241), so it surfaces as a stream error that breaks the prefetch response rather than a clean degrade.
The validators are far more permissive than 4096 bytes: isStringArray allows 64 entries × 128 chars, and pageDynamicSuspenseOrdinalsByElementId allows 64 keys × 256 chars (rsc-completion-metadata.ts:22-43) — comfortably >4096 bytes for a route with many params / long element IDs. The updated test only bumps the size constant (tests/rsc-completion-metadata.test.ts) and doesn't cover an oversized vary payload.
Suggest either (a) bounding/truncating the metadata producer so it can never overflow the footer, or (b) making the emit path degrade gracefully (skip the footer) instead of erroring the stream, plus a test that drives a realistic oversized payload end-to-end. This is only reachable with the flags on, so it's not a release blocker, but it's a latent correctness bug for the feature's target users.
3. \0-delimited composite variant keys are theoretically ambiguous
runtimeTemplateVariantKey is built as ${sharedCacheKey}\0${loadingShellSharedCacheKey} (e.g. navigation.ts and link.tsx), but each operand already contains \0 internally (from resolveAppPrefetchSharedCacheKey, which joins pattern parts with \0). Concatenating two \0-containing strings with another \0 is not an injective encoding — ("a\0b","c") and ("a","b\0c") both yield "a\0b\0c". In practice both keys derive from the same URL+route so the split is deterministic, but it's a fragile invariant. Consider a length-prefixed or JSON.stringify([a, b]) join to make the composite unambiguous.
Minor / non-blocking
optimisticRouteTemplatesnow keys byvariantKeyin addition to route+context+slots, so a route hit with many distinct observed param values can accumulate more template entries. They're cleared on prefetch invalidation, but there's no per-route cap — matches existing prefetch-cache behavior, just noting the growth surface.- Monotonicity of
learnAppPrefetchVaryMetadatalooks correct (mergeVaryParamNamesunions,||=for booleans is monotonic). Good. - Dev/prod parity: this is App-Router-only and prod delegates to the built RSC entry, so no separate Pages-router prod path needs the change. Confirmed.
Overall the design is sound and the change is safely gated. #1 (unconditional build-time I/O) and #2 (stream-erroring footer overflow) are the two I'd want resolved before enabling the feature broadly.
|
Review posted to PR #2861. SummaryI reviewed all 40 changed files in PR #2861. The feature (varying segment prefetches by accessed params) is well-architected and fully gated behind three experimental flags ( I posted 3 substantive findings plus minor notes:
Plus minor notes on template-map growth and confirmation that monotonicity and dev/prod parity are correct. Findings #1 and #2 are the two I flagged as worth resolving before enabling the feature broadly; neither is a release blocker since the feature defaults off. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — PR #2861 (round 2, head 9730d999)
Re-reviewed after 9730d999 fix(cache): address vary prefetch review findings. The three findings from the prior review are handled:
- #1 (unconditional build-time I/O) — fixed.
toLinkPrefetchRoute/toLinkPrefetchRoutesnow takeprefetchVaryEnabledand skipanalyzeAppPrefetchCapabilitieswhen the feature is off (index.ts:1522gates oncacheComponents && varyParams && optimisticRouting). Default path is I/O-free again. ✅ - #3 (
\0-delimited composite key ambiguity) — fixed.encodeAppPrefetchRuntimeTemplateVariantKeynow usesJSON.stringify([a, b]), which is injective. All three call sites updated. ✅ - #2 (footer overflow erroring the stream) — changed, but the chosen fix introduces a silent correctness bug (see below).
1. Footer-overflow "graceful skip" silently under-varies the prefetch cache (correctness)
encodeFooter now returns undefined on overflow and the emit path drops the footer (rsc-completion-metadata.ts:93-107, 240-244). This removes the stream error, but for the vary-params feature it trades a loud failure for silent wrong-content caching.
The vary metadata is emitted in two places with different contents:
- Header (
app-page-render.ts:897-901) is set synchronously before the RSC body streams, sogetPrefetchVaryMetadata()returns the observations known at response-start. - Footer (
app-page-render.ts:909-913) is computed in the stream'spull()at end-of-render, so it captures the complete observation set — including params read deep inside Suspense boundaries (pageDynamicSuspenseOrdinalsByElementId, latepageParamNames).getPrefetchVaryMetadata()reads live mutable sets (app-layout-param-observation.ts:284-306), so the two snapshots genuinely differ.
The client prefers the footer and falls back to the header: extracted.metadata?.prefetchVary ?? decodePrefetchVaryHeader(...) (navigation.ts:1269-1271), and re-learns from the same source in the final learn step (navigation.ts:1480-1491). Learning is monotonic and additive (app-route-prefetch-policy.ts:174-185), and the default (no vary params learned) is permissive — resolveAppPrefetchSharedCacheKey treats every unmentioned param as a shared pattern placeholder (app-route-prefetch-policy.ts:225-230), i.e. "safe to reuse across all values".
Consequence: when the footer is dropped, the client silently loses the late-observed params and falls back to the pre-render header snapshot, which lacks exactly those params. It then treats them as non-varying and reuses one cached segment across different param values — the precise failure the feature exists to prevent. The overflow trigger (many params / long element IDs in pageDynamicSuspenseOrdinalsByElementId) is also exactly the payload the header snapshot can't carry, so the header fallback provides no protection here.
The new test (tests/rsc-completion-metadata.test.ts "omits oversized completion metadata…") asserts metadata: undefined and that the stream doesn't fail — i.e. it locks in the silent-drop behavior without checking that vary correctness survives.
Suggested direction (pick one):
- Bound the producer so the footer can never overflow. Cap/truncate
getPrefetchVaryMetadata()output to fitMAX_FOOTER_BYTES, but truncate toward the conservative side — droppingpageDynamicSuspenseOrdinalsByElementIdgranularity or collapsing to "vary by all params" rather than dropping param names. Then the fallback is stricter, not looser. - Or raise
MAX_FOOTER_BYTESfor the vary payload and keep the validators (isStringArray64×128,isOrdinalRecord64×256) in sync with a limit the payload can't exceed. - Either way, add a test that drives a realistic oversized vary payload end-to-end and asserts the client does not reuse across the affected params (correctness), not just that the stream survives.
This is only reachable with the experimental triple-gate on, so it's not a release blocker — but it's a latent correctness bug for the feature's target users, and the current fix makes it silent rather than loud.
Minor / non-blocking
parseFooterCandidate(rsc-completion-metadata.ts:114-116) andstripRscCompletionMetadata(:273-274) still throw/error on a >MAX_FOOTER_BYTEScandidate, which is now unreachable from our own producer (it never emits one). That asymmetry is fine defensively, just noting the producer and consumer now disagree on whether oversize is fatal.- Growth surface unchanged from prior note:
optimisticRouteTemplateskeyed byvariantKeycan accumulate entries for routes hit with many distinct observed param values; cleared on prefetch invalidation, no per-route cap. Matches existing prefetch-cache behavior. - Monotonicity and dev/prod parity (App-Router-only; prod delegates to the built RSC entry) still hold.
Overall: findings #1 and #3 are cleanly resolved. The #2 fix removes the crash but should be reworked so the degradation is conservative (over-vary / stricter reuse) rather than permissive (reuse across params that actually vary).
|
Review posted to PR #2861. SummaryI re-reviewed the head commit Resolved:
New finding (#2 fix is incorrect):
I recommended reworking the degradation to be conservative (truncate toward "vary by all params" / stricter reuse) rather than permissive, or raising |
|
Exact-head deploy/CI audit for
The PR is mergeable with no conflict, so I did not merge/rebase/update its base. It must remain draft for now:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — PR #2861 (round 3, head 68f1163)
Re-reviewed after 68f1163 fix(app-router): fail closed on oversized vary metadata, which reworks the footer-overflow handling flagged in the round-2 review. The prior correctness finding is now resolved, and the fix is in the right (conservative) direction.
Prior finding #2 (footer overflow → silent under-varying) — resolved ✅
The previous fix silently dropped the footer on overflow, which fell back to the permissive pre-render header snapshot and could reuse one cached segment across param values that actually vary. The new commit replaces that with a fail-closed candidate ladder in both the header and footer encoders:
- Try the exact parsed metadata.
- If it doesn't fit, drop
pageDynamicSuspenseOrdinalsByElementIdbut keep the flatpageDynamicSuspenseOrdinalsunion (a strict superset of every element's ordinals → over-invalidates, never under-invalidates). - If still too big, emit a fixed-size conservative payload (
varyAllParams: true,pageAllSuspenseDynamic: true, both search-param booleanstrue).
I verified the two new client flags actually enforce stricter reuse, not looser:
varyAllParams→learnAppPrefetchVaryMetadatamixes ingetAllRouteParamNames(route)(all pattern + slot params) for every vary set and forces the search-param booleans (app-route-prefetch-policy.ts:178-199). Every param becomes value-keyed → no cross-value reuse.pageAllSuspenseDynamic→ threaded intoreplaceObservedDynamicSuspenseChildren(..., replaceAllSuspense=true), which replaces every Suspense boundary (and, at the depth-100 guard, the whole subtree) withOptimisticRouteSegmentregardless of observed ordinals (app-optimistic-routing.ts:356, 499, 601). That forces re-fetch → the safe direction.
Both the header (app-page-render.ts:900) and the footer (:909-913) route through the same conservative encoders, so the two snapshots degrade consistently — closing the header/footer parity gap that made the old drop dangerous.
The non-null assertion in encodeFooter (rsc-completion-metadata.ts:212-214) is sound: the conservative payload plus the validated numeric/null scalar fields (dynamicStaleTimeSeconds, serverStaleTimeSeconds) are all fixed, bounded size, so it can never overflow MAX_FOOTER_BYTES.
Tests now assert correctness, not just stream survival ✅
The old test only checked metadata: undefined + stream survival. The new tests assert the content of the degraded metadata:
- Oversized payload degrades to the conservative
varyAllParams/pageAllSuspenseDynamicset (rsc-completion-metadata.test.ts"coarsens oversized completion metadata without failing or under-varying"). - Oversized per-element map coarsens to the safe global union while preserving
pageDynamicSuspenseOrdinals/pageParamNames. - Malformed and oversized headers fail closed to conservative metadata.
- Sparse-array producer/wire inputs are rejected via the new
isDenseArrayguard.
All targeted suites pass locally (rsc-completion-metadata, app-optimistic-routing, app-prefetch-vary-analysis, app-page-probe, app-layout-param-observation, app-page-element-builder, app-rsc-cache-busting, thenable-params, link — 352 tests green), and vp check is clean on all changed files.
Producer invariant (defensive validator) — confirmed safe
The new isOrdinalRecordCoveredByGlobalUnion validator rejects a per-element map that references ordinals absent from the global union. getPrefetchVaryMetadata builds pageDynamicSuspenseOrdinals as the union of every element's ordinals by construction (app-layout-param-observation.ts:289-297), so legitimate producer metadata always satisfies the invariant — the check only guards against tampered wire data and never spuriously downgrades a valid payload.
Minor / non-blocking (unchanged from prior rounds)
parseFooterCandidate/stripRscCompletionMetadatastillthrow/erroron a> MAX_FOOTER_BYTEScandidate. This is now unreachable from our own producer (the encoder can no longer emit one), so it's purely defensive against corrupted input — fine to keep.optimisticRouteTemplateskeyed byvariantKeycan still accumulate entries for routes hit with many distinct observed param values; cleared on prefetch invalidation, no per-route cap. Matches existing prefetch-cache behavior.- Findings #1 (build-time I/O gating) and #3 (
JSON.stringifycomposite keys) from prior rounds remain resolved. - Dev/prod parity holds (App-Router-only; prod delegates to the built RSC entry).
Verdict
The remaining actionable correctness finding from round 2 is resolved. The overflow path now degrades conservatively (over-vary / stricter reuse) rather than permissively, header and footer degrade in lockstep, and the new tests lock in the correct behavior. The feature remains fully gated behind the experimental triple-gate (cacheComponents && varyParams && optimisticRouting), so it defaults off. No blocking findings.
|
Review posted to PR #2861. SummaryI re-reviewed the head commit What changed and why it's correct: The prior fix silently dropped the footer on overflow, falling back to the permissive pre-render header snapshot — which could reuse one cached segment across param values that actually vary (the exact bug the feature prevents). The new commit replaces that with a fail-closed candidate ladder applied to both the header and footer encoders:
I verified both new flags enforce stricter reuse on the client:
Both header and footer now route through the same conservative encoders, closing the parity gap. The tests were also upgraded from "assert Verification: 352 targeted tests green locally, Verdict: No blocking findings. Findings #1 and #3 from prior rounds remain resolved; #2 is now resolved. Feature stays fully gated behind the experimental triple-gate (defaults off). |
…y-params # Conflicts: # packages/vinext/src/shims/link.tsx # packages/vinext/src/shims/navigation.ts
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
…y-params # Conflicts: # packages/vinext/src/shims/root-params.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk review — PR #2861 (round 4, head 93dbe24)
Independent full re-review of all 40 changed files at head 93dbe24 (merge of main into the feature branch; last feature commit 68f1163). The three actionable findings from prior rounds remain resolved, all targeted suites are green locally, and vp check is clean on the changed source. No blocking findings.
Verified resolved
- #1 (build-time I/O gating) —
analyzeAppPrefetchCapabilitiesis now gated:toLinkPrefetchRouteskips it whenprefetchVaryEnabled === false(app-browser-entry.ts:126), and both call sites inindex.ts(:1522,:3828) compute the flag fromcacheComponents && varyParams && optimisticRouting. Default path is I/O-free. ✅ - #2 (footer overflow) — Fail-closed candidate ladder in
encodeFooter/encodePrefetchVaryHeader(rsc-completion-metadata.ts:124-215). Header and footer degrade in lockstep through the same conservative encoders (app-page-render.ts:897-913), so overflow over-varies (varyAllParams/pageAllSuspenseDynamic) rather than reusing across params that vary. ✅ - #3 (composite key ambiguity) —
encodeAppPrefetchRuntimeTemplateVariantKeyusesJSON.stringify([a, b])(app-route-prefetch-policy.ts:261-266). ✅
New observations this round
A. Cache-busting back-compat (outside the feature gate) — verified correct. app-rsc-cache-busting.ts adds VINEXT_PREFETCH_VARY_REQUEST_HEADER to the current hash input (and to VINEXT_RSC_VARY_HEADER). This is the one change that touches the non-gated request path for all users. The rollout is handled correctly: previous* hashes strip only the prefetch-vary header, and the new older* hashes strip both prefetch-vary and render-mode, so in-flight requests from an older client during a deploy still match an accepted hash. tests/app-rsc-cache-busting.test.ts (38 tests) covers this and passes.
B. Producer invariant holds by construction. getPrefetchVaryMetadata builds pageDynamicSuspenseOrdinals as the union of every element's ordinals (app-layout-param-observation.ts), so the defensive isOrdinalRecordCoveredByGlobalUnion validator never spuriously downgrades legitimate producer metadata — it only guards tampered wire data. Confirmed.
Minor / non-blocking
- Latent trap: the sync Suspense-replacement branch in
createOptimisticRouteElementsis effectively dead but silently diverges from the probe/async ordinal counting. The probe (app-page-probe.ts:215) and the async replay (replaceObservedDynamicSuspenseChildrenAsync, which resolvesreact.lazynodes viareadReactLazyNode) both count Suspense ordinals through lazy subtrees. The syncreplaceObservedDynamicSuspenseChildren(app-optimistic-routing.ts:598-604) does not resolve lazy nodes, so it would miscount ordinals for any serialized tree containing a lazy node followed by a Suspense boundary. Today this is unreachable: everypreservePageElementstemplate is always run through the asyncprepareOptimisticRouteTemplatebefore being stored (app-browser-entry.ts:516-518), which setspageElementsPrepared = true, andcreateOptimisticRouteElementsthencontinues past the sync branch (:593). So it's not a live bug — but it's a correctness landmine for a future refactor that ever stores an unprepared preserve-template. Consider either deleting the sync suspense-replacement branch (rely on the async prepare) or adding anassert(pageElementsPrepared)there so the invariant can't silently regress. optimisticRouteTemplateskeyed byvariantKeycan accumulate entries for a route hit with many distinct observed param values; cleared on prefetch invalidation, no per-route cap. Matches existing prefetch-cache behavior (unchanged from prior rounds).- Dev/prod parity holds — App-Router-only; prod delegates to the built RSC entry.
Verification
vp test run:rsc-completion-metadata(22),app-optimistic-routing(20),app-prefetch-vary-analysis(12),app-layout-param-observation(6),app-page-probe(40),app-page-element-builder(65),app-rsc-cache-busting(38),thenable-params(22),link,next-config(216),entry-templates(54) — all green.vp checkclean on all changed source files.
Verdict
The feature is fully gated behind the experimental triple-gate (cacheComponents && varyParams && optimisticRouting) and defaults off. All prior findings are resolved. The remaining item is a non-blocking maintainability note about dead-but-divergent sync code. No blocking findings for merge.
|
Review posted to PR #2861. SummaryI performed an independent full re-review of all 40 changed files at head Verified resolved:
New this round:
One non-blocking maintainability note: the sync Suspense-replacement branch in All targeted test suites (rsc-completion-metadata, app-optimistic-routing, app-prefetch-vary-analysis, app-page-probe, app-layout-param-observation, thenable-params, entry-templates, and more) pass locally, and |
# Conflicts: # packages/vinext/src/client/vinext-next-data.ts # packages/vinext/src/entries/app-browser-entry.ts # packages/vinext/src/shims/internal/app-route-prefetch-policy.ts # packages/vinext/src/shims/link.tsx # packages/vinext/src/shims/navigation.ts
1 participant
Summary
Original failures
This addresses the 11 failures for
test/e2e/app-dir/segment-cache/vary-params/vary-params.test.tsreported by deploy-suite run 31290819291:Validation
vp test runexpanded focused set: 820/820 on the independently reviewed headvp check: cleanIndependent cumulative review of
8d336dd96b0d4bf133312c5354abf67abfe396a1: no findings.