fix(cache): support segment prefetch inlining - #2855
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: segment prefetch inlining
Reviewed the 23 changed files with focus on correctness, security, dev/prod parity, and the build/runtime hint pipeline. This is a solid, well-tested change. Focused Vitest suites (app-route-tree-prefetch, app-prerender-endpoints, inject-pregenerated-paths) pass 36/36 locally, and targeted vp check is clean.
Findings
1. (Non-blocking, perf) Children-slot page is measured twice.
In measureAppRouteTreePrefetchSizes (app-route-tree-prefetch.ts:355-380), when a children slot's page is promoted to pageValue (line 375), the same decoded element is measured once via slotTasks and again via the page measurement in the final Promise.all. That's two _prerender + gzip passes over an identical model per children-slot route. It's correct (the test sizes.page).toBe(sizes.slots[childrenPage]) confirms parity), but each redundant measurement re-encodes a Flight model during the build. Consider reusing the slot measurement for page when pageValue === childrenSlotPageValue. Low priority given build-time only.
2. (Non-blocking, robustness) Early return in __collectPrefetchHints skips head-map cleanup.
In entries/app-rsc-entry.ts:741, when matchRoute misses / pattern mismatches / the route is a route handler, the function returns null before deleting the captured head from __prefetchHeadsByAffinity (the delete is at line 744, after the guard). This is currently harmless because the finally block in prerenderApp (build/prerender.ts:1666-1685) always issues the affinity DELETE, which calls discardPrefetchHead. So the entry is always reclaimed. But the safety net is the only thing preventing an unbounded per-build accumulation — worth a short comment noting the dependency, or moving the delete above the guard so cleanup is local to the collector.
Verified as correct (no action needed)
- Slot-id reconstruction matches the wire codec.
buildTreereconstructsownerTreePathviaroute.routeSegments.slice(0, ownerPosition)andencodeSlotId(slot.name, ownerTreePath).ownerPositionis a tree position fromlayoutTreePositions, and this matchescreateAppPageTreePath(routeSegments, treePosition)(app-page-route-wiring.tsx:304-313) used to encode slot ids at render time. Repeated slot names at different tree paths stay distinct (covered by the new "keeps full parallel-slot identities" test). - Affinity header is a trusted-only channel. Read only when
VINEXT_PRERENDER === "1"and the prerender secret matches, in bothapp-rsc-handler.ts:1694andprod-server.ts:1141. It's added toVINEXT_INTERNAL_HEADERSand stripped byfilterInternalHeaders(regression test added inrequest-pipeline.test.ts). No forgeability from external requests. - Body buffering in
rscHandler. Switchingbody: req.body-> bufferedarrayBuffer()(build/prerender.ts:1126) is correct and necessary: it avoids streaming-body/duplexissues when forwarding POSTs (prefetch-hints, static-params) to the render pool, andrscData.slice().bufferproduces a right-sized transfer for the hints POST. HEAD_OUTLINEDbit is intentional (asserted by the "outlines an oversized measured head" test) and rides throughstripMutableFieldsverbatim to the client.getPrecomputedTree/isTreePrefetchprovides sound defense-in-depth validation of the deep-frozen build-time hint tree, correctly guards the optionalpatternfield, and cycle-guards via aWeakSet.
Parity note
Confirm the head-inlining gate change from node.name === PAGE_SEGMENT to !hasChildren (leaf) in computePrefetchHints (app-route-tree-prefetch.ts:477-486) matches Next.js's algorithm. I could not diff against the upstream implementation because .nextjs-ref is not present in this environment. The change is plausible (a __PAGE__ node is always a leaf) and the parity E2E suite reportedly passes 7/7, so this is a request to double-check against packages/next/src/.../segment-cache rather than a suspected defect.
Overall: no blocking issues. The two non-blocking items are worth a follow-up but do not affect correctness given existing coverage.
|
Review posted to PR #2855. Summary of my analysis: Review outcome: no blocking issuesThe PR is well-structured and well-tested (36/36 focused Vitest, clean Non-blocking findings:
Verified correct (no action):
Parity note: I flagged the head-inlining gate change ( |
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Addressed every actionable Bonk item in 22a52d1:
Re-ran the exact targeted Next.js wrapper: 7/7 tests and 7/7 snapshots. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
Final follow-up on a64a915:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Follow-up review: segment prefetch inlining (22a52d1)
Independent re-review after the feedback commit. Focus: verifying the two addressed items and the previously-unverified parity claim, plus a fresh pass over the affinity channel and the build-artifact injection path. Cloned .nextjs-ref@canary (unavailable in the prior run) to diff the upstream algorithm directly.
Verification results (36/36 → 242/242 focused Vitest green, targeted vp check clean).
Both prior findings correctly resolved
- Double-measurement of children-slot pages — fixed.
measureAppRouteTreePrefetchSizesnow reuses the slot'sPromise<number|null>forpagewhenpageValue === undefined(app-route-tree-prefetch.ts:356-379), and the newrendered.filter(... === "children page").toHaveLength(1)assertion locks the single-render contract. - Head-map cleanup ordering — fixed.
__collectPrefetchHintsnow reads +deletes the affinity head before thematchRoute/route-handler early return (app-rsc-entry.ts:740-743), so cleanup is local to the collector and no longer depends solely on thefinally-block DELETE.
Parity claim verified against upstream (the prior open question)
The head-inlining gate change (node.name === PAGE_SEGMENT → !hasChildren) matches Next.js packages/next/src/server/app-render/collect-segment-data.tsx:792-805. Upstream gates on !headInlineState.inlined && isBundleTerminal && isPageSegment, where isBundleTerminal = !didInlineIntoChild && !isStaticPrefetchDisabled and isPageSegment = segment === PAGE_SEGMENT_KEY. In vinext's tree model this reduces cleanly to !hasChildren:
- Every branch leaf in
buildTreeis a__PAGE__node (app-route-tree-prefetch.ts:242-293, including slotpage ?? defaultleaves), so!hasChildren ⟺ isPageSegment. didInlineIntoChildis always false at a leaf (no children), and vinext has noStaticPrefetchDisabledconcept, soisBundleTerminalis constant-true at leaves.
The substitution is a sound simplification for vinext's flattened wire tree, not a divergence. HEAD_OUTLINED-on-root when no page accepts the head also matches upstream collect-segment-data.tsx:571-575 (createRouteTreePrefetch:543-545).
Fresh checks (no findings)
- Affinity channel security.
renderAffinityis a monotonic per-render counter (unique head-map key), while pool routing usesaffinityIndex % poolSize— HTML render, hints POST, and DELETE cleanup all carry the same value, so they deterministically hit the same worker where the head was captured. The header is inVINEXT_INTERNAL_HEADERS(stripped byfilterInternalHeaders) and re-attached only after=== prerenderSecretvalidation at the prod-server boundary (prod-server.ts:1141-1161); the!== nullcheck inapp-rsc-handler.ts:1694-1698is the documented downstream secondary gate after upstream verification. - Build-artifact injection.
injectPregeneratedConcretePathsembedsJSON.stringify(prefetchHints)as a JS expression (not inside<script>), sourced from trusted build-time prerender output, and the runtime re-validates viaisTreePrefetch/getPrecomputedTree(deep, cycle-guarded,pattern-optional). No external input reaches this path. - Fixture parity.
no-inline.tsxnow mirrors upstream's ≥2KiB-gzip"use cache"component with a 10k-iteration safety cap.
Verdict: no blocking issues. The feedback commit resolves both prior items, and the head-inlining gate is confirmed to match upstream.
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment prefetch inlining (a64a915)
Fresh independent pass focused on correctness, dev/prod parity, and build-failure modes. Type-checks clean on all touched server/build files; focused Vitest green (app-route-tree-prefetch, app-prerender-endpoints, inject-pregenerated-paths = 36/36). The change is well-structured and the two prior Bonk findings (double-measurement, head-map cleanup ordering) are correctly resolved. The upstream parity claim (!hasChildren leaf gate) is sound.
One item worth addressing before merge, plus a couple of low-priority notes.
1. (Should fix) A hint-collection null turns into a hard build failure
handlePrefetchHintsEndpoint returns notFoundResponse() (404) whenever collectPrefetchHints yields null (app-prerender-endpoints.ts:87), and __collectPrefetchHints returns null on any of: matchRoute(pathname) miss, match.route.pattern !== pattern, or match.route.routeHandler (app-rsc-entry.ts:746). In prerenderApp, a non-ok hints response is escalated to a thrown error that fails the whole build:
// build/prerender.ts:1594
const hintsResponse = await rscHandler(hintsRequest);
if (!hintsResponse.ok) {
const detail = await hintsResponse.text();
throw new Error(`[vinext] prerenderApp: prefetch hint collection returned ${hintsResponse.status} ...`);
}
For normal pages the concrete urlPath re-matches its own routePattern, so this is fine in the happy path. But __collectPrefetchHints re-runs matchRoute(pathname) independently of the route being prerendered — so any routing asymmetry between "the route we chose to prerender" and "what matchRoute resolves the concrete path to" (a more-specific static route shadowing a dynamic pattern, an intercept/parallel-route edge, a rewrite-affected path) would produce a null, a 404, and a hard build failure rather than a soft "no hints for this route."
This is a fragile failure mode for a build-time optimization: prefetch inlining is a performance feature, and its hint collection failing to match should degrade to "skip hints for this route," not abort the build. Recommend either:
- have the endpoint distinguish "collector errored" (500 → throw) from "no hints / route mismatch" (e.g. return 200 with an empty/absent tree, or a dedicated 204), and treat the latter as
prefetchHints = undefinedinprerenderApp; or - at minimum, in
prerenderApptreat a 404 from the hints endpoint as "no hints" (skip) and only throw on 5xx.
A regression test that prerenders a route whose concrete path resolves to a different pattern (or a route-handler-shadowed path) would lock this in.
2. (Low, type hygiene) pattern is read off AppRouteTreePrefetchRoute but not declared on it
getPrecomputedTree gates on "pattern" in route and then reads route.pattern (app-route-tree-prefetch.ts:431-432), but AppRouteTreePrefetchRoute (line 47) has no pattern field. It type-checks today only because the in narrows to Record<"pattern", unknown>, and it works at runtime because the concrete AppRscHandlerRoute structurally carries pattern. This is the load-bearing key that joins build-time writePrerenderIndex (prefetchHints[route.route]) to runtime lookup (__VINEXT_PREFETCH_HINTS__[route.pattern]). Adding an explicit optional pattern?: string to AppRouteTreePrefetchRoute would make that contract self-documenting and guard against a future rename silently breaking the precomputed-tree path (which has no runtime assertion that a hit was expected).
Verified correct (no action)
- Affinity channel.
renderAffinityis a monotonic counter used as (a) the head-map key and (b)affinityIndex % renderPorts.lengthpool routing, so HTML render, hints POST, and DELETE cleanup deterministically hit the worker that captured the head. Header is inVINEXT_INTERNAL_HEADERS, stripped byfilterInternalHeaders, and only re-attached after=== prerenderSecretat both boundaries (prod-server.ts:1141,app-rsc-handler.ts:1694). Not externally forgeable. - Head cleanup ordering.
__collectPrefetchHintsnow reads + deletes the affinity head before the early return, so it no longer depends solely on thefinallyDELETE. - Body buffering in
rscHandler(prerender.ts:1126) is required for POST forwarding of the hints body. - Build-artifact injection.
injectPregeneratedConcretePathsdeep-freezes the hint object in-process and emits a matching self-freezing IIFE into the bundle; runtime re-validates viaisTreePrefetch/getPrecomputedTree(deep, cycle-guarded, optionalpattern). Trusted build-time source only. - Second
no-storegate (prerender.ts:1604) correctly still returnsstatus: "skipped"while attaching hints for cache-component dynamic routes.
Overall: no correctness defects in the happy path and good coverage. Finding #1 is the one I'd want resolved (or explicitly waved off with reasoning) before merge — it converts a benign "no hints" into a build abort.
|
Review posted to PR #2855. SummaryI reviewed the segment prefetch inlining PR ( One item worth fixing before merge:
Low-priority note:
Verified correct (no action): affinity channel security & determinism, head-map cleanup ordering, body buffering, deep-freeze injection parity, and the second No happy-path correctness defects. Finding #1 is the one I'd want resolved or explicitly waved off before merge. |
|
Addressed the exact-head Bonk finding in 4862dcb:
Validation: 326/326 affected tests, full check + Knip, and the exact targeted Next.js wrapper at 7/7 tests + 7/7 snapshots. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment prefetch inlining (4862dcb)
Independent pass on the final head, focused on whether the three prior Bonk findings are actually resolved and on the seams they didn't cover (layout-size alignment, param validator/producer parity, affinity concurrency safety, ESM injection ordering, cleanup on non-hint render paths). Focused Vitest green locally: app-route-tree-prefetch + app-prerender-endpoints + inject-pregenerated-paths = 36/36; app-router-next-config-codegen + prerender + request-pipeline = 236/236.
Verdict: no blocking issues. All three prior findings are correctly resolved and I found no new correctness defects.
Prior findings — confirmed resolved
- Double-measurement of children-slot pages — fixed. When
pageValue === undefined,pageMeasurementreuses thechildrenslot'sPromise<number|null>rather than re-rendering (app-route-tree-prefetch.ts:356-379). One_prerender+gzip pass per children-slot page. - Head-map cleanup ordering — fixed.
__collectPrefetchHintsreads +deletes the affinity head before thematchRoute/pattern-mismatch/route-handler early return (app-rsc-entry.ts:744-746), so cleanup is local to the collector and no longer relies solely on thefinallyDELETE. - Hint-collection
null→ hard build failure — fixed.prerenderAppnow soft-skips a 404 from the hints endpoint (prerender.ts:if (hintsResponse.status === 404) { await hintsResponse.body?.cancel(); }) while still throwing on non-404 non-ok (5xx collector errors). The successfully rendered route is preserved.tests/prerender.test.tscovers the 404 soft-skip path.
Fresh checks (no findings)
- Affinity concurrency safety. Renders run under
runWithConcurrency, so multiple renders can be in flight on the same worker (affinityIndex % poolSize). BecauserenderAffinityis a monotonic per-render counter used as the head-map key, concurrent renders on one worker never collide, and each render'sfinallyissues its own keyed DELETE. HTML render, optional RSC render, hints POST, and DELETE cleanup all carry the same affinity, so they deterministically hit the worker that captured the head. paramvalidator ↔ producer parity.isTreePrefetch'sparamacceptance (type ∈ {d,c,oc},key === null,siblings === null | string[]) exactly matchesdynamicRouteTreeSegmentoutput (app-route-tree-prefetch.ts:166-182). Static segments (param === null) are accepted. No legitimate producer output is rejected by the runtime re-validator.route.patternis present at runtime.getPrecomputedTreereadsroute.pattern;AppRscHandlerRoutedeclarespattern: string(app-rsc-handler.ts:173), and this PR adds the explicit optionalpattern?: stringtoAppRouteTreePrefetchRoute, documenting the build↔runtime join key (prior finding #2).- Precomputed tree is served verbatim without
stripMutableFields, correctly. The precomputed tree originates from build-time collector output that is alreadyTreePrefetch-shaped (mutable fields stripped) and deep-frozen; the request path onlyJSON.stringifys it, so freezing is inert. The fallback (getPrecomputedTree→ null) still runsbuildTree+computePrefetchHintswithHEAD_INLINE_SIZE, preserving pre-PR heuristic behavior. - ESM injection ordering.
injectPregeneratedConcretePathsprepends the__VINEXT_PREFETCH_HINTS__IIFE above the file's imports — same pattern as the existing pregenerated-paths block, which already ships in production.importhoisting makes a top-level statement before imports legal. - Security gating unchanged and sound. The prefetch-hints endpoint 403s without a matching prerender secret (
prod-server.ts:1684-1689); the affinity header is read only whenVINEXT_PRERENDER === "1"+ secret matches (app-rsc-handler.ts:1691-1695,prod-server.ts:1141-1146), is inVINEXT_INTERNAL_HEADERS, and is stripped byfilterInternalHeaders(regression test inrequest-pipeline.test.ts). Not externally forgeable. - No head-map leak on non-hint render paths. Fallback renders (
isFallback) still capture a head but skip the hints POST; theconfig.prefetchInlining-gatedfinallyDELETE always reclaims it. Errored renders that captured a head are likewise reclaimed. - Default-bundle regression avoided.
generateRscEntry(..., prefetchInlining=false)omitsapp-route-tree-prefetch.js,createFromReadableStream, and__collectPrefetchHints(codegen assertions inapp-router-next-config-codegen.test.ts), so the measurement runtime stays off the common path.
One low-priority note (non-blocking)
- Layout-size positional alignment is an implicit contract.
measureAppRouteTreePrefetchSizesbuildslayoutsby iteratingelements.__layoutIdsin order, andbuildTreereassociates them viamodulesByTreePosition(measuredSizes.layouts, route.layoutTreePositions)— i.e. it assumes__layoutIds[i]corresponds tolayoutTreePositions[i]. This holds today and the E2E parity suite exercises it, but the two orderings are produced by different modules with no shared assertion binding them. A regression here would silently mis-attribute a layout's measured size to the wrong tree position (a wrong inlining decision, not a crash). Worth a comment noting the ordering dependency, or a focused test that mismatches the two arrays. Low priority — not required for merge.
Overall: the PR is well-structured, well-tested, and the three review rounds have converged cleanly. Nothing here should block merge.
|
Review posted to PR #2855. Review outcome: no blocking issuesI did an independent pass on the final head ( Prior findings confirmed resolved:
Fresh checks (no findings): affinity concurrency safety (monotonic per-render key, no collision under One low-priority note (non-blocking): layout-size positional alignment ( The PR is well-structured, well-tested, and the review rounds have converged cleanly. Nothing should block merge. |
|
Backlog mapping for Actions run |
# Conflicts: # packages/vinext/src/build/prerender.ts # packages/vinext/src/server/app-prerender-endpoints.ts # packages/vinext/src/server/app-rsc-handler.ts # packages/vinext/src/server/headers.ts # packages/vinext/src/server/prod-server.ts
1 participant
Summary
Upstream parity target
test/e2e/app-dir/segment-cache/prefetch-inlining/prefetch-inlining.test.tsValidation