fix(app-router): align app static ISR lifecycle

[james-elicx]

Summary

• skip App Router prerender artifacts when concrete static params render no-store output
• preserve route-level ISR policy during stale App page regeneration and foreground-refresh stale fetches during prerender regeneration
• cache force-static lazy App pages in production and keep edge stale App page responses non-blocking

Validation

• vp test run tests/app-page-cache.test.ts tests/app-page-response.test.ts tests/fetch-cache.test.ts tests/shims.test.ts -t "preserves route-level revalidate|writes force-static|foreground fetch refresh|unstable_noStore does not opt out"
• vp test run tests/app-router-production-server.test.ts -t "revalidates static App pages|does not cache generated static params|caches lazy dynamic params|serves stale edge App page" --maxWorkers=1
• vp check --fix
• REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" NEXT_TEST_CONCURRENCY=1 ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/app-static/app-static.test.ts (passes target residual rows; remaining failures are the excluded useSearchParams Suspense server-response row and updateTag server-action row)

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

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

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

commit: a8edae3

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: fix(app-router): align app static ISR lifecycle

I reviewed all 17 changed files, ran the targeted unit + integration suites (app-page-cache, app-page-response, fetch-cache, shims → 1361 passed; app-router-production-server targeted → 4 passed, 63 skipped), and ran vp check on every changed source file (format + lint + types all clean).

The four behavioral changes are cohesive and correct:

1. prerender.ts — skip no-store concrete static params. Dropping the isSpeculative guard so htmlCacheControl.includes("no-store") skips even concrete generateStaticParams paths is right: a generated param decides admissible URLs, not that the render output is reusable. Well-covered by the new app-static-gen-params-dynamic/[slug] fixture (next: { revalidate: 0 }).

2. Route-level ISR preservation during stale regeneration. resolveRegeneratedAppPageCachePolicy min-merges the route revalidate with the render-discovered revalidate instead of letting the render replace it (?? options.revalidateSeconds → Math.min). The > 0 guard correctly falls back to the render value for the 0/Infinity seed cases. Matches Next.js "effective page revalidate = min of route config and all fetch revalidates."

3. Fetch revalidate propagation + foreground refresh. recordFiniteFetchRevalidate → _setRequestScopedCacheLife({revalidate}) correctly makes a fetch({ next: { revalidate }}) feed the page's revalidate via the existing Math.min merge, matching Next.js. Foreground stale refresh (gated on VINEXT_PRERENDER === "1") uses dedupeFetch (consistent with the miss path and correct for the render dedupe scope), while the background path keeps originalFetch (correct — background refetch must not join the torn-down render dedupe map). The refactor into buildFetchCacheValue / writeFetchCacheResponse removes real duplication.

4. Force-static production caching + edge stale parity. resolveAppPageHtmlResponsePolicy now writes force-static/dynamic-error pages to cache in production (MISS + shouldWriteToCache: true), consistent with the adjacent revalidate === Infinity branch. The edge special-casing in readRequestCacheLifeForPrerender / readRequestCacheLifeForCachePolicy plus !options.isEdgeRuntime on the speculative-write guard keeps edge stale App pages non-blocking (verified by the serves stale edge App page test: <500ms, count > 0).

unstable_noStore inside unstable_cache is now a no-op via isInsideUnstableCacheScope() (backed by _unstableCacheAls.run(true, ...)), matching Next.js. addCollectedRequestTags(tags) in unstable_cache receives already-encoded tags (consistent with currentRequestTags) and correctly bubbles tags on both hit and miss.

Non-blocking observations

• lowerFetchCacheRevalidateIfNeeded awaits a blocking handler.set on the cache-hit read path. Previously a hit was pure-read. It only fires in the narrow case where a shorter-revalidate fetch hits a longer-revalidate entry, so impact is bounded, but the write does not need to block the returned response body — it could be fire-and-forget (via waitUntil) to keep the hit path latency unchanged. Worth a follow-up if fetch-cache hit latency is sensitive.

• Foreground stale refresh drops good stale data on a non-200 fresh response. In the foreground path, a non-200 fresh fetch returns the error response and skips the cache write, discarding the previously-good stale entry; the background path instead keeps the stale entry and returns stale data. This only runs during prerender, where surfacing the fresh error is arguably correct, so it's acceptable — just noting the intentional divergence.

Both are minor. No correctness blockers found; the change is well-tested and aligns with Next.js behavior.

[ask-bonk]

Review posted to PR #2472.

Summary

I reviewed all 17 changed files, ran the targeted test suites (1361 unit tests + 4 integration tests passing, 63 skipped), and confirmed vp check is clean on every changed source file.

Verdict: No correctness blockers. The PR is cohesive, well-tested, and aligns with Next.js behavior. The four changes work together correctly:

