Phase 2: Stream responses to client via StreamingBody

[aram356]

Summary

Stream HTTP responses directly to the client via Fastly's StreamingBody API when Next.js is disabled. This eliminates full-body buffering on the proxy path, reducing peak memory from ~4x response size to constant and improving TTFB.

Closes #573, closes #574, closes #575, closes #576.
Part of epic #563. Depends on Phase 1 (#583).

What changed

Entry point migration (main.rs):

• Replaced #[fastly::main] with undecorated main() using Request::from_client()
• route_request returns Option<Response> — None when the streaming path already sent the response via stream_to_client()
• All non-streaming routes use explicit send_to_client()

process_response_streaming now generic over W: Write (publisher.rs):

• Changed from returning Body (internal Vec<u8>) to writing into &mut W
• Enables passing StreamingBody directly as the output sink
• Deduplicated PipelineConfig creation and content-encoding extraction

Streaming path via PublisherResponse enum (publisher.rs):

• handle_publisher_request returns PublisherResponse::Stream or PublisherResponse::Buffered
• Streaming gate: should_process && !request_host.is_empty() && (!is_html || !has_post_processors)
• Synthetic ID / cookie headers set before body processing (body-independent)
• Content-Length removed before streaming (chunked transfer)
• stream_publisher_body() public API bridges core ↔ adapter
• Error handling: pre-stream errors → send_to_client() with status; mid-stream → log and drop(streaming_body) (abort)

Files changed

File	Lines	What
main.rs	+56 -15	Entry point migration, streaming dispatch
publisher.rs	+185 -120	PublisherResponse enum, W: Write refactor, streaming gate

Task 10 (Chrome DevTools metrics) — deferred

Requires a running publisher origin to measure TTFB/TTLB. Local dev uses localhost:9090 which has no mock server. Metrics capture deferred to staging deployment. See #577.

Verification

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

Test plan

