Group batch-sync mappings by EC ID before KV updates

[ChristianPavilonis]

Summary

• Validate the full batch first, then group valid mappings by normalized EC ID in deterministic first-occurrence order.
• Apply only the last valid partner UID for each distinct EC through the existing CAS-protected writer while preserving per-input outcomes.
• Define and test grouped eligibility, error ordering, and infrastructure-abort accounting.

This PR is stacked on #901.

Changes

File	Change
crates/trusted-server-core/src/ec/batch_sync.rs	Group valid mappings before KV work, fan group outcomes back to original indexes, and add ordering/accounting/HTTP coverage
crates/trusted-server-core/src/ec/kv.rs	Add regression coverage for the existing conditional writer's CAS retry behavior
docs/guide/api-reference.md	Document normalized grouping, last-valid-wins, outcome fan-out, sorted errors, and groupwise failure behavior
docs/guide/ec-setup-guide.md, docs/guide/edge-cookies.md	Align operational guidance with the grouped contract
docs/superpowers/plans/2026-07-13-issue-882-group-batch-sync-by-ec-id.md	Record the reviewed design and verification contract

Closes

Closes #882

Test plan

• cargo test-fastly && cargo test-axum
• cargo clippy-fastly && cargo clippy-axum
• cargo fmt --all -- --check
• JS tests: cd crates/trusted-server-js/lib && npx vitest run
• JS format: cd crates/trusted-server-js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve — not run
• Other: focused grouped batch-sync/CAS tests, cargo test-cloudflare, cargo test-spin, cross-adapter parity, and all Cloudflare/Spin native + WASM clippy targets

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses log macros (not println!)
• New code has tests
• No secrets or credentials committed

[prk-Jr]

Summary

Grouping S2S batch-sync mappings by normalized EC ID before touching KV is correct as implemented, and the accounting invariant holds by construction: every input index lands in exactly one bucket (a validation error, or a single group that is accepted or fanned out as an error), and the abort slice groups[group_index..] includes the failing group, so accepted + errors.len() == mappings.len(). I traced the duplicate-UID orderings by hand and the final KV state is identical to the pre-grouping code — fewer writes, same result. No blocking findings.

3 of the 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 remaining comment describes the fix in prose because splitting the bundled test spans two ranges and adds a new test function, which can't be auto-applied.

Non-blocking

♻️ refactor

• Conflicting UIDs inside one group are discarded with no signal — see inline at crates/trusted-server-core/src/ec/batch_sync.rs:231
• handle_batch_sync_reports_grouped_success_and_rejection_counts bundles two tests — see inline at crates/trusted-server-core/src/ec/batch_sync.rs:883

⛏ nitpick

• expect message says "serialize" on a deserialize — see inline at crates/trusted-server-core/src/ec/batch_sync.rs:425
• "Therefore" is a non-sequitur in the docs contract — see inline at docs/guide/api-reference.md:182

Cross-cutting / body-level findings

• ⛏ Stale third mock result in process_mappings_aborts_on_kv_unavailable (crates/trusted-server-core/src/ec/batch_sync.rs:544-572) — three distinct EC IDs now produce two writer calls, so the queued third Ok(UpsertResult::Written) is dead. The test still catches a missing break (a third call would pop it and push accepted to 2, failing the accepted == 1 assertion), but the leftover misrepresents the new call shape. Drop it and add assert_eq!(writer.calls().len(), 2, "should stop after the failing group") — this PR added call recording precisely for that. These lines are unchanged by the diff, so there is no hunk to anchor an inline comment to.

• ♻️ No test at the batch size the issue is about — issue #882's acceptance criterion is "work bounded by distinct valid normalized EC IDs", but the largest duplicate case covered is 3 mappings. A MAX_BATCH_SIZE-sized batch of one repeated EC ID asserting writer.calls().len() == 1 would pin the actual performance contract rather than inferring it from the 2-3 mapping cases. The existing grouping tests prove the mechanism; this would prove the bound.

• 🤔 No CHANGELOG entry for a partner-visible contract change — CHANGELOG.md is actively maintained (59 bullets under [Unreleased], including endpoint-level behavior entries such as the admin Basic-auth coverage change) and follows Keep a Changelog's "all notable changes". This PR changes the documented /_ts/api/v1/batch-sync contract in two partner-observable ways: accepted now counts group members rather than individual writes, and the abort boundary moved from positional to groupwise, so kv_unavailable and accepted indexes can interleave (a partner retrying "from the first kv_unavailable index onward" will now re-send already-accepted mappings — harmless because the write is idempotent, but a behavior change to reason about). Neither this PR nor #901 touches the file. Worth a ### Changed bullet, or an explicit decision that batch-sync internals stay out of the changelog.

• 📝 Three-deep stack — 902 → 901 (fix/idempotent-ec-withdrawal-tombstones, open) → 900 (fix/no-op-kv-reads) → main. The diff reviewed here is the top delta only; merge order matters.

• 👍 Load-bearing details that were easy to get wrong — the explicit errors.sort_by_key plus the debug_assert_eq! accounting invariant: the sort is genuinely required now that abort fan-out appends group indexes out of order, and the assertion documents the one-outcome-per-input property at the point it could break. The new upsert_partner_id_if_exists_retries_cas_conflict test in kv.rs pins the CAS boundary the grouping now leans on and was previously uncovered by any upsert_partner_id_if_exists test. And documenting the A, B, A boundary change as intentional in both the API reference and the plan, rather than leaving it implicit, is the right call for a contract partners integrate against.

