Add a server-side ad template switch and cache policy - #1008
Add a server-side ad template switch and cache policy#1008ChristianPavilonis wants to merge 9 commits into
Conversation
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Separating publisher template delivery from the global [auction].enabled kill switch is the right boundary, and the direct-/auction regression test proves the separation holds. The configuration half of this PR is well covered.
The cache-policy half needs work. The new else branch does not just preserve origin policy when templates are inactive — it overwrites it, and its only protection is a two-token check for private/no-store. I verified against this branch (throwaway probes in ssat_cache_policy_tests, cargo test -p trusted-server-core) that origin no-cache, max-age=0, must-revalidate and s-maxage=0 are all replaced with max-age=60, and that 500/404/503 HTML responses become cacheable for 60 seconds. Details inline.
Blocking
🔧 wrench
- Origin revalidation directives silently overwritten — the
private/no-storeguard missesno-cache,max-age=0,must-revalidate,s-maxage=0. A personalized page markedno-cacheby origin, on a repeat visit that emits noSet-Cookie(so the adapter cookie-privacy net does not fire), getsmax-age=60with noprivateand noVary— shared-cacheable and replayable to other users for 60s. (crates/trusted-server-core/src/publisher.rs:2986) - Non-2xx HTML becomes cacheable for 60s — no status gate on the new branch, so a transient origin
5xxis pinned in every browser and intermediary for a minute past recovery. (crates/trusted-server-core/src/publisher.rs:2961) - Rollback with
enabled = falseis a site-wide 500, and is undocumented —deny_unknown_fieldsmakes an older binary reject the blob, andload_settings_from_config_store()failing returns500for every request (crates/trusted-server-adapter-fastly/src/main.rs:112-118). The plan file states the "fail loud" intent, but neitherdocs/guide/configuration.md:1315nor theCHANGELOG.md:12entry warns operators what "loud" means here. (crates/trusted-server-core/src/config.rs:333)
❓ question
- Absent
[creative_opportunities]section also gets its cache policy rewritten —is_some_andmakes "never configured" behave like "explicitly disabled", so deployments that never enabled server-side ad templates have their originCache-Controlreplaced withmax-age=60. Onmainthose responses passed through untouched. Intended blast radius? (crates/trusted-server-core/src/publisher.rs:2643)
Non-blocking
🤔 thinking
max-age=60is an unexplained magic constant — no derivation in the CHANGELOG, configuration guide, or plan file, and not operator-tunable. (crates/trusted-server-core/src/publisher.rs:2992)- The empty-
slotdisable already existed and was rollback-safe — worth stating in the PR body why the new field's rollback cost was accepted. (crates/trusted-server-core/src/creative_opportunities.rs:206)
♻️ refactor
ad_templates_enabled/ad_templates_disabledare not complements — both arefalsewhen the section is absent; an explicit three-state enum would make that unmissable. (crates/trusted-server-core/src/publisher.rs:2644)
⛏ nitpick
should_run_server_side_ad_stackstill takes 7 arguments — the new struct absorbed only 2 of the 8 flags, leaving 6 positional bools at every call site. (crates/trusted-server-core/src/publisher.rs:1765)- Test name contradicts its assertion —
disabled_creative_opportunities_flag_is_visible_to_legacy_schemaassertsexpect_err, i.e. the legacy schema rejects the field...._is_rejected_by_legacy_schemawould read correctly. (crates/trusted-server-core/src/config.rs:333)
📝 note
- Cache-policy test matrix has the same gap as the code —
navigation_without_matched_slots_preserves_private_origin_cache_policycovers"private, max-age=0"and"No-Store"only. Once the two wrench findings are settled,no-cacheand a non-200 status belong in the same loop, or the regressions will not be caught. (crates/trusted-server-core/src/publisher.rs:5071)
👍 praise
- Direct
/auctionregression test —TemplateSwitchProbeProvidercounts real provider invocations rather than asserting a status code, so it would actually fail if the template flag were later threaded intohandle_auction. (crates/trusted-server-core/src/auction/endpoints.rs:707) - Rollback-compatible serialization of the default —
skip_serializing_ifkeeping defaulttrueout of pushed blobs matches the existingsection_rootprecedent and keeps the no-opt-in case safe. (crates/trusted-server-core/src/creative_opportunities.rs:204)
CI Status
All 19 GitHub checks pass on 58054463.
- fmt: PASS
- clippy (fastly / axum / cloudflare native+wasm / spin native+wasm): PASS
- rust tests (fastly, axum native, cloudflare, spin, cross-adapter parity, ts CLI): PASS
- js tests (vitest): PASS
- format-typescript / format-docs: PASS
- integration + browser integration tests: PASS
The findings above are behavioral gaps that the current test matrix does not exercise, not CI failures.
aram356
left a comment
There was a problem hiding this comment.
Summary
The switch mechanics are solid: default-true serde field with rollback-aware serialization, consistent accessor/handler gating, POST /auction independence proven by a provider-probe test, and validation still runs when disabled. The blocking concern is concentrated in the new cache-clamp branch, which overrides origin freshness directives beyond the private/no-store preserve-guard.
Blocking
🔧 wrench
- Cache clamp overrides origin freshness directives beyond
private/no-store: origins sendingno-cache,must-revalidate, ormax-age=0get replaced withmax-age=60(crates/trusted-server-core/src/publisher.rs:2988 — see inline comment)
Non-blocking
🤔 thinking
- Clamp blast radius: applies to all HTML, all methods, and publishers with no
[creative_opportunities]section at all (crates/trusted-server-core/src/publisher.rs:2980 — see inline comment) enabled = falseconfig blobs break not-yet-upgraded binaries: the explicit-false rollback hazard is codified in a test but undocumented for operators (crates/trusted-server-core/src/config.rs:333 — see inline comment)
🌱 seedling
- Hardcoded 60-second TTL: likely needs to become configurable when SSAT is re-architected for cacheability (crates/trusted-server-core/src/publisher.rs:2992 — see inline comment)
CI Status
- fmt: PASS
- clippy (all targets): PASS
- rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
- js tests (vitest): PASS
- browser integration tests: PASS
5805446 to
12f0f5c
Compare
aram356
left a comment
There was a problem hiding this comment.
Summary
The update resolves four of the five round-one concerns: the inactive policy is now correctly narrowed to 200 OK GET document HTML (with regression coverage for 206/404/500/503, POST, and non-document fetches), the enabled = false rollback hazard is documented with the correct re-push sequencing in the guide/example/CHANGELOG, GPT-diagnostics privacy now takes precedence via the finalize_response reordering, and the 60s TTL source is documented. The direct-auction probe test survived the rebase intact.
One blocking concern remains — and it was introduced by the round-one fix itself: the preserve guard was removed entirely rather than extended, so origin private/no-store HTML is now rewritten to a shared-cacheable policy.
Blocking
🔧 wrench
- Removing the preserve guard makes origin
private/no-storeHTML shared-cacheable: inactive 200 OK GET documents replace origin privacy directives with baremax-age=60, and the test at crates/trusted-server-core/src/publisher.rs:5240 pins that in (crates/trusted-server-core/src/publisher.rs:2995 — see inline comment)
CI Status
- fmt: PASS
- clippy (all targets): PASS
- rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
- js tests (vitest): PASS
- browser integration tests: PASS
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Round-two pass, scoped to what the update introduces rather than re-litigating round one. The status/method/document narrowing and the finalize_response reordering both look right, and I verified that nothing per-user is injected on the inactive path — so the residual risk in the new branch is entirely about origin-declared policy and cache-variant mixing, not injected identity.
Two new blocking findings, neither of which overlaps the outstanding preserve-guard concern. The first is additive to it: even with origin private/no-store preserved, a no-cache origin still exposes an ad-free variant. The second is an operator-facing documentation/test defect in the new environment override.
Blocking
🔧 wrench
- Request-scoped suppression makes the ad-free variant the only shared-cacheable one: the inactive branch has no
!is_bot/!is_prefetch/ consent gate, so bot, prefetch, and consent-denied navigations of an ad-eligible URL get baremax-age=60while consenting humans getprivate, no-store— with noVaryon the publisher path (crates/trusted-server-core/src/publisher.rs:2995, see inline comment for probe output). - The documented environment override for the new switch silently no-ops on the real deploy path:
Settings::from_toml_and_envis#[cfg(test)]-only, and the EdgeZero overlay cannot create missing TOML leaves (docs/guide/configuration.md:1381, see inline comment).
Non-blocking
🤔 thinking
max-age=60is documented only where its largest affected audience won't read it: the policy also applies withenabled = true, with no[creative_opportunities]section at all, and on bot/prefetch/consent-denied requests (docs/guide/configuration.md:1350,trusted-server.example.toml:184-188).- Undocumented, untested exception — first-visit navigations never get
max-age=60:enforce_set_cookie_cache_privacyrewrites anySet-Cookie-bearing response toprivate, max-age=0(crates/trusted-server-core/src/publisher.rs:2999).
📝 note
- Verified: nothing per-user is injected when the ad stack is inactive. The
</body>bids/adInit()script is skipped (crates/trusted-server-core/src/html_processor.rs:373-377), every integrationhead_insertsimplementation ignores the request context and serializes config values only (gpt.rs:489,prebid.rs:1077,sourcepoint.rs:1021,didomi.rs:340,datadome.rs:768), and the injected bundlesrcis a content hash rather than a per-user token (crates/trusted-server-core/src/tsjs.rs:5-9). Recording this so the blocking discussion stays on the actual mechanism: origin-declared policy replacement plus variant mixing, not leaked identity in the markup. - No
Varyis written anywhere on the publisher HTML path.crates/trusted-server-core/src/publisher.rscontains noVarywrites; the existing writers all target other response types (static/tsjs bundles,/identify, proxied GPT and Sourcepoint assets, the fingerprint debug endpoint). Only an origin-suppliedVarysurvives into themax-age=60response.
👍 praise
- The
finalize_responsereordering is pinned by a test that asserts ordering, which is what actually regressed (crates/trusted-server-core/src/publisher.rs:5337). - The narrowing tests lock the blast radius round one asked about: 206/404/500/503, POST, and non-document coverage all assert origin-policy preservation rather than just a status code.
CI Status
- fmt: PASS
- clippy (fastly / axum / cloudflare native + wasm / spin native + wasm): PASS
- rust tests (fastly, axum, cloudflare, spin, cross-adapter parity, ts CLI): PASS
- js tests (vitest): PASS
- browser + Fastly EC lifecycle integration tests: PASS
276856a to
38c9636
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Adds a dedicated [creative_opportunities].enabled switch so publisher HTML and page-bids template delivery can be turned off independently of [auction].enabled, and applies the issue #1007 Cache-Control: max-age=60 policy to successful GET publisher documents when the server-side ad stack is structurally inactive. The structural-vs-request-scoped split is the right design and the test matrix around it is thorough. One blocking concern on the emitted directive and one question on scope.
Blocking
🔧 wrench
max-age=60is shared-cacheable, not a browser-only policy: a baremax-age=60with noprivateauthorizes shared caches, contradicting the "browser-facing" framing in the code comment and the configuration guide. An originno-cacheon a personalized document becomes 60s of shared cacheability for any intermediary that does not read the CDN-specific headers (crates/trusted-server-core/src/publisher.rs:3054).
❓ question
- Absent
[creative_opportunities]adopts the new policy too: deployments that never configured the feature now have originCache-Controlreplaced on every successful GET HTML navigation, with no opt-out. Intended? (crates/trusted-server-core/src/publisher.rs:3033)
Non-blocking
♻️ refactor
- Duplicated request-eligibility gate: the cache branch re-spells the request-scoped half of
should_run_server_side_ad_stack; a future gate added to one will not reach the other (crates/trusted-server-core/src/publisher.rs:3037). - Third copy of the "already uncacheable" check: the same lowercase +
containspattern now exists inpublisher.rsand twice inresponse_privacy.rs(crates/trusted-server-core/src/publisher.rs:3050).
🤔 thinking
ServerSideAdStackConfigbundles 2 of 8 gates: six positional bools remain, which is where mis-ordering actually bites (crates/trusted-server-core/src/publisher.rs:1804).- Rollback failure mode is documentation-only:
enabled = falseplusdeny_unknown_fieldsmeans an older binary fails to load settings and every request fails; consider ats config pushwarning alongside the guide's warning block (crates/trusted-server-core/src/creative_opportunities.rs:206).
🌱 seedling
- Publisher-HTML body coverage:
disabled_ad_templates_use_short_browser_cache_policyasserts headers only; the page-bids path has an equivalent body assertion, the publisher path does not (crates/trusted-server-core/src/publisher.rs:5278).
⛏ nitpick
- Redundant Option walks: the new
creative_opportunitieslocal is bound and then re-derived on the next line (crates/trusted-server-core/src/publisher.rs:2708). - Unrelated test fixture weakened:
enforce_set_cookie_cache_privacyis untouched by this PR, but its fixture lost the origin-public scenario it was written for (crates/trusted-server-adapter-fastly/src/middleware.rs:432).
👍 praise
- Structural vs request-scoped split, with bot/prefetch/consent-denied retaining the origin policy, and a test matrix covering non-200, non-GET, non-document, and mixed-case
No-Store(crates/trusted-server-core/src/publisher.rs:5433). - Moving
gpt_diagnostics::finalize_responsebelow the cache block so diagnostics privacy wins, pinned by a test (crates/trusted-server-core/src/publisher.rs:3060). - Direct
/auctionregression test with a probe provider, proving the PR's central premise end to end (crates/trusted-server-core/src/auction/endpoints.rs:738).
CI Status
All 19 checks pass on 38c9636.
- fmt: PASS
- clippy / cargo check (fastly, axum, cloudflare native + wasm, spin native + wasm): PASS
- rust tests (fastly, axum, cloudflare, spin, ts CLI, cross-adapter parity): PASS
- integration tests (browser, Fastly EC lifecycle): PASS
- js tests (vitest) and format (typescript, docs): PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
The structural-vs-request-scoped split and its test matrix are sound, and the round-three concerns remain resolved on this head. One blocking issue remains in the emitted directive itself: bare max-age=60 authorizes shared caches, which both contradicts the documented "browser-facing" intent and reintroduces a cross-user vector for origins that serve personalized HTML with bare no-cache. The fix is one word. Non-blocking cleanups accompany it.
Blocking
🔧 wrench
- Emit
private, max-age=60, not baremax-age=60: a baremax-ageauthorizes shared caches and removes the revalidation obligationno-cacheorigins relied on (crates/trusted-server-core/src/publisher.rs:3054 — see inline comment)
Non-blocking
♻️ refactor
- Duplicated request-eligibility gate: the cache branch re-spells the request-scoped half of
should_run_server_side_ad_stack(crates/trusted-server-core/src/publisher.rs:3033) - Third copy of the "already uncacheable" check: the lowercase +
containspattern now exists here and twice inresponse_privacy.rs(crates/trusted-server-core/src/publisher.rs:3050)
🤔 thinking
- Six positional bools remain on
should_run_server_side_ad_stack(crates/trusted-server-core/src/publisher.rs:1804) - Rollback failure mode is documentation-only: a
ts config pushwarning would catch it at the point of action (crates/trusted-server-core/src/creative_opportunities.rs:206)
🌱 seedling
- Publisher-HTML body coverage: the disabled-templates test asserts headers only (crates/trusted-server-core/src/publisher.rs:5278)
📝 note
- Absent-section scope is confirmed intended: answered in an earlier round, documented in the guide, and pinned by a dedicated test (crates/trusted-server-core/src/publisher.rs:5484)
⛏ nitpick
- Redundant
Optionwalks (crates/trusted-server-core/src/publisher.rs:2707) - Middleware fixture lost its origin-public scenario (crates/trusted-server-adapter-fastly/src/middleware.rs:432)
CI Status
- fmt: PASS
- clippy (all targets): PASS
- rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
- js tests (vitest): PASS
- browser integration tests: PASS
# Conflicts: # crates/trusted-server-cli/tests/config_env_overlay.rs
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Clean separation of publisher template delivery from the global [auction].enabled switch, with thorough table-driven coverage of the new cache policy across status codes, request kinds, and origin policies. One blocking issue: the private, max-age=60 cap is defeated on revalidation, because the inactive-stack path preserves origin validators but does not carry the policy onto the resulting 304.
None of the inline comments below carry a one-click GitHub
suggestion— each proposed fix either spans more than one hunk (the code change plus its new test) or is a design question rather than an edit, so every fix is described in prose and has to be applied manually.
Blocking
🔧 wrench
304revalidation replays origin freshness, defeating the 60s cap — see inline atcrates/trusted-server-core/src/publisher.rs:3054
Non-blocking
🤔 thinking
- No escape hatch for deployments that never configured SSAT — see inline at
crates/trusted-server-core/src/publisher.rs:3048 privatediverges from the issue text — see inline atcrates/trusted-server-core/src/response_privacy.rs:27- Policy now varies per request with no
Vary— see inline atcrates/trusted-server-core/src/publisher.rs:3053
♻️ refactor
- Extension marker has no end-to-end coverage — see inline at
crates/trusted-server-core/src/response_privacy.rs:30
👍 praise
- Quoted-string-aware
Cache-Controldirective parsing — see inline atcrates/trusted-server-core/src/response_privacy.rs:32
Cross-cutting / body-level findings
-
📝 PR description file table is incomplete — three changed files are missing from the table in the description:
crates/trusted-server-adapter-fastly/src/middleware.rs(the late-cookie downgrade test now loops over both policies),crates/trusted-server-cli/tests/config_env_overlay.rs(the newTRUSTED_SERVER__CREATIVE_OPPORTUNITIES__ENABLEDoverlay assertion), anddocs/superpowers/plans/2026-08-06-server-side-ad-templates-cache-control.md(the plan doc). The plan doc matches the establisheddocs/superpowers/plans/convention, so this is only about the table being a complete map of the diff. -
📝 Checked and found clean — recording these so the next reviewer does not have to re-derive them:
prepare_runtimestill callscompile_slots/compile_unit_templates/validate_runtimefor a config withenabled = false, so the switch cannot be used to smuggle an invalid slot config past startup validation and then flip it on. The EC identity cookie is not refreshed for returning users (ec/finalize.rs:91), so cookie-privacy finalization does not silently downgrade every navigation toprivate, max-age=0and leavemax-age=60unreachable in practice. Movinggpt_diagnostics::finalize_responseafter the cache-policy block is header-neutral on the active path, since both it andenforce_synthesized_html_cache_privacyland onprivate, no-storeand both strip the CDN cache headers. TheGeneratedInactiveAdStackBrowserCachePolicyextension does survive to every adapter's finalize — no path betweenhandle_publisher_requestand the adapter rebuilds the response head (see the ♻️ comment for the coverage gap that follows from this being untested).
CI Status
All 19 reported checks PASS; 4 are required under branch protection.
- 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
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
# Conflicts: # CHANGELOG.md # crates/trusted-server-adapter-fastly/src/middleware.rs # crates/trusted-server-core/src/publisher.rs # crates/trusted-server-core/src/response_privacy.rs
aram356
left a comment
There was a problem hiding this comment.
Cross-cutting / body-level findings
- 📝 CI did not run on this head — only
Analyze (javascript-typescript)triggered for8927e136f; the rest of the matrix (fmt, clippy chain, adapter tests, parity, integration, vitest, CodeQL) is not run per the check-suites API. Verified locally in a clean worktree of this head:cargo fmt --all -- --checkpasses and the fullssat_cache_policy/response_privacysuite (17 tests, including both new 304 revalidation tests) passes undercargo test-fastlyonwasm32-wasip1. Recommend re-triggering the workflows (empty-commit push or close/reopen) so the full remote matrix is green before merge.
CI Status
- Analyze (javascript-typescript): PASS
- all other checks: not run (workflows never triggered for this push)
- local: cargo fmt PASS; cargo test-fastly (cache-policy suite, 17/17) PASS
Summary
[creative_opportunities].enabledswitch for publisher server-side ad-template delivery.304 Not Modifiedrevalidations to the issue Improve cache header for html content when SSAT is off #1007 browser-only cache policy (private, max-age=60), replacing origin freshness directives while preserving originprivate/no-storepolicies and CDN-specific cache headers.POST /auctionavailable when publisher templates are disabled.Issue #1007 exposed that publisher HTML caching was tied to whether the server-side ad stack ran, while the global auction setting also controlled unrelated auction behavior. This change separates publisher template delivery from the direct auction API and makes the cache behavior explicit.
Changes
crates/trusted-server-core/src/creative_opportunities.rsenabledconfiguration field and serialization coverage.crates/trusted-server-core/src/settings.rscrates/trusted-server-cli/tests/config_env_overlay.rscrates/trusted-server-core/src/config.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/response_privacy.rscrates/trusted-server-adapter-fastly/src/middleware.rscrates/trusted-server-core/src/auction/endpoints.rsPOST /auctionstill dispatches when templates are disabled.trusted-server.example.tomldocs/guide/configuration.mddocs/superpowers/plans/2026-08-06-server-side-ad-templates-cache-control.mdCHANGELOG.mdcrates/trusted-server-js/lib/src/core/index.tscrates/trusted-server-js/lib/src/integrations/gpt/index.tscrates/trusted-server-js/lib/test/integrations/gpt/spa_hook.test.tsScope
The change is limited to configuration, core publisher/page-bids execution, direct-auction regression coverage, browser comments, and documentation. Existing adapter routes already use the centralized settings accessor, so no divergent adapter-specific switch was needed. Active server-side templates retain
private, no-store; inactive200 OKGET document HTML and its304 Not Modifiedrevalidations use exactlyprivate, max-age=60, intentionally replacing origin freshness directives per #1007 while preventing storage by shared caches that useCache-Control. Originprivate/no-storepolicies, other response statuses, non-GET requests, and non-document responses remain unchanged. Request-scoped privacy finalization still takes precedence, and validators plus CDN-specific headers remain unchanged. An empty slot list could disable delivery rollback-safely, but the dedicated switch preserves configured slot definitions for reversible operations; because explicitenabled = falseis serialized, the guide documents the required config re-push before rolling back to a pre-field binary.Closes
Closes #1007
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 servecargo test-cloudflare,cargo test-spin, focused publisher tests, and all configured native/WASM clippy targetsChecklist
CLAUDE.mdconventionsunwrap()in production code — useexpect("should ...")println!