Skip to content

Migrate handler layer to HTTP types (PR 12)#624

Open
prk-Jr wants to merge 12 commits intofeature/edgezero-pr11-utility-layer-migration-v2from
feature/edgezero-pr12-handler-layer-types
Open

Migrate handler layer to HTTP types (PR 12)#624
prk-Jr wants to merge 12 commits intofeature/edgezero-pr11-utility-layer-migration-v2from
feature/edgezero-pr12-handler-layer-types

Conversation

@prk-Jr
Copy link
Copy Markdown
Collaborator

@prk-Jr prk-Jr commented Apr 8, 2026

Summary

  • Move the PR 12 handler boundary to http request/response types so core handlers no longer depend on Fastly request/response APIs.
  • Keep Fastly isolated to the adapter entry/exit path and the still-unmigrated integration/provider boundary, which keeps PR 13 scope separate.
  • Lock the migrated surface with tests and migration guards so later changes do not accidentally reintroduce handler-layer Fastly types.

Changes

File Change
crates/trusted-server-adapter-fastly/src/main.rs Move the conversion boundary to the adapter entry/exit path and route core handlers with HTTP-native requests and responses.
crates/trusted-server-core/src/auction/endpoints.rs Migrate the /auction handler to http types and rebuild a Fastly request only at the provider-facing AuctionContext boundary.
crates/trusted-server-core/src/auction/formats.rs Consume HTTP requests directly and return HTTP auction responses.
crates/trusted-server-core/src/auction/orchestrator.rs Keep provider parsing Fastly-based while making the provider response conversion explicit in the orchestrator.
crates/trusted-server-core/src/compat.rs Add the minimal borrowed HTTP-to-Fastly request bridge needed for the remaining provider boundary.
crates/trusted-server-core/src/geo.rs Make response header injection operate on HTTP responses.
crates/trusted-server-core/src/integrations/google_tag_manager.rs Adapt the integration boundary so the shared proxy helper can stay HTTP-native while the integration trait remains Fastly-based.
crates/trusted-server-core/src/integrations/gpt.rs Adapt the integration boundary so the shared proxy helper can stay HTTP-native while the integration trait remains Fastly-based.
crates/trusted-server-core/src/integrations/testlight.rs Adapt the integration boundary so the shared proxy helper can stay HTTP-native while the integration trait remains Fastly-based.
crates/trusted-server-core/src/migration_guards.rs Extend the guard to cover the handler modules migrated in PR 12.
crates/trusted-server-core/src/proxy.rs Migrate first-party proxy/click/sign/rebuild handlers and response rewriting to HTTP request/response types.
crates/trusted-server-core/src/publisher.rs Migrate publisher handlers to HTTP types and use the platform HTTP client for publisher-origin fetches.
crates/trusted-server-core/src/request_signing/endpoints.rs Migrate request-signing and admin handlers to HTTP request/response types.

Closes

Closes #493

Test plan

  • cargo test --workspace
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • JS tests: cd crates/js/lib && npx vitest run (not run; no JS/TS files changed)
  • JS format: cd crates/js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: verified remaining Fastly usage is limited to the adapter boundary and the still-unmigrated integration/provider boundary

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!)
  • New code has tests
  • No secrets or credentials committed

@prk-Jr prk-Jr self-assigned this Apr 8, 2026
@prk-Jr prk-Jr changed the title (PR 12) Migrate handler layer to HTTP types Migrate handler layer to HTTP types (PR 12) Apr 8, 2026
@prk-Jr prk-Jr linked an issue Apr 8, 2026 that may be closed by this pull request
@prk-Jr prk-Jr linked an issue Apr 9, 2026 that may be closed by this pull request
Copy link
Copy Markdown
Collaborator

@ChristianPavilonis ChristianPavilonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Clean migration of the handler layer from fastly::Request/fastly::Response to http::Request<EdgeBody>/http::Response<EdgeBody>. The conversion boundary is pushed to the adapter entry/exit and the still-unmigrated integration trait boundary. Well-tested, CI passes.

