Harden the Edge Cookie withdrawal write path by prk-Jr · Pull Request #1113 · IABTechLab/trusted-server

prk-Jr · 2026-09-02T07:16:08Z

Summary

Consent withdrawal now writes a tombstone only for an identity held by this deployment. Unknown client-selected identifiers no longer create rows. The browser cookie is expired even when the store check or write fails.

Existence uses a strongly consistent exact-key check. An eventually consistent miss can hide a newly issued identity that has already been disclosed to a partner; skipping its withdrawal would let later batch sync keep that identity live. Cookie deletion alone does not close that server-side exposure.

Implementation

EcKvStore::key_exists requires strong consistency and exact equality. The Fastly adapter executes individual list pages, follows their cursors, and compares complete keys.
Listing is bounded to four pages of 100 keys. ItemNotFound terminates immediately. Store errors and an exhausted page budget return an error without an eventual-lookup fallback or a blind write.
Manual pagination avoids the Fastly SDK iterator path that can reissue a failed page.
write_withdrawal_tombstone returns Written or UnknownIdentity; an inconclusive check remains an error. Errors are logged at the request boundary, while expected unknown identities log at debug.
The same call takes the post-withdrawal snapshot write-back as a parameter, and the graph builds that snapshot from the entry it actually wrote. Every path out of the method, the error path included, hands the caller the state it must now hold.
Identifier logging truncates by character and keeps complete identifiers out of locally constructed error messages.

The existence check and tombstone write remain non-atomic. An entry that expires between them can briefly be restored as a tombstone. The unconditional write preserves withdrawal across concurrent entry updates.

Removing main's CAS tombstone path

The merge drops tombstone_existing_from_snapshot, its MAX_CAS_RETRIES loop, the DisappearOnConflictEcKv double, and six tests, replacing them with the strong existence check followed by an unconditional overwrite. This is a deliberate trade, not an artefact of the merge.

Withdrawal must always win over a concurrent write. Main's CAS loop could lose that race outright — tombstone_existing_from_snapshot_returns_failed_after_cas_exhaustion asserts exactly that losing case, where the row stays live with consent granted. The cost is that a row deleted between the check and the write is recreated as a tombstone. That tombstone denies consent, expires on TOMBSTONE_TTL, and cannot mint an identity, so it fails safe; the CAS loop's failure mode does not.

`key_exists_confirmed` also gates orphan recovery

This helper has a second caller outside the withdrawal path: confirm_then_recover_orphaned_ec in finalize.rs. Moving it from a prefix count to an exact match changes behaviour there too, and the change is wanted. Under the prefix count, a longer key sharing the orphaned ID as a prefix reported Ok(true) and suppressed a legitimate identity rotation — the same prefix collision this PR closes on the withdrawal path.

Validation

Regression coverage includes a lagging ordinary lookup with a successful strong check and subsequent batch-sync rejection; exact versus prefix matches; later-page and last-allowed-page matches; immediate error termination; page-budget exhaustion; and no ordinary lookup on the withdrawal path. Orphan recovery is covered against a prefix-neighbour key: with only a longer key seeded the orphan is proven absent, so recovery runs and the identity rotates. The Fastly backend also has local Viceroy coverage for real KV operations.

Full local validation: formatting, all six adapter clippy targets, Fastly/Axum/Cloudflare/Spin tests, cross-adapter parity, JS build/tests/format, and docs format.

Integration considerations

This changes the withdrawal API and touches the finalize path shared with #901 and the provider stack (#1043–#1047, #1084, #1094). Preserve strong existence checking and error handling when reconciling that work; provider-specific identifier validation belongs at the appropriate caller boundary.

Dropping the snapshot write-back during that reconciliation would let post-send pull sync disclose a just-withdrawn identity to partners, so it is a build failure rather than a silent regression: both stale call shapes now fail to compile against the new signature. #901 is the one to watch — its write_withdrawal_tombstone returns Result<(), _> with no existence gate at all, so landing it over this branch would revert the fix outright rather than only lose the write-back.

Closes #1116

