perf(build): parallelize prerender across a pool of render processes - #2437
Conversation
commit: |
Build-time prerender rendered every static route by fetching it from a single in-process production server driven by a promise pool. React SSR/RSC rendering is CPU-bound JS, so the pool only overlaps I/O — every render serialized on one core, and raising --prerender-concurrency did nothing. Next.js forks a worker pool and saturates every core. Fork a pool of production-server child processes (one per core, capped) and round-robin the per-route render fetch across them, keeping route scanning, getStaticPaths/static-params resolution, file writing and the manifest on the main process. Pool size scales by cores AND routes, so small apps, low-memory machines, and --prerender-concurrency 1 keep the single in-process server (no fork, no regression); running from source (no built .js worker entry) also falls back to single-process. child_process, not worker_threads: worker threads contend for CPU on this workload (measured ~2x slower per route and non-scaling), which is also why Next.js uses processes. react.dev (809 routes, cold cache, same machine): 29.9s -> 15.1s, now faster than its own Next.js build (~19.3s). An 801-route static fixture: ~22s -> ~2-6s. Prerender output is byte-identical to the single-process path on deterministic renders; workers install the same NoOp cache handler the in-process path uses. A worker that exits unexpectedly fails the build loudly instead of shipping partial output.
608720f to
9e013ac
Compare
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the parallel-prerender pool. Overall this is a careful, well-documented change with good separation of concerns: orchestration (route scan, getStaticPaths/generateStaticParams resolution, secret endpoints, file writes, manifest) stays on the main process via baseUrl, and only the per-route render fetches are round-robined across forked workers via renderPorts. Worker/in-process parity is correctly maintained (both install NoOpCacheHandler; VINEXT_PRERENDER=1 is set before any module loads via the fork env), the hybrid _prodServer path is untouched, and prerenderPoolAvailable() correctly falls back to single-process when running from source. Crash handling (assertHealthy + disconnect-exit + signal-kill fast-fail) is thoughtful. vp check is clean on the new files and the sizing test passes.
A few non-blocking observations below — mostly comment/doc accuracy and test coverage. Nothing I'd consider a correctness blocker.
One thing I confirmed is not a bug: in prerenderApp.renderUrl, the HTML fetch and the RSC fallback fetch can land on two different workers (the round-robin counter advances per fetch). Because workers are stateless and the cache handler is a NoOp, this is safe — worth being aware of, but no action needed.
| // Leave one core for the main thread (fetch + file writes) and OS. | ||
| const byCores = Math.max(1, Math.min(cores - 1, MAX_POOL_SIZE)); | ||
| // Cap by available memory so K bundles don't OOM on constrained CI. | ||
| const byMemory = Math.max(1, Math.floor(os.totalmem() / APPROX_BYTES_PER_WORKER)); |
There was a problem hiding this comment.
Memory cap uses os.totalmem() but the doc comments ("Cap by available memory", "so K render processes don't OOM on constrained CI") describe available memory. totalmem ignores memory already consumed by the main build process (full bundle + in-process prod server, also hundreds of MB) and anything else on the box. On a tight 2GB CI runner this yields floor(2GB/768MB)=2 workers, but main + in-process server + 2 workers can still exceed 2GB. assertHealthy() does catch the resulting OOM-kill loudly, so this is a tuning/wording nit rather than a correctness issue — but consider either (a) reserving a budget for the main process here, or (b) softening the comment from "available memory" to "total memory (coarse upper bound)" so the intent isn't misread later.
| // A child killed by signal reports code === null; reject on either so an | ||
| // OOM-killed (SIGKILL) startup fails fast instead of waiting out the 60s | ||
| // readiness timeout. | ||
| child.once("exit", (code, signal) => { |
There was a problem hiding this comment.
Minor: this startup-exit listener is never removed after the child becomes ready, so once a ready worker later crashes, both this handler and the post-ready crash recorder (line 128) fire. The reject() here is a harmless no-op on an already-resolved promise, so this is fine as-is — just flagging that the double-registration is intentional/benign and might warrant a one-line comment to avoid a future "why two exit handlers?" question.
| if (typeof process.send === "function") { | ||
| process.send({ type: "error", error: message }); | ||
| } | ||
| // Give the IPC message a tick to flush before exiting. |
There was a problem hiding this comment.
The 50ms setTimeout before process.exit(1) is a heuristic to flush the IPC error message. If serialization of a large stack takes longer than 50ms the parent may miss the message and instead surface the generic startup-exit rejection (code 1) without the real error text. Low-risk, but child.send/process.send accepts a callback (process.send(msg, () => process.exit(1))) which would exit deterministically once the message is flushed rather than racing a fixed timer.
|
|
||
| expect(resolvePrerenderPoolSize(384, 1)).toBe(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test coverage is limited to resolvePrerenderPoolSize (pure sizing math). The riskier runtime surface — startPrerenderServerPool readiness aggregation, the crash→assertHealthy() path, and startup-exit fast-fail — has no test. I understand prerenderPoolAvailable() is false from source (the worker entry is .ts, not a forkable .js), which makes the real fork path hard to unit-test here. Still worth considering a focused test that forks a tiny stub entry (or factors the IPC ready/crash bookkeeping into a pure helper) so assertHealthy()'s "fail the build loudly" guarantee — the main correctness claim of the PR — is actually exercised in CI rather than only validated manually.
|
Review posted successfully: #2437 (review) SummaryI reviewed PR #2437 (parallelize prerender across a pool of render processes) and posted a batch review with 4 inline comments. No correctness blockers found. What's solid:
Non-blocking observations raised:
I also confirmed a potential concern that turned out not to be a bug: the HTML and RSC-fallback fetches for one App Router route can hit different workers, which is safe since workers are stateless with a NoOp cache. |
3ab93e0 to
e9f007f
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the parallel-prerender pool. The design is solid and the separation of concerns is exactly right: route scanning, getStaticPaths/generateStaticParams resolution, and the secret /__vinext/prerender/* endpoints all stay on the in-process server via baseUrl, and only the per-route render fetches round-robin across renderPorts. Worker/in-process parity is correct (both install NoOpCacheHandler; VINEXT_PRERENDER=1 is set via the fork env before any module loads; forked workers read the prerender secret from the same outDir on disk). Hybrid _prodServer is untouched, and prerenderPoolAvailable() cleanly degrades to single-process from source. The runtime tests now cover startPrerenderServerPool start/close, the crash to assertHealthy() path, and startup-exit fast-fail — addressing the earlier coverage gap.
One genuine (minor) robustness concern around the assertHealthy() timing, plus a couple of smaller notes. Nothing I'd consider a correctness blocker.
I also re-confirmed the RSC-fallback fetch landing on a different worker than the HTML fetch (round-robin advances per fetch) is safe — workers are stateless with a NoOp cache.
| // A worker that crashed mid-render makes its routes fail with connection | ||
| // errors that are otherwise recorded as non-fatal — fail the build loudly | ||
| // instead of shipping partial output. | ||
| renderPool?.assertHealthy(); |
There was a problem hiding this comment.
The assertHealthy() guarantee has a theoretical timing gap on the last render. crash is set by the child's async exit event, but assertHealthy() is called synchronously right after await runWithConcurrency(...). The in-flight fetch() to a dying worker rejects on a socket error (ECONNRESET/ECONNREFUSED), and the child exit event is an independent libuv event with no ordering guarantee relative to that socket error. So if a worker crashes on the final route(s), assertHealthy() can run before crash is populated, and the build could still emit partial output and exit 0 — the exact failure this guards against.
In practice it's well mitigated: after a crash, every subsequent route routed to the dead port also fails, and draining those failures buys enough event-loop time for exit to fire. The test even sleeps 100ms before asserting, which acknowledges this dependency. Worth a short comment noting the reliance on the loop draining, or consider making assertHealthy() also fail when accumulated per-route connection errors exceed a threshold, so the guarantee doesn't hinge purely on exit timing.
| // No custom 404 | ||
| } | ||
| } | ||
| renderPool?.assertHealthy(); |
There was a problem hiding this comment.
Minor: just above, the 404 render's renderPage(...) fetch can land on a crashed worker's port and reject, which the surrounding catch {} silently swallows as "No custom 404" — so an existing custom 404 would be quietly skipped. It's recovered because this assertHealthy() still surfaces the crash and fails the build, so output is never shipped silently. Flagging that the swallowed connection error there is intentionally relying on this assertHealthy() to be the real signal; a one-line comment would prevent a future "why is the 404 silently missing?" investigation.
| * Used with total system memory as a coarse upper bound on constrained machines | ||
| * so K render processes don't OOM. Conservative — measured ~260-380 MB/worker | ||
| * for a mid-size app. */ | ||
| const APPROX_BYTES_PER_WORKER = 768 * 1024 * 1024; |
There was a problem hiding this comment.
APPROX_BYTES_PER_WORKER = 768MB is roughly 2-3x the measured per-worker footprint (the comment says ~260-380 MB/worker). Combined with the new APPROX_MAIN_PROCESS_BYTES reservation this is appropriately conservative for avoiding OOM, but on, say, a 16-core / 8GB box it caps workers at floor((8GB-768MB)/768MB)=9 → effectively core-bound, whereas the actual memory could comfortably host more. That's the safe direction (under-fork rather than OOM), so not a blocker — just noting the budget is coarse and may leave throughput on the table on high-core/moderate-memory machines.
|
Review posted successfully: #2437 (review) SummaryI reviewed PR #2437 (parallelize prerender across a pool of render processes) and posted a batch review with 3 inline comments. No correctness blockers. What's solid:
Non-blocking observations raised:
|
e9f007f to
2994e18
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the parallel-prerender pool again, focusing on cross-path parity between the Pages Router and App Router prerender flows. The design is sound and most concerns from the earlier reviews still hold. One concrete asymmetry stands out that I'd treat as a real (if low-probability) parity bug rather than a nit, plus a couple of smaller notes.
App Router 404 catch is missing the recordRenderError that the Pages Router 404 catch got.
The Pages Router 404 path was correctly updated to record transport failures (prerender.ts:946-951):
} catch (e) {
// No custom 404. When the render-worker pool is active, a transport
// failure here is still captured by assertHealthy() below ...
renderPool?.recordRenderError(e);
}
but the App Router 404 path (prerender.ts:1620-1622) still swallows silently:
} catch {
// No custom 404 — skip silently
}
This matters because the App Router 404 fetch goes through rscHandler, which round-robins across renderPorts (prerender.ts:1082) — so it can land on a crashed worker just like a normal route. The whole reason recordRenderError exists (per its own docstring at prerender-server-pool.ts:72-75) is to close the timing gap where the request rejects with a socket error before Node delivers the child exit event. The following renderPool?.assertHealthy() at prerender.ts:1623 only inspects the exit-driven crash field and the recorded renderTransportError; if the 404 fetch hits a dead worker and the exit event hasn't fired yet, nothing is recorded and assertHealthy() passes, so the build writes the manifest and exits 0 having silently skipped an existing custom 404. That is exactly the failure the Pages Router fix guards against. The two router paths should behave identically here.
The remaining items are non-blocking and overlap with prior reviews.
| @@ -1546,6 +1620,7 @@ export async function prerenderApp({ | |||
| } catch { | |||
| // No custom 404 — skip silently | |||
| } | |||
There was a problem hiding this comment.
This App Router 404 catch should mirror the Pages Router 404 catch (prerender.ts:946-951), which was updated to record transport errors. The 404 fetch here goes through rscHandler, which round-robins across renderPorts (line 1082), so it can hit a crashed worker. The assertHealthy() on the next line only sees the exit-driven crash field and any recorded renderTransportError — without recording here, a socket error that beats the exit event leaves nothing for assertHealthy() to catch, and the build can silently skip an existing custom 404 and still exit 0. That is the exact gap recordRenderError was added to close (see its docstring at prerender-server-pool.ts:72-75).
| } | |
| } catch (e) { | |
| // No custom 404. When the render-worker pool is active, a transport | |
| // failure here is still captured by assertHealthy() below so a crashed | |
| // worker cannot silently skip an existing custom 404 (mirrors the | |
| // Pages Router 404 path). | |
| renderPool?.recordRenderError(e); | |
| } |
| const byMemory = Math.max(1, Math.floor(memoryForWorkers / APPROX_BYTES_PER_WORKER)); | ||
| const cap = maxOverride && maxOverride > 0 ? Math.min(maxOverride, byCores) : byCores; | ||
| const byRoutes = Math.floor(routeCount / MIN_ROUTES_PER_WORKER); | ||
| return Math.max(1, Math.min(cap, byMemory, byRoutes)); |
There was a problem hiding this comment.
Note the interaction with DEFAULT_CONCURRENCY = min(cores, 8) (prerender.ts:255): concurrency is passed as maxOverride here and used as the in-flight fetch limit in runWithConcurrency. So by default the pool size is silently capped at min(cores, 8) workers as well as at cores - 1. That's a fine default, but it means the documented "one per core" only holds up to 8 cores and the cap is effectively min(cores-1, 8, concurrency). Worth a one-line note so a future reader doesn't expect more workers than the concurrency limit allows.
| // assertHealthy() can fail the build instead of shipping partial output. | ||
| child.once("exit", (code, signal) => { | ||
| if (!shuttingDown && !crash) { | ||
| crash = { port: msg.port, code, signal }; |
There was a problem hiding this comment.
crash only ever records the first unexpected exit (if (!shuttingDown && !crash)). That's fine for failing the build, but combined with the per-fetch round-robin it means: if worker A crashes, assertHealthy() fires (good); but if assertHealthy() at an earlier checkpoint already passed and a second worker dies later, the message still names the first port. Purely cosmetic for the error text — flagging only so the single-slot design is intentional and not mistaken for "records every crash."
|
Review posted successfully: #2437 (review) SummaryI focused this review on cross-path parity between the Pages Router and App Router prerender flows, since the earlier two reviews already covered the general design, crash handling, and sizing math. One concrete finding (the main one):
Two non-blocking notes:
No correctness blockers beyond the App Router 404 asymmetry, which is low-probability (only manifests on a worker crash during the final 404 render) but is a genuine dev/prod-style parity gap between the two router paths. |
2994e18 to
ad426ce
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed with a focus on what the three prior /bigbonk passes didn't cover: the App Router 404 recordRenderError asymmetry has already been fixed in ad426ce, and crash handling / sizing math are well-covered. I looked instead at whether moving the render into a forked worker changes any manifest metadata (not just the HTML/RSC bytes), since the PR's correctness claim is byte-identical output.
One concrete finding: the App Router prerender derives revalidate/expire for the manifest partly from the request-scoped cache-life process-global (_consumeRequestScopedCacheLife()), which no longer lives in the same process as the render once the pool is forked. In the common case this is recovered from the response Cache-Control header, but there's one branch where it isn't — details inline. It's a narrow cacheLife({ expire <= revalidate }) case, so I'd treat it as a non-blocking parity note rather than a blocker, but it does mean pooled vs single-process output is not strictly identical for those routes.
Everything else looks solid. Nice, well-scoped change.
| ...(isFallback ? { fallback: true } : {}), | ||
| }; | ||
| } catch (e) { | ||
| renderPool?.recordRenderError(e); |
There was a problem hiding this comment.
Parity note (App Router only), about the cache-life metadata feeding resolveRenderedCacheControl a few lines below (line 1525-1526):
htmlRender.requestCacheLife comes from _consumeRequestScopedCacheLife() (line 1440), which reads a process-global cache-life set during render. With the pool active the render runs in a forked worker, so this global is empty in the main process and requestCacheLife is always {}.
In the common case that's fine — resolveRenderedCacheControl falls back to parsing s-maxage + stale-while-revalidate from the response Cache-Control header (line 1656-1659), which the worker emits via buildRevalidateCacheControl. But buildRevalidateCacheControl returns a bare s-maxage=R (no stale-while-revalidate) when revalidate >= expire (cache-control.ts:57-58). In that case the header fallback yields staleWhileRevalidate === undefined, so revalidate resolves to undefined and expire falls back to config.expireTime — whereas the single-process path would have read the real R/E from requestCacheLife.
Net effect: for a route using cacheLife({ revalidate, expire }) with expire <= revalidate, the manifest revalidate/expire differ between single-process and pooled builds. The rendered HTML/RSC bytes are still identical, but the cache metadata in vinext-prerender.json isn't — which slightly undercuts the "byte-identical" correctness claim for those routes. Low-probability (named cacheLifeProfiles all have revalidate < expire), but worth either documenting as a known gap or having the worker round-trip the cache life back to the main process (e.g. a response header) so the manifest matches single-process exactly.
| function isWorkerTransportError(err: unknown): boolean { | ||
| const code = getErrorCauseCode(err); | ||
| if (code && PRERENDER_WORKER_TRANSPORT_ERROR_CODES.has(code)) return true; | ||
| return err instanceof TypeError && err.message === "fetch failed"; |
There was a problem hiding this comment.
isWorkerTransportError also matches any bare TypeError("fetch failed"). Combined with assertHealthy() throwing on the first recorded transport error, a single transient socket failure from an otherwise-healthy worker (e.g. a brief connection reset under load, not a crash) would fail the whole build with the OOM-flavored message. That's the safe direction (fail loud rather than ship partial output), but it means the pool path is slightly less tolerant of transient I/O than the single-process path, which would surface the one route as a normal render error. Worth a comment noting this is intentional, or scoping the transport-error fatality to cases where a worker exit was also observed.
| let crash: { port?: number; code: number | null; signal: NodeJS.Signals | null } | null = null; | ||
| let renderTransportError: Error | null = null; | ||
|
|
||
| const close = async (): Promise<void> => { |
There was a problem hiding this comment.
close() fires SIGKILL but resolves synchronously without awaiting the children's exit, so await renderPool.close() in the finally returns before the OS has reaped the processes. In practice the disconnect handler in the entry plus SIGKILL make orphans unlikely, but if the build process exits immediately after close() resolves there's a small window where killed-but-not-yet-reaped servers still hold their ports. If you want close() to actually mean "all workers are gone," await each child's exit (with a short timeout) before resolving.
|
Review posted successfully: #2437 (review) SummaryI reviewed PR #2437 (parallelize prerender across a pool of render processes), deliberately covering ground the three prior I focused on whether forking the render into a child process changes any manifest metadata (not just the HTML/RSC bytes the PR claims are byte-identical), since that's the one thing process isolation can silently break. One concrete finding (non-blocking parity gap):
Two smaller notes:
|
ad426ce to
d1b0f42
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed with a focus on what the four prior /bigbonk passes did not cover: the new VINEXT_PRERENDER_CACHE_LIFE_HEADER round-trip (added since the 4th review, which had flagged exactly this cache-life divergence — nice fix) and its interaction with the existing internal-header machinery.
CI blocker (main finding)
Adding VINEXT_PRERENDER_CACHE_LIFE_HEADER to VINEXT_INTERNAL_HEADERS (in packages/vinext/src/server/headers.ts) breaks an existing assertion in tests/request-pipeline.test.ts:794 that pins the array to exactly [VINEXT_PRERENDER_ROUTE_PARAMS_HEADER]. Confirmed failing locally:
FAIL tests/request-pipeline.test.ts > filterInternalHeaders > strips vinext-only internal headers ...
AssertionError: expected [ …(2) ] to deeply equal [ 'x-vinext-prerender-route-params' ]
- Expected
+ Received
[
"x-vinext-prerender-route-params",
+ "X-Vinext-Prerender-Cache-Life",
]
The required Vitest check will be red. Fix is a one-liner — update the toEqual at tests/request-pipeline.test.ts:794 to include the new header and add it to the imports at the top of that file:
expect(VINEXT_INTERNAL_HEADERS).toEqual([
VINEXT_PRERENDER_ROUTE_PARAMS_HEADER,
VINEXT_PRERENDER_CACHE_LIFE_HEADER,
]);Confirmed NOT a leak
The new response header is gated behind isPrerender === true (requestCacheLifeForPrerender defaults to null, and applyPrerenderCacheLifeHeader early-returns on null), so it is never emitted on normal client responses. It is also not persisted into the static HTML/RSC files (only response.text() / arrayBuffer() bodies are written; the manifest stores only link). Adding it to VINEXT_INTERNAL_HEADERS for inbound stripping is correct defense-in-depth.
Parity note (non-blocking)
In the single-process path both responseCacheLife (parsed from the new header) and processCacheLife (_consumeRequestScopedCacheLife()) are populated, and responseCacheLife now wins via ?? (prerender.ts:1443). This is the intended fix for the pooled-vs-single cache-metadata divergence the 4th review flagged, and the new app-page-render tests confirm the header carries the same {revalidate, expire}. Worth a one-line comment that the header is now authoritative in both modes (and that _consumeRequestScopedCacheLife() is still consumed to drain the process-global / cover the no-header fallback) so a future reader doesn't assume the two can silently diverge.
The pool design, crash handling, and sizing math are solid and well-covered by the earlier reviews — this is just the one stale test.
|
Review posted: #2437 (review) SummaryI reviewed PR #2437, deliberately covering ground the four prior One CI blocker found:
Confirmed safe (not a leak):
One non-blocking parity note:
The pool design, crash handling, and sizing math (covered by the earlier reviews) are solid. The single actionable item is the stale test. |
d1b0f42 to
e604dfb
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Sixth /bigbonk pass. The CI blocker from the fifth review (the stale VINEXT_INTERNAL_HEADERS toEqual assertion) is fixed — I ran the affected files locally and they're green:
✓ tests/prerender-server-pool.test.ts (7 tests)
✓ tests/request-pipeline.test.ts (102 tests)
✓ tests/app-page-render.test.ts (51 tests)
The pool design, crash handling, sizing math, and the cache-life header round-trip (which fixes the divergence the 4th review flagged) are all solid and well covered by the earlier passes. I re-verified the things process isolation can silently break:
- Render/resolution split is correct. Secret resolution endpoints (
/__vinext/prerender/static-params,pages-static-paths) usebaseUrl(the in-process server,prerender.ts:696,1123); only render fetches round-robin acrossrenderPorts(prerender.ts:1083,1420,1504,1611). isPrerenderparity in workers holds. It's derived fromprocess.env.VINEXT_PRERENDER === "1"(app-page-dispatch.ts:1004), which the fork env sets before any module loads, so forked workers emitX-Vinext-Prerender-Cache-Lifeexactly like the in-process server.renderPortsis initialized beforerscHandlercan run (prerender.ts:1066vs the render loop), so no% 0divide-by-zero despite the[]default.- App Router 404
recordRenderError(the 3rd review's finding) is present atprerender.ts:1631.
The _consumeRequestScopedCacheLife() global is still drained on the success path; responseCacheLife ?? processCacheLife correctly prefers the header in both modes.
One concrete finding the prior five passes missed: a latent header-casing inconsistency
The new constant is mixed-case: VINEXT_PRERENDER_CACHE_LIFE_HEADER = "X-Vinext-Prerender-Cache-Life". Every other header in VINEXT_INTERNAL_HEADERS (just VINEXT_PRERENDER_ROUTE_PARAMS_HEADER = "x-vinext-prerender-route-params") is lowercase, and that's load-bearing in a second consumer the prior reviews didn't trace:
packages/vinext/src/index.ts:4570-4575 (the dev-server inbound strip — not in this PR's diff, which is why it's easy to miss):
for (const header of VINEXT_INTERNAL_HEADERS) {
delete req.headers[header]; // raw Node IncomingMessage
}Node's HTTP parser always lowercases req.headers keys, so delete req.headers["X-Vinext-Prerender-Cache-Life"] never matches the stored x-vinext-prerender-cache-life key. A forged inbound header survives in req.headers.
This is the same root cause that forced the .map((header) => header.toLowerCase()) workaround you added in request-pipeline.ts:575 for the Web-Headers Set. That fix covers filterInternalHeaders(rawHeaders) (index.ts:4569) but not the raw req.headers delete loop two lines below it.
Severity: low / defense-in-depth, not an active hole. The cache-life header is only read at build time via case-insensitive headers.get() (prerender.ts:1681), never from runtime inbound req.headers, so a forged value can't influence runtime behavior today. But the loop's invariant ("all stripped internal-header constants are lowercase") is now silently violated, and any future code reading this header from req.headers would get the forged value.
Cleanest fix: make the constant lowercase (suggestion below). Header names are case-insensitive on the wire, so emitting via headers.set() and reading via headers.get() are unaffected — but the raw-req.headers strip at index.ts:4573 starts working, and you can then drop the request-pipeline.ts:575 .toLowerCase() workaround entirely. (Display-cased X-Vinext-* constants are fine because they're response-only and never appear in the inbound strip list; this one is in VINEXT_INTERNAL_HEADERS, so it should follow the lowercase convention of the headers that actually get stripped.)
| export const VINEXT_DYNAMIC_STALE_TIME_HEADER = "X-Vinext-Dynamic-Stale-Time"; | ||
|
|
||
| /** Prerender-only JSON side channel carrying request cacheLife metadata. */ | ||
| export const VINEXT_PRERENDER_CACHE_LIFE_HEADER = "X-Vinext-Prerender-Cache-Life"; |
There was a problem hiding this comment.
Define this lowercase to match VINEXT_PRERENDER_ROUTE_PARAMS_HEADER ("x-vinext-prerender-route-params"), the only other entry in VINEXT_INTERNAL_HEADERS:
| export const VINEXT_PRERENDER_CACHE_LIFE_HEADER = "X-Vinext-Prerender-Cache-Life"; | |
| export const VINEXT_PRERENDER_CACHE_LIFE_HEADER = "x-vinext-prerender-cache-life"; |
Why: this header is added to VINEXT_INTERNAL_HEADERS (line 213), which is consumed in two inbound-strip sites. One is request-pipeline.ts (the Web-Headers Set — you correctly added .toLowerCase() at line 575 to handle the mixed case). The other is index.ts:4573, which does delete req.headers[header] on a raw Node IncomingMessage whose keys are always lowercased by Node — so the mixed-case constant never matches there and a forged x-vinext-prerender-cache-life survives the strip. Lowercasing the constant fixes that second site and lets you revert the request-pipeline.ts:575 workaround. Header names are case-insensitive on the wire, so headers.set()/headers.get() (app-page-response.ts:133, prerender.ts:1681) are unaffected.
|
Review posted successfully: #2437 (review) SummarySixth One concrete finding the prior five passes missed — a latent header-casing inconsistency: The new Severity is low / defense-in-depth — the header is only read at build time via case-insensitive Confirmed solid (re-verified): render/resolution split (secret endpoints stay on |
e604dfb to
b895a2d
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed PR #2437 again as an independent pass. The PR has converged nicely across the prior six /bigbonk rounds: the lowercase VINEXT_PRERENDER_CACHE_LIFE_HEADER constant fixes the index.ts:4573 raw-req.headers strip (verified the loop now matches the lowercased Node key, and the request-pipeline.ts .toLowerCase() workaround is gone), the App Router 404 recordRenderError is present (prerender.ts:1557/1631), renderPorts is initialized before any rscHandler fetch (no % 0), closeChild now awaits exit with a timeout, and the cache-life header round-trip is authoritative via responseCacheLife ?? processCacheLife. I ran the three affected files locally — tests/prerender-server-pool.test.ts, tests/request-pipeline.test.ts, tests/app-page-render.test.ts — all 160 green.
The render/resolution split is correct (secret endpoints + getStaticPaths/generateStaticParams stay on baseUrl; only renders round-robin across renderPorts), the pool outDir matches the in-process server's outDir in both routers, and setCacheHandler(new NoOpCacheHandler()) gives worker/in-process parity.
One new finding the prior passes didn't surface, plus two minor notes. No correctness blockers.
Main finding (a behavior regression risk): the pool is a pure perf optimization layered on top of an already-healthy in-process server, but if startPrerenderServerPool rejects, the whole build fails instead of degrading to single-process. A transient fork hiccup, an ephemeral-port race, or a slow CI box exceeding the 60s readiness timeout could newly fail a build that succeeds today — even though prodServer is up and renderPorts is still pointed at it. Inline below.
| const poolSize = resolvePrerenderPoolSize(pagesToRender.length, concurrency); | ||
| if (poolSize > 1) { | ||
| const poolOutDir = path.dirname(path.dirname(pagesBundlePath)); | ||
| renderPool = await startPrerenderServerPool(poolOutDir, poolSize); |
There was a problem hiding this comment.
The render pool is a pure performance optimization stacked on top of the in-process prodServer, which is already started and fully capable of rendering every route (renderPorts defaults to [prodServer.port]). But startPrerenderServerPool is awaited inside the main try with no catch, so if pool startup rejects — a worker that fails readiness, an ephemeral-port race, a transient fork error, or a slow CI machine exceeding WORKER_READY_TIMEOUT_MS (60s) — the whole build fails.
This means a build that succeeds today (single in-process server) could newly fail purely because of a worker-startup hiccup, even though the in-process server is healthy and renderPorts still points at it. Pool render crashes correctly fail loud via assertHealthy() (that's output correctness), but pool startup failure is different: nothing has been rendered yet and the fallback is right there. Consider catching startup failure and degrading to single-process (log a warning, leave renderPorts as-is) rather than aborting the build:
| renderPool = await startPrerenderServerPool(poolOutDir, poolSize); | |
| if (!options._prodServer && pagesBundlePath && prerenderPoolAvailable()) { | |
| const poolSize = resolvePrerenderPoolSize(pagesToRender.length, concurrency); | |
| if (poolSize > 1) { | |
| const poolOutDir = path.dirname(path.dirname(pagesBundlePath)); | |
| try { | |
| renderPool = await startPrerenderServerPool(poolOutDir, poolSize); | |
| renderPorts = renderPool.ports; | |
| } catch (e) { | |
| // Pool startup is a perf optimization; the in-process server can | |
| // render everything. Degrade rather than failing the build. | |
| console.warn( | |
| `[vinext] prerender render pool failed to start; falling back to single-process render: ${ | |
| e instanceof Error ? e.message : String(e) | |
| }`, | |
| ); | |
| renderPool = null; | |
| } | |
| } | |
| } |
(Same applies to the App Router call at prerender.ts:1576.)
| if (!options._prodServer && prerenderPoolAvailable()) { | ||
| const poolSize = resolvePrerenderPoolSize(urlsToRender.length, concurrency); | ||
| if (poolSize > 1) { | ||
| renderPool = await startPrerenderServerPool(path.dirname(serverDir), poolSize); |
There was a problem hiding this comment.
Same as the Pages Router pool-start at line 851: a pool startup failure here aborts the whole build, even though the in-process prodServer (which renderPorts already points at) could render every route. Wrap in a try/catch and degrade to single-process on startup failure so the pool stays a strictly-additive optimization. (Render-time crashes should still fail loud via assertHealthy() — only startup failure should fall back.)
| noCompression: true, | ||
| purpose: "prerender", | ||
| }); | ||
| if (typeof process.send === "function") { |
There was a problem hiding this comment.
Minor: each forked worker starts with purpose: "prerender", so logProdServerStarted prints [vinext] Production server for prerendering running at ... once per worker (up to 8 lines on a high-core box) in addition to the in-process server's line. Cosmetic, but it makes the build log noisier than single-process. Consider suppressing the per-worker startup log (the parent already knows the ports via IPC) or only logging it for the in-process server.
|
Review posted successfully: #2437 (review) SummarySeventh Confirmed resolved (from the six prior passes):
New finding (behavior regression risk, non-blocking):
Two minor notes:
The pool design, crash handling, sizing math, and cache-life round-trip are solid. The single actionable item is the startup-failure fallback. |
b895a2d to
6a4bad1
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Eighth review pass on PR #2437. I independently re-verified the code and confirmed every actionable finding from the seven prior /bigbonk passes is now resolved in the diff:
- Startup-failure fallback (7th review):
startOptionalPrerenderServerPoolwrapsstartPrerenderServerPoolin try/catch and degrades to single-process with a warning rather than failing the build (prerender.ts:83-98), wired into both routers (prerender.ts:870,1595). - App Router 404
recordRenderError(3rd review): present atprerender.ts:1650, mirroring the Pages Router path atprerender.ts:965. - Lowercase
VINEXT_PRERENDER_CACHE_LIFE_HEADER(6th review): now"x-vinext-prerender-cache-life"(headers.ts:62), and therequest-pipeline.test.tsassertion explicitly asserts everyVINEXT_INTERNAL_HEADERSentry is lowercase (tests/request-pipeline.test.ts:801-803). - Cache-life header round-trip (4th/5th review):
responseCacheLife ?? processCacheLife(prerender.ts:1465) makes the header authoritative while still draining the process-global; newapp-page-rendertests assert the header payload (tests/app-page-render.test.ts:1040,1078). closeChildawaits exit with a timeout (prerender-server-pool.ts:241-257);silentsuppresses per-worker startup logs (prod-server.ts).- Stale
VINEXT_INTERNAL_HEADERStest (5th review): updated attests/request-pipeline.test.ts:798-800.
I also verified render/resolution split (secret endpoints + static-paths stay on baseUrl, only renders round-robin), worker/in-process NoOpCacheHandler + noCompression: true parity, pool outDir matching the in-process server in both routers, and isPrerender-gated header emission (never on client responses, never persisted into HTML/RSC files).
No correctness blockers. One small fresh note inline confirming a non-issue, for the record.
| payload.revalidate = requestCacheLife.revalidate; | ||
| } | ||
| if (typeof requestCacheLife.expire === "number" && Number.isFinite(requestCacheLife.expire)) { | ||
| payload.expire = requestCacheLife.expire; |
There was a problem hiding this comment.
Confirmed-not-a-bug note (no change needed): the header payload carries only revalidate/expire and drops the stale field that CacheLifeConfig also has. In single-process mode processCacheLife (from _consumeRequestScopedCacheLife()) includes stale, so pooled vs single-process requestCacheLife differ in that field — but resolveRenderedCacheControl (prerender.ts:1677-1690), the only consumer feeding the manifest, reads exactly revalidate and expire and never stale. So the manifest output is identical either way and the byte-identical claim holds for cache metadata too. Flagging only so a future reader who adds stale to the manifest derivation remembers to also add it to this side-channel payload.
|
Review posted successfully: #2437 (review) SummaryEighth Confirmed resolved:
Independently re-verified solid: render/resolution split (only renders round-robin, secret endpoints stay on One fresh note (non-issue, documented for the record): the cache-life side-channel header drops the |
2 participants
Problem
vinext build(with--prerender-all,output: 'export', or aprerenderconfig) renders every static route by fetching it from a single in-process production server, driven by a bounded promise pool (runWithConcurrencyinbuild/prerender.ts). React SSR/RSC rendering is CPU-bound JS, so the promise pool only overlaps I/O — every render serializes on one core, and raising--prerender-concurrencychanges nothing. Next.js forks a worker pool and saturates every core. On static-heavy sites vinext therefore wins the bundle phase (rolldown) but loses the prerender phase, and loses total build time.Change
Fork a pool of production-server child processes (one per core) and round-robin the per-route render fetch across them. All orchestration — route scanning,
getStaticPaths/generateStaticParamsresolution, file writing, the manifest — stays on the main process; only where the render runs changes. The render path is otherwise untouched.build/prerender-server-pool.ts(new):startPrerenderServerPool(outDir, size)forkssizestartProdServerchildren, each reporting its ephemeral port over IPC.resolvePrerenderPoolSize(routeCount, maxOverride)scales by cores (availableParallelism() - 1, capped at 8), available memory, and routes (≥48/worker), returning1(= no fork, current behavior) for small apps, low-memory/low-core machines, or--prerender-concurrency 1.build/prerender-server-entry.ts(new): the child entry — installs the sameNoOpCacheHandlerthe in-process prerender path uses, thenstartProdServer({ port: 0, purpose: "prerender" }).build/prerender.ts:prerenderPagesandprerenderAppkeep the in-process server for the resolution endpoints; when they own the server (non-hybrid), the worker entry exists (built package), and the pool size > 1, they fork the pool and round-robin renders across the child ports, closing it infinally. Hybrid (_prodServer) is unchanged. Running from source (no built.jsentry) falls back to single-process.child_process, notworker_threads: worker threads contend for CPU on this workload — measured ~2× slower per route and non-scaling — which is also why Next.js uses processes.Results — react.dev (real app, 809 routes)
Same machine (14-core), cold (
.next/dist+ MDXnode_modules/.cache+ Vite cache cleared before each run), phase-split via per-line timestamps. Next is built from a pristine react.dev tree (the app's real webpack build — see note); vinext is rolldown. Both emit 809 pages.The prerender phase is the whole story: single-process loses it 2.1× (22.0 vs 10.5), and the pool flips it to a win (7.7 vs 10.5) — so total goes from 1.45× slower than Next to 1.35× faster. (Totals corroborated across 2 full runs: pool ~15.1 s, single ~29.9 s, Next ~19.3 s.)
Results — synthetic 801-route static fixture (vs Next 16 + Turbopack)
So the win isn't just "rolldown vs webpack" — same machine, cold, vs Next 16's Turbopack build:
vinext (multi-worker) is faster than Turbopack-Next on total here too; the Next numbers are noisier (Turbopack engine + worker warmup).
Correctness
NoOpCacheHandleras the in-process path (the handler is a process-global), so no ISR/unstable_cache/fetch-cache reuse leaks across routes within a worker.assertHealthy()rather than emitting partial output and exiting 0; children exit on parentdisconnectso they don't orphan; a signal-killed startup fails fast instead of waiting out the readiness timeout.Tuning
Existing
--prerender-concurrency Ncaps the render-process count. Default = cores − 1, max 8.Out of scope (separate issue)
vinext's prerender uses streaming
renderToReadableStream, so a Suspense boundary backed by a lazyimport()serializes in streamed-deferred form on a cold render and inline-resolved form when warm (reproducible single-process — not caused by this change). Deterministic SSG output would usereact-dom/static'sprerender.