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:
|
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: Pages Router SSR Streaming
The core change is well-motivated and the implementation is thorough. Preserving the Response.body stream through sendWebResponse instead of buffering via arrayBuffer() is the right fix for TTFB and memory on streamed Pages SSR. The edge-case handling for no-body statuses and stale Content-Length is comprehensive, and the test coverage is excellent.
I found one correctness issue, one subtle sync divergence between the three merge implementations, and a few minor observations.
Issue 1: mergeWebResponse doesn't filter middleware content-length from the merge (correctness / sync divergence)
The worker-utils.ts and deploy.ts merge implementations both filter out middleware content-length unconditionally during the initial header merge loop (if (isContentLengthHeader(k)) continue). But prod-server.ts's mergeWebResponse delegates to mergeResponseHeaders, which spreads all middleware headers (including content-length) into the merged record with no filtering.
In practice this is currently safe because:
- Streamed HTML responses hit
shouldStripStreamLengthwhich removes it - Non-streamed SSR responses go through
sendCompressedwhich now strips content-length from extraHeaders - No-body statuses strip it explicitly
But it's a latent inconsistency: if any future code path reads the merged headers from mergeWebResponse without going through sendCompressed or sendWebResponse, a middleware content-length will leak through. The other two implementations prevent this at the source.
The early-return check also differs: prod-server.ts uses !Object.keys(middlewareHeaders).length while the others use !Object.keys(extraHeaders).some(key => !isContentLengthHeader(key)). If middleware only sets content-length, prod-server.ts skips the early return and creates a new Response unnecessarily.
Not a blocker since the downstream consumers currently handle it, but worth aligning for defensive correctness.
Issue 2: sendWebResponse HEAD path writes headers before canceling the body (ordering nit)
At line 598-604, writeHead(nodeHeaders) is called, then the HEAD check cancels the body and calls res.end(). The cancelResponseBody addition is good (the previous review flagged the leak and this PR fixes it), but writeHead runs unconditionally, including the compression negotiation logic at lines 580-596 which deletes content-length from headers and adds Content-Encoding/Vary for compressible types.
For HEAD requests, this means the client receives Content-Encoding: br and Vary: Accept-Encoding headers even though no body (and no compression) is applied. This is technically correct per HTTP (HEAD should return headers as if it were a GET), but it's worth noting that the compression-related header mutation runs even when the body is never sent. Minor -- just an observation, not a change request.
Minor observations
See inline comments.
| statusOverride?: number, | ||
| ): Response { | ||
| const status = statusOverride ?? response.status; | ||
| const mergedHeaders = mergeResponseHeaders(middlewareHeaders, response); |
There was a problem hiding this comment.
mergeResponseHeaders at line 140 does { ...middlewareHeaders } which copies ALL middleware headers (including content-length) into the merged record. In contrast, worker-utils.ts and deploy.ts both skip middleware content-length during the merge loop:
if (isContentLengthHeader(k)) continue;This means mergeWebResponse can produce a merged record that still contains a stale middleware content-length for non-streamed, non-no-body responses. Currently mitigated by sendCompressed stripping it downstream, but it's a latent inconsistency with the other two implementations.
Consider either:
- Adding content-length filtering to
mergeResponseHeaders(or a wrapper around it) - Stripping middleware content-length from
middlewareHeadersbefore passing it tomergeResponseHeaders
This would also let the early-return check at line 227 align with the !Object.keys(extraHeaders).some(key => !isContentLengthHeader(key)) pattern used in the other two implementations.
|
|
||
| // HEAD requests: send headers only, skip the body | ||
| if (req.method === "HEAD") { | ||
| cancelResponseBody(webResponse); |
There was a problem hiding this comment.
Good fix -- this addresses the HEAD request body leak that the previous review flagged. The SSR render stream is now properly canceled instead of left running with no downstream reader.
One subtlety: writeHead at line 598 has already been called at this point, including the compression negotiation that may have mutated nodeHeaders (deleted content-length, added Content-Encoding). For HEAD requests this is technically fine (HEAD should mirror GET headers), but it means the content-length that the client sees for HEAD is whatever was in the response headers minus the compression stripping. If a future caller relies on HEAD returning an accurate Content-Length for range-request preflight, this could be surprising.
| 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 ordering here is critical and correct -- isVinextStreamedHtmlResponse must check the original response before mergeWebResponse creates new Response(...) which drops the expando. A short comment documenting this invariant would help future readers:
| const shouldStreamPagesResponse = isVinextStreamedHtmlResponse(response); | |
| // Capture the streaming flag before merging -- mergeWebResponse creates a new Response | |
| // which drops the __vinextStreamedHtmlResponse expando property. | |
| const shouldStreamPagesResponse = isVinextStreamedHtmlResponse(response); |
| const { | ||
| "content-length": _cl, | ||
| "Content-Length": _CL, | ||
| "content-type": _ct, | ||
| "Content-Type": _CT, | ||
| ...headersWithoutBodyHeaders | ||
| } = extraHeaders; |
There was a problem hiding this comment.
Good change -- stripping both content-type and content-length from extraHeaders before sendCompressed applies its own values is correct. Previously only content-length was stripped in the uncompressed branch, so a stale Content-Type from middleware or gSSP could leak through in the compressed branch.
Note that this uses exact-case destructuring ("content-length" and "Content-Length"). Since mergeResponseHeaders uses Headers.forEach() which yields lowercase keys, the lowercase variants will catch the response's own headers. But if middleware headers are in some other casing (e.g., "Content-length"), neither destructuring key would match. In practice Next.js middleware uses Headers which normalizes to lowercase, so this should be fine.
| }); | ||
| // 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 works but is inherently fragile -- it will be silently dropped by response.clone(), new Response(response.body, ...), or anything that reconstructs the response. The code currently handles this correctly (checking the flag before merging), but it's worth documenting this invariant.
Consider adding a comment here like:
// WARNING: This expando property is lost if the Response is cloned or reconstructed.
// All consumers must check it before creating new Response objects from this one.
Alternatively, a custom response header (e.g., x-vinext-streamed: 1) would survive cloning and reconstruction, though it adds a header to the wire response that would need to be stripped before sending.
| 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 as ResponseWithVinextStreamingMetadata). This is intentional since the template is emitted as plain JS, but a brief comment noting this would help readers who are cross-referencing the three implementations.
| }); | ||
|
|
||
| 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.
The streaming snapshot test is a great approach. One concern for CI reliability: the fixture uses a 600ms setTimeout and the snapshot is captured at 120ms after the first chunk. Under CI load, timer precision can slip -- if the 600ms timer fires early, the snapshot might include the delayed content, causing a false failure on the not.toContain assertion.
If this becomes flaky, consider either:
- Increasing the fixture delay to 2-3 seconds
- Asserting on
snapshot.length < body.length(size-based) rather than exact content presence/absence
Not a blocker -- just flagging for future reference.
| const status = statusOverride ?? response.status; | ||
| const merged = new Headers(); | ||
| for (const [k, v] of Object.entries(extraHeaders)) { | ||
| if (isContentLengthHeader(k)) continue; |
There was a problem hiding this comment.
This unconditionally drops middleware content-length, which is correct for the merge step. But it means that even if middleware intentionally sets content-length for a non-streamed response (e.g., a custom binary middleware response), it gets stripped. In practice this is fine because the response's own Content-Length (from the Response constructor) takes precedence via merged.set(k, v) in the response header loop below. Just noting the semantic: middleware can never influence Content-Length, which matches Next.js behavior.
|
Review submitted successfully. Here's a summary of what I found: 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