Skip to content

fix: Pages Router SSR streaming #514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JaredStowell wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix: Pages Router SSR streaming #514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
914e483
fix pages router streaming
JaredStowell Mar 13, 2026
4565b58
handle regressions
JaredStowell Mar 13, 2026
25dc13a
fix: align pages response merge behavior
JaredStowell Mar 17, 2026
d2b4655
fix: drop stale worker content-length headers
JaredStowell Mar 17, 2026
4877173
fix: preserve no-body rewrite parity
JaredStowell Mar 17, 2026
46e7de5
Merge upstream/main into jstowell/pages-prod-ssr-streaming
JaredStowell Mar 17, 2026
7def74c
fix: cancel streamed HEAD responses
JaredStowell Mar 17, 2026
552e07c
test: support merged prod server shape
JaredStowell Mar 17, 2026
f0dd479
Merge upstream/main into jstowell/pages-prod-ssr-streaming
JaredStowell Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 52 additions & 4 deletions packages/vinext/src/deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,17 +813,34 @@ export default {
* Response headers take precedence over middleware headers for all headers
* except Set-Cookie, which is additive (both middleware and response cookies
* are preserved). Matches the behavior in prod-server.ts. Uses getSetCookie()
* to preserve multiple Set-Cookie values.
* to preserve multiple Set-Cookie values. Keep this in sync with
* prod-server.ts and server/worker-utils.ts.
*/
function mergeHeaders(
response: Response,
extraHeaders: Record<string, string | string[]>,
statusOverride?: number,
): Response {
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;
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ask-bonk ask-bonk bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
function isContentLengthHeader(name: string): boolean {
return name.toLowerCase() === "content-length";
}
function cancelResponseBody(response: Response): void {
const body = response.body;
if (!body || body.locked) return;
void body.cancel().catch(() => {
/* ignore cancellation failures on discarded bodies */
});
}

const status = statusOverride ?? response.status;
const merged = new Headers();
// Middleware/config headers go in first (lower precedence)
for (const [k, v] of Object.entries(extraHeaders)) {
if (isContentLengthHeader(k)) continue;
if (Array.isArray(v)) {
for (const item of v) merged.append(k, item);
} else {
Expand All @@ -838,9 +855,40 @@ function mergeHeaders(
});
const responseCookies = response.headers.getSetCookie?.() ?? [];
for (const cookie of responseCookies) merged.append("set-cookie", cookie);

const shouldDropBody = NO_BODY_RESPONSE_STATUSES.has(status);
const shouldStripStreamLength =
isVinextStreamedHtmlResponse(response) && merged.has("content-length");

if (
!Object.keys(extraHeaders).some((key) => !isContentLengthHeader(key)) &&
statusOverride === undefined &&
!shouldDropBody &&
!shouldStripStreamLength
) {
return response;
}

if (shouldDropBody) {
cancelResponseBody(response);
merged.delete("content-encoding");
merged.delete("content-length");
merged.delete("content-type");
merged.delete("transfer-encoding");
return new Response(null, {
status,
statusText: status === response.status ? response.statusText : undefined,
headers: merged,
});
}

if (shouldStripStreamLength) {
merged.delete("content-length");
}

return new Response(response.body, {
status: statusOverride ?? response.status,
statusText: response.statusText,
status,
statusText: status === response.status ? response.statusText : undefined,
headers: merged,
});
}
Expand Down
9 changes: 8 additions & 1 deletion packages/vinext/src/entries/pages-server-entry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1108,7 +1108,14 @@ async function _renderPage(request, url, manifest) {
if (_fontLinkHeader) {
responseHeaders.set("Link", _fontLinkHeader);
}
return new Response(compositeStream, { status: finalStatus, headers: responseHeaders });
const streamedPageResponse = new Response(compositeStream, {
status: finalStatus,
headers: responseHeaders,
});
// 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;
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ask-bonk ask-bonk bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return streamedPageResponse;
} catch (e) {
console.error("[vinext] SSR error:", e);
_reportRequestError(
Expand Down
163 changes: 142 additions & 21 deletions packages/vinext/src/server/prod-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,107 @@ function mergeResponseHeaders(
return merged;
}

function toWebHeaders(headersRecord: Record<string, string | string[]>): Headers {
const headers = new Headers();
for (const [key, value] of Object.entries(headersRecord)) {
if (Array.isArray(value)) {
for (const item of value) headers.append(key, item);
} else {
headers.set(key, value);
}
}
return headers;
}

const NO_BODY_RESPONSE_STATUSES = new Set([204, 205, 304]);

function hasHeader(headersRecord: Record<string, string | string[]>, name: string): boolean {
const target = name.toLowerCase();
return Object.keys(headersRecord).some((key) => key.toLowerCase() === target);
}

function stripHeaders(
headersRecord: Record<string, string | string[]>,
names: readonly string[],
): void {
const targets = new Set(names.map((name) => name.toLowerCase()));
for (const key of Object.keys(headersRecord)) {
if (targets.has(key.toLowerCase())) delete headersRecord[key];
}
}

function isNoBodyResponseStatus(status: number): boolean {
return NO_BODY_RESPONSE_STATUSES.has(status);
}

function cancelResponseBody(response: Response): void {
const body = response.body;
if (!body || body.locked) return;
void body.cancel().catch(() => {
/* ignore cancellation failures on discarded bodies */
});
}

type ResponseWithVinextStreamingMetadata = Response & {
__vinextStreamedHtmlResponse?: boolean;
};

function isVinextStreamedHtmlResponse(response: Response): boolean {
return (response as ResponseWithVinextStreamingMetadata).__vinextStreamedHtmlResponse === true;
}

/**
* Merge middleware/config headers and an optional status override into a new
* Web Response while preserving the original body stream when allowed.
* Keep this in sync with server/worker-utils.ts and the generated copy in
* deploy.ts.
*/
function mergeWebResponse(
middlewareHeaders: Record<string, string | string[]>,
response: Response,
statusOverride?: number,
): Response {
const status = statusOverride ?? response.status;
const mergedHeaders = mergeResponseHeaders(middlewareHeaders, response);
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ask-bonk ask-bonk bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Adding content-length filtering to mergeResponseHeaders (or a wrapper around it)
  2. Stripping middleware content-length from middlewareHeaders before passing it to mergeResponseHeaders

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.

const shouldDropBody = isNoBodyResponseStatus(status);
const shouldStripStreamLength =
isVinextStreamedHtmlResponse(response) && hasHeader(mergedHeaders, "content-length");

if (
!Object.keys(middlewareHeaders).length &&
statusOverride === undefined &&
!shouldDropBody &&
!shouldStripStreamLength
) {
return response;
}

if (shouldDropBody) {
cancelResponseBody(response);
stripHeaders(mergedHeaders, [
"content-encoding",
"content-length",
"content-type",
"transfer-encoding",
]);
return new Response(null, {
status,
statusText: status === response.status ? response.statusText : undefined,
headers: toWebHeaders(mergedHeaders),
});
}

if (shouldStripStreamLength) {
stripHeaders(mergedHeaders, ["content-length"]);
}

return new Response(response.body, {
status,
statusText: status === response.status ? response.statusText : undefined,
headers: toWebHeaders(mergedHeaders),
});
}

/**
* Send a compressed response if the content type is compressible and the
* client supports compression. Otherwise send uncompressed.
Expand All @@ -174,6 +275,13 @@ function sendCompressed(
const buf = typeof body === "string" ? Buffer.from(body) : body;
const baseType = contentType.split(";")[0].trim();
const encoding = compress ? negotiateEncoding(req) : null;
const {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"content-length": _cl,
"Content-Length": _CL,
"content-type": _ct,
"Content-Type": _CT,
...headersWithoutBodyHeaders
} = extraHeaders;
Comment on lines +278 to +284
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const writeHead = (headers: Record<string, string | string[]>) => {
if (statusText) {
Expand All @@ -200,7 +308,7 @@ function sendCompressed(
varyValue = "Accept-Encoding";
}
writeHead({
...extraHeaders,
...headersWithoutBodyHeaders,
"Content-Type": contentType,
"Content-Encoding": encoding,
Vary: varyValue,
Expand All @@ -210,11 +318,8 @@ function sendCompressed(
/* ignore pipeline errors on closed connections */
});
} else {
// Strip any pre-existing content-length (from the Web Response constructor)
// before setting our own — avoids duplicate Content-Length headers.
const { "content-length": _cl, "Content-Length": _CL, ...headersWithoutLength } = extraHeaders;
writeHead({
...headersWithoutLength,
...headersWithoutBodyHeaders,
"Content-Type": contentType,
"Content-Length": String(buf.length),
});
Expand Down Expand Up @@ -494,6 +599,7 @@ async function sendWebResponse(

// HEAD requests: send headers only, skip the body
if (req.method === "HEAD") {
cancelResponseBody(webResponse);
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

res.end();
return;
}
Expand Down Expand Up @@ -1180,23 +1286,31 @@ async function startPagesRouterServer(options: PagesRouterServerOptions) {
response = new Response("404 - API route not found", { status: 404 });
}

// Merge middleware + config headers into the response
const responseBody = Buffer.from(await response.arrayBuffer());
const mergedResponse = mergeWebResponse(
middlewareHeaders,
response,
middlewareRewriteStatus,
);

if (!mergedResponse.body) {
await sendWebResponse(mergedResponse, req, res, compress);
return;
}

const responseBody = Buffer.from(await mergedResponse.arrayBuffer());
// API routes may return arbitrary data (JSON, binary, etc.), so
// default to application/octet-stream rather than text/html when
// the handler doesn't set an explicit Content-Type.
const ct = response.headers.get("content-type") ?? "application/octet-stream";
const responseHeaders = mergeResponseHeaders(middlewareHeaders, response);
const finalStatus = middlewareRewriteStatus ?? response.status;
const finalStatusText =
finalStatus === response.status ? response.statusText || undefined : undefined;
const ct = mergedResponse.headers.get("content-type") ?? "application/octet-stream";
const responseHeaders = mergeResponseHeaders({}, mergedResponse);
const finalStatusText = mergedResponse.statusText || undefined;

sendCompressed(
req,
res,
responseBody,
ct,
finalStatus,
mergedResponse.status,
responseHeaders,
compress,
finalStatusText,
Expand Down Expand Up @@ -1247,20 +1361,25 @@ async function startPagesRouterServer(options: PagesRouterServerOptions) {
return;
}

// Merge middleware + config headers into the response
const responseBody = Buffer.from(await response.arrayBuffer());
const ct = response.headers.get("content-type") ?? "text/html";
const responseHeaders = mergeResponseHeaders(middlewareHeaders, response);
const finalStatus = middlewareRewriteStatus ?? response.status;
const finalStatusText =
finalStatus === response.status ? response.statusText || undefined : undefined;
const shouldStreamPagesResponse = isVinextStreamedHtmlResponse(response);
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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);

Copy link
Contributor

@ask-bonk ask-bonk bot Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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 mergedResponse = mergeWebResponse(middlewareHeaders, response, middlewareRewriteStatus);

if (shouldStreamPagesResponse || !mergedResponse.body) {
await sendWebResponse(mergedResponse, req, res, compress);
Comment on lines +1367 to +1368
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Cancel page render stream on HEAD in streamed Pages path

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 👍 / 👎.

Copy link
Contributor

@ask-bonk ask-bonk bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return;
}

const responseBody = Buffer.from(await mergedResponse.arrayBuffer());
const ct = mergedResponse.headers.get("content-type") ?? "text/html";
const responseHeaders = mergeResponseHeaders({}, mergedResponse);
const finalStatusText = mergedResponse.statusText || undefined;

sendCompressed(
req,
res,
responseBody,
ct,
finalStatus,
mergedResponse.status,
responseHeaders,
compress,
finalStatusText,
Expand Down Expand Up @@ -1291,6 +1410,7 @@ async function startPagesRouterServer(options: PagesRouterServerOptions) {
// Export helpers for testing
export {
sendCompressed,
sendWebResponse,
negotiateEncoding,
COMPRESSIBLE_TYPES,
COMPRESS_THRESHOLD,
Expand All @@ -1299,4 +1419,5 @@ export {
trustProxy,
nodeToWebRequest,
mergeResponseHeaders,
mergeWebResponse,
};
Loading
Loading