Phase 4: Stream binary pass-through responses via io::copy

[aram356]

Summary

Stream non-processable 2xx responses (images, fonts, video) directly to the client instead of buffering the entire body in memory. Uses send_to_client() with the unmodified body to preserve Content-Length and avoid chunked encoding overhead.

Closes #592, closes #593.
Part of epic #563. Depends on Phase 3 (#591).

Performance results (staging vs production, median over 8 runs, Chrome 1440x900)

Metric	Production (v135, buffered)	Staging (v141, streaming+passthrough)	Delta
TTFB	57 ms	28 ms	-29 ms (-51%)
First Paint	252 ms	182 ms	-70 ms (-28%)
First Contentful Paint	252 ms	182 ms	-70 ms (-28%)
DOM Content Loaded	322 ms	248 ms	-74 ms (-23%)
DOM Complete	678 ms	582 ms	-96 ms (-14%)

Measured on getpurpose.ai via Chrome DevTools Protocol with --host-resolver-rules to route to staging Fastly edge.

How it works

The streaming gate in handle_publisher_request now has three outcomes:

is_success (2xx, not 204)
├── should_process && (!is_html || !has_post_processors) → Stream (pipeline)
├── should_process && is_html && has_post_processors     → Buffered (post-processors)
└── !should_process                                      → PassThrough (send_to_client)

204 No Content or !is_success
└── any content type                                     → Buffered

PassThrough reattaches the unmodified body and sends via send_to_client(), preserving Content-Length. This avoids both WASM memory buffering and chunked encoding overhead.

What changed

File	Lines	What
publisher.rs	+60 -5	PassThrough variant, gate logic, 204 exclusion, tests
main.rs	+5	Handle PassThrough: reattach body, return for send_to_client()

Tests added

• pass_through_gate_streams_non_processable_2xx — images/fonts return PassThrough
• pass_through_gate_buffers_non_processable_error — 4xx stays Buffered
• pass_through_gate_does_not_apply_to_processable_content — HTML goes through Stream
• pass_through_gate_excludes_204_no_content — 204 stays Buffered (HTTP spec)
• pass_through_gate_applies_with_empty_request_host — empty host still passes through
• pass_through_preserves_body_and_content_length — byte-for-byte identity + CL preserved

Verification

• cargo test --workspace — 772 passed, 0 failed
• cargo clippy --workspace --all-targets --all-features -- -D warnings — clean
• cargo fmt --all -- --check — clean
• cargo build --release --target wasm32-wasip1 — success

Test plan

• Pass-through gate unit tests pass (6 tests)
• Byte-level body + Content-Length preservation test passes
• 204 No Content exclusion test passes
• All existing streaming/buffered tests pass
• Full workspace tests pass (772)
• WASM build succeeds
• Staging performance verified (51% TTFB, 28% FCP, 14% DOM Complete improvement)

Dismissing to resubmit with complete review body — inline comments and body were split across two API calls due to tooling issue.

[prk-Jr]

Summary

Phase 4 is mechanically correct: the PassThrough variant cleanly separates binary pass-through from streaming/buffered paths, Content-Length is preserved via send_to_client(), the 204 exclusion is properly handled, and the once_cell → LazyLock migration reduces the dependency surface. Two items need to be addressed before merge.

Blocking

🔧 wrench

• Stale PR title: The title says "via io::copy" but the final implementation uses response.set_body(body) + send_to_client(). Commit 3aa88fe replaced the io::copy approach specifically to preserve Content-Length. The title should read something like "Phase 4: Pass-through binary responses with Content-Length preservation via send_to_client".

Non-blocking

🤔 thinking

• Empty-host + non-processable 2xx behavioral change is undocumented: Before Phase 4, empty-host requests always went Buffered. Now non-processable 2xx responses with an empty host take the PassThrough path instead. Intentional and correct (URL rewriting is skipped either way), but not mentioned anywhere in the PR description. Worth a one-liner: "Non-processable 2xx with empty request host now PassThrough instead of Buffered — URL rewriting is skipped either way."

📝 note

• Content-Length fix in buffered path not mentioned: publisher.rs:449 — Phase 3 left the buffered path calling response.remove_header(CONTENT_LENGTH) after processing; this PR correctly sets it to the processed output length. The fix is right but it changes observable response headers and isn't mentioned in the description or commit messages.

👍 praise

• once_cell → std::sync::LazyLock migration (datadome.rs, google_tag_manager.rs, rsc.rs, shared.rs): Correct across all four sites, reduces the dependency surface, and all Regex::new expect() messages follow the "should ..." convention.
• 204 No Content exclusion: RFC 9110 §15.3.5 prohibits a message body on 204 — the guard placement before take_body() and the dedicated test both make this correct and visible.

🌱 seedling

• No end-to-end test for the PassThrough path: The gate-logic tests are good but none of them call handle_publisher_request() with a backend returning a binary content type. An integration-level test exercising the full path would catch regressions in the gate condition itself. Follow-up suggestion — does not block merge.

CI Status

• Integration tests: PASS
• fmt / clippy / cargo test: NOT RUN (CI gates only trigger on PRs targeting main; expected for phased branching strategy)

[ChristianPavilonis]

Review Summary

2 findings (1× P2, 1× P3) — both are suggestions for improvement, no blockers.

[prk-Jr]

Summary

Adds PublisherResponse::PassThrough for non-processable 2xx responses (images, fonts, video), streaming them directly to the client via send_to_client() instead of buffering in WASM memory. The implementation is correct: Content-Length is preserved, 204 No Content is properly excluded, and EC/consent headers are still applied. The 51% TTFB improvement is well-documented with real staging measurements.

