fix(isr): honor route expire ceilings#961
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.
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
from
08f892f to
de72991
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de72991bcf
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b37e39517
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62696bd65b
ℹ️ 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".
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix(isr): honor route expire ceilings
This is a well-executed, thorough implementation. I verified the expire ceiling threading end-to-end across all five ISR write paths (App page, App route handler, Pages Router, prerender seed, cached functions), both cache handlers (Memory + KV), the centralized cache-control.ts header construction, the speculative cacheLife write path, and the prerender metadata flow.
Architecture
The data flow is clean: next.config.expireTime → resolved config → __expireTime in generated entries → expireSeconds threaded through render/cache/response helpers → expireAt in cache entries → hard-delete enforcement in handlers → finite stale-while-revalidate in Cache-Control headers. The layering follows the repo's stated preference for thin generated entries delegating to typed helpers.
Key design decisions are sound:
- Stored metadata wins over config:
buildAppPageCachedResponsepreferscacheControlfrom the cache entry over caller-provided defaults. This means route-specific policy survives across deploys even if the global config changes. - Replace, not min-merge, for expire:
cacheLife()replaces the config-level expire (vs min-merge for revalidate). This matches Next.js semantics wherecacheLife()is the authoritative source of expire. - Backward-compatible degradation: Old entries without
expireAt/cacheControlskip expire enforcement and get unbounded SWR headers. Both handlers handle this cleanly via different null/undefined checks appropriate to their storage format. _peekRequestScopedCacheLife: The non-destructive peek for prerender response shaping, leaving_consumeRequestScopedCacheLifefor the manifest writer, is a correct fix for the destructive-read ordering issue.
Test coverage
Good: cache-control.test.ts covers finite SWR, equal revalidate/expire, unknown expire, STALE+known-expire, HIT+known-expire, and revalidate=0 edge cases. ISR tests cover hard-expire enforcement, prerender fixtures exercise cacheLife(), KV tests validate stored metadata shape, and shim tests verify inline config normalization. The cacheLife-only prerender fixtures are a valuable addition.
Remaining observations (non-blocking)
A few minor items that are fine to address later or leave as-is.
|
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 (App page, App route handler, Pages Router, prerender seed, cached functions), both cache handlers (Memory + KV), and response header construction. Key strengths:
5 non-blocking comments posted:
|
NathanDrake2406
force-pushed
the
nathan/fix-isr-expire
branch
2 times, most recently
from
e546ba0 to
75c823e
Compare
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
75c823e to
89d2a14
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 correctly threaded end-to-end across all five ISR write paths (App page, App route handler, Pages Router, prerender seed, cached functions), both cache handlers (Memory + KV), and the centralized Cache-Control header construction.
Key strengths
- Complete data flow:
next.config.expireTime→ resolved config →__expireTimein generated entries →expireSecondsthrough render/cache/response helpers →expireAtin cache entries → hard-delete enforcement in handlers → finitestale-while-revalidateinCache-Controlheaders. - Clean backward compatibility: Old entries without
expireAt/cacheControlskip expire enforcement and get unbounded SWR headers. Both handlers use appropriate null/undefined checks for their storage formats (memory:!== null; KV:!== undefined && !== null). - Stored metadata wins:
buildAppPageCachedResponseandbuildRouteHandlerCachedResponseboth prefercacheControlfrom the cache entry over caller-provided defaults. Route-specific policy survives across deploys. - Background regen correctness: The dispatch layer consumes
_consumeRequestScopedCacheLife()from the regen render, and the cachedcacheControlis used for subsequentisrSetwrites with proper fallback to the global config. _peekRequestScopedCacheLife/_consumeRequestScopedCacheLifesplit: Correctly solves the destructive-read ordering issue where prerender response shaping needs metadata before the manifest writer consumes it.cache-control.tscentralization: Clean module with well-documented branching between STALE-with-expire (route policy) and STALE-without-expire (legacy fallback).- Speculative cacheLife writes:
resolveAppPageCacheWritePolicycorrectly returnsnullwhen no valid cache policy exists, andpreserveClientResponseHeaderspreventsno-storefrom being stamped on responses that may not be ISR-eligible.
CI is fully green across all 21 checks. I have a few non-blocking observations below.
Non-blocking observations
-
expireSeconds: 0ambiguity: Both cache handlers computeexpireAt = effectiveExpire > 0 ? now + ... : null, soexpire: 0means "no expire enforcement" (same asundefined). ButbuildRevalidateCacheControl(60, 0)producess-maxage=60(no SWR) becauserevalidateSeconds >= expireSeconds. The header says "don't serve stale" but the stored entry has no hard-expire timestamp. Not a practical issue sincecacheLife()validation warns whenexpire < revalidate, but the semantic gap is worth being aware of. -
Speculative tee memory overhead: Every production App Router request with
revalidateSeconds === nullcreates a tee'd RSC stream that gets captured and buffered even when the route never callscacheLife(). The write correctly bails, but the buffer is allocated. This is acknowledged as a future optimization target. -
DEFAULT_EXPIRE_TIMEduplication: The constant31_536_000appears in bothnext-config.tsandapp-rsc-entry.ts. A single export would be slightly cleaner, though the current approach is defensible since the entry generator needs a static fallback. -
No
expireTimeconfig validation:resolveNextConfigaccepts anynumberforexpireTimewithout checking for negative values or NaN. The downstreamNumber.isFinite()guard onresolveAppPageCacheWritePolicywould catch NaN for cache writes, but negative values would still flow through tobuildRevalidateCacheControland produce a valid-looking but semantically wrong header.
LGTM — this is a solid, well-tested implementation that correctly matches Next.js expire ceiling semantics.
| serverActionsAllowedOrigins, | ||
| optimizePackageImports, | ||
| serverActionsBodySizeLimit, | ||
| expireTime: typeof config.expireTime === "number" ? config.expireTime : DEFAULT_EXPIRE_TIME, |
There was a problem hiding this comment.
Nit: no validation that config.expireTime is a positive finite number. Negative values or NaN would pass the typeof === "number" check and flow through to cache handlers and buildRevalidateCacheControl.
Not blocking since NaN would be caught by the Number.isFinite() guard in resolveAppPageCacheWritePolicy and negative values are unlikely in practice, but a clamp or warning here would be more defensive:
| expireTime: typeof config.expireTime === "number" ? config.expireTime : DEFAULT_EXPIRE_TIME, | |
| expireTime: typeof config.expireTime === "number" && config.expireTime > 0 ? config.expireTime : DEFAULT_EXPIRE_TIME, |
|
|
||
| // `expire <= revalidate` is a zero-width stale window: downstream caches | ||
| // should refetch after s-maxage instead of serving stale. | ||
| if (revalidateSeconds >= expireSeconds) { |
There was a problem hiding this comment.
When expireSeconds === 0, this condition is true for any non-negative revalidateSeconds, producing s-maxage=N with no SWR directive. But both cache handlers treat expire: 0 as "no expire" (since effectiveExpire > 0 is false, expireAt is null). So the header says "don't serve stale" but the stored entry has no hard-expire enforcement.
This asymmetry is unlikely to occur in practice (cacheLife validation warns when expire < revalidate), but documenting it here would help future readers:
| if (revalidateSeconds >= expireSeconds) { | |
| // `expire <= revalidate` is a zero-width stale window: downstream caches | |
| // should refetch after s-maxage instead of serving stale. | |
| // Note: expire === 0 also lands here; cache handlers treat 0 as "no expire" | |
| // (no expireAt is stored), but the header correctly omits the SWR directive. | |
| if (revalidateSeconds >= expireSeconds) { |
| return entry?.value.value && entry.value.value.kind === "APP_PAGE" ? entry.value.value : null; | ||
| } | ||
|
|
||
| function resolveAppPageCacheWritePolicy(options: { |
There was a problem hiding this comment.
Observation: resolveAppPageCacheWritePolicy and applyRequestCacheLife (in app-page-render.ts:138) share nearly identical merge logic — Math.min for revalidate, direct replacement for expire, same null handling. The different return types justify separate functions, but extracting the shared merge step into a tiny helper (e.g., mergeRequestCacheLifeValues) would eliminate the duplication and make it easier to keep them in sync if the merge semantics evolve.
Not blocking — the current split is clear and well-commented.
| const headers = config?.headers ?? []; | ||
| const allowedOrigins = config?.allowedOrigins ?? []; | ||
| const bodySizeLimit = config?.bodySizeLimit ?? 1 * 1024 * 1024; | ||
| const expireTime = config?.expireTime ?? DEFAULT_EXPIRE_TIME; |
There was a problem hiding this comment.
Minor: DEFAULT_EXPIRE_TIME is duplicated here and in next-config.ts. If the default ever changes, both must be updated in lockstep. Consider exporting from next-config.ts (or a shared constants module) and importing here, since the entry generator already imports other types from that module.
| if (htmlResponsePolicy.shouldWriteToCache) { | ||
| const shouldSpeculativelyWriteCache = | ||
| options.isProduction && | ||
| shouldCaptureRscForCacheMetadata && |
There was a problem hiding this comment.
The speculative tee allocates a captured RSC buffer for every production App Router request with revalidateSeconds === null, even when the route never calls cacheLife(). The write is correctly gated by resolveAppPageCacheWritePolicy, so this is a memory overhead issue, not a correctness one.
Worth noting in a comment that this is a known optimization target — if profiling shows elevated memory on non-ISR routes, this is the place to look.
|
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 the expire threading end-to-end from config resolution through all five ISR write paths, both cache handlers, and response header construction. All 21 CI checks are green. 5 non-blocking inline comments posted:
|
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