Make EC withdrawal tombstoning idempotent across request bursts

[ChristianPavilonis]

Summary

• Make existing EC withdrawal tombstones a true no-op across repeated and bursty requests.
• Preserve the first tombstone's 24-hour TTL by avoiding backend work once authoritative tombstone state is observed.
• Keep existing-key-only privacy, CAS race handling, two-ID withdrawal, and best-effort browser response behavior intact.
• Use an exact completion-marker check to suppress stale-read rewrites without adding marker I/O to normal fresh-ID creation.

This PR is stacked on #900.

Changes

File	Change
crates/trusted-server-core/src/ec/kv.rs	Return matching tombstone snapshots before KV work, use exact marker-first stale-miss checks, keep fresh creation independent of markers, narrow tombstone APIs, and add operation/race/namespace coverage
crates/trusted-server-core/src/ec/finalize.rs	Add two-present-ID, repeated-withdrawal, and KV-failure cookie-deletion integration coverage
crates/trusted-server-core/src/ec/mod.rs	Document that snapshot generations do not independently authorize writes
docs/guide/edge-cookies.md	Document repeated-withdrawal TTL behavior
docs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.md	Record the reviewed lifecycle semantics and verification contract

Closes

Closes #881

Test plan

• cargo fmt --all -- --check
• cargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasm
• cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spin
• ./scripts/test-cli.sh
• cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity
• JS tests: cd crates/trusted-server-js/lib && npx vitest run
• JS format: cd crates/trusted-server-js/lib && npm run format
• JS build: cd crates/trusted-server-js/lib && node build-all.mjs
• 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

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

[aram356]

@ChristianPavilonis please assign ticket for this PR

[prk-Jr]

Summary

Solid change. The core design holds up: the completion marker is written after the root tombstone succeeds and cleared before a key can go live again, so every partial failure lands on the side that permits a future withdrawal write rather than suppressing one. consent.ok = false can only originate from KvEntry::tombstone() (every other constructor sets ok: true, and all upsert paths reject tombstones), so the new authoritative-tombstone short-circuit can never skip clearing partner IDs.

No blocking findings. All six comments below are non-blocking.

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 comments describe the fix in prose because the change spans multiple ranges or is a design question rather than a patch.

Non-blocking

♻️ refactor

• RecordingEcKv does not record list operations — see inline at crates/trusted-server-core/src/ec/kv.rs:2031
• Two strongly-consistent list ops on the path this PR optimizes — see inline at crates/trusted-server-core/src/ec/kv.rs:967

🤔 thinking

• One extra in-path KV write per withdrawal — see inline at crates/trusted-server-core/src/ec/kv.rs:1103
• Marker list failure now fails create_or_revive outright — see inline at crates/trusted-server-core/src/ec/kv.rs:375

🌱 seedling

• clear_withdrawal_marker's delete-failure recheck is untested — see inline at crates/trusted-server-core/src/ec/kv.rs:875

⛏ nitpick

• write_withdrawal_tombstone is pub with no production caller outside kv.rs — see inline at crates/trusted-server-core/src/ec/kv.rs:909

Cross-cutting / body-level findings

• 📝 PR description overstates the removal — the body says "remove the dead unconditional overwrite API", but no pub fn was removed in this diff. Only the write_withdrawal_tombstone_overwrites_live_entry test was deleted; the API is still pub (see the inline nitpick). The body's Changes table also lists 3 files — docs/guide/edge-cookies.md is a 4th.

• 📝 create_or_revive has no production callers — generate_if_needed (crates/trusted-server-core/src/ec/mod.rs:382) uses create_if_absent, and on an AlreadyExists collision it mints a new EC ID rather than reviving the existing key. So in production no live key can ever inherit a stale completion marker, and the new clear_withdrawal_marker calls at kv.rs:375 / kv.rs:411 plus their two tests exercise a test-only path. Not a defect — the safety property is real, it just has no production flow to protect today. Worth noting that create_or_revive's doc comment still claims "Called by generate_if_needed() instead of create()", which is stale (pre-existing on the base branch, not introduced here).

