Stream publisher origin bodies end-to-end on Fastly#867
Open
prk-Jr wants to merge 15 commits into
Open
Conversation
Publisher pages were fully buffered before the first byte reached the client: the platform client materialized the origin body (10 MiB cap), the rewrite pipeline ran over an in-memory cursor, and the EdgeZero finalize buffered the assembled response while awaiting auction collection. TTFB therefore tracked full origin transfer plus the auction instead of origin first byte. - Add supports_streaming_responses() to PlatformHttpClient (default false, Fastly true) and request with_stream_response() on the publisher origin fetch only where honored - Teach the pipeline to consume Body::Stream asynchronously: BodyChunkSource (cumulative raw-byte cap via publisher.max_buffered_body_bytes), push-style BodyStreamDecoder/BodyStreamEncoder in streaming_processor - Replace the Fastly buffered finalize with publisher_response_into_streaming_response: a lazy Body::Stream that commits headers at origin first byte, streams rewritten chunks, and holds only the </body> tail for auction collection; bids still inject before body close - Share one hold implementation (hold_step_decoded_chunk / hold_finish_segments) between the lazy body and the writer-driven loop so the paths cannot drift; collect_non_html_auction dedupes the collect-before-stream path - Finalize brotli decode with close() so truncated origin streams error instead of silently truncating; decode failures emit stream_decode_error telemetry - Guard bodiless (HEAD/204/304) responses and log wasted auction dispatch, matching the buffered finalizer Local A/B on a 183 KB gzip publisher page with a live 3-slot auction (release builds, 20 interleaved rounds): TTFB median 741 ms buffered vs 161 ms streamed (-78%); guest wall time and wasm heap unchanged.
Address the deep-review findings on the streaming cutover: - Cap cumulative decoded bytes in BodyStreamDecoder against publisher.max_buffered_body_bytes: the chunk source only bounds raw compressed bytes, so a decompression bomb could expand ~1000x past it and push unbounded decoded volume through the rewrite pipeline - Detect truncated deflate streams: write::ZlibDecoder::try_finish accepts truncated input silently, so the deflate arm now drives flate2::Decompress directly and requires Status::StreamEnd at finalization; trailing bytes after the end marker stay ignored. Add truncated-gzip and truncated-deflate regression tests - Make BodyChunkSource::next_chunk cancellation-safe by polling the body in place instead of moving it out across an await; a cancelled pull no longer turns into a silent EOF - Log dispatched auctions dropped uncollected (client disconnect mid-stream or never-polled body) via DispatchedAuctionGuard; the guard is created before the lazy stream so unpolled drops log too - Share the pull+decode step between the lazy publisher body and the write-sink drivers (hold_step_next_chunk / passthrough_step), removing the unreachable!() error plumbing and the triplicated processor selection; document body_close_hold_loop_stream as groundwork for the buffered adapters' streaming cutover - Pass identity-encoded chunks through zero-copy and finish encoders by consuming them instead of allocating a throwaway replacement - Add a Fastly dispatch test asserting the publisher fallback returns Body::Stream without a stale Content-Length, plus a comment on why the publisher fetch gates streaming on capability while the asset path does not Behavior note: gzip bodies with trailing garbage after the trailer now error mid-stream; the old read-path decoder ignored them.
The buffered finalizer abandons a dispatched auction with processor_init_error telemetry when HTML processor construction fails; the streaming finalizer dropped the in-flight SSP responses silently. Make publisher_response_into_streaming_response async and emit the same abandonment before returning the construction error.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #867 against main, focusing on the new Fastly end-to-end publisher streaming path, body/HTTP semantics, decoder limits, adapter contracts, and regression coverage. The change is well-tested and CI is green, but I found two correctness/resource-safety issues that should be addressed before relying on this path in production.
Findings
See the inline comments for the detailed findings.
CI / Existing Reviews
gh pr checks 867 reports all checks passing, including Rust/JS tests, formatting, CodeQL, browser integration, and cross-adapter parity. No existing PR reviews or inline comments were present when this automated review ran.
ChristianPavilonis
requested changes
Jul 13, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
I found two high-impact issues and two medium-severity issues in the new Fastly streaming path. Valid deflate responses can be truncated, and enabling the supported Next.js integration still withholds every body byte until origin EOF, so issue #849's FCP objective is not met for that configuration. Valid multi-member gzip framing and cancellation-time auction observability also need correction.
I am requesting changes. The two earlier unresolved threads—bodiless streamed responses and decoded-limit enforcement only after a complete expansion—also remain current and are intentionally not duplicated here. All 19 CI checks are green, but the focused codec cases below are not covered by them.
Resolve five correctness and resource-safety findings from the PR #867 review of the end-to-end Fastly publisher streaming path. - Drive the deflate decoder to StreamEnd at finalization so a valid stream that exactly fills the internal output buffer is no longer rejected as truncated; the inflater is also drained after all input is consumed within a chunk. - Decode concatenated (multi-member) gzip bodies via MultiGzDecoder on both the streaming decoder and the buffered read pipeline so adapters agree. - Enforce the decoded-body cap during decompression through a bounded sink shared by the gzip and brotli codecs, so a compression bomb errors before its expanded bytes are buffered instead of after a full chunk expands; the deflate codec charges each produced block as it is emitted. - Drop the body of bodiless responses (HEAD, 204, 205, 304) in both the streaming and buffered finalizer Buffered arms, and add RESET_CONTENT to response_carries_body, so a buffered-unmodified stream body is never streamed to the client for a response that must be bodiless. - Keep the dispatched-auction guard armed across the collection await and disarm it only once collection reaches a terminal result, so a body dropped while collection is pending still logs the discarded SSP work. Add regression tests for the deflate output-buffer boundary, multi-member gzip, bodiless buffered stream bodies, and the auction guard sentinel.
Resolve conflicts in favor of the streaming pipeline: - app.rs: keep the lazy publisher_response_into_streaming_response import; Fastly streams, so buffer_publisher_response_async is unused here. - publisher.rs: keep module-level flate2/futures imports and the body.is_stream() streaming branch in stream_html_with_auction_hold. Main moved core to edition 2024 via workspace inheritance, enabling let-chains and tripping clippy::collapsible_if; collapse the nested if in passthrough_finish_segments to satisfy it.
BodyStreamEncoder::encode_chunk wrote each processed chunk into the gzip/deflate/brotli codec and drained the inner buffer, but never flushed the codec. Compressors buffer internally: write_all alone left gzip emitting at most its 10-byte header and deflate/brotli emitting nothing until finish(). Fastly commits the response headers early, so the browser saw an open stream with no decodable HTML until the entire origin transfer completed — the FCP regression tracked in #849. Sync-flush the flate2 encoders (Flush::Sync — byte-aligned, no trailer) and emit a brotli flush marker (BROTLI_OPERATION_FLUSH) after each write. finish() still writes the terminating trailer, so the full stream stays valid. Add a regression test that decodes the first chunk with a flushed-but-unfinished decoder for every codec.
The close-body hold step processed a chunk into ready segments and the held closing tail together, but awaited collect_stream_auction() before returning any of them. When </body> landed in the first source chunk (common for small pages), the client received no HTML until the auction finished; for larger pages the whole prefix from the close-containing chunk was delayed — contradicting the contract that only the </body> tail is held while the auction rides alongside transfer. Split hold_step_decoded_chunk so it returns the ready prefix plus a close_found flag without collecting, and move collection plus held-tail processing into a new hold_collect_close_tail. Both the lazy Fastly stream and the write-sink driver now emit the ready prefix first, then await collection, then emit the tail. Add a regression test asserting the prefix is ready with close_found set while the auction is still uncollected (ad_bids_state stays None until the tail step).
Both publisher finalizers dropped the body for bodiless responses but kept the origin Content-Length verbatim, including a possible nonzero value. RFC 9110 §8.6 forbids Content-Length on 204, and a nonzero length on a now-empty 205 (§15.4.6) is invalid; inconsistent framing risks interoperability failures and response-splitting across intermediaries. Add make_response_bodiless: it empties the body and removes Content-Length for 204, normalizes it to 0 for 205, and preserves it for HEAD and 304 (which legitimately advertise the GET representation length). Call it from the buffered-unmodified arm of both buffer_publisher_response_async and publisher_response_into_streaming_response. Split the combined bodiless regression test into per-status expectations and add the matching coverage for the buffered finalizer.
A conditional navigation can dispatch a server-side auction and receive a processable HTML 304 that classification routes to Stream. Both finalizers' bodiless branches logged a warning and returned, dropping params.dispatched_auction without emit_abandoned_auction — so the SSP work and quota consumption had no terminal auction event, leaving a gap in auction observability on a legitimate conditional-request path. Take the dispatched auction in the bodiless branch of both buffer_publisher_response_async and publisher_response_into_streaming_response and await emit_abandoned_auction(..., "bodiless_response") with the retained observation, matching the processor-init-error path. Add a recording-telemetry-sink test (via a new test_support helper) asserting both finalizers emit the bodiless_response abandonment for a 304 with a dispatched auction.
The merge moved core to edition 2024, whose rustfmt sorts use-imports case-sensitively (lowercase items last) and rewraps long lines. Reformat the conflict-resolved imports and the new test assertions to match; no behavior change.
aram356
requested changes
Jul 16, 2026
aram356
left a comment
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Reviewed PR #867 at 5d6fc21f6 against main. All four findings from the 2026-07-14 review are fixed with one-to-one regression tests (per-chunk encoder flush, ready-prefix-before-collect, 204/205 framing, bodiless abandonment telemetry), and the earlier deflate/multi-gzip/decode-cap/guard findings all hold at head. One P1 from the 2026-07-13 review remains unaddressed and unanswered — post-processor configs still buffer the full document — plus one behavioral divergence and two smaller cleanups. All 19 CI checks pass.
Blocking
❓ question
- Next.js-enabled configs still deliver zero body bytes until origin EOF:
HtmlWithPostProcessingaccumulates the whole rewritten document whenever a post-processor is registered, so the lazy stream emits nothing until EOF + post-process + recompress; #849's FCP objective is unmet for that configuration and the exception isn't documented (crates/trusted-server-core/src/publisher.rs:369 — see inline comment)
Non-blocking
🤔 thinking
- gzip trailing-garbage tolerance regressed vs main, and gzip/deflate now diverge:
MultiGzDecodererrors on trailing junk that single-memberGzDecoderignored, while the deflate arm deliberately ignores trailing bytes (crates/trusted-server-core/src/streaming_processor.rs:150 — see inline comment)
⛏ nitpick
- Post-release chunks copied needlessly in
hold_step_decoded_chunk(crates/trusted-server-core/src/publisher.rs:731 — see inline comment)
🏕 camp site
publisher.rsis now ~7,300 lines: the streaming machinery added here (BodyChunkSource,AuctionHoldState,DispatchedAuctionGuard, thehold_*/passthrough_*helpers, both finalizers) is a coherent unit that would read better as apublisher/streaming.rssubmodule — not necessarily in this PR, but the file is past the point where navigation hurts
CI Status
- fmt: PASS
- clippy: PASS
- rust tests (fastly/axum/cloudflare/spin + parity): PASS
- js tests / format / docs format: PASS
- browser + integration tests: PASS
Replace flate2's MultiGzDecoder with a shared GzipStreamDecoder used by both the streaming BodyStreamDecoder and the buffered pipeline (via a Read adapter). At each member boundary the next bytes are sniffed for the gzip magic number: a match starts the next member, anything else is dropped once at least one member has fully decoded — matching GNU gzip, main's single-member tolerance, and the deflate codec. Truncated or corrupt members still error.
Once the close-body hold is released, decoded chunks were still copied via to_vec() before processing. Use Cow so the post-release path borrows the chunk and only the held path allocates.
HTML post-processor configs (the nextjs integration) accumulate the full rewritten document until origin EOF, so those pages do not stream body bytes early. Record the limitation and the intended follow-up (an up-front or streaming should_process gate) in the plan's Out of scope section.
Collaborator
Author
|
Re the camp-site note on Summary of this round (33384cd, 911268f, 666b3cb):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Body::Streamso headers commit at origin first byte while the auction rides the transfer — only the</body>tail is held so bids still inject before body close.fastly compute serve, same seeded config, live origin, 183 KB gzip page, live 3-slot server-side auction, 20 interleaved rounds): TTFB median 741 ms buffered → 161 ms streamed (−78%, 4.6×); guest wall time (738 vs 744 ms) and wasm heap (4.3 vs 4.2 MB) unchanged — the win is purely overlapping delivery with transfer, and >10 MiB pages now stream instead of erroring. Buffered adapters (Axum/Cloudflare/Spin) keep the bounded buffered finalizer; their platform clients don't produce streams yet.Changes
crates/trusted-server-core/src/platform/http.rssupports_streaming_responses()toPlatformHttpClient(defaultfalse) so callers only request the streaming contract where the adapter honors itcrates/trusted-server-adapter-fastly/src/platform.rssupports_streaming_responses() = truecrates/trusted-server-core/src/publisher.rswith_stream_response()when supported;BodyChunkSource(async chunk pull + cumulative raw-byte cap);publisher_response_into_streaming_responsebuilds the lazy processedBody::Stream(auction hold inside the generator); sharedhold_step_decoded_chunk/hold_finish_segmentsused by both async hold paths;collect_non_html_auctiondedupes collect-before-stream; bodiless (HEAD/204/304) guard + wasted-dispatch warning;body_as_readernow errors on stream bodies instead of silently emptying themcrates/trusted-server-core/src/streaming_processor.rsBodyStreamDecoder/BodyStreamEncoder(write-based flate2/brotli codecs; brotli finalize usesclose()so truncated input errors instead of silently truncating); sharedSTREAM_CHUNK_SIZEcrates/trusted-server-adapter-fastly/src/app.rsbuffer_publisher_response_asynccrates/trusted-server-adapter-fastly/src/main.rssend_edgezero_responsedoc: streaming now covers publisher bodies, not just assetscrates/trusted-server-core/src/proxy.rsstream_asset_bodydocs generalized — it now bridges publisher streams toocrates/trusted-server-core/src/platform/test_support.rscrates/trusted-server-core/src/settings.rsmax_buffered_body_bytesdoc: also the cumulative raw-byte cap on the streaming path; cap trip after headers truncates rather than 5xxCargo.toml,crates/trusted-server-core/Cargo.toml,Cargo.lockasync-stream(already in the tree via edgezero-core) for the lazy body generatordocs/superpowers/plans/2026-07-08-true-origin-streaming-fastly.mdCloses
Closes #849
Test plan
cargo test-fastly && cargo test-axum(core: 1642 passed; pluscargo test-cloudflare && cargo test-spin)cargo clippy-fastly && cargo clippy-axum(plusclippy-cloudflare,clippy-cloudflare-wasm,clippy-spin-native,clippy-spin-wasm, all on 1.95.0)cargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve: side-by-side A/B vsmain(20 interleaved rounds, live origin + live 3-slot auction) — TTFB 741 → 161 ms median,transfer-encoding: chunkedwith gzip preserved, bids injected before</body>on both builds, byte-parity on body size</body>survives the auction hold; truncated brotli stream errors; cumulative cap enforcement; Stream-vs-Once parity across gzip/deflate/brotli/identity; stream-flag gating on/offChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)Known limitation (deferred)
Configs with an HTML post-processor registered (the
nextjsintegration) do not stream yet:HtmlWithPostProcessingaccumulates the full rewritten document and runs post-processors at origin EOF, so body bytes for those pages are not emitted until the complete origin transfer. Headers still commit early. Tracked in #924; also recorded in the plan doc's Out-of-scope section.