Non-blocking

♻️ refactor

  • Duplicate error-to-response functions: http_error_response (new, returns HttpResponse) and to_error_response (existing, returns FastlyResponse) implement identical logic with different return types. Expected migration scaffolding — track for cleanup in PR 15 when Fastly types are fully removed. (main.rs:255-267 / error.rs:10)

CI Status

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

Comment thread crates/trusted-server-adapter-fastly/src/main.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/auction/formats.rs
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Solid mechanical migration of the handler layer from Fastly request/response types to http crate types. The conversion boundary is cleanly pushed to the adapter entry point, with proper compat shims for the still-unmigrated integration/provider boundary.

Non-blocking

🤔 thinking

  • platform_response_to_fastly duplicated into orchestrator.rs: Copy-pasted from proxy.rs rather than shared via compat.rs. Two separate HTTP→Fastly response paths risk divergence (this copy uses set_header which drops multi-value headers; compat::to_fastly_response doesn't). (orchestrator.rs:15)
  • sync→async handle_publisher_request: Public API signature change — only caller uses block_on so it works, but worth documenting. (publisher.rs:305)

♻️ refactor

  • Cursor::new(body.into_bytes()) repeated 4 times: Could extract a small body_as_reader helper to centralize body materialization. (proxy.rs:175, publisher.rs:228,246,263)

🌱 seedling

  • DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUT hardcoded at 15s: Good to make it explicit, but may need to be configurable per deployment. (publisher.rs:22)

📝 note

  • Integration proxy double-conversion: GTM, GPT, testlight each wrap proxy_request with from_fastly_request/to_fastly_response, copying bodies twice. Expected for the PR 13 boundary.

⛏ nitpick

  • Inconsistent test type alias style: proxy.rs tests use HttpMethod/HttpStatusCode prefixes; publisher.rs tests don't. The unprefixed style is cleaner.

CI Status

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

Comment thread crates/trusted-server-core/src/auction/orchestrator.rs Outdated
Comment thread crates/trusted-server-core/src/auction/orchestrator.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/google_tag_manager.rs
@prk-Jr prk-Jr requested a review from aram356 April 13, 2026 12:31
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Mechanically sound migration of the handler layer from fastly::{Request,Response} to http::{Request,Response}<EdgeBody>. The adapter entry/exit is now the only fastly conversion site for handlers, and the migration guard test is extended to lock this in. Overall high quality — a handful of concrete concerns to address before merge, mostly around silent failure modes that the type migration makes easier to miss.

Blocking

🔧 wrench

  • Silent EdgeBody::Stream dropcompat.rs:132-134 and auction/orchestrator.rs:29-32 log a warning and drop the body on the Stream variant. No test pins this behavior; debug_assert! only fires in debug. If a streaming body ever reaches this boundary, the client gets an empty 200.
  • Unbounded request body → Stringproxy.rs:889 and proxy.rs:1003 do String::from_utf8(req.into_body().into_bytes().to_vec()) with no size cap. OOM/DoS risk on hostile POST.
  • Silent JSON serialization fallbackproxy.rs:1156 returns {} on serde_json::to_string failure instead of propagating an error.
  • Geo lookup moved before authmain.rs:95-110 runs the geo lookup on every request, including ones that will immediately 401. Confirm this is intentional.

❓ question

  • Integration trait still fastly-basedmain.rs:182-196 pays a full http → fastly → http round-trip per integration request. Deferred to PR 13? If so, please leave a // TODO(PR13) marker.
  • to_fastly_request_ref drops bodyauction/endpoints.rs:90 call site has no comment explaining why the empty body is safe. Please add one line so this invariant doesn't regress.

Non-blocking

🤔 thinking

  • Multi-valued Set-Cookie survival through rebuild_response_with_body (proxy.rs:138) — add a test, also consider mem::take to avoid the header clone.
  • Invalid settings.response_headers are silently warn-skipped at request time (main.rs:250-262); CLAUDE.md prefers startup-time validation.

♻️ refactor

  • migration_guards.rs uses substring matching on "fastly::Request" — prefer a \bfastly::(Request|Response|http::(Method|StatusCode))\b regex to avoid false positives on future string literals or doc comments.

🌱 seedling

  • End-to-end streaming (EdgeBody::Stream) is effectively unreachable today. Worth a follow-up to preserve streaming semantics through publisher fetch → rewrite → response once PR 13 lands.

📝 note

  • publisher.rs:325 vs publisher.rs:361 — inconsistent services.client_info() vs services.client_info.client_ip access idiom.

👍 praise

  • Tight adapter boundary in main.rs:89-120: one from_fastly_request/to_fastly_response pair framing route_request. Very clean.
  • insert_geo_header warn-and-continue is the right shape for derived geo data.
  • Migration guard extended to cover the handler layer — exactly the right regression gate for this refactor.

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS
  • js tests: PASS

(All three GitHub Actions checks — browser integration tests, integration tests, prepare integration artifacts — are green.)

  • publisher.rs:325 vs publisher.rs:361 — inconsistent services.client_info() (method) vs services.client_info.client_ip (field) access idiom. Please pick one.

Comment thread crates/trusted-server-core/src/compat.rs
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/main.rs
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/main.rs
Comment thread crates/trusted-server-core/src/auction/endpoints.rs
Comment thread crates/trusted-server-core/src/geo.rs
Comment thread crates/trusted-server-core/src/migration_guards.rs
prk-Jr added 2 commits April 16, 2026 12:45
Brings in PR11 review-finding fixes (compat shim additions, testlight
integration changes, migration guard comments). Conflicts resolved by
keeping PR12's http-native handler layer throughout — all compat shim
insertions from PR11 are superseded by the http type migration.
@prk-Jr prk-Jr requested a review from aram356 April 16, 2026 12:54
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Second-round review. The PR addresses all four blocking findings and both questions from the prior review — unbounded body → String in sign/rebuild, silent JSON serialization fallback, geo lookup before auth, silent EdgeBody::Stream drop (now test-pinned in compat.rs), TODO(PR13) marker for the integration trait, and the body-drop comment on to_fastly_request_ref. CI is green.

Two new blocking issues — public POST endpoints /verify-signature and /auction still accept unbounded bodies — are the only security-material gaps in this round. The rest are follow-ups and polish.

Blocking

🔧 wrench

  • Unbounded body on /verify-signature (crates/trusted-server-core/src/request_signing/endpoints.rs:89) — public endpoint materializes the full request into memory with no cap. See inline.
  • Unbounded body on /auction (crates/trusted-server-core/src/auction/endpoints.rs:43) — public endpoint, same pattern. See inline.

Non-blocking

🤔 thinking

  • platform_response_to_fastly still duplicated (crates/trusted-server-core/src/auction/orchestrator.rs:19-40 vs crates/trusted-server-core/src/compat.rs:119-138) — the prior review flagged this. Now consistent on append_header and documented as PR 15 removal, which is an improvement, but platform_resp.response is already http::Response<EdgeBody> and can be passed to compat::to_fastly_response directly. See inline.
  • Admin endpoints also lack body-size caps (crates/trusted-server-core/src/request_signing/endpoints.rs:170, 270) — basic-auth-protected so attack surface is narrow, but defence-in-depth is cheap once RequestTooLarge exists. See inline.
  • EdgeBody::Stream silent drop in release (crates/trusted-server-core/src/compat.rs:74-77, 132-135 and crates/trusted-server-core/src/auction/orchestrator.rs:29-32) — tests at compat.rs:584-629 pin the drop-to-empty behaviour, and debug_assert! catches it in debug, but in release a future streaming backend would silently return a 200 with an empty body. Acceptable transient debt given PR 15 removal scope — please carry this into the PR 15 plan so streaming isn't silently lost when the boundary moves.

♻️ refactor

  • No 413 regression tests for the new size-capped endpoints (crates/trusted-server-core/src/proxy.rs:884-891 and :1007-1014) — the 65536-byte cap is a security control with no negative test. GTM has the pattern worth copying (crates/trusted-server-core/src/integrations/google_tag_manager.rs:1100-1137). See inline.

📝 note

  • response_headers validation uses .expect() in the hot path (crates/trusted-server-adapter-fastly/src/main.rs:249-255). Correct today because Settings::prepare_runtime validates at load (crates/trusted-server-core/src/settings.rs:486-498), but route_request keeps a defensive fallback for the handler regex with the same "validated at load time" invariant (main.rs:132-138). Consider matching that pattern — warn-skip instead of panic — for symmetry and one less panic site in the wasm binary.

CI Status

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

Comment thread crates/trusted-server-core/src/request_signing/endpoints.rs
Comment thread crates/trusted-server-core/src/auction/endpoints.rs Outdated
Comment thread crates/trusted-server-core/src/auction/orchestrator.rs Outdated
Comment thread crates/trusted-server-core/src/request_signing/endpoints.rs
Comment thread crates/trusted-server-core/src/proxy.rs
…tion

- Add VERIFY_MAX_BODY_BYTES (4096) cap to handle_verify_signature
- Add AUCTION_MAX_BODY_BYTES (65536) cap to handle_auction
- Add ADMIN_MAX_BODY_BYTES (4096) cap to handle_rotate_key and handle_deactivate_key
- Delete platform_response_to_fastly from orchestrator; both call sites now use crate::compat::to_fastly_response directly
- Add sign_rejects_oversized_body and rebuild_rejects_oversized_body regression tests asserting 413 on oversized POST bodies
@prk-Jr prk-Jr requested a review from aram356 April 22, 2026 12:48
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Round 3 review. The PR cleanly addresses every blocking finding from the prior two rounds: /verify-signature and /auction now enforce body-size caps (4 KiB and 64 KiB), platform_response_to_fastly was deleted from auction/orchestrator.rs (both call sites use crate::compat::to_fastly_response directly), and /admin/keys/{rotate,deactivate} got matching 4 KiB caps. The expect() in finalize_response (main.rs:251-253) is now safe because Settings::prepare_runtime validates response_headers at startup (settings.rs:486-498).

One new blocking finding: to_fastly_request_ref (introduced in PR 12) silently collapses multi-value headers, which is a latent correctness regression in the provider path. See inline.

CI is green; local cargo test --workspace (814 passed), cargo clippy --workspace --all-targets --all-features -- -D warnings, and cargo fmt --all -- --check all pass.

Blocking

🔧 wrench

  • to_fastly_request_ref clobbers multi-value headers: see inline at compat.rs:90.

Non-blocking

🤔 thinking

  • VERIFY_MAX_BODY_BYTES = 4096 may be tight for legitimate signed payloads (request_signing/endpoints.rs:78). After base64 signature (~88 bytes for Ed25519), kid, JSON wrapper, and quoting/escaping, the usable payload budget is ~3 KiB. Plausibly fine for typical OpenRTB-shaped payloads but can clash with publishers signing larger blobs (e.g. embedded events). Consider a brief // safety: comment explaining the chosen limit, or making it config-derived alongside the existing GTM max_beacon_body_size pattern.

📝 note

  • Body-size caps fire after the body is already materialized into memory (compat.rs:40-43). compat::from_fastly_request calls req.take_body_bytes() at the adapter entry, so the entire body is buffered before any handler-level cap runs. The new caps protect parse allocations but not the receive allocation. This matches the compat shim's semantics today and is a PR-15 concern, not PR-12 — worth pinning in the PR 15 plan that streaming-aware caps need to land with the boundary removal so the protection isn't quietly downgraded when the shim is replaced.

♻️ refactor

  • No 413 regression tests for /verify-signature, /auction, /admin/keys/{rotate,deactivate} (request_signing/endpoints.rs:93,183,292, auction/endpoints.rs:45). This commit added sign_rejects_oversized_body and rebuild_rejects_oversized_body (proxy.rs:2358-2395) — exactly what the previous review asked for. The same pattern wasn't applied to the four endpoints whose caps are introduced in this commit. Without negative tests, a future commit that drops any cap won't be caught. Same shape as the existing proxy tests — five lines per endpoint.

  • Body-size-cap pattern duplicated 6× across handlers. if body.len() > MAX { return Err(Report::new(RequestTooLarge { format!("X payload {} exceeds limit of {}", body.len(), MAX) })); } appears at proxy.rs:884, proxy.rs:1007, auction/endpoints.rs:45, and three times in request_signing/endpoints.rs (93, 183, 292). A small helper (fn enforce_max_body_size(bytes: &[u8], limit: usize, what: &'static str) -> Result<(), Report<TrustedServerError>>) in http_util would drop ~50 lines and centralize the message format. Defer if you'd rather land in a follow-up.

CI Status

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

Comment thread crates/trusted-server-core/src/compat.rs Outdated
@prk-Jr prk-Jr requested a review from aram356 April 27, 2026 08:15
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Round-4 review. Round-3's blocking finding (to_fastly_request_ref clobbering multi-value headers) is resolved at compat.rs:95 with a regression test at compat.rs:478. CI is green.

Two new concerns this round: (1) the round-3 ♻️ ask for 413 regression tests on the four endpoints whose caps were introduced in this PR is still unaddressed — only the proxy endpoints got sign_rejects_oversized_body/rebuild_rejects_oversized_body. (2) Two doc-vs-code mismatches in compat.rs — the # Panics blocks describe a panic site that doesn't exist (URL parsing has a silent fallback to /), while the real panic site (header construction) is undocumented.

Blocking

🔧 wrench

  • No 413 regression tests for /verify-signature, /admin/keys/{rotate,deactivate} (request_signing/endpoints.rs:103, :254, :381). Round-3 explicitly asked. See inline.
  • No 413 regression test for /auction; file has no #[cfg(test)] module (auction/endpoints.rs:45). See inline.

Non-blocking

🤔 thinking

  • # Panics doc claims URL-parse panic that cannot occur (compat.rs:42-44, compat.rs:56-58). Doc/code mismatch. See inline.
  • Silent URL fallback to / masks malformed URIs (compat.rs:18-21). build_http_request uses unwrap_or_else(|_| http::Uri::from_static("/")). Realistically unreachable given Fastly pre-validation, but produces silent misrouting (catch-all hits the publisher origin at main.rs:215) if it ever fires. Either document why the fallback is reachable-but-safe, or assert the invariant with expect("should parse Fastly-validated URL").

♻️ refactor

  • 65536 literal duplicated 4× in proxy.rs (proxy.rs:884, :887, :1007, :1010) — the only handler in this PR not using named constants. See inline.
  • String::from_utf8(body_bytes.to_vec()) allocates twice (proxy.rs:892, :1015). See inline.
  • Body-size cap pattern duplicated 6× (carry-over from round 3). Same if body.len() > MAX { return Err(RequestTooLarge { format!(...) }) } shape at proxy.rs:884, proxy.rs:1007, auction/endpoints.rs:45, and three times in request_signing/endpoints.rs (103, 254, 381). A small enforce_max_body_size(bytes, limit, what) helper in http_util would centralize the message format and remove ~50 lines.

🌱 seedling

  • DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUT = 15s still hardcoded (publisher.rs:21). Carry-over from round 1 — worth a config knob during PR-15 boundary cleanup.
  • Body-size caps still fire after req.take_body_bytes() materializes (compat.rs:46). Carry-over from round 3 — caps protect parse allocations but not the receive allocation. PR-15 streaming-aware shim should land alongside boundary removal so the protection isn't quietly downgraded.

📝 note

  • platform_response_to_fastly is now the only diverging conversion (compat.rs:245-264). Returns Err on streaming bodies instead of to_fastly_response's silent-drop. The asymmetry is correct for orchestrator use, but worth pinning in the PR-15 plan so streaming closes consistently.

CI Status

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

Comment thread crates/trusted-server-core/src/compat.rs Outdated
Comment thread crates/trusted-server-core/src/request_signing/endpoints.rs
Comment thread crates/trusted-server-core/src/auction/endpoints.rs
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
@prk-Jr prk-Jr requested a review from aram356 May 3, 2026 15:55
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Re-review of the handler-layer HTTP-types migration. The migration is structurally clean, but two concerns block merge: (1) the publisher streaming dispatch added in #562 is regressed — PublisherResponse::Stream now buffers the entire pipeline output instead of writing chunks via stream_to_client(), and (2) cargo test --workspace fails on the current head because the debug_assert! in compat::to_fastly_response (introduced in PR 11 and inherited here) contradicts the test that exercises the documented silent-drop fallback. CI did not catch (2) because PR 624's base branch causes test.yml and format.yml to skip.

Blocking

🔧 wrench

  • Streaming regression on the publisher fallback path. Before this PR, Ok(PublisherResponse::Stream { … }) in adapter-fastly/src/main.rs called response.stream_to_client() and wrote chunks via stream_publisher_body. After this PR, resolve_publisher_response materializes the entire pipeline output into a Vec<u8>, sets Content-Length, replaces the body, and the adapter sends via compat::to_fastly_response(response).send_to_client(). The whole publisher response is now buffered in WASM memory.

    Observable: the PublisherResponse::Stream enum variant becomes equivalent to PublisherResponse::Buffered until streaming is restored. TTFB is delayed by the full pipeline pass, and WASM memory pressure scales with body size.

    Structural cause: compat::to_fastly_response cannot move an EdgeBody::Stream into a fastly::StreamingBody — there is no streaming bridge across the compat shim. Two reasonable directions:

    • Preferred: Keep the streaming dispatch in the adapter. Have route_request return Result<HandlerOutcome, …> where HandlerOutcome::Streaming(…) carries the unbuffered body+params, and main() opens stream_to_client() on the converted Fastly response and calls stream_publisher_body against it. Honest "no behavior regressions" version of PR 12.
    • Alternative: Block the streaming arm and route everything through BufferedProcessed until PR 15 introduces a streaming body bridge — but explicitly call this out as a known regression in the PR description and add a tracking issue.

    No test today asserts streaming behavior. The publisher tests (stream_publisher_body_*) all use Vec<u8> outputs and don't distinguish streamed-to-client from buffered-then-sent. So the regression is entirely silent.

  • cargo test --workspace fails: compat::tests::to_fastly_response_with_streaming_body_produces_empty_body panics on a debug_assert! that contradicts the test.

    In crates/trusted-server-core/src/compat.rs (line ~138), the function asserts matches!(&body, EdgeBody::Once(_)). The test below it (line ~676) constructs an EdgeBody::Stream and asserts the body comes out empty — so the test panics in cargo test debug builds.

    Both the debug_assert! and the test were introduced in PR 11 (commit 76df6f8), so PR 624 inherits the contradiction rather than introducing it — but it materializes here because (a) the PR's test plan checkbox [x] cargo test --workspace does not pass on the current head, and (b) when this stack lands on main and a downstream PR opens, the standard test.yml workflow will fail. The PR 11 review can fix it upstream, or this PR can apply the fix.

    Fix (preferred — the warn-and-empty fallback is documented behavior):

    pub fn to_fastly_response(resp: http::Response<EdgeBody>) -> fastly::Response {
        // … build headers …
        match body {
            EdgeBody::Once(bytes) => {
                if !bytes.is_empty() {
                    fastly_resp.set_body(bytes.to_vec());
                }
            }
            EdgeBody::Stream(_) => {
                log::warn!("streaming body in compat::to_fastly_response; body will be empty");
            }
        }
        fastly_resp
    }

    Alternative: keep the debug_assert! and gate the test on #[cfg(not(debug_assertions))]. But the warn-log already covers misuse and the documented fallback path should be exercised by tests.

Non-blocking

🤔 thinking

  • Silent body drop on EdgeBody::Stream in compat. Both to_fastly_request and to_fastly_response log a warning and emit an empty body when handed a streaming EdgeBody. Combined with the streaming-dispatch removal above, this means a future caller that tries to stream through the compat boundary will silently see truncated bodies. Promote the misuse to a Result return (the Fastly adapter's edge_request_to_fastly already does this), or add a comment listing the audited non-streaming call sites so the constraint is verifiable. Not in PR 12's diff but the streaming-regression fix will reintroduce the constraint.

♻️ refactor

  • bytes.to_vec() copies the full body across the compat boundary. compat.rs:144-146 and compat.rs:81-84 memcpy the body. For large publisher responses this is unavoidable O(N) at the shim. Worth a one-line comment that this cost goes away with PR 15 so future readers don't try to optimize inside the shim. (Not in PR 12's diff.)

🌱 seedling

  • Sync between route_request admin routes and Settings::ADMIN_ENDPOINTS is comment-only. main.rs:166 carries a "Keep in sync with Settings::ADMIN_ENDPOINTS" comment. A test that compares the constant against the routes registered in route_request would prevent future drift. Follow-up issue.

📝 note

  • CI workflow gate gap. .github/workflows/test.yml and .github/workflows/format.yml trigger only on pull_request: branches: [main]. PR 624 targets the PR 11 feature branch, so cargo test, fmt, and clippy never ran on this PR. Only integration-tests.yml ran (and passed). This is how the failing compat test stayed invisible. Out of scope for PR 12 to fix — but the consequence (a regression that will only surface when this lands on main and a downstream PR opens) is in scope. Either drop the branches: [main] filter for the EdgeZero stack PRs, or workflow_dispatch the test workflow against each stack head.

CI Status

  • fmt: PASS (locally)
  • clippy: PASS (locally)
  • rust tests: FAIL locally (compat::tests::to_fastly_response_with_streaming_body_produces_empty_body); not run on CI for this PR
  • integration tests: PASS (CI)
  • browser integration tests: PASS (CI)

log::warn!("geo lookup failed: {e}");
None
})
};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — 401 responses no longer carry geo headers — silent behavior change.

