Skip to content

Add a server-side ad template switch and cache policy - #1008

Merged
ChristianPavilonis merged 7 commits into
mainfrom
issue-1007-cache-control
Aug 24, 2026
Merged

ChristianPavilonis merged 7 commits into
mainfrom
issue-1007-cache-control

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Aug 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Add a clear [creative_opportunities].enabled switch for publisher server-side ad-template delivery.
  • Set structurally inactive successful GET publisher documents and their 304 Not Modified revalidations 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 origin private/no-store policies and CDN-specific cache headers.
  • Keep direct POST /auction available 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

File Change
crates/trusted-server-core/src/creative_opportunities.rs Adds the default-true enabled configuration field and serialization coverage.
crates/trusted-server-core/src/settings.rs Hides creative-opportunity slots from runtime handlers when template delivery is disabled and tests environment overrides.
crates/trusted-server-cli/tests/config_env_overlay.rs Verifies the typed CLI applies the new environment override on pushed configurations.
crates/trusted-server-core/src/config.rs Verifies compatibility for omitted defaults and explicit disabled values.
crates/trusted-server-core/src/publisher.rs Gates publisher HTML and page-bids template delivery, records the disabled-template reason, and tests cache behavior.
crates/trusted-server-core/src/response_privacy.rs Centralizes shared-storage policy checks and keeps cookie privacy stronger than the inactive browser policy.
crates/trusted-server-adapter-fastly/src/middleware.rs Verifies late cookie finalization downgrades the generated inactive browser policy.
crates/trusted-server-core/src/auction/endpoints.rs Proves direct POST /auction still dispatches when templates are disabled.
trusted-server.example.toml Documents the new setting in the example configuration.
docs/guide/configuration.md Documents the switch, cache policy, and environment override.
docs/superpowers/plans/2026-08-06-server-side-ad-templates-cache-control.md Records the implementation decisions, compatibility rules, and verification plan.
CHANGELOG.md Records the new switch and cache behavior.
crates/trusted-server-js/lib/src/core/index.ts Clarifies browser defaults when template delivery is gated.
crates/trusted-server-js/lib/src/integrations/gpt/index.ts Clarifies page-bids behavior when template delivery is disabled.
crates/trusted-server-js/lib/test/integrations/gpt/spa_hook.test.ts Updates the related regression-test explanation.

Scope

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; inactive 200 OK GET document HTML and its 304 Not Modified revalidations use exactly private, max-age=60, intentionally replacing origin freshness directives per #1007 while preventing storage by shared caches that use Cache-Control. Origin private/no-store policies, 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 explicit enabled = false is 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-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
  • Other: cargo test-cloudflare, cargo test-spin, focused publisher tests, and all configured native/WASM clippy targets

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses project logging macros, not println!
  • New code has tests
  • No secrets or credentials committed

@ChristianPavilonis ChristianPavilonis self-assigned this Aug 6, 2026
@ChristianPavilonis ChristianPavilonis changed the title Use a short browser cache policy for non-SSAT HTML Use a browser cache policy for non-SSAT HTML Aug 6, 2026
@ChristianPavilonis ChristianPavilonis changed the title Use a browser cache policy for non-SSAT HTML Add a server-side ad template switch and cache policy Aug 7, 2026

@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

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-store guard misses no-cache, max-age=0, must-revalidate, s-maxage=0. A personalized page marked no-cache by origin, on a repeat visit that emits no Set-Cookie (so the adapter cookie-privacy net does not fire), gets max-age=60 with no private and no Vary — 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 5xx is pinned in every browser and intermediary for a minute past recovery. (crates/trusted-server-core/src/publisher.rs:2961)
  • Rollback with enabled = false is a site-wide 500, and is undocumented — deny_unknown_fields makes an older binary reject the blob, and load_settings_from_config_store() failing returns 500 for every request (crates/trusted-server-adapter-fastly/src/main.rs:112-118). The plan file states the "fail loud" intent, but neither docs/guide/configuration.md:1315 nor the CHANGELOG.md:12 entry 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_and makes "never configured" behave like "explicitly disabled", so deployments that never enabled server-side ad templates have their origin Cache-Control replaced with max-age=60. On main those responses passed through untouched. Intended blast radius? (crates/trusted-server-core/src/publisher.rs:2643)