Tombstone only an identity the graph already holds. The marker exists to stop later reads of a real row, so writing one for an identifier that was never issued enforces nothing while still consuming a write and a row, and the identifier arrives in a client-supplied cookie. Confirm existence with the list API rather than a lookup. A lookup is eventually consistent, so a stale miss would discard a genuine withdrawal; the list is strongly consistent. Reject anything that is not a well-formed EC ID before querying, since this is a prefix query and an empty or truncated value would match unrelated keys. When the list cannot answer, re-check with a lookup instead of writing regardless. Eventual consistency yields false negatives, never false positives, so a hit is proof the identity exists while no fabricated identifier can produce one. If neither can answer, report the withdrawal unconfirmed and write nothing; the browser cookie is expired either way and remains the primary enforcement. Split the unusable-consent branch out of ec_finalize_response and route the per-identity result through one place, so an unconfirmed identity is logged as a fault while an unknown one is not.

The degraded path dropped both the list and lookup errors, leaving a store outage undiagnosable. Log both, and use the raw lookup so a corrupt-but-present row is not read as absent. Add a test pinning the fixed-width assumption the prefix check relies on.

Counting keys by prefix reported a held identity whenever any longer key started with the one asked for, so a withdrawal for an identity that was never issued still wrote a row. Add an exact, strongly consistent `key_exists` to the store and use it. This also drops the dependency on the identifier grammar: the check no longer cares what shape an identifier takes, only whether that key is present. Redact the key in the Fastly lookup error, matching the list error. Carry the reason for an unconfirmed withdrawal so it is logged once.

Scanning one page of prefix matches assumed the exact key would be in it. Nothing guarantees that when other keys share the prefix, and stopping early reports a held identity as missing, discarding its withdrawal. Iterate the pages instead. Also correct two test comments that still described the removed grammar gate.

A byte index landing inside a multi-byte character makes `get` return `None`, and the fallback printed the whole identifier — the opposite of what the redaction is for. Truncate by character, and reuse the helper in the Fastly store rather than repeating the byte form there. Prove the bounds check avoids a store call with a counting backend, and rename the test that claimed to cover a grammar gate that no longer exists.

jwrosewell · 2026-09-02T07:33:12Z

Sequencing note on this PR and the provider stack.

