Make EC withdrawal tombstoning idempotent across request bursts - #901
Make EC withdrawal tombstoning idempotent across request bursts#901ChristianPavilonis wants to merge 2 commits into
Conversation
9aa4f0f to
83028e7
Compare
|
@ChristianPavilonis please assign ticket for this PR |
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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
RecordingEcKvdoes not record list operations — see inline atcrates/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_reviveoutright — see inline atcrates/trusted-server-core/src/ec/kv.rs:375
🌱 seedling
clear_withdrawal_marker's delete-failure recheck is untested — see inline atcrates/trusted-server-core/src/ec/kv.rs:875
⛏ nitpick
write_withdrawal_tombstoneispubwith no production caller outsidekv.rs— see inline atcrates/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 fnwas removed in this diff. Only thewrite_withdrawal_tombstone_overwrites_live_entrytest was deleted; the API is stillpub(see the inline nitpick). The body's Changes table also lists 3 files —docs/guide/edge-cookies.mdis a 4th. -
📝
create_or_revivehas no production callers —generate_if_needed(crates/trusted-server-core/src/ec/mod.rs:382) usescreate_if_absent, and on anAlreadyExistscollision 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 newclear_withdrawal_markercalls atkv.rs:375/kv.rs:411plus 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 thatcreate_or_revive's doc comment still claims "Called bygenerate_if_needed()instead ofcreate()", 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_completionswallowing marker errors after a successful root write.withdrawal_marker_existscan 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 stableconsent.updatedtimestamp would be.finalize_withdrawal_keeps_cookie_deletion_on_kv_failureis 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
| self.inner.insert(key, write) | ||
| } | ||
|
|
||
| fn count_keys_with_prefix( |
There was a problem hiding this comment.
♻️ refactor — RecordingEcKv doesn't record list operations, so the "skips backend" assertions can't catch an added prefix list.
The doc comment above the struct says it "records every backend operation before delegation", but count_keys_with_prefix here and delete just below both delegate unrecorded. This PR turns entirely on how many strongly-consistent list ops each path performs — withdrawal_marker_exists is a list — and the plan's verification contract asks the wrapper to count operations at the boundary.
Concretely: a future change that added a marker lookup to the authoritative-tombstone fast path would still pass tombstone_existing_from_snapshot_skips_backend_for_authoritative_tombstone, because that test only asserts lookup_count() == 0 and inserts().is_empty(). The regression the marker exists to prevent is exactly the one the harness is blind to.
Proposed fix (apply manually — spans four ranges across separate hunks, so it can't be a single suggestion):
// RecordedEcKvOperations
lists: std::sync::atomic::AtomicUsize,
// reset()
self.lists.store(0, std::sync::atomic::Ordering::Relaxed);
// accessor
fn list_count(&self) -> usize {
self.lists.load(std::sync::atomic::Ordering::Relaxed)
}
// RecordingEcKv::count_keys_with_prefix
self.operations
.lists
.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
self.inner.count_keys_with_prefix(prefix, limit)Then add assert_eq!(operations.list_count(), 0, "should not list for a tombstone") to the fast-path tests.
| /// write. An existence check that itself fails leaves the withdrawal | ||
| /// unresolved rather than silently dropped. | ||
| fn tombstone_unproven_missing(&self, ec_id: &str, missing: EcKvSnapshot) -> EcKvSnapshot { | ||
| match self.key_exists_confirmed(ec_id) { |
There was a problem hiding this comment.
♻️ refactor — two strongly-consistent list ops on the exact path this PR optimizes.
key_exists_confirmed is a list, and withdrawal_marker_exists is a second list. On the repeated-stale-miss path — the one the completion marker exists to make cheap — that's two primary-data-source reads before the function decides to do nothing. Checking the marker first short-circuits with one.
Outcomes are identical in every combination except one: when key_exists_confirmed itself errors and a marker exists, the current code returns Failed, whereas marker-first would return missing. Since the marker is only ever written after a successful root tombstone, treating it as authoritative is arguably the more correct answer — but it is a change in fail-closed posture, so flagging it rather than proposing it as a one-click patch.
Proposed fix (apply manually — the reorder changes behaviour in the key_exists_confirmed-errors case, so it deserves a deliberate call rather than a suggestion button):
fn tombstone_unproven_missing(&self, ec_id: &str, missing: EcKvSnapshot) -> EcKvSnapshot {
match self.withdrawal_marker_exists(ec_id) {
Ok(true) => {
log::debug!(
"withdrawal tombstone for '{}': completion marker already exists",
log_id(ec_id)
);
return missing;
}
Ok(false) => {}
Err(err) => {
// Marker failure must not weaken withdrawal.
log::warn!(
"withdrawal completion marker lookup failed for '{}': {err:?}",
log_id(ec_id)
);
}
}
match self.key_exists_confirmed(ec_id) {
Ok(false) => missing,
Ok(true) => { /* unconditional privacy write, unchanged */ }
Err(err) => { /* Failed, unchanged */ }
}
}| EcKvWriteMode::IfGenerationMatch(generation), | ||
| ) { | ||
| Ok(EcKvWriteOutcome::Written) => { | ||
| self.record_withdrawal_completion(ec_id); |
There was a problem hiding this comment.
🤔 thinking — one extra in-path KV write per withdrawal.
record_withdrawal_completion adds a second synchronous KV write inside ec_finalize_response, which runs before the response is sent rather than post-send. docs/superpowers/specs/2026-03-24-ssc-technical-spec-design.md:549 budgets ≈ 25ms per tombstone write, so a withdrawal-carrying request roughly doubles its KV finalize cost.
Withdrawals are rare and the benefit (no TTL refresh, no repeat root writes) is real, so this is very likely the right trade — just confirming it's deliberate rather than incidental, since the marker write is only ever read by the stale-miss branch. If the added latency ever matters, the marker on this CAS-success path is the one that could plausibly move off the critical path.
|
|
||
| // Completion markers belong to withdrawn generations. Remove any | ||
| // marker before this key can become live again. | ||
| self.clear_withdrawal_marker(ec_id)?; |
There was a problem hiding this comment.
🤔 thinking — a marker list failure now fails create_or_revive outright.
The ? here means a transient count_keys_with_prefix failure turns a create that would otherwise have succeeded into an error. That's asymmetric with the rule stated everywhere else in this PR — "completion-marker failure never suppresses the privacy write" — and it's the one place where marker trouble degrades an unrelated operation.
Failing closed is defensible on the revive branch: if the stale marker can't be cleared, a later stale-miss withdrawal could be suppressed against a live row. But on the fast path this guards, the very next statement is an Add that only succeeds when no key exists at all — there is no tombstone to have left a marker behind, so blocking the create is pure downside.
Moving the clear below the fast-path Add (keeping the in-loop clear as-is) would preserve the safety property where it matters and drop one list op from every fresh create.
| } | ||
|
|
||
| let marker_key = Self::withdrawal_marker_key(ec_id); | ||
| match self.store.delete(&marker_key) { |
There was a problem hiding this comment.
🌱 seedling — the delete-failure recheck branch is untested.
MarkerFailingEcKv fails marker insert and count_keys_with_prefix, but its delete delegates straight to the inner store, so this Err(delete_err) => match self.withdrawal_marker_exists(...) recovery path never executes in the suite. It's the subtlest piece of the marker lifecycle (a failed delete that turns out to have been won by a concurrent request), and it's the one piece with no coverage.
Not for this PR necessarily — adding a delete_fails flag to MarkerFailingEcKv scoped to WITHDRAWAL_MARKER_PREFIX keys would cover both arms cheaply whenever you next touch this file.
| /// the browser path should log at `error` level and continue — cookie | ||
| /// deletion is the primary enforcement mechanism. | ||
| /// Returns [`TrustedServerError::KvStore`] when serialization or storage fails. | ||
| pub fn write_withdrawal_tombstone( |
There was a problem hiding this comment.
⛏ nitpick — write_withdrawal_tombstone is pub but has no production caller outside this file.
The rewritten doc says it is "reserved for an eventually consistent point read that misses a key still visible from the primary data source". pub(crate) makes that reservation enforceable rather than aspirational. The only callers are tombstone_unproven_missing below and two tests (ec/pull_sync.rs:1079, ec/admin.rs:1118), all inside trusted-server-core, and no Rust intra-doc link to it survives — this PR removed the one in delete's doc. Archival specs reference it in prose only, which the plan explicitly says may stay unchanged.
Verified in a scratch worktree at e89fb15: cargo fmt --check, all six clippy aliases, test-fastly / test-axum / test-cloudflare / test-spin, and the cross-adapter parity suite all pass with this applied.
| pub fn write_withdrawal_tombstone( | |
| pub(crate) fn write_withdrawal_tombstone( |
e89fb15 to
4e9a01e
Compare
3 participants
Summary
This PR is stacked on #900.
Changes
crates/trusted-server-core/src/ec/kv.rscrates/trusted-server-core/src/ec/finalize.rsdocs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.mdCloses
Closes #881
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve— not runcargo test-cloudflare,cargo test-spin, cross-adapter parity, and all Cloudflare/Spin native + WASM clippy targetsChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)