Non-blocking

🤔 thinking

• PassThrough doc implies finalize_response() is the immediate caller's obligation — but in main.rs it's called by the outer route_request path, not the PassThrough arm itself. Current code is correct; the doc could mislead a future adapter implementor. (publisher.rs:277)

♻️ refactor

• Redundant get_status() call — is_success: bool is captured at line 455, then response.get_status() is called again at line 470 to produce status. The inner status.is_success() duplicates is_success. Hoist let status = response.get_status() before the outer if and replace let is_success = ... with let is_success = status.is_success(). (publisher.rs:470)

📝 note

• No debug log when returning PassThrough — the block logs "Skipping response processing..." at the top but has no log at the PassThrough return. A log::debug!("Pass-through: binary response, Content-Type: {}, status: {}") would help with production tracing. (publisher.rs:473)
• Gate tests verify boolean arithmetic, not handle_publisher_request() directly — consistent with the existing streaming_gate_* pattern and an accepted trade-off given the Fastly runtime can't be mocked. Noted for awareness. (publisher.rs:766)

⛏ nitpick

• 205 Reset Content not excluded — RFC 9110 §15.3.6 says 205 MUST NOT include message body content. Adding status != StatusCode::RESET_CONTENT alongside the 204 guard would be spec-complete, low risk. (publisher.rs:471)

👍 praise

• send_to_client() over stream_to_client() — correct call; stream_to_client() forces chunked encoding which would corrupt Content-Length for binary assets. (main.rs:247)
• 204 No Content excluded correctly — HTTP spec prohibits a body in 204; tested and documented. (publisher.rs:471)
• apply_ec_headers called before the PassThrough gate — EC/consent headers are correctly applied to pass-through responses, consistent with existing buffered behaviour.
• Staging performance results — 51% TTFB, 28% FCP improvement with methodology documented. Exemplary.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS

[prk-Jr]

♻️ refactor — Redundant get_status() call

is_success: bool is captured at line 455 but response.get_status() is called again here. The inner status.is_success() re-checks what is_success already holds.

Suggested fix: hoist status before the outer if block:

let status = response.get_status();
let is_success = status.is_success();

if !should_process || request_host.is_empty() || !is_success {
    // ...
    if is_success && status != StatusCode::NO_CONTENT && !should_process {

Eliminates the second get_status() call and makes the guard conditions read symmetrically.

[prk-Jr]

🤔 thinking — Doc implies finalize_response() is the immediate caller's obligation, but it happens implicitly in the outer path

The variant doc lists:

/// 2. Call `finalize_response()` on the response

In main.rs, the PassThrough arm only does response.set_body(body); Ok(response). finalize_response() is called by the general route_request control flow — not explicitly in the PassThrough branch. The current code is correct, but the doc reads as a per-arm obligation.

A future adapter (Axum, Cloudflare) implementing its own route_request could interpret this as calling finalize_response() in the match arm, then have it called again by the outer path. Consider updating to:

/// The adapter must call `response.set_body(body)` before sending.
/// `finalize_response()` and `send_to_client()` are the adapter's
/// responsibility at the outer response-dispatch level.

[prk-Jr]

⛏ nitpick — 205 Reset Content not excluded alongside 204

RFC 9110 §15.3.6: "A server MUST NOT generate content in a 205 response." If a misbehaving origin sends 205 with Content-Length, PassThrough would relay it. Low risk in practice, but spec-complete would be:

if is_success
    && status != StatusCode::NO_CONTENT      // 204
    && status != StatusCode::RESET_CONTENT   // 205 — RFC 9110 §15.3.6
    && !should_process
{

[prk-Jr]

👍 praise — 204 No Content exclusion is correct and well-reasoned

HTTP spec prohibits a message body in 204 responses. Excluding it from PassThrough (which reattaches a body and calls send_to_client()) is correct. Tested and documented.

[prk-Jr]

📝 note — No debug log before returning PassThrough

The block logs "Skipping response processing - should_process: ..., status: ..." at the top, but there's no log when actually returning PassThrough. Production tracing would benefit from:

log::debug!(
    "Pass-through: binary response, Content-Type: {}, status: {}",
    content_type, status
);
return Ok(PublisherResponse::PassThrough { response, body });

[prk-Jr]

📝 note — Gate tests verify boolean arithmetic, not handle_publisher_request() directly

This matches the existing streaming_gate_* pattern, so it's consistent. All five pass_through_gate_* tests verify the formula is_success && !should_process in isolation — a typo in the actual gate condition would pass them all undetected.

pass_through_preserves_body_and_content_length is more meaningful since it exercises real Fastly API behaviour. Given the runtime can't be mocked in unit tests, this is a known trade-off — not a blocker.

[prk-Jr]

👍 praise — Correct choice of send_to_client() over stream_to_client()

stream_to_client() forces chunked Transfer-Encoding which would conflict with the origin's Content-Length header, causing browsers to see a malformed response for binary assets. send_to_client() lets Fastly relay the body from its internal buffer with the original headers intact. Clean and correct.

[prk-Jr]

⛏ nitpick — PR title references io::copy but the implementation uses send_to_client()

The title reads: Phase 4: Stream binary pass-through responses via io::copy

The first commit used io::copy + stream_to_client(), but that was superseded by commit ef576a19 ("Preserve Content-Length for PassThrough by using send_to_client"). The final implementation has no io::copy anywhere — send_to_client() is the right description.

Suggested title: Phase 4: Stream binary pass-through responses via send_to_client()