Remove legacy entry point, canary routing, and flag plumbing#744
Remove legacy entry point, canary routing, and flag plumbing#744prk-Jr wants to merge 13 commits into
Conversation
route_tests.rs tested route_request() and HandlerOutcome, which are legacy-path only. Equivalent EdgeZero dispatch coverage exists in app.rs. Part of #501 legacy entry point cleanup.
Remove legacy_main(), route_request(), HandlerOutcome, and all flag-reading machinery (edgezero_enabled / edgezero_rollout_pct). Entry point is now a direct trampoline to edgezero_main(), which opens the config store internally. Part of #501 legacy entry point cleanup.
Remove from_fastly_request(), to_fastly_response_skeleton(), and their build_http_request() helper from compat.rs — all were only called from legacy_main(). Delete error.rs entirely (to_error_response() was its only export; only caller was legacy_main()). Part of #501.
Remove 14 canary-routing test functions and the JA4 debug test (if not already removed). Strip all imports that were only consumed by deleted code. Part of #501.
build_runtime_services() was the legacy-path per-request service factory. The EdgeZero path uses build_per_request_services() in app.rs instead. Remove the dead function, its noop_kv_store() test helper, and its tests. Part of #501.
middleware.rs referenced deleted finalize_response() and route_request(). app.rs referenced deleted http_error_response() and route_request(). platform.rs had an orphaned section header after build_runtime_services was removed. Part of #501.
Update middleware.rs module doc from 'dual-path' to 'EdgeZero entry point'. Update two prose mentions of route_request in app.rs to describe current EdgeZero behavior. Part of #501.
The app no longer reads these config store keys. Remove them from the local dev config store. The trusted_server_config store itself stays — it is still opened by open_trusted_server_config_store(). Part of #501.
Add backticks around EdgeZero in doc comments to satisfy doc_markdown lint. Remove unused Arc import from platform.rs test module leftover from build_runtime_services deletion. Part of #501.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR cleanly removes the legacy Fastly entry point, canary routing, and the EdgeZero flag plumbing (~710 lines of dead code). The deletions themselves are well-scoped and correct. However, the cutover is not mergeable as-is: it turns three previously-green CI checks red — cargo test, integration tests, and browser integration tests — and the integration failures indicate the EdgeZero path is not at the "full functional parity" the description claims. I confirmed on the base branch (5f81e80) that all three checks pass, so this PR introduces all three failures.
1 of the inline comments below carries a one-click GitHub
suggestion. The two blocking findings are cross-cutting — their fixes live outside this PR's diff — and are described in full in the Cross-cutting / body-level findings section.
Non-blocking
⛏ nitpick
middleware.rsmodule doc still references "the legacy entry point" — see inline atcrates/trusted-server-adapter-fastly/src/middleware.rs:4
Cross-cutting / body-level findings
-
🔧
cargo testfails — staleadmin_endpoints_match_fastly_routerparity test. The core test atcrates/trusted-server-core/src/settings.rs:2057doesinclude_str!("../../trusted-server-adapter-fastly/src/main.rs")and asserts everySettings::ADMIN_ENDPOINTSliteral (/admin/keys/rotate,/admin/keys/deactivate) appears inmain.rs. The basemain.rscarried those literals in the legacy router match arms (e.g.(Method::POST, "/admin/keys/rotate") => …); this PR deletes the legacy router, so the newmain.rscontains zero/admin/occurrences and the assertion fails — the wasm test binary aborts (SIGABRT, exit 134). This is deterministic, not a flake. The test's premise (admin routes hard-coded inmain.rs) is obsolete now that EdgeZero routes them via config[[handlers]], already enforced byvalidate_admin_coverage/from_toml_rejects_config_without_admin_handler. Fix: update or remove that core test in the same change set — it lives outside this PR's diff, so it can't be expressed as a GitHub suggestion. -
🔧
integration tests+browser integration testsfail — EdgeZero-only cutover is not at parity. On the base branch,main()readedgezero_enabled(default"false") and routed tolegacy_main, so both suites only ever exercised the legacy path there — and passed (verified on base commit5f81e80: all three checks green). This PR removes the fallback and forces all traffic through EdgeZero, and now:integration tests→test_all_combinationspanics with "Script tag not found in HTML" (tests/common/assertions.rs:66) — the EdgeZero path serves HTML without the injected TS publisher<script>;browser integration tests→ "Service … not ready after 30 attempts" — the EdgeZero-only server never becomes ready. This contradicts the PR / plan-doc claim that EdgeZero has reached full functional parity. The PR-body test plan listscargo test-fastly(48 passed) and JS, but not the workspacecargo test, integration, or browser suites — the exact gates now failing. Resolve the EdgeZero parity gap (or confirm these suites pass against EdgeZero) before removing the legacy fallback.
CI Status
- cargo fmt: PASS
- format-typescript: PASS
- format-docs: PASS
- vitest: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check/build (spin native + wasm32-wasip1): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- prepare integration artifacts: PASS
- cargo test: FAIL — see body finding #1
- integration tests: FAIL — see body finding #2
- browser integration tests: FAIL — see body finding #2
No checks are marked required under branch protection on this branch; per CLAUDE.md's CI gates these are still PR gates, so the failures are merge-blocking in practice.
| //! | ||
| //! Provides two middleware types that mirror the finalization and auth logic | ||
| //! from the legacy [`crate::finalize_response`] and [`crate::route_request`]: | ||
| //! used in the legacy entry point: |
There was a problem hiding this comment.
⛏ nitpick — Line 1 already states this module is for the EdgeZero entry point, but this line still says "the legacy entry point," which no longer exists after this PR. Small doc-accuracy fix:
| //! used in the legacy entry point: | |
| //! used by the `EdgeZero` entry point: |
2 participants
Summary
legacy_main,route_request,HandlerOutcome, and all canary flag-reading machinery (edgezero_enabled/edgezero_rollout_pct) now that EdgeZero has reached full functional parity and the cutover canary (PR19) is merged.errormodule, the legacyroute_testsfile, and unused compat functions that only existed to support the old code path.mainto a direct trampoline intoedgezero_main, removing ~710 lines of dead code and the two config-store keys fromfastly.toml.Changes
crates/trusted-server-adapter-fastly/src/main.rslegacy_main,route_request,HandlerOutcome, flag constants, and all associated imports (~710 lines removed)crates/trusted-server-adapter-fastly/src/error.rscrates/trusted-server-adapter-fastly/src/route_tests.rscrates/trusted-server-adapter-fastly/src/compat.rscrates/trusted-server-adapter-fastly/src/platform.rsbuild_runtime_services(legacy-only)crates/trusted-server-adapter-fastly/src/app.rsruntime_services_for_consent_routeandbuild_state(legacy-only); update importscrates/trusted-server-adapter-fastly/src/middleware.rsfastly.tomledgezero_enabledandedgezero_rollout_pctconfig-store entriesdocs/superpowers/plans/2026-05-27-pr20-legacy-cleanup.mdCloses
Closes #501
Test plan
cargo fmt --all -- --checkcargo clippy-fastlycargo clippy-axumcargo test-fastly— 48 tests passed, 0 failedcargo test-axum— 20 tests passed, 0 failedcd crates/js/lib && npx vitest run— 291 tests passedcd crates/js/lib && npm run formatcd docs && npm run formatChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)