Skip to content

Implement configurable cache header policies - #860

Merged
ChristianPavilonis merged 13 commits into
mainfrom
refactor/cache-headers
Aug 21, 2026
Merged

Implement configurable cache header policies#860
ChristianPavilonis merged 13 commits into
mainfrom
refactor/cache-headers

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Standardizes cache policy rendering across Fastly, Cloudflare, CDN, and s-maxage fallback headers.
  • Adds safe, configurable caching for hash-validated TSJS, publisher-origin static assets, and rehosted asset proxy responses.
  • Hardens privacy handling so private, no-store, and cookie-bearing responses strip shared edge-cache headers.

Changes

File Change
crates/trusted-server-core/src/cache_policy.rs Adds typed cache-policy rendering for browser and edge headers, including no-store/private cleanup.
crates/trusted-server-core/src/settings.rs Adds cache.asset_rules config, matchers/presets, validation, runtime prep, and path-to-policy resolution.
trusted-server.example.toml Documents disabled operator-controlled static/fingerprinted asset cache-rule examples.
crates/trusted-server-core/src/http_util.rs Routes static ETag responses through the cache-policy renderer.
crates/trusted-server-core/src/tsjs.rs Uses exact module-set hashes when available and avoids unverifiable fallback hashes.
crates/trusted-server-js/Cargo.toml Makes hashing dependencies available to the build script.
crates/trusted-server-js/build.rs Generates per-module SHA-256 metadata for bundled JS modules.
crates/trusted-server-js/src/bundle.rs Exposes per-module hashes and caches concatenated bundle hashes.
crates/trusted-server-core/src/publisher.rs Applies hash-validated immutable TSJS caching and configured publisher asset cache rules.
crates/trusted-server-core/src/proxy.rs Applies normalized cache policies to rehosted asset proxy responses and reapplies them after finalization.
crates/trusted-server-core/src/response_privacy.rs Removes shared-cache headers from private/no-store/cookie-bearing responses.
crates/trusted-server-core/src/integrations/prebid.rs Makes the neutralized Prebid shim no-store, private instead of long-lived public cache.
crates/trusted-server-core/src/integrations/testlight.rs Updates the default TSJS fallback comment/source behavior for registry-free configuration.
crates/trusted-server-core/src/lib.rs Exports the new cache_policy module.
crates/trusted-server-adapter-axum/src/app.rs Passes the portable s-maxage fallback edge header mode to TSJS and publisher handlers.
crates/trusted-server-adapter-cloudflare/src/app.rs Passes the Cloudflare-specific CDN cache header mode to TSJS and publisher handlers.
crates/trusted-server-adapter-fastly/src/app.rs Passes Fastly Surrogate-Control mode through EdgeZero fallback dispatch.
crates/trusted-server-adapter-fastly/src/main.rs Reapplies asset cache policies with Fastly Surrogate-Control at the shared finalization point used by both legacy and EdgeZero flows.
crates/trusted-server-adapter-fastly/src/route_tests.rs Updates route-test finalization for selected edge cache headers.
crates/trusted-server-adapter-spin/src/app.rs Passes the portable s-maxage fallback edge header mode to TSJS and publisher handlers.
docs/superpowers/specs/2026-07-06-cache-control-header-design.md Adds the cache-control design scope and deferred dynamic caching notes.
docs/superpowers/plans/2026-07-06-cache-control-header-implementation-plan.md Adds the implementation and verification plan for cache-header work.

Closes

Closes #293

Follow-ups:

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
  • Other: cargo clippy-cloudflare && cargo clippy-spin-native && cargo clippy-spin-wasm
  • Other: cd crates/trusted-server-js/lib && node build-all.mjs
  • Other: git diff --check
  • Other: npx prettier --check docs/superpowers/plans/2026-07-06-cache-control-header-implementation-plan.md docs/superpowers/specs/2026-07-06-cache-control-header-design.md

Note: cd docs && npm run format failed because docs-local Prettier was not installed; touched docs were checked with npx prettier instead.

Checklist

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

Cache-delivery notes

  • Registry-free Testlight defaults omit ?v= because configuration defaults do not know the enabled module set. Those URLs use the normal short TTL after a deploy.
  • On Fastly, static edge TTLs now use Surrogate-Control. Cache-Control no longer repeats s-maxage, but the configured TTL is unchanged.

