Skip to content

fix(app-router): honor cacheLife stale on the client router - #25

Closed
NathanDrake2406 wants to merge 15 commits into
mainfrom
fix/client-stale-time-from-cache-life
Closed

NathanDrake2406 wants to merge 15 commits into
mainfrom
fix/client-stale-time-from-cache-life

Conversation

@NathanDrake2406

@NathanDrake2406 NathanDrake2406 commented Jul 25, 2026 •

Copy link
Copy Markdown
Owner

Overview

Goal Make the client router honor cacheLife.stale, the client-reuse dimension of a cache profile that vinext computed and then discarded
Core change One projection, resolveClientStaleTimeSeconds, carries the completed render's minimum stale onto every artifact that outlives the render: the ISR entry, the prerender seed, the initial-HTML done-script, and the regenerated entry. Cache hits replay it as x-nextjs-stale-time; both client caches bound reuse by it
Key boundary stale is a property of a completed render (aggregation is minimum-wins, and use cache scopes keep registering while the stream is consumed), so it travels only on completion-time channels, never in streaming headers
Expected impact A subtree declaring cacheLife("seconds") (stale: 30) is reused by the browser for at most 30s instead of the 5-minute client cache TTL

Why

cacheLife profiles carry three independent numbers. revalidate and expire govern server and shared caches; stale is the contract with the client router: how long the browser may reuse route output without asking the server. vinext min-reduces stale across all use cache scopes of a request and then projected it out of every consumer, so the only freshness signal a browser ever saw was the build-time experimental.staleTimes constant. Request-scoped declarations had no observable effect on navigation freshness.

The constraint that shapes the design: a streaming response's headers cannot carry its own resolved cacheLife. Headers commit when the Flight stream is created; child Server Components register use cache scopes while the stream is consumed. Because aggregation is minimum-wins, a value read at header time is not approximately right, it is unbounded too large. The value is therefore read only at completion time and attached to durable artifacts:

Area Principle / invariant What this PR changes
ISR write (app-page-cache-finalizer.ts) The cache-write closure runs after the captured stream drains, so its read is the completed minimum CacheControlMetadata gains stale?; the write policy persists it via the new AppPageCacheSetter policy object (isrSetAppPage)
Cache-hit replay (app-page-cache.ts) Replay must claim exactly what the producing render claimed, and Next.js replays the stored header verbatim on every hit HIT and STALE responses emit x-nextjs-stale-time from entry metadata, unaged and unclamped; expire stays a serve-side ceiling (expired entries are blocking misses)
Background regeneration (app-page-cache-render.ts) A regen must not silently widen client reuse The real regen producer carries stale on its cacheControl into the refreshed entry
Prerender seed (build/prerender.ts, seed-cache.ts) Seeded entries must make the same claim a runtime render would persist VINEXT_PRERENDER_CACHE_LIFE_HEADER carries stale into the route results; PrerenderRouteResult declares it and writePrerenderIndex copies it into vinext-prerender.json, where seeding reads it
Cloudflare KV backend (kv-data-adapter.runtime.ts, prerender-kv-populate.ts) The primary deployment target must round-trip the same metadata the in-memory backend does set() persists cacheControl.stale (validated on read); deploy-time KV population carries route.stale into bulk-import entries
Initial HTML (app-ssr-stream.ts, app-page-render.ts) The done-script is emitted by finalize() after the full RSC drain, so it is a completion-time channel The metadata getter peeks the request-scoped cacheLife and ships staleTimeSeconds; hydration seeds the visited-response entry from it, identically in dev and prod
Data-cache propagation (shims/cache-runtime.ts) Advertised freshness must not depend on data-cache temperature The use cache entry write persists stale; a warm hit re-registers it on the request scope and, when nested inside another use cache, pushes it into the enclosing scope's lifeConfigs exactly like the MISS path — so the outer entry keeps the child's claim once both go warm
Cold RSC fetch (app-page-render.ts, shims/navigation.ts) An unresolved claim, once floored, can never license less than 30s Cold cacheable responses stream with X-Vinext-Stale-Time-Pending: 1; both client caches cap such a response at the 30s floor instead of their fallback TTLs

Two normalization rules, documented at the resolver: an absent stale is never synthesized from revalidate or expire (the default profile would license ~136 years of reuse), and no other field constrains it (seconds is { stale: 30, revalidate: 1, expire: 60 } on purpose; Next.js floors the replayed value at 30s client-side, so client reuse may overshoot expire by design).

What changed