CI Status

All 14 reported checks pass. gh pr checks --required reports no required checks on this branch, so none of these is merge-blocking under branch protection.

• cargo fmt: PASS
• cargo test: PASS
• cargo test (axum native): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• vitest: PASS
• format-typescript: PASS
• format-docs: PASS
• prepare integration artifacts: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS

Locally, in a worktree at 3a1e5de3: cargo test -p trusted-server-core --lib ec:: (349 pass), cargo clippy -p trusted-server-core --target wasm32-wasip1 --all-targets --all-features -- -D warnings (clean), cargo fmt --all -- --check (clean), and the pinned docs prettier 3.8.1 --check (clean).

[prk-Jr]

Approving. Nothing in the earlier review was blocking: the grouping logic is correct, every input index resolves to exactly one outcome by construction, and the final KV state matches the pre-grouping code for every duplicate-UID ordering — with one writer call per distinct normalized EC ID instead of one per mapping.

The findings in #902 (review) stand as non-blocking follow-ups, at the author's discretion: three one-click suggestions (a debug log when a batch supplies conflicting UIDs for one EC, an expect message that says "serialize" on a deserialize, and a docs sentence whose "Therefore" points the implication backwards), plus test-hygiene and CHANGELOG notes in the review body.

CI is green across all 14 reported checks.

[aram356]

@ChristianPavilonis please assign issue to this PR

[aram356]

Summary

The grouping rework is well-executed: validation-before-write, the vector-plus-map split for deterministic first-occurrence ordering, group outcome fan-out, and the per-input accounting invariant are all correct, and the 22 tests cover the contract thoroughly. The docs and the recorded design plan match the implementation exactly.

The blocking issue is CI state rather than the algorithm: the required cargo test and cargo fmt checks are both red, and as a consequence none of this PR's new tests have actually executed in CI.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it as a commit on the PR branch. The remaining findings are prose because they concern CI state or span more than the diff.

Blocking

🔧 wrench

• CI is red: duplicate InMemoryEcKv import breaks the test build — see "Cross-cutting / body-level findings" below

Non-blocking

⛏ nitpick

• debug_assert_eq! missing the descriptive message CLAUDE.md requires — see inline at crates/trusted-server-core/src/ec/batch_sync.rs:275

Cross-cutting / body-level findings

• 🔧 CI is red: duplicate InMemoryEcKv import breaks the test build — cargo test and cargo fmt are both FAIL, and both are required checks under branch protection. Both trace to a single compile error:

  error[E0252]: the name `InMemoryEcKv` is defined multiple times
      --> crates/trusted-server-core/src/ec/kv.rs:1591:9
       |
  1401 |     use crate::ec::kv_backend::test_support::InMemoryEcKv;
       |         ------------------------------------------------- previous import of the type `InMemoryEcKv` here
  ...
  1591 |     use crate::ec::kv_backend::test_support::InMemoryEcKv;
       |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `InMemoryEcKv` reimported here
  

  Two things worth separating:

  1. This is inherited from the base branch, not introduced here. This PR's diff touches zero InMemoryEcKv lines, and PR #901 shows the identical two failing checks. The one-line fix (delete kv.rs:1591) belongs in #901.
  2. It means this PR's new tests have never run in CI. The whole trusted-server-core lib-test target fails to compile, so all 22 new batch_sync tests plus the new upsert_partner_id_if_exists_retries_cas_conflict test were skipped. Verified locally: with line 1591 deleted, all 22 batch_sync tests pass.

  Also inherited from #901 and surfacing only once compilation is unblocked: ec::finalize::tests::finalize_withdrawal_keeps_cookie_deletion_on_kv_failure fails on the base branch as well. Both need to clear in #901 before this PR can merge green.

  Finally, the PR description checks off cargo test-fastly && cargo test-axum and cargo fmt --all -- --check as passing. Those gates are currently red, so the test plan should be corrected.

• 👍 Infrastructure-abort accounting is precisely handled — crates/trusted-server-core/src/ec/batch_sync.rs:263-269. groups[group_index..] includes the failing group and then breaks, so no input is counted twice, and pre-validated inputs keep their specific error reasons instead of being flattened to kv_unavailable the way the old positional abort did. The interaction with the trailing errors.sort_by_key is subtle, and process_mappings_aborts_by_group_and_preserves_sorted_accounting pins it exactly.

CI Status

• cargo test: FAIL (required)
• cargo fmt: FAIL (required)
• format-typescript: PASS (required)
• format-docs: PASS (required)
• cargo test (axum native): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• vitest: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• prepare integration artifacts: PASS
• CodeQL: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS

[aram356]

Resolved — the bare debug_assert_eq! flagged in the previous pass now carries a descriptive message, matching CLAUDE.md and the other debug_assert_eq! sites in the codebase (publisher.rs:3021, openrtb.rs:480, registry.rs:823).

Verified at e9557c86: cargo fmt --all -- --check clean, cargo clippy-fastly clean, and cargo test -p trusted-server-core --lib passes 2729 tests with 0 failures.

[aram356]

Resolved — this stale third queued result (Ok(UpsertResult::Written)) could never be consumed, so the test would still have passed if the abort had failed to stop group processing. Removing it plus the new writer.calls().len() == 2 assertion now pins the abort boundary directly.

This was a latent gap in the previous revision that the prior review pass did not catch.