refactor(isr): centralise HIT/STALE/MISS + Cache-Control decision in isr-decision.ts by Divkix · Pull Request #1883 · cloudflare/vinext

Divkix · 2026-06-09T15:41:47Z

What

Adds server/isr-decision.ts as the single owner of the ISR cache policy decision. All four call sites that previously re-derived HIT/STALE/MISS disposition, background-regen scheduling, and Cache-Control string selection now delegate to decideIsr() or its MISS helpers.

Also eliminates all hardcoded Cache-Control literals outside cache-control.ts — raw strings like "private, no-cache, no-store, max-age=0, must-revalidate" that were copy-pasted across dev-server.ts, pages-page-response.ts, and pages-page-handler.ts.

Call sites migrated

server/app-page-cache.ts — removed local buildAppPageCacheControl wrapper
server/app-route-handler-response.ts — removed local buildRouteHandlerCacheControl; cached responses use decideIsr(kind:"app-route"), fresh MISS responses use buildAppRouteMissIsrCacheControl (preserves the revalidate=0→NEVER and Infinity→STATIC gates)
server/pages-page-data.ts — buildPagesCacheResponse delegates to decideIsr(kind:"pages")
server/dev-server.ts — all four inline literals replaced

Deliberate behaviour change

Dev STALE responses now emit s-maxage=0, stale-while-revalidate (matching prod Pages Router and the canonical buildCachedRevalidateCacheControl helper) instead of s-maxage=<revalidate>, stale-while-revalidate. The old value incorrectly told downstream caches that a stale-served payload was freshly cacheable — a dev/prod parity gap that was masked by the duplication.

Equivalence

Every other migrated path emits the same header as before. The full per-call-site equivalence table is in the isr-decision.ts module doc comment.

Tests

tests/isr-decision.test.ts — 33 new unit tests covering all dispositions, router kinds, 0/Infinity special cases, metadata fallback semantics, and the MISS helpers
All existing cache-related tests pass unchanged: app-page-cache, app-route-handler-cache, app-route-handler-response, pages-page-data, isr-cache, cache-control, features

…isr-decision.ts Add server/isr-decision.ts as the single owner of the ISR cache policy decision across all four call sites (app-page-cache, app-route-handler, pages-page-data, dev-server). Every cache disposition, background-regen flag, and Cache-Control string now flows through decideIsr() or its MISS helpers. Also eliminate all hardcoded Cache-Control literals outside cache-control.ts (NEVER_CACHE_CONTROL, NO_STORE_CACHE_CONTROL, and raw s-maxage strings in dev-server.ts, pages-page-response.ts, pages-page-handler.ts). One deliberate behaviour change: dev STALE responses now emit s-maxage=0, stale-while-revalidate (matching prod Pages Router) instead of s-maxage=<revalidate>, stale-while-revalidate. This closes a dev/prod parity gap that was masked by the duplication. Closes cloudflare#1783

pkg-pr-new · 2026-06-09T15:43:02Z

Open in StackBlitz

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

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

commit: 78c5a38

…ort)

james-elicx · 2026-06-10T13:20:05Z

/bigbonk review for issues

ask-bonk

Reviewed the refactor end-to-end against cache-control.ts and all four migrated call sites. The equivalence claims hold up:

app-page HIT/STALE: precedence cacheControlMeta?.revalidate ?? revalidateSeconds and the expireSeconds only-when-meta-present rule are preserved exactly. Infinity still routes through buildCachedRevalidateCacheControl's internal STATIC gate. ✅
app-route cached + MISS: the old buildRouteHandlerCacheControl("HIT", ...) for the MISS path reduced to buildRevalidateCacheControl(r, e) for finite values, which is exactly what the new buildAppRouteMissIsrCacheControl emits. The 0→NEVER / Infinity→STATIC gates are carried over. ✅
pages HIT/STALE: cacheControl?.revalidate ?? revalidateSeconds ?? 60 is preserved (meta inside decideIsr, ?? 60 hoisted to effectiveRevalidateSeconds). ✅
dev MISS / nonce / gssp-default: byte-for-byte identical to the previous literals. ✅
dev STALE: the one deliberate change (s-maxage=<secs> → s-maxage=0, stale-while-revalidate) is correct and well-justified — it brings dev in line with prod Pages Router. No existing test asserted the old dev STALE header (tests/features.test.ts only checks x-vinext-cache), so nothing breaks. ✅

Ran tests/isr-decision.test.ts (33 pass) plus app-page-cache, app-route-handler-response, pages-page-data, cache-control (87 pass). All green.

The refactor is solid and removes real copy-paste duplication. One scope/accuracy nit on the module docstring below — not blocking.

ask-bonk · 2026-06-10T13:22:55Z