@ChristianPavilonis
ChristianPavilonis changed the base branch from main to server-side-ad-templates-impl July 7, 2026 17:59
Base automatically changed from server-side-ad-templates-impl to main July 7, 2026 20:08
ChristianPavilonis

This comment was marked as low quality.

@ChristianPavilonis
ChristianPavilonis marked this pull request as ready for review July 8, 2026 19:00

@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

Configurable, safe-by-default cache-header policies across all four adapters. Cache policy is expressed once as typed data (CachePolicy / EdgeCacheHeader) and rendered per-runtime; hash-gated immutability, privacy stripping, and operator-controlled asset rules are all well-tested. No blocking issues — logic is sound and correctly platform-scoped. Findings below are all non-blocking.

Verified during review:

  • Hash-gated immutability is safeserve_tsjs_static marks a response immutable (1yr) only when the request ?v= equals the hash of the content actually being served, so a stale URL after a redeploy falls back to the short TTL rather than pinning old content.
  • Injection ↔ serving hash consistency — HTML injection (html_processor.rs) and the serving path (publisher.rs) both derive the hash from js_module_ids_immediate() + concatenated_hash; deferred modules use single_module_hash on both sides.
  • Privacy hardeningprivate / no-store / cookie-bearing responses strip all four edge-cache headers, with the downgrade re-run after operator headers are applied.
  • Origin no-store not upgraded — a split later Cache-Control field carrying no-store correctly blocks the normalized upgrade.
  • Asset-proxy finalization is correctly Fastly-onlyhandle_asset_proxy_request / apply_after_route_finalization are wired only on Fastly, so there is no missing-reapplication gap on Cloudflare / Axum / Spin.

Non-blocking

🤔 thinking

  • Hex-only fingerprint heuristic: filename_contains_hash misses base62/base36 bundler hashes (false negative → silently uncached) and can match coincidental hex stems (false positive → stale). (settings.rs)
  • Normalized policy overrides origin no-cache/Vary: upgrade gate checks only private/no-store. (publisher.rs)

🌱 seedling

  • handle_publisher_request is now at the 7-argument CLAUDE.md limit; EdgeCacheHeader is threaded through several signatures — consider a request-context struct. (publisher.rs)

⛏ nitpick

  • SURROGATE_CACHE_HEADERS re-export is now a misnomer (contains CDN headers) with no in-tree consumers. (response_privacy.rs:21)

📝 note

  • #[validate(nested)] on cache is a no-op; real validation lives in prepare_runtime. (settings.rs)

👍 praise

  • Directive-exact Cache-Control matching closes the old substring-match privacy hole. (cache_policy.rs)

CI Status

  • fmt: PASS
  • clippy (fastly / axum / cloudflare / cloudflare-wasm / spin-native / spin-wasm): PASS
  • rust tests (fastly / axum / cloudflare / spin / CLI / parity): PASS
  • js tests (vitest): PASS
  • docs / typescript format: PASS
  • CodeQL + integration/browser tests: PASS

Comment thread crates/trusted-server-core/src/settings.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/response_privacy.rs Outdated
Comment thread crates/trusted-server-core/src/settings.rs
Comment thread crates/trusted-server-core/src/cache_policy.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