• All existing tests pass (no behavioral regression)
• WASM build succeeds
• Clippy and fmt clean
• Manual Viceroy verification with real origin (staging)
• Chrome DevTools TTFB/TTLB comparison (Phase 2, Task 10: Chrome DevTools MCP baseline and comparison #577)

[prk-Jr]

Summary

Clean Phase 2 implementation. The PublisherResponse enum correctly models the streaming/buffered decision at the type level, the entry point migration is well-motivated, and the streaming error stratification handles all three failure tiers (pre-stream, mid-stream, finish) correctly. CI is all green.

Non-blocking

🤔 thinking

• test_content_type_detection tests shadow logic (publisher.rs:487) — the test reimplements should_process inline rather than calling production code. If the logic in handle_publisher_request changes, this test won't catch it. It also checks text/css and text/javascript as separate conditions, but the production code uses content_type.contains("text/") — currently equivalent but coupled by coincidence. Consider extracting is_processable_content_type(ct: &str) -> bool and testing that directly.

🌱 seedling / ⛏ nitpick

See inline comments.

👍 praise

See inline comments.

CI Status

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

[ChristianPavilonis]

Review Summary

Clean, well-scoped Phase 2 implementation (~375 lines, 2 files). The PublisherResponse enum, W: Write generic refactor, and entry-point migration are well-designed. CI is green.

Highlights:

• PublisherResponse enum models streaming/buffered at the type level — compile-time correctness
• Entry point migration doc comment explains why — excellent for maintainers
• Three-tier error stratification (pre-stream → HTTP response, mid-stream → log+abort, finish → log)
• W: Write generic is the minimal right abstraction
• PipelineConfig deduplication removes prior triplication

Totals: 1 P1, 4 P2, 3 P3 — No blockers.

---

Findings not placeable on diff lines (test code unchanged in this PR):

⛏ P3: PR description lists "2xx status" gate but code doesn't implement it
The PR description says "The backend returns a 2xx status" as a streaming condition, but this isn't implemented. Either update the description or add the check (see P1 inline comment on the streaming gate).

🤔 P2: Test reimplements predicate differently than production (publisher.rs ~L486-491)
test_content_type_detection uses individual contains("text/html") || contains("text/css") || ... but production uses broad contains("text/"). The test won't catch regressions if the production predicate changes. Consider extracting the predicate into a named function (fn should_process_content_type(ct: &str) -> bool) and testing that directly.

⛏ P3: test_content_encoding_detection tests Fastly SDK, not app logic (publisher.rs ~L558-578)
This test creates a Request, sets headers, then reads them back — it tests Fastly SDK header behavior, not the application's content-encoding handling. Consider testing Compression::from_content_encoding directly instead.

⛏ P3: UTF-8 tests verify stdlib behavior, not application code (publisher.rs ~L517-549)
test_invalid_utf8_handling and test_utf8_conversion_edge_cases verify String::from_utf8 behavior — standard library invariants. They provide no regression coverage of application code. Consider replacing with tests that exercise actual invalid UTF-8 handling through the pipeline.

[aram356]

Addressing review feedback:

Fixed in this push:

• OwnedProcessResponseParams fields narrowed to pub(crate)
• Buffered path now sets Content-Length to output.len() instead of removing it
• Added has_html_post_processors() -> bool on IntegrationRegistry to avoid Vec<Arc<…>> clone
• Extracted is_processable_content_type() function and updated test_content_type_detection to test production code directly instead of reimplementing the predicate
• Fixed stray merge artifact in apply_synthetic_id_headers

Addressed in later phases:

• P1 missing 2xx status guard + PR description mismatch → Phase 3 (Phase 3: Make script rewriters fragment-safe for streaming #591) adds the 2xx streaming gate with streaming_gate_blocks_non_2xx_responses test
• Binary responses still fully buffered → Phase 4 (Phase 4: Stream binary pass-through responses via io::copy #594) adds PassThrough variant for non-processable 2xx responses

Acknowledged (P3 nitpicks):

• test_content_encoding_detection tests SDK behavior — valid observation, low risk
• UTF-8 tests verify stdlib — valid observation, low risk

[ChristianPavilonis]

Summary

Overall this is a well-executed PR with strong test coverage (754 passing), clean architecture (PublisherResponse enum, generic W: Write, process_chunks consolidation), and significant security improvements (synthetic ID validation, proxy allowlist, error propagation from integration builders).

Requesting changes for three issues:

1. Streaming gate should check HTTP status code — non-2xx responses (404/500 error pages) will be streamed with JS injection
2. Unsupported Content-Encoding fallback — misbehaving origins returning unsupported encodings produce broken streamed responses
3. Proxy allowlist docs inaccurate — docs say only redirects are checked, but code also checks initial target

Additional suggestions: fix synthetic ID debug logging inconsistency, update CHANGELOG, and correct the "2xx" doc comment.

---

Cross-cutting findings (not in diff)

🔧 Proxy allowlist docs only describe redirect enforcement, but code also enforces on initial target

Files: docs/guide/first-party-proxy.md (~line 441), docs/guide/configuration.md (~line 761)

The docs say the allowlist applies when "a proxied request receives an HTTP redirect," but proxy.rs now also checks the initial target host before the first request. Operators reading the docs would think only redirect destinations are filtered.

Suggestion: Update to say: "Both the initial proxy target and any redirect destinations are checked against allowed_domains."

🤔 Synthetic ID logged in debug output

File: crates/trusted-server-core/src/publisher.rs (~line 383-386, not in diff)

log::debug!("Proxy synthetic IDs - trusted: {}, ...") exposes the raw synthetic ID value. The rest of this PR carefully avoids logging raw synthetic IDs (e.g., synthetic.rs changed to log without the value). This log line is inconsistent with that approach.

Suggestion: Remove the synthetic ID value from the log message.

🤔 CHANGELOG missing Phase 2 entries

File: CHANGELOG.md (~line 8-13, not in diff)

The [Unreleased] section only has the synthetic ID validation entry. This PR introduces several user-facing changes that should be documented: streaming response support, proxy.allowed_domains, integration config errors failing startup, placeholder secret policy change, entry point migration.

[prk-Jr]

Summary

This change wires StreamingBody into the publisher proxy and moves response finalization into explicit send/stream paths. I found two blocking correctness issues in the new streaming gate/fallback logic plus one scope question.

CI Status

• prepare integration artifacts: PASS
• integration tests: PASS
• browser integration tests: PASS
• local fmt/clippy/rust/js verification: not rerun in this review (the PR description reports clean runs)

[prk-Jr]

🔧 wrench — This gate is still too permissive for HTML. has_html_post_processors() only catches the Next.js path, but google_tag_manager registers a script rewriter without a post-processor and create_html_processor() still switches HTML into new_buffered() whenever any script rewriter is present. In that configuration we commit headers via stream_to_client() even though the body is still fully buffered, so the constant-memory and clean-error guarantees no longer hold.

Fix: Gate HTML streaming on script rewriters as well, or narrow Phase 2 to configurations with neither script rewriters nor post-processors.

[prk-Jr]

🔧 wrench — The unsupported-encoding fallback still routes the body through process_response_streaming(). Because Compression::from_content_encoding() maps unknown encodings to None, a Content-Encoding: zstd HTML/JS/JSON response will be treated as identity-encoded bytes instead of being passed through unchanged or rejected explicitly.

Fix: If the encoding is unsupported, bypass rewriting and return the origin response untouched, or raise an explicit error before calling the rewrite pipeline.

[prk-Jr]

❓ question — The PR summary/spec says Phase 2 covers binary pass-through, but this early return keeps every !should_process response on the plain buffered path. Is binary streaming intentionally deferred, and if so should the PR description/spec be narrowed to match the implementation?