Make EC withdrawal tombstoning idempotent across request bursts - #901
ChristianPavilonis wants to merge 13 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
e89fb15 to
4e9a01e
Compare
1cc335b to
7b0fab3
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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
usebreaks the test build — see inline atcrates/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_revivemarker 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 atcrates/trusted-server-core/src/ec/kv.rs:1139write_withdrawal_tombstoneshould be narrowed topub(crate)— see Cross-cutting belowwithdrawal_marker_existsuses prefix matching where the codebase uses exact — see inline atcrates/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_revivemarker clearing is dead code; the live re-consent path never clears the marker — This PR clears withdrawal markers increate_or_revive(kv.rs:388andkv.rs:423) and indelete(kv.rs:1343). Neither method has a production caller. Both production re-consent/creation sites callcreate_if_absentinstead (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_absentcall 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_absentclear the marker (with a test on the real path), and (b) ifcreate_or_revivegenuinely has no callers, should it be removed rather than extended? Note also thatcreate_or_revivenow?-propagates a marker-list failure before the fast-pathAdd(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 markedStatus: 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 entiretrusted-server-corelib-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_tombstoneshould be narrowed —kv.rs:979is stillpub, but after this PR it has zero callers outside theecmodule, and its only production caller is internal (kv.rs:1084, insidetombstone_unproven_missing). Itsrecord_snapshot: impl FnOnceparameter and the 11-line "Propagating the result" rationale (kv.rs:960-970) exist specifically to discipline thefinalize.rscaller this PR deletes; the surviving internal caller now works around the callback with a mutable capture (kv.rs:1083-1086). Considerpub(crate), or folding it intotombstone_unproven_missingas a private helper and dropping the callback.TombstoneOutcome(kv.rs:141) is likewise crate-internal now —finalize.rsdropped 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 assertingcount_hash_prefix_keysis unchanged by a present marker would be cheap insurance. -
📌 Spec docs still describe a single-namespace key space —
docs/guide/edge-cookies.mdwas updated, butdocs/superpowers/specs/2026-03-24-ssc-technical-spec-design.md:577still states "KV key: Full EC ID in{64-char hex}.{6-char alphanumeric}format", and the store table at:571-572lists 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 -- --checkcargo clippy-fastly,clippy-axum,clippy-cloudflare,clippy-cloudflare-wasm,clippy-spin-native,clippy-spin-wasmcargo test-fastly— 2702 passed, 0 failedcargo 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).
850c8d0 to
91558be
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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 atcrates/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_failureis now tautological — see inline atcrates/trusted-server-core/src/ec/kv.rs:1978- Test-double sprawl: eleven
EcKvStoreimplementations 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_existsis a strongbuild_listpage, 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 thenwrite_withdrawal_tombstone→tombstone_held_identity→key_exists_confirmedagain (kv.rs:1020).The third is redundant with the second:
tombstone_unproven_missinghas 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 givingtombstone_held_identitya variant that skips the check when the caller has already confirmed existence, or by havingtombstone_unproven_missingwrite 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
EcKvStoreimplementations tokv.rs's test module (DisappearOnConflictEcKvatkv.rs:1403,RecordingEcKvatkv.rs:2343,MarkerFailingEcKvatkv.rs:2426), joining four already there and four more inkv_backend::test_support— eleven doubles for a single trait.Two pairs are near-duplicates worth merging:
RecordingEcKvis a strict superset ofCountingEcKv(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_lookupsatkv.rs:2354,MarkerFailingEcKv's counter atkv.rs:2428) alongside the existingStaleLookupEcKvinkv_backend.rs.MarkerFailingEcKvis also over-configured: five fields across three constructors producing three mutually exclusive configurations, including a tri-stateOption<bool>serving two tests.Roughly 1,555 added lines in
kv.rsagainst 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:577still 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 -- --checkcargo clippy-fastly,clippy-axum,clippy-cloudflare,clippy-cloudflare-wasm,clippy-spin-native,clippy-spin-wasm— all cleancargo test-fastly— 2712 passed, 0 failedcargo 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
6645bd2 to
8ac70a9
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Third review pass, against 8ac70a9d9. Both blocking findings from the previous pass are genuinely fixed, and I verified that by mutation rather than by reading the replies:
| Prior finding | Mutation applied | Result |
|---|---|---|
| Stale marker suppresses withdrawal on a live row | .any(|v| v > now) -> .any(|_| true) |
killed by stale_completion_marker_does_not_suppress_withdrawal_after_root_recreation |
clear_withdrawal_marker guard silently deletable |
removed the list-budget error block | killed by marker_list_saturation_blocks_revival_and_hard_delete_before_mutation |
| (probe) TTL bound is load-bearing | valid_until drops the TOMBSTONE_TTL addition |
killed by two independent tests |
The redesign fixes the problem at its root rather than narrowing the window: marker keys now carry valid_until = consent.updated + TOMBSTONE_TTL and are trusted only while that bound is in the future, so a marker cannot outlive the tombstone it describes. The regression test models the exact sequence I reported (root expiry, same-key recreation through the production create_if_absent, stale point-read miss). The new list_keys_with_prefix is strongly consistent, so no eventual-consistency problem is reintroduced. publisher.rs and ec/mod.rs are mechanical test-double updates, not scope creep.
One blocking finding remains, and it is a consequence of what the new design now depends on rather than anything this PR broke.
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 or span more than one contiguous range.
Blocking
🔧 wrench
current_timestamp()falls back to0, making every completion marker valid — see inline atcrates/trusted-server-core/src/ec/kv.rs:896
Non-blocking
♻️ refactor / 🤔 thinking / 📝 note / 📌 out of scope
- Three message-less assertions remain — see inline at
crates/trusted-server-core/src/ec/kv.rs:2483 - Marker keys now accumulate where they previously did not — see inline at
crates/trusted-server-core/src/ec/kv.rs:911 list_keys_with_prefixdoes not mapItemNotFoundto an empty result — see inline atcrates/trusted-server-adapter-fastly/src/ec_kv.rs:174count_keys_with_prefixis now a redundant defaulted wrapper — see inline atcrates/trusted-server-core/src/ec/kv_backend.rs:150- Two test names overstate the mechanism they exercise — see Cross-cutting below
- Spec docs still describe a single-namespace key space — see Cross-cutting below
Cross-cutting / body-level findings
-
📝 Two test names overstate the mechanism they exercise — Both carry real signal; the names are what mislead.
withdrawal_marker_existence_requires_an_exact_key(kv.rs:2131) seeds a"<valid_until>-longer"key and passes becauseparse::<u64>()fails on it, not because of prefix exactness. A lenient parse that split on-before parsing would still be caught, so the coverage is real, but a reader will take the name as proof of exact-key matching, which is not what the assertion establishes.tombstone_stale_miss_accepts_marker_when_root_check_fails(kv.rs:3412) asserts onlymatches!(outcome, Missing { .. })with no store read-back. It distinguishesMissingfrom theFailedthat a root-check failure would otherwise produce, which is one genuine bit. But the marker short-circuit its name advertises is actually pinned bytombstone_existing_from_repeated_stale_miss_preserves_first_write(kv.rs:3202), not by this test.Renaming both to describe what they actually assert would keep the next reader from over-trusting them.
-
📌 Spec docs still describe a single-namespace key space —
docs/superpowers/specs/2026-03-24-ssc-technical-spec-design.md:577still states "KV key: Full EC ID in{64-char hex}.{6-char alphanumeric}format". With this revision the store holds a second namespace whose keys are now{prefix}:{ec_id}:{valid_until}— three segments rather than one. Carried from both prior passes; the plan explicitly defers historical specs, so this stays a flag rather than a blocker.
Verification performed
Full CLAUDE.md CI gate re-run locally in an isolated worktree at 8ac70a9d9, independent of GitHub's checks:
cargo fmt --all -- --checkcargo clippy-fastly,clippy-axum,clippy-cloudflare,clippy-cloudflare-wasm,clippy-spin-native,clippy-spin-wasm— all cleancargo test-fastly— 2716 passed, 0 failedcargo test-axum,cargo test-cloudflare,cargo test-spin— all pass- cross-adapter parity — 13 passed
Plus the three mutation experiments in the table above, each applied and reverted individually against a clean tree.
CI Status
All 20 checks pass:
- 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
- CLAUDE.md symlink guard: PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- CodeQL: PASS
…rver into fix/idempotent-ec-withdrawal-tombstones
An unavailable clock must not make expired completion markers look valid.\nKeep marker validation fallible and retain the strong root-existence gate.\n\nCover persisted withdrawal, missing roots, and expiry boundaries; clarify\nthe bounded-list contract and record review follow-up verification.
3 participants
Summary
This PR is stacked on #900.
Changes
crates/trusted-server-core/src/ec/kv.rscrates/trusted-server-core/src/ec/finalize.rscrates/trusted-server-core/src/ec/mod.rscrates/trusted-server-core/src/ec/kv_backend.rscrates/trusted-server-adapter-fastly/src/ec_kv.rscrates/trusted-server-core/src/publisher.rsdocs/superpowers/specs/2026-03-24-ssc-technical-spec-design.mddocs/guide/edge-cookies.mddocs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.mdCloses
Closes #881
Test plan
cargo fmt --all -- --checkcargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spin./scripts/test-cli.shcargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test paritycd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd crates/trusted-server-js/lib && node build-all.mjscd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve— not runChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)Latest review follow-up
Commit
f22dc56afixes clock-failure marker validation, adds deterministic withdrawal and expiry-boundary regressions, supplies the three missing assertion messages, and clarifies the strong bounded-list contract. The malformed-expiry test now describes suffix validation; the valid-marker test asserts one point read, one marker list, zero root checks, and zero writes. The historical spec now records the second key namespace.The clock regression fails before the fix and when mutated back to the unsafe zero fallback. Independent correctness review found no defects. All 18 requested local gates passed: 3,208 Rust tests passed with 10 existing tests ignored, and 959 JS tests passed. These are local results, not a claim about the new GitHub CI run. Clock failure is injected at the private withdrawal decision, not through an actual failed host clock.
Deferred deliberately: moving expiry into the marker value requires a strong value-read contract; Fastly list
ItemNotFoundnormalization requires an operation-specific platform contract or production observation. Neither behavior was changed speculatively.