• 👍 Fail-safe ordering throughout — clear-before-write on revival, mark-after-write on withdrawal, and record_withdrawal_completion swallowing marker errors after a successful root write. withdrawal_marker_exists can only produce false negatives (an under-filled list page), which fall back to the unconditional privacy write; false positives are impossible. The asymmetry is in the right direction everywhere.

• 👍 The TTL-refresh proof is well constructed — recording RecordedEcKvInsert { mode, ttl } at the wrapper boundary and asserting zero further insert attempts on repetition is genuinely stronger evidence that the 24-hour tombstone TTL was not refreshed than a stable consent.updated timestamp would be. finalize_withdrawal_keeps_cookie_deletion_on_kv_failure is a good addition too — it pins the "cookie deletion is the primary enforcement mechanism" contract against a fully unavailable store.

CI Status

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

[aram356]

Summary

This PR makes EC withdrawal tombstoning idempotent, and the core design (authoritative-tombstone fast path, CAS-guarded write, strongly-read completion marker for the stale-miss fallback) is sound. However, the branch does not compile: two required checks (cargo fmt, cargo test) fail, and the compile error is masking a second, independent test failure underneath it.

Both blocking build issues were reproduced locally and fixes verified through the full CI gate list in CLAUDE.md.

2 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change spans multiple call sites or lines outside the diff and can't be auto-applied.

Blocking

🔧 wrench

• Duplicate use breaks the test build — see inline at crates/trusted-server-core/src/ec/kv.rs:1591
• New KV-failure test asserts a header that is never emitted — see inline at crates/trusted-server-core/src/ec/finalize.rs:1640
• create_or_revive marker clearing is unreachable; the live re-consent path never clears the marker — see Cross-cutting below

❓ question

• PR body and plan claim gates that do not pass — see Cross-cutting below

Non-blocking

🤔 thinking / ♻️ refactor / 📝 note / 📌 out of scope

• Present { tombstone, generation: Some(g) } is newly representable — see inline at crates/trusted-server-core/src/ec/kv.rs:1139
• write_withdrawal_tombstone should be narrowed to pub(crate) — see Cross-cutting below
• withdrawal_marker_exists uses prefix matching where the codebase uses exact — see inline at crates/trusted-server-core/src/ec/kv.rs:883
• Marker/cluster-count collision verified impossible, but untested — see Cross-cutting below
• Spec docs still describe a single-namespace key space — see Cross-cutting below

Cross-cutting / body-level findings

• 🔧 create_or_revive marker clearing is dead code; the live re-consent path never clears the marker — This PR clears withdrawal markers in create_or_revive (kv.rs:388 and kv.rs:423) and in delete (kv.rs:1343). Neither method has a production caller. Both production re-consent/creation sites call create_if_absent instead (ec/finalize.rs:188, ec/mod.rs:389), which does not clear the marker. The invariant the plan states — "revival or hard deletion clears the marker before the key can become live again" — is therefore not enforced on any reachable path.

  Practical impact today is bounded: both create_if_absent call sites operate on freshly generated EC IDs, so hitting a pre-existing marker is effectively unreachable, and markers carry a 24h TTL. But the guard is unexercised against the path that actually runs, so a future change to the re-consent flow would silently inherit a stale marker that suppresses the next withdrawal's fallback write.

  Two things worth deciding: (a) should create_if_absent clear the marker (with a test on the real path), and (b) if create_or_revive genuinely has no callers, should it be removed rather than extended? Note also that create_or_revive now ?-propagates a marker-list failure before the fast-path Add (kv.rs:388) — if that method ever becomes live, a marker read failure turns EC creation into a hard error where it previously succeeded.

• ❓ PR body and plan claim gates that do not pass — The PR description checks off cargo test-fastly && cargo test-axum, and the plan document is marked Status: Implemented and verified (docs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.md:3) with a Definition of Done requiring "every applicable repository gate" to pass. The branch does not compile, so the entire trusted-server-core lib-test target never ran. Was the final verification run before the last commit (7b0fab389) was pushed? Asking because it affects how much of the plan's "verified" checklist should be re-run rather than trusted.

