fix(app-router): honor cacheLife stale on the client router by NathanDrake2406 · Pull Request #2708 · cloudflare/vinext

NathanDrake2406 · 2026-07-25T16:37:53Z

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: durable cache metadata, the HTML done-script, or an internal trailer after the Flight payload
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	Completion metadata replaces the provisional pending bound with `staleTimes.dynamic`; the dynamic response is not published as a reusable static snapshot
`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 (#961)

Maintainer review path

packages/vinext/src/utils/cache-control-metadata.ts: the shared projection and its two normalization rules; every emitter goes through it.
packages/vinext/src/server/app-page-cache-finalizer.ts: why the write-time read is sound (post-drain) and how stale enters the entry.
packages/vinext/src/server/app-page-cache.ts: verbatim replay on HIT/STALE and the regeneration policy.
packages/vinext/src/server/app-page-render.ts: the RSC branch, completion metadata appended after Flight, and the done-script metadata getter; streaming headers remain provisional because they cannot carry the completed value.
packages/vinext/src/shims/navigation.ts: the client resolver (min of both lattices, uniform 30s floor, pending handling) and snapshot/replay round-trip.
packages/vinext/src/server/app-browser-entry.ts: hydration seeding and the visited-response store.
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 provisional pending marker; the late-dynamic scenario pins completion-time replacement with staleTimes.dynamic, skipped ISR publication, and no stale prefetch snapshot; 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: additive response headers (x-nextjs-stale-time, matching Next.js, plus vinext-internal pending/completion markers). When dynamic usage is discovered after headers commit, a bounded internal footer follows the Flight payload; vinext strips it before React consumes the stream and records the resolved dynamic bound in the client snapshot.
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 #961); dropping the marker would widen reuse beyond the declaration. The initial-HTML path is unaffected — the done-script travels in the body.
Late dynamic usage: headers still carry the conservative pending marker, but body completion now supplies the authoritative dynamic bound. This preserves prefetch reuse for statically-prefetchable dynamic-param routes while preventing a completed dynamic response from being seeded with the provisional 30s snapshot.
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 #961 deliberately keeps vinext's cold responses streaming with a background waitUntil write. The pending marker bounds the residual window instead.
Next.js's StaleTimeIterable payload-field transport: it depends on cacheComponents staged rendering. Vinext instead uses a small internal footer appended only after the lazy Flight stream completes, keeping the React payload and completion signal separately decodable.
Tag-based invalidation, SWR serve-and-refresh, and back/forward traversal freshness, which keeps MAX_TRAVERSAL_CACHE_TTL bfcache semantics.

Review hardening

The completion transport now carries both client-freshness lattices after the Flight stream drains: dynamicStaleTimeSeconds plus the completed cacheLife.stale result. A numeric cacheLife result replaces the provisional pending marker with a resolved minimum; explicit null proves that the completed dynamic render had no cacheLife claim; an absent field conservatively retains the 30s pending floor.

The footer uses byte-stuffed framing rather than a magic string: ordinary Flight 0xff bytes encode as 0xff 0xff, leaving 0xff 0x00 as an unambiguous footer tag. Buffered prefetch extraction and streaming HMR/React normalization share the same strict footer validator and fail closed on invalid escapes, truncated, oversized, malformed, or multiple frames. The streaming decoder holds at most one ambiguous escape byte, passes ordinary chunks through without copying, and no longer retains an arbitrary 256-byte Flight tail until close.

Review regressions cover every split of encoded framing, ordinary reserved bytes in buffered and streaming paths, short footerless payloads, malformed-frame rejection, cancellation/error propagation, dynamic=300 combined with completed cacheLife.stale=30, and the no-cacheLife dynamic 60s production navigation boundary.

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)
#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.

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.

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.

pkg-pr-new · 2026-07-25T16:38:53Z

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2708

npm i https://pkg.pr.new/create-vinext-app@2708

npm i https://pkg.pr.new/@vinext/types@2708

npm i https://pkg.pr.new/vinext@2708

commit: f67488f

github-actions · 2026-07-25T16:44:45Z

Performance benchmarks

Compared f67488f against base 66ce817 using alternating same-runner rounds. Next.js was unchanged and skipped.

1 improved · 0 regressed · 5 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	133.2 KB	134.4 KB	⚫ +0.9%
Client entry size (gzip)	vinext	120.8 KB	122.0 KB	⚫ +1.0%
Dev server cold start	vinext	3.00 s	2.92 s	🟢 -2.8%
Production build time	vinext	3.15 s	3.16 s	⚫ +0.4%
RSC entry closure size (gzip)	vinext	105.6 KB	106.3 KB	⚫ +0.7%
Server bundle size (gzip)	vinext	180.3 KB	181.9 KB	⚫ +0.9%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}

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).

NathanDrake2406 · 2026-07-25T16:50:20Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 437f640819

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…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.

NathanDrake2406 · 2026-07-25T17:27:52Z

@codex review

chatgpt-codex-connector · 2026-07-25T17:32:54Z

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 83296eb23f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

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.

NathanDrake2406 · 2026-07-27T06:30:56Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2363b944da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

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.

NathanDrake2406 · 2026-07-27T06:55:35Z

@codex review

chatgpt-codex-connector · 2026-07-27T07:04:08Z

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: a6181be93d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

james-elicx · 2026-07-27T09:05:58Z

/bigbonk review for issues

ask-bonk · 2026-07-27T09:06:57Z

UnknownError: ProviderInitError

github run

ask-bonk · 2026-07-27T14:45:20Z

UnknownError: ProviderInitError

github run

james-elicx · 2026-07-27T14:46:05Z

/bigbonk review for issues

ask-bonk · 2026-07-27T14:47:10Z

UnknownError: ProviderInitError

github run

james-elicx · 2026-07-27T14:48:27Z

/bigbonk review for issues

ask-bonk · 2026-07-27T14:49:28Z

UnknownError: ProviderInitError

github run

james-elicx · 2026-07-27T14:51:30Z

/bigbonk review for issues

ask-bonk

Reviewed the full diff at head, tracing every emitter and consumer of the new stale / serverStaleTime dimension and re-checking each prior review finding against the current code (not the commit the earlier reviews ran against). No blocking issues.

Architecture

Sound. resolveClientStaleTimeSeconds (utils/cache-control-metadata.ts:40) is a single projection routed through every write/replay/seed path; the two normalization rules (never synthesize stale from revalidate/expire, never clamp) are documented at the resolver and hold. stale never enters streaming headers — it travels only on completion-time channels (ISR write, HIT/STALE replay, done-script, prerender seed, or the post-Flight footer). The byte-stuffed footer framing (0xff 0xff for reserved bytes, 0xff 0x00 as the terminal footer tag) is clean and the encode/decode/extract paths share one validator.

Prior findings — all resolved at head

KV stale round-trip (Codex P1): kv-data-adapter.runtime.ts:369,391,583 persists and validates cacheControl.stale; prerender-kv-populate.ts:130-170 carries route.stale into bulk-import entries. ✅
Prerender index projection (Codex P1): PrerenderRouteResult.stale declared and copied by writePrerenderIndex; seedMemoryCacheFromPrerender threads it through. ✅
Nested use cache HIT propagation (Codex P1): recordRequestScopedCacheControl (cache-runtime.ts:698) pushes the stored lifetime into the enclosing lifeConfigs, mirroring the MISS path, and only when a parent scope exists. ✅
Done-script dynamic bound on captured dynamic renders (Codex P1): app-page-render.ts:1047-1051 emits dynamicStaleTimeSeconds whenever the completed kind is dynamic, regardless of the speculative-capture decision. ✅
Late-dynamic completion dropping the pending bound (Codex/last-bonk P2): resolved and covered. The completion footer for a dynamic render always includes the serverStaleTimeSeconds key (app-page-render.ts:917-920 emits null or a number), so hasCompletedServerStaleTime (navigation.ts:1002) is always true for a completed dynamic render and the provisional pending state is dropped. tests/prefetch-cache.test.ts:456-482 pins exactly the scenario the last review flagged: dynamicStaleTimeSeconds: 60 + pending header → resolveCachedRscResponseTtlMs returns 60_000, not 30_000. The concern in the most recent bonk review does not reproduce against this head. ✅
Footer stripping withholding a 256-byte tail (Codex/last-bonk P2): the streaming decoder was rewritten to byte-stuffing (stripRscCompletionMetadata, rsc-completion-metadata.ts:167). It now holds at most one ambiguous escape byte and passes ordinary chunks through without copying — no fixed 256-byte tail is retained on the live-navigation path. ✅

Independent checks

x-nextjs-stale-time matches Next.js NEXT_ROUTER_STALE_TIME_HEADER.
The positional→policy isrSet refactor updated every caller, including the pages-basic fixture endpoint (seed-revalidate-cache-headers.ts:19-24).
escapeFlightChunk escapes per-chunk with both bytes emitted together, so a chunk boundary can never split an escape pair; the decoder carries escapePending across chunks correctly.
Snapshot seeding (app-browser-entry.ts:2269-2296) correctly refuses to publish a completed-dynamic response as a reusable static snapshot and drops the provisional prefetch expiry in favor of the resolved bound.
Ran tests/rsc-completion-metadata.test.ts, tests/shims.test.ts, tests/prefetch-cache.test.ts locally — all green (1320 + 16 + prefetch suite).

The two documented known limits (edge-CDN no-op set() capping RSC reuse at the 30s floor; late-dynamic streaming not enforcing staleTimes.dynamic below 30s at header time) are deliberate trade-offs tied to #961 and reasonable given the streaming model.

One minor non-blocking note inline.

ask-bonk · 2026-07-27T14:56:03Z

+  cacheLife: { stale?: number } | null | undefined,
+): number | undefined {
+  const stale = cacheLife?.stale;
+  return isFiniteNonNegative(stale) ? stale : undefined;


Minor / non-blocking: resolveClientStaleTimeSeconds returns 0 for cacheLife({ stale: 0 }) (0 is finite and non-negative), which flows through as a real x-nextjs-stale-time: 0 claim; the client floors it back to 30s via Math.max(seconds, 30), matching Next.js getStaleTimeMs. That behavior is correct, but the undefined-vs-0 boundary is load-bearing for the "absent = no claim" invariant this resolver enforces. Worth a one-line note that stale: 0 is intentionally carried as a real claim (not dropped as "no claim") so a future reader doesn't "simplify" the check to stale ? stale : undefined and silently change the semantics.

ask-bonk · 2026-07-27T14:56:14Z

Review posted successfully. Here's a summary of what I did:

Review Summary for PR #2708

I reviewed the full diff at the current head (cbbb4de), tracing every emitter and consumer of the new stale / serverStaleTime dimension, and specifically re-verified each prior review finding against the current code rather than the commits the earlier reviews ran against. Posted a COMMENT verdict (no blocking issues) with one minor inline note.

Key finding: the two most-recent P2s are already resolved at head

The last ask-bonk review (against an older commit 3df0304b54) flagged two open P2s. Both are fixed in the current head:

Late-dynamic completion dropping the pending bound — The completion footer for a dynamic render always includes the serverStaleTimeSeconds key (null or a number, per app-page-render.ts:917-920), so hasCompletedServerStaleTime is always true for a completed dynamic render and the provisional pending state is dropped. tests/prefetch-cache.test.ts:456-482 pins the exact scenario (dynamic=60 + pending header → TTL resolves to 60s, not 30s). The bug described in the prior review does not reproduce here.
Footer stripping withholding a 256-byte tail — stripRscCompletionMetadata was rewritten to byte-stuffed framing that holds at most one ambiguous escape byte and passes ordinary chunks through without copying. No fixed 256-byte tail is retained on the live-navigation path.

Also verified resolved

The three earlier Codex P1s (KV round-trip, prerender index projection, nested use cache HIT propagation) and the done-script dynamic-bound P1 are all correctly fixed with regression tests.

Independent checks

Byte-stuffing encode/decode correctness (escape pairs can't split across chunk boundaries)
Snapshot seeding refuses to publish completed-dynamic responses as reusable static snapshots
isrSet positional→policy refactor updated every caller including the flagged fixture
Ran 3 relevant test files locally — all green

One minor non-blocking note: stale: 0 is intentionally carried as a real claim (not dropped as "no claim"), and the undefined-vs-0 boundary is load-bearing — worth a comment to prevent a future "simplification" from changing the semantics.

github run

james-elicx · 2026-07-27T15:06:00Z

/bigbonk review for issues

ask-bonk

Reviewed the full diff at head (cbbb4de), tracing every emitter and consumer of the new stale / serverStaleTime dimension and re-checking each prior review finding against the current code. No blocking issues.

Architecture

Sound. resolveClientStaleTimeSeconds (utils/cache-control-metadata.ts:40) is a single projection routed through every write/replay/seed path; the two documented normalization rules (never synthesize stale from revalidate/expire, never clamp) hold. stale never enters streaming Cache-Control — it travels only on completion-time channels: ISR write, HIT/STALE replay (app-page-cache.ts:247), done-script, prerender seed, or the post-Flight footer. The byte-stuffed footer framing (0xff 0xff for reserved bytes, 0xff 0x00 as the terminal tag) is clean and fail-closed; the encode/decode/extract paths share one validator.

Prior findings — all resolved at head

KV stale round-trip (P1): kv-data-adapter.runtime.ts:369,391,583 persists and validates cacheControl.stale; prerender-kv-populate.ts:130-170 carries route.stale into bulk-import entries. ✅
Prerender index projection (P1): PrerenderRouteResult.stale declared and copied by writePrerenderIndex; seedMemoryCacheFromPrerender threads it through. ✅
Nested use cache HIT propagation (P1): recordRequestScopedCacheControl (cache-runtime.ts:698) pushes the stored lifetime into the enclosing lifeConfigs, mirroring the MISS path, and only when a parent scope exists. ✅
Done-script dynamic bound on captured dynamic renders (P1): app-page-render.ts:1047-1051 emits dynamicStaleTimeSeconds whenever the completed kind is dynamic, regardless of the speculative-capture decision. ✅
Late-dynamic completion dropping the pending bound (P2): resolved and covered. The completion footer for a dynamic render always includes serverStaleTimeSeconds (app-page-render.ts:917-920 emits null or a number), so hasCompletedServerStaleTime (navigation.ts:1002-1003) is always true for a completed dynamic render and the provisional pending state is dropped. tests/prefetch-cache.test.ts:456-482 pins the exact scenario a prior review flagged: dynamicStaleTimeSeconds: 60 + pending header → TTL resolves to 60_000, not 30_000. The bug does not reproduce against this head. ✅
Footer stripping withholding a 256-byte tail (P2): stripRscCompletionMetadata (rsc-completion-metadata.ts:167) was rewritten to byte-stuffing. It holds at most one ambiguous escape byte and passes ordinary chunks through without copying — no fixed 256-byte tail is retained on the live-navigation path. tests/rsc-completion-metadata.test.ts:51 pins early delivery of a small Flight shell. ✅

Independent checks

x-nextjs-stale-time matches Next.js canary NEXT_ROUTER_STALE_TIME_HEADER verbatim (verified against app-router-headers.ts).
The positional→policy isrSet refactor updated every caller, including the flagged pages-basic fixture endpoint (seed-revalidate-cache-headers.ts:19-24).
escapeFlightChunk escapes per-chunk with both bytes emitted together, so a chunk boundary can never split an escape pair; the streaming decoder carries escapePending across chunks correctly.
Malformed-frame rejection is exhaustive: invalid escapes, truncated footers, oversized frames, multiple frames, cancellation, and error propagation are all tested and fail closed.
Snapshot seeding (app-browser-entry.ts:2269-2296) correctly refuses to publish a completed-dynamic response as a reusable static snapshot and drops the provisional prefetch expiry in favor of the resolved bound.
Ran ~10 relevant test files locally (rsc-completion-metadata, prefetch-cache, app-page-cache, app-page-render, shims, kv-cache-handler, seed-cache, prerender, app-visited-response-cache, app-page-cache-render) — all green (1646+ tests).

The two documented known limits (edge-CDN no-op set() capping RSC reuse at the 30s floor; late-dynamic streaming not enforcing staleTimes.dynamic below 30s at header time) are deliberate trade-offs tied to #961 and reasonable given the streaming model.

One minor non-blocking note inline.

ask-bonk · 2026-07-27T15:10:42Z

+  cacheLife: { stale?: number } | null | undefined,
+): number | undefined {
+  const stale = cacheLife?.stale;
+  return isFiniteNonNegative(stale) ? stale : undefined;


Minor / non-blocking: resolveClientStaleTimeSeconds returns 0 for cacheLife({ stale: 0 }) (0 is finite and non-negative), which flows through applyClientStaleTimeHeader (app-page-response.ts:139, which correctly gates on === undefined rather than falsy) and emits a real x-nextjs-stale-time: 0 claim. The client floors it to 30s via Math.max(seconds, 30), matching Next.js getStaleTimeMs, so behavior is correct.

The undefined-vs-0 boundary is load-bearing for the "absent = no claim" invariant this resolver enforces. Worth a one-line note that stale: 0 is intentionally carried as a real claim (not dropped as "no claim"), so a future reader doesn't "simplify" isFiniteNonNegative(stale) ? stale : undefined to a stale ? stale : undefined check and silently change the semantics.

ask-bonk · 2026-07-27T15:10:54Z

Review posted successfully. Here's a summary of what I found:

Review Summary for PR #2708

I reviewed the full diff at head (cbbb4de), tracing every emitter and consumer of the new stale / serverStaleTime cache-control dimension, and re-verified each prior review finding against the current code (not the commits earlier reviews ran against). Posted a COMMENT verdict — no blocking issues — with one minor inline note.

Key verification: both most-recent P2s are resolved at head

Late-dynamic completion dropping the pending bound — The completion footer for a dynamic render always includes serverStaleTimeSeconds (app-page-render.ts:917-920), so hasCompletedServerStaleTime is always true and the provisional pending state is dropped. tests/prefetch-cache.test.ts:456-482 pins dynamic=60 + pending header → 60s (not 30s). The bug described in an earlier review does not reproduce here.
Footer stripping 256-byte tail — Rewritten to byte-stuffed framing; holds at most one ambiguous escape byte, passes ordinary chunks through without copying. A dedicated test pins early delivery of a small Flight shell.

Also confirmed resolved

The three earlier Codex P1s (KV round-trip, prerender index projection, nested use cache HIT propagation) and the done-script dynamic-bound P1 are all correctly fixed with regression tests.

Independent checks

x-nextjs-stale-time matches Next.js canary verbatim
isrSet positional→policy refactor updated every caller including the flagged fixture
Byte-stuffing framing is fail-closed with exhaustive malformed-frame tests
Snapshot seeding refuses to publish completed-dynamic as static
Ran ~10 test files locally (1646+ tests) — all green

The two documented known limits (edge-CDN no-op set(), late-dynamic streaming below 30s) are deliberate trade-offs tied to #961 and reasonable.

One minor non-blocking inline note: the undefined-vs-0 boundary in resolveClientStaleTimeSeconds is load-bearing (stale: 0 is intentionally carried as a real claim), worth a comment to prevent a future "simplification."

github run

james-elicx · 2026-07-27T15:12:16Z

/bigbonk review for issues

james-elicx · 2026-07-27T15:23:18Z

/bigbonk review for issues

ask-bonk · 2026-07-27T15:24:21Z

UnknownError: ProviderInitError

github run

james-elicx · 2026-07-27T15:28:25Z

/bigbonk review for issues

james-elicx · 2026-07-27T16:35:45Z

/bigbonk review for issues

ask-bonk

Reviewed the full diff at head (35175ac), tracing every emitter and consumer of the new stale / serverStaleTime dimension and re-verifying each prior review finding against the current code. No blocking issues.

Architecture

Sound. resolveClientStaleTimeSeconds (utils/cache-control-metadata.ts:40) is a single projection routed through every write/replay/seed path, and the two documented normalization rules (never synthesize stale from revalidate/expire, never clamp) hold. stale never enters streaming Cache-Control — it travels only on completion-time channels: ISR write, HIT/STALE replay (app-page-cache.ts:247), done-script, prerender seed, or the post-Flight footer.

Prior findings — all resolved at head

KV stale round-trip (P1): kv-data-adapter.runtime.ts:369,391,583 persists and validates cacheControl.stale; prerender-kv-populate.ts:130-170 carries route.stale into bulk-import entries. ✅
Prerender index projection (P1): PrerenderRouteResult.stale declared and copied by writePrerenderIndex; seedMemoryCacheFromPrerender threads it through. ✅
Nested use cache HIT propagation (P1): recordRequestScopedCacheControl (cache-runtime.ts:698) pushes the stored lifetime into the enclosing lifeConfigs at line 712 (mirroring the MISS path) and to the request scope at line 732, and only when a parent scope exists. It fires only on the HIT path (cache-runtime.ts:584,589); the MISS path's push lives in runCachedFunctionWithContext, so there is no double-count. ✅
Done-script dynamic bound on captured dynamic renders (P1): app-page-render.ts:1047-1051 emits dynamicStaleTimeSeconds whenever the completed kind is dynamic, regardless of the speculative-capture decision. ✅
Late-dynamic completion dropping the pending bound (P2): resolved. The completion footer for a dynamic render always includes serverStaleTimeSeconds (app-page-render.ts:917-920 emits null or a number), so hasCompletedServerStaleTime (navigation.ts:1002-1003) is true for a completed dynamic render and the provisional pending state is replaced by the resolved bound (navigation.ts:1008-1012). tests/prefetch-cache.test.ts pins dynamicStaleTimeSeconds: 60 + pending header → 60s, not 30s. ✅
Footer stripping withholding a 256-byte tail (P2): stripRscCompletionMetadata (rsc-completion-metadata.ts:167) uses byte-stuffing, holds at most one ambiguous escape byte, and passes ordinary chunks through without copying (:212,217) — no fixed 256-byte tail retained on the live path. tests/rsc-completion-metadata.test.ts:51 pins early delivery of a small Flight shell. ✅

Independent checks

Byte-stuffed framing is unambiguous: escapeFlightChunk doubles every 0xff, so a real 0xff 0x00 in Flight data decodes as 0xff + 0x00 and can never be mistaken for the 0xff 0x00 footer tag. Encode/decode is correct across all chunk boundaries (:98-124) and fail-closed on invalid escapes, truncated, oversized, and multiple frames.
extractRscCompletionMetadata passes the entire remaining buffer to parseFooterCandidate, whose length check rejects any trailing bytes after the footer (multiple-frame guard).
The undefined-vs-0 boundary is guarded with === undefined at every emitter (app-page-response.ts:139, :161, :167), so stale: 0 correctly flows as a real claim.
The positional→policy isrSet refactor updated every caller including the pages-basic fixture endpoint (seed-revalidate-cache-headers.ts:19-24).
Snapshot seeding (app-browser-entry.ts:2269-2296) refuses to publish a completed-dynamic response as a reusable static snapshot.
Ran tests/rsc-completion-metadata.test.ts, tests/shims.test.ts, tests/prefetch-cache.test.ts locally — all green (1336 tests).

The two documented known limits (edge-CDN no-op set() capping RSC reuse at the 30s floor; late-dynamic streaming not enforcing staleTimes.dynamic below 30s at header time) are deliberate trade-offs tied to #961 and reasonable given the streaming model.

One minor non-blocking note inline.

ask-bonk · 2026-07-27T16:39:17Z

+  cacheLife: { stale?: number } | null | undefined,
+): number | undefined {
+  const stale = cacheLife?.stale;
+  return isFiniteNonNegative(stale) ? stale : undefined;


Minor / non-blocking (flagged by prior reviews, still uncommented in code): resolveClientStaleTimeSeconds returns 0 for cacheLife({ stale: 0 }) because 0 is finite and non-negative, so it flows through applyClientStaleTimeHeader (which correctly gates on === undefined, not falsy) and emits a real x-nextjs-stale-time: 0. The client then floors it to 30s via Math.max(seconds, 30), matching Next.js getStaleTimeMs, so behavior is correct.

The undefined-vs-0 boundary is load-bearing for the "absent = no claim" invariant this resolver enforces. Worth a one-line note so a future reader doesn't "simplify" isFiniteNonNegative(stale) ? stale : undefined to a stale ? stale : undefined check and silently change the semantics — that rewrite would drop stale: 0 as "no claim" and let the configured fallback license far more reuse.

ask-bonk · 2026-07-27T16:39:26Z

Review posted successfully. Here's a summary of what I found:

Review Summary for PR #2708

I reviewed the full diff at head (35175ac), tracing every emitter and consumer of the new stale / serverStaleTime cache-control dimension and independently re-verifying each prior review finding against the current code. Posted a COMMENT verdict — no blocking issues — with one minor inline note.

Verified correct

Architecture — resolveClientStaleTimeSeconds is a single projection routed through every write/replay/seed path; stale never enters streaming Cache-Control, only completion-time channels.
All four prior P1s resolved — KV round-trip, prerender index projection, nested use cache HIT propagation (correctly pushes to both request scope and parent lifeConfigs, only on the HIT path so no double-count), and the done-script dynamic bound.
Both prior P2s resolved — late-dynamic completion drops the pending bound (footer always carries serverStaleTimeSeconds), and footer stripping no longer withholds a 256-byte tail (rewritten to byte-stuffing).
Independent checks — byte-stuffing framing is unambiguous (0xff doubling prevents footer-tag collision), multiple-frame guard via length check, undefined-vs-0 boundary guarded with === undefined, isrSet refactor updated all callers, and completed-dynamic responses are refused as static snapshots. Ran 3 relevant test files locally — all 1336 tests green.

One minor non-blocking note

The stale: 0 handling in resolveClientStaleTimeSeconds is intentionally carried as a real claim (not dropped as "no claim"), and the undefined-vs-0 distinction is load-bearing — worth a one-line comment to prevent a future "simplification" from silently changing the semantics.

The two documented known limits (edge-CDN no-op set(), late-dynamic streaming below 30s) are deliberate trade-offs tied to #961 and reasonable.

github run

james-elicx · 2026-07-27T16:41:27Z

/bigbonk review for issues

ask-bonk · 2026-07-27T16:42:29Z

UnknownError: ProviderInitError

github run

james-elicx · 2026-07-27T16:43:22Z

/bigbonk review for issues

ask-bonk

Reviewed the full diff at head (3b7beb08), tracing every emitter and consumer of the new stale / serverStaleTime / completion-footer machinery independently rather than trusting the prior review chain. No blocking issues.

Architecture

Sound. resolveClientStaleTimeSeconds (utils/cache-control-metadata.ts:40) is a single projection routed through every write/replay/seed path, and at head it correctly does not clamp stale by expire (line 44 is isFiniteNonNegative(stale) ? stale : undefined) — matching the PR's stated "never clamp" rule. (Note: gh pr diff still surfaces an older revision of this function that lived in app-page-response.ts and did Math.min(stale, expire); that clamping revision is not present in the final tree, so the diff view is misleading here but the shipped code is correct.)

stale never enters streaming Cache-Control — it travels only on completion-time channels: ISR write, HIT/STALE replay (app-page-cache.ts:247), the done-script, the prerender seed, or the post-Flight footer.

Prior findings — all resolved at head

KV stale round-trip (P1): kv-data-adapter.runtime.ts reads effectiveStale and persists/validates cacheControl.stale; prerender-kv-populate.ts carries route.stale into bulk-import entries. ✅
Prerender index projection (P1): PrerenderRouteResult.stale declared and copied by writePrerenderIndex; seedMemoryCacheFromPrerender threads it through. ✅
Nested use cache HIT propagation (P1): recordRequestScopedCacheControl (cache-runtime.ts:698) pushes the stored lifetime into the enclosing lifeConfigs (mirroring the MISS path) and the request scope, only when a parent scope exists — no double-count. ✅
Done-script dynamic bound on captured dynamic renders (P1): app-page-render.ts:1047-1051 emits dynamicStaleTimeSeconds whenever the completed kind is dynamic, regardless of the speculative-capture decision. ✅
Late-dynamic completion dropping the pending bound (P2): resolved. The footer always carries serverStaleTimeSeconds (app-page-render.ts:915-921, null or a number), so hasCompletedServerStaleTime (navigation.ts:1002) is true for a completed dynamic render and the provisional pending state is replaced (navigation.ts:1008-1012). ✅
Footer stripping withholding a 256-byte tail (P2): stripRscCompletionMetadata uses byte-stuffing, holds at most one ambiguous escape byte, and passes ordinary chunks through without copying (rsc-completion-metadata.ts:210-217). tests/rsc-completion-metadata.test.ts:51 pins early delivery of a small Flight shell. ✅

Independent checks I ran

Round-trip escaping discipline is airtight. The "1" completion header means the body was byte-stuffed (needs strip/extract); "resolved" means clean body + header-only metadata (no strip). Verified every RSC-response consumer honors this: live nav strips (app-browser-entry.ts:2185,2542), snapshot buffers extract (navigation.ts:995), and preparePrefetchResponse (link.tsx:901 → app-browser-entry.ts:2404) only ever receives already-decoded snapshot buffers via restoreRscResponse, which sets "resolved" (never "1") and never re-escapes. No consumer feeds escaped bytes to React.
Byte-stuffing is unambiguous: escapeFlightChunk doubles every 0xff, so a real 0xff 0x00 in Flight data decodes as two bytes and can never collide with the 0xff 0x00 footer tag. The every-byte-split test (:98-124) confirms escape pairs never split across chunk boundaries.
Fail-closed framing: invalid escapes, truncated footers, oversized (>256B) frames, and multiple frames all reject in both the buffered and streaming decoders; trailing bytes after a footer are caught by the exact-length check in parseFooterCandidate (:58).
isrSet positional→policy refactor updated every caller, including the pages-basic fixture endpoint and the Pages Router writePagesIsrCache (revalidate: false still round-trips via isrCacheControl(false)).
Snapshot seeding (app-browser-entry.ts:2269-2296) refuses to publish a completed-dynamic response as a reusable static snapshot.
Ran tests/rsc-completion-metadata, tests/prefetch-cache, tests/app-page-cache, tests/shims, tests/kv-cache-handler locally — all green (1447 tests).

The two documented known limits (edge-CDN no-op set() capping RSC reuse at the 30s floor; late-dynamic streaming not enforcing staleTimes.dynamic below 30s at header time) are deliberate trade-offs tied to #961 and reasonable given the streaming model.

One minor non-blocking note inline — the only thing still worth touching.

ask-bonk · 2026-07-27T16:48:03Z

+  cacheLife: { stale?: number } | null | undefined,
+): number | undefined {
+  const stale = cacheLife?.stale;
+  return isFiniteNonNegative(stale) ? stale : undefined;


Minor / non-blocking (raised by several prior reviews, still uncommented in code): resolveClientStaleTimeSeconds returns 0 for cacheLife({ stale: 0 }) because 0 is finite and non-negative, so it flows through applyClientStaleTimeHeader (which correctly gates on === undefined, not falsy) and emits a real x-nextjs-stale-time: 0. The client then floors it to 30s via Math.max(seconds, 30), matching Next.js getStaleTimeMs — so behavior is correct.

The undefined-vs-0 boundary is load-bearing for the "absent = no claim" invariant this resolver enforces. Worth a one-line note so a future reader doesn't "simplify" the finite check to a truthy check and silently reclassify stale: 0 as "no claim" (which would then let the configured staleTimes fallback license far more reuse than the render declared):

Suggested change

return isFiniteNonNegative(stale) ? stale : undefined;

export function resolveClientStaleTimeSeconds(

cacheLife: { stale?: number } | null | undefined,

): number | undefined {

// `stale: 0` is a real claim (floored to 30s client-side), NOT "no claim".

// The undefined-vs-0 boundary is load-bearing: only an absent `stale` means

// "no client-freshness claim" and lets the configured fallback apply. Do not

// collapse this to a truthy check.

const stale = cacheLife?.stale;

return isFiniteNonNegative(stale) ? stale : undefined;

}

ask-bonk · 2026-07-27T16:48:16Z

Review posted successfully. Here's a summary of what I found:

Review Summary for PR #2708

I reviewed the full diff at head (3b7beb08), tracing every emitter and consumer of the new stale / serverStaleTime / completion-footer machinery independently rather than trusting the prior review chain. Posted a COMMENT verdict — no blocking issues — with one minor inline note.

Notable finding while verifying

The gh pr diff view surfaces an older, superseded revision of resolveClientStaleTimeSeconds that lived in app-page-response.ts and did Math.min(stale, expire) — which would clamp stale by expire, contradicting the PR's stated "never clamp" rule. I confirmed against the actual head commit (git show 3b7beb08:...) that this clamping revision is not in the shipped tree; the function was moved to utils/cache-control-metadata.ts and the clamp removed (final version: isFiniteNonNegative(stale) ? stale : undefined). So the code is correct — but the diff view is misleading, which I called out so a reader doesn't get alarmed.

Verified correct

Architecture — single projection through every write/replay/seed path; stale never enters streaming Cache-Control, only completion-time channels.
All four prior P1s and both P2s resolved — KV round-trip, prerender index projection, nested use cache HIT propagation, done-script dynamic bound, late-dynamic pending replacement, and byte-stuffed footer stripping (no 256-byte tail withholding).
Round-trip escaping discipline is airtight — traced every RSC-response consumer; the "1" (escaped body + footer) vs "resolved" (clean body, header-only) header contract is honored everywhere, including the prefetch path via restoreRscResponse. No consumer feeds escaped bytes to React.
Byte-stuffing is provably unambiguous and fail-closed (invalid/truncated/oversized/multiple-frame rejection tested).
Ran 5 relevant test suites locally — 1447 tests green.

One minor non-blocking note

The undefined-vs-0 boundary in resolveClientStaleTimeSeconds is load-bearing (stale: 0 is intentionally a real claim floored to 30s, not "no claim"). Posted a suggestion to add a one-line comment so a future "simplification" to a truthy check doesn't silently reclassify it.

github run

james-elicx · 2026-07-27T16:50:56Z

/bigbonk review for issues

ask-bonk

Reviewed the full diff at head (0d4532a), tracing every emitter and consumer of the new stale / serverStaleTime / completion-footer machinery independently. No blocking issues.

Architecture

Sound. resolveClientStaleTimeSeconds (utils/cache-control-metadata.ts:41) is a single projection routed through every write/replay/seed path; the two documented normalization rules (never synthesize stale from revalidate/expire, never clamp) hold — line 45 is isFiniteNonNegative(stale) ? stale : undefined. stale never enters streaming Cache-Control; it travels only on completion-time channels: ISR write, HIT/STALE replay, done-script, prerender seed, or the post-Flight footer.

The minor note raised across every prior review — the undefined-vs-0 boundary being load-bearing — is now documented in the resolver JSDoc (cache-control-metadata.ts:38-39, added in the docs(cache) commit at head). Nothing left to flag there.

Prior findings — all resolved at head

KV stale round-trip (P1): kv-data-adapter.runtime.ts:369,391 persists cacheControl.stale; validateCacheEntry:583 accepts it on read; prerender-kv-populate.ts carries route.stale into bulk-import entries. ✅
Prerender index projection (P1): PrerenderRouteResult.stale declared and copied by writePrerenderIndex; seedMemoryCacheFromPrerender threads it through staleSeconds. ✅
Nested use cache HIT propagation (P1): recordRequestScopedCacheControl (cache-runtime.ts:698) pushes the stored lifetime into the enclosing lifeConfigs (:712, mirroring the MISS path) and only when a parent scope exists. ✅
Done-script dynamic bound on captured dynamic renders (P1): app-page-render.ts:1047-1051 emits dynamicStaleTimeSeconds whenever the completed kind is dynamic, regardless of the speculative-capture decision. ✅
Late-dynamic completion dropping the pending bound (P2): resolved. The completion footer for a dynamic render always includes serverStaleTimeSeconds (app-page-render.ts:917-920 emits null or a number), so hasCompletedServerStaleTime (navigation.ts:1002-1003) is true for a completed dynamic render and the provisional pending state is replaced (navigation.ts:1008-1012). ✅
Footer stripping withholding a 256-byte tail (P2): stripRscCompletionMetadata (rsc-completion-metadata.ts:167) uses byte-stuffing, holds at most one ambiguous escape byte, and passes ordinary chunks through without copying (:210-217). tests/rsc-completion-metadata.test.ts:51 pins early delivery of a small Flight shell. ✅

Independent checks

Byte-stuffing is unambiguous and fail-closed. escapeFlightChunk doubles every 0xff, so a real 0xff 0x00 in Flight data decodes as two bytes and can never collide with the 0xff 0x00 footer tag. The every-byte-split test (:98-124) confirms escape pairs never split across chunk boundaries. Multiple frames are caught by the exact-length check in parseFooterCandidate:58 (payloadLength !== lengthOffset - FOOTER_PREFIX_BYTES); invalid escapes, truncation, and oversized frames all reject in both decoders.
Escaped-vs-clean body contract is honored. The completion header "1" means the body was byte-stuffed (strip/extract required); restoreRscResponse (navigation.ts:1089) sets "resolved" on a snapshot buffer that was already extracted, so no consumer ever re-strips or feeds escaped bytes to React.
isrSet positional→policy refactor updated every caller, including the pages-basic fixture endpoint (seed-revalidate-cache-headers.ts:19-24, now passing { cacheControl: { revalidate: 3600 } }).
Hydration seeding (app-browser-entry.ts:1528-1568) carries the done-script staleTimeSeconds onto the visited-response snapshot as a resolved server stale time.
Ran tests/rsc-completion-metadata.test.ts (16) and tests/prefetch-cache.test.ts (51) locally at head — all green.

The two documented known limits (edge-CDN no-op set() capping RSC reuse at the 30s floor; late-dynamic streaming not enforcing staleTimes.dynamic below 30s at header time) are deliberate trade-offs tied to #961 and reasonable given the streaming model. LGTM.

ask-bonk · 2026-07-27T16:54:39Z

Review posted successfully. Here's a summary of what I found:

Review Summary for PR #2708

I reviewed the full diff at head (0d4532a), independently tracing every emitter and consumer of the new stale / serverStaleTime / completion-footer machinery. Posted a COMMENT verdict — no blocking issues.

Key findings

Architecture is sound. resolveClientStaleTimeSeconds is a single projection routed through every write/replay/seed path; the two normalization rules (never synthesize, never clamp) hold; stale never enters streaming Cache-Control, only completion-time channels.

All prior review findings are resolved at head, verified against the actual code:

Three Codex P1s (KV round-trip, prerender index projection, nested use cache HIT propagation) — fixed with regression tests
Done-script dynamic bound on captured dynamic renders (P1) — fixed
Late-dynamic completion dropping the pending bound (P2) — resolved; the footer always carries serverStaleTimeSeconds, so hasCompletedServerStaleTime is always true and the pending state is replaced
Footer stripping withholding a 256-byte tail (P2) — rewritten to byte-stuffing, no fixed tail retained

The one recurring minor note (the undefined-vs-0 boundary in the resolver) is now documented in the JSDoc at head, so there was nothing left to flag inline.

Independent verification:

Byte-stuffed framing is provably unambiguous (0xff doubling can't collide with the 0xff 0x00 footer tag) and fail-closed
The escaped-vs-clean body contract ("1" vs "resolved" header) is honored everywhere — no consumer feeds escaped bytes to React
The isrSet positional→policy refactor updated every caller including the flagged fixture endpoint
Ran the two most relevant test suites locally — all 67 tests green

The two documented known limits are deliberate trade-offs tied to #961 and reasonable.

github run

# Conflicts: # packages/vinext/src/shims/navigation.ts

NathanDrake2406 added 6 commits July 25, 2026 18:26

Merge branch 'main' into fix/client-stale-time-from-cache-life

ae4273b

chatgpt-codex-connector Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread packages/vinext/src/server/isr-cache.ts Outdated

Comment thread packages/vinext/src/build/prerender.ts

Comment thread packages/vinext/src/server/app-page-render.ts Outdated

Comment thread packages/vinext/src/shims/cache-runtime.ts

NathanDrake2406 marked this pull request as draft July 25, 2026 17:00

NathanDrake2406 added 2 commits July 26, 2026 03:13

perf: trim shipped comment bytes in the stale-time plumbing

83296eb

The added modules ride in every consumer build environment; the review-grade rationale lives in the PR body, the code keeps one-line constraints.

NathanDrake2406 marked this pull request as ready for review July 25, 2026 17:36

NathanDrake2406 added 2 commits July 27, 2026 16:28

chatgpt-codex-connector Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread packages/vinext/src/server/isr-cache.ts

Comment thread packages/vinext/src/server/app-page-render.ts

ci: re-run after unrelated dev-overlay canary flake

ad844f7

This was referenced Jul 27, 2026

Flaky E2E: dev error overlay soft-nav canary lost to a pre-hydration Link click #2716

Closed

test(e2e): gate clicks that can land before hydration commits #2717

Merged

ask-bonk Bot reviewed Jul 27, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into codex/pr-2708-ready

35175ac

ask-bonk Bot reviewed Jul 27, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into codex/pr-2708-ready

3b7beb0

ask-bonk Bot reviewed Jul 27, 2026

View reviewed changes

docs(cache): clarify zero stale client claim

0d4532a

ask-bonk Bot reviewed Jul 27, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into codex/pr-2708-ready

f67488f

# Conflicts: # packages/vinext/src/shims/navigation.ts

Uh oh!

Conversation

NathanDrake2406 commented Jul 25, 2026 • edited by james-elicx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Why

What changed

Review hardening

References

Uh oh!

pkg-pr-new Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance benchmarks

Uh oh!

NathanDrake2406 commented Jul 25, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NathanDrake2406 commented Jul 25, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 25, 2026

Uh oh!

NathanDrake2406 commented Jul 27, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

NathanDrake2406 commented Jul 27, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 27, 2026

Uh oh!

james-elicx commented Jul 27, 2026

Uh oh!

ask-bonk Bot commented Jul 27, 2026

Uh oh!

ask-bonk Bot commented Jul 27, 2026

Uh oh!

james-elicx commented Jul 27, 2026

Uh oh!

ask-bonk Bot commented Jul 27, 2026

Uh oh!

james-elicx commented Jul 27, 2026

Uh oh!

ask-bonk Bot commented Jul 27, 2026

Uh oh!

james-elicx commented Jul 27, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Architecture

Prior findings — all resolved at head

Independent checks

Uh oh!

ask-bonk Bot Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Jul 27, 2026

Review Summary for PR #2708

Key finding: the two most-recent P2s are already resolved at head

Also verified resolved

Independent checks

Uh oh!

james-elicx commented Jul 27, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Architecture

Prior findings — all resolved at head

NathanDrake2406 commented Jul 25, 2026 •

edited by james-elicx

Loading

pkg-pr-new Bot commented Jul 25, 2026 •

edited

Loading

github-actions Bot commented Jul 25, 2026 •

edited

Loading