Skip to content

Complete removal of the legacy consent KV path - #903

Open
ChristianPavilonis wants to merge 2 commits into
perf/group-batch-sync-by-ec-idfrom
refactor/remove-legacy-consent-store
Open

ChristianPavilonis wants to merge 2 commits into
perf/group-batch-sync-by-ec-idfrom
refactor/remove-legacy-consent-store

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Remove the inactive legacy consent KV fallback/write path, storage module, source metadata, and Fastly route dependency.
  • Keep live consent request-local and preserve ec.ec_store as the sole KV-backed EC identity/withdrawal store.
  • Fail fast when stale TOML or JSON/app-config still contains consent_store, with explicit operator migration guidance.

This PR is stacked on #902.

Changes

Area Change
Consent core Remove KV fields/branches from ConsentPipelineInput, delete the dead storage module, and remove ConsentSource::KvStore
Configuration Remove ConsentConfig::consent_store and add strict TOML/runtime-JSON rejection tests
Fastly Stop opening an unused consent store for auction, page-bids, and publisher routes; remove the local fixture and named opener
Other adapters Correct generic platform-KV comments that still advertised consent persistence
Documentation Explain request-local consent authority, strict upgrade failure, binding cleanup, rollback retention, and why legacy records are not migrated into ec.ec_store

Migration note

Deployments that still configure [consent].consent_store must remove the field before upgrading. Strict config loading intentionally rejects it. The old store may be retained unchanged for a rollback window, then unlinked and deleted; its records must not be copied into ec.ec_store.

Before this change, Fastly returned 503 on auction, page-bids, and publisher routes when a configured legacy store could not be opened, even though consent code never read that store. After this removal, a stale consent_store field instead prevents normal startup, and Fastly's startup-error router returns 500 on registered user routes until the configuration is corrected.

Closes

Closes #883

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve — not run
  • Other: strict TOML/JSON migration tests, consent and Fastly dispatch tests, cargo test-cloudflare, cargo test-spin, parity, ./scripts/test-cli.sh, all Cloudflare/Spin clippy targets, and all-features core rustdoc

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

@ChristianPavilonis ChristianPavilonis self-assigned this Jul 13, 2026
@ChristianPavilonis
ChristianPavilonis force-pushed the refactor/remove-legacy-consent-store branch from 8a53ee1 to 4bd5bcd Compare September 2, 2026 19:16
@ChristianPavilonis ChristianPavilonis added this to the 202609 milestone Sep 3, 2026
@ChristianPavilonis
ChristianPavilonis marked this pull request as ready for review September 3, 2026 17:05
@aram356

aram356 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

@ChristianPavilonis please assign issue for this PR

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Removes the legacy consent KV persistence path — config field, dead storage module, ConsentSource::KvStore, the Fastly consent-route store opener, and the local fixture — and replaces it with strict fail-fast config rejection plus operator migration guidance. I verified the central claim independently: at the base commit the sole production ConsentPipelineInput call site (crates/trusted-server-core/src/ec/mod.rs:298) already passed ec_id: None, kv_store: None, every Some(...) was a test, and delete_consent_from_kv had no callers at all. The path really was dead, so this is a pure-subtraction change with no live behavior removed.

A whole-tree residual sweep for consent_store, consent_kv, ConsentSource::KvStore, storage::kv_store, open_kv_store, and runtime_services_for_consent_route comes back clean — matches are limited to the strict-rejection tests, the migration docs, the new plan, and one spec already marked obsolete. Dropping the ConsentSource::KvStore variant carries no stored-record hazard (ConsentSource has no serde derives), no intra-doc link references a removed item, and the absence of Cargo.toml/Cargo.lock churn is correct since hex and sha2 remain widely used. The migration_guards.rs entry removal is required, not optional — include_str! on the deleted paths would fail to compile. The deleted edgezero_missing_consent_store_breaks_only_consent_routes test also asserted admin-401; that coverage survives at app.rs:1597 and app.rs:2286, so nothing of value was lost.

No blocking findings. Four non-blocking, three of them one-click suggestions.

3 of the inline comments below carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. All three were applied in an isolated worktree at this head and verified there: cargo fmt --all -- --check, cargo clippy-spin-native, and cargo clippy-spin-wasm clean for the Rust one; the repo-pinned Prettier 3.8.1 --check clean for the two Markdown ones; and a byte-exact before/after patch comparison confirmed verification did not mutate the tree beyond the reviewed bytes.