Previously geo_info was looked up before routing and applied via finalize_response to every path including auth early-returns, so 401s had geo headers. Now geo_info = if status == UNAUTHORIZED { None } else { … } skips the lookup on 401.

This may be intentional (avoid leaking geo to unauthenticated callers) but it's a silent change. Add a one-line comment to make the rationale explicit:

// Skip geo lookup for 401s so geo headers are not exposed to unauthenticated
// callers. Authenticated routes do their own lookups for consent context.
let geo_info = if response.status() == StatusCode::UNAUTHORIZED {};

}))
}),
(m, path) if integration_registry.has_route(&m, path) => {
// TODO(PR13): migrate integration trait to http types here
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 noteTODO(PR13) is captured. The bridge through compat::to_fastly_request / compat::from_fastly_response is correct for the scope of this PR. No action here — confirming the deferred work is tracked.

.unwrap_or_else(|e| {
log::warn!("geo lookup failed: {e}");
None
});
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — Geo lookup is now duplicated.

The handler does its own lookup here for the consent context, and the adapter does another lookup at response-finalization time for header injection (main.rs finalize_response). Lookup is cheap (in-memory on Fastly) so this is more of a 📌 out of scope note — a follow-up could thread the same Option<GeoInfo> through both. Same pattern in auction/endpoints.rs:75-81.

use fastly::geo::{geo_lookup, Geo};
use fastly::{Request, Response};
use edgezero_core::body::Body as EdgeBody;
use fastly::geo::Geo;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 notefastly::geo::Geo import remains in core after this migration. The geo_from_fastly field-mapping helper still lives here, so crates/trusted-server-core retains a Fastly type leak after PR 12. Predates this PR and is acknowledged out of scope, but worth tracking for PR 13 / PR 15 since geo.rs is one of the migrated files.

let auction_status = match auction_result {
Ok(resp) => resp.status(),
Err(ref e) => e.current_context().status_code(),
};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ refactor — Dedup the match Result { Ok(r) => r.status(), Err(e) => e.current_context().status_code() } block.

It appears twice in this file (auction + publisher cases). A small helper at the top of the test module would clean this up:

fn status_of(
    result: &Result<HttpResponse, Report<TrustedServerError>>,
) -> StatusCode {
    match result {
        Ok(resp) => resp.status(),
        Err(e) => e.current_context().status_code(),
    }
}

Not a blocker — duplication is small.

params,
} => {
let mut output = Vec::new();
stream_publisher_body(body, &mut output, &params, settings, integration_registry)?;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench — Streaming regression: this arm now buffers the full pipeline output instead of streaming chunks to the client.

Before this PR, the adapter handled Ok(PublisherResponse::Stream { … }) by calling response.stream_to_client() and writing chunks via stream_publisher_body against the StreamingBody — the streaming pipeline added in #562 (commit 5ff3e676) emitted bytes as the upstream body was processed.

After this PR, resolve_publisher_response materializes the entire pipeline output into a Vec<u8>, sets Content-Length, replaces the body, and the adapter sends via compat::to_fastly_response(response).send_to_client(). The whole publisher response is now buffered in WASM memory:

  • PublisherResponse::Stream becomes observably equivalent to PublisherResponse::Buffered — the variant no longer means "stream".
  • TTFB is delayed by the full pipeline pass rather than starting at first chunk.
  • WASM memory pressure scales with body size for every publisher response.

Structural cause: compat::to_fastly_response cannot move an EdgeBody::Stream into a fastly::StreamingBody — there is no streaming bridge across the compat shim, so the only way to honor the variant is to streaming-dispatch from the adapter itself before going through compat.

Fix (preferred) — keep the streaming dispatch in the adapter:

// Have route_request return Result<HandlerOutcome, …> where HandlerOutcome
// distinguishes streamed vs buffered responses, then in main():
match outcome {
    HandlerOutcome::Streaming { mut response, body, params } => {
        finalize_response(&settings, geo_info.as_ref(), &mut response);
        let mut fastly_resp = compat::to_fastly_response_skeleton(response);
        let mut sb = fastly_resp.stream_to_client();
        if let Err(e) = stream_publisher_body(body, &mut sb, &params, &settings, &integration_registry) {
            log::error!("Streaming processing failed: {e:?}");
            drop(sb);
        } else if let Err(e) = sb.finish() {
            log::error!("Failed to finish streaming body: {e}");
        }
    }
    HandlerOutcome::Buffered(response) => {
        compat::to_fastly_response(response).send_to_client();
    }
}

Alternative — fold this arm into BufferedProcessed until PR 15 introduces a streaming body bridge, and call it out as a known regression in the PR description with a tracking issue.

No test today asserts streaming behavior — the publisher tests (stream_publisher_body_*) all use Vec<u8> outputs and don't distinguish streamed-to-client from buffered-then-sent. So the regression is entirely silent until you measure TTFB or memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thread RuntimeServices into integration and provider entry points Handler layer type migration

3 participants