fix: Pages Router SSR streaming#514
Conversation
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 914e4830b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4565b5863c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46e7de5dfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (shouldStreamPagesResponse || !mergedResponse.body) { | ||
| await sendWebResponse(mergedResponse, req, res, compress); |
There was a problem hiding this comment.
Routing tagged Pages responses through sendWebResponse here means HEAD requests now take the streamed path, but sendWebResponse exits on HEAD without consuming or canceling webResponse.body (prod-server.ts sendWebResponse HEAD branch). Because the Pages renderer builds a live ReadableStream, those requests can keep server-side rendering work running with no downstream reader, wasting CPU/memory under HEAD health checks or crawlers. Please cancel the body stream (or avoid the streamed path for HEAD) before returning.
Useful? React with 👍 / 👎.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Pages Router SSR Streaming
The core change is sound -- preserving the Response.body stream through sendWebResponse instead of buffering via arrayBuffer() is the right fix for TTFB and memory. The edge-case handling for no-body statuses and stale Content-Length is thorough, and the test coverage is excellent.
I have two issues worth addressing (one correctness, one maintainability) and a few minor observations.
Issue 1: HEAD requests leak the SSR render stream (correctness)
Now that streamed Pages responses route through sendWebResponse (line 1289), HEAD requests will enter the streaming path. sendWebResponse handles HEAD by calling res.end() at line 599 without consuming or canceling webResponse.body. For static or short-lived bodies this is mostly harmless (GC cleans up), but for the Pages SSR render stream this means React will continue server-rendering the full component tree with no downstream reader. On crawlers or health checks that send HEAD, this wastes CPU and memory.
The Codex review flagged this too. Suggested fix: cancel the body stream when sendWebResponse exits for HEAD (add webResponse.body.cancel().catch(() => {}) before the return in the HEAD branch around line 599). You already have cancelResponseBody() defined -- reuse it.
Issue 2: Three near-identical merge implementations (maintainability)
The no-body/streamed-length logic is now triplicated across:
prod-server.ts->mergeWebResponseworker-utils.ts->mergeHeadersdeploy.ts->mergeHeaders(template string)
These must stay perfectly in sync or one deployment target silently diverges. I understand the constraint (deploy.ts is a code-gen template, worker-utils.ts runs in Workers), but this is getting complex enough that a follow-up to factor out a shared implementation or at least add a prominent sync-warning comment would reduce future risk.
Not a blocker for this PR, but worth tracking.
| const mergedResponse = mergeWebResponse(middlewareHeaders, response, middlewareRewriteStatus); | ||
|
|
||
| if (shouldStreamPagesResponse || !mergedResponse.body) { | ||
| await sendWebResponse(mergedResponse, req, res, compress); |
There was a problem hiding this comment.
HEAD requests will leak the SSR render stream. With this change, streamed Pages responses now always route through sendWebResponse, which on HEAD calls res.end() without canceling webResponse.body. Since the Pages renderer produces a live ReadableStream backed by React's render pipeline, the rendering work continues with no downstream reader.
This is a pre-existing gap in sendWebResponse, but this PR significantly increases its impact by routing streamed Pages SSR responses through this path. Consider adding cancelResponseBody(webResponse) in the HEAD branch of sendWebResponse (around line 599).
| statusOverride?: number, | ||
| ): Response { | ||
| const status = statusOverride ?? response.status; | ||
| const mergedHeaders = mergeResponseHeaders(middlewareHeaders, response); |
There was a problem hiding this comment.
Minor: mergeResponseHeaders is called unconditionally here, even for the early-return path (lines 223-230). Since it iterates all response headers and copies them into a new record, this is wasted work when the function returns the original response unchanged. The early-return optimization is weaker than it appears because the checks for shouldDropBody and shouldStripStreamLength depend on mergedHeaders. Not a big deal for correctness, just worth noting.
| if (!Object.keys(extraHeaders).length && !statusOverride) return response; | ||
| const NO_BODY_RESPONSE_STATUSES = new Set([204, 205, 304]); | ||
| function isVinextStreamedHtmlResponse(response: Response): boolean { | ||
| return response.__vinextStreamedHtmlResponse === true; |
There was a problem hiding this comment.
Nit: In the deploy.ts template, response.__vinextStreamedHtmlResponse is accessed without a type cast (unlike worker-utils.ts and prod-server.ts which use (response as ResponseWithVinextStreamingMetadata).__vinextStreamedHtmlResponse). This works because the template is emitted as plain JS, but the inconsistency is slightly surprising when reading the three implementations side-by-side. A comment noting this is intentional (template JS, no TS narrowing needed) would help future readers.
| }); | ||
| // Mark the normal streamed HTML render so the Node prod server can strip | ||
| // stale Content-Length only for this path, not for custom gSSP responses. | ||
| streamedPageResponse.__vinextStreamedHtmlResponse = true; |
There was a problem hiding this comment.
Using an ad-hoc expando property on the Response object to pass metadata across module boundaries works but is fragile -- it will be silently lost if anything clones the response (e.g., response.clone(), or constructing new Response(response.body, ...)). This is fine for the current code paths (and mergeWebResponse in prod-server.ts correctly checks the flag before creating a new Response), but it's worth documenting this invariant somewhere so future code doesn't accidentally drop the flag.
| const finalStatus = middlewareRewriteStatus ?? response.status; | ||
| const finalStatusText = | ||
| finalStatus === response.status ? response.statusText || undefined : undefined; | ||
| const shouldStreamPagesResponse = isVinextStreamedHtmlResponse(response); |
There was a problem hiding this comment.
The shouldStreamPagesResponse flag is captured from the original response before mergeWebResponse creates a new one. This is correct (since mergeWebResponse constructs new Response(...) which drops the expando), but subtle. A brief comment explaining the ordering would help:
| const shouldStreamPagesResponse = isVinextStreamedHtmlResponse(response); | |
| // Check the original response before merging -- mergeWebResponse creates a new Response | |
| // which drops the __vinextStreamedHtmlResponse expando property. | |
| const shouldStreamPagesResponse = isVinextStreamedHtmlResponse(response); |
| const buf = typeof body === "string" ? Buffer.from(body) : body; | ||
| const baseType = contentType.split(";")[0].trim(); | ||
| const encoding = compress ? negotiateEncoding(req) : null; | ||
| const { |
There was a problem hiding this comment.
Good change -- stripping both content-type and content-length from extraHeaders before sendCompressed applies its own is correct. Previously only content-length was stripped in the uncompressed branch, so a stale Content-Type from middleware/gSSP could leak through the spread in the compressed branch.
| }); | ||
|
|
||
| it("streams Pages SSR responses incrementally in production", async () => { | ||
| // Parity target: Next.js streams Node responses via sendResponse() -> |
There was a problem hiding this comment.
Solid test -- verifying the snapshot at a time delay to confirm incremental streaming behavior is a nice approach. The 600ms setTimeout in the fixture + 120ms snapshot delay gives a good window.
One concern: in CI under load, the 120ms snapshot delay could fire after the full response arrives if the 600ms timer resolves faster than expected (timers aren't precise). If this test becomes flaky, consider increasing the fixture's delay or asserting on the snapshot vs final body difference rather than exact content presence/absence.
|
Review posted successfully. Here's a summary of the feedback: Key findings:
|
2 participants
Fix the Pages Router Node production SSR path so responses stay streamed instead of being fully buffered before send.
Previously, the Pages SSR path in
prod-server.tsdrained the renderedResponsewithresponse.arrayBuffer()before writing it to the socket. That removed progressive streaming, copied the full HTML payload, and increased TTFB and memory usage on large responses.This change preserves the original
Response.bodystream through the existing streamed Node response path and fixes the related response-merge edge cases that showed up once streaming was preserved.Changes
204,205,304) correctly when middleware rewrites a streamed Pages SSR responseContent-Lengthfrom streamed Pages SSR responses, including values inherited from:getServerSidePropsContent-LengthfromgetServerSidePropsContent-Lengthfrom middleware rewritesWhy
Pages Router SSR already produces a streamed
Response, but the Node prod server was materializing that stream into a full buffer before sending it.This PR preserves streaming behavior and also handles the edge cases with the streamed merge path:
204,205, and304Content-Lengthon streamed SSR responsesTesting
pnpm test tests/pages-router.test.tspnpm test tests/features.test.ts -t "mergeWebResponse"pnpm run fmtpnpm run typecheck