1. prerender skip for no-store concrete static params — dropping the isSpeculative guard is correct; a generated param decides admissible URLs, not reusability.
2. route-level ISR preservation — resolveRegeneratedAppPageCachePolicy min-merges instead of replacing route revalidate, matching Next.js.
3. fetch revalidate propagation + foreground refresh — correctly feeds page revalidate; dedupeFetch (foreground/prerender) vs originalFetch (background) distinction is sound.
4. force-static prod caching + edge stale parity — consistent with the adjacent Infinity branch and verified non-blocking on edge.

Also verified unstable_noStore inside unstable_cache correctly no-ops via the ALS scope, and tag collection uses correctly-encoded tags.

Two non-blocking observations flagged for potential follow-up:

• lowerFetchCacheRevalidateIfNeeded adds a blocking handler.set on the cache-hit path (could be fire-and-forget via waitUntil).
• Foreground stale refresh drops good stale data on a non-200 fresh response (only runs during prerender, so acceptable/intentional).

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: fix(app-router): align app static ISR lifecycle

I reviewed all 18 changed files across both commits (26282f0 app static ISR, 559cea4 honor shorter revalidate on hits) and ran the targeted fetch-cache suite (125 passed). The four App Router ISR changes are cohesive and correct, and I agree with the prior review's assessment of them. I'm flagging one issue the prior review missed.

Dev/prod parity gap: shorter-revalidate-on-hit is only implemented in MemoryCacheHandler

Commit 559cea4 teaches the data cache get() to treat an entry as stale when the current fetch requests a shorter revalidate than what the entry was stored with. This is implemented in MemoryCacheHandler.get() (packages/vinext/src/shims/cache-handler.ts:253-259) via the new readPositiveNumberField(ctx, "revalidate") check:

const requestedRevalidate = readPositiveNumberField(ctx, "revalidate");
const requestedRevalidateAt =
  requestedRevalidate === undefined ? null : entry.lastModified + requestedRevalidate * 1000;
const isStale =
  (entry.revalidateAt !== null && now > entry.revalidateAt) ||
  (requestedRevalidateAt !== null && now > requestedRevalidateAt);

The KV data adapter's get() (packages/cloudflare/src/cache/kv-data-adapter.runtime.ts:203,253) does not apply this rule. It names its ctx _ctx (unused) and only checks entry.revalidateAt:

