Skip to content

Add per-cookie template cache policy - #1150

Open
prk-Jr wants to merge 11 commits into
mainfrom
feature/1138-per-cookie-template-cache-policy
Open

Add per-cookie template cache policy#1150
prk-Jr wants to merge 11 commits into
mainfrom
feature/1138-per-cookie-template-cache-policy

Conversation

@prk-Jr

@prk-Jr prk-Jr commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Separate shared HTML templates by bounded cookie variants, so experiment arms remain isolated when a downstream CDN derives a variant header after TS has selected its cache key.
  • Route requests carrying configured session cookies inline without shared lookup or storage. Scope the existing cookie-independence assertion to unlisted cookies while preserving conservative defaults and unconditional Vary: Cookie refusal.
  • Validate configuration and ambiguous prepared cookie input while tolerating compact JSON and comma-list values in ignored cookies. Preserve existing request preparation and cover variant isolation, session bypass, reader assembly, and rollback.

Changes

File Change
crates/trusted-server-core/src/creative_opportunities.rs Add optional key/bypass cookie lists, case-sensitive name validation, scoped independence documentation, and configuration tests.
crates/trusted-server-core/src/cookies.rs Register the internal template-cookie policy module.
crates/trusted-server-core/src/cookies/template_cache_policy.rs Evaluate all prepared cookie fields; distinguish absence from empty values; retain strict key-cookie values and duplicate rejection while allowing bounded JSON/comma-list syntax for ignored cookies.
crates/trusted-server-core/src/platform/template_cache.rs Add framed, hashed cookie dimensions while preserving legacy keys when dimensions are empty; redact values in Debug output.
crates/trusted-server-core/src/platform/mod.rs Export the cookie dimension type.
crates/trusted-server-core/src/publisher.rs Reuse one cookie decision for lookup and storage; add end-to-end regressions, distinct bids on warm hits, and outbound Cookie assertions.
crates/trusted-server-adapter-fastly/src/template_cache.rs Update the cache-key fixture with empty cookie dimensions.
scripts/template-cache-local-test.sh Add repeatable cookie-selected runtime coverage in ESI and inline modes, including origin counts and content assertions.
trusted-server.example.toml Document optional policies, bounded variants, downstream header coverage, and safe rollback.
docs/guide/configuration.md Explain combined and independent deployment patterns, eligibility, limitations, and rollback.
docs/superpowers/specs/2026-09-08-1138-per-cookie-template-cache-policy-design.md Record the reviewed behavior and request-preparation boundary.
docs/superpowers/plans/2026-09-08-1138-per-cookie-template-cache-policy.md Record implementation steps and validation results.

Ignored-value tolerance applies only when independence is asserted. Names, key values, and cookie framing remain validated; unmatched quotes, unsafe bytes, and comma-delimited cookie assignments still bypass. The origin must treat ignored values as opaque and parse semicolon-separated cookies independently; parsers that stop at nonstandard values cannot safely make this assertion.

Closes

Closes #1138

Test plan

Fresh review-readiness validation on 2026-09-10 used 1f56ee08cb25a35d79cccc19a442a698334a398c plus the harness/documentation follow-up in 191380f0. Production Rust and JavaScript are unchanged by that follow-up.

  • Fastly release artifact: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1.
  • Repeatable runtime test: BID_DELAY=3 ./scripts/template-cache-local-test.sh esi — 22 harness checks passed, including all 17 cookie-matrix requests.
  • Inline control: BID_DELAY=3 ./scripts/template-cache-local-test.sh inline — 9 harness checks passed, including all 17 cookie-matrix requests with an origin fetch on every request.
  • Negative control: a temporary harness copy with cookie keying removed failed on the first B request because it received the wrong variant HTML. No production code was changed for this control.
  • cargo test-fastly — 2,902 tests/doc-tests passed; 10 existing tests ignored.
  • cargo test-axum, cargo test-cloudflare, cargo test-spin — 41, 44, and 86 tests passed respectively.
  • cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity — 13 passed.
  • All six target-specific Clippy aliases and cargo fmt --all -- --check.
  • JS build; Vitest under the pinned Node 24.12.0 from crates/trusted-server-js/lib — 45 files, 901 tests passed; JS and docs formatting.
  • Independent review of the harness extension; bash -n scripts/template-cache-local-test.sh and git diff --check.

The harness now returns distinguishable cookie-selected HTML and Vary: X-Exp-Variant without a client variant header. Both modes verify A/B isolation, absent/empty buckets, ignored compact JSON/comma-list cookies, warm session bypass including empty sessions, and cold session bypass without populating anonymous templates. Each request checks content, cache diagnostics, origin fetch counts, private response policy, and winning-bid assembly. Existing CI runs both harness modes.

These runtime runs invoke Viceroy directly, not fastly compute serve. The earlier headless-browser/Viceroy smoke test reported separate A/B miss-storedhit sequences and session bypass-request, all HTTP 200, with page JavaScript disabled to keep cookies stable. Its exact tested commit was not recorded in the previous description; it is historical evidence, not a fresh browser run at the current head.