Non-blocking

♻️ refactor

  • Spin TTL doc names the wrong KvError variant — see inline at crates/trusted-server-adapter-spin/src/platform.rs:212
  • Migration doc understates the blast radius and omits the pre-flight command — see inline at docs/guide/configuration.md:505
  • Retained legacy store is not covered by EC withdrawal deletion — see inline at docs/guide/fastly.md:307

Cross-cutting / body-level findings

  • 📌 Generic RuntimeServices KV slot now has zero production consumers — after this PR, RuntimeServices::kv_store(), kv_handle(), and with_kv_store() have no production callers; only tests reach them. The plan's contract item 7 states that retaining this infrastructure is deliberate and out of scope, so this is a follow-up rather than a change request. Two consequences seem worth tracking. Cloudflare (crates/trusted-server-adapter-cloudflare/src/platform.rs:616) and Spin (crates/trusted-server-adapter-spin/src/platform.rs:726) still resolve a real KV binding on every request to populate a slot nothing reads. And Fastly permanently populates it with UnavailableKvStore (crates/trusted-server-adapter-fastly/src/app.rs:189), so the first future consumer of the generic slot will silently degrade on the production adapter rather than fail loudly. A tracked issue would keep that from being rediscovered the hard way.

  • 👍 Test-first ordering on a deliberately breaking config change — writing settings_rejects_removed_consent_store_toml / _json and confirming they failed while the field still existed is exactly the right sequencing for a strict-schema removal; it proves the guard tests the removal rather than merely passing alongside it. Covering both Settings::from_toml and Settings::from_json_value matters too, since the runtime blob path goes through the latter via config_payload.rs:39. The residual audit also holds up under an independent sweep, and dropping runtime_services_for_consent_route removed an entire nesting level from dispatch_fallback for free.

CI Status

  • cargo fmt: PASS (this job also runs all 8 target-matched clippy invocations plus the CLI and OpenRTB-codegen clippy steps)
  • cargo test: PASS
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • browser integration tests: PASS
  • vitest: PASS
  • format-typescript: PASS
  • format-docs: PASS
  • Analyze (rust) / CodeQL: not run (absent from gh pr checks output for this PR)

Comment thread crates/trusted-server-adapter-spin/src/platform.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/fastly.md Outdated
@ChristianPavilonis
ChristianPavilonis force-pushed the refactor/remove-legacy-consent-store branch 2 times, most recently from 9ea1f1c to aee5bf6 Compare September 9, 2026 16:00
@ChristianPavilonis
ChristianPavilonis added this pull request to stack #1156 September 9, 2026 17:46
@aram356 aram356 linked an issue Sep 10, 2026 that may be closed by this pull request
@aram356
aram356 force-pushed the refactor/remove-legacy-consent-store branch from aee5bf6 to 2c5ba81 Compare September 10, 2026 22:19
Comment thread crates/trusted-server-core/src/ec/kv.rs
Comment thread crates/trusted-server-core/src/ec/finalize.rs
Comment thread crates/trusted-server-adapter-fastly/src/app.rs
Comment thread crates/trusted-server-adapter-fastly/src/app.rs
@ChristianPavilonis
ChristianPavilonis force-pushed the refactor/remove-legacy-consent-store branch from 33e8de8 to 240f0f8 Compare September 11, 2026 14:59
use crate::platform::{
FastlyPlatformBackend, FastlyPlatformConfigStore, FastlyPlatformGeo, FastlyPlatformHttpClient,
FastlyPlatformSecretStore, UnavailableKvStore, open_kv_store,
FastlyPlatformSecretStore, UnavailableKvStore,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — Re-verified at 240f0f8e2: with open_kv_store and runtime_services_for_consent_route gone, the generic RuntimeServices KV slot still has no production consumer anywhere in the workspace. The only remaining .kv_store() call is a test assertion in adapter-spin/src/platform.rs:1065, and with_kv_store has no non-test caller — every Fastly request now carries UnavailableKvStore in that slot.

The plan doc's contract item 7 scopes this out as platform infrastructure, which is a fair deferral. But I searched open and closed issues and found no follow-up filed, so nothing currently tracks it. A quick issue to either wire the slot to a real consumer or delete with_kv_store / kv_store() would keep it from ossifying as permanent dead plumbing. Not a merge blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Complete removal of the legacy consent KV persistence path

3 participants