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.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed a547d44efc60a54db2260257fab8faf10789b70e against 6cae7f5da8911c746cf873581885f90c3820dd96. Requesting changes; three actionable findings are attached inline.
The default cache policy and purge paths passed focused validation. Production Fastly readthrough behavior remains unverified; Viceroy validates the cache decision, not production readthrough hits.
Validation
cargo test --locked --package trusted-server-cli --target x86_64-unknown-linux-gnu --test origin_probe --test cache_purge: 42 passed.cargo test-fastly --locked -- cache: 260 passed, including default-policy preservation and both origin-fetch paths.cargo test-fastly --locked -- shareability: 3 passed.cargo test --locked --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity admin_cache_purge: 3 passed.BID_DELAY=3 ./scripts/template-cache-local-test.sh purge: 33 passed, including authenticated purge-all and purge-by-URL.cargo fmt --all -- --checkandgit diff --check 6cae7f5...a547d44: passed.- Additional in-memory loopback HTTP fixtures exercised the built probe CLI. Reproduction details are in the inline findings.
- Schema comparison confirmed all 34 Rust fields and all eight fixture rows match Tinybird.
All 19 reported CI checks passed. No existing review bodies, inline comments, issue comments, or review threads were present. Tinybird deployment remains unverified. No repository files were edited, and the review was not delegated.
aram356
left a comment
There was a problem hiding this comment.
Summary
Reviewed against origin/main at merge-base 6cae7f5d, head a547d44e. 48 files, ~5,400 lines of code plus ~2,600 of plans and specs. All 20 CI checks pass, and cargo fmt --check passes locally — every blocking finding below is something CI cannot catch.
The architecture is sound. The predicate split is the right decomposition, PlatformCacheIntent encodes a genuine Fastly footgun into the type system, the cache-key v5 bump is correctly unreachable from v4, and the risk documentation is unusually honest about what readthrough cannot guarantee. Test coverage on the readthrough decision is strong, including a 256-case exhaustive predicate test and a case pinning a near-miss that "previously passed all 2,697 tests."
The blocking findings cluster in one place: ts origin probe-shareability can return "shareable" for three origin shapes that trusted-server-core itself classifies as not shareable. That matters more than a normal disagreement because this PR's own documentation designates the probe as the only response-safety control on the readthrough path — there is no runtime gate behind it to catch a wrong verdict. Two of the three contradict gates already implemented and commented in publisher.rs, and one reintroduces a bug whose fix is recorded there.
All three are small, local fixes. I verified each in a scratch worktree at the PR head: cargo fmt --check, cargo clippy --package trusted-server-cli --all-targets --all-features -- -D warnings, and all 280 CLI tests pass — individually and applied together.
5 of the inline comments carry a one-click
suggestion. The rest describe the fix in prose because it spans multiple hunks, multiple files, or is documentation.
Blocking
🔧 wrench
- Probe passes
no-cache, which core refuses — see inline atcrates/trusted-server-cli/src/commands/origin/probe.rs:405(suggestion) Vary: *read as blanket coverage rather than a refusal — see inline atcrates/trusted-server-cli/src/commands/origin/probe.rs:278- Non-UTF-8 header values silently dropped, failing open — see inline at
crates/trusted-server-cli/src/commands/origin/probe.rs:565(suggestion) origin_is_cookie_independentrustdoc promises a guard readthrough does not have — see inline atcrates/trusted-server-core/src/creative_opportunities.rs:348- Four comments mangled by the
C1/C3rename — see inline atpublisher.rs:1727,:2154,:5999,:11808(4 suggestions) - Missing regression tests for the three probe gaps — see Cross-cutting below
Non-blocking
🤔 thinking
- Scheme mismatch purges nothing and reports success — see inline at
crates/trusted-server-core/src/platform/template_cache.rs:186 --serviceunvalidated, admin credential can go over plaintext HTTP — see inline atcrates/trusted-server-cli/src/commands/cache/purge.rs:57- Cache-intent test restates the function's own expression — see inline at
crates/trusted-server-core/src/publisher.rs:10134 - Enablement guide overstates telemetry coverage — see Cross-cutting below
- README points operators at a stub pipe — see Cross-cutting below
⛏ nitpick
- Dangling empty doc line — see inline at
crates/trusted-server-core/src/platform/types.rs:298(suggestion) tinybird/README.mdfails prettier — see inline attinybird/README.md:23(suggestion)
Cross-cutting / body-level findings
-
🔧 No regression tests for the three probe gaps. The probe's 34 tests are genuinely good and mostly assert rejection, which is the right bias. But the three findings above are each uncovered, and one is covered backwards:
vary_star_covers_everythingpins the wrong behaviour as intended. Please add, alongside the existing fixtures: an origin sendingpublic, max-age=300, no-cache(and one sendingPragma: no-cache) → must fail freshness; an origin that varies on User-Agent and sendsVary: *→ must fail; and a response carrying a non-UTF-8Set-Cookie→ must fail. Without these, the same three gaps can reopen silently, since the fixes are one-line predicate changes. -
🤔 The enablement checklist overstates what the telemetry column shows.
docs/guide/configuration.mdstep 5 saysorigin_cache_shareable"records the predicate on every row, so it shows how much traffic the gate admits." Butapply_origin_cache_intentgoverns every publisher origin fetch, while a summary row is only emitted when ad slots match — a bot, a prefetch, a consent-denied reader, or a page with no matched slot produces no row at all. The gate's tightening half applies to exactly that non-ad traffic (asa_request_that_skips_the_ad_stack_is_still_judged_on_shareabilityasserts), so the traffic the flag newly affects is the traffic the column cannot see.tinybird/README.mdgets this exactly right under "The denominator is ad-serving pageviews, not all requests" — the two documents disagree, and the enablement checklist is the one an operator follows. Suggest pointing step 5 at that README section and restating the denominator inline. -
🤔
tinybird/README.mddirects operators to a monitoring surface that reports nothing. The deploy-ordering section says rows carrying an undeclared column "go to quarantine rather than being rejected loudly — seepipes/quarantine_counts.pipe." That pipe is a stub: its entire body returnsCAST(NULL, 'Nullable(UInt64)') AS quarantined_rowsand'unconfigured' AS status, with a description saying deployments should replace it with the workspace quarantine source. An operator who deploys code-first and then checks quarantine counts to confirm no rows were lost sees nothing and concludes all is well — the opposite of the truth. Either note that the pipe must be pointed at the real quarantine source before it reports anything, or drop the reference and say quarantine must be checked in the Tinybird UI.
CI Status
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cloudflare native): PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- CLAUDE.md symlink guard: PASS
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed c09f9c22b0a38fc337f405694dd50db2cb839a71 against 2ca5d39ca7a5f600285f585435e169a5dad4dabe. Requesting changes for three actionable findings attached inline.
The default-off cache policy, purge route guards, and Fastly cache-intent mapping passed focused validation. All 20 reported CI checks pass. Production Fastly readthrough hits and surrogate-key purging remain unverified because Viceroy does not implement readthrough caching. Existing review threads were checked; these findings do not duplicate them.
…eep probe cookies out of argv Bots and prefetches are excluded from the ad stack but not from shareability: neither classification reaches origin_response_is_shareable, so a crawler, challenge, or prefetch document served without Vary could be stored and handed to a human navigation. Add a bot axis (crawler User-Agent) and a prefetch axis (Sec-Purpose), each varying one signal against the same baseline. An axis is now named for the request class rather than the header it sends, so vary_covers_axis maps bot to user-agent and prefetch to sec-purpose. Compare a canonical representation instead of the body alone. Fastly stores response headers with the body, so two responses with identical HTML, a per-audience Content-Security-Policy, and no matching Vary are cross-served policies. Fetched::canonical() renders the cache-visible policy headers in a fixed order followed by the body, and every axis compares that, so header differences are judged by the same Vary rules as body differences. The policy set is an allowlist rather than a denylist of volatile fields: a denylist fails an origin for every Date, request id, or trace header it emits, and a probe that cries wolf is one an operator learns to rerun until it passes. The printed limits state what is outside the set. Read probe cookies from the environment and validate transport before reading them. --cookie and --admission-cookie accepted credentials on the command line, where they are visible through ps and land in shell history, and no scheme check stood between them and a remote plaintext origin. They are now TRUSTED_SERVER_PROBE_COOKIES and TRUSTED_SERVER_PROBE_ADMISSION_COOKIE, matching the admin password in ts cache purge. The scheme, loopback, and userinfo checks move from purge_endpoint into url_guard so both commands share one implementation.
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Splits origin shareability from template eligibility and adds the opt-in origin_readthrough_enabled gate, with a PlatformCacheIntent enum that makes "bypass + surrogate key" unrepresentable, plus the shareability probe, purge endpoint/CLI, and telemetry. The design is careful and the tests are thorough.
My earlier finding (untagged readthrough objects) is resolved by c09f9c22: eligible requests now use Shared with both the page key and ts-template. Thanks.
One blocking issue remains: URL purges acknowledge success for input that can never match. I checked the existing threads before posting; the comments below are new, and the two that sit next to earlier threads link to them.
3 of the inline comments below carry a one-click GitHub
suggestion. The rest describe the fix in prose because it touches more than one hunk or needs a design decision.
Blocking
🔧 wrench
- Unparseable purge URLs are acknowledged as purged — see inline at
crates/trusted-server-core/src/cache_purge.rs:72
Non-blocking
🤔 thinking
- Page purge can miss readthrough objects shared by reader-URL spellings — see inline at
crates/trusted-server-core/src/publisher.rs:4495 - Enabling readthrough changes caching of non-ad assets and widens
--all— see inline atcrates/trusted-server-core/src/publisher.rs:4102
🏕 camp site
- Probe message references the removed
--admission-cookieflag — see inline atcrates/trusted-server-cli/src/commands/origin/probe.rs:368
⛏ nitpick
set_origin_cache_shareabledoc says "admitted" — see inline atcrates/trusted-server-core/src/auction/telemetry.rs:205ALL_ADMIN_METHODSduplicatesLEGACY_ADMIN_DENY_METHODS— see inline atcrates/trusted-server-adapter-fastly/src/app.rs:1138
CI Status
All 20 checks pass, including the 4 required ones (cargo fmt, cargo test, format-typescript, format-docs).
A URL purge accepted any non-empty string. reader_url_surrogate_key hashes whatever it is given, so "/article", "example.com/article" and "ftp://..." were each acknowledged as purged: true under a key no stored object can carry — the silent no-op that key's own documentation calls the failure that matters, and one a CMS webhook sending paths would never find out about. Require an absolute http(s) URL with a host, and answer 400 otherwise. Apply the readthrough gate to document requests only. It answers a question about pages, but it was reached by every publisher request, including subresources: a cookie-bearing or conditional asset request bypassed the edge cache, and every shareable asset was tagged ts-template, so the template rollback purge became an origin-wide asset flush. Browsers send first-party cookies on subresources, so that covered most repeat-visitor asset traffic. Documents keep the shareability policy, including those that skip the ad stack. Document that readthrough page purges are unreliable when one page is served under several reader-facing spellings. Reader URLs that rewrite to one origin URL share a single stored object tagged with the reader URL of whichever request filled it first, so multi-host and dual-scheme deployments must purge with --all. Also fix a probe message that still named the removed --admission-cookie flag, reword the origin_cache_shareable doc as the request predicate it records, and drop ALL_ADMIN_METHODS in favour of the identical LEGACY_ADMIN_DENY_METHODS the Axum and Spin adapters already use for this route.
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed b8461e9 against my previous pass (at e58a8d9). All six findings are addressed. The purge endpoint now refuses anything that isn't an absolute http(s) URL with a host, and tests cover both the refused and the accepted cases. Readthrough now applies only to document requests, so subresources keep the platform default and are never tagged ts-template. The new subresource test and the added document column in the policy table pin this. The shared-object purge case is documented as "use --all on multi-host or dual-scheme deployments", and I agree with the reasoning against a path-only key. The probe message, the telemetry doc and the duplicate method constant are all fixed.
👍 Narrowing on is_html_document_request rather than on the ad stack was the right call. It keeps non-ad documents on the shareability policy.
CI Status
All checks pass. Verified locally as well: cargo fmt --check, clippy-fastly, clippy-cli, test-fastly and the CLI test suite.
aram356
left a comment
There was a problem hiding this comment.
Summary
Second pass, against b8461e9e (merge-base 9558943e). All thirteen findings from my previous review are fixed, and three were fixed better than proposed: the freshness check now matches directive names exactly rather than by substring, the encoding guard keeps the raw HeaderMap and adds a dedicated verdict instead of a placeholder string, and the transport guard also refuses userinfo and lives in one shared module so it cannot drift between commands.
The two behavioural changes since then are both improvements. Restricting readthrough to documents closes the subresource case, and tagging readthrough objects with surrogate keys makes them purgeable, which the docs previously said they were not. I checked that no stale "not purgeable" text survives anywhere operator-facing, that the Fastly mapping fails closed on a malformed key rather than caching something unreachable, and that insert and purge derive the reader-URL key through the same function.
Two new blocking findings, both in has_positive_freshness and canonical. Each lets the probe certify an origin as shareable when it is not, which matters because this probe remains the only response-safety control on the readthrough path: there is no runtime gate behind it to catch a wrong verdict. Both are small and local. I verified the proposed fixes in a scratch worktree at this head: cargo fmt --check, cargo clippy --package trusted-server-cli --all-targets --all-features -- -D warnings, and all 633 CLI tests including the 47 probe tests, individually and applied together.
Locally I also ran cargo test-fastly (2,976 pass), test-axum, test-cloudflare, test-spin, and the parity suite. All green.
1 of the inline comments carries a one-click
suggestion. The rest describe the fix in prose because it spans more than one region of a file, or is documentation.
Blocking
🔧 wrench
s-maxage=0does not defeat a positivemax-age— see inline atcrates/trusted-server-cli/src/commands/origin/probe.rs:714(suggestion)- Ten of twelve
POLICY_HEADERScan be dropped from the comparison — see inline atcrates/trusted-server-cli/src/commands/origin/probe.rs:93
Non-blocking
🤔 thinking
- The RSC flight exemption is keyed on the profile, not on the RSC header — see inline at
crates/trusted-server-cli/src/commands/origin/probe.rs:526 - A template rollback now also flushes cached origin documents — see inline at
docs/guide/configuration.md:2139
⛏ nitpick
--pageis not validated client side — see Cross-cutting below- The prefetch arm does not send legacy
Purpose: prefetch— see Cross-cutting below - The new cookie-parsing helpers have no tests — see Cross-cutting below
Cross-cutting / body-level findings
-
⛏
ts cache purge --pageis not validated client side.crates/trusted-server-core/src/cache_purge.rs:72-86now refuses a URL that is not absolutehttp(s)with a host, which closes the silent-success case properly, server side. The CLI still forwards/article,example.com/articleandftp://…to the wire and lets the service answer 400. That is correct but costs a round trip and turns a local mistake into a remote-dependent error.request_bodyincrates/trusted-server-cli/src/commands/cache/purge.rscould reuse the same parse predicate; no test covers a client-side refusal today. -
⛏ The prefetch arm does not send legacy
Purpose: prefetch.crates/trusted-server-cli/src/commands/origin/probe.rs:255-259sends onlysec-purpose. Chrome still emitsPurpose: prefetchfor<link rel=prefetch>, so an origin keyed on the legacy header is not exercised by this axis. Sending both in the arm and addingpurposeto the prefetch axis's required set would close it in a couple of lines. Low priority, sinceSec-Purposeis the modern signal. -
⛏ The new cookie helpers in
crates/trusted-server-cli/src/commands/origin/mod.rshave no tests.split_cookie_header(~line 101) andload_cookies_from_environment(~line 114) are new, parse a credential-bearing string, and that file has no#[cfg(test)]module. Worth a small unit test for semicolon splitting, trimming, empty-segment filtering, and the branch at ~line 118 where the environment must not clobber a caller-set admission cookie. Moving the cookies out of argv is right, and#[arg(skip)]on both fields means clap cannot accept them from the command line at all, which I verified.
CI Status
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cloudflare native): PASS
- cargo test (ts CLI, native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- CLAUDE.md symlink guard: PASS
|
Addressed this review in 9f18354, with replies on all four inline threads. The body-level findings are also fixed: --page now rejects invalid reader URLs locally, the prefetch arm sends both Purpose and Sec-Purpose and requires both in Vary coverage, and cookie parsing/environment precedence have unit coverage without mutating process-wide environment variables. Merged current main in the separate commit 1751342. Conflict resolution preserves both the cache-purge and Next.js parity tests, plus both sets of Tinybird documentation. Validation on the merged tree: all eight target-matched clippy gates, Rust formatting, Fastly (3,021), Axum (42), Cloudflare (45), Spin (87), parity (17), CLI (642), JS build and Vitest (1,130), JS formatting, and docs formatting passed locally. |
# Conflicts: # crates/trusted-server-adapter-fastly/src/platform.rs # crates/trusted-server-core/src/publisher.rs
Add opt-in origin readthrough caching, an operator shareability probe, and purge tooling for templates and tagged origin responses. Keep origin shareability separate from template eligibility and record the predicate in auction telemetry.
origin_readthrough_enabledis unset or false: ad-serving requests bypass; other publisher requests use the platform default. When enabled, both origin-fetch paths use the same shareability decision and attach the reader-page and purge-all surrogate keys. Configured template cookie dimensions bypass readthrough while retaining separate cached templates.ts origin probe-shareability. Inspect every response, including repeats, for expected content type, status, cache hits, freshness, Set-Cookie, CSP nonces, and Vary coverage. Navigation uses HTML Accept and Fetch Metadata; RSC uses an independent browser-fetch control. Unreadable safety headers, no-cache, Pragma: no-cache, and Vary: * fail closed. Probe configured headers independently, both with and without RSC. Use representative browser user agents and reject differences between decoded encoding variants.POST /_ts/admin/cache/purgeandts cache purgefor all templates/tagged origin objects or one reader-facing URL. Claim non-POST methods locally, require JSON, bound request bodies, and return 501 on unsupported adapters. The CLI requires HTTPS except on loopback, refuses redirects, and validates purge acknowledgments. Page purges retain the exact reader-facing scheme, host, and port.origin_cache_shareableto telemetry, the Tinybird datasource and fixtures, with deployment ordering documented. The measured population is matching-slot candidates, including skipped summaries; the quarantine pipe must be configured before it can report counts.Validation: Rust format, six adapter Clippy aliases, host CLI Clippy, all adapter tests, cross-adapter parity, host CLI tests, JavaScript build/tests/format, docs format, and the purge/inline harnesses, including cookie-variant isolation. Regression fixtures cover cached later probe responses, variant-only unsafe headers, RSC-dependent custom-header variation, mobile documents, navigation content negotiation, undecodable safety headers, wildcard Vary, revalidation refusals, readthrough cookie exclusions, and redirected purge requests.
Readthrough cache hits and surrogate-key behavior still require a deployed Fastly service; Viceroy verifies the decision only.
ts cache purgereaches transformed templates and newly tagged origin readthrough objects using the same keys. Disabling readthrough restores the previous policy; older untagged origin objects still require TTL expiry. Apply the Tinybird schema before deploying code that emits the new column.Refs #852. Production readthrough verification remains outstanding.