Add platform abstraction layer with traits and RuntimeServices#545
Add platform abstraction layer with traits and RuntimeServices#545
Conversation
Rename crates/common → crates/trusted-server-core and crates/fastly → crates/trusted-server-adapter-fastly following the EdgeZero naming convention. Add EdgeZero workspace dependencies pinned to rev 170b74b. Update all references across docs, CI workflows, scripts, agent files, and configuration.
Introduces trusted-server-core::platform with PlatformConfigStore, PlatformSecretStore, PlatformKvStore, PlatformBackend, PlatformHttpClient, and PlatformGeo traits alongside ClientInfo, PlatformError, and RuntimeServices. Wires the Fastly adapter implementations and threads RuntimeServices into route_request. Moves GeoInfo to platform/types as platform-neutral data and adds geo_from_fastly for field mapping.
…o-pr2-platform-traits
- Defer KV store opening: replace early error return with a local UnavailableKvStore fallback so routes that do not need synthetic ID access succeed when the KV store is missing or temporarily unavailable - Use ConfigStore::try_open + try_get and SecretStore::try_get throughout FastlyPlatformConfigStore and FastlyPlatformSecretStore to honour the Result contract instead of panicking on open/lookup failure - Encapsulate RuntimeServices service fields as pub(crate) with public getter methods (config_store, secret_store, backend, http_client, geo) and a pub new() constructor; adapter updated to use new() - Reference #487 in FastlyPlatformHttpClient stub (PR 6 implements it) - Remove unused KvPage re-export from platform/mod.rs - Use super::KvHandle shorthand in RuntimeServices::kv_handle()
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Solid architectural direction — this PR lays the groundwork for decoupling trusted-server-core from the Fastly SDK. The trait design, object-safety assertions, and graceful KV degradation are well done. A few issues to address before merge, mostly around constructor ergonomics and a question about Send bounds.
Blocking
🔧 wrench
RuntimeServices::newtakes 7 args with lint suppression: At the boundary today, will exceed it whencreative_store/counter_storeare added per the TODO. Use a builder pattern instead. (crates/trusted-server-core/src/platform/types.rs:103)- PR description overstates decoupling:
trusted-server-corestill depends onfastlydirectly (geo.rs,backend.rs,fastly_storage.rs,settings.rs). The description says this "enables future non-Fastly adapter support" — should clarify this is step N of M and thefastlydep removal comes in a follow-up.
❓ question
PlatformPendingRequestis!Send— intentional?:Box<dyn Any>is!Send, whilePlatformHttpClient: Send + Sync. Will this asymmetry hold for future adapters usingtokio::spawn? (crates/trusted-server-core/src/platform/http.rs:64)
Non-blocking
🤔 thinking
PlatformBackendSpecduplicatesBackendConfigfields: Both types havescheme,host,port,certificate_check,first_byte_timeout— one owns, the other borrows. Everyensurecall allocates owned Strings immediately borrowed. Consider sharing the type.- Asymmetric key semantics on
PlatformConfigStore:gettakesstore_name,put/deletetakestore_id. Newtype wrappers would make misuse a compile error. (crates/trusted-server-core/src/platform/traits.rs:19)
♻️ refactor
geo_from_fastlyshould live in adapter, not core: Importsfastly::geo::Geoin core, tying it to Fastly. Beingpubmeans new code could accidentally depend on it. (crates/trusted-server-core/src/geo.rs:10)UnavailableKvStoreshould live in core, not adapter: Platform-neutral fallback that any future adapter would duplicate. (crates/trusted-server-adapter-fastly/src/platform.rs:276)
⛏ nitpick
- Prefer
attach_withoverattach(format!(...)): Avoids allocation unless the report is inspected. Appears throughoutplatform.rs.
🌱 seedling
- No
Debugimpl forRuntimeServices: A manualDebugimpl printingclient_infowhile omitting service handles would help debugging.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
| /// Adapter crates should call this constructor rather than using struct | ||
| /// literal syntax so that any future invariants on the struct are enforced | ||
| /// in one place. | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
🔧 wrench — RuntimeServices::new takes 7 arguments and suppresses the lint
CLAUDE.md says "Functions should never take more than 7 arguments — use a struct instead." This constructor is at the boundary and uses #[allow(clippy::too_many_arguments)]. More importantly, the TODO at line 82-85 mentions adding creative_store, counter_store — which will push past 7.
Fix: Use a builder pattern:
RuntimeServices::builder()
.config_store(config_store)
.secret_store(secret_store)
.kv_store(kv_store)
.backend(backend)
.http_client(http_client)
.geo(geo)
.client_info(client_info)
.build()This also resolves the "struct literal vs constructor" concern in the doc comment at line 98-101.
| /// | ||
| /// The core stores this as an opaque support type. Adapter implementations can | ||
| /// recover their concrete runtime handle through [`Self::downcast`]. | ||
| pub struct PlatformPendingRequest { |
There was a problem hiding this comment.
❓ question — PlatformPendingRequest uses Box<dyn Any> which is !Send
PlatformPendingRequest stores Box<dyn Any> (!Send), while RuntimeServices stores Arc<dyn PlatformHttpClient> requiring Send + Sync. The trait contract says PlatformHttpClient: Send + Sync but async methods' futures are !Send (via #[async_trait(?Send)]).
Is this intentional asymmetry? Will it hold for future adapters (e.g., an Axum adapter using tokio::spawn)?
| /// | ||
| /// Returns [`PlatformError::ConfigStore`] when the key does not exist or | ||
| /// the store cannot be opened. | ||
| fn get(&self, store_name: &str, key: &str) -> Result<String, Report<PlatformError>>; |
There was a problem hiding this comment.
🤔 thinking — get vs put/delete have asymmetric key semantics
get takes store_name (edge-visible name) while put/delete take store_id (management API identifier). This is documented but is a footgun — someone will pass a store_name to put or vice versa.
Consider newtype wrappers (StoreName(String) / StoreId(String)) to make misuse a compile error.
| //! blocks for construction and response header injection. | ||
|
|
||
| use fastly::geo::geo_lookup; | ||
| use fastly::geo::{geo_lookup, Geo}; |
There was a problem hiding this comment.
♻️ refactor — geo_from_fastly should live in the adapter crate, not core
This function imports fastly::geo::Geo in core, tying the core to Fastly. Being pub means new code could accidentally depend on it. Should move to trusted-server-adapter-fastly in the follow-up PR.
| /// Every method returns [`KvError::Unavailable`], ensuring that handlers | ||
| /// which call [`RuntimeServices::kv_handle`] receive a typed error rather than | ||
| /// a panic. Routes that do not touch the KV store are unaffected. | ||
| pub(crate) struct UnavailableKvStore; |
There was a problem hiding this comment.
♻️ refactor — UnavailableKvStore should live in core, not the adapter
This is a platform-neutral fallback (all methods return KvError::Unavailable). Any future adapter would need the same stub. Move to trusted-server-core::platform to avoid duplication across adapters.
| impl PlatformConfigStore for FastlyPlatformConfigStore { | ||
| fn get(&self, store_name: &str, key: &str) -> Result<String, Report<PlatformError>> { | ||
| let store = ConfigStore::try_open(store_name).map_err(|e| { | ||
| Report::new(PlatformError::ConfigStore) |
There was a problem hiding this comment.
⛏ nitpick — Prefer attach_with over attach(format!(...))
attach_with(|| format!(...)) avoids the allocation unless the report is actually inspected. This pattern appears throughout this file.
// Current:
.attach(format!("failed to open config store '{store_name}': {e}"))
// Preferred:
.attach_with(|| format!("failed to open config store '{store_name}': {e}"))2 participants
Summary
trusted-server-core::platformmodule with six platform-neutral traits (PlatformConfigStore,PlatformSecretStore,PlatformKvStore,PlatformBackend,PlatformHttpClient,PlatformGeo) enabling future non-Fastly adapter supportClientInfo(extract-once plain data struct),PlatformErrorenum, andRuntimeServicesaggregate that threads all platform services intoroute_requestGeoInfotoplatform/typesas platform-neutral data and adds ageo_from_fastlymapping function; wires full Fastly implementations in the adapter crateChanges
crates/trusted-server-core/src/platform/mod.rsRuntimeServices, re-exports, object-safety assertions, unit testscrates/trusted-server-core/src/platform/traits.rsasync_trait+ WASM-conditional?Sendcrates/trusted-server-core/src/platform/types.rsClientInfo,GeoInfo(moved here fromgeo.rs); service fieldspub(crate)with public getters andnew()constructorcrates/trusted-server-core/src/platform/error.rsPlatformErrorenumcrates/trusted-server-core/src/platform/http.rsPlatformHttpClientrequest/response typescrates/trusted-server-core/src/geo.rsplatform/types, addsgeo_from_fastlycrates/trusted-server-core/src/backend.rsPlatformBackendtraitcrates/trusted-server-core/src/fastly_storage.rsPlatformKvStore/PlatformConfigStoreimpl wiringcrates/trusted-server-adapter-fastly/src/platform.rstry_open/try_getthroughoutcrates/trusted-server-adapter-fastly/src/main.rsRuntimeServices, threads intoroute_request; KV store failure degrades gracefullycrates/trusted-server-adapter-fastly/Cargo.tomlasync-trait,futuresdependenciescrates/trusted-server-core/Cargo.tomlasync-traitdependencyCloses
Closes #483
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)