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.
The cache key is built from the path exactly as the reader sent it, so `?a=1&b=2` and `?b=2&a=1` can be cached as two entries. Their purge handles were derived the same way, so purging the ordering an operator happened to type left the other entry serving stale content while the command reported success. Sort the raw query pairs before hashing the purge handle. The cache key is deliberately untouched: collapsing the orderings there would risk serving one reader's entry to another, and separate entries sharing one purge handle is the outcome we want. Sorting operates on raw pairs rather than decoded ones so percent-encoded values stay byte-exact, and drops empty pairs, which over-purges by one spelling — the safe direction.
`RequestBuilder::header` appends rather than replaces, so layering an arm's override on top of the default sent `User-Agent` and `Accept-Encoding` twice. An origin that reads the first instance never saw the override, so the user-agent and accept-encoding arms fetched the same document as the baseline and passed an origin nobody had varied. Resolve the headers into one map before building the request. The freshness verdict read only the first `Cache-Control` and `Surrogate-Control` instance. A proxy that appends `private` after the origin's `public, max-age=300` would pass. Judge every instance, as the set-cookie and csp-nonce verdicts already did, and drop the first-instance accessor so nothing reaches for it again. The fixture origin collected headers with `insert`, keeping the last value, which is why the existing user-agent axis test passed against the append bug. It now records every instance and resolves reads to the first, modelling the origin class the bug defeats — that test fails without this fix, as it always should have.
Every axis compares two responses. A cache between the probe and the origin can serve both from one stored object, so all five axes read identical and the report goes green on an origin that personalizes freely on a miss. It is the one failure that invalidates a whole run at once, and nothing detected or disclosed it. Add a blocking `fronting-cache` verdict, judged before the others, on a positive `Age` or a vendor hit header. `Age: 0` passes, since that is what a conforming cache sends on a miss and failing it would make the probe unusable against any origin that reports age. Detected rather than defeated: busting the cache needs either a query parameter, which changes the cache key and the page identity, or a no-cache request header, which can change the origin's own caching and with it the freshness verdict. Perturbing the measurement to rescue it would make a green result mean less. Also state three limits a green report cannot reveal on its own: the cookies are synthetic, only the signals with axes are varied, and a few back-to-back requests cannot see variation on a slower cycle.
Review found the docs and the code had drifted apart in five ways. All 113 checkboxes were unchecked, including for finished work, so nothing distinguished "done" from "not started". Part 1 and part 2's Sections A and B are now checked; part 2's C and D and all of part 3 stay open, which is accurate. Part 1 was written for three telemetry fields and one shipped. Its task bodies still build all three, and its Task 0 trim instruction names the opposite field from the one that was actually cut. Rather than rewrite the code blocks of a completed plan — churn that risks new inaccuracies for work nobody will re-execute — the divergence is stated once at the top, with the reason each field was dropped. The task bodies stay as the record of what was planned; the code is the record of what was built. Part 3 told the operator to watch `template_cache_bypass_reason`, which does not exist. That line is destined for the runbook, so it is fixed rather than annotated. The spec's probe section was one axis behind the code and is now two, since review added the `fronting-cache` verdict. Both the RSC axis and that verdict are now in the spec's tables, and every "four axes" and "four verdicts" reads five. The spec's own Open risks section still framed observability as an unresolved approval question with the outcome tacked on parenthetically, which is what made the shipped column read as surviving scope creep. It now states that the trim was taken, and argues why the one remaining field belongs to #852. Fixture row 0 was an `auction_api` row carrying a cache decision, which the README says is structurally NULL for that source, while rows 1-3 shared its auction_id and disagreed — no real emission can look like that, since base() stamps one observation onto every row. The /auction rows are now NULL and the navigation rows carry both a 1 and a 0.
The five shareability inputs each had a necessity test against hardcoded expectations. The two conditions that make template caching stricter than plain shareability had none of their own — they were covered only by the 128-combination test, which compares the predicate against a restatement of its own body. That comparison does catch either term being dropped, since the reference formula is written out independently, but it says nothing about a bug inside origin_response_is_shareable, and it reports a combination rather than a condition when it fails. Assert both directly, plus that template eligibility cannot outlive origin shareability.
POST /_ts/admin/cache/purge with {"scope":"all"} or
{"scope":"url","url":"..."}. The URL scope hashes the reader-facing
surrogate key, so callers pass the URL a reader would see and never
replay this service's origin rewriting.
The route claims every method rather than POST alone. A method a named
route does not claim falls through to the publisher, and
enforce_basic_auth leaves the Authorization header attached, so a GET
here would authenticate and then ship the shared admin credential to the
origin. The handler answers non-POST with 405 itself. The test asserts
this against publisher_fallback_methods() rather than a copy of the
list, so a method added there cannot quietly open the hole again.
Content-Type must be exactly application/json. Browsers attach
basic-auth credentials automatically and a cross-origin form post with
enctype="text/plain" is not preflighted, so requiring POST alone does
not stop CSRF; requiring a type no form can produce does.
The body is parsed as a flat struct rather than an internally-tagged
enum. deny_unknown_fields does not reach the unit variant of such an
enum, so {"scope":"all","url":"..."} parsed as a full flush — an
operator who mistyped the scope while meaning to purge one page would
have emptied the cache and been told it worked. That combination is now
an error naming the confusion.
A full flush is an unbounded origin-stampede lever behind one shared
static credential and there is no rate-limit primitive on this path, so
the authenticated username is logged every time. The username only; the
password is a shared secret and must never reach a log line.
Purge is idempotent and purge_all is a single surrogate-key call, so
there is no partial state: the response says so, because an operator
mid-incident needs to know whether retrying is safe.
Adding the path to ADMIN_ENDPOINTS is a breaking config-validation
change: an operator whose handler regexes enumerate admin paths will
fail validation until the new path is covered. The narrow-regex test
that had to be updated here is that migration in miniature.
An unregistered path falls through to the publisher origin and 404s, which reads to a CMS purge webhook as "this endpoint does not exist" rather than "not supported on this platform". Register it on Axum, Cloudflare and Spin with an explicit 501 and a message naming the adapter that does support it. Registered for every publisher-fallback method on all three, matching the Fastly route and the legacy admin aliases: a method a route does not claim falls through to the publisher with the caller's Authorization header still attached. Axum's named_routes() array goes 16 to 17 and Spin's named_fallback_paths() likewise, so the count is compile-enforced rather than left to a reader to notice.
Three tests rather than one, because the obvious single test passes for the wrong reason. The suite sets basic auth on ^/_ts/admin, so an unauthenticated probe gets 401 and never reaches a handler; a bare "assert not 200" would be satisfied by that 401 whether or not the route exists. So: the authenticated probe asserts 501, the unauthenticated one asserts 401 to prove the first reached a handler through auth, and the third walks every non-POST method to pin the credential-forwarding guard cross-adapter — a method a route does not claim falls through to the publisher with the Authorization header attached. No new helpers and no new dependency were needed: the credential- carrying axum/cf/spin_authorized_json helpers already existed, so the integration crate's separate lockfile is untouched.
The plan had this call the Fastly purge API directly, and noted that if the token scope could not be granted the commit should be dropped. The recorded token is config- and secret-store write only, with no purge permission, so as planned it was blocked. Calling the admin endpoint instead removes the dependency: the purge runs inside the service, where the platform SDK needs no API token at all, so this works with credentials an operator already has. It also leaves exactly one implementation — the surrogate key is derived server-side, so the CLI cannot drift out of agreement with the cache it is purging. The plan's own D1 test existed to catch that drift; this shape makes it unrepresentable. The password is read from the environment and is deliberately not a flag: an argument is visible to every process on the host through `ps` and lands in shell history. Neither --all nor --page is an error rather than a default, so a bare `ts cache purge` cannot flush production. 401, 404 and 501 each get their own hint, because a purge is usually run mid-incident and the three send the operator somewhere different. Fixes a real defect found while testing: the purge path built a reqwest client without installing the rustls crypto provider, so any HTTPS purge would have panicked with "No provider set" on the operator's machine. The provider install moves out of the probe into `tls.rs` so both clients share it, with the rationale for the `-no-provider` feature kept in one place. The fixture origin gains method and body capture so the e2e test pins the wire format against the endpoint's guards: POST, exactly application/json, and credentials attached.
Viceroy implements purge_surrogate_key against the same in-process cache it serves reads from, so store -> hit -> purge -> miss is testable end to end with no Fastly service involved. That makes this the strongest check available for the purge path, and the only one that exercises the real cache rather than a double. The new `purge` mode reuses the esi configuration exactly — a CONFIG_MODE indirection so the generator and every esi assertion keep running unchanged — then purges and asserts the entry is gone. It also drives the endpoint's guards against the real router rather than a unit double: unauthenticated is refused, a GET is answered locally with 405 instead of reaching the publisher with the admin credential, a form-postable Content-Type is refused, and scope "all" carrying a url is refused rather than silently flushing. The assertion is `miss-stored`, not `miss`: the purged entry is gone and the same request immediately refills it, which proves the purge removed an entry without disabling the cache. The admin password is read back out of the generated manifest rather than repeated, because the config's `password = "handler_password"` is a secret-store reference and the basic-auth value is the seeded secret — two literals that would otherwise drift apart silently. CI gains a matching step; the script's mode list and usage banner accept `purge` alongside `inline` and `esi`.
On Fastly, setting a surrogate key or a TTL reverses a prior set_pass: both carry "overrides any previous Request::set_pass call" (fastly-0.12.1/src/http/request.rs:2462, :2381). A pair of booleans could therefore express "bypass the cache, and tag it for purge", which silently means "do not bypass" — the opposite of how it reads. The enum makes that combination unspellable, and the builders replace rather than accumulate so call order cannot decide the outcome either. No behavior change: Bypass is what with_cache_bypass already did, and Default is the absence of it. The Shared branch is added but nothing constructs it yet; the gate that will is the next commit. Shared attaches the surrogate key and deliberately does not set pass, because readthrough is enabled by omitting set_pass, never by adding a TTL — set_ttl would additionally override the origin's own private and no-store, turning the hazard this work exists to close into an API. A surrogate key that cannot be encoded as a header value falls back to bypass rather than caching without it. Storing an object no purge can reach would be worse than not storing it, since "purge the key" is the whole rollback story for readthrough. Cloudflare maps Shared to its default mode, having no surrogate-key concept, rather than pretending to honor a key it cannot use.
Stop forcing an origin MISS on every ad-serving pageview. The bypass now keys on origin_response_is_shareable, which asks whether the origin's response may be held in a shared cache. should_run_ad_stack asked something unrelated: whether this request runs an auction says nothing about whether the response behind it can be shared. This change cuts both ways and is not "strictly more conservative": - Loosening, for cookieless ad-serving navigations, which stop forcing a MISS. This is the ~485ms of a 773ms TTFB the issue exists to recover. - Tightening, for cookie-bearing, Authorization-bearing, conditional, range and non-GET requests, including bot traffic that previously did not bypass and now does. Four existing tests changed in this direction and their messages now say why. Nothing about this is enabled by the code alone: an origin that marks HTML private still stores nothing, so the win only materializes where an operator has verified shareability with ts origin probe-shareability. Both origin-fetch paths call one decision function rather than repeating the condition. They are alternatives for the same fetch — one inside the EC-preload fan-out, one in the branch taken when that did not fire — and reverting only the first passed all 2,697 tests, because a behavioral test can reach that path only with a valid signed EC id. A duplicated condition would have made readthrough eligibility depend on whether EC preload fired, which is not a property of the origin response at all. One function makes that divergence unrepresentable rather than merely tested for.
Two independent reviews of the previous commit reached the same critical finding, and they were right: that commit's loosening had no operator control at all. origin_is_cookie_independent reads like the switch but is not one here. cookie_disqualifies is `request_had_cookie && !origin_is_cookie_independent`, so for a request carrying no Cookie header the flag never participates: every cookieless GET navigation was judged shareable and stopped forcing an origin MISS the moment the binary shipped, on every deployment, with no operator action. Rollback would have been a code revert and redeploy. That is also the worst population to enable silently. The design doc names it: `origin_response_is_shareable` is true precisely for readers carrying no cookie — first-time visitors, which is exactly when an origin issues a session cookie. There is no response-side guard on this path; the decision is made before the origin replies and no post-response hook is reachable on the Fastly adapter. Safety rests on the origin's own Cache-Control plus an operator's verification, so enabling it has to be a deliberate act rather than a consequence of deploying. Add creative_opportunities.origin_readthrough_enabled, default false, ANDed into the gate as a separate term. The shareability predicate is still evaluated and still recorded on telemetry, so an operator can see how much traffic the gate would admit before turning it on. Rollback is now a config flip plus a purge. Also closes two review findings: The EC-preload fetch path now has behavioral cover. Reverting only that call site previously passed all 2,697 tests. Reaching it needs three things the harness did not supply — a KV store, an EC id, and a client reporting pending-streaming support — not just the EC id I had claimed; with all three, reverting that site alone now fails. A doc comment was misattached: the new function's block ran on from request_can_use_shared_template's with no blank line between, so rustdoc gave the template function's summary to the cache-intent function and left that pub(crate) item undocumented.
Review finding: request_requires_origin folded two different questions together, and the predicate was read before the headers it judges are stripped. A repeat visitor sends If-None-Match. When the ad stack runs, those headers are stripped before the origin is asked anything, so the origin answers an unconditional question with a full document — as shareable as any other. The predicate was computed pre-strip, so that request was marked unshareable and forced an origin MISS, losing readthrough for exactly the repeat-visit population this work exists to speed up, in exchange for nothing. Split the two questions: - reader_requires_origin, read pre-strip, gates the *template* cache. A reader who asked for a range or a revalidation must reach the origin; stripping changes what the origin is asked, not what the reader wanted. This is now its own term on request_can_use_shared_template rather than riding inside the shared inputs. - request_requires_origin, read post-strip, feeds origin shareability. The strip removes four headers while this predicate tests six plus Cache-Control request directives, so If-Match, If-Unmodified-Since and a no-store reader still disqualify either way. A first attempt moved the single value and broke revalidation_partial_and_conditional_requests_bypass_a_warm_template: a range request would have been answered from a warm shared template. That test is the reason the two questions are now separate rather than reordered. The 128-combination sweep becomes 256 to cover the new term, and both directions are pinned: reverting the split fails the readthrough test, reverting the reader term fails the range and revalidation tests.
Both reviews flagged that the operator-facing docs said nothing about this cache: no mention of the gate, the probe, or the new flag. An operator had no config surface to reason about a change with cross-reader blast radius. The section says plainly that readthrough is a different and weaker cache than the template cache, and gives the refusal-by-refusal comparison. The three rows the template cache enforces on the response — Set-Cookie, CSP nonce, missing positive freshness — are not covered here and cannot be, because the decision is made before the origin replies and no post-response hook is reachable on this adapter. Those rows are named as accepted operator risk rather than left for a reader to derive, along with the sharpest case: readthrough admits requests carrying no cookie, which is exactly the first-time visitor an origin issues one to. Rollback is documented as it actually is, not as we would like it. The config flip is real and takes effect on the next request. Purge is not: `ts cache purge` covers `ts-template` only, and whether readthrough objects can be tagged at all is unverified against a real Fastly service, so nothing claims to reach them. Already-stored objects age out on the origin's TTL. Shipping a rollback step that does not affect the cache being rolled back would be worse than admitting the gap. That unverified staging check is also why the `ts-origin` surrogate key task is not implemented: its own plan gates it on that verdict, and an unverifiable purge is the failure mode where an operator believes stale content is gone when it is not. Also points `origin_is_cookie_independent` at the probe that now answers its question, in both the doc comment and the example config, instead of leaving "unsafe unless independently verified" with no way to verify.
The last of the five items this issue was rescoped to. The cache has shipped behind an opt-in assembly mode, is covered by the local harness and the rendered-document byte-identity tests, and now has an operator purge surface — but every doc comment still described it as a #1009 validation spike to be removed, which is no longer what it is. The substantive one was the `VarySpec` note, which called the configured `Vary` list "a spike-grade choice, not a production one". Re-examined rather than reworded: the drift it risks fails closed, because the origin's actual `Vary` is compared before storage and an uncovered name refuses the template, carrying the offending names so a stale config is identifiable from one log line. A configuration that falls behind the origin costs hit rate, not correctness. That is a defensible production posture, and the note now says so along with what would justify revisiting it — the extra lookup of a two-phase design, not a limitation of the guard. The module's three-cache table also still used the retired C1 label for the origin readthrough cache, which this issue has since given its own gate; it now names both and says they are independent. The remaining markers were status rather than substance: the optional config fields keep `Option` + `skip_serializing_if`, because the reason for them is rollback compatibility with binaries that use `deny_unknown_fields`, which is a production concern and not a spike one. Documentation only. No behaviour change, and the rustdoc warning count is unchanged at 30 against the branch point.
A "~485ms of a 773ms TTFB" figure had worked its way into a code comment and a test assertion on this branch. It came from an earlier session's measurement against a real deployment, not from anything verified here, and it was being repeated as though it were established. Replaced with what was actually measured, locally, this time: Under Viceroy 0.17, with origin_readthrough_enabled set, the request instrumented and confirmed shareable, and the stub origin answering `Cache-Control: public, max-age=60` with no `Set-Cookie`, two identical navigations still produced two origin fetches. Viceroy does not implement the readthrough cache. Three confounds were eliminated before drawing that conclusion: the origin's response is cacheable, the gate admitted the request rather than bypassing, and the flag was on. The consequence is worth stating where people will look for it, so it is recorded both at the test that could be mistaken for proof of a saving — it asserts the recorded intent, not a cache hit — and in the operator guide, which now says plainly that local timings say nothing about this setting and the first evidence comes from a deployed service.
Run against a real protected origin, the probe produced a confident verdict describing content the origin never served. Its baseline arm sent no cookies, so a DataDome-style wall answered it with a challenge page, and every axis and verdict then reported on that page: "private, no-store, must-revalidate", a Set-Cookie, and no Vary. The origin's actual response is `max-age=60` with a declared Vary and no Set-Cookie. That is a false FAIL on a plausible candidate, and it reads exactly like a real one. For a tool whose entire job is answering this question, and whose failure direction was supposed to be the safe one, silently describing the wrong document is worse than refusing to answer. Two changes: A non-200 baseline now aborts with the status and what to do about it, rather than proceeding to judge. Nothing downstream of a challenge page is evidence about the origin. `--admission-cookie` is carried by every arm, including the baseline and the self-identity repeats, because without it a protected origin answers each arm with the same wall. It is separate from `--cookie`, which is what the cookie axis varies: the admission cookie is what gets the probe admitted at all, so including it in both arms keeps that axis measuring personalization rather than admission. Seeded inside `fetch` rather than at each call site, so an arm that sets its own `cookie` header replaces it through the existing resolution logic instead of sending the header twice. A first attempt threaded it through the call sites and missed `self_identity_axis`, which the new fixture test caught by comparing a challenge page against real content.
An axis that differs on a signal the origin declares in `Vary` is not a hazard: that signal is part of the platform's cache key, so each value gets its own stored object and both readers get the right one. The probe failed those anyway, so an origin doing exactly the right thing was marked not shareable. Measured against a real origin declaring `Vary: rsc, next-router-*, accept-encoding, arena-exp`: the accept-encoding and rsc axes both failed, while the vary-coverage verdict passed on the same run and said so. Two of four failures were the probe contradicting itself. Axes now carry `covered_by_vary` and pass when the origin declared the signal, printing the reason rather than reporting a silent pass. `vary-coverage` keeps judging the raw observation through a new `differs()`, because it is the check that decides whether a difference is declared — reading `passed()` there would have made it vacuous. Self-identity is excluded: it varies no request signal, so no `Vary` can key it, and a page unstable against itself cannot be shared however it is keyed.
Two independent reviews of the whole branch. No critical or high correctness defects were found in the code; the substantive findings were places the branch asserted things it knew to be untrue. A false PASS the probe could produce. An axis differing on a signal the origin declares in Vary now passes, which is right for accept-encoding and rsc but wrong for cookie: that axis answers "does the origin ignore cookies", and `Vary: Cookie` is the origin saying it does not. Passing it would have printed a green verdict whose own closing line reads "do not enable origin_is_cookie_independent", and would have contradicted the template cache, which refuses `Vary: Cookie` at runtime. Cookie is now excluded from that excuse. The Tinybird README still said no readthrough gate consumes the predicate. True when written; the gate landed later on this same branch. The config field's rustdoc promised rollback by "a purge of the ts-origin surrogate key". No production code applies that key — the operator guide says so at length and the rustdoc an engineer reads first said the opposite. `origin_readthrough_enabled` was missing from the list of keys to remove before rolling back to an older binary. Those binaries use deny_unknown_fields, and setting the flag to false to roll back serializes it into the blob, so the rollback step itself would have made every request fail. Adding /_ts/admin/cache/purge to ADMIN_ENDPOINTS is a breaking config-validation change for operators whose handlers enumerate admin paths. Now stated as an upgrade note. Also: the template-cache rollback pointed at "normal purge tooling" rather than the command this branch added; the spike framing survived in the two operator-facing places after being removed from the code; part 3's plan named the superseded flag; and the retired C1/C3 cache labels survived in two files the earlier sweep missed. Full CI gate list run, which the code review was explicit about not having done: fmt, all eight clippy invocations, 2,917 + 41 + 44 + 86 adapter tests, 268 CLI, 16 parity, all three harness modes, 901 JS tests, docs prettier.
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