This PR and the open provider stack (#1043 to #1047, #1084, #1094) rework the same Edge Cookie finalize flow. A merge simulation of this PR's head (f6181d1, and identically its earlier head b5b68bb) against six of the seven stack heads conflicts in one file, crates/trusted-server-core/src/ec/finalize.rs, in four regions, being two import collisions and two larger blocks of roughly 40 and 90 lines where this PR's withdrawal-path rework and the stack's permission-gate restructuring rewrite the same code. The spec-only #1084 merges clean, and every other file in this PR auto-merges.

Reproduce:

git fetch upstream main refs/pull/1113/head:pr-1113
git merge-tree --write-tree --name-only pr-1113 <stack head>

The request is the one we have made on #885, #940 and #1094. The stack has been open since 19 August, carrying work that has been under review since 2 July as #838, is green on required CI, and its branches are kept rebased close to main, so please land the stack first, or say here that this PR goes first so we rebase once against a known base. The tombstone tightening here sits directly on the finalize path the stack restructures, so sequencing the two deliberately keeps both reviews readable.

The exact-key scan lived in the Fastly store, where no test double can exercise paging. Extract it as `contains_exact_key` and have the backend supply pages to it, so multi-page matches, prefix-only keys, early exit and page errors are all covered natively.

prk-Jr · 2026-09-02T07:59:38Z

Closes #1116

A third `Ok` variant was discarded by any caller inspecting only the error case, dropping a possibly-unrecorded withdrawal in silence. Returning `Err` keeps the underlying reports intact instead of flattening them into a string. Finish the redaction pass — insert, delete, and the deserialize paths still embedded the raw key — treat an empty prefix listing as absent rather than a failure, bound the pages an existence check will walk, and put the store trait's doc comment back on the trait.

The exact-key check followed a listing for a bounded number of pages and read running out of budget as absence. Absence is what tells the withdrawal path the identity was never issued, so a real identity on an unread page had its tombstone silently dropped — while the constant's own note and the tombstone docs both said the caller would treat that case as unconfirmed. Report it as a third outcome and map it to an error at the adapter, which puts it on the path that already re-checks by lookup. The page budget moves into the checked function so the listing is passed untruncated and there is no count to keep in agreement at the call site.

Nine messages interpolated the whole identifier: a duplicate create, upserts naming a missing or withdrawn key, and the CAS-exhaustion paths. Callers log these reports with debug formatting, so each one put a full identifier in a log line. The existing test only drove an injected backend failure, which never reaches them, so the module's claim that every message goes through the truncating helper held for the wrong reason. Route them through it too, and cover the paths a request can actually reach.

The test drove three of the message paths, so the other five held only by inspection. Extend it to the batched upserts and the three CAS-exhaustion terminal errors, which needed a conflict-injecting store that can hold a live entry rather than only a tombstone.

The conditional partner upsert's CAS-exhaustion error used the redacted template but no test executed it, so it was the one message still holding by inspection alone.

aram356

Summary

Gating the withdrawal tombstone on an existence check is the right call, and the premise holds up: handle_batch_sync maps both UpsertResult::NotFound and UpsertResult::ConsentWithdrawn to the same REASON_INELIGIBLE (crates/trusted-server-core/src/ec/batch_sync.rs:211), so skipping the write for a row that does not exist costs no enforcement. The identifier-redaction sweep is thorough, and the negative-case tests are well chosen.

The objection is to the mechanism rather than the goal. Choosing a prefix list over the lookup already on the trait introduces a reachable defect in the only production backend, doubles the happy-path round trips on the withdrawal response path, and pulls in the paging machinery, the third Undetermined state, the new trait method, and the expanded public surface that come with it. Switching the check to lookup_raw resolves the first finding and deletes the rest.

Verified locally against 7b5ea5720: cargo clippy-fastly clean, cargo fmt --all -- --check clean, cargo test -p trusted-server-core --target wasm32-wasip1 2280 passed, cross-adapter parity 13 passed.

1 of the inline comments below carries a one-click GitHub suggestion — use Commit suggestion to apply it. The remaining comments describe the fix in prose because the change spans multiple files or lines outside the diff and cannot be auto-applied.

Blocking

🔧 wrench

ItemNotFound mapped to an empty page re-issues the list instead of ending it — see inline at crates/trusted-server-adapter-fastly/src/ec_kv.rs:160
lookup answers existence in one round trip, and the strong-consistency rationale is not maintained end to end — see inline at crates/trusted-server-core/src/ec/kv.rs:666
The only production EcKvStore implementation has no test coverage — see Cross-cutting below

Non-blocking

🤔 thinking / ♻️ refactor / 📝 note

MAX_EC_ID_LEN is unreachable from the production call graph — see inline at crates/trusted-server-core/src/ec/kv.rs:131
warn for a successful, expected fallback inverts this PR's own level convention — see inline at crates/trusted-server-core/src/ec/kv.rs:735
Redaction verified clean at every reachable construction site — see inline at crates/trusted-server-core/src/ec/kv.rs:746

Cross-cutting / body-level findings

🔧 The only production EcKvStore implementation has no test coverage. crates/trusted-server-adapter-fastly/src/ec_kv.rs has no #[cfg(test)] mod tests, and FastlyEcKvStore is the sole non-test implementor of the trait — crates/trusted-server-adapter-{axum,cloudflare,spin} contain no EcKvStore implementation at all, and the only production construction sites are crates/trusted-server-adapter-fastly/src/main.rs:445 and :477.

The PR notes the pagination loop as a known gap, but the gap is wider than pagination. contains_exact_key is a pure function over an already-materialized page iterator and carries 8 tests; the untested code is the adapter glue that builds that iterator, decides what an Err page means, and converts Undetermined into a Report — which is exactly where the first finding above lives. A test double implementing the Iterator<Item = Result<ListPage, KVStoreError>> shape and asserting the round-trip count would have caught it without a live Fastly store.

CI Status

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

No failing checks. The three pending gates were still in progress at review time.

The prefix listing was chosen for its strong consistency, but that guarantee was never held end to end: when the listing could not answer, the fallback consulted `lookup` and treated `Ok(None)` as grounds for not writing, so the strong read was abandoned precisely when the store was degraded. It also carried a defect in the only production backend. `ListResponse::next` in fastly-0.12.1 declares `iterator_did_error` but never assigns it, and its error arm returns `Some(Err(..))` before the `keys.is_empty()` check that ends iteration, so a page error does not close the iterator — the next call re-issues the list with the same cursor. Mapping `ItemNotFound` to an empty page therefore did not mean "absent"; it meant "ask again", until the page budget tripped. That arm was reachable, because `KvSysError::NotFound` maps to `KVStoreError::ItemNotFound` in the `From` impl every KV operation shares. `lookup` is exact by construction and answers in one round trip, so it needs no page budget, no third `Undetermined` state, no trait method, and no bound on the identifier's length — nothing is scanned. A withdrawal for a held identity now costs one read and one write instead of two round trips, on the slower of the two read paths. The residual risk is stated in the doc comment rather than hidden: an identity issued and withdrawn inside the replication lag is reported as unknown and gets no tombstone. The browser cookie is expired unconditionally either way, which is the primary enforcement, so the exposure is the batch-sync window.

`FastlyEcKvStore` is the only non-test implementor of `EcKvStore` and had no tests at all, so the adapter glue — what an error from the platform means, which failures are control flow and which are faults — held only by inspection. It needs no live service to exercise: `fastly.toml` already declares `ec_identity_store` for the local simulator, and the Fastly adapter's tests run under Viceroy, so the backend can be driven against a real KV store. Cover the unlinked store, a missing key reading as absent rather than as a failure, an insert/lookup/delete round trip, both precondition modes, and prefix counting.

prk-Jr · 2026-09-04T08:17:29Z

Addressed all five findings. Two commits: 0031fc3 swaps the check, cee87c2 adds the adapter coverage. Net -186 lines.

Blocking

ItemNotFound re-issues the list — confirmed against fastly-0.12.1 before acting: iterator_did_error is declared but never assigned, and the error arm returns above the keys.is_empty() termination check, so a page error leaves the iterator open on the same cursor. Resolved by the durable fix rather than by terminating the iteration.
Use lookup — adopted. key_exists_confirmed is now Ok(self.lookup_raw(ec_id)?.is_some()). That deleted contains_exact_key, ExactKeyMatch, both EXACT_MATCH_* constants, the Undetermined state, EcKvStore::key_exists and its five test doubles, the pub widening in kv_backend.rs, and the fallback block that hosted the :735 and :746 comments. The residual replication-lag exposure you named is documented in the doc comment.
No adapter test coverage — fastly.toml already declares ec_identity_store for the local simulator and the adapter tests run under Viceroy, so FastlyEcKvStore is testable against a real KV store with no live service. Six tests added: unlinked store, missing key reading absent rather than failing, insert/lookup/delete round trip, Add precondition, generation mismatch, prefix counting.

Non-blocking

MAX_EC_ID_LEN — dropped with its branch-only test. Verified your call-graph trace first (private fn, one production call site through is_valid_ec_id, fixed 71 bytes).
warn level and the interpolated error string — both lines removed with the fallback block. The redaction invariant point survives its line; noted as a follow-up rather than bolted on here.

Also added a round-trip budget test pinning one read per withdrawal, so a future change cannot quietly restore a two-read gate.

Verification: cargo fmt --all -- --check clean; all six clippy targets clean; cargo test-fastly 172 + 2271 passed (adapter 166 -> 172 from the new tests, core 2280 -> 2271 as the removed tests exceed the one added); axum, cloudflare, spin suites pass; cross-adapter parity 13 passed.

ChristianPavilonis

Summary

The write gate correctly prevents arbitrary client-selected identities from creating tombstones, and the redaction and local outcome handling look sound. I found one blocking correctness issue in the existence decision; details are inline.

ChristianPavilonis

Review summary

Reviewed 12f5f1e8e7102c8e2fe633e6907ab8d178db4860 against 640e93d1389a0fcf1ea6dab1e2d8ea026c5f2045.

No actionable regressions found in the seven changed files. Traced the bounded, strongly consistent exact-key check through withdrawal, orphan recovery, request snapshots, and downstream identity consumers. The earlier lookup-lag and pagination findings are addressed.

Safety proof

Executed regression tests confirm that a lagging ordinary lookup does not discard withdrawal of a strongly confirmed identity, unknown keys do not create tombstones, and subsequent batch sync rejects the withdrawn identity.
Executed finalization tests confirm that a failed existence check still expires the browser cookie, removes the EC response header, and invalidates the live snapshot.
Exact-head CI logs confirm all 13 Fastly KV tests passed, including explicit cursor traversal, immediate error termination, page-budget exhaustion, and real simulator KV operations. The pinned Fastly SDK defaults list requests to strong consistency.

Validation

cargo test -p trusted-server-core --locked ec::: 321 passed.
cargo test -p trusted-server-core --locked --lib --quiet: 2,367 passed.
cargo clippy -p trusted-server-core --locked --all-targets --all-features -- -D warnings: passed.
cargo fmt --all -- --check: passed.
cargo test-fastly --locked ec::: blocked locally before tests by missing wasm32-wasip1; Viceroy is also unavailable locally. Inspected exact-head CI logs confirming Fastly KV tests and the EC lifecycle integration test passed.
All reported PR checks passed. Reviewed existing reviews, comments, replies, and all six resolved threads; no duplicate findings.

Residual risk

Live Fastly replication and concurrent deletion were not reproduced. The existence check and tombstone write remain non-atomic. An inconclusive check intentionally leaves withdrawal unrecorded server-side while expiring the browser cookie.

aram356

Summary

Gates the Edge Cookie withdrawal tombstone on an exact, strongly consistent existence check, so a client-supplied ts-ec cookie can no longer mint a row for an identity the graph never held. The approach is sound and the new Viceroy-backed backend tests close the coverage gap the previous revision listed as unclosable. Verified locally at 12f5f1e8e: 321 core EC tests, 13 new Fastly backend tests, cargo fmt --check, and cargo clippy-fastly all pass, and all 19 GitHub checks are green.

Requesting changes on three points: this merge silently removes main's CAS-based tombstone path, key_exists_confirmed changed semantics for a second caller outside this PR's stated scope, and the interaction with #1043 is a compile-silent hazard that would drop the new snapshot write-back.

2 of the inline comments below carry a one-click GitHub suggestion. The remaining comments describe the fix in prose because the finding spans a deletion, multiple files, or is cross-cutting and cannot be auto-applied.

Blocking

🔧 wrench

Merge deletes main's CAS tombstone path; unconditional overwrite can resurrect a deleted row — see "Cross-cutting" below
key_exists_confirmed silently changed semantics for a second, unrelated caller — see "Cross-cutting" below

❓ question

Cross-PR reconciliation with #1043 is a compile-silent hazard — see "Cross-cutting" below

Non-blocking

🤔 thinking / ♻️ refactor

Worst-case 8 strong list round-trips per withdrawal on the response path — see inline at crates/trusted-server-adapter-fastly/src/ec_kv.rs:16
The strong-consistency guarantee is Fastly-only and unenforced — see inline at crates/trusted-server-core/src/ec/kv_backend.rs:96
Stray trailing comma in single-argument format! — see inline at crates/trusted-server-adapter-fastly/src/ec_kv.rs:103 and :176

Cross-cutting / body-level findings

🔧 Merge deletes main's CAS tombstone path; unconditional overwrite can resurrect a deleted row. This merge removes tombstone_existing_from_snapshot, its MAX_CAS_RETRIES loop, the DisappearOnConflictEcKv double, and six tests that existed on main (git show origin/main:crates/trusted-server-core/src/ec/kv.rs, around lines 959 and 2182-2300), replacing all of it with a check followed by an unconditional EcKvWriteMode::Overwrite.

I confirmed the behavioural consequence by running it rather than reasoning about it. With a store double that deletes the row immediately after the existence check returns true, the withdrawal path reports Written and the row exists afterwards — a row that no longer existed is recreated:
```
PROBE1 outcome=Written row_exists_after=true
```
I do think the trade-off is defensible, and I am not asking you to restore the CAS loop. A resurrected tombstone denies consent and expires on TOMBSTONE_TTL, so it fails safe, and the unconditional write means a withdrawal can no longer lose a CAS race the way main could — tombstone_existing_from_snapshot_returns_failed_after_cas_exhaustion on main asserts exactly that losing case, where the row stayed live with consent granted.

What blocks is that none of this appears in the PR description or the commit messages. A reviewer reading the diff sees ~130 lines of tested concurrency machinery disappear inside a merge commit with no statement of intent. Please state the removal and its rationale explicitly (the "withdrawal must always win over a concurrent write" argument is the right one), so the trade-off is a recorded decision rather than an artefact of the merge.
🔧 key_exists_confirmed silently changed semantics for a second, unrelated caller (crates/trusted-server-core/src/ec/finalize.rs:223, unchanged context so it carries no inline anchor).

On main this helper was prefix-based:
```
pub fn key_exists_confirmed(&self, ec_id: &str) -> Result<bool, Report<TrustedServerError>> {
    Ok(self.store.count_keys_with_prefix(ec_id, 1)? > 0)
}
```
This PR reimplements it as an exact match against strongly consistent state. That is correct and is the point of the change — but this call site, the orphan-recovery path, is not part of the PR's stated scope and its behaviour changes as a side effect.

The change is an improvement here too: previously a longer key sharing this ID as a prefix would report Ok(true) and suppress a legitimate identity rotation, which is the same prefix-collision bug the PR fixes on the withdrawal path. So I am not asking you to revert it — I am asking that it be deliberate and covered.

Please add a test asserting orphan recovery behaviour when only a longer key exists under the same prefix: with the exact check, key_exists_confirmed returns false and recover_orphaned_ec should run, where the old prefix logic would have taken the Ok(true) branch and skipped recovery. Mentioning the second caller in the PR description would also help, since the body currently describes this as a withdrawal-path change only.
❓ Cross-PR reconciliation with #1043 is a compile-silent hazard. Your reviewer note flags this; I verified it against #1043's actual diff and it is worse than the note implies. #1043 keeps this shape at the shared call site:
```
if let Err(err) = graph.write_withdrawal_tombstone(kv_key) { ... }
```
That still compiles against the new Result<TombstoneOutcome, _>. It discards UnknownIdentity, which is benign, but it also drops the set_kv_snapshot write-back this PR adds at finalize.rs:316-331. That write-back is what propagates the tombstone into EcContext so dispatch_pull_sync cancels — pull_sync.rs runs post-send and discloses the raw ec_id to partners, and dispatch_pull_sync_skips_dispatch_when_ec_is_tombstoned_after_snapshot_capture is the test covering exactly that. A merge resolved toward #1043's side would therefore reintroduce partner disclosure of a just-withdrawn identity, with no compile error and no failing test in this PR.

Two questions: what is the intended landing order across #901, #1043, and this PR; and can the outcome be made structurally impossible to discard — #[must_use] on TombstoneOutcome plus having write_withdrawal_tombstone take the context write-back as a closure, or returning a type the call site cannot ignore — so this converts into a compile error instead of a silent behaviour loss?

CI Status

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

Writing the tombstone only half-enforces a withdrawal. Post-send work in the same request reads the in-request snapshot rather than taking a fresh read, and pull sync discloses the raw EC ID to partners from it, so a tombstone that never reaches that snapshot still leaks the identity it just withdrew. Rebuilding the snapshot was previously the caller's job, left to a separate statement after the call. A caller that inspected only the error case still compiled and silently dropped the write-back. Take the write-back as a parameter instead, and build the snapshot in the graph from the entry actually written. Every path out of the method, the error path included, hands back the state the caller must now hold, so a caller that drops it no longer compiles. Mark TombstoneOutcome must_use for the same reason; that already caught two tests discarding the outcome after expect, which now assert it.

key_exists_confirmed gates orphan recovery as well as withdrawal, and moving it from a prefix count to an exact match changed behaviour at that second call site too. The change is an improvement: a longer key sharing the orphaned ID as a prefix previously reported the orphan as still held and suppressed a legitimate rotation, the same collision this branch closes on the withdrawal path. Assert it. Seeding only a neighbouring longer key leaves the orphan proven absent, so recovery runs and the identity rotates, while the neighbour is left untouched. Reverting the helper to the prefix count fails the rotation assertion on its own, independently of the existence pre-assert.

The strong-consistency requirement is a contract the signature cannot express. FastlyEcKvStore is currently the only production implementor, so the guarantee holds today, but the next backend is the risk: a Workers KV list is eventually consistent, so the natural implementation would satisfy the type and silently violate the contract, dropping the withdrawal of a recently issued identity. Say so in the trait doc, along with the consequence and the instruction to return an error rather than a false the caller will trust. Note too that the in-memory double is trivially strong, so the core tests cannot catch a backend that breaks this.

Leftovers from the multi-argument form these format! calls replaced.

prk-Jr · 2026-09-08T10:11:37Z

Thanks for the review — the three body-level findings are addressed in c4d569d (four commits on top of a4c61b5).

Cross-PR reconciliation with #1043 — now a compile error

Made structural rather than documented, along the lines you suggested. write_withdrawal_tombstone takes the write-back as a parameter:

pub fn write_withdrawal_tombstone(
    &self,
    ec_id: &str,
    record_snapshot: impl FnOnce(EcKvSnapshot),
) -> Result<TombstoneOutcome, Report<TrustedServerError>>

The graph now builds the snapshot itself, from the entry it actually wrote, and calls record_snapshot on every path out of the method including the error path. Verified by reproducing #1043's exact call shape in a scratch tree:

error[E0061]: this method takes 2 arguments but 1 argument was supplied
   --> crates/trusted-server-core/src/ec/finalize.rs:321:37
    |
321 |             if let Err(err) = graph.write_withdrawal_tombstone(ec_id) {
    |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^------- argument #2 is missing

TombstoneOutcome is also #[must_use] now. On its own that would not have caught #1043's shape — if let Err(..) counts as a use of the Result — but it did catch two existing tests discarding the outcome after expect, which now assert Written.

A side effect worth flagging: #901 is caught too, and it is the more dangerous of the two. Its write_withdrawal_tombstone returns Result<(), _> with no existence gate at all — the pre-fix shape — so landing it over this branch would revert the gate entirely, not just drop the write-back. It now fails to compile against both the signature and the call site.

On landing order: I have not fixed one, and it is worth deciding explicitly rather than by merge accident. The relevant facts are that #1043 bases on main and renamed withdrawal_ec_ids to withdrawal_kv_keys, so that hunk conflicts textually regardless and a human resolves it either way — the compile error is the backstop if it is resolved toward #1043's side. #901 stacks on fix/no-op-kv-reads rather than main. Since both stale shapes now fail to build against this one, no ordering silently loses the write-back; happy to sequence them however you prefer.

`key_exists_confirmed`'s second caller — covered

Agreed on all counts, including that the change is an improvement at that call site and should not be reverted. Added finalize_rotates_when_only_a_longer_key_shares_the_orphan_prefix (e99a127): seeding only a longer neighbouring key leaves the orphan proven absent, so recover_orphaned_ec runs and the identity rotates, while the neighbour is left untouched.

I checked that the test earns its keep rather than just passing. Reverting the helper to count_keys_with_prefix fails the rotation assertion on its own, with the existence pre-assert stripped out:

assertion `left != right` failed: a proven-absent orphan must rotate even with a prefix neighbour present

CAS tombstone path removal

Your reading of the trade-off is the one I would defend, and you are right that it is currently invisible. Recording it here, and I will add it to the PR description.

The merge drops tombstone_existing_from_snapshot, its MAX_CAS_RETRIES loop, the DisappearOnConflictEcKv double and six tests, replacing them with a strong existence check followed by an unconditional overwrite. The trade is deliberate: withdrawal must always win over a concurrent write, and main's CAS loop could lose that race outright — tombstone_existing_from_snapshot_returns_failed_after_cas_exhaustion asserts exactly that losing case, leaving a live row with consent granted. The cost is the resurrection you reproduced: a row deleted between check and write comes back as a tombstone. That denies consent, expires on TOMBSTONE_TTL, and cannot mint an identity, so it fails safe.

Validation

cargo fmt, all eight clippy targets, test-fastly under Viceroy (all 13 ec_kv::tests, including cursor traversal and page-budget exhaustion), test-axum, test-cloudflare, test-spin, cross-adapter parity, and 2,467 core lib tests — all passing. Each of the four commits was checked independently for fmt and --all-targets build, so no intermediate state is broken.

prk-Jr added 5 commits September 1, 2026 20:36

prk-Jr self-assigned this Sep 2, 2026

prk-Jr and others added 2 commits September 2, 2026 13:15

Merge branch 'main' into fix/ec-withdrawal-write-gate

6ebd8c3

prk-Jr marked this pull request as draft September 2, 2026 08:11

aram356 added this to the 202609 milestone Sep 2, 2026

prk-Jr added 4 commits September 2, 2026 20:56

Reach the last redacted message from the test

b36eb1e

The conditional partner upsert's CAS-exhaustion error used the redacted template but no test executed it, so it was the one message still holding by inspection alone.

prk-Jr marked this pull request as ready for review September 3, 2026 06:01

prk-Jr requested review from ChristianPavilonis and aram356 September 3, 2026 06:03

Merge branch 'main' into fix/ec-withdrawal-write-gate

7b5ea57

aram356 requested changes Sep 4, 2026

View reviewed changes

prk-Jr added 2 commits September 4, 2026 13:45

ChristianPavilonis requested changes Sep 4, 2026

View reviewed changes

Comment thread crates/trusted-server-core/src/ec/kv.rs Outdated

Confirm withdrawals against strong KV state

eea0f6b

prk-Jr requested review from ChristianPavilonis and aram356 September 7, 2026 07:26

Merge main and reconcile Edge Cookie withdrawal safeguards

12f5f1e

ChristianPavilonis approved these changes Sep 7, 2026

View reviewed changes

aram356 requested changes Sep 8, 2026

View reviewed changes

Comment thread crates/trusted-server-adapter-fastly/src/ec_kv.rs

Comment thread crates/trusted-server-core/src/ec/kv_backend.rs

Comment thread crates/trusted-server-adapter-fastly/src/ec_kv.rs Outdated

Comment thread crates/trusted-server-adapter-fastly/src/ec_kv.rs Outdated

prk-Jr and others added 5 commits September 8, 2026 14:53

Merge branch 'main' into fix/ec-withdrawal-write-gate

a4c61b5

Drop stray trailing commas in Fastly KV error messages

c4d569d

Leftovers from the multi-argument form these format! calls replaced.

prk-Jr requested a review from aram356 September 8, 2026 16:03

Navigation Menu

Uh oh!

Harden the Edge Cookie withdrawal write path - #1113

Harden the Edge Cookie withdrawal write path#1113
prk-Jr wants to merge 22 commits into
mainfrom
fix/ec-withdrawal-write-gate

prk-Jr commented Sep 2, 2026 •

edited

Loading

Uh oh!

jwrosewell commented Sep 2, 2026 •

edited

Loading

Uh oh!

prk-Jr commented Sep 2, 2026

Uh oh!

aram356 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prk-Jr commented Sep 4, 2026

Uh oh!

ChristianPavilonis left a comment

Uh oh!

Uh oh!

ChristianPavilonis left a comment

Uh oh!

aram356 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prk-Jr commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

prk-Jr commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation

Removing main's CAS tombstone path

key_exists_confirmed also gates orphan recovery

Validation

Integration considerations

Uh oh!

jwrosewell commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prk-Jr commented Sep 2, 2026

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench

Non-blocking

🤔 thinking / ♻️ refactor / 📝 note

Cross-cutting / body-level findings

CI Status

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prk-Jr commented Sep 4, 2026

Uh oh!

ChristianPavilonis left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

Uh oh!

ChristianPavilonis left a comment

Choose a reason for hiding this comment

Review summary

Safety proof

Validation

Residual risk

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench

❓ question

Non-blocking

🤔 thinking / ♻️ refactor

Cross-cutting / body-level findings

CI Status

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prk-Jr commented Sep 8, 2026

Cross-PR reconciliation with #1043 — now a compile error

key_exists_confirmed's second caller — covered

CAS tombstone path removal

Validation

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

prk-Jr commented Sep 2, 2026 •

edited

Loading

`key_exists_confirmed` also gates orphan recovery

jwrosewell commented Sep 2, 2026 •

edited

Loading

`key_exists_confirmed`'s second caller — covered