Repository navigation
Separate shared async EC on the compatible demo baseline - #1534
ChristianPavilonis wants to merge 18 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Converts EcKvStore and its core callers to local (?Send) async futures, keeping Fastly's blocking KV hostcalls behind a futures::executor::block_on bridge in the synchronous response hook and post-send pull sync. The conversion is mechanical: CAS/retry loops, withdrawal-marker handling, tombstone and finalize ordering are unchanged, and no lock or borrow is held across an await.
Checked and not a problem:
- Nested executor: in the pinned edgezero revision the finalize hook runs only after
block_on(app.dispatch_admitted(..))has returned, and post-send pull sync after delivery, so the newblock_oncalls are not nested. The Fastly EC lifecycle integration job exercises this path under Viceroy. - Other adapters:
FastlyEcKvStoreis the only production implementation and all newly async callers are in core or the Fastly adapter, so Axum/Cloudflare/Spin gain no blocking bridge. - Pull sync concurrency: all partner requests are still dispatched before any are awaited.
- Removed
apply_withdrawal_tombstonestest: coverage is retained end-to-end byfinalize_withdrawal_tombstones_both_present_ids_once.
1 of the inline comments below carries a one-click GitHub
suggestion.
Non-blocking
⛏ nitpick
- Assertions without messages — see inline at
crates/trusted-server-core/src/ec/pull_sync.rs:977
CI Status
All 17 checks PASS at a8e61218: CLAUDE.md symlink guard, browser integration tests, cargo check (cloudflare native + wasm32-unknown-unknown), cargo check/build/test (spin native + wasm32-wasip2), cargo fmt (includes all adapter clippy lanes), cargo test, cargo test (axum native), cargo test (cross-adapter parity), cargo test (ts CLI, native) (macos-latest), cargo test (ts CLI, native) (ubuntu-latest), docs-proposal scripts, format-docs, format-typescript, integration tests, integration tests (Fastly EC lifecycle), prepare integration artifacts, vitest.
| assert_eq!(stub.recorded_backend_names().len(), 1); | ||
| let (entry, _) = graph | ||
| .get(&ec_id) | ||
| .await | ||
| .expect("should read row") | ||
| .expect("row should exist"); | ||
| assert_eq!( | ||
| entry.ids.get("alpha.example.com").map(|id| id.uid.as_str()), | ||
| Some("synced-uid"), | ||
| ); |
There was a problem hiding this comment.
⛏ nitpick: assertions without messages
Both assert_eq! calls in this new test lack a descriptive message, which AGENTS.md asks for in test code.
| assert_eq!(stub.recorded_backend_names().len(), 1); | |
| let (entry, _) = graph | |
| .get(&ec_id) | |
| .await | |
| .expect("should read row") | |
| .expect("row should exist"); | |
| assert_eq!( | |
| entry.ids.get("alpha.example.com").map(|id| id.uid.as_str()), | |
| Some("synced-uid"), | |
| ); | |
| assert_eq!( | |
| stub.recorded_backend_names().len(), | |
| 1, | |
| "should dispatch one pull request to the single eligible partner" | |
| ); | |
| let (entry, _) = graph | |
| .get(&ec_id) | |
| .await | |
| .expect("should read row") | |
| .expect("row should exist"); | |
| assert_eq!( | |
| entry.ids.get("alpha.example.com").map(|id| id.uid.as_str()), | |
| Some("synced-uid"), | |
| "should persist the pulled partner UID" | |
| ); |
Retain the existing shared EC head, async local non-Send store futures, snapshot/withdrawal/CAS behavior and the cutover's bounded direct pull-sync groups. Preserve current body-read failures and response ownership without restoring pending/select or legacy HTTP interfaces. Await EC work through handlers and use the existing Fastly synchronous hook as the production executor boundary. Migrate the current post-delivery fixture to the async graph and verify real Fastly runtime paths with the explicitly repaired local Viceroy.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Converts EcKvStore and its core and Fastly callers to local (?Send) async futures, and bridges the synchronous EdgeZero response hook and post-send pull sync with futures::executor::block_on. The core changes match #1526 at e529e5576 byte for byte. With async/.await stripped and both sides rustfmt-normalized, production kv.rs, identify.rs, admin.rs and the auction, publisher and registry callers are identical to the base. The remaining production changes are the inlined withdrawal loop, the two removed pull-sync block_on calls, the two bridges and the app.rs async blocks. In EdgeZero 83d7a629 the finalize hook runs after the dispatch block_on returns and pull sync runs after delivery, so no bridge nests, and the v0.0.12-rc-axum.202611.1 repin resolves to that same commit. The blocking items are a test gap (no unit test runs finalization or EC generation under the executor Fastly uses, and four nested-block_on mutations survive), integration jobs that did not run at this head, and a PR description that does not describe this diff.
7 of the 9 inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion, or Add suggestion to batch for several at once, to apply them as commits on the PR branch. The other 2 describe the fix in prose because it touches lines outside the diff hunks.Each suggestion was applied alone to this head and passed rustfmt, and each new test passes on its own and fails under its target mutation. All of them applied together pass the six adapter clippy aliases,
cargo test-fastly,test-fastly-reuse,test-cloudflare,test-spinand the parity suite.test-axumalso passes exceptmanaged_ingress_ignores_direct_spoof_and_isolates_two_gateway_clients, which fails the same way at this head on macOS because it binds127.0.0.2; it passes in CI on Linux, and no suggestion touches Axum.
Blocking
🔧 wrench
- No test drives finalization under the executor Fastly uses: see inline at
crates/trusted-server-core/src/ec/finalize.rs:681 - EC generation has the same gap, on the router path: see inline at
crates/trusted-server-core/src/ec/mod.rs:844
Non-blocking
♻️ refactor
- State the cancellation contract on the trait, and drop "native backends": see inline at
crates/trusted-server-core/src/ec/kv_backend.rs:75
🏕 camp site
- The active-ID guard on the snapshot correction is untested: see inline at
crates/trusted-server-core/src/ec/finalize.rs:492 dispatch_pull_syncstill documents itself as background work: see inline atcrates/trusted-server-core/src/ec/pull_sync.rs:116generate_if_neededunder-documents its errors: see inline atcrates/trusted-server-core/src/ec/mod.rs:382handle_batch_syncdocuments failures it does not return: see inline atcrates/trusted-server-core/src/ec/batch_sync.rs:118- Record the EdgeZero ordering this bridge relies on: see inline at
crates/trusted-server-adapter-fastly/src/main.rs:522
⛏ nitpick
- The Fastly module doc prescribes behavior for other backends: see inline at
crates/trusted-server-adapter-fastly/src/ec_kv.rs:7
Cross-cutting / body-level findings
-
🔧 The integration jobs did not run at this head.
prepare integration artifactsfailed at its upload step: GitHub's artifact service returned an HTML page instead of JSON on all five attempts (Failed to CreateArtifact: ... Unexpected token '<'), sointegration tests,integration tests (Fastly EC lifecycle)andbrowser integration testswere skipped. This is an infrastructure failure, not a code one, but the Fastly EC lifecycle job is the only check that drives finalization through the realblock_onbridge (itsConsentWithdrawalscenario). Please re-run the workflow atb3ff6f795. No checks are configured as required on this base, so nothing blocks the merge mechanically. -
🔧 The PR description does not describe this diff.
- It names head
a8e61218; the head isb3ff6f795, and the EdgeZero pin is now the tagv0.0.12-rc-axum.202611.1(the same commit,83d7a629). - Numbers are glued to the preceding word throughout ("EdgeZero83d7a629", "TS2423a2ed", "PR1525/1526 ready and1527/1528", "Core3006", "only9356 ->ee7").
- "Hosted docs-build correction", "Startup negative-probe correction" and "Browser CI host compatibility" describe base-branch commits; they touch none of the 14 files in this diff.
- "The user selected compatible-baseline cleanup", "No new worker launched or paused experiment resumed" and "All local parent runners are complete" are working-session context a reviewer cannot see or act on.
- The "Changes" table repeats the summary instead of listing the changes.
Suggested replacement; keep only the checks you ran at the current head:
## Summary - Make `EcKvStore` a local (`?Send`) async trait and await it through `KvIdentityGraph` and every core and Fastly caller. - Bridge Fastly's two synchronous entry points with `futures::executor::block_on`: EC finalization in the EdgeZero response hook, and post-send pull sync. Pull sync no longer calls `block_on` itself. - No Fastly behavior change: `FastlyEcKvStore` still completes each operation with a blocking KV hostcall, and CAS, tombstone, withdrawal and finalize ordering are unchanged. Unit of the #1530 baseline stack, on top of #1533. The core change is the same as #1526. ## Changes | Area | Change | | --- | --- | | `ec/kv_backend.rs` | `EcKvStore` I/O methods are `async` under `#[async_trait(?Send)]`; the test stores follow. | | `ec/kv.rs`, `ec/mod.rs`, `ec/identify.rs`, `ec/admin.rs`, `ec/batch_sync.rs` | Graph operations and their callers are awaited. | | `ec/finalize.rs` | Finalization is async; the withdrawal closure helper becomes an inline loop and its unit test is removed. | | `ec/pull_sync.rs` | `dispatch_pull_sync` awaits the platform HTTP client instead of calling `block_on`. | | `auction/endpoints.rs`, `publisher.rs`, `integrations/registry.rs` | Snapshot loads and EC generation are awaited. | | `adapter-fastly/src/ec_kv.rs` | `FastlyEcKvStore` implements the async trait over the blocking SDK. | | `adapter-fastly/src/main.rs`, `app.rs` | `block_on` bridges for finalization and post-send pull sync; route handlers await. | ## Test plan - [ ] `cargo test-fastly`, `cargo test-fastly-reuse`, `cargo test-axum`, `cargo test-cloudflare`, `cargo test-spin` and the parity suite - [ ] The six adapter clippy aliases - [ ] New regressions: `graph_lookup_yields_to_other_work_with_a_non_send_store`, `dispatch_pull_sync_runs_inside_the_fastly_executor_boundary` - [ ] Hosted checks at the current head, including `integration tests (Fastly EC lifecycle)` Refs #1530.
- It names head
-
📌 The real post-send pull sync never runs end to end. The integration fixture (
crates/trusted-server-integration-tests/fixtures/configs/trusted-server.integration.toml) declares two[[ec.partners]], and neither setspull_sync_enabledorpull_sync_url. The Fastly EC lifecycle job therefore exercises the finalize bridge but never the post-send pull-sync bridge with the realFastlyPlatformHttpClient, and the new regression test usesStubHttpClient. Please add a pull-enabled partner backed by a fixture endpoint, or open a follow-up issue and link it here. -
🌱 A native backend for this trait needs EdgeZero KV features that do not exist yet. The trait doc anticipates a backend that yields, but EdgeZero's
KvStoreat83d7a629(imported in core asPlatformKvStore) offers onlyget_bytes,exists,put_bytes,put_bytes_with_ttl,deleteandlist_keys_page. It has no generation token, no conditional (add or if-generation-match) write, no metadata and no strong-consistency listing, andEcKvStoreneeds all of these. A shared native backend therefore depends on EdgeZero asks rather than Trusted Server plumbing.
CI Status
- prepare integration artifacts: FAIL
- integration tests: SKIPPED
- integration tests (Fastly EC lifecycle): SKIPPED
- browser integration tests: SKIPPED
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip2): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- docs-proposal scripts: PASS
- format-docs: PASS
- format-typescript: PASS
- vitest: PASS
- ts dev lint domains (changed lines): not run on this base; the added lines name only
example.comhosts
| @@ -687,23 +679,6 @@ mod tests { | |||
| assert!(ids.contains(&valid_ec), "should keep the valid EC ID"); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
🔧 wrench: No test drives finalization under the executor Fastly uses
This PR moves apply_edgezero_ec_finalize into futures::executor::block_on inside the EdgeZero response hook (crates/trusted-server-adapter-fastly/src/main.rs:523). Every ec_finalize_response test runs under #[tokio::test(flavor = "current_thread")], and tokio tolerates a nested futures block_on. A block_on reintroduced anywhere under finalization would pass the unit suites and panic in production with EnterError.
Reproduced at this head. Each of these mutations passes all 3006 core lib tests:
- line 480:
futures::executor::block_on(graph.tombstone_existing_from_snapshot(ec_id, initial))(withdrawal) - line 314:
futures::executor::block_on(graph.create_if_absent(&ec_id, &entry))(orphan recovery) - line 377:
futures::executor::block_on(graph.key_exists_confirmed(ec_id))(orphan confirmation)
The ConsentWithdrawal scenario in integration tests (Fastly EC lifecycle) would catch the first one through the real binary, but that job did not run at this head, and no lifecycle scenario reaches orphan recovery. dispatch_pull_sync_runs_inside_the_fastly_executor_boundary already guards pull sync this way. The suggestion adds the same guard for finalization where the removed apply_withdrawal_tombstones_invokes_writer_for_each_ec_id sat. It passes at this head, and under each mutation above it is the only failing test (cannot execute LocalPool executor from within another executor: EnterError).
| #[test] | |
| fn finalize_runs_inside_the_fastly_executor_boundary() { | |
| // Fastly's entry point drives finalization with | |
| // `futures::executor::block_on`, which panics if an awaited step | |
| // nests a second blocking executor. | |
| futures::executor::block_on(async { | |
| let settings = create_test_settings(); | |
| let withdrawn = sample_ec_id("fxwd01"); | |
| let consent = ConsentContext { | |
| jurisdiction: Jurisdiction::UsState("CA".to_owned()), | |
| gpc: true, | |
| source: ConsentSource::Cookie, | |
| ..Default::default() | |
| }; | |
| let mut ec_context = | |
| make_context_with_consent(Some(&withdrawn), Some(&withdrawn), true, false, consent); | |
| let graph = KvIdentityGraph::in_memory("test_store"); | |
| graph | |
| .create(&withdrawn, &live_entry()) | |
| .await | |
| .expect("should seed the withdrawn identity"); | |
| ec_context.set_kv_snapshot(graph.load_snapshot(&withdrawn).await); | |
| let mut response = empty_response(); | |
| ec_finalize_response( | |
| &settings, | |
| &mut ec_context, | |
| Some(&graph), | |
| &PartnerRegistry::empty(), | |
| None, | |
| None, | |
| &mut response, | |
| ) | |
| .await; | |
| let (entry, _) = graph | |
| .get(&withdrawn) | |
| .await | |
| .expect("should read the withdrawn row") | |
| .expect("should keep the withdrawn row"); | |
| assert!( | |
| !entry.consent.ok, | |
| "withdrawal should tombstone the identity under the Fastly executor" | |
| ); | |
| let orphan = sample_ec_id("fxor01"); | |
| let mut ec_context = returning_user_context( | |
| &orphan, | |
| EcKvSnapshot::Missing { | |
| ec_id: orphan.clone(), | |
| }, | |
| true, | |
| ); | |
| let mut response = empty_response(); | |
| ec_finalize_response( | |
| &settings, | |
| &mut ec_context, | |
| Some(&graph), | |
| &PartnerRegistry::empty(), | |
| None, | |
| None, | |
| &mut response, | |
| ) | |
| .await; | |
| assert_ne!( | |
| ec_context.ec_value(), | |
| Some(orphan.as_str()), | |
| "orphan recovery should rotate the identity under the Fastly executor" | |
| ); | |
| }); | |
| } | |
|
|
||
| #[test] | ||
| fn generate_if_needed_retries_id_collision_then_persists() { | ||
| #[tokio::test(flavor = "current_thread")] |
There was a problem hiding this comment.
🔧 wrench: EC generation has the same gap, on the router path
EdgeZero drives the router with futures::executor::block_on(app.dispatch_admitted(..)) (edgezero-adapter-fastly request.rs:420 at 83d7a629), so generate_if_needed runs inside that executor for every new visitor. With futures::executor::block_on(graph.create_if_absent(&ec_id, &entry)) at line 423, all 3006 core lib tests and all 222 Fastly adapter tests still pass. The suggested test drives one collision and the retry under the same executor. It passes at this head and is the only test that fails under that mutation.
| #[tokio::test(flavor = "current_thread")] | |
| #[test] | |
| fn generate_if_needed_runs_inside_the_fastly_executor_boundary() { | |
| // Fastly's router runs under `futures::executor::block_on`, which | |
| // panics if an awaited step nests a second blocking executor. | |
| futures::executor::block_on(async { | |
| let settings = create_test_settings(); | |
| let mut ec = EcContext::new_for_test_with_ip( | |
| None, | |
| granting_consent(), | |
| Some("192.0.2.7".to_owned()), | |
| ); | |
| let graph = KvIdentityGraph::new(AddCollidingEcKv::new(1)); | |
| ec.generate_if_needed(&settings, Some(&graph)) | |
| .await | |
| .expect("should generate inside the Fastly executor"); | |
| assert!( | |
| matches!(ec.kv_snapshot(), EcKvSnapshot::Present { .. }), | |
| "should persist the generated EC ID under the Fastly executor" | |
| ); | |
| }); | |
| } | |
| #[tokio::test(flavor = "current_thread")] |
| /// I/O returns local futures so native backends can yield without blocking the | ||
| /// request runtime. Neither stores nor their futures need to be `Send` or `Sync`. |
There was a problem hiding this comment.
♻️ refactor: State the cancellation contract on the trait, and drop "native backends"
No native backend exists in this tree, so the doc should describe the capability rather than a backend type. More importantly, graph operations now have await points between dependent writes: generate_if_needed commits create_if_absent before it sets ec_value (crates/trusted-server-core/src/ec/mod.rs:423 and :451), and the withdrawal loop can tombstone one ID and not the other (crates/trusted-server-core/src/ec/finalize.rs:469). Nothing drops these futures today, since EdgeZero runs the handler and both Fastly bridges run to completion, so this documents the contract for the next backend author and caller.
| /// I/O returns local futures so native backends can yield without blocking the | |
| /// request runtime. Neither stores nor their futures need to be `Send` or `Sync`. | |
| /// I/O returns local futures so a backend with real async I/O can yield without | |
| /// blocking the request runtime. Neither stores nor their futures need to be | |
| /// `Send` or `Sync`. Dropping a returned future does not roll back a write that | |
| /// may already have committed; callers must not infer either outcome without | |
| /// the returned result. |
| @@ -491,7 +492,7 @@ fn finalize_unusable_consent( | |||
| if ec_context.ec_value() == Some(ec_id) { | |||
There was a problem hiding this comment.
🏕 camp site: The active-ID guard on the snapshot correction is untested
if ec_context.ec_value() == Some(ec_id) keeps the context snapshot on the active identity when the withdrawal cookie names a different one. Replacing the guard with if true passes all 3006 core lib tests at this head: no test withdraws two present IDs and then checks which one the context snapshot belongs to. The closure-to-loop change here is a good moment to pin it. Withdrawal IDs come from a HashSet, so the test repeats with fresh sets to cover both visit orders:
#[tokio::test(flavor = "current_thread")]
async fn finalize_withdrawal_keeps_the_active_snapshot_when_the_cookie_id_differs() {
// Withdrawal IDs come from a HashSet, so visit order varies between
// runs. Repeat with fresh sets so a snapshot overwritten by the
// non-active cookie ID's outcome cannot pass by luck.
for round in 0..16 {
let settings = create_test_settings();
let active_ec = sample_ec_id("activ9");
let cookie_ec = sample_ec_id("cook9e");
let consent = ConsentContext {
jurisdiction: Jurisdiction::UsState("CA".to_owned()),
gpc: true,
source: ConsentSource::Cookie,
..Default::default()
};
let mut ec_context =
make_context_with_consent(Some(&active_ec), Some(&cookie_ec), true, false, consent);
let graph = KvIdentityGraph::in_memory("test_store");
graph
.create(&active_ec, &live_entry())
.await
.expect("should seed active row");
graph
.create(&cookie_ec, &live_entry())
.await
.expect("should seed cookie row");
ec_context.set_kv_snapshot(graph.load_snapshot(&active_ec).await);
let mut response = empty_response();
ec_finalize_response(
&settings,
&mut ec_context,
Some(&graph),
&PartnerRegistry::empty(),
None,
None,
&mut response,
)
.await;
assert!(
ec_context.kv_snapshot().belongs_to(&active_ec),
"round {round}: the context snapshot should stay on the active ID"
);
assert!(
ec_context
.kv_snapshot()
.entry_for(&active_ec)
.is_some_and(|entry| !entry.consent.ok),
"round {round}: the context snapshot should hold the active ID's tombstone"
);
}
}It passes at this head and fails with the guard removed. Apply manually: it adds a test outside the diff hunks, for example just before finalize_withdrawal_tombstones_both_present_ids_once.
| /// Panics if the HTTP request builder produces an invalid request, which | ||
| /// cannot happen with the hardcoded method and well-formed URI used here. | ||
| pub fn dispatch_pull_sync( | ||
| pub async fn dispatch_pull_sync( |
There was a problem hiding this comment.
🏕 camp site: dispatch_pull_sync still documents itself as background work
The function doc (line 96, "Dispatches partner pull-sync requests in the background.") and the module doc (line 1, "Pull sync background dispatch.") predate this PR, but on an async fn they read as spawned or fire-and-forget work. Nothing is spawned: the returned future does the live-row read, the sends, the waits and the CAS merge, and does none of it unless the caller drives it, which crates/trusted-server-adapter-fastly/src/main.rs:551 does after delivery.
Proposed fix (apply manually; lines 1 and 96 are outside the diff hunks, so this cannot be a suggestion):
//! Pull sync dispatch, run after the client response is sent.
/// Dispatches partner pull-sync requests and merges the returned UIDs.
///
/// Run this after the client response is sent. Nothing is spawned: the caller
/// must drive the returned future to completion.
In crates/trusted-server-core/src/ec/mod.rs:30, change Background pull-sync dispatcher to Post-send pull-sync dispatcher.
| /// Returns an error if the client IP is unavailable and generation is | ||
| /// needed, or if HMAC generation fails. |
There was a problem hiding this comment.
🏕 camp site: generate_if_needed under-documents its errors
The # Errors section predates this PR, but the signature changes here. It omits two Err paths: identity-graph persistence failure (line 438) and collision exhaustion after MAX_CREATE_ATTEMPTS (line 457).
| /// Returns an error if the client IP is unavailable and generation is | |
| /// needed, or if HMAC generation fails. | |
| /// Returns an error if the client IP is unavailable and generation is | |
| /// needed, if HMAC generation fails, if persisting the new ID to the | |
| /// identity graph fails, or if five generated IDs in a row collide. |
| @@ -114,16 +116,16 @@ struct MappingGroup { | |||
| /// # Errors | |||
| /// | |||
| /// Returns [`TrustedServerError`] on serialization or KV store failures. | |||
There was a problem hiding this comment.
🏕 camp site: handle_batch_sync documents failures it does not return
KV write failures never return Err from this handler: process_mappings turns them into per-mapping kv_unavailable rejections in a 207 response. The real error paths are the rate limiter (the ? at line 141), a body the adapter did not buffer (line 156), invalid JSON (line 168) and response serialization (line 194). This predates the PR, but the signature changes here.
| /// Returns [`TrustedServerError`] on serialization or KV store failures. | |
| /// Returns [`TrustedServerError`] when the rate limiter fails, the adapter did | |
| /// not buffer the body or it is not valid JSON, or the response cannot be | |
| /// serialized. Identity-graph write failures do not return an error: they | |
| /// reject the affected mappings as `kv_unavailable` in a `207 Multi-Status` | |
| /// response. |
| // The Fastly response hook is synchronous; its KV operations | ||
| // use hostcall waits. Keep this bridge out of native executors. |
There was a problem hiding this comment.
🏕 camp site: Record the EdgeZero ordering this bridge relies on
This block_on is safe only because EdgeZero calls the finalize hook after its own block_on(app.dispatch_admitted(..)) has returned (dispatch_core_request and deliver_with_hooks in edgezero-adapter-fastly request.rs at 83d7a629). EdgeZero does not document that ordering on send_request_with_registries_and_hooks, so an EdgeZero change that ran the hook inside dispatch would turn this into an EnterError panic on every request with EC state. Saying so here gives the next EdgeZero bump something to check.
| // The Fastly response hook is synchronous; its KV operations | |
| // use hostcall waits. Keep this bridge out of native executors. | |
| // The Fastly response hook is synchronous; its KV operations | |
| // use hostcall waits. EdgeZero calls this hook only after its | |
| // dispatch `block_on` returns, so this bridge does not nest. | |
| // Keep it out of native executors. |
| //! writes (`if_generation_match`). The SDK exposes only blocking completion | ||
| //! hostcalls for KV operations, so these local async methods retain that Fastly | ||
| //! behavior. Native backends must yield during network I/O instead. |
There was a problem hiding this comment.
⛏ nitpick: The Fastly module doc prescribes behavior for other backends
"Native backends must yield during network I/O instead" is a rule for code that does not exist, placed in the Fastly module. Describing what this backend does is enough; the trait doc is the place for the general contract.
| //! writes (`if_generation_match`). The SDK exposes only blocking completion | |
| //! hostcalls for KV operations, so these local async methods retain that Fastly | |
| //! behavior. Native backends must yield during network I/O instead. | |
| //! writes (`if_generation_match`). The SDK exposes only blocking completion | |
| //! hostcalls for KV operations, so these async methods finish their I/O before | |
| //! returning and never yield. |
There was a problem hiding this comment.
Superseded by a follow-up review with inline comments: #1534 (review)
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
This PR moves EcKvStore and the identity-graph callers to local (?Send) async futures. Most of the change is mechanical .await threading. The Fastly adapter bridges the async finalize path with one block_on, which runs after EdgeZero dispatch has returned. I checked the conversion for held locks across awaits and for nested executors and found neither. I have two small non-blocking items.
1 of the inline comments below carries a one-click GitHub
suggestion. Use Commit suggestion to apply it. The other comment describes the fix in prose because the new test goes outside this PR's hunks.
Non-blocking
⛏ nitpick
- Explain why the finalize
block_onis not nested. See inline atcrates/trusted-server-adapter-fastly/src/main.rs:459.
🏕 camp site
- Two-ID withdrawal coverage dropped with
apply_withdrawal_tombstones. See inline atcrates/trusted-server-core/src/ec/finalize.rs(removed lines 690-705).
CI Status
- 25 checks reported by
gh pr checks: all PASS. No failed, cancelled or pending checks. Branch protection lists no required checks for this base branch.
|
|
||
| let state = ec_state.and_then(|mut ec_state| { | ||
| match apply_edgezero_ec_finalize(inputs.settings, &mut ec_state, response) { | ||
| match futures::executor::block_on(apply_edgezero_ec_finalize( |
There was a problem hiding this comment.
⛏ nitpick — Say why this block_on is safe here. A reader seeing futures::executor::block_on inside the Fastly entry point will reasonably worry about a nested executor. It is not one: send_request_with_registries_and_hooks runs dispatch to completion and only then calls the finalize hook from finish_request, and the Fastly KV backend's async methods wrap blocking hostcalls, so they complete on first poll. A short comment saves the next reviewer that trace.
| match futures::executor::block_on(apply_edgezero_ec_finalize( | |
| // Not a nested executor: EdgeZero calls this finalize hook after its | |
| // dispatch executor has returned, and the Fastly KV backend's futures | |
| // complete on first poll because the SDK only exposes blocking hostcalls. | |
| match futures::executor::block_on(apply_edgezero_ec_finalize( |
| #[test] | ||
| fn apply_withdrawal_tombstones_invokes_writer_for_each_ec_id() { | ||
| let first = sample_ec_id("first1"); | ||
| let second = sample_ec_id("second"); | ||
| let mut ids = HashSet::new(); | ||
| ids.insert(first.clone()); | ||
| ids.insert(second.clone()); | ||
|
|
||
| let mut written = Vec::new(); | ||
| apply_withdrawal_tombstones(&ids, |ec_id| written.push(ec_id.to_owned())); | ||
| written.sort(); | ||
|
|
||
| let mut expected = vec![first, second]; | ||
| expected.sort(); | ||
| assert_eq!(written, expected, "should write a tombstone for each EC ID"); | ||
| } |
There was a problem hiding this comment.
🏕 camp site — This test went away with apply_withdrawal_tombstones, and nothing replaces its two-ID case. The remaining withdrawal tests (finalize_withdrawal_tombstones_a_held_identity and the others) all use one EC ID, where the cookie ID and the active ID are the same. The now-inlined for ec_id in &ids_to_withdraw loop in finalize_unusable_consent still has a separate branch for the non-active cookie ID: it gets EcKvSnapshot::NotRead as its initial snapshot and its outcome is not stored on the context. No test covers that branch now.
Suggested replacement: an async test that seeds two live rows (cookie ID and a different active ID), runs ec_finalize_response with consent withdrawn, and asserts both rows read back with consent.ok == false, and that only the active ID's snapshot is updated on the context.
#[tokio::test(flavor = "current_thread")]
async fn finalize_withdrawal_tombstones_both_cookie_and_active_identities() {
// seed rows for sample_ec_id("cookie") and sample_ec_id("active"),
// build the context with both IDs and withdrawn consent,
// call ec_finalize_response(...).await,
// then assert kv.get(..) shows !consent.ok for both IDs.
}Apply manually. This can't be a one-click suggestion because the new test goes outside the hunks this PR changed.
Per the user, every feature is built once and works on every adapter in platform order, with platform gaps sent to EdgeZero (DEC-16, SH-5). T21 moves the identity routes, EC lifecycle, providers and write-back into one router in core that each adapter mounts; A2, A10, T8, T9, T17 and T18 shrink to turning it on per adapter and passing the conformance suite, and T7, T14 and A11 fold into A2 and A10. EZI-2a adds EdgeZero's local store for Axum development and EZI-4 binds Axum's geo provider from the manifest. Restack the Axum RC units on #1506: #1531 carries A1 in place of #1528, #1533 and #1534 close as copies of #1525 and #1526, and the image and Terraform units sit directly on #1532 so A6 and A7 stop waiting on A2. Record #1566, refresh the provider stack's fixes at their current heads, and flag #1535 as unplanned.
Summary
Draft baseline review unit. Not a current-main merge candidate, deployment approval or completed current-head CI claim.
Baseline and sequence
Head:
a8e61218fea4ccccfa77b8d0b932b38d1090dcbe. Base:baseline/http-auction-20261110.Historical main anchor:
2dd968aeebeb2cc0d5110115f64e0ad58e2e1d13. Original demo: TS2423a2ed plus separately preserved dirty inputs. Matching EdgeZero: draft431, exact revision83d7a6296d285e135c4ea70021b19e6c8dca7e9f.The user selected compatible-baseline cleanup. Current-main integration with EdgeZero462 and the authenticated-forwarder incompatibility are deferred. Preserve current-main PR1525/1526 ready and1527/1528 draft, with their existing bases. No excluded1499 or independent1506 implementation, relay/privacy experiment, new transport fallback, group/codec/phase design or operational deployment is included.
Changes
Closes
Refs #1530. This unit completes one part of the baseline extraction and does not close the overall tracker on its own.
Spec
Not a new feature. This is extraction of existing demo behavior and bounded build/caller corrections. Original native and EC storage design/review records remain in the baseline where applicable; they are not presented as approval of later combined source. No new forwarding, consent, timer or provider policy is designed here.
Test plan
Core3006, native all-feature suites and strict Core lint pass; Fastly WASM check passes. Fastly-only synchronous hooks bridge the original async operations; native and worker executors do not gain nested blocking executors. Current-main PR1526 remains untouched.
The b67ffc6 baseline checkpoint has passing documented local gates: Fastly/default/reuse/native/Cloudflare/Spin suites, all adapter strict lint lanes, 17 cross-adapter parity cases, 1252 JS cases, CLI suites, formatting, clock/digest/proposal checks and changed-lines domain lint. Passing checks at121ebcc36 are reused only for unchanged relevant inputs; the final follow-up covers the CLI test-import and documentation-only corrections. These results are not a substitute for this PR's current-head hosted checks.
Checklist
Operational Terraform remains in trusted-server-deployer. Local checks and prototype tests do not qualify cloud access, fleet durability or a live publisher.
Hosted docs-build correction
The first hosted format-docs job caught an additional site-build gate that the original local formatter checkpoint had not run: a relative link from the native EC guide to a Terraform example outside the site. Native EC correction8b11dbad3 replaces it with the plain repository path; no dead-link check is disabled. That docs-only correction is carried through the existing upper PR identities. Native EC and final integration8985b111 both pass docs ESLint and VitePress build; the final docs formatter also passes. Runtime, dependency, image-build-context and test inputs are unchanged from b67ffc6, so their prior exact-input proofs are reused. Hosted current-head checks still control readiness.
Startup negative-probe correction
A hosted native EC case failed the no-listener probe while its child logged the expected startup validation failure. The old log has no socket endpoints, so the failure's exact subtype is not claimed confirmed. A separate controlled Linux reproduction demonstrates a successful TCP self-connection with no listening server. Native correction8490c691d distinguishes equal local/peer endpoints from a real listener; actual socket controls prove the self-connection is excluded and a real listener is still detected. Nonzero exit, expected diagnostic, secret-redaction and the original startup deadline remain required. This changes only the acceptance fixture, not production admission, transport or trust.
The earliest native unit and final9356c557 default/all-feature acceptance checks and strict lint pass, including both controls and the original failure case. Ignores remain explicit; earlier separately executed celld proof is not relabeled as a new ignored-case run. Correction is merged upward through existing PR identities without force-pushing or retargeting. Current-head hosted checks must pass; the old failure is retained rather than rerun-only waived. Runtime/API/dependency inputs remain unchanged. A fresh exact9356 producer build and real runtime smoke now pass: image
sha256:65a735d38e9dbc0573d0f6068a8b261bd202c43aea05085c2b2d6bccc91ebdbd, linux/amd64 UID/GID10001, including startup rejection and in-flight SIGTERM drain. Prior b67 image receipts remain separate; no image was pushed. All local parent runners are complete.Browser CI host compatibility
Preserved older browser failures show Playwright1.58.2 rejecting ubuntu-latest's new Ubuntu26.04 image before browser installation; later missing-executable failures are consequences, not passing browser tests. Nativeb1216763a pins only the browser job to ubuntu-24.04, leaving dependency versions, tests and assertions unchanged. Direct Actionlint1.7.12 passes at the owning native and final integration points; the first shim-only attempt could not run because no global version was configured, and is not counted as lint proof. No global tooling was changed. The only9356 ->ee7 delta is this workflow host/comment; runtime, Cargo, docs, tests and Docker-context inputs are unchanged. Actual browser execution on the supported hosted runner remains pending and must pass before readiness. No waiver or browser skip.