feat: seed memory cache from pre-rendered routes#645
feat: seed memory cache from pre-rendered routes#645NathanDrake2406 wants to merge 2 commits intocloudflare:mainfrom
Conversation
Read vinext-prerender.json and pre-rendered HTML/RSC files at production server startup to populate the MemoryCacheHandler. This ensures the first request to any pre-rendered page is a cache HIT instead of triggering a full re-render. - Add seedMemoryCacheFromPrerender() in server/seed-cache.ts - Add buildId, router, and trailingSlash to the prerender manifest - Call seeding during App Router production server startup - 11 unit tests covering ISR routes, static routes, dynamic routes, trailingSlash, graceful degradation, and missing files Closes cloudflare#561
commit: |
|
Only had a brief look to see if this handled Workers as well - do you have any thoughts on how this could be adapt to support that environment as well? Admittedly there would be gotcha's we need to make sure people are aware of if they use the memory cache with Workers, but it would be good to stub that with the prerendered routes rather than just SSR. |
I'll do that in a follow up |
|
Oh my god I need to ref the issue instead of closing |
|
@james-elicx what's ur idea? KV? 🤔 |
I had made a separate issue for remote caches like KV (#562) because it would be a different mechanism to seeding an in-memory cache. KV is opt-in for Workers deployments, so the default which would be the memory cache should be able to use the prerendered files ideally.... But we should encourage people to use a custom handlers for deployments to environments with short-lived compute. |
|
/bigbonk review |
There was a problem hiding this comment.
Good feature — seeding the memory cache from pre-rendered routes is the obvious thing to do for cold-start performance. The implementation is clean and well-tested. A few issues to address, one of which is a correctness bug:
Key issues:
- Bug:
trailingSlash: falsesilently omitted from manifest — the falsy check means the explicitfalsevalue is never written, which happens to work today due to the?? falsedefault on read, but is fragile and misleading. - Missing
setRevalidateDuration()calls — without populating the revalidate duration map, the ISR layer can't emit correctCache-Controlheaders on seeded entries (if the App Router production path reads this map for seeded entries). - Synchronous file I/O at startup —
readFileSyncin a loop over many routes could add noticeable latency to server startup for large sites. - Silent failure on manifest parse errors — worth at least a warning log so operators can diagnose corrupt builds.
| const index = { routes: indexRoutes }; | ||
| const index = { | ||
| ...(buildId ? { buildId } : {}), | ||
| ...(trailingSlash ? { trailingSlash } : {}), |
There was a problem hiding this comment.
Bug (latent): trailingSlash is only written to the manifest when truthy. Since the config type is boolean, trailingSlash: false is intentionally never serialized. This works today because seed-cache.ts defaults to false, but it means the manifest format is lossy — you can't distinguish "not set" from "explicitly false".
The safer pattern is to always write it when the value is a boolean, regardless of truthiness:
| ...(trailingSlash ? { trailingSlash } : {}), | |
| ...(typeof trailingSlash === "boolean" ? { trailingSlash } : {}), |
This way trailingSlash: false is explicitly recorded in the manifest and the reader doesn't need to guess the default.
| try { | ||
| manifest = JSON.parse(fs.readFileSync(manifestPath, "utf-8")); | ||
| } catch { | ||
| return; |
There was a problem hiding this comment.
Silently swallowing parse errors makes corrupt manifests very hard to diagnose. Consider logging a warning:
| return; | |
| } catch (err) { | |
| console.warn("[vinext] Failed to parse vinext-prerender.json, skipping cache seeding:", err); | |
| return; |
| const revalidateCtx = | ||
| typeof route.revalidate === "number" ? { revalidate: route.revalidate } : {}; | ||
|
|
||
| await seedAppRoute(handler, prerenderDir, pathname, buildId, trailingSlash, revalidateCtx); |
There was a problem hiding this comment.
Missing setRevalidateDuration() call. The ISR layer in isr-cache.ts maintains a separate revalidateDurations map used to emit Cache-Control headers on cache hits. If the App Router production path ever reads getRevalidateDuration() for seeded entries, it'll get undefined and fall back to a default (or skip the header entirely).
I checked and the App Router RSC entry currently doesn't use getRevalidateDuration() (only the Pages Router dev server does), so this isn't a live bug today. But it's a correctness gap — if you're seeding the cache to look like a real ISR entry, you should also seed the revalidate duration so the abstraction is complete:
| await seedAppRoute(handler, prerenderDir, pathname, buildId, trailingSlash, revalidateCtx); | |
| await seedAppRoute(handler, prerenderDir, pathname, buildId, trailingSlash, revalidateCtx); | |
| // Also populate the revalidate duration map so Cache-Control headers | |
| // are correct on first HIT (if the serving path reads it). | |
| if (typeof route.revalidate === "number") { | |
| const { setRevalidateDuration } = await import("./isr-cache.js"); | |
| const baseKey = isrCacheKey("app", pathname, buildId); | |
| setRevalidateDuration(baseKey + ":html", route.revalidate); | |
| setRevalidateDuration(baseKey + ":rsc", route.revalidate); | |
| } |
(Or import setRevalidateDuration at the top and call it inside seedAppRoute alongside the handler.set calls.)
| const htmlFullPath = path.join(prerenderDir, htmlRelPath); | ||
| if (!fs.existsSync(htmlFullPath)) return; | ||
|
|
||
| const html = fs.readFileSync(htmlFullPath, "utf-8"); |
There was a problem hiding this comment.
All the file reads in seedAppRoute (existsSync + readFileSync, called twice per route) are synchronous. For a handful of routes this is fine, but for a large site with hundreds of pre-rendered routes, this could add measurable startup latency since each readFileSync blocks the event loop.
Not a blocker for this PR, but worth a // TODO: comment noting that this could be migrated to fs/promises + Promise.all batching if startup time becomes a concern:
| const html = fs.readFileSync(htmlFullPath, "utf-8"); | |
| const html = fs.readFileSync(htmlFullPath, "utf-8"); | |
| // TODO: Consider fs/promises for large route sets to avoid blocking the event loop at startup. | |
| const baseKey = isrCacheKey("app", pathname, buildId); |
| const rscValue: CachedAppPageValue = { | ||
| kind: "APP_PAGE", | ||
| html: "", | ||
| rscData: rscBuffer.buffer.slice( |
There was a problem hiding this comment.
Nit: Buffer.prototype.buffer returns the underlying ArrayBuffer, which may be a pool allocation larger than the Buffer itself. The .slice() here correctly handles that. However, in Node.js 20+ Buffer has a .buffer that returns a proper ArrayBuffer when the Buffer owns the whole thing, but for pooled small buffers it still shares.
The code is correct as-is. Just noting that an alternative is new Uint8Array(rscBuffer).buffer which always produces a right-sized copy, though that's an extra allocation. Current approach is fine.
|
|
||
| // Seed the memory cache with pre-rendered routes so the first request to | ||
| // any pre-rendered page is a cache HIT instead of a full re-render. | ||
| await seedMemoryCacheFromPrerender(path.dirname(rscEntryPath)); |
There was a problem hiding this comment.
Good placement — after the RSC module import but before the HTTP server starts listening. This guarantees all seeded entries are in the cache before any request arrives.
Minor: should this log the number of routes seeded? A single info line like [vinext] Seeded N pre-rendered routes into memory cache would be helpful for operators to confirm the feature is working. Silent success can be hard to distinguish from silent no-op.
| @@ -0,0 +1,361 @@ | |||
| /** | |||
There was a problem hiding this comment.
Solid test coverage — the 11 tests cover the important cases (ISR, static, dynamic, index, trailingSlash, skipped, errored, missing files, multiple routes). A few cases that would strengthen coverage for future work:
- Corrupt/malformed manifest JSON (e.g., truncated file) — verifies the
catchbranch doesn't throw - Missing
buildIdin manifest — verifies the early return - Very long pathname that triggers the FNV hash path in
isrCacheKey— ensures seeded keys match what the RSC entry would generate
Not blocking — just suggestions for follow-up.
|
Review posted successfully. Here's a summary of the feedback I left on PR #645: Key findings:
|
- Warn on corrupt manifest JSON instead of silent swallow - Always serialize trailingSlash (typeof boolean, not truthiness) - Populate setRevalidateDuration() for seeded entries so Cache-Control headers are correct on first HIT - Return seeded route count and log it at startup - Add tests: corrupt JSON, missing buildId, FNV hash path, revalidate duration tracking, return value assertions Addresses review comments on cloudflare#645.
- Warn on corrupt manifest JSON instead of silent swallow - Always serialize trailingSlash (typeof boolean, not truthiness) - Populate setRevalidateDuration() for seeded entries so Cache-Control headers are correct on first HIT - Return seeded route count and log it at startup - Add tests: corrupt JSON, missing buildId, FNV hash path, revalidate duration tracking, return value assertions Addresses review comments on cloudflare#645.
0047beb to
2b744d3
Compare
|
@james-elicx istg if they isn't approved I'll murder bonk I ran like 3 passes of reviews |
Summary
vinext-prerender.jsonat production server startup and populates theMemoryCacheHandlerwith pre-rendered HTML/RSC files fromdist/server/prerendered-routes/buildId,router, andtrailingSlashto the prerender manifest so the seeding function can construct matching cache keys and locate files correctlyChanges
src/server/seed-cache.tsseedMemoryCacheFromPrerender()functionsrc/build/prerender.tsroutertoPrerenderRouteResult, addbuildId/trailingSlashto manifestsrc/build/run-prerender.tsbuildId/trailingSlashtowritePrerenderIndexsrc/server/prod-server.tstests/seed-cache.test.tsScope
App Router only for now. Pages Router seeding requires the prerender phase to also write
pageDataJSON files (not currently done), so it's left for a follow-up.Closes #561
Test plan