• ♻️ write_withdrawal_tombstone should be narrowed — kv.rs:979 is still pub, but after this PR it has zero callers outside the ec module, and its only production caller is internal (kv.rs:1084, inside tombstone_unproven_missing). Its record_snapshot: impl FnOnce parameter and the 11-line "Propagating the result" rationale (kv.rs:960-970) exist specifically to discipline the finalize.rs caller this PR deletes; the surviving internal caller now works around the callback with a mutable capture (kv.rs:1083-1086). Consider pub(crate), or folding it into tombstone_unproven_missing as a private helper and dropping the callback. TombstoneOutcome (kv.rs:141) is likewise crate-internal now — finalize.rs dropped its import in this diff.

• 📝 Marker/cluster-count collision verified impossible, but untested — I confirmed the plan's claim that markers are "excluded from hash-prefix cluster counts" holds: ec_hash (ec/generation.rs:109-115) returns the bare leading 64-hex with no dot, and marker keys begin with _, which is not an ASCII hex digit, so no marker can ever be prefixed by a cluster hash. There is no regression test pinning this, though — no test seeds a marker alongside a cluster count. Since the whole namespace-safety argument rests on this property, a small test asserting count_hash_prefix_keys is unchanged by a present marker would be cheap insurance.

• 📌 Spec docs still describe a single-namespace key space — docs/guide/edge-cookies.md was updated, but docs/superpowers/specs/2026-03-24-ssc-technical-spec-design.md:577 still states "KV key: Full EC ID in {64-char hex}.{6-char alphanumeric} format", and the store table at :571-572 lists one row. That is now an incomplete description of the store's key space. The plan explicitly says "Historical design documents may remain unchanged" (:135), so flagging rather than blocking — but a one-line annotation on the spec's store table would keep it honest.

Verification performed

With findings 1 and 2 applied in an isolated review worktree at 7b0fab389, the full CLAUDE.md CI gate passes:

• cargo fmt --all -- --check
• cargo clippy-fastly, clippy-axum, clippy-cloudflare, clippy-cloudflare-wasm, clippy-spin-native, clippy-spin-wasm
• cargo test-fastly — 2702 passed, 0 failed
• cargo test-axum, cargo test-cloudflare, cargo test-spin — all pass
• cross-adapter parity suite — 13 passed

Without those two fixes, cargo test-fastly fails to compile (E0252), and with only the first fix applied, ec::finalize::tests::finalize_withdrawal_keeps_cookie_deletion_on_kv_failure aborts the wasm test binary (exit 134).

CI Status