+/**
+ * Centralised ISR cache-decision module.
+ *
+ * The HIT/STALE/MISS disposition, the `scheduleRegeneration` flag, and the


The docstring claims this module is the single owner of the disposition and scheduleRegeneration flag, and that "No caller may produce these values independently." In practice that isn't what landed: every migrated call site computes isStale (and therefore the disposition) itself and passes it in, then destructures only { cacheControl } from the result. disposition and scheduleRegeneration are never read by any caller (verified across app-page-cache.ts, app-route-handler-response.ts, pages-page-data.ts, dev-server.ts).

So what this module actually centralizes is the Cache-Control string, not the disposition decision. Consider tightening the prose to match the real (still valuable) scope, otherwise the next reader will assume disposition logic is owned here when it isn't. Either drop the unused disposition/scheduleRegeneration fields, or note them as informational-only for now.

ask-bonk · 2026-06-10T13:22:55Z

+ * caller's responsibility and must happen *before* this call. `hasUsableValue`
+ * must only be true when those guards have already passed.
+ */
+export function decideIsr(options: DecideIsrOptions): IsrDecision {


Minor: resolveRevalidate(options) is called identically in both the HIT and STALE branches. You could hoist it above the if (!options.isStale) to compute it once and remove the duplication (the MISS early-return already short-circuits before either branch). Not a correctness issue.

ask-bonk · 2026-06-10T13:23:04Z

Review submitted successfully. Here's my summary:

Review of PR #1883 — `refactor(isr): centralise HIT/STALE/MISS + Cache-Control decision`

Verdict: Solid refactor, no blocking issues.

I verified all the equivalence claims by tracing each migrated call site against cache-control.ts:

app-page, app-route, pages HIT/STALE/MISS paths: header output is byte-for-byte equivalent. The metadata-over-route-default precedence (cacheControlMeta?.revalidate ?? revalidateSeconds), the expireSeconds-only-when-meta-present rule, the 0→NEVER / Infinity→STATIC gates, and the pages ?? 60 legacy fallback are all preserved correctly.
dev STALE is the one deliberate change (s-maxage=<secs> → s-maxage=0, stale-while-revalidate). It's correct and brings dev into parity with prod. No existing test asserted the old value, so nothing breaks.

I ran the new tests/isr-decision.test.ts (33 pass) plus app-page-cache, app-route-handler-response, pages-page-data, and cache-control (87 pass) — all green.

Two non-blocking comments posted inline:

The module docstring overstates scope — it claims to own the disposition and scheduleRegeneration flag and that "no caller may produce these values independently," but in reality every caller computes isStale/disposition itself and only reads cacheControl. The disposition/scheduleRegeneration fields are dead. Suggested tightening the prose or dropping the unused fields.
resolveRevalidate(options) is duplicated across the HIT and STALE branches and could be hoisted.

github run

…alue/isStale Replace the two-field hasUsableValue+isStale decomposition with a single cacheState: "HIT" | "STALE" | "MISS" parameter that callers already have. Also remove the before/after equivalence table from the module docstring, which is only relevant during the migration window. All five call sites updated; tsc --noEmit passes with no errors.

james-elicx · 2026-06-10T14:26:32Z

/bigbonk re-review for issues

ask-bonk

Re-review (/bigbonk). I re-traced every migrated call site against cache-control.ts at the current HEAD (78c5a38) and ran the relevant suites — tests/isr-decision.test.ts (33), cache-control (11), app-page-cache (30), app-route-handler-response (20), pages-page-data (26): all green. vp check on the new module is clean (format + lint + types).

Prior review comments are now resolved by later commits:

072fe61 hoisted resolveRevalidate above the HIT/STALE branches (was ask-bonk's dedup nit) and tightened the docstring so disposition/scheduleRegeneration are honestly labelled informational.
a571a1b switched callers to pass cacheState directly (james-elicx's "infer from state" suggestion).

Equivalence verification (all confirmed byte-for-byte):

app-page HIT/STALE — cacheControlMeta?.revalidate ?? revalidateSeconds precedence and the expire-only-when-meta-present rule preserved; Infinity still routes through buildCachedRevalidateCacheControl's STATIC gate.
app-route cached + MISS — old buildRouteHandlerCacheControl("HIT", …) for the MISS path reduces to buildRevalidateCacheControl(r, e) for finite values, exactly what buildAppRouteMissIsrCacheControl emits; 0→NEVER / Infinity→STATIC gates carried over.
pages HIT/STALE — verified the ?? 60 hoist preserves precedence. Original (cacheControl?.revalidate ?? revalidateSeconds) ?? 60 vs new cacheControlMeta?.revalidate ?? (revalidateSeconds ?? 60) are equivalent for every input (the inner/outer ?? only fire on null/undefined, and 0 short-circuits identically in both).
dev nonce / gssp-default / MISS — identical to the previous literals.
dev STALE — the one deliberate change (s-maxage=<secs> → s-maxage=0, stale-while-revalidate). Confirmed no E2E or unit test asserts the old dev STALE header, so nothing breaks; this is a genuine dev/prod parity fix.

No blocking issues. One minor leftover noted inline.

ask-bonk · 2026-06-10T14:29:30Z

+ * an ISR response. Every ISR code path (app-page, app-route, pages,
+ * dev-server) routes through it.
+ *
+ * `disposition` and `scheduleRegeneration` are informational fields for


The docstring now honestly labels these as informational, which resolves the earlier concern. But disposition and scheduleRegeneration are still returned by decideIsr and read by no caller anywhere in the repo (verified with a repo-wide grep — only the test references .disposition/.scheduleRegeneration). Since YAGNI-style dead return surface tends to rot, consider either dropping them now and returning just { cacheControl }, or keeping them only if there's a concrete near-term consumer. Not blocking — the prose is accurate as-is.

ask-bonk · 2026-06-10T14:29:39Z

Re-review submitted successfully.

Summary

I re-reviewed PR #1883 at the current HEAD (78c5a38) and verified the refactor is correct with no blocking issues.

Key findings:

Prior review comments were already addressed by later commits. The PR had 6 commits; commits 072fe61 and a571a1b (landed after the earlier ask-bonk/james-elicx reviews) resolved both prior nits:
- resolveRevalidate is now hoisted above the HIT/STALE branches (dedup nit).
- The docstring no longer overstates scope — disposition/scheduleRegeneration are honestly labelled informational.
- Callers now pass cacheState directly (james-elicx's "infer from state" suggestion).
Equivalence verified independently by tracing each migrated call site against cache-control.ts. The one I scrutinized hardest was the pages ?? 60 precedence change — confirmed (cacheControl?.revalidate ?? revalidateSeconds) ?? 60 and the new cacheControlMeta?.revalidate ?? (revalidateSeconds ?? 60) are equivalent for all inputs (including the 0 short-circuit edge cases).
Tests pass — ran isr-decision (33), cache-control (11), app-page-cache (30), app-route-handler-response (20), pages-page-data (26), all green. vp check on the new module is clean.
The dev STALE behavior change is a genuine dev/prod parity fix with no test asserting the old header value.

One non-blocking inline comment: the disposition/scheduleRegeneration return fields are still dead code (no caller reads them). Suggested either dropping them or keeping only with a concrete consumer in mind.

github run

fix(isr): un-export IsrDisposition and IsrPolicyKind (knip unused-exp…

26c2a23

…ort)

ask-bonk Bot reviewed Jun 10, 2026

View reviewed changes

Divkix added 2 commits June 10, 2026 06:25

Merge branch 'main' into refactor/isr-decision-module

c6dba24

docs(isr): tighten isr-decision docstring and hoist resolveRevalidate

072fe61

james-elicx reviewed Jun 10, 2026

View reviewed changes

Comment thread packages/vinext/src/server/app-route-handler-response.ts Outdated

james-elicx reviewed Jun 10, 2026

View reviewed changes

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

james-elicx reviewed Jun 10, 2026

View reviewed changes

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

Divkix added 2 commits June 10, 2026 06:51

test(isr): update decideIsr tests for cacheState

78c5a38

ask-bonk Bot reviewed Jun 10, 2026

View reviewed changes

james-elicx merged commit 743f252 into cloudflare:main Jun 10, 2026
42 checks passed

Divkix deleted the refactor/isr-decision-module branch June 10, 2026 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(isr): centralise HIT/STALE/MISS + Cache-Control decision in isr-decision.ts#1883

refactor(isr): centralise HIT/STALE/MISS + Cache-Control decision in isr-decision.ts#1883
james-elicx merged 6 commits into
cloudflare:mainfrom
Divkix:refactor/isr-decision-module

Divkix commented Jun 9, 2026

Uh oh!

pkg-pr-new Bot commented Jun 9, 2026 •

edited

Loading

Uh oh!

james-elicx commented Jun 10, 2026

Uh oh!

ask-bonk Bot left a comment

Uh oh!

ask-bonk Bot Jun 10, 2026

Uh oh!

ask-bonk Bot Jun 10, 2026

Uh oh!

ask-bonk Bot commented Jun 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

james-elicx commented Jun 10, 2026

Uh oh!

ask-bonk Bot left a comment

Uh oh!

ask-bonk Bot Jun 10, 2026

Uh oh!

ask-bonk Bot commented Jun 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Divkix commented Jun 9, 2026

What

Call sites migrated

Deliberate behaviour change

Equivalence

Tests

Uh oh!

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

Uh oh!

Uh oh!

james-elicx commented Jun 10, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Jun 10, 2026

Review of PR #1883 — refactor(isr): centralise HIT/STALE/MISS + Cache-Control decision

Uh oh!

Uh oh!

Uh oh!

Uh oh!

james-elicx commented Jun 10, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Jun 10, 2026

Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pkg-pr-new Bot commented Jun 9, 2026 •

edited

Loading

Review of PR #1883 — `refactor(isr): centralise HIT/STALE/MISS + Cache-Control decision`