fix(metadata): preserve Content-Length for fully buffered responses - #2703
Conversation
closeAfterResponseWithBody() wraps every body-bearing response in a TransformStream to support function-form after(), which strips Content-Length even when the body is already fully materialized. Metadata file convention responses (robots(), sitemap(), manifest(), static icons) always serialize to a string or byte array with no producer left, so mark those bodies (markFullyBufferedBody) and skip the wrap when nothing is registered. Arbitrary Route Handler responses and a metadata route's Response passthrough stay wrapped: their body's producer may still call after() after the handler resolves, so a "nothing registered yet" check can't prove them safe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqasUFjUPt2Q8gkrzrLGyB
|
/bigbonk review for issues |
|
/bigbonk review for issues |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full diff and traced the request lifecycle from metadata response construction through middleware rebuild, finalizeAppRscResponse, and closeAfterResponseWithBody. The core design is sound and the safety reasoning holds up under scrutiny:
Correctness of the skip gate
- The
requiresResponseCloseTracking()gate correctly captures all three waysafter()work can be outstanding at wrap time: queued function-form callbacks (callbacks.length), in-flight ones (pendingCallbacks), and promise-form work whose continuation may register a callback later (pendingPromises). TheresolveCompletion !== nullbelt-and-suspenders check handles re-entry after callbacks have started. - The key race is handled well: a user
robots()/sitemap()/manifest()generator that itself callsafter(promise)bumpspendingPromisessynchronously during theawaited generator call, so by the timecloseAfterResponseWithBodyruns the skip is correctly not taken. The ported Next.js after-context test (tests/shims.test.ts:5093) exercises exactly this and is the strongest test in the set. - Marker survival was the thing I was most worried could regress silently. I verified:
finalizeAppRscResponsemutates headers in-place and returns the same object (marker survives), config-header application mutates in-place (survives), and the one genuine rebuild —applyMiddlewareContextToResponse— is now guarded bypreserveFullyBufferedBodyMetadata. Theresult instanceof Responsepassthroughs (metadata-route-response.ts:222, :307) are correctly left unmarked. No missed buffered-body construction sites.
Skip branch never calls closeAfterResponse — I confirmed this is safe: when the skip is taken, nothing is queued and no producer remains for a fully-buffered metadata body, so responseClosed staying false leaks nothing (no completion promise exists). This differs from the !response.body branch which does schedule a microtask close, but that difference is justified because the empty-body path can still receive a late registration whereas the skipped buffered-body path cannot.
Two minor, non-blocking observations below. Nice work — the doc comments and test coverage make the safety argument easy to follow.
| type SitemapEntry, | ||
| } from "./metadata-routes.js"; | ||
| import { notFoundResponse } from "./http-error-responses.js"; | ||
| import { markFullyBufferedBody } from "vinext/shims/unified-request-context"; |
There was a problem hiding this comment.
Minor style nit (non-blocking): this uses the bare vinext/shims/unified-request-context specifier while every other import in this file uses relative ./*.js paths (e.g. ./metadata-routes.js, ./http-error-responses.js). app-rsc-handler.ts also 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.
| remote: false, | ||
| persist: false, | ||
| logLevel: "none", | ||
| watch: false, |
There was a problem hiding this comment.
The /robots.txt case is the interesting one because it goes through the middleware rebuild (new Response(response.body, ...)), so the final body handed to workerd is a ReadableStream, not a string. The assertion that Content-Length is preserved therefore depends on workerd being able to derive a fixed length from a stream sourced from an already-materialized Response.body. That's a runtime-specific behavior, so this real-workerd test is the right way to pin it. Worth keeping an eye on if workerd's compatibility date ever changes how new Response(stream) length inference works — the /sitemap.xml case (no middleware, string body reaches workerd directly) is the more robust guarantee. No change needed.
|
Review posted successfully: #2703 (review) SummaryI reviewed PR #2703 ("fix(metadata): preserve Content-Length for fully buffered responses") by tracing the full request lifecycle from metadata response construction through the middleware rebuild, Verdict: sound, no blocking issues. This is a carefully-reasoned change with strong test coverage. Key things I verified:
Two minor non-blocking notes posted inline: an import-style inconsistency (bare vs relative specifier) and a note that the |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
I traced the full request lifecycle from metadata response construction (markFullyBufferedBody) through the middleware rebuild (applyMiddlewareContextToResponse), finalizeAppRscResponse/config-header application, and into closeAfterResponseWithBody. The change is sound and I found no blocking issues. The safety argument holds and the test coverage is strong.
What I verified:
-
Skip gate correctness.
requiresResponseCloseTracking()covers all three outstanding-work forms (callbacks.length,pendingCallbacks,pendingPromises) plus theresolveCompletion !== nullre-entry guard. The important race — a metadata generator that itself callsafter(promise)whose continuation later registers a function-form callback — is handled two ways:pendingPromisesis bumped synchronously inside the awaited generator (so the skip is refused at close time), and even if the promise settles early, the continuation'squeueAfterCallbackpushes tocallbacksand setsresolveCompletionbefore the body is consumed. Confirmedafter(promise)(server.ts:1247) does not callensureAfterCompletion, so pure promise-form work relies onpendingPromises— which is correct. -
Marker survival across rebuilds.
finalizeAppRscResponsemutates headers in place and returns the same object (marker survives); config-header application mutates in place; the one genuinenew Response(...)rebuild on the metadata path (applyMiddlewareContextToResponse) is now guarded bypreserveFullyBufferedBodyMetadata. Theresult instanceof Responsepassthroughs (metadata-route-response.ts:222 andwithMetadataRouteCacheHeaderat :98) are correctly left unmarked, since a usericon.tsx/opengraph-image.tsxcan return a streamingImageResponse. No missed buffered-body construction site. -
Skip branch never calls
closeAfterResponse— safe, because a fully-materialized string/byte body has no remaining producer that could register lateafter()work, so nothing leaks.
Two non-blocking notes inline.
| * bodies can still be producing and must keep close tracking. | ||
| */ | ||
| export function markFullyBufferedBody(response: Response): Response { | ||
| (response as ResponseWithFullyBufferedBodyMetadata).__vinextFullyBufferedBody = true; |
There was a problem hiding this comment.
Non-blocking: __vinextFullyBufferedBody becomes an own enumerable property on the Response instance handed to the runtime. This mirrors the existing __vinextStreamedHtmlResponse convention below (line 324), so it's consistent and Response objects aren't JSON-serialized or spread in practice — just flagging that the marker is technically observable on the wire object. No change needed.
| type SitemapEntry, | ||
| } from "./metadata-routes.js"; | ||
| import { notFoundResponse } from "./http-error-responses.js"; | ||
| import { markFullyBufferedBody } from "vinext/shims/unified-request-context"; |
There was a problem hiding this comment.
Minor style nit (non-blocking): this uses the bare vinext/shims/unified-request-context specifier while the other imports in this file are relative (./metadata-routes.js, ./http-error-responses.js). app-rsc-handler.ts already imports it via the bare specifier so this resolves fine and there's precedent, but a relative ./ import would read more consistently within this file.
| }); | ||
| expect(res.status).toBe(200); | ||
| expect(res.headers.get("content-encoding")).toBeNull(); | ||
| expect(res.headers.get("x-metadata-middleware")).toBe("applied"); |
There was a problem hiding this comment.
The /robots.txt assertion is the more fragile of the two Content-Length cases: it goes through the middleware rebuild (new Response(response.body, ...)), so the body handed to workerd is a ReadableStream sourced from an already-materialized body, and the preserved Content-Length depends on workerd inferring a fixed length from that stream. That's runtime-specific behavior worth watching if the compatibility date ever changes stream-length inference. The /sitemap.xml case (no middleware matcher, string body reaches workerd directly) is the more robust guarantee — good that both are covered.
|
Review posted successfully: #2703 (review) SummaryI reviewed PR #2703 ("fix(metadata): preserve Content-Length for fully buffered responses") by tracing the full request lifecycle from metadata response construction through the middleware rebuild, Verdict: sound, no blocking issues. Carefully reasoned change with strong test coverage. Key things I verified:
Three non-blocking notes posted inline: the marker property is technically observable on the wire object (consistent with existing My review largely corroborates the earlier |
2 participants
Summary
closeAfterResponseWithBody()wraps every body-bearing App/Pages Routerresponse in a
TransformStreamso it can release deferredafter()workonce the body closes. That wrap strips
Content-Length, even for a responsevinext has already fully materialized in memory.
Metadata file convention responses (
robots(),sitemap(),manifest(),static icons) always serialize their body to a string or byte array before
returning — no producer is left that could still call
after(). This marksthose responses (
markFullyBufferedBody) and skips the wrap when nothing iscurrently registered, letting the runtime emit an accurate
Content-Lengthinstead of chunked transfer encoding.
Arbitrary Route Handler responses, and a metadata route's
Responsepassthrough (a user
icon.tsx/opengraph-image.tsxcan return a streamingImageResponse), are left unmarked — their body's producer can still callafter()after the handler resolves, so a "nothing registered yet" checkcan't prove them safe. Those stay close-tracked.
Test plan
tests/after-response-close-worker.test.ts(real Cloudflare Workersruntime via wrangler):
robots.txtandsitemap.xmlregainContent-Length; a hand-written Route Handler stays chunked; a RouteHandler returning
new Response(stream)still runs a lateafter()callcorrectly after the stream completes or is cancelled.
tests/shims.test.ts: unit coverage for the marker gate, and protectivetests proving a late
after()call from an unmarked stream waits forcompletion/cancellation and connects to
ExecutionContext.waitUntil.after()/dispatch, App/PagesRouter prod and dev servers) pass.
vp checkpasses. Both packages build.(
EXDEV) failures in this sandbox, unrelated to this change andreproducing identically on unmodified
main; not fully green.Limitations
Hand-written Route Handler responses stay close-tracked even when their
current body happens to be buffered, because a returned
ReadableStreammay keep producing and register
after()after the handler functionresolves. Safely optimizing those would need a separate, principled
buffered-body signal.