• cargo fmt: FAIL (required)
• cargo test: 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 (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• vitest: PASS
• browser integration tests: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS
• Analyze (rust): PASS
• Analyze (actions): PASS
• Analyze (javascript-typescript): PASS
• CodeQL: PASS

Both failing checks share a single root cause (finding 1) plus the failure it masks (finding 2).

[aram356]

Summary

Second review pass, against 91558be1a. All nine findings from the previous pass are genuinely addressed — I verified each against the code rather than the reply: the duplicate imports are gone, the KV-failure test now seeds PullSyncMarkerState::Invalid, write_withdrawal_tombstone and TombstoneOutcome are pub(crate), withdrawal_marker_exists uses exact key_exists instead of prefix counting, the EcKvSnapshot::Present generation invariant is documented, and a cluster-count regression test now exists. CI is green across all 19 checks.

Two new blocking findings come out of changes made in this revision. The first is a consequence of moving the marker check ahead of the root existence check; the second is a guard with no test holding it in place, which I confirmed by mutation.

1 of the inline comments below carries a one-click GitHub suggestion. The rest describe the fix in prose because they need a design decision, span multiple call sites, or would add code outside the existing hunks.

Blocking

🔧 wrench

• Stale completion marker can suppress a real withdrawal on a live row — see inline at crates/trusted-server-core/src/ec/kv.rs:1053
• clear_withdrawal_marker's early-return guard is silently deletable — see inline at crates/trusted-server-core/src/ec/kv.rs:897

Non-blocking

♻️ refactor / 🌱 seedling / 📌 out of scope

• Stale-miss path performs three strongly consistent list operations — see Cross-cutting below
• Eight bare assertions without messages in new test code — see inline at crates/trusted-server-core/src/ec/kv.rs:2144
• create_or_revive_fresh_entry_ignores_marker_store_failure is now tautological — see inline at crates/trusted-server-core/src/ec/kv.rs:1978
• Test-double sprawl: eleven EcKvStore implementations for one trait — see Cross-cutting below
• Spec docs still describe a single-namespace key space — see Cross-cutting below

Cross-cutting / body-level findings

• ♻️ Stale-miss path performs three strongly consistent list operations — On Fastly, key_exists is a strong build_list page, the most expensive operation in the set. The stale-miss-with-existing-row path now runs three of them: withdrawal_marker_exists (kv.rs:1053), key_exists_confirmed (kv.rs:1072), and then write_withdrawal_tombstone → tombstone_held_identity → key_exists_confirmed again (kv.rs:1020).

  The third is redundant with the second: tombstone_unproven_missing has already proven the row exists immediately before calling into the helper that re-proves it. The duplication predates this PR, but this is the path the completion marker exists to make cheap, so it is worth collapsing now — either by giving tombstone_held_identity a variant that skips the check when the caller has already confirmed existence, or by having tombstone_unproven_missing write the tombstone directly.

  No test asserts the operation count for this path, so the triple check is currently unmeasured. tombstone_existing_from_repeated_stale_miss_preserves_first_write (kv.rs:2966) does assert all five counters for the repeated stale miss and is the strongest test in the file — the first-time stale miss deserves the same treatment.

• ♻️ Test-double sprawl — This PR adds three EcKvStore implementations to kv.rs's test module (DisappearOnConflictEcKv at kv.rs:1403, RecordingEcKv at kv.rs:2343, MarkerFailingEcKv at kv.rs:2426), joining four already there and four more in kv_backend::test_support — eleven doubles for a single trait.

  Two pairs are near-duplicates worth merging: RecordingEcKv is a strict superset of CountingEcKv (kv.rs:3705) except for the latter's liveness-based lag, and the stale-lookup mechanic is now implemented a third time (RecordingEcKv::with_stale_lookups at kv.rs:2354, MarkerFailingEcKv's counter at kv.rs:2428) alongside the existing StaleLookupEcKv in kv_backend.rs. MarkerFailingEcKv is also over-configured: five fields across three constructors producing three mutually exclusive configurations, including a tri-state Option<bool> serving two tests.

  Roughly 1,555 added lines in kv.rs against a production delta of about 60. Given the privacy stakes the coverage depth is defensible; the infrastructure duplication is what I would trim.

• 📌 Spec docs still describe a single-namespace key space — docs/superpowers/specs/2026-03-24-ssc-technical-spec-design.md:577 still states "KV key: Full EC ID in {64-char hex}.{6-char alphanumeric} format", which no longer fully describes the store. Carried over from the previous pass; the plan explicitly defers historical specs, so flagging rather than blocking.

Verification performed

Full CLAUDE.md CI gate re-run locally in an isolated worktree at 91558be1a, independent of GitHub's checks:

• cargo fmt --all -- --check
• cargo clippy-fastly, clippy-axum, clippy-cloudflare, clippy-cloudflare-wasm, clippy-spin-native, clippy-spin-wasm — all clean
• cargo test-fastly — 2712 passed, 0 failed
• cargo test-axum, cargo test-cloudflare, cargo test-spin — all pass
• cross-adapter parity — 13 passed

Mutation check for finding 2: removing the early-return guard at kv.rs:897-899 leaves all 2712 tests passing.

CI Status

All 19 checks pass, including both previously-failing required checks:

• cargo fmt: PASS (required)
• cargo test: PASS (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 (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• vitest: PASS
• browser integration tests: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS
• Analyze (rust): PASS
• Analyze (actions): PASS
• Analyze (javascript-typescript): PASS
• CodeQL: PASS