fix(app-router): reuse visited responses for full prefetch - #2858
fix(app-router): reuse visited responses for full prefetch#2858james-elicx wants to merge 7 commits into
Conversation
|
Mapping note for the segment-cache backlog from Actions run |
|
/bigbonk review for issues |
commit: |
|
3b35846 to
8f2d014
Compare
|
Updated exact-head verification after the CI-only Knip cleanup: the targeted Next.js force-stale wrapper still passes 2/2 at |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(app-router): reuse visited responses for full prefetch
Reviewed the exact head. The core logic in claimVisitedResponseCacheEntryForFullPrefetch is sound: it scans canonical _rsc aliases, selects the newest claimable (non-stale, mounted-slot-compatible) entry, promotes its expiresAt with Math.max so a longer dynamic deadline is never shortened, and re-inserts it as MRU without deleting older history entries. The router.prefetch/Link wiring, one-shot retainPrefetchInvalidationUntil expiry, and integration with invalidatePrefetchCache (cache-clear notifications) all look correct, and the targeted tests pass locally.
One notable finding below.
Dead code: unused options plumbing on findVisitedResponseCacheEntry
packages/vinext/src/server/app-visited-response-cache.ts
The PR adds a VisitedResponseCacheLookupOptions type and threads an options parameter through findVisitedResponseCacheEntry (the isEntryCompatible / mountedSlotsHeader compatibility gates on both the exact-key and alias-scan branches):
type VisitedResponseCacheLookupOptions = {
isEntryCompatible?: (
entry: VisitedResponseCacheEntry,
mountedSlotsHeader: string | null,
) => boolean;
mountedSlotsHeader?: string | null;
};But nothing ever passes these options. All four call sites of findVisitedResponseCacheEntry (two in source — deleteVisitedResponseCacheEntry and app-browser-entry.ts:707 — and two in tests/app-visited-response-cache.test.ts) call it with exactly three positional args. The new full-prefetch path (claimVisitedResponseCacheEntryForFullPrefetch) does not delegate to findVisitedResponseCacheEntry; it implements its own scan loop (because it needs newest-wins selection plus in-place mutation) and calls isVisitedResponseCacheEntryCompatibleForFullPrefetch directly.
So the VisitedResponseCacheLookupOptions type (lines 11-17) and the added compatibility branches inside findVisitedResponseCacheEntry (lines 103-108 and 119-125) are unreachable. Suggest removing them to keep the module lean, or, if the intent was for claimVisitedResponseCacheEntryForFullPrefetch to reuse findVisitedResponseCacheEntry, wire it up and drop the duplicate scan. Either way there is currently untested, unused branching in a hot lookup helper.
Question (non-blocking): claiming for full-after-shell / unstable_dynamicOnHover
In link.tsx, the claim fires for mode !== "auto", which includes full-after-shell (the unstable_dynamicOnHover intent mode), not just full. router.prefetch gates the same behavior on kind === "full" only. Reusing an already-committed visited response for a dynamic-on-hover prefetch could serve staler dynamic content than a fresh shell+full fetch would. Is the full-after-shell inclusion deliberate and matched to Next.js's Segment Cache behavior? A short comment or a test asserting the full-after-shell path would remove the ambiguity. (Could not verify against .nextjs-ref — the local clone is not present in this environment.)
Verified
vitest runon the three touched test files: 168 passed.tsc --noEmitonpackages/vinext: clean.retainPrefetchInvalidationUntilcorrectly participates ininvalidatePrefetchCache's retained-invalidation sweep, and theMath.max(0, expiresAt - Date.now())timer fires the callback exactly once (covered by the two new prefetch-cache tests).- Existing
findVisitedResponseCacheEntrycallers are unaffected becauseoptions.mountedSlotsHeader === undefinedshort-circuits the new branches (though that only matters because the branches are otherwise dead).
|
Review posted to PR #2858. SummaryI reviewed PR #2858 at exact head and posted a review. The core full-prefetch reuse logic is sound and well-tested (168 tests pass locally, Dead code (main finding): The PR adds a Non-blocking question: Link's claim fires for Verified good: |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
8f2d014 to
b4a69a3
Compare
|
Updated exact head b4a69a3 with the exact-deadline fix and removal of dead lookup-options plumbing. Current validation:
/bigbonk review for issues |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): reuse visited responses for full prefetch — exact head b4a69a3d1
Re-reviewed at the current head. The two items from the prior /bigbonk review are resolved:
- Dead lookup-options plumbing removed.
findVisitedResponseCacheEntryis back to three positional args, andVisitedResponseCacheLookupOptions/ theisEntryCompatiblecompatibility branches are gone. Confirmed no dangling references (grepforVisitedResponseCacheLookupOptions/isEntryCompatibleacrosssrcandtestsreturns nothing). The module is lean again. - Exact-deadline fix. The claim window now excludes the boundary via
now >= entry.createdAt + staleTimeMs(app-visited-response-cache.ts:164). Thepreserves an old BFCache response for back/forward traversaltest pins this: atnow === createdAt + staleTimeMsthe entry is not claimable and stays available for traversal restore.
Verified good
- Newest-wins alias scan (
app-visited-response-cache.ts:154-167): scans every canonical_rscalias, selects the newest claimable entry, and re-inserts as MRU without deleting older/expired history entries.Math.maxdeadline promotion never shortens a longer dynamic lifetime. Covered by the alias-scan and dynamic-deadline tests. retainPrefetchInvalidationUntil(navigation.ts:896-909) participates ininvalidatePrefetchCache's retained-invalidation sweep (navigation.ts:1062) and fires the caller'sonInvalidateexactly once at the static deadline viafireRetainedPrefetchInvalidation(self-deleting,clearTimeout).- Claim-then-return without
prefetched.addin both Link (link.tsx:652-657) and router.prefetch (navigation.ts:2779-2785): re-claiming is idempotent (re-promotesexpiresAt, re-inserts MRU), and if the claimed entry later ages out the path correctly falls through to a real fetch. Good design, consistent across both callers. - Client-only seam. The capability is wired through the single browser runtime seam (
app-browser-entry.ts:2391); no prod-server / worker-entry parity concern since this is browser navigation runtime only. - Focused suites pass locally:
app-visited-response-cache19/19,link-navigation+prefetch-cache149/149.vp checkon all five changed files is clean.
Non-blocking (carried over): full-after-shell claim gate
The mode/kind gate is intentionally asymmetric and, on inspection, correct — not a bug:
router.prefetchonly ever resolveskindto"full"or"auto"(navigation.ts:2721), sokind === "full"is the complete gate there.- Link's
mode !== "auto"(link.tsx:645) additionally admits"full-after-shell", which is only reachable viaunstable_dynamicOnHover(link.tsx:1316-1320). On hover, Link already callspromotePrefetchEntriesForNavigation(link.tsx:1322) — the intent is explicit navigation, so reusing a recently-visited response within the bounded stale window is aligned, not a staleness regression. ThePREFETCH_CACHE_TTL-from-creation bound prevents serving anything genuinely stale.
That said, the clarity gap the prior review raised still stands: there is no test exercising the full-after-shell claim path and no inline comment explaining why the gate is mode !== "auto" rather than mode === "full". A one-line comment at link.tsx:645 (or a small full-after-shell claim test mirroring the existing prefetch: true test) would remove the ambiguity for future readers. Non-blocking.
Overall: the core reuse logic is sound and well-tested, the prior findings are addressed, and I found no new correctness issues.
|
Review posted to PR #2858. SummaryRe-reviewed exact head Prior findings — resolved:
Verified good:
Non-blocking (carried over): The |
b4a69a3 to
ba3e51e
Compare
|
Updated exact head ba3e51e to address the final Bonk clarity note for the full-after-shell claim path. Current exact-head validation:
All CI checks on the preceding functional head were green; fresh CI is now running for this comment-only follow-up. |
|
/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 |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): reuse visited responses for full prefetch — exact head ba3e51e
Re-reviewed at the current head. The two prior /bigbonk findings (dead lookup-options plumbing, exact-deadline boundary) are still resolved, and the full-after-shell comment was added at link.tsx:644-645. The newest-wins alias scan, Math.max deadline promotion, and history-preservation all remain sound and well-tested.
However, I found one new correctness bug in the router.prefetch claim path.
Bug: repeated router.prefetch(href, { kind: "full", onInvalidate }) fires the callback more than once
packages/vinext/src/shims/navigation.ts:2772-2786 + retainPrefetchInvalidationUntil (navigation.ts:896-909)
When a Full prefetch claims a visited response, the callback is preserved via retainPrefetchInvalidationUntil, which unconditionally allocates a brand-new RetainedPrefetchInvalidation record (with its own Set and timer) and adds it to retainedPrefetchInvalidations on every call:
function retainPrefetchInvalidationUntil(onInvalidate, expiresAt): void {
if (onInvalidate === undefined) return;
const retained = { callbacks: new Set([onInvalidate]), timer: setTimeout(...) };
retainedPrefetchInvalidations.add(retained); // new record every time
}There is no dedup keyed on the route/callback. So if the same route is Full-prefetched more than once with the same onInvalidate (common — hover re-prefetch, React re-renders, or repeated intent), each claim registers a separate retained record holding the same callback. On invalidatePrefetchCache() (navigation.ts:1062-1064), every record fires, so the caller's onInvalidate runs once per claim.
This diverges from the PR's own "one-shot onInvalidate expiry" goal and from the non-claimed path. The normal prefetch path dedups through a per-entry Set (addPrefetchInvalidationCallback, navigation.ts:1009-1018) so the same callback added N times still fires exactly once. The pre-existing retainPrefetchInvalidationAfterConsume (navigation.ts:911) is safe for the same reason — it hands over an already-deduplicated per-entry Set. The new claim path has no cache entry to attach to, so it bypasses that dedup entirely.
Repro (fails on this head):
appRouterInstance.prefetch("/reports", { kind: "full", onInvalidate });
await waitForPrefetchSetup(() => claim.mock.calls.length > 0);
appRouterInstance.prefetch("/reports", { kind: "full", onInvalidate });
await waitForPrefetchSetup(() => claim.mock.calls.length > 1);
invalidatePrefetchCache();
expect(onInvalidate).toHaveBeenCalledTimes(1); // ← got 2I ran this locally against ba3e51e and it fails with expected 1 times, but got 2 times. The existing reuses a visited response for kind: "full" router.prefetch without fetching test only prefetches once, so it doesn't catch this.
Suggested fix: dedup retained claim callbacks per route (e.g. key retainedPrefetchInvalidations by cache key + callback, reusing/refreshing an existing record's timer instead of allocating a new one), mirroring the per-entry Set dedup used by addPrefetchInvalidationCallback. Add a regression test that prefetches the same kind: "full" route twice with the same onInvalidate and asserts a single fire.
Verified good
- Dead code stays removed:
findVisitedResponseCacheEntryis 3-arg; noVisitedResponseCacheLookupOptions/isEntryCompatiblereferences. - Newest-wins scan (
app-visited-response-cache.ts:154-167): compatibility filter runs before the createdAt comparison, so an incompatible newer alias never blocks an older compatible one; older/expired history entries are preserved (MRU re-insert only). - Exact-deadline exclusion (
app-visited-response-cache.ts:164) is consistent with the navigation freshness check (expiresAt > now): an entry claimed at the last claimable instant becomes non-fresh for navigation exactly at the deadline. Math.maxpromotion never shortens a longer dynamic deadline (covered by test).- Link claim path doesn't touch
onInvalidate(Link has no such option), so it's unaffected by the bug above; themode !== "auto"gate now has an explanatory comment. - Focused suites pass locally:
app-visited-response-cache19/19,link-navigation+prefetch-cache149/149.
The core reuse logic is correct; the blocker is the repeated-claim callback multi-fire in retainPrefetchInvalidationUntil.
|
Review posted to PR #2858. SummaryRe-reviewed at exact head Bug — repeated This diverges from:
I reproduced it locally against Verified good: newest-wins scan filters compatibility before the createdAt comparison, exact-deadline exclusion is consistent with navigation freshness, |
ba3e51e to
d2d44ef
Compare
|
Addressed the exact-head Bonk blocker at d2d44ef. The repeated full router.prefetch claim now reuses one retained invalidation record per cache key and deduplicates callbacks with the same Set semantics as a normal prefetch cache entry. Added a regression that failed 2 calls before the fix and now fires onInvalidate exactly once. Current validation:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): reuse visited responses for full prefetch — exact head d2d44ef
Re-reviewed at the current head. The prior /bigbonk blocker (repeated router.prefetch(kind: "full") firing onInvalidate more than once) is resolved, and I found no new correctness issues.
Prior blocker — resolved
retainPrefetchInvalidationUntil (navigation.ts:907-931) now dedups per route via a retainedClaimPrefetchInvalidations Map keyed by cacheKey. A repeated claim for the same key hits the existing branch, adds the callback into the shared Set (idempotent), and refreshes the timer only when a later deadline is proposed (expiresAt > existing.expiresAt). fireRetainedPrefetchInvalidation (navigation.ts:945-957) deletes the record from both the Set and the Map atomically before running callbacks, so no stale Map reference can survive a fire. The new regression (prefetch-cache.test.ts) prefetches the same kind: "full" route twice and asserts onInvalidate fires exactly once — it fails on the pre-fix head and passes here.
The dedup key (cacheKey = encodeCacheKey(rscUrl, interceptionContext)) is the same request-identity key the rest of the prefetch cache uses (prefetched.has(cacheKey), attachPrefetchInvalidationCallback). For a reusable full prefetch the header snapshot is normalized (navigation.ts:2761-2763 deletes NEXT_ROUTER_PREFETCH_HEADER), so repeated identical calls compute the same RSC cache-busting hash → identical rscUrl/cacheKey → correct dedup. Consistent with the existing per-entry Set dedup in addPrefetchInvalidationCallback.
Verified good
- No double-fire across normal + claim paths. The claim path (
navigation.ts:2800) is only reached whenhasPrefetchCacheEntryForNavigationreturns false, so a route with a livePrefetchCacheEntryattachesonInvalidateto that entry instead — the two retention mechanisms never both hold the same callback for the same route. invalidatePrefetchCachesweep (navigation.ts:1090-1092) iteratesretainedPrefetchInvalidations; each fire self-removes and cleans the claim Map. SecondinvalidatePrefetchCache()is a no-op (test covers idempotency).- Newest-wins alias scan (
app-visited-response-cache.ts:154-167): compatibility filter runs before thecreatedAtcomparison;Math.maxdeadline promotion never shortens a longer dynamic lifetime; older/expired history entries are preserved (MRU re-insert only). Covered by the promotion and dynamic-deadline tests. - Exact-deadline exclusion (
app-visited-response-cache.ts:164,now >= createdAt + staleTimeMs) keeps an at-deadline entry available for BFCache traversal. - Link claim path (
link.tsx:646-659):full-after-shellinclusion now has an explanatory comment; it doesn't touchonInvalidate(Link has no such option), doesn't add toprefetched, is idempotent on re-hover, and falls through to a real fetch if the entry ages out. - Client-only runtime seam (
app-browser-entry.ts:2391,navigation-runtime.ts): typed, optional, validated viaisOptionalRuntimeFunction; no prod-server/worker-entry parity concern.
Checks run locally at this head
app-visited-response-cache19/19,prefetch-cache73/73,link-navigation76/76,app-browser-entry234/234 — all pass.vp checkon all 5 changed source files: format, lint, and types clean.
No blocking issues. LGTM.
|
Review posted to PR #2858. SummaryRe-reviewed at exact head Prior blocker — resolved: The repeated-claim Verified good:
Checks at this head: focused suites 402/402 pass ( No blocking issues. |
…he-force-stale-current # Conflicts: # packages/vinext/src/shims/navigation.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): reuse visited responses for full prefetch — exact head 14b9156
Re-reviewed at the current head. All three prior /bigbonk findings remain resolved (dead lookup-options plumbing removed, exact-deadline boundary excluded, and the repeated-claim multi-fire dedup via retainedClaimPrefetchInvalidations). But the dedup fix from the last round only covers claim-then-claim; I found a new correctness bug in the sibling case: claim-then-real-fetch.
Bug: a claim followed by a real prefetch entry for the same route fires onInvalidate twice
packages/vinext/src/shims/navigation.ts:2817-2854
The dedup added last round keys retained claim callbacks by cacheKey in retainedClaimPrefetchInvalidations, so two claims for the same route share one record. But a claim and a PrefetchCacheEntry for the same route are stored in two independent structures — retainedClaimPrefetchInvalidations (the claim record) and the entry's own onInvalidateCallbacks Set — and invalidatePrefetchCache() sweeps both:
// navigation.ts:1115-1124
for (const [cacheKey, entry] of cache) {
deletePrefetchCacheEntry(cache, prefetched, cacheKey, entry, true); // fires entry.onInvalidateCallbacks
}
...
for (const retained of retainedPrefetchInvalidations) {
fireRetainedPrefetchInvalidation(retained); // fires the retained claim record
}So if the same onInvalidate ends up in both places for one route, it runs twice.
Reachable sequence (I reproduced it locally against this head):
router.prefetch("/reports", { kind: "full", onInvalidate })— no livePrefetchCacheEntry, claim succeeds →onInvalidateretained inretainedClaimPrefetchInvalidations["/reports…"].- Later
router.prefetch("/reports", { kind: "full", onInvalidate })— the visited response has since been consumed/aged, so the claim misses (claimVisitedResponseForFullPrefetchreturnsnull), the code falls through toprefetchRscResponse, which creates aPrefetchCacheEntryand attaches the sameonInvalidatetoentry.onInvalidateCallbacks. invalidatePrefetchCache()→ the entry sweep fires the callback once, then the retained-claim sweep fires it again.
Temp repro (fails on this head with expected 1 times, but got 2 times):
let claimCalls = 0;
const claim = vi.fn(() => { claimCalls += 1; return claimCalls === 1 ? Date.now() + 60_000 : null; });
// runtime.functions.claimVisitedResponseForFullPrefetch = claim
appRouterInstance.prefetch("/reports", { kind: "full", onInvalidate });
await waitForPrefetchSetup(() => claim.mock.calls.length > 0);
appRouterInstance.prefetch("/reports", { kind: "full", onInvalidate }); // claim misses → real fetch entry
await waitForPrefetchSetup(() => fetch.mock.calls.length > 0 || claimCalls > 1);
invalidatePrefetchCache();
expect(onInvalidate).toHaveBeenCalledTimes(1); // ← got 2This still diverges from the PR's stated "one-shot onInvalidate expiry" goal. The existing reuses a visited response for repeated kind:"full" router.prefetch test only exercises claim-then-claim (both claims succeed), so it doesn't catch the claim-then-fetch handoff.
Why the last round's fix is insufficient: the retainedClaimPrefetchInvalidations Map dedups within the claim path, but the claim record and the prefetch entry are separate sinks for the same callback and there is no cross-structure dedup or handoff between them. retainPrefetchInvalidationAfterConsume is safe only because it moves the entry's existing Set into the retained record (single sink); the claim path adds a second sink.
Suggested fix (pick one):
- When a real prefetch entry is created for a route that has a live retained-claim record (same
cacheKey), hand the claim's callbacks over to the entry (or drop the claim record), so a single sink owns the callback — mirroring theretainPrefetchInvalidationAfterConsumehandoff. - Or, before
attachPrefetchInvalidationToEntry/addPrefetchInvalidationCallbackin the reusable-fallback path, checkretainedClaimPrefetchInvalidationsfor the samecacheKeyand skip re-attaching a callback already retained there.
Add a regression that claims once, then forces a claim miss so a real prefetch entry is created for the same route with the same onInvalidate, and asserts a single fire.
Verified good (unchanged from prior rounds)
- Newest-wins alias scan (
app-visited-response-cache.ts:154-167): compatibility filter runs before thecreatedAtcomparison;Math.maxdeadline promotion never shortens a longer dynamic lifetime; older/expired history entries preserved (MRU re-insert only). - Exact-deadline exclusion (
app-visited-response-cache.ts:164) keeps an at-deadline entry available for BFCache traversal. - Repeated-claim dedup (
navigation.ts:939-963) — two claims for the same key share one record and only refresh the timer on a later deadline;fireRetainedPrefetchInvalidationcleans both the Set and Map atomically. - Link claim path (
link.tsx:646-661):full-after-shellinclusion is commented, doesn't touchonInvalidate, is idempotent on re-hover, and falls through to a real fetch if the entry ages out. - Client-only runtime seam (
app-browser-entry.ts:2391,navigation-runtime.ts:57): typed, optional, validated — no prod-server/worker-entry parity concern. - Focused suites pass at this head:
app-visited-response-cache19,prefetch-cache75,link-navigation77 (171 total);vp checkclean on all 5 changed files.
The core reuse logic is sound; the blocker is the claim-then-real-fetch callback double-fire on invalidatePrefetchCache().
|
Review posted to PR #2858. SummaryRe-reviewed at exact head Bug — claim-then-real-fetch double-fires
I reproduced it locally against this head (temp test failed Verified good: newest-wins scan filters compatibility before the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): reuse visited responses for full prefetch — exact head 8be232b
Re-reviewed at the current head, which contains a substantial rework (8be232b "preserve full prefetch freshness") that changes the whole callback-retention design. The prior /bigbonk blocker (claim-then-real-fetch onInvalidate double-fire) is resolved — structurally, not with a patch. No new correctness issues found.
Prior blocker — resolved by design change
The previous rounds retained claim callbacks in a side structure (retainedClaimPrefetchInvalidations / retainPrefetchInvalidationUntil), which created a second sink for the same callback and required per-key dedup that still didn't cover the claim-then-real-fetch handoff. That whole mechanism is gone (grep confirms zero references to retainedClaimPrefetchInvalidations / retainPrefetchInvalidationUntil in src and tests).
The claim now seeds a real PrefetchCacheEntry. The runtime seam (app-browser-entry.ts:2401-2413) calls seedPrefetchResponseSnapshot(rscUrl, createVisitedResponseFullPrefetchSnapshot(claim), …, PREFETCH_CACHE_TTL) on a successful claim, and router.prefetch then attaches onInvalidate to that entry via attachPrefetchInvalidationCallback(cacheKey, …) (navigation.ts:2804). This makes the entry the single sink for the callback, so every downstream path stays exactly-once:
- Claim-then-claim (same route): the second
router.prefetchno longer even re-claims —hasPrefetchCacheEntryForNavigationfinds the fresh seeded entry and attachesonInvalidateto it (navigation.ts:669), then returns. The updated test assertsclaimVisitedResponseForFullPrefetchis called once (not twice) andonInvalidatefires once. Correct. - Claim-then-real-fetch (seeded entry aged out):
prefetchRscResponsedeletes the pre-existing seeded entry withnotify: false(navigation.ts:1369-1371) before creating the fetch entry, so no double-fire; the freshness-gate delete atnavigation.ts:673-679fires the stale entry's callback exactly once for its own genuine expiry (a distinct invalidation event), which is the correct Next.js semantic. discardLearningOnlyPrefetchCacheEntrydoes not eat the seeded entry: it only targetscacheForNavigation !== false && prefetchKind === "navigation"(navigation.ts:994); the seeded entry leavesprefetchKindundefined, so it survives.
New in this head — verified good
- 30s stale-window floor.
claimVisitedResponseCacheEntryForFullPrefetchnow routes its stale time throughresolveFullPrefetchStaleTimeMs=Math.max(staleTimeMs, 30_000)(navigation.ts:291-293), matching Next.js'sgetStaleTimeMs(Math.max(staleTimeSeconds, 30) * 1000). Covered by the newit.each([0, 10_000, 29_999])"floors … static window" test. Good parity improvement over the prior?? PREFETCH_CACHE_TTL. - BFCache traversal boundary. The claim no longer mutates the visited entry in place — the earlier exact-deadline exclusion (
>=) was a workaround for that mutation and is now back to inclusive (now > createdAt + staleTimeMs,app-visited-response-cache.ts:179). This is safe because the visited entry's ownexpiresAtis left untouched (expect(entry.expiresAt).toBe(now)in the claim tests); traversal reads the untouched entry regardless of the claim boundary. The claimed deadline lives only on the separately-seeded prefetch snapshot. - Prepared-elements handoff.
createVisitedResponseFullPrefetchSnapshotcopiespreparedElementsonly when the visited entry has decodedelements, and does not mutateentry.response(expect(entry.response.preparedElements).toBeUndefined()). Snapshot-only entries still gate onmountedSlotsHeaderequality. - Newest-wins alias scan unchanged and still sound: compatibility filter runs before the
createdAtcompare;Math.maxdeadline promotion never shortens a longer dynamic deadline; older/expired history preserved (MRU re-insert only). - Link claim path (
link.tsx:646-661) shares the same runtime seam, so it inherits the seeding behavior; Link has noonInvalidate, thefull-after-shellinclusion is commented, and the path is idempotent on re-hover. - Client-only runtime seam — no prod-server / worker-entry parity concern.
Checks run locally at this head
app-visited-response-cache23/23,prefetch-cache75/75,link-navigation77/77 — all pass.vp checkon all 5 changed source files: format, lint, and types clean.
No blocking issues. The rework is a genuine simplification — collapsing two callback sinks into one — and it closes the claim-then-real-fetch gap at the design level rather than adding another dedup layer. LGTM.
|
Review posted to PR #2858. SummaryRe-reviewed at exact head Key finding — the fix is a design change, not a bandaid:
Also verified new-in-this-head:
Checks at this head: LGTM — no blocking issues. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): reuse visited responses for full prefetch — exact head d57fade
Independent review at the current head (d57fade, one test-only commit past 8be232b's freshness rework). All prior /bigbonk findings remain resolved and I found no new correctness issues. LGTM.
Design is sound — single-sink callback ownership
The core of this fix is that a claim seeds a real PrefetchCacheEntry rather than parking the callback in a side structure. This collapses the two-sink problem that produced the earlier double-fire bugs:
claimVisitedResponseForFullPrefetch(app-browser-entry.ts:2392-2412) claims a compatible visited response and immediatelyseedPrefetchResponseSnapshot(...)publishes it into the prefetch cache underencodeCacheKey(rscUrl, interceptionContext).router.prefetchthen attachesonInvalidateto that same entry viaattachPrefetchInvalidationCallback(cacheKey, …)(navigation.ts:2804) — the cache keys line up exactly (both derive from the samerscUrl/interceptionContext), so there is a single owner for the callback.- Repeat prefetch: the seeded entry is
cacheForNavigation: true, so the secondrouter.prefetchshort-circuits athasPrefetchCacheEntryForNavigation(navigation.ts:2786) and re-attaches to the existing entry rather than re-claiming — the test assertsclaimis called once. - Claim-then-real-fetch:
prefetchRscResponsedeletes any pre-existing seeded entry withnotify: false(navigation.ts:1369-1371) before creating the fetch entry, so the seed's callbacks are cleared (onInvalidateCallbacks = undefined) without firing — no double-fire.
Verified good
- Timer wiring for the seeded entry.
seedPrefetchResponseSnapshotcallsschedulePrefetchInvalidationwhileonInvalidateCallbacksis still undefined, so it no-ops (no orphan timer). The timer is correctly scheduled later insideattachPrefetchInvalidationToEntrybecause the entry'soutcome === "cache-seeded"(navigation.ts:1053-1055). Confirmed by the "invalidates … once at its static deadline" test. - Static-window expiry uses the original navigation time, not
Date.now().createVisitedResponseFullPrefetchSnapshotstampsexpiresAt: claim.expiresAt(=entry.createdAt + staleTimeMs);seedPrefetchResponseSnapshot→resolveCachedRscResponseExpiresAtreturns that verbatim via theisCacheExpiresAtshort-circuit (navigation.ts:484-486). So the seeded deadline is measured from the BFCache entry's creation, exactly as intended. - 30s static floor via
resolveFullPrefetchStaleTimeMs = Math.max(staleTimeMs, 30_000)matches Next'sgetStaleTimeMs(Math.max(staleTimeSeconds, 30) * 1000); the shared helper also now backs the tworesolvePrefetchedRscResponseExpiresAtfloors (navigation.ts:518,530), removing duplicatedMath.maxliterals. Covered by theit.each([0, 10_000, 29_999])floor test. - Visited entry is left untouched. The claim no longer mutates
entry.expiresAt— every claim test assertsentry.expiresAtis unchanged. Because of that, the BFCache traversal boundary can safely revert to inclusive (now > createdAt + staleTimeMs,app-visited-response-cache.ts:179); traversal reads the untouched entry regardless of the claim window. Thepreserves an old BFCache response for back/forward traversaltest pins this. - Newest-wins alias scan (
app-visited-response-cache.ts:169-181): the compatibility filter runs before thecreatedAtcomparison, so an incompatible newer alias never blocks an older compatible one; older/expired history entries are preserved (MRU re-insert only, no deletes). Covered by the alias-scan test. - Prepared-elements handoff copies
preparedElementsonly when the visited entry has decodedelementsand never mutatesentry.response(entry.response.preparedElementsstays undefined in the test). Consumption path is the existing one (navigation.ts:1574-1578,app-browser-entry.ts:2010). - Link claim path (
link.tsx:646-661):full-after-shellinclusion is commented and only reachable viaunstable_dynamicOnHover; Link has noonInvalidate, so it can't hit the callback machinery; it doesn't add toprefetched(the seed does) and is idempotent on re-hover because the seeded entry satisfies the freshness gate atlink.tsx:621. - Client-only runtime seam (
navigation-runtime.ts:57, validated viaisOptionalRuntimeFunction;app-browser-entry.ts:2392) — no prod-server / worker-entry parity concern.
Non-blocking observations
- Claim-at-exact-deadline seeds an immediately-dead entry. When
now === createdAt + staleTimeMs, the claim succeeds and seeds an entry withexpiresAt === now;hasPrefetchCacheEntryForNavigation's strict> Date.now()check then rejects it on the very next read and deletes it. Harmless (worst case: one wasted seed + a real fetch on the following navigation), and theadvanceTime(..., + 1)e2e adjustments show the boundary is exercised deliberately. Not worth changing. full-after-shellstill has no unit test. The Link claim path is exercised forprefetch: true(full) and correctly not forauto, but there's no test asserting themode === "full-after-shell"branch claims. The inline comment covers the intent, so this is purely a coverage gap, not a correctness one.
Checks run locally at this head
app-visited-response-cache23/23,prefetch-cache75/75,link-navigation77/77,app-browser-entry234/234 — all pass.vp checkon all 5 changed source files: format, lint, and types clean.
No blocking issues.
|
Review posted to PR #2858. SummaryReviewed at exact head Key finding — the fix is structurally correct. The rework (
Verified good:
Non-blocking: (1) a claim made at the exact deadline seeds an immediately-dead entry (harmless), and (2) the Local checks: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): reuse visited responses for full prefetch — exact head 83ec07d
Re-reviewed at the current head (one test-only commit — 83ec07d "measure stale time from latest visit" — past the last /bigbonk round at d57fade). All prior findings remain resolved and I found no new correctness issues. LGTM.
The new commit is a legitimate test correction
83ec07d only touches the first advanceTime in client-cache.spec.ts (270_001 → 5 * 60 * 1_000 + 1). This is correct: storeVisitedResponseSnapshot (app-browser-entry.ts:776-787) deletes and re-creates the visited entry with createdAt: now on every visit, and the claim measures its static window from match.entry.createdAt + staleTimeMs (app-visited-response-cache.ts:189). Because the test performs an intervening navigation at +30s that refreshes createdAt, the deadline must be measured from that latest visit, not the original — matching Next's navigatedAt semantics. The +1 correctly crosses the boundary given the claim window is inclusive at the deadline (now > createdAt + staleTimeMs, app-visited-response-cache.ts:179).
Single-sink callback ownership holds — verified end to end
The core fix (a claim seeds a real PrefetchCacheEntry rather than parking the callback in a side structure) remains the reason every onInvalidate path is exactly-once:
- Seed timer wiring:
seedPrefetchResponseSnapshotcallsschedulePrefetchInvalidationwhileonInvalidateCallbacksis stillundefined(line 1021 early-returns → no orphan timer).router.prefetchthen attaches viaattachPrefetchInvalidationCallback(navigation.ts:2804), and becauseentry.outcome === "cache-seeded"the timer is scheduled there (attachPrefetchInvalidationToEntry, line 1053-1055). - Repeat claim (same route): repeated
kind:"full"prefetches deleteNEXT_ROUTER_PREFETCH_HEADER(line 2755) → deterministicrscUrl/cacheKey, so the second call short-circuits athasPrefetchCacheEntryForNavigation(line 2786) and re-attaches to the seeded entry —claimruns once (pinned by theprefetch-cache.test.tsrepeated-claim test). - Claim-then-real-fetch: a stale seeded entry is deleted with
notify:falsebyprefetchRscResponse(line 1370) before the fetch entry is created — no residual callback. If the seeded entry ages out first,hasPrefetchCacheEntryForNavigationdeletes it withnotify:true(line 673) which clears its timer and nulls callbacks (deletePrefetchCacheEntry→notifyPrefetchInvalidated), so a following real fetch's ownonInvalidatecannot double-fire. Both sinks can never simultaneously hold the same callback for one route.
Verified good (unchanged and re-confirmed at this head)
- Newest-wins alias scan (
app-visited-response-cache.ts:169-182): compatibility filter runs before thecreatedAtcomparison; older/expired history entries are preserved (MRU re-insert only, no deletes). Math.max/deadline independence: the claim returnscreatedAt + staleTimeMsand never mutatesentry.expiresAt(every claim test assertsentry.expiresAtunchanged), so a longer dynamic navigation deadline is never shortened and BFCache traversal reads the untouched entry.- 30s static floor via
resolveFullPrefetchStaleTimeMs = Math.max(staleTimeMs, 30_000)matches Next'sgetStaleTimeMs; the shared helper also backs the tworesolvePrefetchedRscResponseExpiresAtfloors. Covered by theit.each([0, 10_000, 29_999])test. - Slot-compatibility is re-validated at consumption, not just at claim. The claim gate admits a decoded entry regardless of
mountedSlotsHeader(isVisitedResponseCacheEntryCompatibleForFullPrefetch), but the seeded entry is re-checked at consumption viaisPrefetchCacheEntryCompatibleWithMountedSlots(navigation.ts:1550) against both the pinned request-time slots and the snapshot's server-declared slots. A navigation whose slots match neither rejects the seed (line 1553) and falls through to a fresh fetch — so a tree decoded under a different slot context is never rendered incorrectly. - Client-only runtime seam (
navigation-runtime.ts:57, validated viaisOptionalRuntimeFunction;app-browser-entry.ts:2392) — no prod-server / worker-entry parity concern. - Link
full-after-shellclaim (link.tsx:646-661) is commented, only reachable viaunstable_dynamicOnHover, has noonInvalidate, doesn't add toprefetched(the seed does), and is idempotent on re-hover.
Non-blocking (carried over)
- Claim-at-exact-deadline seeds an immediately-dead entry (
expiresAt === now), rejected on the next read by the strict> Date.now()freshness check. Harmless (one wasted seed + a real fetch on the following navigation). full-after-shellstill has no dedicated unit test. The inline comment covers intent, so this is a coverage gap, not correctness.
Checks run locally at this head
app-visited-response-cache23/23,prefetch-cache75/75,link-navigation77/77 — all pass.
No blocking issues.
|
Review posted to PR #2858. SummaryReviewed at exact head The new commit ( Single-sink callback ownership verified end-to-end. The claim seeds a real
Extra verification beyond prior rounds: confirmed slot-compatibility is re-validated at consumption ( Non-blocking (carried over): claim-at-exact-deadline seeds an immediately-dead entry (harmless); Local checks: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): reuse visited responses for full prefetch — exact head 5f258b4
Re-reviewed at the current head (one test-only commit, 5f258b4 "cover visited full-prefetch promotion", past the last /bigbonk round at 83ec07d). All prior findings remain resolved and I found no new correctness issues. LGTM.
The new commit is a legitimate coverage + comment improvement
5f258b4 refines the link.tsx:646-649 comment (now explaining that unstable_dynamicOnHover starts as an automatic shell and upgrades to Full on hover, which is why the upgraded task may claim a visited response) and adds a link-navigation.test.ts case: reuses a visited response when an automatic prefetch is promoted to Full on hover. The test is well-constructed:
- Viewport intersection first performs only the automatic loading-shell request and asserts
claimVisitedResponseForFullPrefetchis not called. - Hover upgrades the task to Full (
instance.mode = "full-after-shell") and asserts exactly one claim + one fetch, that the seeded entry'ssnapshot.expiresAt === now + 30_000(the 30s static floor from the latest visit), and thatvisitedEntry.expiresAtstays atnow— pinning the invariant that Full-prefetch ownership never mutates the visited entry's ordinary-navigation deadline. This closes thefull-after-shellcoverage gap called out in the prior round.
Single-sink callback ownership verified end to end
The core fix — a claim seeds a real PrefetchCacheEntry (app-browser-entry.ts:2392→seedPrefetchResponseSnapshot) rather than parking the callback in a side structure — remains the reason every onInvalidate path is exactly-once. I re-traced all three paths at this head:
- Seed timer wiring:
seedPrefetchResponseSnapshotcallsschedulePrefetchInvalidationwhileonInvalidateCallbacksis stillundefined, so it no-ops (navigation.ts:1021, no orphan timer).router.prefetchthen attaches viaattachPrefetchInvalidationCallback(navigation.ts:2804), and becauseentry.outcome === "cache-seeded"the timer is scheduled exactly once there (attachPrefetchInvalidationToEntry,navigation.ts:1053-1055), withclearPrefetchInvalidationguarding against a double timer. - Repeat claim: the seeded entry is
cacheForNavigation: true, so the secondrouter.prefetchshort-circuits athasPrefetchCacheEntryForNavigation(navigation.ts:2786) and re-attaches to the existing entry —claimruns once. - Claim-then-real-fetch:
seedPrefetchResponseSnapshotdeletes any stale existing entry withnotify: false(navigation.ts:1102) andprefetchRscResponsedoes the same before creating a fetch entry, so no double-fire.
Verified good (re-confirmed at this head)
- Cache-key alignment: the seed publishes under
encodeCacheKey(rscUrl, interceptionContext)(the caller'srscUrl), androuter.prefetchattaches the callback to the identicalcacheKey(navigation.ts:2776), even when the claim matched an older_rscalias — a single owner. expiresAtverbatim: the snapshot carriesexpiresAt: claim.expiresAt(= entry.createdAt + staleTimeMs);resolveCachedRscResponseExpiresAtshort-circuits onisCacheExpiresAt(navigation.ts:484-485), so the seeded deadline is measured from the visit time, not the seed time — matching Next'snavigatedAt.- 30s static floor via
resolveFullPrefetchStaleTimeMs = Math.max(staleTimeMs, MIN_PREFETCH_STALE_TIME_MS)matches Next'sgetStaleTimeMs; the shared helper also de-dups the tworesolvePrefetchedRscResponseExpiresAtfloors. - Newest-wins alias scan (
app-visited-response-cache.ts:169-182): compatibility filter runs before thecreatedAtcomparison; the claim never mutatesmatch.entry(only MRU re-insert), so a longer dynamic navigation deadline is never shortened and BFCache traversal reads the untouched entry — the inclusive boundary (now > createdAt + staleTimeMs) is safe here. - Slot compatibility re-validated at consumption (
isPrefetchCacheEntryCompatibleWithMountedSlots,navigation.ts:1550), so a decoded tree from a different slot context is rejected and falls through to a fresh fetch. - Link
mode !== "auto"gate is not over-broad. Although the union includes"disabled", aprefetch={false}App Router link cannot reach the claim: viewport is gated bycanLinkPrefetch(prefetch !== false) and hover bycanLinkIntentPrefetch(App Router requiresprefetch !== false; the Pages Router allowance is moot because the claim lives inside thehasAppNavigationRuntime()block). So the gate is effectively only"full"/"full-after-shell", matching the comment. - Client-only runtime seam (
navigation-runtime.ts:57, validated viaisOptionalRuntimeFunction) — no prod-server / worker-entry parity concern.
Non-blocking (carried over)
- Claim-at-exact-deadline seeds an immediately-dead entry (
expiresAt === now), rejected on the next read by the strict> Date.now()freshness check. Harmless (one wasted seed + a real fetch on the following navigation).
Checks run locally at this head
app-visited-response-cache23/23,prefetch-cache75/75,link-navigation78/78,app-browser-entry234/234 — all pass.vp checkon all 5 changed source files: format, lint, and types clean.
No blocking issues.
|
Review posted to PR #2858. SummaryReviewed at exact head The new commit ( Verified this round:
Non-blocking (carried over): claim-at-exact-deadline seeds a harmless immediately-dead entry. Local checks: |
1 participant
Summary
Next.js targeted E2E
This maps both failures from Actions run 31290819291 in the force-stale suite: pending Full-prefetch reuse during navigation and BFCache reuse during a Full prefetch.
Additional validation
The targeted Next.js E2E is the primary acceptance check.