Repository navigation
Make shared EC persistence awaitable - #1526
ChristianPavilonis wants to merge 4 commits into
Conversation
Extract the local-future EC store contract and existing caller awaits from 812354b without native persistence or issuance policy changes. Await pull-sync transport operations directly. Bridge only synchronous Fastly entry-point finalization and post-send hooks with block_on. Keep Fastly KV hostcalls and existing consent and write semantics. Add regression coverage for non-Send yielding stores and pull sync inside the Fastly executor boundary.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Reviewed e529e55 against cd8977c.
No blocking or non-blocking defects found in the scoped async EC conversion. Inspected storage and caller changes for preservation of issuance backing proof, consent gates, CAS retry and write-precondition behavior, tombstones, snapshot classification, batch failure accounting, and Fastly executor boundaries. The router, pre-send finalization, response streaming, and post-send work execute at sequential boundaries; pull sync directly awaits transport operations.
The delayed non-Send store regression provides useful suspension coverage. Confirmed in current-head hosted logs that it and the pull-sync executor-boundary regression executed and passed. Hosted test and lint results were reused; no local builds or tests were rerun. Large unrelated unchanged publisher and adapter code was sampled around affected paths. Actual entry-point post-send timing, cancellation rollback, native providers, and deployed storage consistency are outside this review's qualification.
CI Status
- CodeQL: PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- browser integration tests: PASS
- integration tests: PASS
- trace runtime and browser acceptance: PASS
- integration tests (Fastly EC lifecycle): PASS
- docs-proposal scripts: PASS
- CLAUDE.md symlink guard: PASS
- cargo fmt: PASS
- HTML EOF runtime (Fastly): PASS
- CI Node pin guard: PASS
- cargo test: PASS
- format-typescript: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- format-docs: PASS
- vitest: PASS
- ts dev lint domains (changed lines): PASS
- worker-deadlines (cloudflare): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (cross-adapter parity): PASS
- prepare integration artifacts: PASS
- worker-deadlines (spin): PASS
- cargo test (axum native): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
The trait change and caller conversion check out. Production code is the existing logic with async/.await added, plus the inlined withdrawal loop and the removal of the two block_on calls inside pull sync. No operation is reordered, no future is left unawaited, no lock is held across an await, and no new block_on is reachable inside the router or the new bridges. The blocking items are a test gap this PR opens (finalization now runs under futures::executor::block_on, and no test drives it there), spec provenance that cites commits and hashes no reader can resolve, and documentation that has not caught up with the async contract.
11 of the 14 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 3 describe the fix in prose because it touches lines outside the diff hunks or several sites.Each suggestion was applied alone to this head and checked with rustfmt, all six adapter clippy aliases (the Fastly ones for the
main.rscomment), the finalize or pull-sync tests undercargo test-fastlywhere it adds test code, and docs Prettier for the spec. All of them applied together passcargo test-fastly,test-fastly-reuse,test-axum,test-cloudflare,test-spinand the parity suite.
Blocking
🔧 wrench
- No test drives finalization under the executor Fastly now uses: see inline at
crates/trusted-server-core/src/ec/finalize.rs:681 - The spec names commits and a tree that do not exist: see inline at
docs/superpowers/specs/2026-11-10-shared-async-ec-design.md:5 - The review log cites artifacts nobody can check: see inline at
docs/superpowers/specs/2026-11-10-shared-async-ec-design.md:91 - Assertion messages on the new pull-sync regression: see inline at
crates/trusted-server-core/src/ec/pull_sync.rs:986 - Record why the send sits between the two bridges: see inline at
crates/trusted-server-adapter-fastly/src/main.rs:549 - The
block_oninvariant as written is contradicted by the entry point: see inline atdocs/superpowers/specs/2026-11-10-shared-async-ec-design.md:68 - The two "retained" regressions are new: see inline at
docs/superpowers/specs/2026-11-10-shared-async-ec-design.md:74 EcFinalizeStatehas no snapshots field and is not changed here: see inline atdocs/superpowers/specs/2026-11-10-shared-async-ec-design.md:50- State the cancellation contract on the trait: see inline at
crates/trusted-server-core/src/ec/kv_backend.rs:75 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:115- Fold the two finalize, send and pull-sync blocks into one helper: see inline at
crates/trusted-server-adapter-fastly/src/main.rs:577 - The active-ID guard on the snapshot correction is untested: see inline at
crates/trusted-server-core/src/ec/finalize.rs:492
Cross-cutting / body-level findings
- 🔧 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. So the Fastly EC lifecycle job exercises the new finalize bridge but never the pull-sync bridge with the realFastlyPlatformHttpClient, and the new regression usesStubHttpClient. Please either add a pull-enabled partner backed by a local fixture endpoint to the integration suite, or open a follow-up issue and link it from the spec's "Evidence and limits" section. - 🔧 The PR description overstates the checks and mixes in private context.
- "All46 current-head hosted checks passed": the 46 are 23 jobs counted twice (two identical
pull_requestruns).gh pr checksnow reports 29 checks, all passing, since CodeQL finished. - The
pbs.example.comfixture paragraph describes base commitcd8977cad(#1525), brought in by mergee529e5576; it is not part of this diff. - "Retain the original demo RC #1424 and its dirty worktrees", "raw eligibility retained", "Original failure/ignore receipts retained separately" and the claim about "the later compatibility/native descendant" refer to local or private state a reviewer cannot see. Please drop them or link what they refer to.
- "All46" and "All27" are missing a space.
- "All46 current-head hosted checks passed": the 46 are 23 jobs counted twice (two identical
- 🔧 A native backend for this trait needs EdgeZero KV features that do not exist yet. The trait's stated purpose is to let a backend with real async I/O yield, but EdgeZero's
KvStore(imported in core asPlatformKvStore) offers onlyget_bytes,exists,put_bytes,put_bytes_with_ttl,deleteandlist_keys_page, and documents Fastly and Cloudflare KV as eventually consistent. It has no generation token, no conditional (add or if-generation-match) write, no metadata and no strong-consistency listing, andEcKvStorerequires all of these. The native follow-on therefore depends on EdgeZero asks rather than on Trusted Server plumbing. Please record that dependency in the spec's "Evidence and limits" section so the follow-on is scoped against it.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CI Node pin guard: PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- HTML EOF runtime (Fastly): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- trace runtime and browser acceptance: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- worker-deadlines (spin): PASS
| @@ -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 now uses
This PR moves apply_edgezero_ec_finalize inside futures::executor::block_on (crates/trusted-server-adapter-fastly/src/main.rs:550 and :577). 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 therefore pass CI and panic in production on every withdrawal or orphan-recovery request.
Reproduced at this head: with let outcome = futures::executor::block_on(graph.tombstone_existing_from_snapshot(ec_id, initial)); at line 480, all 40 finalize tests still pass. dispatch_pull_sync_runs_inside_the_fastly_executor_boundary already guards pull sync this way; finalization needs the same guard. The suggestion adds it where the removed apply_withdrawal_tombstones_invokes_writer_for_each_ec_id test sat. It passes at this head, and under the mutation above it is the only failing test (cannot execute LocalPool executor from within another executor: EnterError). Please also list it with the other new regressions in the spec's evidence section.
| #[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" | |
| ); | |
| }); | |
| } | |
| Status: reviewed reconstruction of an implemented unit. Two independent reviews followed implementation and preceded readiness disposition; this does not record preimplementation approval or deployment readiness. | ||
|
|
||
| The scoped change is `2c79ea596621143c38167a0ee33e39d32696309e` to `7d0add324349f349933da1d8f85bb450b990f02b`. The implementation head has tree `77ce936af36cdf2233a2f53c8f739f0c56c02c5c`. |
There was a problem hiding this comment.
🔧 wrench: The spec names commits and a tree that do not exist
2c79ea59…, 7d0add32… and tree 77ce936a… are not in this PR, its base, main or #1424, and GitHub returns 422/404 for them, so no reader can check what "the scoped change" was. The implementation in this PR is 8118581bc (tree b7284b62…), and its commit message names 812354b8e as the source, which is reachable on rc/edgezero-431-demo. The status sentence is also process history rather than design state. Suggest an Issue/Status line like other specs in this directory:
| Status: reviewed reconstruction of an implemented unit. Two independent reviews followed implementation and preceded readiness disposition; this does not record preimplementation approval or deployment readiness. | |
| The scoped change is `2c79ea596621143c38167a0ee33e39d32696309e` to `7d0add324349f349933da1d8f85bb450b990f02b`. The implementation head has tree `77ce936af36cdf2233a2f53c8f739f0c56c02c5c`. | |
| **Issue:** #1522. **Status:** implemented in #1526 by `8118581bc`, which extracts the shared async EC store and caller conversion from `812354b8e` (#1424) without its native persistence. |
| ## Review log | ||
|
|
||
| The implementation and its execution evidence existed before the reconstructed contract. Both independent reviewers read the frozen contract and complete 14-file implementation delta. These reviews do not backdate design approval or establish execution of later combined trees. | ||
|
|
||
| | Date | Round | Frozen target | Result | | ||
| | ---------- | -------------- | ----------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | 2026-11-10 | 1 | Contract SHA256 `8d7d0cd3e61a9286eab9a44077fb0e0ec68b924361925aff944167dcc6f2e673`; implementation `2c79ea59..7d0add32` | No blocking or nonblocking findings. Full source delta and matching prior proof inputs inspected; no new runtime commands executed. | | ||
| | 2026-11-10 | 2, independent | Same frozen contract and implementation; all 16 source/manifest fingerprints checked | No findings or requested changes. Local-future ownership, CAS/privacy policy, SDK mechanics and lifecycle proof limits inspected independently. | | ||
|
|
||
| Private execution and review records remain outside the public package. This copy retains the reviewed contract and records completed reviews without publishing private qualification history. The extraction replays the unit without relay ancestry onto current main. Later SDK caller ports require their own affected checks; no historical whole-tree or production qualification is claimed. | ||
|
|
||
| ## Review and readiness boundary | ||
|
|
||
| The two recorded review rounds are complete for their exact frozen inputs. Current target-matched tests, lint, formatting and required combined checks remain publication/readiness obligations. Native persistence, actual service integration and deployment acceptance remain outside this unit. |
There was a problem hiding this comment.
🔧 wrench: The review log cites artifacts nobody can check
- Contract SHA256
8d7d0cd3…matches no file in any ref. The spec has one commit (8c1897d0d), and the file at this head hashes to96ae5a05…. 2c79ea59..7d0add32do not exist (see the comment on line 5).- "all 16 source/manifest fingerprints" conflicts with the 14-file delta in line 80.
- "Private execution and review records remain outside the public package", "do not backdate design approval" and the "Review and readiness boundary" section describe process rather than the design, and give a maintainer nothing to verify or act on.
The suggestion keeps both review rounds and their results, and replaces the readiness section with checks a maintainer can rerun:
| ## Review log | |
| The implementation and its execution evidence existed before the reconstructed contract. Both independent reviewers read the frozen contract and complete 14-file implementation delta. These reviews do not backdate design approval or establish execution of later combined trees. | |
| | Date | Round | Frozen target | Result | | |
| | ---------- | -------------- | ----------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | | |
| | 2026-11-10 | 1 | Contract SHA256 `8d7d0cd3e61a9286eab9a44077fb0e0ec68b924361925aff944167dcc6f2e673`; implementation `2c79ea59..7d0add32` | No blocking or nonblocking findings. Full source delta and matching prior proof inputs inspected; no new runtime commands executed. | | |
| | 2026-11-10 | 2, independent | Same frozen contract and implementation; all 16 source/manifest fingerprints checked | No findings or requested changes. Local-future ownership, CAS/privacy policy, SDK mechanics and lifecycle proof limits inspected independently. | | |
| Private execution and review records remain outside the public package. This copy retains the reviewed contract and records completed reviews without publishing private qualification history. The extraction replays the unit without relay ancestry onto current main. Later SDK caller ports require their own affected checks; no historical whole-tree or production qualification is claimed. | |
| ## Review and readiness boundary | |
| The two recorded review rounds are complete for their exact frozen inputs. Current target-matched tests, lint, formatting and required combined checks remain publication/readiness obligations. Native persistence, actual service integration and deployment acceptance remain outside this unit. | |
| ## Review log | |
| - 2026-11-10, round 1: independent review of this contract and the 14-file change, before the change was replayed onto this branch. No findings. No new commands were run. | |
| - 2026-11-10, round 2: independent review of the same inputs. No findings. | |
| Review of `8118581bc` as merged happens on #1526. | |
| ## Verification | |
| `cargo test-fastly`, `cargo test-fastly-reuse` and the target-matched clippy aliases cover this unit; the full CI gate list in `AGENTS.md` applies. Hosted results are on #1526. |
| 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.
🔧 wrench: Assertion messages on the new pull-sync regression
AGENTS.md asks for descriptive assertion messages and expect("should ...") messages. Both assert_eq! calls here have no message, and "row should exist" inverts the should ... format.
| 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 the eligible partner under an outer executor" | |
| ); | |
| let (entry, _) = graph | |
| .get(&ec_id) | |
| .await | |
| .expect("should read row") | |
| .expect("should keep the live row"); | |
| assert_eq!( | |
| entry.ids.get("alpha.example.com").map(|id| id.uid.as_str()), | |
| Some("synced-uid"), | |
| "should persist the pulled UID" | |
| ); |
| // Fastly's entry-point hooks are synchronous, and its KV SDK | ||
| // completes I/O through blocking hostcalls. Keep this bridge here, | ||
| // never in native or worker executors. |
There was a problem hiding this comment.
🔧 wrench: Record why the send sits between the two bridges
send_edgezero_response reaches send_response_to_client, which runs its own futures::executor::block_on(stream_asset_body(..)) for streaming bodies (line 766). Collapsing finalize, send and pull sync into one block_on(async { .. }), which is the obvious cleanup of these four call sites, would panic with EnterError on every streamed publisher or asset response. The comment explains why a bridge exists but not why there are two. The reload branch at line 577 has no comment, so this one should state the constraint for both.
| // Fastly's entry-point hooks are synchronous, and its KV SDK | |
| // completes I/O through blocking hostcalls. Keep this bridge here, | |
| // never in native or worker executors. | |
| // Fastly's entry-point hooks are synchronous, and its KV SDK | |
| // completes I/O through blocking hostcalls. Keep this bridge here, | |
| // never in native or worker executors. Keep the client send between | |
| // the two bridges: streaming bodies run their own `block_on` in | |
| // `send_response_to_client`, so a single combined bridge would nest. |
| /// 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.
🔧 wrench: 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 the async change, but on an async fn they now 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. The spec's own wording is "post-send work awaited by the entry point, not a durable background job".
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.
🔧 wrench: generate_if_needed under-documents its errors
The # Errors section predates this PR, but the signature changes here. It omits two Err paths: KV persistence failure (line 443) 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. |
| @@ -111,16 +113,16 @@ struct MappingGroup { | |||
| /// # Errors | |||
| /// | |||
| /// Returns [`TrustedServerError`] on serialization or KV store failures. | |||
There was a problem hiding this comment.
🔧 wrench: 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 138), invalid JSON (BadRequest at line 161) and response serialization. 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 body 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. |
| match load_settings_from_config_store(&runtime_stores) { | ||
| Ok(settings) => { | ||
| match apply_edgezero_ec_finalize(&settings, &mut ec_state, &mut response) { | ||
| match futures::executor::block_on(apply_edgezero_ec_finalize( |
There was a problem hiding this comment.
🔧 wrench: Fold the two finalize, send and pull-sync blocks into one helper
The snapshot branch (line 546) and the reload branch (line 575) already duplicated this flow at base. This PR adds two block_on bridges to each, so the ordering constraint from the comment at line 547 now has to hold in four places. A helper keeps it in one:
/// Finalizes EC state, sends the response, then runs post-send pull sync.
///
/// Returns the unsent response when the partner registry cannot be built, so
/// the caller can send it without EC finalization.
fn finalize_ec_and_send(
settings: &Settings,
ec_state: &mut EcFinalizeState,
mut response: HttpResponse,
request_filter_effects: Option<&RequestFilterEffects>,
counters: Option<&SandboxCounters>,
) -> Option<HttpResponse> {
// Keep the client send between the two bridges: streaming bodies run their
// own `block_on` in `send_response_to_client`, so one bridge would nest.
match futures::executor::block_on(apply_edgezero_ec_finalize(
settings,
ec_state,
&mut response,
)) {
Ok(partner_registry) => {
send_edgezero_response(response, request_filter_effects, counters);
futures::executor::block_on(run_edgezero_pull_sync_after_send(
settings,
&partner_registry,
ec_state,
));
None
}
Err(e) => {
log::error!("EdgeZero EC finalize skipped: failed to build partner registry: {e:?}");
Some(response)
}
}
}and in edgezero_main:
if let Some(mut ec_state) = ec_state {
let reloaded;
let settings = match settings_snapshot.as_deref() {
Some(settings) => Some(settings),
None => match load_settings_from_config_store(&runtime_stores) {
Ok(settings) => {
reloaded = settings;
Some(&reloaded)
}
Err(e) => {
log::warn!("EdgeZero EC finalize skipped: failed to reload settings: {e:?}");
None
}
},
};
if let Some(settings) = settings {
match finalize_ec_and_send(
settings,
&mut ec_state,
response,
request_filter_effects.as_ref(),
counters.as_ref(),
) {
None => return,
Some(unsent) => response = unsent,
}
}
}Log messages and branch behavior are unchanged; this compiles, passes cargo clippy-fastly and passes the Fastly adapter tests at this head. Apply manually: it adds a function and restructures both branches, which cannot be expressed as one suggestion.
| @@ -491,7 +492,7 @@ fn finalize_unusable_consent( | |||
| if ec_context.ec_value() == Some(ec_id) { | |||
There was a problem hiding this comment.
🔧 wrench: 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 cookie names a different one. Removing the guard (always set_kv_snapshot(outcome)) passes every test at this head and at base: no test withdraws two present IDs and then checks which one the context snapshot belongs to. The closure-to-loop change here is the natural 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"
);
}
}Verified locally: it passes at this head and fails with the guard removed. Apply manually: it adds a test outside the diff hunks.
Summary
Ready for feature review. All46 current-head hosted checks passed at e529e55. This is not deployment or production readiness.
Stack and exact input
Head:
e529e5576c1052f0d2fb40a4f9920f0428e6e633. Base:review/http-auction-provider.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 #1522.
Spec
Shared async EC contract and completed review log. Original reviews cover the exact historical scoped implementation, not preimplementation approval or later combined-source readiness.
Test plan
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.Matching historical trait/backend proof remains bounded to its exact inputs. Current combined native Core and adapter tests pass on the later compatibility/native descendant, not this isolated head. All46 current-head hosted checks passed on this isolated head. No native provider, cancellation rollback, actual entry-point post-send qualification or fleet consistency is implied.
Checklist