Scenario Before After
Warm ISR hit, HTML or RSC Configured staleTimes / 5-minute fallback Entry's own stale replayed verbatim on both artifacts; Cache-Control unchanged
Initial HTML view, dev and prod, cached or not Config signal only Done-script carries the completed render's stale; a dynamic render carries both signals and the client takes the minimum
Prerendered route Fallback Seeded entries replay the prerender's resolved stale
Entry refreshed by background regeneration n/a The regenerating render's claim is preserved on the refreshed entry
Cold cacheable RSC fetch navigation 5-minute fallback Capped at the 30s floor via the pending marker until the entry is warm
Cold RSC fetch that completes dynamic mid-stream 5-minute fallback Capped at 30s; the ISR write is skipped, so the next request re-renders. Known limit: a cold stream cannot know its outcome, so staleTimes.dynamic below 30s is not enforced on this one response (see risk)
cacheLife({ stale: 5 }) Fallback TTLs Held 30s in both client caches (one floor rule, mirroring Next.js getStaleTimeMs), whether the route arrived via prefetch or cold navigation
Warm hit on the Cloudflare KV backend stale dropped at serialization; fallback cacheControl.stale round-trips through KV and deploy-time bulk import
Nested use cache, inner entry warm Outer entry stored with its default lifetime; inner claim lost once outer goes warm Inner HIT constrains the outer entry's stored lifetime like a MISS does
Streamed responses, shared caches No stale anywhere Unchanged: stale never enters Cache-Control, and cold responses stay non-blocking (cloudflare#961)
Maintainer review path
  1. packages/vinext/src/utils/cache-control-metadata.ts: the shared projection and its two normalization rules; every emitter goes through it.
  2. packages/vinext/src/server/app-page-cache-finalizer.ts: why the write-time read is sound (post-drain) and how stale enters the entry.
  3. packages/vinext/src/server/app-page-cache.ts: verbatim replay on HIT/STALE and the regeneration policy.
  4. packages/vinext/src/server/app-page-render.ts: the RSC branch (pending marker paired with the dynamic bound) and the done-script metadata getter; the negative guard test explains why streaming headers can never carry the value.
  5. packages/vinext/src/shims/navigation.ts: the client resolver (min of both lattices, uniform 30s floor, pending handling) and snapshot/replay round-trip.
  6. packages/vinext/src/server/app-browser-entry.ts: hydration seeding and the visited-response store.
  7. packages/vinext/src/shims/cache-runtime.ts, build/prerender.ts, server/seed-cache.ts: the propagation links that would otherwise silently drop the value.
Validation
  • Write path: { stale: 30, revalidate: 1, expire: 60 } persists 30 (revalidate does not clamp); { stale: 300, expire: 45 } persists 300 (expire never clamps); a streaming render's headers advertise nothing even with a page-level claim (regression guard for the header-time approach).
  • Replay: verbatim on HIT and STALE, no aging; default-shaped entries emit no header.
  • Regeneration: the real producer carries a mid-render claim onto its cacheControl (tests/app-page-cache-render.test.ts; fails without the producer fix).
  • Cold path: the non-blocking streaming test pins the pending marker; the late-dynamic scenario (capture-eligible, dynamic usage mid-stream, ISR write skipped) pins the 30s cap and the skipped write; the segment-cache-client-params compat E2E pins full prefetch reuse of statically-prefetchable dynamic-param routes.
  • Client: min-combination, uniform floor, pending cap, and header round-trips in both cache suites.
  • KV: cacheControl.stale round-trip through KVCacheHandler and deploy-time bulk-import entries; writePrerenderIndex regression test pins stale in vinext-prerender.json; the nested-HIT test fails without the lifeConfigs propagation fix (verified red).
  • Ran vp check and the affected suites locally; CI runs the full suite and all E2E projects.
Risk / compatibility
  • Persisted format: CacheControlMetadata.stale is optional; entries written before this change simply emit no header and clients keep their configured fallback.
  • Wire: two additive response headers (x-nextjs-stale-time, matching Next.js, and the vinext-internal X-Vinext-Stale-Time-Pending).
  • Known limit: with an edge CDN adapter whose set() is a no-op, the streamed response is the cached artifact, so edge HITs replay the pending marker and RSC navigations stay capped at the conservative 30s floor instead of a wider completed claim. Attaching the completed claim would require blocking the stream (rejected in fix(isr): honor route expire ceilings cloudflare/vinext#961); dropping the marker would widen reuse beyond the declaration. The initial-HTML path is unaffected — the done-script travels in the body.
  • Known limit: a render that turns dynamic only during streaming still carries the pending marker, so its epoch-cold response is reused up to 30s even when staleTimes.dynamic is lower. Bounding every pending response by staleTimes.dynamic instead was tried and reverted: it destroys prefetch reuse of statically-prefetchable dynamic-param routes (the segment-cache-client-params compat test), because a cold stream cannot distinguish the two outcomes. Eliminating the window entirely requires blocking cold renders, rejected in fix(isr): honor route expire ceilings cloudflare/vinext#961.
  • Deliberate divergences from Next.js, each documented in code: the client combines the config and cacheLife signals by minimum instead of header-wins (vinext keeps the config signal as a separate carrier), and the done-script advertises the claim in dev where Next.js emits nothing.
  • Shared caches: stale never enters Cache-Control; expire handling is unchanged.
Non-goals
  • Blocking cold cacheable renders to emit the resolved value in headers: Next.js does this for ISR misses, but fix(isr): honor route expire ceilings cloudflare/vinext#961 deliberately keeps vinext's cold responses streaming with a background waitUntil write. The pending marker bounds the residual window instead.
  • An in-band trailing transport (Next.js's StaleTimeIterable payload field): it exists only in cacheComponents staged renders, where cache scopes settle before the render task queue drains. In vinext's lazy streaming model the iterable could never be closed (the stream cannot end while it is open, and completion is observable only via stream end).
  • Tag-based invalidation, SWR serve-and-refresh, and back/forward traversal freshness, which keeps MAX_TRAVERSAL_CACHE_TTL bfcache semantics.

References

Reference Why it matters
Next.js app-render.tsx (applyMetadataFromPrerenderResult) The header is emitted only from fully awaited static-generation renders; streamed dynamic renders carry nothing
Next.js app-page-runtime.ts cached-header replay Stored headers, including the stale time, are re-emitted verbatim on every hit; the basis for vinext's unaged replay
Next.js server/app-render/stale-time.ts The StaleTimeIterable transport this PR deliberately does not port (see non-goals)
cloudflare#961 Established that cold ISR responses stream and must not block on cache metadata

No changeset: this repo generates them from merged SHAs via scripts/create-changeset.mts.

`cacheLife` profiles carry three independent numbers, and `stale` is the
client-router dimension: how long the browser may reuse cached route output
without asking the server. vinext aggregated it correctly (`resolveCacheLife`
min-reduces all three; the request scope accumulates min-wins) and then
projected it away at every consumer, so the only staleness a browser ever saw
was `dynamicStaleTimeSeconds` from `experimental.staleTimes` — a build-time
constant unrelated to the cached subtrees that produced the render. A subtree
declaring `cacheLife("seconds")` (stale: 30) was held for the full 5-minute
visited-response TTL.

Carry the resolved `stale` to the client on `x-nextjs-stale-time` (matching
Next.js's `NEXT_ROUTER_STALE_TIME_HEADER`) and combine it with the config
value by taking the minimum, so neither min-wins lattice overrides the other.
The 30s prefetch floor applies to the new value too.

Two normalization rules, both deliberate:

- An absent `stale` is never synthesized from `revalidate`/`expire`. The
  `default` profile has no `stale` and an `expire` of ~136 years, so deriving
  one would license session-long reuse without a refresh.
- The three numbers are not assumed to be ordered — `seconds` is
  `{ stale: 30, revalidate: 1, expire: 60 }` — so `revalidate` never
  constrains `stale`. Only the hard `expire` ceiling does.
The previous commit derived `x-nextjs-stale-time` from `peekRequestCacheLife()`
at response-construction time. That read happens after `probeAppPageBeforeRender`
but before the RSC stream is consumed, and the probe only awaits the page
component's own async result — it does not render the returned tree. Any
`use cache` scope in a child Server Component registers later, during stream
consumption, so the header described the probe rather than the output.

Because `cacheLife` aggregation is minimum-wins, missing one late scope is
enough to make the value wrong, and "it can only shorten" did not hold: with no
`dynamicStaleTimeSeconds` present (the usual case for `use cache` pages, which
are not dynamic renders), a peeked `stale: 300` widened the prefetch window from
`PREFETCH_CACHE_TTL` (30s) to 300s — 10x longer than today, in the direction the
change set out to fix.

Carry the value on the cache entry instead. The ISR write path reads the
request-scoped accumulation via the consuming `getRequestCacheLife()` inside the
cache-write closure, which runs after the captured stream has drained, so it
observes the completed render's minimum. `CacheControlMetadata` gains `stale`,
`isrSet` persists it, and `buildAppPageCachedResponse` re-advertises it on hits.
Prerender seeds carry it through `VINEXT_PRERENDER_CACHE_LIFE_HEADER` and the
prerender manifest so a seeded entry makes the same claim a runtime render would.

Two links that would otherwise silently drop the claim are closed with it: the
`use cache` entry write now persists `stale`, and `recordRequestScopedCacheControl`
re-registers it on a data-cache hit — without both, a page's advertised freshness
would depend on data-cache temperature rather than on what it declared.

Streaming responses now advertise nothing and leave the client on its configured
`experimental.staleTimes`, which is the honest answer for a value that is not yet
known when headers are committed. Covering fresh streaming renders needs a
render-completion signal the streaming RSC path does not have today; Next.js
solves it by streaming an `AsyncIterable<number>` in the RSC payload
(`app-render.tsx` `baseResponse.s`, closed after the render settles), which is
the natural follow-up.

The client-side combination logic is unchanged: both stale signals are still
min-reduced, absent `stale` is never synthesized from `revalidate`/`expire`,
`revalidate` never clamps `stale`, and `expire` still caps it.
@NathanDrake2406

Copy link
Copy Markdown
Owner Author

Design review

Read the diff, the surrounding server/client cache paths, and the Next.js reference in .nextjs-ref.

Verdict: the mechanism is right; the delivery seam is narrower than it needs to be.

The core judgement — that stale must travel on the cache entry rather than be read at header-commit time — is correct and the constraint analysis holds up against the code. resolveAppPageCacheWritePolicy really is called after readStreamAsText(streamForCache) (app-page-cache-finalizer.ts:147-161) and after await capturedRscDataPromise (:266-277), so the consuming _consumeRequestScopedCacheLife() read there is the completed render's minimum. And the reason "just peek at header time" isn't a smaller version of this change is worth restating: cacheLife aggregation is min-wins (cache-request-state.ts:199-224), so a value read early isn't approximately right, it's unbounded too large — the scope that would have lowered it hasn't registered yet. Rejecting the first commit's approach was correct.

What I'd push back on is coverage, and one seam the description says doesn't exist.


1. The trailing metadata frame already exists (for HTML)

The description says closing the fresh-render gap "needs a settle signal — or a trailing metadata frame — before it can be built here," and separately lists getInitialNavigationCacheMetadata as out of scope. Those are in tension, because that is the frame:

// app-ssr-stream.ts:147-153
async finalize(): Promise<string> {
  await pumpPromise;                                    // full RSC stream drained
  let scripts = this.flush();
  scripts += createInlineScriptTag(
    createNavigationRuntimeRscDoneScript(getInitialNavigationCacheMetadata?.()),

finalize() awaits pumpPromise — every RSC byte read — and then calls the metadata getter, which already ships dynamicStaleTimeSeconds to the client in an inline done-script (app-ssr-stream.ts:24-27, :79-84). That is a render-completion-time channel already carrying the sibling value of exactly this quantity.

Wiring peekRequestCacheLife() into that getter would close the "first visitor gets configured staleTimes" gap for every initial page view — dev and prod, cached or not, no new header, no dependency on the ISR entry being warm. RSC fetch navigations genuinely need the AsyncIterable follow-up (the payload is encoded up front by AppElementsWire.encodeOutgoingPayload), but HTML does not, and initial loads are the common first contact with any route.

The target shape I'd argue for: one resolver (resolveClientStaleTimeSeconds, already built) feeding three emitters — initial-HTML done-script, cache-entry replay, prerender seed. This PR is two of three with the same resolver; the third is small and removes most of the asymmetry.

2. Client freshness is now coupled to server cacheability

These are independent concerns and the chosen carrier fuses them. resolveAppPageCacheWritePolicy returns null when revalidateSeconds <= 0 (:115-117), and the write is skipped entirely on dynamic usage (:149-155). So a page that reads cookies at the page level but wraps its data in use cache subtrees declares cacheLife and never advertises it — no entry is ever written. Next.js emits the header from every render regardless of cacheability (app-render.tsx:3183-3186, unconditional on collectedStale).

Compounding it: shouldCaptureRscForCacheMetadata requires options.isProduction || options.isPrerender (app-page-render.ts:767-773), so in dev no entry is written and the header is never emitted at all. Given the dev/prod parity rule in AGENTS.md that deserves an explicit line in the description rather than being implicit — cacheLife("seconds") behaving differently under dev vs prod is the kind of thing that comes back as a bug report. Fix #1 collapses most of this for the HTML path.

3. The expire clamp doesn't enforce the invariant its comment claims

resolveClientStaleTimeSeconds clamps stale <= expire, justified as "reuse past expire would hand back output the server already considers gone." But the serve path replays the full window from the moment of the hit, ignoring entry age. Entry written at t=0 with { stale: 30, expire: 60 }, hit at t=59 → the client is told to reuse for 30s, holding to t=89, 29s past expire. The clamp bounds the value, not the elapsed window, so it doesn't deliver the stated guarantee.

The data is already on the entry: CacheHandlerValue.lastModified (cache-handler.ts:8), and the caller already holds cached.value (app-page-cache.ts:502). Math.min(stale, expire - age) at serve time is available today. Either enforce it, or soften the comment — as written the code documents a guarantee it doesn't provide, which is worse than an acknowledged gap.

4. The min-combination rule is currently unreachable

resolveRscResponseStaleTimeSeconds takes Math.min(dynamicStaleTimeSeconds, staleTimeSeconds), documented as a core semantic with two tests exercising both-present. But no vinext response can carry both:

  • shouldEmitDynamicStaleTime requires dynamicUsedDuringBuild || isForceDynamic || !shouldCaptureRscForCacheMetadata (app-page-render.ts:836-840) — a dynamic render.
  • The new header is set only by buildAppPageCachedResponse and prerender seeds, and buildAppPageCachedHeaders never sets the dynamic one.

All four quadrants (dev/prod × cached/dynamic) yield at most one header. It also diverges from Next.js, where the header wins outright and staleTimes is only the fallback (segment-cache/cache.ts:3428-3444: header → getStaleTimeMs, else STATIC_STALETIME_MS).

Two coherent resolutions: make it reachable on purpose (emit the configured value on cached responses too, so the min genuinely arbitrates), or drop to the Next.js rule and delete the combination plus its tests. Right now it reads as load-bearing semantics for a state the system can't produce.

5. Two client caches, one signal, two floor rules

Prefetch entries floor at MIN_PREFETCH_STALE_TIME_MS (30s, navigation.ts:419-438); visited-response entries honor the value verbatim. Next.js applies getStaleTimeMs = Math.max(stale, 30) * 1000 uniformly across the segment cache (cache.ts:122-124).

Observable consequence: with cacheLife({ stale: 5 }), a route prefetched by a <Link> yields a visited entry that inherits the prefetch snapshot's floored expiresAt (per the existing "inherits the expiry carried by a consumed prefetch snapshot" test, plus the isCacheExpiresAt early return), so it's held 30s. The same route navigated to cold is held 5s. Same declaration, two behaviors, keyed on whether a prefetch happened to fire. Worth one rule rather than two.

6. Plumbing shape — cheap to fix here, expensive later

AppPageCacheSetter is now six positional params ending in two optional numbers, and it's declared four times (app-page-cache-finalizer.ts:15, app-page-cache.ts:27, app-page-dispatch.ts:108, app-page-render.ts:92). One field cost four identical type edits plus undefined, undefined padding through app-page-render.test.ts. Meanwhile isrSet immediately re-assembles that positional tail into the CacheControlMetadata object it was exploded from (isr-cache.ts:185-197).

The shape that wants to exist: isrSet(key, data, { revalidate, expire, stale, tags }) with one exported setter type. Every call site is already in this diff, so the collapse is close to free now.

Separately, this adds a fifth structurally-identical cache-life shape (PrerenderCacheLife in build/prerender.ts) alongside CacheLifeConfig (cache-request-state.ts:9-13, which already has stale), two AppPageRequestCacheLife, and AppPagePrerenderCacheLife. Not debt this PR created, but stale is the third dimension to traverse the chain and each one pays the same tax. Fine to defer — worth naming.


What's clearly right

  • resolveClientStaleTimeSeconds shared across write / serve / seed is the best structural decision here. Those three paths disagreeing silently is exactly the failure this class of change produces; there's now one place to be wrong.
  • Refusing to synthesize stale from revalidate/expire, and refusing to let revalidate clamp it. The profile table proves the point — seconds: { stale: 30, revalidate: 1, expire: 60 } (cache-request-state.ts:17). A reviewer's instinct is "surely stale <= revalidate", and it's wrong; the comment preempts that correctly.
  • Closing the use cache write and recordRequestScopedCacheControl links. Without both, advertised freshness would vary with data-cache temperature — cold and warm renders of identical output making different claims. That's the leak that survives review and shows up in production.
  • Keeping stale out of Cache-Control (applyRequestCacheLife) — right boundary, well documented.
  • The streaming regression guard is the right test to have written, and right to have written first.

The theme

Points 2–4 are the same issue viewed from different angles: where a value's authority lives. stale is a property of a completed render. This gives it a home on the ISR cache entry — a property of a cacheable completed render. Everywhere those sets differ (dynamic pages, dev mode, first visit) the value goes missing; where the entry outlives the render's assumptions (age vs expire) it drifts the other way. That mismatch between a carrier's lifetime and a value's meaning is the thing to watch, more than any individual gap it produces.

Suggested landing sequence

  1. Land the cache-entry carrier as-is — necessary regardless, replayed bytes must not lie about their freshness.
  2. Add the getInitialNavigationCacheMetadata emitter here or immediately after. Same resolver, existing channel; converts the headline gap from "every cold visit" to "RSC fetches on cold routes".
  3. Resolve ci(bonk): switch bonk + bigbonk to GPT-5.5 xhigh #4 before merge — unreachable documented semantics are cheap to remove now.
  4. Pick one floor rule for the two client caches (refactor: introduce createAppRscHandler #5).
  5. Either enforce the age-aware expire clamp at serve, or soften the comment (chore: upgrade claude-opus-4-6 to claude-opus-4-7 #3).

The previous round persisted the completed render's cacheLife stale onto
the ISR entry and replayed it on cache hits, which left the value missing
or drifting wherever the entry's lifetime diverged from the render's:

- Fresh streaming renders advertised nothing, so the first visitor of any
  route (and every dev render, since dev writes no ISR entry) stayed on
  the configured staleTimes despite a declared cacheLife. The done-script
  emitted by the RSC embed transform's finalize() runs only after the
  full RSC stream has drained, so it can carry the completed render's
  minimum where streaming headers cannot. Emit the peeked request-scoped
  cacheLife there and seed the hydration visited-response entry from it.
  This also makes the client's min-combination of the config and
  cacheLife signals reachable: a dynamic render with use cache subtrees
  now legitimately carries both.

- The expire clamp bounded the value but not the elapsed window: an
  entry of { stale: 30, expire: 60 } hit at age 59s replayed a 30s reuse
  window reaching 29s past expire. Age the serve-time clamp with the
  entry's lastModified so only the remaining window is advertised.

- The two client caches applied different floors: prefetch entries
  floored a cacheLife stale at 30s while cold navigations honored it
  verbatim, so the same declaration produced two behaviors keyed on
  whether a prefetch fired first. Floor the cacheLife signal once in the
  shared resolver, mirroring Next.js getStaleTimeMs, before the min so
  it can never raise the config-derived bound.

Mechanically, the app-page cache setter's six-position signature
(declared identically in four modules) collapses into one exported
AppPageCacheSetter taking an AppPageCacheWritePolicy object, with
isrSetAppPage adapting to the shared positional isrSet, which stays
unchanged for the Pages Router and route handlers.
@NathanDrake2406

Copy link
Copy Markdown
Owner Author

Addressed in f66544e, following the suggested landing sequence. Point by point:

1. Done-script emitter — added. You were right that the frame already exists; the "needs a settle signal" claim in the description only held for RSC fetch navigations. getInitialNavigationCacheMetadata now peeks the request-scoped cacheLife (peek, not consume — the cache-write closure keeps the consuming read, and it runs after the body completes) and ships staleTimeSeconds in the done-script; hydration threads it into the visited-response snapshot. The resolver now feeds four emitters. Tests: the metadata getter contract in app-page-render.test.ts, the finalize-after-drain emission in app-ssr-stream.test.ts.

2. Dev/prod parity — collapsed for HTML by #1. Dev writes no ISR entry, but the done-script advertises the claim identically in dev and prod. The remaining asymmetry is RSC fetch navigations on cold routes, now stated explicitly in the description's Known gap with the AsyncIterable follow-up.

3. Age-aware expire clamp — enforced, not softened. resolveClientStaleTimeSeconds takes ageSeconds; serve paths pass the entry's lastModified, write/seed paths run at age 0. Your exact scenario is the regression test: { stale: 30, expire: 60 } hit at age 59s replays 1. (The client-side 30s floor means a near-expired entry is still held ~30s by the browser — that matches Next.js, where getStaleTimeMs floors every header value; the clamp keeps the header honest and bites for windows above the floor.)

4. Min-combination — made reachable on purpose. With #1, a dynamic initial HTML render whose use cache subtrees declared a cacheLife legitimately carries both signals in the done-script metadata; there is a test asserting exactly that state. Kept min rather than adopting header-wins: Next.js can use header-wins because selectStaleTime folds the config into the collected stale server-side; vinext carries the config signal separately, so min is the only combination that preserves both lattices' bounds. The resolver comment now names the reachable state.

5. One floor rule. The 30s floor moved into resolveRscResponseStaleTimeSeconds, applied to the cacheLife signal before the min, so cacheLife({ stale: 5 }) is held 30s in both caches regardless of whether a prefetch fired. Applying it before the min means it can never raise the config-derived bound (dynamic: 10, stale: 5 still yields 10). The prefetch-only minimum still covers the config/fallback dimensions, unchanged from pre-PR behavior.

6. Plumbing. AppPageCacheSetter is now one exported type taking an AppPageCacheWritePolicy object, with isrSetAppPage adapting to the shared isrSet. I stopped short of the full isrSet(key, data, {…}) signature change: the positional form is also the Pages Router and route-handler seam, which never carries stale, so changing it would push churn into paths outside this diff. The cache-life shape consolidation (five structurally identical types) is left as named debt.

vp check clean; the six directly affected suites plus seventeen adjacent cache/ISR/prerender/navigation suites pass locally (~2000 tests).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a client-router caching parity bug in vinext’s App Router by correctly carrying cacheLife().stale (the client reuse bound) from completed renders into both server replay channels (ISR cache entries / prerender seeds) and the initial-HTML “done-script” metadata, so the browser no longer reuses cached output longer than the render declared.

Changes:

  • Add a shared resolveClientStaleTimeSeconds() resolver and persist stale through cache metadata, prerender manifests, and ISR write/replay paths (including age-aware clamping against expire).
  • Plumb x-nextjs-stale-time (NEXT_ROUTER_STALE_TIME_HEADER) end-to-end (cache write, cache hit replay, RSC snapshot/restore, prerender seed, initial HTML bootstrap), and combine config vs cacheLife stale signals on the client via min.
  • Add/extend targeted tests across server cache write/serve paths, prerender seeding, stream finalization, and both client caches (visited + prefetch).

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/shims.test.ts Adds regression test ensuring warm use cache hits re-register stale onto request scope.
tests/seed-cache.test.ts Verifies prerender seeding carries staleSeconds into both HTML and RSC artifacts.
tests/prefetch-cache.test.ts Covers snapshot/replay of x-nextjs-stale-time, min-combination, and floor behavior in prefetch cache.
tests/app-visited-response-cache.test.ts Covers honoring server stale, flooring, min-combination, and fallback behavior for visited-response cache.
tests/app-ssr-stream.test.ts Ensures done-script can include staleTimeSeconds emitted after stream drain.
tests/app-page-render.test.ts Exercises persist/clamp behavior on ISR writes, ensures streaming headers don’t advertise stale, and validates done-script metadata.
tests/app-page-dispatch.test.ts Updates assertions for the new App Router ISR setter policy object signature.
tests/app-page-cache.test.ts Adds HIT/STALE replay tests for x-nextjs-stale-time, including age-aware clamping and default-profile behavior.
packages/vinext/src/utils/cache-control-metadata.ts Introduces resolveClientStaleTimeSeconds() normalization/clamp helper.
packages/vinext/src/shims/navigation.ts Stores/restores server staleTimeSeconds, adds unified stale resolution + flooring logic for client caches.
packages/vinext/src/shims/cache-runtime.ts Persists stale into cache metadata for use cache entries and re-registers it on hits.
packages/vinext/src/shims/cache-handler.ts Extends CacheControlMetadata with stale and round-trips it through MemoryCacheHandler.
packages/vinext/src/server/seed-cache.ts Threads prerender-manifest stale into seeded entry write metadata.
packages/vinext/src/server/prerender-manifest.ts Adds stale?: number to prerender manifest route schema.
packages/vinext/src/server/isr-cache.ts Adds stale support to isrSet, introduces AppPageCacheWritePolicy + isrSetAppPage.
packages/vinext/src/server/headers.ts Defines NEXT_ROUTER_STALE_TIME_HEADER constant (x-nextjs-stale-time).
packages/vinext/src/server/app-ssr-stream.ts Adds staleTimeSeconds to InitialNavigationCacheMetadata and done-script emission.
packages/vinext/src/server/app-page-response.ts Adds applyClientStaleTimeHeader() and includes stale in prerender cache-life header payload.
packages/vinext/src/server/app-page-render.ts Emits staleTimeSeconds via initial-HTML metadata getter at finalize-time.
packages/vinext/src/server/app-page-dispatch.ts Wires peekRequestCacheLife() through dispatch to support finalize-time metadata emission.
packages/vinext/src/server/app-page-cache.ts Replays x-nextjs-stale-time from cache entries (age-aware) and preserves stale through regen writes.
packages/vinext/src/server/app-page-cache-finalizer.ts Persists resolved staleSeconds onto ISR entries at cache-write time.
packages/vinext/src/server/app-browser-entry.ts Seeds initial visited-response cache entry with staleTimeSeconds from done-script metadata.
packages/vinext/src/entries/app-rsc-entry.ts Switches App Router ISR setter wiring to isrSetAppPage.
packages/vinext/src/client/navigation-runtime.ts Extends bootstrap payload validation to accept staleTimeSeconds.
packages/vinext/src/build/prerender.ts Extracts stale from prerendered responses and writes it into the prerender manifest.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +974 to +978
const requestCacheLife = options.peekRequestCacheLife?.();
const staleTimeSeconds = resolveClientStaleTimeSeconds({
expire: requestCacheLife?.expire,
stale: requestCacheLife?.stale,
});
Comment on lines +113 to +117
// Callers reach this only after the render's stream has been drained, so the
// request-scoped accumulation is the completed render's minimum rather than a
// partial one. That is what makes persisting it onto the entry sound.
// Clamp against the *effective* expire, not just the one `cacheLife` supplied,
// so a route-config expire also bounds client reuse.
The cold RSC fetch gap is a design decision, not a pending follow-up:
Next.js's AsyncIterable stale transport only works under staged rendering
(cacheComponents), where cache scopes settle before the render task queue
drains — in vinext's lazy streaming model the iterable could never close.
Next.js's plain-mode mechanism is a blocking cold render, which cloudflare#961
deliberately rejected to keep ISR page streams unblocked. Assert the
resulting no-header contract on the streaming-response test.

Also reword the write-policy expire comment: a cacheLife-declared expire
replaces the config expireTime fallback (Next.js precedence), it is not
min-merged with a route-level ceiling — no such ceiling exists outside
cacheLife.
Three fixes from review round 3:

Background regeneration dropped the regenerating render's cacheLife stale:
renderAppPageCacheArtifacts returned only { revalidate, expire }, so
resolveRegeneratedAppPageCachePolicy could never receive the stale it was
built to preserve and the first regen silently widened client reuse back
to the configured fallback. The producer now carries it, with a
real-producer regression test the mocked-cacheControl tests could not
provide.

The age-aware expire clamp is removed. Composed with the client's 30s
floor it delivered neither contract (a clamped 1 re-floored to 30), and
cached HTML replayed the unclamped done-script value regardless. Next.js
stores the stale header at generation and replays it verbatim on every
hit; vinext now does the same, keeping expire a serve-side ceiling.

Cold cacheable RSC responses stream before their cacheLife resolves
(cloudflare#961), which left them on the 300s client fallback — reproducing the
headline bug for the first request of every entry epoch. They now carry
X-Vinext-Stale-Time-Pending, and both client caches bound such responses
at the 30s floor: the unresolved claim, once floored, could never
license less.
The pending marker meant 'capture was attempted', but the client read it
as 'a cacheLife claim exists'. Capture eligibility is decided before the
lazy stream runs, so a late request-API read can make the completed
render dynamic — the finalizer skips the ISR write and no claim ever
exists, yet the marker granted 30 seconds of reuse even under
staleTimes.dynamic: 0.

Pending responses now carry the configured dynamic stale time (including
0) and the client takes the minimum, so an unresolved response never
receives a wider window than the dynamic bound.
NathanDrake2406 and others added 9 commits July 26, 2026 02:39
Pairing the pending marker with the configured dynamic stale time broke
the segment-cache-client-params compat test: dynamic-param routes
prefetch with no minimum TTL, so the paired 0 default made every cold
prefetch entry expire instantly and navigations refetched routes that
Next.js serves entirely from a static prefetch.

A cold stream cannot distinguish a render that will resolve static from
one that turns dynamic mid-stream, and bounding both by staleTimes.dynamic
sacrifices the guaranteed-correct case for the ambiguous one. Pending
responses go back to the 30s floor cap; the late-dynamic exposure
(one epoch-cold response, at most 30s) is documented as the price of
non-blocking cold renders (cloudflare#961).
…d nested cache hits

- writePrerenderIndex now copies `stale` into vinext-prerender.json so
  seedMemoryCacheFromPrerender actually receives it
- KVCacheHandler.set() and buildPrerenderKVPairs persist cacheControl.stale
  so warm hits on the Cloudflare KV backend replay the producing render's
  claim; validateCacheEntry accepts the field
- a nested use cache HIT pushes its stored lifetime into the enclosing
  cache context's lifeConfigs, mirroring the MISS path, so the outer entry
  keeps the child's stale claim once the outer goes warm
The added modules ride in every consumer build environment; the review-grade
rationale lives in the PR body, the code keeps one-line constraints.
CachedRscResponse carried staleTimePending and staleTimeSeconds as two
independent optionals, so the cached form admitted a state the wire never
produces (pending and resolved at once) and every consumer had to encode
the precedence rule. Replace both with a discriminated serverStaleTime
(pending | resolved), collapsed once at the header-parse boundary.
The generic setter had grown to six positional arguments, the last an
App-Router-only `stale` value, with isrSetAppPage as a policy-object
wrapper that unpacked straight back into it. Take { cacheControl, tags }
instead: routers construct the metadata they actually claim, the wrapper
and its type disappear, and the shared isrCacheControl builder replaces
the cache-control literal duplicated across writers.
The done-script metadata reused the RSC header's rule of dropping the
config-derived dynamic stale time while the speculative ISR capture was
armed. That rule only holds on the header path, which substitutes the
pending marker and its 30s floor; the done script emits the resolved
cacheLife instead, so a production render that turned dynamic shipped the
cacheLife claim as its only bound and let the hydration-seeded entry reuse
dynamic output for its full duration. Dev never took the capture path, so
the two diverged.

Also migrates the pages-basic seed fixture, the last positional isrSet
caller, which typecheck does not cover.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants