Conversation
Records what remains of #852 after #1009 shipped the transformed-template cache, and specifies the five remaining pieces: making origin readthrough deliberate, an origin shareability probe, operator and CMS purge surfaces, cache observability, and the documentation to match. Two findings from review are recorded as corrections rather than folded away. The readthrough cache is already shared for every non-ad-stack request, so the change makes an existing sharing decision deliberate rather than opening a new one. And stripping TS-owned cookies would not raise the hit rate, because the gate keys on Cookie header presence rather than cookie identity. The readthrough change is left with an explicit ship/no-ship decision. Its safety rests on probe-verified operator preconditions rather than enforced checks, because the decision is made before the origin responds and no post-response hook is reachable on this adapter.
First of five PRs for issue #852. Ships no behavior change: it extracts the two eligibility predicates as pure functions, splits origin shareability out of template eligibility, and reports both caches' outcomes on the existing auction telemetry row. Plan review found that the bypass reason the spec treats as the primary triage signal cannot be produced where the spec said. template_cache_ttl runs only for requests that already earned a cache key, so its InlineMode, AuthorizedRequest and CookieForwarded variants are unreachable there, and the request-side bypass carries no structured reason at all. The spec is updated to record this and the plan budgets the missing derivation as its own task.
Two independent reviews checked the plan against the repository and against the spec. Five findings would have left the implementer writing code that does not compile or chasing pre-existing problems. StubHttpClient has no Default and run() takes &Arc<Settings>, so every test body in the plan was wrong. RecordingTelemetrySink has no accessor and three copies, one already reachable from the target module, so the proposed move was unnecessary. The fixture is missing user_agent today, so the plan's own verifier script failed before any change. Test filters named a module path that does not match, which reports zero tests as success rather than failure. An earlier correction of mine was itself wrong: the take sites at :4957 and :4996 run after the state write, not before, so naming them as known-None paths would have made the comment false. The spec is corrected where the plan disproved it: the carrier needs no stash variable, the Hit state hook is reachable only at :4617, and the fields are wired in base() rather than the summary row alone. Adds the approval gate the spec requires before this PR, its trim fallback, and a task for the two dashboard caveats.
Tasks 2 and 3 both use it and neither module has one; they must build the context identically or the two tasks' assertions diverge.
The work was specced as five sequential pull requests. It ships as one change set instead, so the sequencing section becomes commit order on one branch and the plan becomes three parts of one plan. Records what the single pull request costs: the readthrough gate is the only change with new blast radius, and it now reverts together with the telemetry that would say whether to revert it. Mitigated by the gate being inert until an operator opts in, and by assembly_mode remaining a runtime kill switch, so the practical rollback is a configuration change rather than a revert. Also records the decision that the readthrough gate ships, which an earlier revision left open, and asks that it be reviewed as its own commit against the precondition list rather than buried in the wider diff.
Completes the plan for issue #852 as a single pull request. Part 2 builds the shareability probe and the purge surface; part 3 changes the bypass condition. Part 3 records three things as settled so they are not re-litigated during implementation: no TTL override, because set_ttl reverses set_pass and overrides the origin's own private and no-store; after_send is unreachable because Viceroy stubs the HTTP Cache ABI; and set_pass and set_surrogate_key are mutually exclusive and order-dependent, so the platform layer models cache intent as one enum rather than two flags. The probe's verdicts are blocking rather than advisory. The gate is decided before the origin responds and no response-side hook is reachable, so none of the template cache's refusals apply to that path and the probe is the only control.
Adds three absent-by-default fields to the observation context and to the event row, wired in AuctionEventRow::base so provider and bid rows carry them too rather than the summary alone. Absent is deliberately distinct from false. A row from a source that does not make the readthrough decision reports None, and a dashboard that reads that as a cache miss will be wrong for every /auction row. Nothing writes the fields yet; the publisher path wiring follows.
The row serializer has no skip_serializing_if, so the three new fields are always on the wire including as null. Undeclared columns are quarantined rather than rejected loudly, so this must reach Tinybird before the emitting code deploys. Also adds user_agent to the fixture rows. It was declared in the datasource and missing from every row beforehand, so the fixture did not match the schema it is meant to exercise.
…ry sink Neither existing builder wires both, so cache-outcome telemetry had no way to be asserted end to end. Includes a self-test: a summary row is only emitted when an auction runs, and without one every assertion built on this harness would pass vacuously.
The single predicate mixed two questions: whether the origin response may be shared at all, and whether this pipeline can assemble a shared template. Gating anything but the template cache on the combined form would couple origin readthrough caching to the assembly mode for no safety reason, and would make assembly_mode = "inline" silently change caching behaviour. Extracted as pure functions so the invariant is testable against real code rather than a re-typed copy of the expression. One test asserts template eligibility still implies shareability across all 128 input combinations; another asserts each shared condition is individually necessary, which is what catches a dropped term. Behaviour is unchanged: nothing consumes the new binding except telemetry. The gate that will consume it is a later commit.
The bypass reason has two sources and only one existed. template_cache_ttl runs inside template_cache_reservation.and_then, and a reservation exists only when a key was built, so its InlineMode, AuthorizedRequest and CookieForwarded variants are structurally unreachable there. The request-side bypass set a response state and free-text logs and nothing else. That put the single most useful triage value on the unreachable side: cookie-disqualified is the expected default in production, because Trusted Server sets its own identity cookie. Adds request_side_bypass_reason to derive it, reusing the existing variants and matching the response-side ordering so one request cannot be described two ways. Adds one variant, NotShareableRequest, covering the four remaining conditions that each already have their own log line and none of which is a cross-serving vector on its own.
The store outcome cannot reach the summary row. On a cold fill the ordering is fixed: stream_publisher_body_async collects the auction, takes the observation and emits the telemetry batch, and only afterwards does store_template_if_authorized run and the state get stamped. The store cannot move earlier because it needs the transform, and the emit cannot move later without giving up collecting during body streaming. So miss-stored and miss-store-error are unreachable while hit is reachable, and a column that records hits but not misses makes hit rate compute as roughly 100 percent. A silently wrong metric is worse than an absent one. template_cache_bypass_reason and origin_cache_shareable carry the triage, and the x-ts-template-cache response header still reports all nine states per response for debugging a single request.
The list omitted the template-cache shell harness, the CLI and openrtb-codegen clippy invocations, the parity crate's fmt and clippy, the bench smoke, the release WASM builds, the JS and docs lint steps, and the entire integration-tests workflow. Points at .github/workflows as authoritative rather than restating it, so the next omission is a stale subset rather than a wrong instruction.
…ndings Adds a Tinybird README covering the deploy ordering and the two ways a query over the cache columns goes wrong: the denominator is ad-serving pageviews rather than all requests, and NULL means not measured rather than false, because the /auction source populates neither column. Records in the spec and plan that template_cache_state was attempted and is unreachable, so nobody tries again without reading why. Adds an RSC axis to the probe. RSC fetches are not navigations, so they never set the bypass and already flow through the readthrough cache while HTML navigations are PASS. Removing the bypass puts both representations under one cache key for the first time, and an origin that varies on rsc or next-router-* without declaring it can serve a flight payload to an HTML navigation. The probe as specced would not have caught it.
Three things had drifted in that the issue does not ask for. The template cache bypass reason diagnoses the template cache's refusals, which is #1009's feature. Origin readthrough has no refusal reasons Trusted Server controls, so the column said nothing about the change this issue makes. Moved to successor issue B, which promotes that cache out of spike status and should instrument it there. Removes the column, its derivation and the enum variant added for it. The CI gate list correction is a genuine docs fix but unrelated to this change; it should land as its own small pull request. The Tinybird README keeps its deploy-ordering section, which is a live hazard this branch's schema migration creates, and drops the guidance for the column that is no longer here. Kept: origin_cache_shareable, which measures exactly what this issue changes. The predicate split stays either way; it is a prerequisite for the gate.
Two independent reviews. The Rust review approved the diff and confirmed the predicate extraction is term-for-term equivalent to what it replaced. The verification review confirmed behaviour-neutrality, the Tinybird three-way schema agreement, and the ordering argument for why a template-cache hit/miss column is impossible. Both found documentation problems. The column description overstated what ships. It said the field reports whether the readthrough gate admitted a request, but no gate exists yet and every ad-serving request still forces an origin fetch. A dashboard author reading it would have concluded readthrough was live. It now says the field records a predicate rather than an outcome, in both the Rust doc comment and the Tinybird README. Reverts a gratuitous hunk in the buffered finalizer. It was shape left over from the telemetry field that was later removed, and behaviour-identical, so it no longer appears in the diff at all. Corrects spec and plan text that still described three telemetry fields, a request-side bypass-reason derivation, a 36-column schema and an AGENTS.md edit, none of which are in this branch any more.
The existing hyper/rustls stack is scoped to macOS, because ts dev proxy needs a native TLS stack that the repo-default wasm32-wasip1 target cannot build. The shareability probe has to run on Linux CI too, so it needs a client in the non-wasm block. reqwest is already a workspace dependency with rustls-tls and is already built natively by the Axum adapter and the integration-tests crate, so this links no new TLS backend.
The existing tests/support module is tokio + tokio-rustls + the dev proxy, all macOS-scoped, and the probe has to be testable on Linux CI too. This one is plain std::net and std::thread. It loop-accepts deliberately. A single-accept fixture caused a CI flake here before, fixed in PR #823: clients open more sockets than they send requests on, and the probe opens one connection per arm and per --repeat, so a one-shot server would hang the second fetch rather than fail it. Self-tests cover the three things later tasks depend on: repeated requests are answered, the fixture can vary its answer per request so the self-identity axis has something to detect, and it sees request headers and cookies so the cookie and user-agent axes can be driven.
Compares an origin's responses across five axes — self-identity, cookie, Accept-Encoding, User-Agent, and RSC router headers — and checks four response-header verdicts: positive shared freshness, no Set-Cookie, no CSP nonce, and Vary coverage of any axis that varied. Every axis and verdict is blocking, and a failure exits non-zero so the command can gate a deploy. That is not caution for its own sake: the origin readthrough gate is decided before the origin responds and no post-response hook is reachable on the Fastly adapter, so none of the template cache's response-side refusals apply to that path. This probe is the only control. Self-identity runs first and is reported separately, because an origin that is unstable against itself would otherwise surface as a failure on whichever axis happened to run next and send the operator after the wrong thing. The RSC axis is the one specific to removing the bypass: RSC fetches are not navigations, so they never set it and already flow through the readthrough cache while HTML navigations are passed. Removing the bypass puts both representations under one cache key for the first time. Output states on every run what the probe cannot see: it runs from one client address, so IP-keyed personalization is undetectable, and a verdict covers the URLs sampled rather than the origin. reqwest is declared directly rather than inherited so its rustls crypto provider can be pinned. Its plain rustls-tls forces ring, while this crate already links aws-lc-rs through reqwest 0.13; compiling both made rustls's process default ambiguous and panicked the dev-proxy tests.
A purge caller knows the page address; the cache key holds the origin-rewritten target URI. Reconstructing one from the other means reimplementing the publisher path's rewrite in every caller, and when that drifts it does not fail — it produces a well-formed key that matches nothing, so the purge returns success and invalidates nothing. That is the worst failure mode on an incident path. Adds request_path to the key, populated before rewrite_origin_request replaces the URI, and a reader-facing surrogate key derived from it. Callers hash the string the operator typed; no origin logic, no reimplementation. The derivation is a free function because neither purge caller can build a whole TemplateCacheKey: they have a URL, not an origin identity, a template fingerprint, or the origin's Vary values. Canonicalizes scheme and host case, default ports, a trailing slash and an empty query, because the digest is over exact bytes and a spelling mismatch is a silent no-op. The query itself is preserved: a different query is a different page. An unparseable URL hashes as given, so an operator typo purges nothing rather than failing the command. Distinct ts-template-readerurl- prefix so the two derivations cannot alias when a staging edge host happens to equal the configured origin host. Schema version 5: the key gained a field, so v4 entries hash differently and must not be read.
purge_url takes a whole TemplateCacheKey, which a purge caller cannot build: an operator or a CMS webhook has a URL, not an origin identity, a template fingerprint, or the origin's Vary values. The new method takes an already-derived key, to be paired with reader_url_surrogate_key. Implemented across all five implementors. The null object used by every adapter without a template cache reports Unsupported rather than succeeding: a purge surface that silently does nothing is worse than one that refuses, because an operator mid-incident would read the success and stop looking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
with_cache_bypass) is not in this branch yet, so merging this changes no request-path behavior on its own.ts origin probe-shareability, the operator-facing evidence gate: five comparison axes and four response-header verdicts, all blocking. Nothing may enable readthrough without it passing first.Why the issue is still open post-ESI
#1009 built the sophisticated caching layer (the template cache). #852 is the crude one, and that is where the measured latency actually sits:
with_cache_bypass()is ~485ms of a 773ms TTFB. The template cache only exists underassembly_mode = "esi", which is off by default — so most deployments today pay the full bypass cost on every ad page and get nothing back from #1009. The two are multiplicative, not redundant.Changes
crates/trusted-server-core/src/publisher.rsSharedRequestInputs,origin_response_is_shareable,request_can_use_shared_templateas pure predicates; capturesrequest_pathbefore origin rewriting; test harness with both a template cache and a telemetry sinkcrates/trusted-server-core/src/auction/telemetry.rsorigin_cache_shareableon the observation context and event rowcrates/trusted-server-core/src/platform/template_cache.rsrequest_pathon the cache key (schema v4 → v5),reader_url_surrogate_keywith URL canonicalization,purge_url_surrogate_keyon the traitcrates/trusted-server-adapter-fastly/src/{app,template_cache,tinybird}.rscrates/trusted-server-cli/src/commands/origin/ts origin probe-shareability— probe, result model, two renderingscrates/trusted-server-cli/tests/crates/trusted-server-cli/Cargo.tomlreqwestwithrustls-tls-webpki-roots-no-provider, so the CLI uses the workspace's existingaws-lc-rsprovider instead of compiling a second onetinybird/origin_cache_shareablecolumn, fixture rows, README covering deploy orderingdocs/superpowers/Refs
Refs #852
Deliberately not
Closes— see "Still to do" below.Still to do on this branch
POST /_ts/admin/cache/purge, with its guard list: registered for all methods with an in-handler 405, because non-primary methods fall through to the publisher andenforce_basic_authleaves theAuthorizationheader in place — aGETwould otherwise ship the admin credential upstream.ts cache purgeCLI command and the harness purge leg.PlatformCacheIntent, the two call sites, thets-originsurrogate key, and the runbook.ts-originkey, which needs a real Fastly service; Viceroy cannot model it.Test plan
cargo test-fastly(2,675 core tests) andcargo test-axumcargo clippy-fastlyandcargo clippy-axumcargo fmt --all -- --check./scripts/test-cli.shfastly compute serve— not meaningful until the gate landsts-originsurrogate key — blocked on a real Fastly serviceNotes for review
Two things worth a reviewer's attention beyond the diff:
with_cache_ttlwas considered and rejected. CallingCacheOverride::set_ttlorset_surrogate_keyreverses a priorset_pass(true)and overrides an origin'sprivate/no-store. Readthrough must be enabled by omittingset_pass, never by layering a TTL over it. This is why every probe verdict is blocking: no post-response hook is reachable on Fastly (Viceroy 0.17 stubs the HTTP Cache ABI), so the probe is the only control on that path.Checklist
unwrap()in production code