fix(cache): persist prerender resume data - #2849
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(cache): persist prerender resume data
I reviewed the full diff (19 files) and ran the affected targeted suites locally. All pass:
tests/prerender-data-cache.test.ts+tests/kv-cache-handler.test.ts— 90 passedtests/shims.test.ts+tests/prerender-server-pool.test.ts+tests/prerender-kv-populate.test.ts— 1301 passedtests/fetch-cache.test.ts+tests/isr-cache.test.ts— 211 passedvp checkon the three core changed files — clean (format, lint, types)
Assessment
The change is well-structured and matches the layering guidance in AGENTS.md: the cross-process build cache lives in a typed server/prerender-data-cache.ts module rather than in a generated entry, and the generated prerender-server-entry.ts stays thin (just picks the handler based on VINEXT_PRERENDER_DATA_CACHE_DIR). Path wiring is consistent end-to-end — outDir in prerenderApp (dist/server/prerendered-routes) equals path.join(serverDir, "prerendered-routes") used by both seed-cache.ts and prerender-kv-populate.ts, so build-time writes and runtime/KV reads target the same directory.
Things I specifically verified and consider correct:
- Single-flight coordination (
.pendinglock) and write serialization (.write-lock) useopenSync(..., "wx")create-exclusive semantics with stale-lock recovery via mtime +PENDING_ENTRY_TIMEOUT_MS. Crash paths are covered:refreshAndStore/unstable_cache/fetch-cacheall callreleasePendingSetin their error branches, andsetreleases infinally. - Timestamp monotonicity — both
MemoryCacheHandlerandKVCacheHandlernow use_nextTimestamp()/_currentTimestamp()so a same-millisecondsetafterrevalidateTagis correctly treated as newer (tested). - Tag revalidation model — the shift from a bare timestamp to
{ expiredAt, staleAt, expireAt }is applied consistently across the memory and KV handlers, with_parseTagRevalidationretaining backward compatibility for legacy bare-timestamp markers (including theNaN"always expired" case). - SWR behavior change in
registerCachedFunction: the hit gate moved fromcacheState !== "stale"tocacheState !== "expired", so staleuse cacheentries are now served at runtime with a backgroundwaitUntilrevalidation, whileVINEXT_PRERENDER=1forces a foreground refresh so static generation never bakes a stale value. This matches Next.js's "stale entries are foreground misses while prerendering; SWR is a runtime-only optimization" semantics and is covered by the new shims tests. revalidate: falseround-trip through the serializing KV adapter no longer sets a TTL and no longer coerces the claim — matchingunstable_cache"cache indefinitely" semantics (tested).
Notes (non-blocking)
-
KV
revalidateTagread-modify-write is not atomic.revalidateTagnow doeskv.get(tagKey)thenkv.put(tagKey, ...)to mergestaleAt/expiredAtonto an existing marker. Cloudflare KV has no CAS, so two concurrent invalidations of the same tag (e.g. a hardrevalidateTag(tag)racing a profiledrevalidateTag(tag, { expire })) can lose one side's field. The in-isolate_lastTimestampmonotonicity that protects the memory handler does not hold across isolates. This is an inherent KV limitation rather than a regression, but worth a code comment near the merge so it isn't mistaken for a strong guarantee. -
Read-path re-persist write amplification during prerender. In
PrerenderDataCacheHandler.get, every memory-hit for a tagged FETCH re-serializes and re-writes the entry file (under the write-lock) to union newly-requested tags. Correct, but on large apps with many shared cached fetches this multiplies build-time file writes. If prerender build time regresses, this is the first place to look — a dirty check (skip the write when the persisted tag set already covers the requested tags) would cut it. -
Consider a brief comment on why
PrerenderDataCacheHandler.setcallsthis.memory.get(key)(no ctx) before persisting — a reader might expect the just-set value and not realize thegetcould return null if a tag was revalidated in the same worker.
No correctness blockers found. The concurrency model, error/lock-release paths, and dev/build/runtime/KV parity all look sound and are backed by targeted tests.
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Review posted to PR #2849. SummaryI reviewed all 19 changed files in PR #2849 ( No correctness blockers found. The change is well-architected:
Non-blocking notes I raised:
|
777407d to
3357bd2
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): persist prerender resume data
Reviewed the full 20-file diff and re-ran the affected targeted suites locally — all green:
tests/prerender-data-cache.test.ts+tests/kv-cache-handler.test.ts— 91 passedtests/shims.test.ts+tests/prerender-server-pool.test.ts+tests/prerender-kv-populate.test.ts— 1301 passedtests/fetch-cache.test.ts+tests/isr-cache.test.ts— 211 passedvp checkon the four core changed files — clean
No correctness blockers found. The layering follows AGENTS.md (cross-process build cache lives in a typed server/prerender-data-cache.ts, generated prerender-server-entry.ts stays thin), path wiring is consistent end-to-end (outDir = serverDir/prerendered-routes across build write, runtime seed, and KV populate), and all error/crash paths release the single-flight .pending and serialization .write-lock locks with openSync(..., "wx") create-exclusive semantics + stale-lock recovery.
Things I specifically verified
- Hard vs profiled tag races on KV are now separated by keyspace. The prior review flagged
revalidateTagas a non-atomic read-modify-write that could lose a hard-vs-profiled race. In this diff, hard invalidations write__tag:and profiled ones write__tag_profile:(separate keys, both read in_getTagRevalidation), so a hardrevalidateTag(tag)and a profiledrevalidateTag(tag, { expire })no longer clobber each other. Only two concurrent profiled invalidations of the same tag still race, which is a genuinely inherent KV limitation. Covered by the "does not weaken a hard invalidation with a later profiled revalidation" test. - Memory size accounting stays consistent through the new read-path tag merge.
MemoryCacheHandler.getnow unions requested tags onto a stored FETCH entry (growingcurrentMemoryCacheSize) before the tag-expiry delete check. This is safe becausedeleteEntryrecomputesestimateEntrySize(existing)from the already-mutated entry, so the grown size added on the merge is exactly the size subtracted on delete. Same for the softTag-null and stale/expired return paths where the entry is retained. - Timestamp monotonicity — both handlers use
nextTimestamp()/currentTimestamp(), so a same-millisecondsetafterrevalidateTagis correctly treated as newer (tested in both memory and KV). revalidate: falseround-trips through the KV serializer without a TTL and without being coerced, matchingunstable_cache"cache indefinitely".validateCacheEntrynow acceptscacheControl.revalidate === false, andbuildPrerenderKVPairsonly setsexpiration_ttlfor numeric revalidate.- SWR behavior change in
registerCachedFunction: the hit gate moved from!== "stale"to!== "expired", so staleuse cacheentries are served at runtime with awaitUntilbackground revalidation, whileVINEXT_PRERENDER=1forces a foreground refresh so static generation never bakes a stale value. The build-time foreground path means the backgroundrefreshAndStore(false)never runs against thePrerenderDataCacheHandler, so there's no interaction betweenskipPropagationand the build cache. requestMemoKey— the per-registration(): void => {}closure used as aWeakMapkey intorequestCacheis correct: it's stable across calls of the same cached function within a request and gives each function its own resume map.
Notes (non-blocking)
PrerenderDataCacheHandler.getre-persists on every tagged FETCH memory hit. Lines 157-164 and 195-202 re-serialize + re-write the entry file (under.write-lock) to union newly-requested tags on every hit, not just when the persisted tag set actually changed. On large apps with many shared cached fetches this multiplies build-time file writes. A dirty check (skip the write when the persisted tags already cover the requested tags) would cut it — first place to look if prerender build time regresses.PrerenderDataCacheHandler.setcallsthis.memory.get(key)with no ctx before persisting (line 249). Worth a one-line comment: the re-read can returnnullif the just-setentry was tag-revalidated in the same worker, in which case nothing is persisted — which is intentional but surprising to a reader who expects the value just written.- Two concurrent profiled
revalidateTag(tag, { expire })on KV can still lose a field (no CAS on__tag_profile:). Inherent KV limitation now that the hard/profiled keyspaces are split; a short comment near the__tag_profile:merge would prevent it being mistaken for a strong guarantee. - Background revalidation floats when not inside a unified scope (
scheduleUseCacheBackgroundRevalidationonly registerswaitUntilunderisInsideUnifiedScope()). The promise has a.catch, so no unhandled rejection, and it's acceptable SWR degradation — noting only for awareness.
The concurrency model, lock-release paths, dev/build/runtime/KV parity, and timestamp/tag semantics all look sound and are backed by targeted tests.
|
Review posted to PR #2849. SummaryI reviewed all 20 changed files in PR #2849 ( No correctness blockers found. Key verifications:
Non-blocking notes raised:
|
|
Mapping note for the cache-components backlog from Actions run |
…-cache # Conflicts: # packages/vinext/src/build/prerender.ts # tests/prerender-server-pool.test.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): persist prerender resume data
Reviewed the full 22-file diff and re-ran the affected suites locally — all green:
tests/prerender-data-cache.test.ts+tests/kv-cache-handler.test.ts— 103 passedtests/shims.test.ts+tests/fetch-cache.test.ts+tests/isr-cache.test.ts+tests/prerender-kv-populate.test.ts+tests/prerender-server-pool.test.ts— 1516 passedvp checkon the four core changed files — clean (format, lint, types)
No correctness blockers found. The layering follows AGENTS.md (cross-process build cache is a typed server/prerender-data-cache.ts module; the generated prerender-server-entry.ts stays thin, just choosing the handler from VINEXT_PRERENDER_DATA_CACHE_DIR), path wiring is consistent (outDir = serverDir/prerendered-routes across build write, runtime seed, and KV populate), and all crash paths release the single-flight .pending and serialization .write-lock locks (openSync(..., "wx") create-exclusive + mtime-based stale recovery).
Things I specifically verified
- Cloudflare KV runtime is not re-seeded per request. I traced
seedPrerenderDataCache(prerenderDir, getDataCacheHandler())inseed-cache.ts— it runs only fromstartProdServer(Node prod server startup), where the default handler isMemoryCacheHandler. The Cloudflarefetch-handler.tsworker entry does not call the seeder; KV is populated once at deploy time viabuildPrerenderKVPairs. So theelsebranch inseedPrerenderDataCache(which callshandler.setbecauseKVCacheHandlerhas noseed) does not create a per-cold-start KV write storm. This was my top concern and it checks out. - Hard vs profiled tag races on KV are separated by keyspace. Hard invalidations write
__tag:and profiled ones__tag_profile:(both read in_getTagRevalidation, merged inrevalidateTag's_tagCacheupdate), so a hardrevalidateTag(tag)and a profiledrevalidateTag(tag, { expire })no longer clobber each other. Covered by the new "does not weaken a hard invalidation..." / "keeps concurrent hard and profiled invalidations independently composable" tests. - Memory size accounting stays consistent through the new read-path tag merge.
MemoryCacheHandler.getgrowscurrentMemoryCacheSizewhen unioning requested tags onto a FETCH entry before the tag-expiry delete;deleteEntryrecomputesestimateEntrySize(existing)from the already-mutated entry, so the grown size is exactly the size subtracted on delete.evictLeastRecentlyUsedis now also called fromget, buttouchEntrymakes the just-read entry MRU so it isn't self-evicted. - Timestamp monotonicity — both handlers use
nextTimestamp()/currentTimestamp();seedbumpslastTimestamptometadata.lastModifiedso a later same-mssetis still treated as newer. Tested in both memory and KV. revalidate: falseround-trips through the KV serializer without a TTL (expiration_ttlonly for numeric revalidate) and without coercion;validateCacheEntryacceptscacheControl.revalidate === false, andbuildPrerenderKVPairsmirrors this.commitdisposition inapp-rsc-handler.ts(!privateCacheUsed || !hasExplicitNonCacheableResponsePolicy(headers)) matches the private-cache-discard test atprerender-data-cache.test.ts:97and the cacheable-commit test at :139.- Error/lock-release parity across
refreshAndStore(cache-runtime),refreshUnstableCacheResult(cache.ts), andcreatePatchedFetch(fetch-cache.ts) — every throw/no-value branch callsreleasePendingSet(cacheKey).
Notes (non-blocking)
observeEntryre-persists on every tagged FETCH memory hit during prerender. When a tracker exists (the normal in-request prerender path),persistEntryalways runs a full read-modify-write under.write-lock— even in theexisting.version === incomingVersion.versionbranch where the persisted tag set is unchanged. A dirty check (skip the write when the persisted tags already cover the incoming tags) would cut build-time file writes on apps with many shared cached fetches. First place to look if prerender build time regresses.PrerenderDataCacheHandler.setre-reads viathis.memory.get(key)(no ctx) before persisting. Worth a one-line comment: the re-read can returnnullif the just-setentry was tag-revalidated in the same worker, in which case nothing is persisted — intentional but surprising.- Two concurrent profiled
revalidateTag(tag, { expire })on KV can still lose a field (no CAS on__tag_profile:). Inherent KV limitation now that hard/profiled keyspaces are split; a short comment near the merge would prevent it being read as a strong guarantee. entryVersiondereferencesentry.value.data.headerswithoutisPersistedFetchEntryguardingdata.headers. Safe for vinext-constructed FETCH values (always haveheaders), but a malformed persisted FETCH would throw inside thereadEntry→persistedVersionpath rather than being treated as a corrupt-entry miss. Defensive only.
The concurrency model, lock-release paths, dev/build/runtime/KV parity, and timestamp/tag semantics all look sound and are backed by targeted tests.
|
Review posted to PR #2849. SummaryI reviewed all 22 changed files in PR #2849 ( No correctness blockers found. The change is well-architected and matches the repo's layering guidance. Key verifications:
Non-blocking notes raised:
|
|
/bigbonk review for issues |
Review: fix(cache): persist prerender resume dataReviewed the full 22-file diff independently and re-ran the affected suites locally — all green:
No correctness blockers found. The change is well-architected and follows Things I independently verified
Notes (non-blocking)
The concurrency model, lock-release paths, dev/build/runtime/KV parity, timestamp monotonicity, and hard/profiled tag semantics all look sound and are backed by targeted tests. |
|
Review posted to PR #2849. SummaryI independently reviewed all 22 changed files in PR #2849 ( No correctness blockers found. The change is well-architected and follows Things I verified independently:
Non-blocking notes raised:
|
…-cache # Conflicts: # packages/vinext/src/server/seed-cache.ts # tests/prerender-kv-populate.test.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): persist prerender resume data
Independent re-review of the full 22-file diff. Re-ran the affected suites locally — all green:
tests/prerender-data-cache.test.ts— 26 passedtests/kv-cache-handler.test.ts— 77 passedtests/shims.test.ts -t "use cache"— 59 passedtests/fetch-cache.test.ts+tests/isr-cache.test.ts— 211 passedvp checkon the four core files — clean (format, lint, types)
No correctness blockers found. The change follows AGENTS.md layering (cross-process build cache is a typed server/prerender-data-cache.ts; the generated prerender-server-entry.ts stays thin, selecting the handler from VINEXT_PRERENDER_DATA_CACHE_DIR).
Things I verified independently
- Path wiring is consistent end-to-end.
outDirfor non-export builds resolves todist/server/prerendered-routes(run-prerender.ts:214), which equalspath.join(serverDir, "prerendered-routes")inseed-cache.ts:122and theprerenderDirpassed tobuildPrerenderKVPairs. The data cache lives under.vinext-resume-data-cacheinside that dir across build write, runtime seed, and KV populate.resetPrerenderDataCache(outDir)runs before the pool starts (prerender.ts:1056vs pool at:1745), so no reset/pool race. commitdisposition (app-rsc-handler.ts:1767) is only set insideVINEXT_PRERENDER=1, and a thrown render leavescommit: null(the earliercatchre-throws), so the finalizer — which treatscommit === truestrictly — correctly drops provisional versions on failed renders.prerenderDataCacheState.finalizers/commitare shared by reference across nested unified scopes via the documented shallow clone.- Runtime SWR change is safe. The
use cachehit gate moved from!== "stale"to!== "expired", so stale entries now serve immediately + background-revalidate at runtime, but foreground-refresh underVINEXT_PRERENDER=1(cache-runtime.ts:864).scheduleUseCacheBackgroundRevalidationdedupes bycacheKeyin a global map so concurrent stale hits schedule only one refresh (asserted bywaitUntilPromiseslength 1 in the new shims test); the backgroundrefreshAndStore(false)runs withskipPropagation: trueand.catch.requestResumeCachegives intra-request read consistency. - Memory tag-model refactor is size-consistent. The new read-path FETCH tag merge (
cache-handler.ts:268) growscurrentMemoryCacheSizebefore the tag-expiry delete;deleteEntryrecomputes from the mutated entry, andtouchEntrymakes the just-read entry MRU so the newevictLeastRecentlyUsed()ingetcan't self-evict.revalidateTagmerges...existingso a profiled revalidation never drops a prior hardexpiredAt. - Hard vs profiled tag races are separated by keyspace (
__tag:vs__tag_profile:, both read in_getTagRevalidation);kv-key.tscorrectly widens the app-prefix budget check to the longerPROFILE_TAG_PREFIX. revalidate: falseround-trips without a TTL (buildPrerenderKVPairsonly setsexpiration_ttlfor numeric revalidate;validateCacheEntryacceptsrevalidate === false).- Error/lock-release parity across
refreshAndStore(cache-runtime),createPatchedFetch(fetch-cache), andset(finally) — every throw / no-value branch callsreleasePendingSet.
Notes (non-blocking)
- Build-time write amplification in
observeEntry. Every tagged FETCH memory hit during prerender runs a full read-modify-write under.write-lock, even when the persisted tag set already covers the incoming tags. A dirty check would cut file writes on apps with many shared cached fetches — first place to look if prerender build time regresses. - KV new-tag detection is imprecise.
kv-data-adapter.runtime.ts:280gates onhardTags.length !== entry.tags.length; if a stored entry'stagscontains dupes/invalid entries the lengths differ with no genuinely new requested tag. Benign (it only re-normalizesrestoredValue.tags), but the memory handler'srequestedTags.some((tag) => !entry.tags.includes(tag))predicate is cleaner and worth aligning. - Concurrent profiled
revalidateTag(tag, { expire })on KV can still lose a field (no CAS on__tag_profile:, now that hard/profiled keyspaces are split). Inherent KV limitation; a short comment near the merge would prevent it being read as a strong guarantee. setre-reads viathis.memory.get(key)(no ctx) before persisting (prerender-data-cache.ts:712) — can returnnullif the just-set entry was tag-revalidated in the same worker, in which case nothing is persisted. Intentional but surprising; worth a one-line comment.
The concurrency model, lock-release paths, dev/build/runtime/KV parity, timestamp monotonicity, and hard/profiled tag semantics are sound and backed by targeted tests.
|
Review posted to PR #2849. SummaryI independently re-reviewed all 22 changed files in PR #2849 ( No correctness blockers found. The change is well-architected and follows Things I verified independently:
Non-blocking notes raised: build-time write amplification in |
1 participant
Summary
Validation
vp checkon all 20 changed filestest/e2e/app-dir/resume-data-cache/resume-data-cache.test.ts— 5/5 passed on committed head3357bd2b2Failure source: Actions run 31290819291.