From de830c80e95b7a956f3ee8e02221dc9cf8308751 Mon Sep 17 00:00:00 2001 From: Mathias Biilmann Date: Sun, 28 Jun 2026 20:31:35 -0700 Subject: [PATCH 1/4] fix: terminate streamed responses cleanly when a render fails A streamed SSR response commits its headers (typically a 200, with no Content-Length) as soon as Next.js flushes them, well before the body finishes. If the render then fails *after* that point, response finalization had no error path: - a render that aborts the response mid-stream leaves the body hung open forever (the @fastly/http-compute-js source stream never closes), and - a render that throws hits the catch which sets statusCode=500 and writes "Internal Server Error" - but the status change is a no-op once headers are sent, so the client receives a 200 with the error string concatenated onto partial HTML (a garbled but "complete" response). Both reach the edge as an unparseable HTTP message, which ATS reports as a 502 (ats_status_502_invalid_http_response). Exposure scales with how long the response stream stays open, i.e. with pages that do a lot of async work during render. Fix: - When the handler fails after headers are committed, end the response cleanly (so buffered bytes flush in order) and record the error instead of appending "Internal Server Error". - Replace the body TransformStream (flush-only, no error path) with a ReadableStream that surfaces a recorded render error to the platform and, if the response was destroyed without a clean end, errors promptly instead of hanging. The happy path is unchanged (full body, then background work, then close). Adds regression tests that mirror the finalization pipeline. Co-Authored-By: Claude Opus 4.8 --- .../server-streaming-termination.test.ts | 239 ++++++++++++++++++ src/run/handlers/server.ts | 88 ++++++- 2 files changed, 317 insertions(+), 10 deletions(-) create mode 100644 src/run/handlers/server-streaming-termination.test.ts diff --git a/src/run/handlers/server-streaming-termination.test.ts b/src/run/handlers/server-streaming-termination.test.ts new file mode 100644 index 0000000000..3452009c6e --- /dev/null +++ b/src/run/handlers/server-streaming-termination.test.ts @@ -0,0 +1,239 @@ +import type { OutgoingHttpHeaders } from 'node:http' + +import { ComputeJsOutgoingMessage, toComputeResponse, toReqRes } from '@fastly/http-compute-js' +import { describe, expect, test } from 'vitest' + +/** + * Regression tests for streamed-response termination on render failure. + * + * A streamed SSR response commits its headers (typically a 200, with no + * Content-Length) as soon as Next.js flushes them - well before the body + * finishes. If the render then fails *after* that point, the response can no + * longer become a 5xx, and the body must terminate cleanly or the edge sees an + * unparseable HTTP message (which ATS reports as a 502, + * ats_status_502_invalid_http_response). + * + * This faithfully mirrors the response-finalization pipeline in + * `src/run/handlers/server.ts`: + * - `disableFaultyTransferEncodingHandling` (verbatim) + * - kicking off the next handler without awaiting it, capturing a late + * render failure and ending the response once headers are committed + * - `toComputeResponse(resProxy)` which resolves when HEADERS are available, + * NOT when the body stream closes + * - the body ReadableStream that keeps the response open until the render + + * background work finish, but errors the stream on a failed/aborted render + * + * We do not import server.ts directly because its module init does top-level + * `await getRunConfig()` + Next.js imports. The streaming/termination logic + * below is kept in lock-step with server.ts so the test exercises the real shape. + * + * Before the fix: + * - a mid-stream abort after headers -> response HUNG open indefinitely + * - a throw after headers -> client got a 200 with "Internal Server Error" + * concatenated onto partial HTML (a garbled but "complete" 200) + * + * After the fix: a failed/aborted render terminates the response stream + * cleanly (errors), so the platform aborts the connection instead of emitting + * an unparseable "success" or hanging. + */ + +// --- verbatim from server.ts --- +const disableFaultyTransferEncodingHandling = (res: ComputeJsOutgoingMessage) => { + const originalStoreHeader = res._storeHeader + res._storeHeader = function _storeHeader(firstLine, headers) { + if (headers) { + if (Array.isArray(headers)) { + // eslint-disable-next-line no-param-reassign + headers = headers.filter(([header]) => header.toLowerCase() !== 'transfer-encoding') + } else { + delete (headers as OutgoingHttpHeaders)['transfer-encoding'] + } + } + return originalStoreHeader.call(this, firstLine, headers) + } +} + +type FakeNextHandler = (res: import('node:http').ServerResponse) => Promise + +/** + * Mirror of the response-finalization half of server.ts's default export. + * `render` plays the role of `nextHandler(req, resProxy)`. + */ +async function finalizeResponseLikeServerHandler(request: Request, render: FakeNextHandler) { + const { req, res } = toReqRes(request) + + Object.defineProperty(req, 'connection', { get: () => ({}) }) + Object.defineProperty(req, 'socket', { get: () => ({}) }) + + disableFaultyTransferEncodingHandling(res as unknown as ComputeJsOutgoingMessage) + + let handlerError: unknown + const nextHandlerPromise = render(res).catch((error) => { + if (res.headersSent) { + handlerError = error instanceof Error ? error : new Error(String(error)) + res.end() + } else { + res.statusCode = 500 + res.end('Internal Server Error') + } + }) + + const response = await toComputeResponse(res) + + async function waitForBackgroundWork() { + await nextHandlerPromise + res.emit('close') + } + + if (!response.body) { + await waitForBackgroundWork() + return new Response(null, response) + } + + const reader = response.body.getReader() + + const responseBody = new ReadableStream({ + start(controller) { + nextHandlerPromise.then(() => { + if (res.destroyed && !res.writableEnded) { + const abortReason = + handlerError ?? new Error('Response stream was destroyed before the render completed') + try { + controller.error(abortReason) + } catch { + // already closed/errored + } + } + }) + }, + async pull(controller) { + try { + const { done, value } = await reader.read() + if (done) { + await waitForBackgroundWork() + if (handlerError) controller.error(handlerError) + else controller.close() + return + } + controller.enqueue(value) + } catch (error) { + try { + controller.error(error) + } catch { + // already errored + } + } + }, + async cancel(reason) { + await reader.cancel(reason).catch(() => {}) + }, + }) + + return new Response(responseBody, response) +} + +type DrainResult = + | { outcome: 'complete'; body: string } + | { outcome: 'errored'; partial: string; error: unknown } + | { outcome: 'hung'; partial: string } + +/** + * Drain the response body, but give up after `hangAfterMs`. The pre-fix bug + * manifested as a body stream that never terminated, so the bound is required + * to keep the assertion deterministic. + */ +async function drainBody(response: Response, hangAfterMs = 500): Promise { + if (!response.body) return { outcome: 'complete', body: '' } + const reader = response.body.getReader() + const decoder = new TextDecoder() + let body = '' + try { + for (;;) { + const timeout = new Promise<'hung'>((resolve) => + setTimeout(() => resolve('hung'), hangAfterMs), + ) + const next = await Promise.race([reader.read(), timeout]) + if (next === 'hung') { + reader.cancel().catch(() => {}) + return { outcome: 'hung', partial: body } + } + const { done, value } = next + if (done) break + if (value) body += decoder.decode(value, { stream: true }) + } + return { outcome: 'complete', body } + } catch (error) { + return { outcome: 'errored', partial: body, error } + } +} + +describe('streamed response termination', () => { + test('baseline: a render that completes cleanly produces a well-framed 200', async () => { + const request = new Request('https://example.netlify.app/page/') + + const response = await finalizeResponseLikeServerHandler(request, async (res) => { + res.writeHead(200, { 'content-type': 'text/html' }) + res.write('') + res.write('fully rendered page') + res.end('') + }) + + expect(response.status).toBe(200) + const drained = await drainBody(response) + expect(drained.outcome).toBe('complete') + if (drained.outcome === 'complete') { + expect(drained.body).toBe('fully rendered page') + } + }) + + test('render aborts mid-stream after headers committed -> stream errors cleanly (no hang)', async () => { + const request = new Request('https://example.netlify.app/heavy-page/') + + // headers + opening HTML flush early, then a later async step (e.g. a data + // fetch) aborts the render. The handler promise does NOT reject: it just + // destroys the response. + const response = await finalizeResponseLikeServerHandler(request, async (res) => { + res.writeHead(200, { 'content-type': 'text/html' }) + res.write('') + await new Promise((r) => setTimeout(r, 10)) + res.destroy(new Error('stream aborted mid-render')) + }) + + // Headers were already committed as 200; we can't retroactively change that. + expect(response.status).toBe(200) + expect(response.headers.get('content-length')).toBeNull() + expect(response.headers.get('transfer-encoding')).toBeNull() + + const drained = await drainBody(response) + // The response no longer hangs - it errors promptly and deterministically + // so the platform aborts the connection (an explicit incomplete signal) + // rather than holding an unterminated stream open until ATS times it out. + expect(drained.outcome).toBe('errored') + if (drained.outcome === 'errored') { + expect(drained.partial.includes('')).toBe(false) + } + }) + + test('render throws after headers committed -> stream errors cleanly, no garbled 200 body', async () => { + const request = new Request('https://example.netlify.app/throws/') + + const response = await finalizeResponseLikeServerHandler(request, async (res) => { + res.writeHead(200, { 'content-type': 'text/html' }) + res.write('partial') + throw new Error('render failed mid-stream') + }) + + // Headers already committed; status stays 200 but the body must abort. + expect(response.status).toBe(200) + + const drained = await drainBody(response) + // Instead of a "complete" 200 with "Internal Server Error" concatenated onto + // the partial HTML, the stream errors. Whatever bytes arrive first are a + // truncated page; the error string is never appended. + expect(drained.outcome).toBe('errored') + if (drained.outcome === 'errored') { + expect(drained.partial).not.toContain('Internal Server Error') + expect(drained.partial.includes('')).toBe(false) + } + }) +}) diff --git a/src/run/handlers/server.ts b/src/run/handlers/server.ts index af509e8344..7f8d83d128 100644 --- a/src/run/handlers/server.ts +++ b/src/run/handlers/server.ts @@ -100,12 +100,27 @@ export default async ( const resProxy = augmentNextResponse(res, requestContext) // We don't await this here, because it won't resolve until the response is finished. + // Tracks a render failure so it can be surfaced on the response stream below. + let handlerError: unknown const nextHandlerPromise = nextHandler(req, resProxy).catch((error) => { getLogger().withError(error).error('next handler error') console.error(error) - resProxy.statusCode = 500 - span?.setAttribute('http.status_code', 500) - resProxy.end('Internal Server Error') + if (resProxy.headersSent) { + // Headers (and almost always a 200) are already on the wire, so we can + // no longer turn this into a 500. Record the error and end the response + // cleanly so buffered bytes flush in order; the body stream below then + // surfaces the error to the platform. Previously this appended + // "Internal Server Error" onto the partial response, producing a garbled + // "successful" 200 that ATS rejects as a 502 + // (ats_status_502_invalid_http_response). + handlerError = error instanceof Error ? error : new Error(String(error)) + resProxy.end() + } else { + // We can still produce a proper error response. + resProxy.statusCode = 500 + span?.setAttribute('http.status_code', 500) + resProxy.end('Internal Server Error') + } }) // Contrary to the docs, this resolves when the headers are available, not when the stream closes. @@ -183,16 +198,69 @@ export default async ( await requestContext.backgroundWorkPromise } - const keepOpenUntilNextFullyRendered = new TransformStream({ - async flush() { - await waitForBackgroundWork() - }, - }) - if (!response.body) { await waitForBackgroundWork() + return new Response(null, response) } - return new Response(response.body?.pipeThrough(keepOpenUntilNextFullyRendered), response) + const reader = response.body.getReader() + + // Stream the body through, keeping it open until the render and any tracked + // background work finish, but tie termination to the request handler so a + // failed/aborted render terminates the response *cleanly*. + // + // The previous implementation piped through a TransformStream whose flush() + // only awaited background work and had no error path. If a render aborted or + // threw after headers were already committed, the response either hung open + // (the source stream never closes) or closed as a truncated/garbled 200. + // ATS reports both as a 502 (ats_status_502_invalid_http_response). Now such + // a render errors the output stream so the platform aborts the response + // instead of emitting an unparseable "success". + const responseBody = new ReadableStream({ + start(controller) { + // If the response was destroyed mid-stream without being cleanly ended, + // the source stream never closes, so `pull` would hang forever. Once the + // handler settles we know no more bytes are coming - error the output so + // the platform aborts the connection promptly. (A handler that threw is + // handled in `pull` via the clean `end()` in the catch above.) + nextHandlerPromise.then(() => { + if (res.destroyed && !res.writableEnded) { + const abortReason = + handlerError ?? new Error('Response stream was destroyed before the render completed') + try { + controller.error(abortReason) + } catch { + // controller may already be closed or errored - nothing to do. + } + } + }) + }, + async pull(controller) { + try { + const { done, value } = await reader.read() + if (done) { + await waitForBackgroundWork() + if (handlerError) { + controller.error(handlerError) + } else { + controller.close() + } + return + } + controller.enqueue(value) + } catch (error) { + try { + controller.error(error) + } catch { + // already errored + } + } + }, + async cancel(reason) { + await reader.cancel(reason).catch(() => {}) + }, + }) + + return new Response(responseBody, response) }) } From 53be0d3c55619a4086ba6fd6c19a84ffaa891f1d Mon Sep 17 00:00:00 2001 From: pieh Date: Mon, 29 Jun 2026 09:49:30 +0200 Subject: [PATCH 2/4] test: use actual handler in the test instead of reimplementing parts of it --- .../server-streaming-termination.test.ts | 182 +++++++----------- 1 file changed, 71 insertions(+), 111 deletions(-) diff --git a/src/run/handlers/server-streaming-termination.test.ts b/src/run/handlers/server-streaming-termination.test.ts index 3452009c6e..3daa420072 100644 --- a/src/run/handlers/server-streaming-termination.test.ts +++ b/src/run/handlers/server-streaming-termination.test.ts @@ -1,7 +1,10 @@ -import type { OutgoingHttpHeaders } from 'node:http' +import type { IncomingMessage, ServerResponse } from 'node:http' -import { ComputeJsOutgoingMessage, toComputeResponse, toReqRes } from '@fastly/http-compute-js' -import { describe, expect, test } from 'vitest' +import type { Context } from '@netlify/functions' +import { describe, expect, test, vi } from 'vitest' + +import { createRequestContext, type RequestContext } from './request-context.cjs' +import handler from './server.js' /** * Regression tests for streamed-response termination on render failure. @@ -13,19 +16,19 @@ import { describe, expect, test } from 'vitest' * unparseable HTTP message (which ATS reports as a 502, * ats_status_502_invalid_http_response). * - * This faithfully mirrors the response-finalization pipeline in - * `src/run/handlers/server.ts`: - * - `disableFaultyTransferEncodingHandling` (verbatim) - * - kicking off the next handler without awaiting it, capturing a late - * render failure and ending the response once headers are committed - * - `toComputeResponse(resProxy)` which resolves when HEADERS are available, - * NOT when the body stream closes - * - the body ReadableStream that keeps the response open until the render + - * background work finish, but errors the stream on a failed/aborted render + * These tests exercise the *real* `server.ts` default export. The only thing we + * substitute is the Next.js request handler itself (the `nextHandler` that + * `getMockedRequestHandler` produces): each test injects a fake handler via + * `mockNextHandler` that plays out a specific render scenario against the + * (real) response object. Everything else - the response-finalization pipeline, + * `disableFaultyTransferEncodingHandling`, `toComputeResponse`, and the body + * `ReadableStream` that ties termination to the render - runs as it does in + * production. * - * We do not import server.ts directly because its module init does top-level - * `await getRunConfig()` + Next.js imports. The streaming/termination logic - * below is kept in lock-step with server.ts so the test exercises the real shape. + * server.ts pulls in Next.js and reads build output at module-init time, so the + * heavy boundaries it touches (config loading, the Next.js import, storage, + * tracing, header post-processing, wait-until/use-cache setup) are mocked out + * below. None of them participate in the streaming/termination logic under test. * * Before the fix: * - a mid-stream abort after headers -> response HUNG open indefinitely @@ -37,99 +40,52 @@ import { describe, expect, test } from 'vitest' * an unparseable "success" or hanging. */ -// --- verbatim from server.ts --- -const disableFaultyTransferEncodingHandling = (res: ComputeJsOutgoingMessage) => { - const originalStoreHeader = res._storeHeader - res._storeHeader = function _storeHeader(firstLine, headers) { - if (headers) { - if (Array.isArray(headers)) { - // eslint-disable-next-line no-param-reassign - headers = headers.filter(([header]) => header.toLowerCase() !== 'transfer-encoding') - } else { - delete (headers as OutgoingHttpHeaders)['transfer-encoding'] - } - } - return originalStoreHeader.call(this, firstLine, headers) +type FakeNextHandler = (req: IncomingMessage, res: ServerResponse) => Promise + +// The fake Next.js handler the mocked `getMockedRequestHandler` delegates to. +// `ref.current` is the render scenario each test swaps in; `handler` is the +// stable wrapper server.ts caches as its `nextHandler`. +const mockNextHandler = vi.hoisted(() => { + const ref: { current: FakeNextHandler | undefined } = { current: undefined } + return { + ref, + handler: (req: IncomingMessage, res: ServerResponse) => ref.current?.(req, res), } -} +}) -type FakeNextHandler = (res: import('node:http').ServerResponse) => Promise +// Only two boundaries of server.ts need stubbing; everything else (tracing, +// header post-processing, storage, wait-until/use-cache setup) runs for real. +// +// - `../config.js`: server.ts does a top-level `await getRunConfig()` that +// reads build output from disk, and `setRunConfig` asserts the compiled +// cache handler exists - neither is present in a unit-test run. +// - `../next.cjs`: this is the module that imports Next.js itself, and it is +// where the `nextHandler` is created. Stubbing `getMockedRequestHandler` +// lets each test inject the render scenario it wants to exercise. + +vi.mock('../config.js', () => ({ + getRunConfig: async () => ({ nextConfig: {}, enableUseCacheHandler: false }), + setRunConfig: (config: unknown) => config, +})) + +vi.mock('../next.cjs', () => ({ + // Returns the request handler server.ts caches as `nextHandler`. We hand back + // a thin wrapper that defers to whatever the current test installed, so each + // test controls the render behavior while the real server pipeline runs. + getMockedRequestHandler: async () => mockNextHandler.handler, +})) + +// Imported after the mocks above are registered (vi.mock is hoisted regardless). +// const { default: handler } = await import('./server.js') /** - * Mirror of the response-finalization half of server.ts's default export. - * `render` plays the role of `nextHandler(req, resProxy)`. + * Drive the real server handler with a given fake render, returning the Response + * it produces. */ -async function finalizeResponseLikeServerHandler(request: Request, render: FakeNextHandler) { - const { req, res } = toReqRes(request) - - Object.defineProperty(req, 'connection', { get: () => ({}) }) - Object.defineProperty(req, 'socket', { get: () => ({}) }) - - disableFaultyTransferEncodingHandling(res as unknown as ComputeJsOutgoingMessage) - - let handlerError: unknown - const nextHandlerPromise = render(res).catch((error) => { - if (res.headersSent) { - handlerError = error instanceof Error ? error : new Error(String(error)) - res.end() - } else { - res.statusCode = 500 - res.end('Internal Server Error') - } - }) - - const response = await toComputeResponse(res) - - async function waitForBackgroundWork() { - await nextHandlerPromise - res.emit('close') - } - - if (!response.body) { - await waitForBackgroundWork() - return new Response(null, response) - } - - const reader = response.body.getReader() - - const responseBody = new ReadableStream({ - start(controller) { - nextHandlerPromise.then(() => { - if (res.destroyed && !res.writableEnded) { - const abortReason = - handlerError ?? new Error('Response stream was destroyed before the render completed') - try { - controller.error(abortReason) - } catch { - // already closed/errored - } - } - }) - }, - async pull(controller) { - try { - const { done, value } = await reader.read() - if (done) { - await waitForBackgroundWork() - if (handlerError) controller.error(handlerError) - else controller.close() - return - } - controller.enqueue(value) - } catch (error) { - try { - controller.error(error) - } catch { - // already errored - } - } - }, - async cancel(reason) { - await reader.cancel(reason).catch(() => {}) - }, - }) - - return new Response(responseBody, response) +function handleRequest(request: Request, render: FakeNextHandler): Promise { + mockNextHandler.ref.current = render + const requestContext: RequestContext = createRequestContext(request) + return handler(request, {} as Context, undefined, requestContext) } type DrainResult = @@ -149,12 +105,14 @@ async function drainBody(response: Response, hangAfterMs = 500): Promise((resolve) => - setTimeout(() => resolve('hung'), hangAfterMs), - ) + const timeout = new Promise<'hung'>((resolve) => { + setTimeout(() => resolve('hung'), hangAfterMs) + }) const next = await Promise.race([reader.read(), timeout]) if (next === 'hung') { - reader.cancel().catch(() => {}) + reader.cancel().catch(() => { + // best-effort cancel; nothing to do if it rejects + }) return { outcome: 'hung', partial: body } } const { done, value } = next @@ -171,7 +129,7 @@ describe('streamed response termination', () => { test('baseline: a render that completes cleanly produces a well-framed 200', async () => { const request = new Request('https://example.netlify.app/page/') - const response = await finalizeResponseLikeServerHandler(request, async (res) => { + const response = await handleRequest(request, async (_req, res) => { res.writeHead(200, { 'content-type': 'text/html' }) res.write('') res.write('fully rendered page') @@ -192,10 +150,12 @@ describe('streamed response termination', () => { // headers + opening HTML flush early, then a later async step (e.g. a data // fetch) aborts the render. The handler promise does NOT reject: it just // destroys the response. - const response = await finalizeResponseLikeServerHandler(request, async (res) => { + const response = await handleRequest(request, async (_req, res) => { res.writeHead(200, { 'content-type': 'text/html' }) res.write('') - await new Promise((r) => setTimeout(r, 10)) + await new Promise((resolve) => { + setTimeout(resolve, 10) + }) res.destroy(new Error('stream aborted mid-render')) }) @@ -217,7 +177,7 @@ describe('streamed response termination', () => { test('render throws after headers committed -> stream errors cleanly, no garbled 200 body', async () => { const request = new Request('https://example.netlify.app/throws/') - const response = await finalizeResponseLikeServerHandler(request, async (res) => { + const response = await handleRequest(request, async (_req, res) => { res.writeHead(200, { 'content-type': 'text/html' }) res.write('partial') throw new Error('render failed mid-stream') From 55560b269862c563c905c84db7fb29ad2df47dab Mon Sep 17 00:00:00 2001 From: pieh Date: Mon, 29 Jun 2026 09:54:36 +0200 Subject: [PATCH 3/4] fix: harden streamed-response termination against source stream close behavior --- src/run/handlers/server.ts | 53 ++++++++++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/src/run/handlers/server.ts b/src/run/handlers/server.ts index 7f8d83d128..46217b39c0 100644 --- a/src/run/handlers/server.ts +++ b/src/run/handlers/server.ts @@ -216,6 +216,16 @@ export default async ( // ATS reports both as a 502 (ats_status_502_invalid_http_response). Now such // a render errors the output stream so the platform aborts the response // instead of emitting an unparseable "success". + // A render that destroyed the response without a clean `end()` left no + // terminator on the wire, so the response must error rather than close - no + // matter whether the source stream hangs (handled in `start`) or happens to + // emit `done` (handled in `pull`). Tying this to `res` state instead of to + // *which* of those two the source picks keeps correctness independent of the + // underlying stream's behaviour. + const wasDestroyedWithoutCleanEnd = () => res.destroyed && !res.writableEnded + const destroyedBeforeRenderError = () => + handlerError ?? new Error('Response stream was destroyed before the render completed') + const responseBody = new ReadableStream({ start(controller) { // If the response was destroyed mid-stream without being cleanly ended, @@ -223,17 +233,29 @@ export default async ( // handler settles we know no more bytes are coming - error the output so // the platform aborts the connection promptly. (A handler that threw is // handled in `pull` via the clean `end()` in the catch above.) - nextHandlerPromise.then(() => { - if (res.destroyed && !res.writableEnded) { - const abortReason = - handlerError ?? new Error('Response stream was destroyed before the render completed') - try { - controller.error(abortReason) - } catch { - // controller may already be closed or errored - nothing to do. + nextHandlerPromise + .then(async () => { + if (wasDestroyedWithoutCleanEnd()) { + const abortReason = destroyedBeforeRenderError() + try { + controller.error(abortReason) + } catch { + // controller may already be closed or errored - nothing to do. + } + // `pull` is parked on a `reader.read()` that will never settle + // because the destroyed response never closes its stream. Cancel + // the source reader so it is released instead of leaking. + try { + await reader.cancel(abortReason) + } catch { + // best-effort release - nothing to do if cancel rejects. + } } - } - }) + }) + .catch(() => { + // nextHandlerPromise already swallows its own errors; this only + // guards the termination logic above. + }) }, async pull(controller) { try { @@ -242,6 +264,11 @@ export default async ( await waitForBackgroundWork() if (handlerError) { controller.error(handlerError) + } else if (wasDestroyedWithoutCleanEnd()) { + // The source emitted `done` for a response that was destroyed + // without a clean `end()` - treat it as an abort, never a clean + // close, so a failed render can't reach the edge as a truncated 200. + controller.error(destroyedBeforeRenderError()) } else { controller.close() } @@ -257,7 +284,11 @@ export default async ( } }, async cancel(reason) { - await reader.cancel(reason).catch(() => {}) + try { + await reader.cancel(reason) + } catch { + // best-effort cancel - nothing to do if it rejects. + } }, }) From 1088ffeff715ca15314346350f17ee0494d6309b Mon Sep 17 00:00:00 2001 From: pieh Date: Mon, 29 Jun 2026 10:14:49 +0200 Subject: [PATCH 4/4] chore: tidy up some comments --- .../server-streaming-termination.test.ts | 12 ++--- src/run/handlers/server.ts | 54 +++++-------------- 2 files changed, 18 insertions(+), 48 deletions(-) diff --git a/src/run/handlers/server-streaming-termination.test.ts b/src/run/handlers/server-streaming-termination.test.ts index 3daa420072..f467e7dd12 100644 --- a/src/run/handlers/server-streaming-termination.test.ts +++ b/src/run/handlers/server-streaming-termination.test.ts @@ -12,9 +12,8 @@ import handler from './server.js' * A streamed SSR response commits its headers (typically a 200, with no * Content-Length) as soon as Next.js flushes them - well before the body * finishes. If the render then fails *after* that point, the response can no - * longer become a 5xx, and the body must terminate cleanly or the edge sees an - * unparseable HTTP message (which ATS reports as a 502, - * ats_status_502_invalid_http_response). + * longer become a 5xx, and the body must terminate cleanly or the edge sees a + * malformed HTTP message and rejects the response. * * These tests exercise the *real* `server.ts` default export. The only thing we * substitute is the Next.js request handler itself (the `nextHandler` that @@ -37,7 +36,7 @@ import handler from './server.js' * * After the fix: a failed/aborted render terminates the response stream * cleanly (errors), so the platform aborts the connection instead of emitting - * an unparseable "success" or hanging. + * a malformed "success" or hanging. */ type FakeNextHandler = (req: IncomingMessage, res: ServerResponse) => Promise @@ -75,9 +74,6 @@ vi.mock('../next.cjs', () => ({ getMockedRequestHandler: async () => mockNextHandler.handler, })) -// Imported after the mocks above are registered (vi.mock is hoisted regardless). -// const { default: handler } = await import('./server.js') - /** * Drive the real server handler with a given fake render, returning the Response * it produces. @@ -167,7 +163,7 @@ describe('streamed response termination', () => { const drained = await drainBody(response) // The response no longer hangs - it errors promptly and deterministically // so the platform aborts the connection (an explicit incomplete signal) - // rather than holding an unterminated stream open until ATS times it out. + // rather than holding an unterminated stream open until the edge times it out. expect(drained.outcome).toBe('errored') if (drained.outcome === 'errored') { expect(drained.partial.includes('')).toBe(false) diff --git a/src/run/handlers/server.ts b/src/run/handlers/server.ts index 46217b39c0..7faf3038c2 100644 --- a/src/run/handlers/server.ts +++ b/src/run/handlers/server.ts @@ -106,13 +106,8 @@ export default async ( getLogger().withError(error).error('next handler error') console.error(error) if (resProxy.headersSent) { - // Headers (and almost always a 200) are already on the wire, so we can - // no longer turn this into a 500. Record the error and end the response - // cleanly so buffered bytes flush in order; the body stream below then - // surfaces the error to the platform. Previously this appended - // "Internal Server Error" onto the partial response, producing a garbled - // "successful" 200 that ATS rejects as a 502 - // (ats_status_502_invalid_http_response). + // Headers are already sent, so we can't turn this into a 500. Record the + // error and end cleanly; the body stream below surfaces it as a failure. handlerError = error instanceof Error ? error : new Error(String(error)) resProxy.end() } else { @@ -205,34 +200,19 @@ export default async ( const reader = response.body.getReader() - // Stream the body through, keeping it open until the render and any tracked - // background work finish, but tie termination to the request handler so a - // failed/aborted render terminates the response *cleanly*. - // - // The previous implementation piped through a TransformStream whose flush() - // only awaited background work and had no error path. If a render aborted or - // threw after headers were already committed, the response either hung open - // (the source stream never closes) or closed as a truncated/garbled 200. - // ATS reports both as a 502 (ats_status_502_invalid_http_response). Now such - // a render errors the output stream so the platform aborts the response - // instead of emitting an unparseable "success". - // A render that destroyed the response without a clean `end()` left no - // terminator on the wire, so the response must error rather than close - no - // matter whether the source stream hangs (handled in `start`) or happens to - // emit `done` (handled in `pull`). Tying this to `res` state instead of to - // *which* of those two the source picks keeps correctness independent of the - // underlying stream's behaviour. + // Stream the body, keeping it open until the render and background work + // finish. A render that aborts/fails after headers are committed must error + // the stream rather than hang or close as a truncated 200. Tying that to + // `res` state keeps it correct whether the source stream hangs (caught in + // `start`) or emits `done` (caught in `pull`). const wasDestroyedWithoutCleanEnd = () => res.destroyed && !res.writableEnded const destroyedBeforeRenderError = () => handlerError ?? new Error('Response stream was destroyed before the render completed') const responseBody = new ReadableStream({ start(controller) { - // If the response was destroyed mid-stream without being cleanly ended, - // the source stream never closes, so `pull` would hang forever. Once the - // handler settles we know no more bytes are coming - error the output so - // the platform aborts the connection promptly. (A handler that threw is - // handled in `pull` via the clean `end()` in the catch above.) + // Destroyed source streams never close, so `pull` would park forever. + // Once the handler settles, error the output and release the reader. nextHandlerPromise .then(async () => { if (wasDestroyedWithoutCleanEnd()) { @@ -240,21 +220,17 @@ export default async ( try { controller.error(abortReason) } catch { - // controller may already be closed or errored - nothing to do. + // already closed or errored } - // `pull` is parked on a `reader.read()` that will never settle - // because the destroyed response never closes its stream. Cancel - // the source reader so it is released instead of leaking. try { await reader.cancel(abortReason) } catch { - // best-effort release - nothing to do if cancel rejects. + // best-effort release } } }) .catch(() => { - // nextHandlerPromise already swallows its own errors; this only - // guards the termination logic above. + // nextHandlerPromise already handles its own errors }) }, async pull(controller) { @@ -265,9 +241,7 @@ export default async ( if (handlerError) { controller.error(handlerError) } else if (wasDestroyedWithoutCleanEnd()) { - // The source emitted `done` for a response that was destroyed - // without a clean `end()` - treat it as an abort, never a clean - // close, so a failed render can't reach the edge as a truncated 200. + // source closed but the render was aborted - error, never close controller.error(destroyedBeforeRenderError()) } else { controller.close() @@ -287,7 +261,7 @@ export default async ( try { await reader.cancel(reason) } catch { - // best-effort cancel - nothing to do if it rejects. + // best-effort cancel } }, })