-
Notifications
You must be signed in to change notification settings - Fork 365
fix(metadata): preserve Content-Length for fully buffered responses #2703
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.
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
Changes from all commits
9f1c55f
304e6ce
c5dcecc
6d11ff6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import { | |
| type SitemapEntry, | ||
| } from "./metadata-routes.js"; | ||
| import { notFoundResponse } from "./http-error-responses.js"; | ||
| import { markFullyBufferedBody } from "vinext/shims/unified-request-context"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor style nit (non-blocking): this uses the bare |
||
|
|
||
| type AppPageParams = Record<string, string | string[]>; | ||
| type MetadataRouteFunction = (props: Record<string, unknown>) => unknown; | ||
|
|
@@ -223,12 +224,15 @@ async function handleGeneratedSitemap( | |
| if (!isSitemapEntries(result)) { | ||
| throw new TypeError("Metadata sitemap routes must return an array."); | ||
| } | ||
| return new Response(sitemapToXml(result), { | ||
| headers: { | ||
| "Content-Type": route.contentType, | ||
| "Cache-Control": metadataRouteCacheHeader(route), | ||
| }, | ||
| }); | ||
| // Body serialized to a string here — fully materialized, no producer left. | ||
| return markFullyBufferedBody( | ||
| new Response(sitemapToXml(result), { | ||
| headers: { | ||
| "Content-Type": route.contentType, | ||
| "Cache-Control": metadataRouteCacheHeader(route), | ||
| }, | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| function findGeneratedImageId( | ||
|
|
@@ -327,12 +331,15 @@ async function callDynamicMetadataRoute( | |
| body = JSON.stringify(result); | ||
| } | ||
|
|
||
| return new Response(body, { | ||
| headers: { | ||
| "Content-Type": route.contentType, | ||
| "Cache-Control": metadataRouteCacheHeader(route), | ||
| }, | ||
| }); | ||
| // Every branch above serialized `result` to a string — fully materialized. | ||
| return markFullyBufferedBody( | ||
| new Response(body, { | ||
| headers: { | ||
| "Content-Type": route.contentType, | ||
| "Cache-Control": metadataRouteCacheHeader(route), | ||
| }, | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| function serveStaticMetadataRoute(route: MetadataRuntimeRoute): Response { | ||
|
|
@@ -348,12 +355,15 @@ function serveStaticMetadataRoute(route: MetadataRuntimeRoute): Response { | |
| for (let index = 0; index < binary.length; index++) { | ||
| bytes[index] = binary.charCodeAt(index); | ||
| } | ||
| return new Response(bytes, { | ||
| headers: { | ||
| "Content-Type": route.contentType, | ||
| "Cache-Control": metadataRouteCacheHeader(route), | ||
| }, | ||
| }); | ||
| // Static file bytes, fully in memory — no producer left. | ||
| return markFullyBufferedBody( | ||
| new Response(bytes, { | ||
| headers: { | ||
| "Content-Type": route.contentType, | ||
| "Cache-Control": metadataRouteCacheHeader(route), | ||
| }, | ||
| }), | ||
| ); | ||
| } catch (error) { | ||
| const reason = error instanceof Error && error.message ? `: ${error.message}` : ""; | ||
| throw new Error( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,7 @@ export type AfterRequestContext = { | |
| callbacks: Array<() => unknown>; | ||
| responseClosed: boolean; | ||
| pendingCallbacks: number; | ||
| pendingPromises: number; | ||
| completion: Promise<void> | null; | ||
| resolveCompletion: (() => void) | null; | ||
| }; | ||
|
|
@@ -129,6 +130,7 @@ export function createRequestContext(opts?: Partial<UnifiedRequestContext>): Uni | |
| callbacks: [], | ||
| responseClosed: false, | ||
| pendingCallbacks: 0, | ||
| pendingPromises: 0, | ||
| completion: null, | ||
| resolveCompletion: null, | ||
| }, | ||
|
|
@@ -194,6 +196,14 @@ export function queueAfterCallback(ctx: UnifiedRequestContext, callback: () => u | |
| } | ||
| } | ||
|
|
||
| /** Track promise-form after() work that can register a callback before settling. */ | ||
| export function trackAfterPromise<T>(ctx: UnifiedRequestContext, promise: Promise<T>): Promise<T> { | ||
| ctx.afterContext.pendingPromises += 1; | ||
| return promise.finally(() => { | ||
| ctx.afterContext.pendingPromises -= 1; | ||
| }); | ||
| } | ||
|
|
||
| /** Bind a callback to every AsyncLocalStorage context active at registration. */ | ||
| export function bindRequestContextSnapshot<T>( | ||
| ctx: UnifiedRequestContext, | ||
|
|
@@ -226,16 +236,80 @@ export async function closeAfterResponse(ctx: UnifiedRequestContext): Promise<vo | |
| return state.completion ?? Promise.resolve(); | ||
| } | ||
|
|
||
| /** Wrap a response so deferred callbacks start on stream completion or cancellation. */ | ||
| /** | ||
| * Whether this request has function-form `after()` work that still needs to | ||
| * observe the response body closing. | ||
| * | ||
| * Promise-form `after(promise)` is included because its continuation can | ||
| * register a function-form `after()` before the promise settles. Function-form | ||
| * work needs the body's close observed because its contract is "run once the | ||
| * response has been sent". `resolveCompletion` stays non-null for as long as | ||
| * any callback is queued or in flight, so checking it alongside the explicit | ||
| * counters keeps this correct on re-entry after callbacks have started. | ||
| */ | ||
| export function requiresResponseCloseTracking(ctx: UnifiedRequestContext): boolean { | ||
| const state = ctx.afterContext; | ||
| return ( | ||
| state.callbacks.length > 0 || | ||
| state.pendingCallbacks > 0 || | ||
| state.pendingPromises > 0 || | ||
| state.resolveCompletion !== null | ||
| ); | ||
| } | ||
|
|
||
| type ResponseWithFullyBufferedBodyMetadata = Response & { | ||
| __vinextFullyBufferedBody?: boolean; | ||
| }; | ||
|
|
||
| /** | ||
| * Mark a response whose body vinext constructed from a fully in-memory string | ||
| * or byte array, as opposed to a body handed back by user code, which could | ||
| * still be producing. With no producer left, no `after()` call can originate | ||
| * from this body — the one signal that makes it safe for | ||
| * `closeAfterResponseWithBody()` to skip close tracking. | ||
| * | ||
| * Not set for a metadata route's `result instanceof Response` passthrough (a | ||
| * user `icon.tsx`/`opengraph-image.tsx` can return a streaming | ||
| * `ImageResponse`) or any handler-returned `new Response(stream)` — those | ||
| * bodies can still be producing and must keep close tracking. | ||
| */ | ||
| export function markFullyBufferedBody(response: Response): Response { | ||
| (response as ResponseWithFullyBufferedBodyMetadata).__vinextFullyBufferedBody = true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: |
||
| return response; | ||
| } | ||
|
|
||
| function isFullyBufferedBody(response: Response): boolean { | ||
| return (response as ResponseWithFullyBufferedBodyMetadata).__vinextFullyBufferedBody === true; | ||
| } | ||
|
|
||
| /** Preserve the internal buffered-body signal when response metadata is rebuilt. */ | ||
| export function preserveFullyBufferedBodyMetadata(source: Response, target: Response): Response { | ||
| return isFullyBufferedBody(source) ? markFullyBufferedBody(target) : target; | ||
| } | ||
|
|
||
| /** | ||
| * Wrap a response so deferred `after()` callbacks start on stream completion | ||
| * or cancellation. Skipped only when the body is marked fully buffered (see | ||
| * `markFullyBufferedBody`) and nothing is currently registered — that lets | ||
| * the runtime send it with an accurate `Content-Length` instead of chunked | ||
| * transfer encoding. | ||
| */ | ||
| export function closeAfterResponseWithBody( | ||
| response: Response, | ||
| ctx: UnifiedRequestContext, | ||
| ): Response { | ||
| if (!response.body) { | ||
| // Resolve the after-lifecycle now (a no-op if nothing is queued) so a | ||
| // callback registered after this call returns doesn't wait forever on a | ||
| // responseClosed flag nothing else will set. | ||
| queueMicrotask(() => void closeAfterResponse(ctx)); | ||
| return response; | ||
| } | ||
|
|
||
| if (isFullyBufferedBody(response) && !requiresResponseCloseTracking(ctx)) { | ||
| return response; | ||
| } | ||
|
|
||
| const passthrough = new TransformStream<Uint8Array, Uint8Array>(); | ||
| void response.body.pipeTo(passthrough.writable).then( | ||
| () => void closeAfterResponse(ctx), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style nit (non-blocking): this uses the bare
vinext/shims/unified-request-contextspecifier while every other import in this file uses relative./*.jspaths (e.g../metadata-routes.js,./http-error-responses.js).app-rsc-handler.tsalso imports it via the bare specifier so this resolves fine and there's precedent — but for local consistency within this file a relative import would read cleaner. Not a blocker.