async get(key: string, _ctx?: Record<string, unknown>): Promise<CacheHandlerValue | null> {
  ...
  if (entry.revalidateAt !== null && Date.now() > entry.revalidateAt) {
    return { ..., cacheState: "stale", ... };
  }

Concrete divergence for a user who opts into kvDataAdapter as their data cache: an entry stored with fetch({ next: { revalidate: 3600 } }), then re-fetched 20s later with fetch({ next: { revalidate: 10 } }):

• Memory (dev + default): requestedRevalidateAt = lastModified + 10s has passed → returns stale → foreground/background refresh serves fresh data. Correct.
• KV (prod opt-in): entry.revalidateAt (3600s out) has not passed → returns a fresh hit of the 20s-old body, then lowerFetchCacheRevalidateIfNeeded rewrites the entry to revalidate: 10 but still returns the stale-by-the-new-policy data. The shorter revalidate is silently ignored on that read.

This only affects users who explicitly configure kvDataAdapter (the default is MemoryCacheHandler in all runtimes, so most deployments are unaffected), which is why it's not a blocker. But AGENTS.md treats dev/prod parity as a hard requirement ("Always check dev and prod server parity"), and this is a behavior that now differs between the two data cache backends. Worth mirroring the same requestedRevalidateAt staleness check into kv-data-adapter.runtime.ts (and adding a KV-adapter test), either here or as a tracked follow-up.

Everything else looks correct

• prerender.ts — dropping the isSpeculative guard so no-store skips concrete generateStaticParams paths is right; covered by the new app-static-gen-params-dynamic/[slug] fixture (next: { revalidate: 0 }).
• resolveRegeneratedAppPageCachePolicy — the > 0 ? Math.min(...) : renderRevalidate merge correctly preserves route-level ISR during stale regeneration and handles the 0/Infinity seed cases.
• recordFiniteFetchRevalidate → _setRequestScopedCacheLife — correctly feeds a fetch revalidate into the page revalidate via the existing applyRequestCacheLife Math.min merge.
• Foreground vs background stale refresh — dedupeFetch (foreground/prerender) vs originalFetch (background) is the right distinction; the buildFetchCacheValue/writeFetchCacheResponse refactor removes real duplication, and the cloneForReturn: false background path correctly avoids the returned-clone. Verified clone() is not called on the background path by the new test.
• Force-static prod caching + edge stale parity — resolveAppPageHtmlResponsePolicy writing force-static/dynamic-error pages in production (MISS/shouldWriteToCache: true) is consistent with the adjacent Infinity branch; the !options.isEdgeRuntime guard on the speculative write plus the edge { revalidate: 0 } special-casing keeps edge stale App pages non-blocking.
• unstable_noStore inside unstable_cache — correctly no-ops via isInsideUnstableCacheScope() (_unstableCacheAls), matching Next.js. addCollectedRequestTags(tags) receives already-encodeCacheTags-encoded tags (cache.ts:587), consistent with currentRequestTags.

Also worth noting (already raised by prior review, still open)

• lowerFetchCacheRevalidateIfNeeded performs a blocking handler.set on the cache-hit read path. Bounded to the shorter-revalidate case, but could be fire-and-forget via waitUntil to keep hit latency unchanged.

[ask-bonk]

Review posted to PR #2472.

Summary

I reviewed both commits (26282f0 App static ISR, 559cea4 honor shorter revalidate on hits), all 18 changed files, and ran the fetch-cache suite (125 passed).

One issue the prior review missed — a dev/prod parity gap:

Commit 559cea4's "honor shorter revalidate on hits" is implemented only in MemoryCacheHandler.get() (cache-handler.ts:253-259), not in the Cloudflare kvDataAdapter (kv-data-adapter.runtime.ts:203,253, whose ctx is named _ctx and ignored). A user who opts into KV as their data cache will see a fetch({ next: { revalidate: 10 } }) hitting an entry stored with revalidate: 3600 returned as a fresh hit instead of stale — the shorter revalidate is silently ignored. It's not a blocker (default is MemoryCacheHandler in all runtimes, KV is opt-in), but AGENTS.md treats dev/prod parity as a hard requirement, so it's worth mirroring the staleness check into the KV adapter here or as a tracked follow-up.

Everything else is correct: the prerender.ts no-store skip, resolveRegeneratedAppPageCachePolicy min-merge, recordFiniteFetchRevalidate propagation, foreground/background stale-refresh split (dedupeFetch vs originalFetch), force-static prod caching + edge stale parity, and unstable_noStore-inside-unstable_cache no-op all check out and align with Next.js behavior. I also re-flagged the prior review's open note that lowerFetchCacheRevalidateIfNeeded blocks on handler.set on the cache-hit path.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: fix(app-router): align app static ISR lifecycle

I reviewed all 20 changed files across three commits (26282f0 app static ISR, 559cea4 honor shorter revalidate on hits, ccf02c8 honor read-time revalidate in KV cache), ran the targeted unit suites (fetch-cache, shims, kv-cache-handler, app-page-cache, app-page-response → 1428 passed), the targeted production-server integration rows (revalidates static App pages, does not cache generated static params, caches lazy dynamic params, serves stale edge App page → 4 passed, 63 skipped), and vp check on every changed source file (format + lint + types clean).

The prior review's parity gap is now fixed

Commit ccf02c8 mirrors the "honor shorter read-time revalidate" staleness check into KVCacheHandler.get() (packages/cloudflare/src/cache/kv-data-adapter.runtime.ts:260-276), matching the MemoryCacheHandler.get() implementation (packages/vinext/src/shims/cache-handler.ts:250-262). Both now compute requestedRevalidateAt = entry.lastModified + requestedRevalidate * 1000 and OR it into the staleness decision, and a KV-adapter test (tests/kv-cache-handler.test.ts:472, "serves stale when a shorter read-time revalidate has elapsed") locks in the behavior. Dev/prod parity is restored — the divergence the previous review flagged is resolved in this PR rather than deferred.

The four App Router ISR changes are cohesive and correct

1. prerender.ts no-store skip. Dropping the isSpeculative guard so cacheControl.includes("no-store") skips even concrete generateStaticParams paths is right: a generated param decides admissible URLs, not that the render output is reusable. Covered by the new app-static-gen-params-dynamic/[slug] fixture (fetch(..., { next: { revalidate: 0 } })).

2. Route-level ISR preservation. resolveRegeneratedAppPageCachePolicy (app-page-cache.ts:202) min-merges routeRevalidateSeconds with the render-discovered revalidate (> 0 ? Math.min(...) : renderRevalidate), preserving route config during stale regeneration instead of letting the render replace it. The > 0 guard correctly handles the 0/Infinity seeds. Verified against the revalidate = 1 route + fetch({ next: { revalidate: 2 } }) fixture → effective min(1, 2) = 1.

3. Fetch revalidate propagation + foreground refresh. recordFiniteFetchRevalidate → _setRequestScopedCacheLife({ revalidate }) feeds a fetch revalidate into the page revalidate via the existing Math.min merge. force-cache/revalidate:false fold to ONE_YEAR_SECONDS (finite), so they can only ever be a harmless no-op through Math.min. Foreground stale refresh (gated on VINEXT_PRERENDER === "1") uses dedupeFetch and clones for the cache write (cloneForReturn defaults on); the background path keeps originalFetch with cloneForReturn: false (correct — background refetch must not join the torn-down render dedupe map, and its response isn't returned). The buildFetchCacheValue/writeFetchCacheResponse refactor removes the previous duplication.

4. Force-static prod caching + edge stale parity. resolveAppPageHtmlResponsePolicy now writes force-static/dynamic-error pages in production (MISS + shouldWriteToCache: true), consistent with the adjacent revalidate === Infinity branch. resolveAppPageCacheWriteRevalidateSeconds/resolveAppPageCacheReadRevalidateSeconds seed Infinity for those cases (immutable until tag invalidation — correct). The !options.isEdgeRuntime guard on the speculative write plus the edge { revalidate: 0 } special-casing in readRequestCacheLifeForPrerender/readRequestCacheLifeForCachePolicy keeps edge stale App pages non-blocking (verified by serves stale edge App page < 500ms).

unstable_noStore inside unstable_cache correctly no-ops via isInsideUnstableCacheScope() (_unstableCacheAls), matching Next.js, and the new shims.test.ts case confirms dynamicUsageDetected stays false. addCollectedRequestTags(tags) in unstable_cache receives already-encodeCacheTags-encoded tags (cache.ts:587) and dedups — consistent with currentRequestTags.

Non-blocking observation (carried over, still open)

• lowerFetchCacheRevalidateIfNeeded performs a blocking handler.set on the cache-hit read path (fetch-cache.ts:1174). It only fires in the narrow shorter-revalidate case, so impact is bounded, but the write doesn't need to block the returned response body — it could be fire-and-forget via waitUntil to keep hit latency unchanged. Worth a follow-up if fetch-cache hit latency is sensitive.

Verdict: no correctness blockers. The PR is well-tested, aligns with Next.js behavior, and — notably — closes the dev/prod parity gap the prior review raised rather than punting it. LGTM.

[ask-bonk]

Review posted to PR #2472.

Summary

I reviewed all 20 changed files across the three commits (26282f0, 559cea4, ccf02c8), ran the targeted unit suites (1428 passed) and the production-server integration rows (4 passed, 63 skipped), and confirmed vp check is clean on every changed source file.

Verdict: no correctness blockers. The PR is cohesive, well-tested, and aligns with Next.js behavior.

Key findings:

• The prior review's dev/prod parity gap is now fixed. Commit ccf02c8 mirrors the "honor shorter read-time revalidate" staleness check into KVCacheHandler.get() to match MemoryCacheHandler.get(), with a new KV-adapter test locking it in. The divergence flagged in earlier rounds is resolved in this PR rather than deferred.

• The four App Router ISR changes are correct: the prerender.ts no-store skip for concrete generateStaticParams paths, resolveRegeneratedAppPageCachePolicy min-merge preserving route-level ISR, recordFiniteFetchRevalidate propagation + foreground/background stale-refresh split (dedupeFetch+clone vs originalFetch+cloneForReturn: false), and force-static prod caching + edge stale parity.

• unstable_noStore-inside-unstable_cache correctly no-ops via the ALS scope, and tag collection uses correctly-encoded, deduped tags.

• One non-blocking observation carried over: lowerFetchCacheRevalidateIfNeeded blocks on handler.set on the cache-hit path — could be waitUntil fire-and-forget as a follow-up.

github run

[james-elicx]

Deploy-suite run 28478866791 / job 84413308650 triage: this PR appears to cover these test/e2e/app-dir/app-static/app-static.test.ts residual static/ISR failures:

• app-dir static/dynamic handling > should revalidate correctly with config and fetch revalidate
• app-dir static/dynamic handling > should stream properly for /stale-cache-serving-edge/app-page
• app-dir static/dynamic handling > should not error with generateStaticParams and dynamic data
• app-dir static/dynamic handling > should honor dynamic = "force-static" correctly (lazy)

[github-actions]

Performance benchmarks

Compared a8edae3 against base 239595e using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	125.6 KB	125.6 KB	⚫ +0.0%
Client entry size (gzip)	vinext	119.8 KB	119.8 KB	⚫ +0.0%
Dev server cold start	vinext	2.60 s	2.59 s	⚫ -0.4%
Production build time	vinext	3.20 s	3.20 s	⚫ -0.0%
RSC entry closure size (gzip)	vinext	96.8 KB	97.4 KB	⚫ +0.6%
Server bundle size (gzip)	vinext	163.2 KB	163.8 KB	⚫ +0.4%

View detailed results and traces

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