Non-blocking

🤔 thinking

  • max-age=60 is 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-slot disable 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_disabled are not complements — both are false when 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_stack still 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_schema asserts expect_err, i.e. the legacy schema rejects the field. ..._is_rejected_by_legacy_schema would 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_policy covers "private, max-age=0" and "No-Store" only. Once the two wrench findings are settled, no-cache and 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 /auction regression test — TemplateSwitchProbeProvider counts real provider invocations rather than asserting a status code, so it would actually fail if the template flag were later threaded into handle_auction. (crates/trusted-server-core/src/auction/endpoints.rs:707)
  • Rollback-compatible serialization of the default — skip_serializing_if keeping default true out of pushed blobs matches the existing section_root precedent 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.

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/config.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/creative_opportunities.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/auction/endpoints.rs
Comment thread crates/trusted-server-core/src/creative_opportunities.rs

@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

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 sending no-cache, must-revalidate, or max-age=0 get replaced with max-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 = false config 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

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/config.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
@aram356 aram356 added this to the 202608 milestone Aug 13, 2026

@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

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-store HTML shared-cacheable: inactive 200 OK GET documents replace origin privacy directives with bare max-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

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
aram356 added a commit that referenced this pull request Aug 16, 2026
#1010)

Resolved publisher.rs to keep #1008's inactive-SSAT cache policy;
datadome protection.rs resolved to main's final #992 squash.

@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

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 bare max-age=60 while consenting humans get private, no-store — with no Vary on 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_env is #[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=60 is documented only where its largest affected audience won't read it: the policy also applies with enabled = 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_privacy rewrites any Set-Cookie-bearing response to private, 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 integration head_inserts implementation 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 bundle src is 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 Vary is written anywhere on the publisher HTML path. crates/trusted-server-core/src/publisher.rs contains no Vary writes; the existing writers all target other response types (static/tsjs bundles, /identify, proxied GPT and Sourcepoint assets, the fingerprint debug endpoint). Only an origin-supplied Vary survives into the max-age=60 response.

👍 praise

  • The finalize_response reordering 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

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs

@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

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=60 is shared-cacheable, not a browser-only policy: a bare max-age=60 with no private authorizes shared caches, contradicting the "browser-facing" framing in the code comment and the configuration guide. An origin no-cache on 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 origin Cache-Control replaced 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 + contains pattern now exists in publisher.rs and twice in response_privacy.rs (crates/trusted-server-core/src/publisher.rs:3050).

🤔 thinking

  • ServerSideAdStackConfig bundles 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 = false plus deny_unknown_fields means an older binary fails to load settings and every request fails; consider a ts config push warning 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_policy asserts 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_opportunities local 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_privacy is 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_response below the cache block so diagnostics privacy wins, pinned by a test (crates/trusted-server-core/src/publisher.rs:3060).
  • Direct /auction regression 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

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-adapter-fastly/src/middleware.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/auction/endpoints.rs

@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

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 bare max-age=60: a bare max-age authorizes shared caches and removes the revalidation obligation no-cache origins 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 + contains pattern now exists here and twice in response_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 push warning 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 Option walks (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

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/creative_opportunities.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-adapter-fastly/src/middleware.rs Outdated
@ChristianPavilonis
ChristianPavilonis requested review from prk-Jr and removed request for prk-Jr August 20, 2026 16:20

@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

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

  • 304 revalidation replays origin freshness, defeating the 60s cap — see inline at crates/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
  • private diverges from the issue text — see inline at crates/trusted-server-core/src/response_privacy.rs:27
  • Policy now varies per request with no Vary — see inline at crates/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-Control directive parsing — see inline at crates/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 new TRUSTED_SERVER__CREATIVE_OPPORTUNITIES__ENABLED overlay assertion), and docs/superpowers/plans/2026-08-06-server-side-ad-templates-cache-control.md (the plan doc). The plan doc matches the established docs/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_runtime still calls compile_slots / compile_unit_templates / validate_runtime for a config with enabled = 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 to private, max-age=0 and leave max-age=60 unreachable in practice. Moving gpt_diagnostics::finalize_response after the cache-policy block is header-neutral on the active path, since both it and enforce_synthesized_html_cache_privacy land on private, no-store and both strip the CDN cache headers. The GeneratedInactiveAdStackBrowserCachePolicy extension does survive to every adapter's finalize — no path between handle_publisher_request and 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

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/response_privacy.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/response_privacy.rs
Comment thread crates/trusted-server-core/src/response_privacy.rs Outdated
@aram356
aram356 self-requested a review August 21, 2026 04:48
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/creative_opportunities.rs
Comment thread crates/trusted-server-core/src/publisher.rs
ChristianPavilonis added a commit that referenced this pull request Aug 21, 2026
# 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 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.

Cross-cutting / body-level findings

  • 📝 CI did not run on this head — only Analyze (javascript-typescript) triggered for 8927e136f; 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 -- --check passes and the full ssat_cache_policy/response_privacy suite (17 tests, including both new 304 revalidation tests) passes under cargo test-fastly on wasm32-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

prk-Jr added a commit that referenced this pull request Aug 24, 2026
Resolve ten conflicts from main's three commits after the #860 merge base.
#943 and #1013 reached main as squashes after rc had already merged earlier
revisions of the same branches, and #1066 is new to rc.

Take main where its commit is the newer revision:

- Fastly `apply_terminal_response_effects`: `must_remain_private` narrows back
  to the `TerminalPrivateResponse` marker, so an unmarked origin
  `private, max-age=600` keeps its own policy and validators instead of being
  overwritten. The terminal guard runs both `enforce_set_cookie_cache_privacy`
  and `enforce_uncacheable_cache_privacy`. Adds main's three review tests for
  page-bids preflight, active diagnostics, and the unmarked-origin case, while
  keeping rc's `resolve_and_sanitize_client_ip` block.
- The three `docs/superpowers` documents: #1066's dead-link fixes, with no
  other difference between the two versions.

Keep rc where it is the functional superset, after confirming no line main
added is missing from it:

- `response_privacy.rs` keeps #1008's inactive-ad-stack browser policy, the
  quoted-string-aware directive parser, and the public `is_private_or_no_store`;
  main's two new edge-cache-header tests are grafted on.
- `publisher.rs` keeps `PageBidsFormat::parse`, which supersedes main's inline
  `format` rejection, and the assembled-response privacy condition that already
  covers main's narrower form.
- `creative_opportunities.rs` and `scripts/template-cache-local-test.sh`.

Two artifacts of the automatic merge needed correcting: `settings.rs` gained ten
duplicate `#[test]` definitions from the #943 squash meeting rc's native merge,
which `cargo check` cannot see because it does not build tests, and
`configuration.md` had a duplicated shared-template heading.

`Cache-Control` keeps rc's `private, no-store` ordering. #1013's final revision
flipped the `enforce_private_no_store` path to `no-store, private`; directive
order carries no meaning, rc asserts the current spelling in 34 places, and the
CI shell harness greps the literal string. Main's grafted assertions were
adapted instead. The JA4 debug endpoint keeps its own `no-store, private`
literal.
@ChristianPavilonis
ChristianPavilonis merged commit 99e0314 into main Aug 24, 2026
19 checks passed
prk-Jr added a commit that referenced this pull request Aug 26, 2026
main carried squash merges of #1008 (server-side ad template switch and
cache policy) and #1042 (publisher-native APS rendering). rc had already
merged both impl branches, so every conflict is a squash-vs-merge artifact
resolved per hunk.

Kept rc's side where rc diverged after its merge: config-first auction
providers, the secret-validation test renames, rc's strip_cdn_cache_headers
and `private, no-store` header ordering, PageBidsFormat::parse, and rc's
existing ssat_cache_policy_tests module.

Took main's side where the PR gained review fixes after rc merged it: the
no-referrer runner document and conditional cancelPendingApsRendering, the
ad_templates_disabled gate on the publisher and page-bids paths, and the
configuration rollback warning.