Viceroy/Fastly tests and Axum socket tests passed after granting the local certificate/keychain and loopback access needed outside the sandbox. An initial Vitest invocation used an incompatible Node environment; rerunning with the pinned Node executable from the library directory passed.

Checklist

  • Changes follow CLAUDE.md conventions.
  • No new unwrap() in production code.
  • Uses log macros, not println!, per project conventions.
  • New code has tests.
  • No secrets or credentials committed.

@prk-Jr prk-Jr self-assigned this Sep 8, 2026
@prk-Jr prk-Jr added this to the 202609 milestone Sep 8, 2026

@ChristianPavilonis ChristianPavilonis 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

Approved. The implementation is sound; one non-blocking code-smell note is attached inline.

ec_context.kv_snapshot().entry_for(identity).is_some(),
"should preload this reader's identity even during withdrawal on a hit"
);
let mut response = match finalizer {

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.

S1. Repeated finalizer switch

The new withdrawal test repeats the existing Finalizer::Streaming and Finalizer::Buffered dispatch at line 9571. Both paths must keep testing streaming and buffered finalization, but duplicating this switch makes later finalizer changes easy to apply in only one location.

Consider extracting the finalizer dispatch into one test helper and calling it from both locations. This adds one helper and some parameter passing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in cd9ccd4. Extracted finalize_test_publisher_response and routed both the shared publisher fixture and the EC withdrawal test through it. Both streaming and buffered finalizers remain covered, including warm-cache withdrawal. The full Fastly test suite passes.

@aram356 aram356 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

Grows the cookie-independence boolean into a per-cookie policy: named key cookies become sorted, length-prefixed dimensions of the shared template key, named bypass cookies force the inline path, and the assertion narrows to unlisted cookies. The runtime logic holds up under tracing: one decision drives lookup, reservation, and store, so a bypass cookie can never read or populate a template; Vary: Cookie refusal is untouched; empty dimensions preserve legacy keys; the fingerprint picks up the new fields through settings serialization; Debug output redacts values. Tests cover the evaluator matrix, key isolation, both finalizers, EC withdrawal on warm hits, and the harness negative control, and CI runs both harness modes.

Nothing here is blocking on its own. The findings below are hit-rate, operator-safety, and readability improvements.

4 of the inline comments below carry a one-click GitHub suggestion. Use Commit suggestion (or Add suggestion to batch) to apply them. Each was applied in isolation and as a batch in a scratch worktree and passed cargo fmt --check, all six clippy aliases, all four adapter test aliases, the parity suite, and prettier. The remaining comments describe the fix in prose because the change spans ranges outside one hunk.

Non-blocking

♻️ refactor

  • Log the cookie bypass reason at the request gate — see inline at crates/trusted-server-core/src/publisher.rs:4304
  • Reject TS identity cookie names in the key list — see inline at crates/trusted-server-core/src/cookies/template_cache_policy.rs:28

🤔 thinking

  • Duplicate unlisted names need not bypass under an independence assertion — see inline at crates/trusted-server-core/src/cookies/template_cache_policy.rs:73

⛏ nitpick

  • Doc sentence for template_cache_bypass_cookies — see inline at crates/trusted-server-core/src/creative_opportunities.rs:344
  • Unformatted 230-character assert — see inline at crates/trusted-server-core/src/cookies/template_cache_policy.rs:335
  • Primary docs example pairs the lists with false — see inline at docs/guide/configuration.md:1912
  • CookieForwarded variant name is stale — see inline at crates/trusted-server-core/src/publisher.rs:5727

Cross-cutting / body-level findings

  • 🌱 No bound on key-cookie value length. is_cookie_value accepts any cookie-octet run of any length, so a mistakenly keyed token or ID enters the key unchecked. The spec scopes cardinality controls out, but a cheap bypass above a fixed byte length (64 is generous for experiment arms and region buckets) would catch the accidental case without limiting real variant labels. Future work, not this PR.
  • 🤔 Named-policy strictness for unlisted values. Under a named policy with independence true, every unlisted cookie value is still framing-checked in is_ignored_cookie_value: a raw space, a non-ASCII byte, or a backslash in any third-party cookie bypasses the whole request. That is stricter than the legacy boolean path and partially reintroduces the "nearly inert" problem this feature targets. The origin-parser rationale is documented in the spec and the guide, so this is a heads-up on expected hit rate during the canary rather than a request to change it.

CI Status

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

Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/creative_opportunities.rs Outdated
Comment thread crates/trusted-server-core/src/cookies/template_cache_policy.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/cookies/template_cache_policy.rs Outdated
Comment thread crates/trusted-server-core/src/cookies/template_cache_policy.rs
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
@prk-Jr
prk-Jr requested a review from aram356 September 14, 2026 15:22
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.

Grow origin_is_cookie_independent into a per-cookie template cache policy

3 participants