Repository navigation
Use request-scoped EdgeZero HTTP and body ownership - #1527
ChristianPavilonis wants to merge 3 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES
Reviewed a020daeec30ceae29ae023b7941853cad8bef8de against base e529e5576c1052f0d2fb40a4f9920f0428e6e633. Six actionable findings are attached inline. All corrections are prose proposals; no one-click suggestions are included.
🔧 Additional runtime blockers: Fastly HTML EOF hangs, assembled winning bids are missing, and browser/integration checks fail. Their root causes remain unresolved; I have not attributed them to the inline findings.
Evidence combines exact pinned SDK source and current-head CI logs. Local Rust formatting passed. No full local suite or fresh runtime reproduction was performed; the forwarding, response-cap, and TLS conclusions are source-established.
CI status
- CodeQL: PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- trace runtime and browser acceptance: FAIL
- browser integration tests: FAIL
- integration tests: FAIL
- integration tests (Fastly EC lifecycle): FAIL
- cargo test (ts CLI, native) (macos-latest): FAIL
- CLAUDE.md symlink guard: PASS
- prepare integration artifacts: PASS
- format-docs: PASS
- worker-deadlines (spin): FAIL
- worker-deadlines (cloudflare): FAIL
- cargo test: FAIL
- cargo test (cross-adapter parity): FAIL
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- format-typescript: PASS
- cargo test (ts CLI, native) (ubuntu-latest): FAIL
- docs-proposal scripts: PASS
- vitest: PASS
- ts dev lint domains (changed lines): FAIL
- CI Node pin guard: PASS
- cargo fmt: FAIL
- cargo test (axum native): FAIL
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- HTML EOF runtime (Fastly): FAIL
| let dispatch = match preflight(&self.settings, request) { | ||
| prepare_trusted_forwarder_parts(request.parts_mut(), &self.settings); | ||
| let dispatch = match preflight(&self.settings, request.parts_mut()) { | ||
| TracePreflight::NotTrace => { |
There was a problem hiding this comment.
🔧 P1 — Native forwarding loses the authenticated public origin.
TS captures forwarding assertions after the new SDK has stripped them. Valid CLI-proxied trace actions consequently fail origin validation with 403, and publisher rewriting loses the browser authority/port. Preserve authenticated capture before sanitization through a compatible SDK boundary. This blocker is already disclosed in the PR; Fastly’s earlier capture protects its path.
| edgezero-adapter-spin = { git = "https://github.com/IABTechLab/edgezero", tag = "v0.0.11", default-features = false } | ||
| edgezero-cli = { git = "https://github.com/IABTechLab/edgezero", tag = "v0.0.11" } | ||
| edgezero-core = { git = "https://github.com/IABTechLab/edgezero", tag = "v0.0.11", default-features = false } | ||
| edgezero-adapter-axum = { git = "https://github.com/IABTechLab/edgezero", rev = "456313675b6a85e11acea870240f6a6d869f1137", default-features = false } |
There was a problem hiding this comment.
🔧 P1 — The SDK migration leaves existing test consumers uncompilable.
The dependency bump breaks CLI fixtures using the old AxumDevServer constructor/configuration and parity fixtures importing EdgeZeroAxumService and using the previous stream error type. Hosted CI confirms these compilation failures. Migrate these consumers while retaining their behavioral assertions.
| match self.target { | ||
| AuctionTargetId::Fastly | AuctionTargetId::Cloudflare => Some(10 * 1024 * 1024), | ||
| AuctionTargetId::Spin => Some(8 * 1024 * 1024), | ||
| AuctionTargetId::Axum => None, |
There was a problem hiding this comment.
🔧 P2 — Axum accidentally inherits a 1 MiB response ceiling.
This leaves Axum’s limit unset, which retains the SDK’s 1 MiB buffered default. A previously supported 2 MiB publisher response will therefore fail before processing, despite the publisher’s default 16 MiB limit. Set an explicit compatible response policy and preserve caller limits.
| .backend(Arc::new(FastlyPlatformBackend)) | ||
| .http_client(Arc::new(FastlyPlatformHttpClient)) | ||
| .http_client( | ||
| trusted_server_core::platform::EdgeZeroHttpClient::for_request( |
There was a problem hiding this comment.
🔧 P2 — Fastly ignores the registered certificate policy.
The replacement client uses backend_name only for correlation. The SDK creates another backend with certificate verification enabled, so documented proxy.certificate_check = false configurations stop supporting self-signed development origins. Preserve this existing setting through the shared transport contract or select a compatible SDK revision.
| @@ -1,33 +1,44 @@ | |||
| //! Local runtime fixture exercising the production HTTP client implementation. | |||
| //! Local fixture using the production request-local EdgeZero client and driver. | |||
There was a problem hiding this comment.
🔧 P2 — Both worker deadline fixtures fail lint before runtime execution.
Their new first-line comments contain bare EdgeZero, triggering clippy::doc_markdown under -D warnings. Backtick the name in both this Cloudflare fixture and crates/trusted-server-adapter-spin/tests/runtime/deadline.rs:1.
| /// ``` | ||
| pub async fn assert_sign_inputs(router: &RouterService) { | ||
| let valid = Body::stream(futures::stream::iter([ | ||
| Bytes::from_static(b"{\"url\":\"https://asset."), |
There was a problem hiding this comment.
🔧 P2 — The streamed signing fixture fails the domain gate.
The fixture splits the URL after https://asset.; the linter recognizes disallowed host asset. Move the frame boundary before the URL scheme while preserving the streamed JSON test. No allowlist change is needed.
Summary
Draft. Required current-head checks have failed. The current-main forwarding/provenance incompatibility below is not waived.
Stack and exact input
Head:
a020daeec30ceae29ae023b7941853cad8bef8de. Base:review/shared-async-ec.Retain the original demo RC #1424 and its dirty worktrees. No source from excluded #1499 or the independent #1506 implementation is imported or coordinated. Relay/privacy work remains separate. No deployment, live input change, main merge, old-RC retirement or recovery deletion is part of this PR.
Changes
Closes
Closes #1523.
Spec
Not a feature. This is the scoped demo caller/API port, independent of the separate broad cutover/spec initiative. It reuses completed upstream minimum compatibility #460; buffer-group, codec-subset, phase-timeout, TLS/cap-policy and relay/privacy experiments remain excluded.
Test plan
Compatibility stop: selected SDK4563136 removes X-Forwarded-Host/Proto before
app.begin_ingress, while current TS main's credential-authenticatedForwarderPreparationneeds those assertions afterward. The isolated test-only constructor correction compiles and strict CLI Clippy passes, but real proxy regressions still return403 instead of200 and lose the browser authority/port. No trust policy, SDK hook/order change, raw-header fallback or assertion relaxation was implemented. Execution is paused for the scope choice between compatible-baseline demo cleanup and separately scoped current-main forwarding/provenance compatibility.Source: SDK native boundary. Native proxy regression output is retained privately with exact tested patches. Parity API compile and earlier Fastly EOF failures remain open. Narrow local fixture/profile work is preserved but unpublished; it is not a full current-head pass.
The inherited HTTP fixture now uses
pbs.example.cominstead of a TEST-NET IP rejected by the repository domain linter. All27 focused auction-plan tests and strict Core Clippy passed on that test-only correction; no allowlist/suppression, behavior or assertion was relaxed. Other unchanged-input results below remain bounded to their recorded source.Exact source committed in232975ab4, followed only by lower-layer review documentation merges: Core3382 passed/2 existing ignores; native client20 passed; Axum suite and Core/Axum strict Clippy/format passed. Registry version-set comparison against the exported port has no additions/removals. The later native descendant's WASM compile results are not presented as this head's full gate result. Fresh current-head CI, required broader tests and EOF/deadline runtime checks remain pending.
Checklist