Also repaired three duplications the automatic merge introduced outside the
conflict markers: a second gpt_diagnostics::finalize_response call, duplicate
definitions in response_privacy.rs and creative_opportunities.rs, and a
repeated paragraph in the APS guide.
prk-Jr added a commit that referenced this pull request Aug 26, 2026
The single conflict is in `handle_publisher_request`, where both sides changed
what happens between the auction dispatch and the origin fetch.

This branch moved origin-request preparation above the auction dispatch so the
origin fetch overlaps the EC identity-graph read. Main (#1008) added the shared
template cache in the space that move vacated: request-derived cacheability
flags, the pre-fetch cache key, and a `lookup_or_reserve` whose hit returns
`AssembleTemplate` without ever contacting the origin.

Resolution splits main's block along the line the move drew. Everything that
reads the request — `request_had_authorization`, `cookie_disqualifies`, the
DataDome suppression flags, reader-compression negotiation,
`request_can_use_shared_template`, and the cache key itself — moves up beside
this branch's origin preparation, because that is now the last point where the
request is in hand. `configured_assembly_mode` moves with them since the key
depends on it. The URI and Host rewrite stays folded into this branch's
`rewrite_origin_request` helper rather than being reinstated inline. The lookup
and its hit handling stay where main put them: they need `dispatched_auction`,
which the hit hands to the assembler.

The two features then contend over one request. A template hit skips the origin
entirely, and an in-flight request cannot be recalled, so the concurrent origin
start is gated on `template_cache_key.is_none()`. A request eligible for a
shared template waits for the lookup instead of speculatively fetching an origin
response a hit would discard. Every other request — including all traffic in the
default `Inline` assembly mode, where the key is always `None` — keeps the
overlap unchanged. The cost is one lost overlap on an ESI-mode cache miss, which
is the correct trade against paying for a discarded fetch on every hit.

Test fallout from main, resolved in kind rather than by reverting either side:

- `handle_publisher_request` gained `edge_header: EdgeCacheHeader` (#860); this
  branch's three scheduling-probe call sites now pass `SurrogateControl`.
- Main renamed the `private, no-store` cache-control value to `no-store,
  private`. The probe now asserts against
  `cache_policy::NO_STORE_PRIVATE_CACHE_CONTROL` so it cannot drift again.
- Main removed the `queue_cacheable_html_response` fixture along with its own
  callers. This branch's surviving caller moves to main's replacement,
  `queue_html_response_with_cache_control`, instead of reviving a duplicate.
- `PublisherResponse::AssembleTemplate` is new; the probe's response matches
  cover it.
- Main added an `/auction` test that predates this branch's `&mut EcContext`
  signature.

Verified: fmt, all six clippy gates, core (2269), fastly under Viceroy, axum,
cloudflare, spin, parity, CLI, openrtb-codegen, JS (888), and both
`scripts/template-cache-local-test.sh` modes — esi 21/21 including a warm hit
that skips the origin, inline 8/8.
prk-Jr added a commit that referenced this pull request Aug 27, 2026
Main added the dedicated `[creative_opportunities].enabled` template switch
(#1008) with its own publisher-local ad-stack gate, while this branch moved the
same gate into core as `evaluate_ad_stack_gate`. Resolve in favor of the shared
gate and give it the new switch, so the CLI diagnostics keep reporting the same
verdict the runtime reaches:

- Drop `ServerSideAdStackConfig`/`should_run_server_side_ad_stack` and route the
  publisher call site through `evaluate_ad_stack_gate` with `ad_templates_enabled`.
- Keep `is_server_side_ad_eligible_navigation`; the inactive-template cache
  policy needs the request-only half of the gate.
- Add `AdStackGateName::AdTemplatesEnabled` and widen the exhaustive gate tests
  to the eighth gate, absorbing the coverage of main's deleted unit test.
- Feed the switch from both CLI gate call sites, add the `Gates` JSON field, the
  `explain` gate row, and the `lint` switch line and status.
@aram356
aram356 deleted the issue-1007-cache-control branch September 16, 2026 19:13
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.

Improve cache header for html content when SSAT is off

3 participants