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]

♻️ refactor — Conflicting UIDs inside one group are discarded with no signal.

This is where a batch carrying two different UIDs for the same EC ID silently loses the earlier one. The final KV state matches the pre-grouping code, so it isn't a regression — but two different UIDs for one EC in one request is almost always a partner-side bug, and right now the response reports accepted: 2 with nothing in the logs to notice it by. debug rather than warn so a partner that legitimately repeats mappings can't flood the log.

Suggested change

	if let Some(&group_index) = group_indexes.get(&ec_id) {
	let group = &mut groups[group_index];
	group.partner_uid.clone_from(&mapping.partner_uid);
	group.indexes.push(index);
	if let Some(&group_index) = group_indexes.get(&ec_id) {
	let group = &mut groups[group_index];
	if group.partner_uid != mapping.partner_uid {
	log::debug!(
	"Batch sync index {index} supplies a conflicting UID for ec_id '{}'; keeping the last valid value",
	log_id(&ec_id),
	);
	}
	group.partner_uid.clone_from(&mapping.partner_uid);
	group.indexes.push(index);

Scratch-verified in a worktree at 3a1e5de3: cargo fmt --all -- --check clean, cargo clippy -p trusted-server-core --target wasm32-wasip1 --all-targets --all-features -- -D warnings clean, cargo test -p trusted-server-core --lib ec:: 349 pass, no post-verification drift.

[prk-Jr]

⛏ nitpick — This expect guards a deserialize, but the message says "serialize".

Suggested change

	serde_json::from_slice(&body).expect("should serialize batch-sync response")
	serde_json::from_slice(&body).expect("should deserialize batch-sync response")

The same swap appears twice more, at :906-907 ("should serialize grouped success response") and :927-928 ("should serialize grouped multi-status response") — both also on serde_json::from_slice. Those need the same one-word fix applied manually; they're separate ranges and can't ride this suggestion.

Scratch-verified in a worktree at 3a1e5de3: cargo fmt --all -- --check clean, clippy clean on wasm32-wasip1 --all-targets, 22 batch-sync tests pass, no post-verification drift.

[prk-Jr]

⛏ nitpick — "Therefore" points the implication the wrong way: sort order doesn't establish one-outcome-per-input. The exactly-one-outcome property is what the code guarantees (each index reaches either a validation error or exactly one group), and accepted + rejected == mappings.len() follows from that; the sort is an independent, separate guarantee about errors ordering.

Suggested change

	- `errors` is sorted by original input index. Therefore each input has exactly
	one outcome and `accepted + rejected` equals the number of submitted
	mappings. The endpoint returns `200 OK` only when all mappings are accepted;
	otherwise it returns `207 Multi-Status`.
	- Each input receives exactly one outcome, so `accepted + rejected` equals the
	number of submitted mappings. `errors` is sorted by original input index. The
	endpoint returns `200 OK` only when all mappings are accepted; otherwise it
	returns `207 Multi-Status`.

Scratch-verified in a worktree at 3a1e5de3: the repo's pinned docs prettier 3.8.1 --check guide/api-reference.md passes, no post-verification drift.

[prk-Jr]

♻️ refactor — This test bundles two independent scenarios into one #[test]: a written group returning 200 OK, and an ineligible group returning 207 Multi-Status. Two arrange-act-assert cycles in one test means a failure in the first half hides the second, and success_body / rejected_body are each shadowed from request JSON into response bytes partway through.

Both halves also hand-roll the response_json helper this PR adds at :420 — which handle_batch_sync_reports_validation_errors_without_writer_calls does use. Splitting them makes the helper pay for itself:

#[test]
fn handle_batch_sync_accepts_every_member_of_a_written_group() {
    let registry = test_registry();
    let limiter = MockRateLimiter {
        should_exceed: false,
    };
    let ec_id = format!("{}.ABC123", "a".repeat(64));
    let writer = MockWriter::new(vec![Ok(UpsertResult::Written)]);
    let body = format!(
        r#"{{"mappings":[{{"ec_id":"{ec_id}","partner_uid":"one","timestamp":1}},{{"ec_id":"{ec_id}","partner_uid":"two","timestamp":2}}]}}"#
    );

    let response =
        handle_batch_sync_with_writer(&writer, &registry, &limiter, authorized_batch_request(&body))
            .expect("should return success response");

    assert_eq!(response.status(), StatusCode::OK);
    let json = response_json(response);
    assert_eq!(json["accepted"], 2);
    assert_eq!(json["rejected"], 0);
}

#[test]
fn handle_batch_sync_rejects_every_member_of_an_ineligible_group() {
    // … same shape with Ok(UpsertResult::NotFound), MULTI_STATUS, and the
    // existing errors assertion.
}

Apply manually — can't be auto-applied as a suggestion because the change spans two separate ranges in this test and introduces a second test function.

[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.