fix(app-router): keep the previous entry when an App page regeneration fails - #3459
james-elicx wants to merge 14 commits into
Conversation
|
/bigbonk review for issues Please review exact head |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Focused tests and checks passed. Worktree remained unchanged. |
2c2b205 to
e7b8d3e
Compare
|
Re the medium finding in #3459 (comment): fixed in e7b8d3e. The whole regeneration, including cache-control resolution, key creation and both fresh |
|
/bigbonk review for issues Please review exact head |
Focused tests passed: 174 tests. |
e7b8d3e to
df42196
Compare
|
Re the medium finding in the last review ( |
|
/bigbonk review for issues Please review exact head |
|
Targeted tests passed: 181 tests. |
df42196 to
01f73c5
Compare
|
Re the medium finding in the last review (dynamic API use failing PPR regenerations): fixed in 01f73c5. As in Next.js ( |
|
/bigbonk review for issues Please review exact head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| const previousTags = cachedValue.renderObservation?.cacheTags; | ||
| if (previousCacheControl && previousTags) { | ||
| try { | ||
| await options.isrSet(isrKey, cachedValue, { |
There was a problem hiding this comment.
Medium: This unconditional restore can overwrite a newer value from a concurrent regeneration. HTML and RSC requests are deduped under different keys, but an HTML regeneration also writes the RSC key. If that write succeeds while an RSC-triggered regeneration later fails, this closure writes its captured stale RSC value back over the fresh one and marks it fresh for the backoff window. Guard the restore with the original entry version/lastModified (or otherwise coordinate the cross-key writes) so it only replaces the entry that actually triggered this regeneration.
There was a problem hiding this comment.
Fixed in d8f4538. The failure handler re-reads the key and skips the restore if another write replaced the entry it found. A missing entry is still restored, as Next.js restores unconditionally.
|
Posted 1 actionable inline finding. Focused checks passed: 184 tests plus formatting, lint, and types. Worktree unchanged. |
01f73c5 to
d8f4538
Compare
|
/bigbonk review for issues Please review exact head |
| // one is restored, as Next.js restores unconditionally. | ||
| const current = await options.isrGet(isrKey); | ||
| if (!current || current.value.lastModified === cached.value.lastModified) { | ||
| await options.isrSet(isrKey, cachedValue, { |
There was a problem hiding this comment.
Medium: The version check is still racy because isrGet() and isrSet() are separate operations. An HTML regeneration can write a fresh RSC value after line 574 returns the captured stale version but before this restore runs, and this isrSet() then overwrites that fresh value with the stale payload. The new test only places the newer write before the read, so it cannot exercise this window. Serialize all writers/restores for the shared RSC key, or use an atomic compare-and-set in the cache backend.
There was a problem hiding this comment.
Declining this one. The memory and KV backends have no compare-and-set, and a per-process lock would be isolate-local state, which vinext avoids for cache decisions (it wouldn't serialize across isolates anyway). Next.js restores unconditionally here, so it has a wider version of this window. The re-read narrows the window to one get→set round trip. If a fresh write does land inside it, the key holds the previous payload for at most the 3–30 s failure backoff, and then the next request regenerates it. The payload was already being served as stale, so nothing is lost for longer than that.
|
Posted 1 actionable inline finding. Focused tests passed: 188 tests. Worktree unchanged. |
d8f4538 to
41fe04f
Compare
|
/bigbonk review for issues Please review exact head |
| // writes the RSC key), so don't replace a newer entry. A missing | ||
| // one is restored, as Next.js restores unconditionally. | ||
| const current = await options.isrGet(isrKey); | ||
| if (!current || current.value.lastModified === cached.value.lastModified) { |
There was a problem hiding this comment.
Medium: This check also mistakes this regeneration’s own successful HTML write for an unrelated newer value. On an HTML-triggered regeneration, the RSC and HTML writes run concurrently; if the RSC write rejects but the HTML write succeeds, Promise.allSettled enters this handler and isrGet(isrKey) returns that just-written HTML with a different lastModified. The restore is therefore skipped, leaving half of a failed regeneration published and the HTML key without failure backoff. Either avoid committing the triggering-key write until its sibling succeeds, or track that this regeneration wrote the current value and restore it while retaining the external-writer guard.
There was a problem hiding this comment.
Fixed in 3b7bbe5: the regeneration now writes the RSC key first, and writes the triggering HTML key only once that succeeds. A failed RSC write leaves nothing half-published, and the restore guard only sees another regeneration's writes. There's a test for an RSC write that rejects.
821668c to
726daa1
Compare
726daa1 to
07f8a14
Compare
07f8a14 to
ca5de55
Compare
…n fails Next.js re-stores the previous entry when a regeneration throws or turns dynamic, with revalidate clamped to 3-30 s and expire raised to at least revalidate + 3 s, so a failing page keeps serving stale content without retrying on every request (server/response-cache/index.ts). - The regeneration render now reports dynamic API use instead of discarding it. A dynamic regeneration fails with Next.js's "Page changed from static to dynamic at runtime" error, which is logged and reported like any other regeneration failure. - The previous entry is re-stored under its own key only, with the tags from its render observation, so an RSC-triggered failure never writes under the HTML key. - revalidate = false retries after 3 s, like Next.js's revalidate || 3.
- An entry without a render observation isn't re-stored: its tags can't be recovered, and re-storing it with path tags only would drop the fetch tags it was written with. - A failing re-store is logged, and the regeneration's own error is still the one reported. - Test a throwing regeneration against a real cache handler, and pin that a force-static regeneration reading headers() and searchParams still stores.
…pp page fails Next.js wraps the whole regeneration, the new entry's store included, in the handler that keeps the previous entry. A failing cache write now gets the same backoff as a failing render.
…he previous entry A failed write rejected the regeneration while a slower sibling write was still in flight, so that write could land over the re-stored previous entry.
Next.js throws the static-to-dynamic error only when the route has no PPR (`!isRoutePPREnabled`); a PPR shell expects dynamic holes, so its regeneration stores as before.
A client page reads searchParams in SSR's child scope, which reaches the render's dynamic latch but not its own flag, so the regeneration was stored.
… RSC write succeeds
…n's HTML write fails Next.js keeps a page's HTML and RSC as one entry, so a failed regeneration never leaves half of it published. An HTML-triggered regeneration writes two keys; it now holds the RSC key's previous entry and puts it back if the HTML write fails, unless another writer has replaced it since. Its own write is recognized by its payload bytes, since a store may hand back a copy.
…regeneration Next.js's IncrementalCache.set only warns when its cache handler fails, so a store failure never reaches handleRevalidate's backoff and the previous page entry stays as it was. A regeneration's store failure now warns the same way without re-storing anything under a backoff policy: a failed RSC write skips the HTML write, and a failed HTML write puts the RSC key's previous entry back with its own policy. A failed render still keeps the previous entry with the backoff policy.
… fails A regeneration's store failure only warns, as Next.js's IncrementalCache.set does, so it no longer holds a backoff window over the page. Fresh RSC beside stale HTML is then exactly the state an RSC-triggered regeneration leaves in the two-key design, and the stale HTML regenerates on its next request. The RSC key's previous entry is no longer read and put back after a failed HTML write: that put-back could skip a missing entry, roll back a concurrent regeneration's byte-identical write, and reset the restored entry's lastModified so the old RSC counted as fresh again. The RSC key is still written first, and a failed RSC write still skips the HTML write. A failed render still keeps the previous entry with a backoff policy.
…te is 0 Next.js throws "Page changed from static to dynamic at runtime" for any non-PPR regeneration whose render's cacheControl.revalidate is 0, not only one that used a dynamic API (build/templates/app-page.ts, from applyMetadataFromPrerenderResult's collectedRevalidate). A stale intercepted entry whose tree turned revalidate = 0 or force-dynamic, rendering no request API, was stored at revalidate 0, or at its cacheLife's. Carry the regenerated tree's route-level revalidate as null when it has none, so a real revalidate = 0 is no longer read as the cacheLife discovery seed, and fail the regeneration when the revalidate it would store is 0. The PPR exemption moves into the cache module with the check, so both follow Next.js's `!isRoutePPREnabled`.
ca5de55 to
3e43e43
Compare
|
/bigbonk review for issues Please review exact head |
Focused tests passed: 187 tests. Worktree unchanged. |
…e route's revalidate
|
Fixed the Medium finding ( |
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
1 participant
Stacked on #3456. First part of plan PR 3 (core parity fixes): regeneration failures.
Problem
When a stale App page regeneration threw or turned out to use a dynamic API, core logged the error and kept the old entry exactly as it was. The next stale hit retried at once, with no backoff. A regeneration that read a dynamic API wasn't reported at all.
Next.js
server/response-cache/index.tscatches every regeneration error. If the previous entry has cache control, it re-stores that entry and rethrows:revalidate = Math.min(Math.max(prev.revalidate || 3, 3), 30);expire = Math.max(revalidate + 3, prev.expire).A static page that uses a dynamic API during a revalidation throws "Page changed from static to dynamic at runtime", which
onRequestErrorreports.Change
renderAppPageCacheRendernow returnsusedDynamicApiinstead of discarding the signal.Page changed from static to dynamic at runtime <path>, plus the docs link).revalidate, the raisedexpire, and its own tags. An entry stored asrevalidate = falseretries after 3 s, as in Next.js. Then the error is rethrown to the existing background handler.headers()orsearchParamsare still stored, as before.Tests
tests/app-page-cache.test.ts:revalidateandrevalidate = false).tests/app-page-dispatch.test.ts:searchParamsrejects and re-stores the previous entry;headers()andsearchParamsstill stores.tests/app-page-cache-render.test.ts:usedDynamicApiis reported.Not matched
, reason: …suffix. vinext doesn't track a bailout description.