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 12 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
12 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
596f0ae
Fix compressed Pages SSR streaming
JaredStowell Mar 21, 2026
f1aa868
Merge upstream/main into jstowell/pages-prod-ssr-streaming
JaredStowell Mar 21, 2026
ceab6cc
Fix ecosystem fixture port collision
JaredStowell Mar 21, 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 @@ -1109,7 +1109,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
193 changes: 169 additions & 24 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 @@ -113,18 +113,26 @@ function negotiateEncoding(req: IncomingMessage): "br" | "gzip" | "deflate" | nu
*/
function createCompressor(
encoding: "br" | "gzip" | "deflate",
mode: "default" | "streaming" = "default",
): zlib.BrotliCompress | zlib.Gzip | zlib.Deflate {
switch (encoding) {
case "br":
return zlib.createBrotliCompress({
...(mode === "streaming" ? { flush: zlib.constants.BROTLI_OPERATION_FLUSH } : {}),
params: {
[zlib.constants.BROTLI_PARAM_QUALITY]: 4, // Fast compression (1-11, 4 is a good balance)
},
});
case "gzip":
return zlib.createGzip({ level: 6 }); // Default level, good balance
return zlib.createGzip({
level: 6,
...(mode === "streaming" ? { flush: zlib.constants.Z_SYNC_FLUSH } : {}),
}); // Default level, good balance
case "deflate":
return zlib.createDeflate({ level: 6 });
return zlib.createDeflate({
level: 6,
...(mode === "streaming" ? { flush: zlib.constants.Z_SYNC_FLUSH } : {}),
});
}
}

Expand Down Expand Up @@ -157,6 +165,123 @@ 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 omitHeadersCaseInsensitive(
headersRecord: Record<string, string | string[]>,
names: readonly string[],
): Record<string, string | string[]> {
const targets = new Set(names.map((name) => name.toLowerCase()));
const filtered: Record<string, string | string[]> = {};
for (const [key, value] of Object.entries(headersRecord)) {
if (targets.has(key.toLowerCase())) continue;
filtered[key] = value;
}
return filtered;
}

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 filteredMiddlewareHeaders = omitHeadersCaseInsensitive(middlewareHeaders, [
"content-length",
]);
const status = statusOverride ?? response.status;
const mergedHeaders = mergeResponseHeaders(filteredMiddlewareHeaders, response);
const shouldDropBody = isNoBodyResponseStatus(status);
const shouldStripStreamLength =
isVinextStreamedHtmlResponse(response) && hasHeader(mergedHeaders, "content-length");

if (
!Object.keys(filteredMiddlewareHeaders).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 +299,10 @@ function sendCompressed(
const buf = typeof body === "string" ? Buffer.from(body) : body;
const baseType = contentType.split(";")[0].trim();
const encoding = compress ? negotiateEncoding(req) : null;
const headersWithoutBodyHeaders = omitHeadersCaseInsensitive(extraHeaders, [
"content-length",
"content-type",
]);

const writeHead = (headers: Record<string, string | string[]>) => {
if (statusText) {
Expand All @@ -200,7 +329,7 @@ function sendCompressed(
varyValue = "Accept-Encoding";
}
writeHead({
...extraHeaders,
...headersWithoutBodyHeaders,
"Content-Type": contentType,
"Content-Encoding": encoding,
Vary: varyValue,
Expand All @@ -210,11 +339,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 +620,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 All @@ -503,7 +630,9 @@ async function sendWebResponse(
const nodeStream = Readable.fromWeb(webResponse.body as import("stream/web").ReadableStream);

if (shouldCompress) {
const compressor = createCompressor(encoding!);
// Use streaming flush modes so progressive HTML remains decodable before the
// full response completes.
const compressor = createCompressor(encoding!, "streaming");
pipeline(nodeStream, compressor, res, () => {
/* ignore pipeline errors on closed connections */
});
Expand Down Expand Up @@ -1203,23 +1332,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 @@ -1270,20 +1407,26 @@ 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;
// Capture the streaming marker before mergeWebResponse rebuilds the Response.
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 +1414 to +1415
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 @@ -1314,6 +1457,7 @@ async function startPagesRouterServer(options: PagesRouterServerOptions) {
// Export helpers for testing
export {
sendCompressed,
sendWebResponse,
negotiateEncoding,
COMPRESSIBLE_TYPES,
COMPRESS_THRESHOLD,
Expand All @@ -1322,4 +1466,5 @@ export {
trustProxy,
nodeToWebRequest,
mergeResponseHeaders,
mergeWebResponse,
};
Loading
Loading