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 4 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
Summary
[creative_opportunities].enabledswitch for publisher server-side ad-template delivery.max-age=60), intentionally replacing the origin browser policy while leaving non-200/non-document responses and CDN-specific cache headers unchanged.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-core/src/config.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/auction/endpoints.rsPOST /auctionstill dispatches when templates are disabled.trusted-server.example.tomldocs/guide/configuration.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 uses exactlymax-age=60, intentionally replacing the origin browser policy per #1007. Non-200, non-GET, and non-document responses retain the origin policy, 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!