fix(isr): honor route expire ceilings#961
fix(isr): honor route expire ceilings#961NathanDrake2406 wants to merge 6 commits intocloudflare:mainfrom
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
This PR updates vinext’s ISR implementation to honor Next.js-style route expire ceilings by tracking and enforcing both revalidate and expire, and by aligning Cache-Control header semantics with Next.js.
Changes:
- Extend cache entries (memory + KV) to include
expireAtand treat beyond-expire entries as hard misses (blocking regeneration). - Plumb
expireTime(with Next.js 1-year default) through config resolution, entry generation, prerender manifests/seeding, and ISR writes. - Centralize
Cache-Controlheader construction to emit finitestale-while-revalidatewindows whenexpireis known.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/pages-page-response.test.ts | Updates expected Pages Router response Cache-Control with finite SWR window. |
| tests/pages-page-data.test.ts | Adds expireSeconds plumbing + updated ISR set call expectations. |
| tests/next-config.test.ts | Adds coverage for expireTime defaulting and config override. |
| tests/kv-cache-handler.test.ts | Adds expire validation + runtime tests for stale-vs-expired behavior. |
| tests/isr-cache.test.ts | Adds expire ceiling behavior tests at ISR layer. |
| tests/cache-control.test.ts | New unit tests for buildRevalidateCacheControl behavior. |
| tests/app-route-handler-response.test.ts | Updates Route Handler Cache-Control expectations to finite SWR. |
| tests/app-route-handler-execution.test.ts | Threads expireSeconds into ISR writes and header expectations. |
| tests/app-route-handler-cache.test.ts | Threads expireSeconds into ISR writes from cache helper path. |
| tests/app-page-response.test.ts | Updates App Page response policy Cache-Control to finite SWR. |
| tests/app-page-render.test.ts | Updates ISR set call signatures to account for new expireSeconds arg. |
| tests/app-page-cache.test.ts | Adds HIT Cache-Control assertion and threads expireSeconds into writes. |
| tests/snapshots/entry-templates.test.ts.snap | Snapshot updates for generated entries (expireTime + shared isr-cache imports). |
| packages/vinext/src/shims/cache.ts | Adds expireAt to memory entries; reads cacheControl.{revalidate,expire} consistently. |
| packages/vinext/src/shims/cache-runtime.ts | Writes cached function results with { cacheControl: { revalidate, expire } }. |
| packages/vinext/src/server/seed-cache.ts | Seeds prerenders with { cacheControl: { revalidate, expire } } when present. |
| packages/vinext/src/server/pages-page-response.ts | Uses shared Cache-Control helper and passes expireSeconds to ISR writes. |
| packages/vinext/src/server/pages-page-data.ts | Uses shared Cache-Control helper and threads expireSeconds into ISR writes/headers. |
| packages/vinext/src/server/isr-cache.ts | Adds expireSeconds to ISR set and treats handler-reported expired as hard miss. |
| packages/vinext/src/server/cache-control.ts | New shared constants + buildRevalidateCacheControl helper aligned with Next.js. |
| packages/vinext/src/server/app-route-handler-response.ts | Replaces inline Cache-Control strings with shared helper/constants + expireSeconds. |
| packages/vinext/src/server/app-route-handler-execution.ts | Threads expireSeconds into response headers and ISR writes. |
| packages/vinext/src/server/app-route-handler-cache.ts | Threads expireSeconds into cached responses and regeneration ISR writes. |
| packages/vinext/src/server/app-page-response.ts | Uses shared Cache-Control helper/constants and accepts expireSeconds. |
| packages/vinext/src/server/app-page-render.ts | Consumes request-scoped cacheLife expire and propagates to policy/cache writes. |
| packages/vinext/src/server/app-page-cache.ts | Threads expireSeconds into cached responses and regeneration ISR writes. |
| packages/vinext/src/index.ts | Plumbs expireTime into App Router config passed to entry generation. |
| packages/vinext/src/entries/pages-server-entry.ts | Embeds expireTime into generated config and passes expireSeconds downstream. |
| packages/vinext/src/entries/app-rsc-entry.ts | Adds expireTime defaulting + passes expireSeconds into helpers and ISR wiring. |
| packages/vinext/src/config/next-config.ts | Adds expireTime to config types and resolves default (1 year). |
| packages/vinext/src/cloudflare/kv-cache-handler.ts | Persists expireAt, enforces hard expiry, and validates stored entry shape. |
| packages/vinext/src/build/prerender.ts | Adds expire to prerender manifest route records and writes it into index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
from
e6ecbdd to
ac18486
Compare
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
from
ac18486 to
05d8419
Compare
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
3 times, most recently
from
672c634 to
97c154a
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(isr): honor route expire ceilings
Solid work. The expire ceiling is threaded consistently through all five ISR write paths (App page, App route handler, Pages, prerender seed, cached functions), both cache handlers (memory + KV) enforce hard expiry, and Cache-Control headers now emit finite stale-while-revalidate windows. The cache-control.ts centralization is clean, the backward compatibility path (unbounded SWR when expire is unknown) is correct, and the prerender manifest now records route-level expire derived from the rendered Cache-Control header rather than blindly using the global config.
Test coverage is good — the new cache-control.test.ts, ISR expire ceiling tests, KV expiry tests, and the prerender fixture with cacheLife({ revalidate: 1, expire: 3 }) all exercise the right boundaries.
I have a few comments — one medium-priority issue around background regeneration using the config-global expire instead of the route's effective expire, and a few smaller items.
|
Review submitted successfully. Here's a summary of my findings: Overall: The PR is well-structured and correctly implements the expire ceiling feature across all ISR paths. The Key issue raised (medium priority):
Other comments:
|
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
from
97c154a to
661b36c
Compare
|
Review follow-up: the Pages Router prerender |
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
from
661b36c to
955e445
Compare
|
/bigbonk review |
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
from
955e445 to
5187ec7
Compare
There was a problem hiding this comment.
Review: fix(isr): honor route expire ceilings
This is solid, well-structured work. The expire ceiling is threaded consistently through all ISR write paths, both cache handlers enforce hard expiry, Cache-Control headers emit finite stale-while-revalidate windows, and the prerender manifest now records route-level expire derived from both cacheLife() and the rendered Cache-Control header.
The previous round's key concern about background regeneration overwriting route-specific expire with the global config has been addressed — renderFreshPageForCache now returns cacheControl from _consumeRequestScopedCacheLife(), and app-page-cache.ts uses the regen result's cacheControl with fallback to options.expireSeconds. The utility extraction (cache-control-metadata.ts) also resolved the prior duplication concern.
I have a few remaining comments — nothing blocking.
|
Review submitted successfully. Here's a summary of my findings: Verdict: APPROVE — The PR is well-structured and correctly implements the expire ceiling feature. Key observations:
Comments posted (non-blocking):
|
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
from
5187ec7 to
d85974b
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(isr): honor route expire ceilings
This is a well-executed feature addition. The expire ceiling is threaded consistently through all five ISR write paths (App page, App route handler, Pages Router, prerender seed, cached functions), both cache handlers enforce hard expiry, and Cache-Control headers now emit finite stale-while-revalidate windows matching Next.js semantics. The architecture is clean: cache-control.ts centralizes header construction, cache-control-metadata.ts extracts shared validation logic, and backward compatibility for entries without expire metadata is preserved throughout.
The previous review rounds' key concerns have been addressed — background regeneration now propagates route-specific cacheControl from the render result, utilities have been deduplicated, and the cacheLife() inline config normalization ensures omitted fields inherit defaults.
A few non-blocking observations below.
|
Review submitted successfully. Here's a summary: Verdict: APPROVE The PR correctly implements Next.js route expire ceilings across the full ISR pipeline. The key strengths:
Four non-blocking comments posted:
|
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
from
ec6cd22 to
ec1f5df
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(isr): honor route expire ceilings
This is a well-executed, thorough implementation. The expire ceiling is threaded consistently through all five ISR write paths (App page, App route handler, Pages Router, prerender seed, cached functions), both cache handlers (memory + KV) enforce hard expiry, and Cache-Control headers now emit finite stale-while-revalidate windows matching Next.js semantics.
Architecture highlights:
cache-control.tsis a clean centralization of header construction with correct backward compatibility for entries without expire metadata.cache-control-metadata.tsdeduplicates thectx.cacheControlfield reading between Memory and KV handlers.- The stored
cacheControlmetadata on cache entries takes priority over caller-level fallbacks inbuildAppPageCachedResponseandbuildRouteHandlerCachedResponse, so route-specific policy survives across deploys even if the global config changes. - Background regeneration correctly propagates
cacheControlfrom the render result, so route-specificcacheLife()policy is preserved across regens. - The
settleCapturedRscRenderForCacheMetadatahelper ensurescacheLife()metadata is available before response headers are finalized. - The prerender path derives expire from
requestCacheLifewith fallback to the renderedCache-Controlheader, matching Next.js behavior.
Test coverage is good: The new cache-control.test.ts, ISR expire ceiling tests, KV expiry tests, stored-metadata-wins tests, cacheLife-only prerender fixtures, and the inline-config-inherits-defaults shim test all exercise the right boundaries.
CI is fully green across all 21 checks (unit, integration, E2E, create-next-app).
I have a few non-blocking observations below — nothing that should block merge.
|
Review submitted successfully. Here's a summary: Verdict: APPROVE The PR correctly implements Next.js route expire ceilings across the full ISR pipeline. All 21 CI checks pass green. The key strengths:
I posted 5 non-blocking observations as inline comments covering: the defensive |
Track expireAt alongside revalidateAt in the memory and KV cache handlers so ISR entries past their expire ceiling become blocking misses instead of stale responses. Plumb expireTime and request cacheLife expire values through App Router, Pages Router, prerender seeding, and cache writes while keeping generated entries as thin app-shape wiring over normal server modules. Match Next.js cache-control semantics for finite stale-while-revalidate windows when an expire value is known.
This reverts commit ee1d1d4.
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
from
06956ec to
eae6e81
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(isr): honor route expire ceilings
This is a thorough, well-structured implementation. The expire ceiling is threaded consistently through all five ISR write paths (App page, App route handler, Pages Router, prerender seed, cached functions), both cache handlers enforce hard expiry, and Cache-Control headers now emit finite stale-while-revalidate windows matching Next.js getCacheControlHeader semantics.
What I verified:
- Expire flows end-to-end from
next.config.ts→ resolved config → generated entries → render lifecycle → cache set → cache get → Cache-Control response headers. - Both
MemoryCacheHandlerandKVCacheHandlerenforce hard expiry (delete + return null pastexpireAt) and returncacheControlmetadata on stale/fresh reads. - Background regeneration in
app-page-cache.tscorrectly propagates route-specificcacheControlfrom the render result, socacheLife()policy survives across regens. - The
settleCapturedRscRenderForCacheMetadatahelper ensurescacheLife()side effects from the RSC render are available before response headers are finalized. - Backward compatibility is clean: old cache entries without
expireAt/cacheControlcontinue to work (no expire enforcement, unbounded SWR headers). ThereadCacheControlNumberFieldutility handles both the new{ cacheControl: { revalidate, expire } }shape and the legacy{ revalidate }shape. - The
cacheLife()inline config normalization ({ ...cacheLifeProfiles.default, ...profile }) matches Next.js behavior where omitted fields inherit the default profile. - All 21 CI checks are green.
Architecture highlights:
cache-control.tsis a clean centralization with well-documented branching between STALE-with-expire (route policy) and STALE-without-expire (legacy fallback).cache-control-metadata.tsdeduplicates ctx field reading between both handlers.- The prerender path correctly derives expire from
requestCacheLife(authoritative) with fallback to the rendered Cache-Control header, then to the global config. - The
isPrerendershort-circuit in the HTML render path avoids double-writing (prerender results are seeded separately).
I have a few minor non-blocking observations below. None should block merge.
| function parseCacheControlSeconds(cacheControl: string, directive: string): number | undefined { | ||
| for (const part of cacheControl.split(",")) { | ||
| const [rawName, rawValue] = part.trim().split("=", 2); | ||
| if (rawName.toLowerCase() !== directive) continue; |
There was a problem hiding this comment.
Nit: rawName is the result of split("=", 2)[0] after trim() on the whole part, but it isn't trimmed itself. A header like s-maxage =60 (space before =) would produce rawName = "s-maxage ", failing the comparison. In practice this never happens because this function only parses headers generated by vinext itself (which never insert spaces around =), but a defensive .trim() on rawName would be more robust.
| if (rawName.toLowerCase() !== directive) continue; | |
| if (rawName.trim().toLowerCase() !== directive) continue; |
| } | ||
|
|
||
| // Eagerly read values that must be captured before the stream is consumed. | ||
| await settleCapturedRscRenderForCacheMetadata(isrRscDataPromise); |
There was a problem hiding this comment.
Worth noting for future readers: settleCapturedRscRenderForCacheMetadata buffers the entire RSC stream before headers are finalized. This is a necessary tradeoff for correctness (cacheLife metadata must be available before response headers are set), but it changes the HTML path from streaming-first to buffer-first semantics for ISR-eligible routes. If TTFB regressions are reported on routes with large RSC payloads, this is the place to look.
| data: IncrementalCacheValue, | ||
| revalidateSeconds: number, | ||
| tags?: string[], | ||
| expireSeconds?: number, |
There was a problem hiding this comment.
Minor: the trailing expireSeconds parameter after tags breaks the established pattern where optional params come last with defaults. Since tags already has a default (?? []), having expireSeconds after it means callers must always pass tags explicitly (or undefined) to specify expire. All current callers do pass tags, so this is fine in practice, but if the function signature is ever extended again, consider using an options object for the optional params.
| // Check time-based expiry — return stale entry with cacheState="stale" | ||
| // Check hard expiry first. Past `expire`, Next.js blocks on fresh | ||
| // regeneration instead of serving stale with background work. | ||
| if (entry.expireAt !== null && Date.now() > entry.expireAt) { |
There was a problem hiding this comment.
Good: the expire check uses entry.expireAt !== null (not !== undefined), which means old MemoryEntry objects that don't have expireAt at all get undefined here, which fails the !== null check, correctly skipping expire enforcement for legacy entries. The KV handler uses !== undefined && !== null for the same reason (JSON deserialization never produces undefined). Clean backward-compat story.
| const staleWhileRevalidate = | ||
| expireSeconds === undefined | ||
| ? ", stale-while-revalidate" | ||
| : revalidateSeconds < expireSeconds |
There was a problem hiding this comment.
Edge case observation: when revalidateSeconds === expireSeconds, the ternary revalidateSeconds < expireSeconds is false, so staleWhileRevalidate is "" and the header is s-maxage=N with no SWR directive at all. This means downstream caches won't serve stale — they'll always refetch after s-maxage. This is correct (zero-width stale window = no stale serving), and the test at line 13 of cache-control.test.ts (buildRevalidateCacheControl(300, 300) → s-maxage=300) verifies it. Just noting the behavior is intentional.
|
Review submitted successfully. Here's a summary: Verdict: APPROVE The PR correctly implements Next.js route expire ceilings across the full ISR pipeline. I verified expire threading end-to-end from config resolution through all five ISR write paths, both cache handlers (memory + KV), and response header construction. Key strengths:
5 non-blocking inline comments posted covering: defensive |
Summary
Closes #957.
Next.js now treats a route-level expire ceiling as the boundary between stale-while-revalidate and blocking regeneration. Vinext was only tracking revalidate, so every time-expired ISR entry stayed eligible for stale serving forever. This PR adds the missing expire dimension and wires it through App Router pages, App Route handlers, Pages Router routes, prerender seeding, and cached function writes.
Next.js references
expirevalue with blocking revalidation vercel/next.js#93211What changed
Validation