Solid, well-shaped abstraction — expressing cache policy as typed data and rendering it per-runtime is the right call, and the multi-value Cache-Control handling (get_all + directive-name-exact matching, so no-storey / not-private don't false-match) is careful work.

Four blocking issues, though. The most important is that the rehosted-asset path will override an origin's explicit no-store, using a guard that this very commit wrote for the publisher path but didn't wire into the asset proxy. The other three are a missing immutable safety check, a fingerprint heuristic that can't match the two most common bundlers, and a docs/behavior mismatch on disabled rules that can hard-fail startup.

Findings below were verified by running the code or by an adversarial pass. Four other hypotheses I chased (a missing GET/HEAD gate on asset routes, an EC-cookie shared-cache leak on Fastly, "zero caching" from the hash gate, and a broad TSJS regression) all turned out to be false and are deliberately not reported.

Blocking

🔧 wrench

  • Asset-proxy rehost overrides an origin no-store / privateproxy.rs:1173. Only the status is checked; publisher.rs:470 guards this correctly for the same feature. The PR's own test (proxy.rs:3755) feeds an origin no-store and asserts it becomes public, max-age=31536000, immutable. No downstream rescue: the Set-Cookie backstop can't fire because the asset proxy strips set-cookie.
  • Normalized re-publicizes after privacy hardeningproxy.rs:127. The same root cause at a second layer. apply_after_route_finalization used to only ever make responses more private, so running it last was safe; the new Normalized arm makes them more public and still runs last. This inverts the invariant in the plan doc (L56-57): hardening "runs after any new policy application". Both sites need the fix.
  • immutable = true accepted with no fingerprint requirementsettings.rs:1983. requires_hash_in_filename defaults to false, so path_prefix = "/assets/" + immutable = true puts a non-revalidatable year-long policy on an unfingerprinted /assets/app.js. Contradicts the plan doc (L47-49): "immutable only for TS-fingerprinted rehosted URLs".
  • Hex-only fingerprint gate never matches Vite or esbuildsettings.rs:2204, with the reachable trap at configuration.md:1041. Verified against real builds: Vite 8 emits /assets/index-DA15JTLU.js (base64url), esbuild /assets/app-VRTVD5R5.js (base32). /assets/ is Vite's default output dir — exactly what the enabled = true docs example globs. And it isn't a clean no-op: ~0.02% of Vite hashes are all-hex by chance, so the rule fires on ~1 in 5,000 assets, varying per build.
  • "Disabled rules are ignored" is falseconfiguration.md:998. prepare_runtime validates every rule regardless of enabled. Confirmed by execution: a disabled rule with a bad regex, or a disabled placeholder with no matcher, both fail startup — which per line 54 of the same page means the service returns its startup-error response. This path has no test, which CLAUDE.md's reviewer checklist explicitly asks for.

❓ question

  • What consumes edge_ttl_seconds on Fastly today?configuration.md:1014. Fastly's read-through cache stores the backend's response and decides TTL from the backend's headers at send(); this PR rewrites headers on egress, after that decision. Caching a Wasm-synthesized response needs an explicit Core/Simple Cache call, and the repo has zero uses of fastly::cache / CacheOverride / SimpleCache. Is there a service-layer piece outside the repo? To be fair: the Surrogate-Control emission predates this PR, so it's not a regression here — but this PR is what turns it into a documented operator knob.
  • Is spec acceptance criterion #138 handled at the service layer? The design doc requires "Runtime cache-key configuration preserves the v query parameter for /static/tsjs=". This matters more now: the same path serves either a 1-year immutable response (matching ?v=) or a 300s one (bare/mismatched), discriminated only by query string — and this PR makes the bare URL a real, emitted URL for the first time. If any shared cache normalizes the query away, those two cross-contaminate. There's no cache-key config in fastly.toml / edgezero.toml, and the operator docs never mention the requirement. All 13 acceptance criteria in the shipped spec are still unchecked.

Non-blocking

🌱 seedling

  • Cloudflare is ~2 lines from actually working. Cloudflare's Workers Cache (GA 2026-07-06) documents cloudflare-cdn-cache-control as its highest-precedence cache directive — exactly what this PR emits. But it's opt-in, and neither wrangler.toml nor wrangler.ci.toml has a [cache] block, so the header is inert today. Adding [cache]\nenabled = true (Wrangler ≥ 4.69.0) would turn EdgeCacheHeader::CloudflareCdnCacheControl from a no-op into a fully effective directive — plausibly the highest-ROI change available here. (compatibility_date = "2024-09-23" is also stale.)
  • tsjs_unified_script_src() dropped ?v=tsjs.rs:30. Bounded ~6-minute post-deploy staleness on the ad-creative path only. Details inline; suggest a follow-up issue rather than expanding this PR.

📌 out of scope

  • The runtime half of this belongs in edgezero, not trusted-server-core. Worth a follow-up issue, not a change to this PR.

    EdgeCacheHeader encodes a purely platform fact — which shared-cache header does this runtime speak. The tell is that all four adapters hand-thread a per-adapter constant (SurrogateControl / CloudflareCdnCacheControl / SMaxageFallback) into handle_tsjs_dynamic and handle_publisher_request. The adapter already knows its own runtime; it shouldn't have to tell core what platform it is. That plumbing is also what pushed handle_publisher_request to exactly 7 parameters, CLAUDE.md's stated ceiling.

    More importantly, the part that would make edge_ttl_seconds actually work can only be built in edgezero. edgezero-core currently has no cache concept at all, and edgezero-adapter-fastly/src/proxy.rs:31 sends upstream with send_async_streaming(&backend_name) and no CacheOverride — so the store/TTL decision for proxied responses is made inside edgezero, before this PR's egress-time header rewrite ever runs. Trusted Server cannot fix that from where it sits.

    A split that seems right:

    • edgezeroEdgeCacheHeader and the edge-header-name registry; the CachePolicy → platform headers render step (ideally behind an adapter method like apply_cache_policy(&policy, &mut resp), so the parameter disappears from core signatures entirely); CacheOverride on backend sends; the Core/Simple Cache API for synthetic responses; the wrangler [cache] block.
    • trusted-serverCachePolicy as typed domain data, the cache.asset_rules config surface and matching, and the response_privacy invariants (which would consume edgezero's header registry rather than own it).

    None of this blocks the PR — the browser-facing half is real and useful today. But it does mean the edge half is currently a promise the runtime layer can't keep, which is worth being explicit about before operators configure edge_ttl_seconds expecting shared-cache behavior.

♻️ refactor

  • SURROGATE_CACHE_HEADERS has zero consumersresponse_privacy.rs:21. Dead re-export, and now a misnomer since it includes the CDN headers. Delete it.

🤔 thinking

  • No validation that a rule sets any TTL. visibility = "public" with neither browser_ttl_seconds nor edge_ttl_seconds renders a bare Cache-Control: public, which hands the response to heuristic freshness. Probably worth rejecting at config load.
  • The cache-rule path is completely silent. There isn't a single log:: statement in settings.rs:1890-2215 or in cache_policy.rs, and the application site is a bare if let with no else. A rule that matches nothing — the hash-gate case above, for instance — is undebuggable in production. A log::debug! on gate rejection would pay for itself.

📝 note

  • #[validate(nested)] on Settings.cache is a no-op. CacheSettings declares no field validators and CacheAssetRule doesn't derive Validate, so the attribute does nothing today. Harmless, but it reads as protection that isn't there.
  • The PR description says asset cache policies are reapplied "in legacy and EdgeZero flows", but there's only one call site (main.rs:206).

CI Status

All 19 checks green at a5eb7a3, verified via gh pr checks:

  • fmt: PASS
  • clippy (fastly / axum / cloudflare / cloudflare-wasm / spin-native / spin-wasm): PASS
  • rust tests (fastly, axum native, cloudflare, spin, cross-adapter parity, ts CLI): PASS
  • js tests (vitest) + format-typescript + format-docs: PASS
  • integration + browser integration + CodeQL: PASS

Comment thread crates/trusted-server-core/src/proxy.rs
Comment thread crates/trusted-server-core/src/proxy.rs
Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/response_privacy.rs Outdated
Comment thread crates/trusted-server-core/src/tsjs.rs
@aram356

aram356 commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

@ChristianPavilonis Please resolve conflicts

@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

Solid, well-tested slice: the typed policy renderer, the per-runtime edge-header mapping, and the exact-directive Cache-Control parsing all improve on what they replace, and the privacy hardening closes a real gap (a late private directive coexisting with an authoritative edge header). Docs are unusually thorough for a config surface this large.

One blocking issue: requires_hash_in_filename is the only safety gate validate_policy_shape accepts for immutable = true, and the Vite/Base64URL branch of that gate accepts ordinary mixed-case filenames. With the documented /assets/**/*.png example rule enabled, a hand-named publisher image gets a one-year immutable policy that no republish can invalidate. Details inline.

Blocking

🔧 wrench

  • Filename fingerprint gate accepts ordinary mixed-case filenames: is_vite_base64url matches any 8-character alphanumeric suffix with at least one uppercase and one lowercase/digit character, so logo-DarkMode.svg, hero-Portrait.jpg, icon-Facebook.svg, banner-Summer24.png and photo-Iceland1.jpg all pass. Because immutable implies no revalidation, a mutable asset that trips this gate is stuck for a year. (crates/trusted-server-core/src/settings.rs:2265)

Non-blocking

🤔 thinking

  • Neutralized Prebid shim now costs a request per page view: public, max-age=31536000no-store, private on a 43-byte stub served from the publisher's (often blocking) prebid.js URL. (crates/trusted-server-core/src/integrations/prebid.rs:682)
  • Rehost path overrides a third-party origin's no-store while the publisher path vetoes on it — the asymmetry is deliberate and documented, but it leans entirely on the fingerprint gate flagged above. (crates/trusted-server-core/src/proxy.rs:1195)
  • Glob * crosses /: glob::Pattern::matches defaults to require_literal_separator: false, so path_globs = ["/assets/*.js"] also matches /assets/a/b/c.js. Every doc example uses **, so the difference is invisible to operators. (docs/guide/configuration.md:1046, crates/trusted-server-core/src/settings.rs:2123)

♻️ refactor

  • cache_rule_method: bool: opaque boolean parameter, and is_get is computed one line earlier from the same method. (crates/trusted-server-core/src/publisher.rs:1083)

📝 note

  • Hash-cache doc comment overstates the win: Fastly Compute builds a fresh Wasm instance per request, so the Mutex<HashMap> never survives a page view there. The real improvement is hashing without allocating the concatenated body. (crates/trusted-server-js/src/bundle.rs:39)

🌱 seedling

  • EdgeCacheHeader::CdnCacheControl is unused in production: no adapter selects the standards-track variant; it stays dead until another runtime needs it.
  • Six planned PRs land as one: the plan doc sequences PR 1–6 and this change implements all of them (~3.1k insertions across 4 adapters, core, and the JS build). Nothing to do now, but bisecting a future cache regression inside this commit range will be painful.

👍 praise

  • ?v= hash-match gate for immutability: immutable is granted only when the request's v equals the hash of the current module set, so a mid-rollout request for a new hash landing on an old instance falls back to the 300s policy instead of pinning wrong content under that key for a year. (crates/trusted-server-core/src/publisher.rs:339)
  • Exact directive matching: replacing contains("private") kills the not-private / no-storey false-positive class, and get_all covers split Cache-Control fields — publisher_asset_cache_policy_respects_split_no_store_origin_header locks that in. (crates/trusted-server-core/src/cache_policy.rs:291)
  • enforce_uncacheable_cache_privacy: closes the window enforce_set_cookie_cache_privacy alone missed, where a late private/no-store directive coexists with an independently authoritative edge header. (crates/trusted-server-core/src/response_privacy.rs:29)

CI Status

All 19 check runs pass on head f2382c1:

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

Separately verified locally that [cache] enabled = true is a recognized Wrangler 4.83 config key — a control run with an invented section produces Unexpected fields found in top-level field, while the checked-in manifests parse clean.

Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/prebid.rs
Comment thread crates/trusted-server-core/src/proxy.rs
Comment thread docs/guide/configuration.md Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-js/src/bundle.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/cache_policy.rs
Comment thread crates/trusted-server-core/src/response_privacy.rs

@ChristianPavilonis ChristianPavilonis left a comment

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.

Note: this PR will need to be reconciled with the changes introduced by #1008 before merge. #1008 also changes publisher template delivery and cache behavior, including the new [creative_opportunities].enabled switch and the inactive-template max-age=60 policy, while this PR centralizes cache-policy application in publisher.rs and settings.rs. When combining the changes, please ensure the new switch and inactive-template behavior flow through the centralized policy renderer without overriding the privacy or CDN-specific headers established here.

@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

The typed cache-policy work is generally well structured, but two enabled configuration paths either expose the whole gateway to shared caching or silently discard an operator-supplied TTL.

Blocking

🔧 wrench

  • Cloudflare cache covers the entire gateway: Global cache.enabled lets ordinary dynamic publisher responses enter Workers Cache (crates/trusted-server-adapter-cloudflare/wrangler.toml:10).
  • Private edge-only asset rules drop their only TTL: An enabled visibility = "private" rule can accept edge_ttl_seconds although the renderer never emits it (crates/trusted-server-core/src/settings.rs:2058).

CI Status

  • GitHub fmt, Rust checks/tests, JS tests, CodeQL, and integration/browser checks: PASS
  • git diff --check: PASS

Comment thread crates/trusted-server-adapter-cloudflare/wrangler.toml Outdated
Comment thread crates/trusted-server-core/src/settings.rs
@aram356 aram356 added this to the 202608 milestone Aug 17, 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

The cache-header work itself is in good shape: the policy rendering, the operator-facing rule schema, and the privacy hardening all read cleanly, and both blocking findings from the previous round are genuinely closed — private rules now reject edge_ttl_seconds, and the global [cache] enabled = true block is gone from both Wrangler manifests with a cookie-boundary runtime regression behind it.

The blocker is unrelated to caching. origin/main is an ancestor of this branch, yet the diff removes 2,354 lines from publisher.rs, including production code from several already-merged PRs. Reviewed at fd6d8324 against origin/main at f6a2fb85.

Blocking

🔧 wrench

  • Branch reverts merged main work in publisher.rs: 563 added / 2,354 deleted against a base that is literally the current main tip. hb_auction_id diagnostics targeting (#974), the non_empty / GAM_TARGETING_VALUE_MAX_LEN hb_adid hardening (#996), the has_renderer creative-rejection guard and PBS-cache fallback gating (#956), and real delivered_winner_slots telemetry (#997) are all gone; 22 tests were deleted with them, which is why CI is green. Details and the suggested merge strategy are inline at crates/trusted-server-core/src/publisher.rs:2517.
  • Duplicate registry API: IntegrationRegistry::is_enabled is a byte-identical copy of the pre-existing integration_enabled, which is left with zero call sites (crates/trusted-server-core/src/integrations/registry.rs:1140).

Non-blocking

🤔 thinking

  • Publisher asset policy never inspects Set-Cookie: correct today only because all four adapters run the privacy pass afterwards, but nothing tests that ordering (crates/trusted-server-core/src/publisher.rs:1100).

📝 note

  • Fastly static responses drop s-maxage: edge TTL moves to Surrogate-Control only; Axum/Spin retain it via SMaxageFallback. Same effective TTL, but an undocumented header-shape change on every /static/tsjs= response (crates/trusted-server-core/src/http_util.rs:283).

⛏ nitpick

  • parse_deferred_module_filename name/doc: it also resolves the non-deferred diagnostics module (crates/trusted-server-core/src/publisher.rs:366).

👍 praise

  • Quoted-string-aware Cache-Control directive matching (crates/trusted-server-core/src/cache_policy.rs:314).
  • Private-visibility TTL validation and the Wrangler cache fix, both with regression coverage (crates/trusted-server-core/src/settings.rs:2063).

CI Status

All 19 GitHub checks pass on fd6d8324.

  • fmt: PASS
  • clippy: PASS (covered by the per-adapter check/build jobs)
  • rust tests: PASS (fastly, axum native, cloudflare, spin, ts CLI, cross-adapter parity)
  • js tests: PASS (vitest, format-typescript)
  • docs format: PASS
  • integration/browser suites: PASS

Note that the passing suite does not cover the reverted publisher.rs behavior — those tests no longer exist on this branch.

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/registry.rs Outdated
Comment thread crates/trusted-server-core/src/http_util.rs
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/cache_policy.rs
Comment thread crates/trusted-server-core/src/settings.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 cache-header work itself is well designed: cache_policy.rs is thoroughly unit-tested, the operator-controlled asset-rule engine validates eagerly at startup with a safe disabled-by-default posture, the neutralized Prebid shim moving to no-store, private closes a real caching hole, and the exact-directive Cache-Control parsing in response_privacy is more correct than the substring matching it replaces. However, the rebase that produced the base commit (d7737d3) carried a stale pre-rebase copy of publisher.rs, silently reverting several features that landed on main after this branch was cut. CI is green only because the rebase deleted the regression tests together with the features, so the suite cannot see the reverts.

Blocking

🔧 wrench

All five inline 🔧 comments share one root cause: publisher.rs was clobbered back to its pre-rebase state. The reverted main features are:

  • #945/#997 hydration-deferred adInit: build_bids_script calls adInit() synchronously again (publisher.rs:3505), re-introducing the React #418 hydration mismatch; the JS scheduleInitialAdInit scheduler is now dead code.

  • #996/#998 hb_adid chain + APS renderer carrier: bid map no longer serializes renderer and loses the non_empty precedence and GAM 40-char guard (publisher.rs:3393).

  • Sanitization bypass: hb_cache_host/hb_cache_path are emitted unconditionally, letting the Prebid Universal Creative fetch the original unsanitized adm from PBS Cache for creatives TS rejected (publisher.rs:3406).

  • #997 GPT render attribution: hb_auction_id is no longer minted while the GPT bundle still consumes it; delivered_winner_slots telemetry is hardcoded to None on all three auction paths (publisher.rs:2517).

  • #918 page-URL sanitization: raw client query strings now leak into page_url/site.page toward SSPs (publisher.rs:3285).

  • Deleted regression tests without replacement (~1,900 lines): the whole ssat_cache_policy_tests module (navigation cache bypass, unexpected-origin-304 fail-closed, conditional/Range header handling), the DataDome suppression pipeline tests, the over-limit dynamic GAM slot tests (build_slot_json reverted from returning Option), and the auction-id tests. The SSAT runtime behaviors themselves survived, so for those it is test coverage that regressed, but the five items above are genuine behavior reverts.

Suggested fix as one operation instead of five: restore main's publisher.rs (git checkout origin/main -- crates/trusted-server-core/src/publisher.rs), then re-apply only this PR's genuine changes: the edge_header parameter on handle_tsjs_dynamic/handle_publisher_request, serve_tsjs_static + request_version_hash, apply_publisher_asset_cache_policy and its call site, and the earlier apply_datadome_client_tag_cache_privacy call-site move. Then diff publisher.rs against main once more to confirm only cache-related hunks remain.

Non-blocking

🤔 thinking

  • Multi-** glob expansion gap: the **/-stripping recursion never generates mixed combinations for patterns with two recursive segments (settings.rs:2221).
  • No content-type gate on publisher asset cache policy: a broad operator glob can mark TS-rewritten HTML publicly immutable; docs warn, a text/html guard would prevent it outright (publisher.rs:1100).

♻️ refactor

  • Duplicate registry API: new is_enabled duplicates the existing integration_enabled (registry.rs:1140).

🌱 seedling

  • Normalized asset policy portability: the runtime edge header depends on a Fastly-only finalization re-apply; a comment on the variant would keep future adapters from silently dropping the edge directive (proxy.rs:118).

📝 note

  • Unversioned unified TSJS fallback: testlight's default shim src loses cache busting; staleness is bounded by the 5-minute TTL, worth noting in the PR description (tsjs.rs:29).

CI Status

  • GitHub checks: all pass (fmt, clippy, cargo test across adapters, cross-adapter parity, vitest, CodeQL); prepare integration artifacts still pending at review time.
  • Local verification in a clean worktree at 7c865ce: cargo fmt --all -- --check PASS, cargo clippy-fastly PASS, cargo test-fastly PASS.
  • Note: green CI does not cover the reverts above because their tests were deleted in the same rebase.

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 Outdated
Comment thread crates/trusted-server-core/src/integrations/registry.rs Outdated
Comment thread crates/trusted-server-core/src/settings.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/tsjs.rs
Comment thread crates/trusted-server-core/src/proxy.rs
@ChristianPavilonis

Copy link
Copy Markdown
Collaborator Author

Resolved the merge conflict against current main, restored the upstream publisher behavior called out in the review, and replied to/resolved all review threads. Re-requesting review from @aram356 and @prk-Jr on head 9c1b236e.

Comment thread crates/trusted-server-core/src/settings.rs
@ChristianPavilonis
ChristianPavilonis merged commit 42a34ae into main Aug 21, 2026
19 checks passed
@aram356
aram356 deleted the refactor/cache-headers branch August 22, 2026 06:20
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.

Refactor and standardize how Trusted Server sets cache response headers

3 participants