Add the shared template cache and edge assembly for #1009 - #1013
Draft
prk-Jr wants to merge 44 commits into
Draft
Conversation
Validates the ESI approach proposed in #1009 and recommends deferring it. ESI presupposes a TS-owned template cache: its pull-based BufRead input cannot sit downstream of lol_html's push-based rewriter without an intermediate buffer, and the cache boundary is that buffer. That cache is in turn blocked on purge capability the service does not have. Revival condition: React #418 resolved and the window.load gate removed. Re-diagnoses the TTFB regression the issue targets. The auction is dispatched before the origin fetch and does not block, and on a Next.js publisher the closing body tag is not reached until the whole document has been buffered, so the auction hold costs approximately nothing. The cost is with_cache_bypass forcing a readthrough-cache miss on every ad-eligible navigation. Removing either alone recovers little; the two are multiplicative. Corrects nine premises in the issue, including that tsjs.adSlots is per-URL rather than per-user, and that moving identity off the inline response is a prerequisite only for a visitor's first navigation. Carries no performance measurements. Every conclusion is derived from code at the pinned baseline so it can be checked by reading the repository.
The strongest claim in the previous revision was wrong. It argued that on a Next.js publisher lol_html never sees the closing body tag until the final chunk, so the auction has the whole download plus rewrite to finish, and concluded that no timing data was needed. The hold does not key off lol_html at all. BodyCloseHoldBuffer::push scans the decoded origin input for the closing tag, and hold_collect_close_tail awaits collect_stream_auction the moment it appears, before post-processing runs. Post-processor buffering is irrelevant to when the hold fires, so the argument applied to every publisher or to none. What survives is the general form: the hold costs max(0, A - T) where T is origin TTFB plus transfer to the closing tag. That needs measurement rather than inference, so Step C now measures the hold directly via hold_wait_ms instead of comparing origin fetch against auction duration through a proxy model. The verdict table follows. Stage 0 becomes an operator flag rather than a code deletion. The risk it gates is cache poisoning, where rollback speed dominates diff size, and a config push reverts in seconds where a release does not. Also: the Vary precondition now covers client Cookie, origin Set-Cookie, and Authorization, which are a larger exposure than the RSC split it previously addressed; a Vary failure is recorded as a live production defect, since RSC fetches already transit the read-through cache; the auction timeout citation pointed at a test fixture rather than the real resolution order; and appendices B, C, E and F are condensed, since they specified work the document recommends against scheduling.
Covers the spec's Steps A/B/C plus Stage 0. Stages 1-5 are out of scope and named as such, since the spec queues them behind the open correctness defects. Three investigations and one code task. Step A curls the origin for its Vary declaration and for cookie personalization, and gates everything downstream. Step B settles whether anything caches the service's own response by inspecting the Fastly topology rather than probing for an age header, and asks whether the publisher backend is shielded, which sizes the win and nothing else in the plan establishes. Step C instruments hold_wait_ms and origin_fetch_ms. The instrumentation goes in collect_stream_auction rather than at its three call sites. All three reach it, and it already destructures settings out of AuctionCollectDeps, so one edit covers every adapter with no new plumbing. The plan names hold_finish_ready_segments and hold_finish_tail_segments explicitly as sites not to instrument: neither awaits the collect, and doing so would double-count. Stage 0 ships as publisher.bypass_origin_cache defaulting to today's behaviour, then flips by config push. Adding that field breaks nine sites the diff does not suggest, including a live doctest, so they are enumerated. The win is measured client-side through the existing tester-cookie harness; origin_fetch_ms is TTFB only and is attribution, not outcome. Records two gotchas hit while writing it: prettier is not idempotent on markdown containing fenced markdown blocks, and it rewrites bare snake_case identifiers inside them as emphasis. Both fail CI gate 7.
Ran the Stage 0 gate against the publisher origin. Verdict is PASS, so Stage 0 takes the operator-flag path rather than the cache-key discriminator, and there is no live cross-serving defect. The origin declares vary on rsc, next-router-state-tree, next-router-prefetch, next-router-segment-prefetch and Accept-Encoding, covering every header that distinguishes the HTML and RSC representations sharing a URL. It names one the plan did not think to probe. Bodies do not differ by cookie, no Set-Cookie rides a shared-cacheable response, and the origin answers without credentials. Two things the check was not looking for. The origin already sets cache-control: max-age=60 with a correct Vary, so it has been cacheable all along and Trusted Server opted out of it — though a 60 second TTL also bounds the win. And the document regenerates roughly 170 ad-slot container IDs as fresh UUIDs per request, so a cached copy serves identical IDs to every visitor within the TTL. That is probably harmless because slot definitions come from config rather than origin markup, but it is an untested interaction with slot matching and belongs on the pre-flip checklist. Also fixes a defect in the plan's own probe. It compared body hashes, which on this origin differ on every request because of those UUIDs, cookie or not — it would have reported a false FAIL every time. Replaced with normalize-then-diff against a measured no-cookie baseline, and noted that the Host override is required because the origin is a shared vhost.
An external review rejected the previous revision's central conclusion and was right to. Verified against the pinned fastly 0.12.1. ESI was called structurally blocked on two grounds, both false. The cache boundary it needs is native: cache::core provides insert(key, max_age).execute() returning a StreamingBody for arbitrary bytes, lookup()/found() to read them back, and Transaction with must_insert() for request collapsing. No separate KV or template service is required. And purge exists in-process via InsertBuilder::surrogate_keys plus http::purge::purge_surrogate_key, so the management-API token scope previously cited is the wrong surface entirely. The error was inspecting what this repository does and reporting it as what the platform permits, which is the same mistake the document criticises #1009 for making in the other direction. The correction is recorded at the top of the spec rather than quietly edited in. The pipeline ordering was also backwards. It said order esi then lol_html; lol_html is what emits the esi:include tags, so ESI must run after it. New section 6.6 gives the corrected pipeline and separates the three caches the documents had been conflating: origin read-through, shared transformed template, and a final assembled-response cache that must never exist. #418 is React's error number, not a repository issue. The tracker is #938. Stage 0 is reframed as a supporting optimisation and the experimental control, not an answer to #1009 — it has no ESI or client-fill arm, so completing it cannot close the issue. Its rollback claim is corrected: flipping the flag stops HTML reading from cache but evicts nothing, so rollback needs a purge or a versioned key namespace and observation past the origin TTL. Step A is downgraded from PASS to provisional. It used a synthetic session cookie, one route, no experiment variant, and no authenticated session through TS. Cached-hit slot resolution becomes a release gate rather than a note. Adds the ESI validation spike plan: four comparable arms plus a TS-off reference, a deterministic synthetic fragment before the real auction, safety gates run against every arm rather than once at the end, a decision rule ratified before collection, purge-based rollback, and reproducibility metadata.
The docs build was broken and committed. `npm run build` failed with 27 dead links from the spec's relative `../../../crates/...` references; VitePress rejects links outside the docs root and no other spec in the repository uses them. I had only ever run `npm run format`, which does not catch this. Converted to plain code references, matching what every other spec does. Build passes. Four design breaks, all verified against the source before fixing. The shared template was not request-neutral. `tsjs.adSlots` was kept in it on the grounds of being per-URL. Its content is per-URL; its presence is gated on should_run_ad_stack, which depends on consent, bot classification, prefetch status and the auction kill switch. The first request to fill the cache would have frozen its own consent decision into an object every later visitor reads. Both slots and bids now move to the request-aware fragment, the template carries an unconditional inert placeholder, and a test asserts the template is byte-identical across requests differing in consent, bot and prefetch state. The Core Cache pseudocode did not compile. surrogate_keys takes and returns self, so the sample discarded the builder and then used a moved binding; execute() yields a write stream rather than the readable object the next step assumed; finish() was never called; and the key omitted the assembly mode, so the client-fill and ESI arms would have poisoned each other. Replaced with a transaction using execute_and_stream_back, an explicit user_metadata envelope since cache::core carries no HTTP semantics, a cancel-on-error path, and a versioned key. The alternative read-through design is named rather than assumed. The ESI fragment contract was broken. It pointed at /_ts/page-bids, which returns JSON, and ESI splices fragment bytes literally — the page would have contained raw JSON where an executable script belongs. Also: the endpoint's same-origin gate rejects internal subrequests, parent identity and consent context did not propagate, root dispatch was not suppressed so spend would double, and path-only validation admits an attacker authority. The no-C3 gate only forbade public, s-maxage and Surrogate-Control. A bare max-age=60 passes that and is still shared-cacheable — and is exactly what the measured origin sends. Now requires private, no-store positively, tested for returning users, who set no EC cookie and so are not covered by the privacy net. Stage 0 could still ship on provisional evidence: the findings said PROVISIONAL PASS but the plan said Step A had passed and the gate accepted only PASS or FAIL. There are now three verdicts, with FINAL PASS requiring a real session cookie, Basic Auth through TS, the experiment variant, representative routes and cached-hit render attribution. Methodology: A3 and A2 are no longer compared on root TTFB, since both serve the same template — the comparison is bids-ready, adInit fire and first attributed creative paint. Sample plan gains allocation, randomization, pilot variance, MDE and power, CI method and carryover control. Correlation becomes a lineage ID carrying the experiment arm through fragment and auction telemetry, since a root-only ID never reaches an auction that runs in a subrequest. C1 and C2 cache status are recorded separately. DCA now calls the setters rather than commenting that defaults suffice, and fragment caching is disabled. Corrected: Viceroy 0.17 does support cache::core locally; only the customized HTTP read-through hooks are unsupported. Also removed leftovers claiming KV latency for what is a cache, and a config-only rollback.
Six blockers from review, all verified against the source before fixing. The three-verdict Stage 0 gate was only half propagated. The findings template still offered PASS/FAIL and routed PASS straight to the flip, and the spec still approved Stage 0 on the Vary check alone. Both now use FINAL PASS / PROVISIONAL PASS / FAIL, and Task 5a is titled for FINAL PASS so the gate cannot be read past. The spec contradicted the spike on request-neutrality, which would have recreated the leakage bug the spike exists to avoid. It still described adSlots as per-URL, kept it in the template, and drew two markers. New section 6.7 gives the rule: content is per-URL, presence is gated on should_run_ad_stack and is therefore per-request, so it must live in the fragment. The correction-table row, the pipeline diagram, the disposition table and the appendix all point at it. The Core Cache example still would not compile and mishandled stale entries. It called Found::to_body, which does not exist — the accessor is to_stream and it is fallible. Worse, it tested found() before must_insert_or_update(), but a stale entry sets both: that ordering serves stale bytes and never fulfils the update obligation, leaving concurrent waiters blocked. Reordered, with abandon plus cancel_insert_or_update on transform failure and an explicit note that the stale state machine is the caller's to write. The finalization order was impossible. The plan streamed ESI output into the client body while claiming EC, geo and privacy headers finalize afterwards; streaming responses on this adapter commit headers first and then pipe chunks. The invariant is now stated the only way it can work: finalize every header, including an unconditional private/no-store, before any body byte is written. The decision rule adopted A3 on the metric the same document forbids. A2 and A3 serve the same template, so root TTFB is near-identical by construction. The rule now turns on bids-ready, adInit fire and first attributed creative paint, with root TTFB kept only as a non-regression guard. Added a request-scoped arm allocator, since a global setting yields sequential blocks and confounds arm with time of day and cache warmth. Operational: Stage 0's rollback pointed at Core Cache surrogate keys, which belong to the transformed-template cache the spike builds and have no effect on the HTTP read-through cache Stage 0 turns on. Purging that needs origin-supplied keys or the HTTP cache's own surface, and until one exists the rollback is waiting out the origin TTL — now recorded as an accepted risk rather than a discovery during an incident.
…sweep Four contradictions found by a mechanical sweep, all verified before fixing. Stage 0 was still summarized as gated only by the Vary check in the spec's decision table, and as reverting with a config push alone in the plan's Task 5 preamble. Both now point at FINAL PASS and at the full flip-purge-observe sequence. The findings still attached C1 rollback keys using InsertBuilder::surrogate_keys, which is the Core Cache API and keys the transformed-template cache the ESI spike would build. It has no effect on the HTTP read-through cache Stage 0 turns on. The spec's invalidation table had the same ambiguity in a row that read fine in section context and wrong when quoted; it is now split into explicit C1 and C2 rows. The Core Cache pseudocode still would not compile after the previous fix. The error arm referenced a writer only the success arm bound, and a helper taking &tx could not call Transaction::insert, which consumes self. Restructured so everything fallible that does not need the writer happens before insert, where cancel_insert_or_update is still reachable, and so finish and abandon are each reached from the arm that owns the writer. The safety gate still asserted privacy finalization runs after assembly, contradicting the streaming rule added directly above it. Headers commit before the body streams on this adapter, so the gate now asserts finalization happened first, including an unconditional private/no-store. The Task 3 file list still said markers go at two seams while the corrected design emits one unconditional body-close marker. Adds scripts/docs-invariants.py and makes it a named gate in both plans. Format and build are necessary but neither can see a claim corrected in one document and left standing in another, which is how every one of the last four review rounds found real defects. The checker is context-aware, since qualifying text usually wraps to an adjacent line, and it is meant to grow a check whenever a correction lands.
…lse-green The checker added in cf204f0 reported 8/8 green on documents that still contained the contradictions it claimed to check. That is worse than having no checker: it certifies bad state. Three causes, each now addressed. It matched literal phrases. The stale text said "two existing injection seams", the pattern looked for "two seams". Patterns are now semantic and tolerant of wording. It matched line by line, so any phrase wrapped across a line break was invisible. Files are now whitespace-normalized before matching, which is how the architecture arrows spanning several lines were being missed. It had no way to know it had stopped working. Every check now carries fixtures: strings that must trip it, and corrected strings that must not. The script exits 2 and refuses to report anything if its own fixtures fail. Writing them caught two of my patterns not firing at all — one defeated by markdown emphasis between "Verdict:" and "PASS", another by a sentence boundary. Proof rather than assertion: run against the cf204f0 tree, the new checker flags all five contradictions there, including the four this review named. The old checker reported that same tree green. The stale text itself. The spike's architecture summary still said two injection seams and ordered assemble before finalize. The spec still described the cheap curl as gating Stage 0, mapped the Vary result straight to a config push, and summarized rollback as config-only in the priority section. Its pipeline diagram contradicted its own caption — the caption said headers finalize first while the arrows still read assemble then finalize. That diagram is a good example of why literal matching failed and why diagrams need checking as prose does. Also disambiguated the Stage 4 note, which cited InsertBuilder::surrogate_keys without saying it keys C2 rather than the C1 read-through cache Stage 0 turns on.
Three structural fixes, no content changes. The title said "ESI and the Cacheable Root" while the actionable front of the document — sections 1 through 4 — is entirely Stage 0. ESI now lives in one section, one appendix, and mostly in a separate plan. Retitled to match. The filename keeps its esi- prefix deliberately: the commit history and every cross-reference point at it, and renaming would cost more than the mismatch. Added a document map. Three documents answer #1009 and nothing said which owns what, which is the seam every cross-document contradiction has appeared in. It also tells a reader arriving from the issue where the ESI answer actually is, rather than leaving them to infer it from a Stage 0 design document. Consolidated the staging. Stage 0 lived in section 4 while Stages 1 through 5 lived in section 7, so the sequence was split across two places, and Stage 5 had become an entry that read "superseded, see the other plan" — a staging list containing something that is not a stage. There is now one table, Stage 5 is gone, and ESI is stated as running independently of Stages 1 through 4 rather than queued behind them. Two stale "Stages 3b-5" ranges followed from that and are corrected.
The cheapest falsifier for #1009 clears. esi 0.7.1 compiles clean on Rust 1.95.0 for wasm32-wasip1, all six clippy targets pass, format is clean, and the integration-tests crate still resolves. ESI is not blocked by this toolchain. Nine new transitive dependencies, none of them displacing an existing one: esi, nom 8, rand 0.10, rand_core 0.10, chacha20, cpufeatures, atoi, html-escape, md5. regex stays at 1.12.4, bytes at 1.12.0 and log at 0.4.33. nom and rand gain new majors that coexist with the versions already in the tree rather than replacing them, which is the outcome that keeps this cheap — a forced bump on a shared dependency is what would have made it expensive. The dependency is added and unused. It belongs to the Fastly adapter rather than trusted-server-core, because the crate is hard-bound to fastly::{Request, Response, Backend} and core has to stay portable across the four adapters. Also corrects a claim in the spike plan that this task falsified. Step 3 told the implementer to check for a desync between the root lockfile and one at crates/trusted-server-integration-tests/Cargo.lock. That file does not exist: the crate is a workspace member and shares the root lockfile, so the hazard cannot arise in that form. The step now checks the thing that does matter, which is whether an existing shared dependency was forced to move. Compiling is not working. Nothing here exercises cache::core, ESI assembly, or any runtime behaviour, and Tasks 2 onward are untouched.
You asked whether a Fastly test service is really needed. Probed it rather than reasoned about it: Viceroy 0.17 implements the whole Core Cache surface this spike uses. A temporary test under cargo test -p trusted-server-adapter-fastly --target wasm32-wasip1 exercised insert/finish/lookup/to_stream, and then the shape Task 3 Step 4 actually specifies — Transaction::lookup, must_insert_or_update, insert(...).surrogate_keys(...).execute_and_stream_back(), and hit-after-insert. All passed. The probe is removed; the result is recorded in the findings. So provisioning is not a prerequisite. An earlier revision made it Task 2 and a blocker on everything downstream, which would have stalled the spike on infrastructure it does not need yet. Almost all the correctness and safety work runs locally: the C2 cache logic, the transform, template byte-identity, ESI assembly (the crate is pure Rust over BufRead/Write), DCA and dispatcher refusal, fragment-failure degradation, header ordering, and the leakage gates. Task 2 is now scoped to what genuinely needs a real service and is no longer on the critical path; the dependency graph reflects that. Two caveats recorded rather than glossed. Viceroy is a single instance, so a passing Transaction test proves the API works and not that request collapsing behaves under load. And local timings are meaningless for Task 7's decision rule — every performance number still needs the real service.
prk-Jr
marked this pull request as draft
August 10, 2026 13:45
First implementation step of the #1009 ESI spike. No behaviour change: the mode defaults to Inline and every existing path is unaffected. AssemblyMode lives on CreativeOpportunitiesConfig as Option<AssemblyMode> with skip_serializing_if, following the section_root pattern already established there. The reason is in that struct's own doc comments: these types use deny_unknown_fields, so a pushed key makes an older binary fail configuration load. Keeping the key absent when unset means a deployment that never sets it stays rollback-compatible. A test asserts the unset value is not serialized, so that property cannot regress silently. The head seam now goes through template_ad_slots_script rather than an inline conditional. Under Inline it keeps today's behaviour, emitting adSlots only when the ad stack runs, which is correct for a response that is never shared. Under ClientFill and Esi it returns None unconditionally, because should_run_ad_stack folds in consent, bot classification, prefetch status and the auction kill switch. A shared template that emitted conditionally would freeze the first-filling request's decision for every later reader: a consent-denied fill would serve a no-ads template to consenting users, and a consenting fill would serve ad markup to someone who refused. Three tests, and the shape of them matters. An absence-of-per-user-values scan would have passed the broken design, because adSlots content really is derived from config and path. What catches it is byte-identity across requests differing only in the gating decision, so that is what is asserted — including across differing slot matches. The inline test exists so a future change cannot make the shared-mode assertions pass by breaking the shipped path. Extracting the decision as a pure function is deliberate: it makes the invariant testable without driving the whole pipeline, which is what let these tests be written before any cache work exists. Verified: fmt, all six clippy targets, and all four adapter suites, including 1838 core tests under Viceroy.
Task 3 Step 3 of the #1009 ESI spike. No behaviour change: under the default Inline mode the gate reports InlineMode and does nothing. cache::core is not an HTTP cache. It stores whatever bytes it is handed and rejects nothing, so every safety condition belongs to the caller. c2_bypass_reason enumerates them rather than leaving them implicit: an authorized request, an origin Set-Cookie, a non-shareable Cache-Control, a non-200 status, and a non-HTML content type. Leak vectors are checked before mere ineligibility so an operator reading the log sees the security reason and not a content-type quibble. A DataDome block needs no separate detection — it replaces the document with a 403 and the status check covers it. There is a test saying so, because the next person will otherwise go looking for a marker that does not exist. Extracted is_uncacheable_by_cache_control into response_privacy rather than writing a third copy of the private/no-store predicate. It was already duplicated verbatim in both arms of the cookie-privacy net; this replaces both. The helper deliberately does not treat no-cache as disqualifying, because no-cache means revalidate before reuse rather than do not store, and the cookie-privacy net's reading is the correct one for HTTP. The C2 gate checks no-cache separately, as the stricter reading is right for a spike-owned cache we control. The gate has a real call site that logs its decision rather than an allow(dead_code). Clippy pushed back on the annotation and was right to: an #[expect] could not be satisfied in both the lib and test targets, and the honest answer was to wire it. Logging makes the decision observable during the spike instead of only once it starts mutating requests, and Authorization is captured before the origin send consumes the request. Verified: fmt, all six clippy targets, all four adapter suites, 1846 core tests.
Fixes a defect the previous commit introduced. Gating the head seam on template neutrality made ad_slots_script None under the shared modes — and the body-close element handler read exactly that value to decide whether to inject at all. So shared modes silently stopped injecting anything at </body> as a side effect of a change to <head>. Safe, since emitting nothing cannot leak, but wrong for the reason the spec warns about: the gate has to be "did this response carry bids", not "does this page have slots". BodyCloseInjection replaces the inference with a named decision — None, InlineBids, or Marker — chosen by body_close_injection() at a site that knows the assembly mode. No new struct field was needed: settings is already threaded to all three processor-construction sites, so the mode is derivable there. Behaviour is unchanged. Inline still injects when slots matched and stays quiet when they did not. Esi deliberately returns None rather than a placeholder marker. The marker has to point at a fragment endpoint returning an executable script; /_ts/page-bids returns JSON and ESI splices fragment bytes verbatim, so aiming at it would put raw JSON where a script belongs. That endpoint does not exist yet, and a marker with nothing behind it is worse than no marker. A test pins the current answer so it changes deliberately rather than silently. The most useful test asserts body-close is identical whether or not the head script is present, under both shared modes. A decision that read the head script would be accidentally correct there today — because the head script is always absent under those modes — and wrong the moment that changes. Seven config literals in tests plus one in a benchmark now state their intent explicitly instead of relying on the old inference, which is the improvement rather than a cost. clippy --all-targets caught the benchmark; test runs alone did not. Verified: fmt, all six clippy targets, all four adapter suites, 1850 core tests.
Steps 1, 2, 2b and 3 are done and behaviour-neutral under the default Inline mode. Step 2c (emit the Esi marker) and Step 4 (the cache read/write) are not, and the record says why rather than leaving them looking merely unstarted: the marker needs a fragment endpoint returning an executable script, and Step 4 is blocked on a design choice the plan deliberately defers. Records the defect this work introduced and then caught. Gating the head seam on neutrality made ad_slots_script None under shared modes, and the body-close handler read that value to decide whether to inject at all — so shared modes silently stopped injecting at </body> as a side effect of a <head> change. Found by reading the handler while starting the next step, not by a failing test. It is the same shape as the bug the whole task exists to prevent: something that looks correct and quietly does nothing. Also records what the coverage does not cover. Fourteen tests prove tsjs.adSlots is request-neutral. They say nothing about the other things injected at the same seam — integration head_inserts, the gpt-diagnostics bootstrap, the RSC placeholder rewriter — which the spec flags for audit and which is still outstanding. Request-neutrality is asserted for one element, not established for the template, and reading the test names would suggest otherwise. And a gate note: clippy --all-targets caught a benchmark construction site that all four test suites missed.
The plan left this open between fastly::cache::core and read-through caching with after_send plus set_body_transform. Investigated and verified against the pinned SDK and Viceroy 0.17 source. Read-through is not viable here, on three hard blockers rather than on preference. Viceroy stubs the entire HTTP Cache ABI, and the SDK converts that into a send error rather than a fallback: is_request_cacheable returns NotAvailable, which makes must_use_host_caching true, which with a send hook set returns HttpCacheApiUnsupported. Setting after_send therefore makes every publisher origin fetch fail under fastly compute serve, cargo test-fastly, and the parity suite. The whole local loop dies. with_cache_bypass makes the hook silently dead anyway. get_caching_mode checks cache_override.is_pass() first and returns host caching, so after_send is never invoked and no error is raised — on exactly the requests in scope, quietly. And the closure bounds are incompatible with this codebase. with_after_send requires Fn + Send + Sync + 'static, while everything the rewriter needs is !Send by construction, which is why the platform layer is async_trait(?Send) throughout. set_body_transform is also synchronous and so could never await the auction collect. Recorded rather than merely chosen, because read-through's appeal is real — CandidateResponse::apply_and_stream_back is execute_and_stream_back with HTTP semantics attached — and someone will otherwise propose it again. Also settled: core cannot reach it at all, since PlatformHttpRequest has no callback slot and adding one would name Fastly types in portable core. Adds the exact insertion point, the one required hoist, and four risks the investigation surfaced that are specific to this codebase: Vary is in the key list but c2_bypass_reason does not check it; store bytes plus a metadata envelope and rebuild every header on a hit rather than replaying origin headers into a path that strips them; Content-Encoding and host/scheme both belong in the key. Plus a follow-up to file rather than fix: the auction is dispatched before the lookup, so under the shared modes it is already pure waste. Tee-ing turns out to be unnecessary. With any post-processor registered — and Next.js always registers one — the transformed document arrives as one contiguous buffer, so it is two write_all calls on the same slice. Keep execute_and_stream_back for transaction correctness and request collapsing, not for memory. Corrects the findings document: Viceroy implements purge_surrogate_key against the same in-process cache, so C2's purge-based rollback is locally testable. C1's, which is what Stage 0 exposes, still is not.
Three findings from a code review of the four preceding commits. Two are fixed here; the third waits on an audit that is still running. The auction dispatch was never gated on AssemblyMode. assembly_mode was computed after the dispatch decision, so flipping to client_fill or esi today would still send real SSP bid requests, hold the response for the full auction budget, and then discard the result — because both injection seams now return None — with no error, no warning and no log. That is precisely the silent-waste signature §5 of the design doc is about, reached by an incomplete feature flag rather than by removing the hold. assembly_mode is hoisted above the dispatch, which the C2 design investigation wanted anyway, and root_auction_is_useful gates it. The interesting test there does not assert per-variant. It derives the invariant: a root auction is useful exactly when a seam will consume its result. A new mode cannot make the dispatch gate and the injection decisions disagree without failing it. c2_bypass_reason omitted the forwarded client Cookie, which the design doc's own §4 names as a leak vector and the plan's checklist also missed. TS forwards client cookies to origin unchanged with no strip on the publisher path, so a response can be cookie-personalized while carrying no Set-Cookie itself, having no Cache-Control at all, and being a 200 HTML — every other condition reports it cacheable. Now disqualifying until an origin Vary covering Cookie is verified. The test uses exactly that shape rather than a response that would fail some other condition anyway. Also folds the duplicated Cache-Control lookup into one pass. The previous version built a lowercased copy and then called is_uncacheable_by_cache_control, which re-fetched and re-lowercased the same header. Not fixed here: the head seam still injects integration head_inserts and the gpt-diagnostics bootstrap unconditionally, so request-neutrality is asserted for adSlots only. It happens not to leak today because gpt_diagnostics::finalize_response stamps private/no-store before the C2 gate reads headers — a load-bearing coincidence that is undocumented and untested. A neutrality audit covering that seam is still in flight; fixing it on partial information would mean doing it twice. Verified: fmt, all six clippy targets, all four adapter suites, 1853 core tests.
Closes the third finding from the code review. The head seam still injected request-scoped content under the shared modes, so request-neutrality was asserted for adSlots alone. Audited the seam. Of the two remaining injectors, integration head_inserts is clean: all three implementations take the context parameter unused, so their output depends on configuration and not on the request. GPT diagnostics is not clean — it is activated by a cookie or query parameter and is documented as an immutable request-scoped decision. It does not leak today, but only by coincidence. requires_private_no_store is a strict superset of the conditions under which either script is emitted, and the resulting private/no-store stamp lands before the C2 gate reads response headers, so the gate refuses. Two independent conditions that happen to align, with nothing enforcing the relationship and no test covering it. Fixed on both sides. The processor now receives no diagnostics decision under the shared modes, so the guarantee is explicit rather than emergent. And a test enumerates every combination of the decision's three fields and asserts that anything which injects also requires the stamp — so if a future change emits a script without requiring private/no-store, it fails there rather than silently in a cached template. Keeping both is deliberate: the gate is the guarantee, the invariant test is the backstop if the gate is ever removed or bypassed. Verified: fmt, all six clippy targets, all four adapter suites, 1855 core tests.
Three HIGH findings, all closed in the preceding two commits. Recorded with the reasoning rather than as a list, because two of them were holes in the plan's own checklist and not merely in the implementation. The cookie gap is the clearest case: the implementation matched Task 3 Step 3's checklist exactly and still had the hole, because the checklist itself omitted the forwarded client Cookie that §4 of the design doc names. Also records what the review says about the tests. All three findings were in code the existing tests covered and passed, because those tests exercise the pure decision functions with hand-built inputs and never the rendered head or body-close bytes. That is still true — no test renders a full document through create_html_processor and compares two requests byte-for-byte, which is what the plan's Task 3 Step 2 actually requires and the most valuable test still missing. Adopts the reviewer's gate: no Task 3 Step 4 and no exposure of AssemblyMode to test or staging traffic until that test exists. The three fixes close the known holes; the test is what would catch the next one. Also notes the audit result for integration head_inserts, which is clean — all three implementations ignore the request context — so the neutrality gap was specific to diagnostics rather than general to the seam.
Closes the gate the review left open, and the one the plan's Task 3 Step 2 actually asked for. Every other test in this area exercises the decision functions with hand-built inputs. That is how three HIGH review findings sat in covered, passing code: the decisions were individually right, and nothing checked what composing them renders. These tests build the config exactly as create_html_stream_processor does — same three decisions, same order — render a document through create_html_processor, and compare bytes across every combination of ad-stack gating, diagnostics activation, and bid availability. Extracted template_gpt_diagnostics so all three decisions are named functions the test can compose, rather than one of them being an inline match the test would have to duplicate. Duplicating it would have made the test agree with itself instead of with production. Mutation-tested both gates rather than trusting that passing tests mean anything. Reverting the diagnostics gate fails two of the three; reverting the head-seam gate fails the same two; the inline control passes in both cases. So the tests detect each gate independently and can still tell varying from non-varying output. Three tests rather than one, because byte-identity alone is satisfiable by rendering the same wrong thing every time. The second asserts the specific request-scoped markers that must be absent, and the third asserts inline still varies — if that one ever passes trivially, the harness is not rendering what it claims to. Adds a cfg(test) constructor for an active diagnostics decision, since the fields are private and built from a cookie or query parameter, with no other way to obtain one across a module boundary. Verified: fmt, all six clippy targets, all four adapter suites, 1858 core tests.
First half of Task 3 Step 4, in portable core. No Fastly implementation yet and no call site, so nothing changes behaviour — this is the shape the adapter will fill in. Follows the PlatformKvStore pattern the repo already uses four times for a Fastly-only capability behind a portable trait with a null object. The null object reports Unsupported rather than erroring, so the shared assembly modes degrade to transforming per request on Cloudflare, Axum and Spin instead of failing there. The modes stay portable; only the caching does not. The key is where the correctness risks live, and it carries the four the design investigation surfaced. Assembly mode, because the client-fill and ESI arms emit different bytes and would otherwise poison each other's entries. Content encoding, because the pipeline pairs input encoding to output encoding, so serving brotli bytes to a client that asked for gzip is a broken response. Host and scheme, because both reach IntegrationHtmlContext and drive URL rewriting. And a schema version, so a deploy that changes the transform reads a miss rather than assembling against markers that moved. Vary values are carried as the origin listed them rather than as a fixed list, because the origin is authoritative and a hard-coded list would drift silently when the origin's changes. Step A already measured four Next-specific headers this branch did not anticipate. Fields are length-prefixed rather than delimiter-joined. A delimiter is ambiguous when a value can contain it, and two distinct keys colliding here means one visitor's template served to another. There is a test for exactly that collision. Metadata is a small envelope rather than stored origin headers. The publisher path forces private/no-store and strips validators after the send, so replaying a stored origin header would fight it; rebuilding every header on a hit means no origin header is ever replayed and the Set-Cookie privacy net stays trivially safe. Malformed metadata decodes to a miss rather than a partial read. Eight tests. The one worth naming asserts every field changes the key — a field that does not is a cross-serving bug, and that property is easy to break by adding a field and forgetting to hash it. Verified: fmt, all six clippy targets, all four adapter suites, 1866 core tests.
Completes Task 3 Step 4. The cache is constructed and reachable through RuntimeServices but has no caller yet, and the assembly mode defaults to Inline, so nothing changes behaviour. Seven tests run against real Core Cache under Viceroy, including purge. That is what the earlier probe established was possible and why provisioning a Fastly service is not on the critical path. Two ordering traps, both caught by the earlier reviews and both real here. must_insert_or_update is tested before found, because a stale entry sets both and checking found first would serve the stale bytes while never discharging the obligation, leaving concurrent waiters blocked until timeout. And get uses a plain lookup rather than a transaction, because a read that never intends to insert must not take an obligation it will not discharge. Transaction::insert takes self, so once the insert begins there is no handle left to cancel it with — a write that fails part-way cannot be retracted. Rather than write a cancel call that does not compile, or pretend the hazard is absent, the metadata carries the intended body length and get rejects a short entry as Truncated. put also refuses a length that disagrees with the body it was given, since storing that would make every subsequent read a truncation miss: a cache that silently never hits. Also treats a stale entry as a miss. Serving stale while revalidating is a real option but it is a state machine cache::core does not implement, and it is not what this spike measures. The trait is Send + Sync with ?Send futures. RuntimeServices lives in a LazyLock static so the trait object must cross threads, while the platform layer is !Send by construction and the futures never do. Wired into RuntimeServices following the kv_store pattern, but defaulted rather than required: an adapter with no template cache should degrade to transforming per request, not fail to build. That is what keeps the shared modes portable across all four adapters with only the caching being Fastly-only. Verified: fmt, all six clippy targets, all four adapter suites, 1866 core tests and 123 Fastly adapter tests.
Wiring the key builder surfaced a problem the plan states but does not solve. The key must cover everything the origin varies on, or two requests needing different templates share one entry. But a lookup happens before the fetch, so on a cold key the origin's Vary is not yet known. Three ways out, recorded in the type's own docs so the trade-off is visible at the call site rather than buried here: configure the list, two-phase lookup with a URL-keyed record holding the last-seen Vary, or store the list alongside and re-key on mismatch. The latter two are correct and double the lookups on every request. Configured is chosen, and it is a spike-grade choice rather than a production one. Step A already measured the origin's actual Vary, and the spike TTL is short, so drift is bounded by a minute rather than being indefinite. The drift is guarded rather than merely accepted. uncovered_by runs after the origin responds, when its Vary is finally known, and reports which names the configured spec missed. A template built under a key that did not cover something the origin varies on is unsafe to store, because a request differing only in that header would read it. Reporting the specific names means a stale config is identifiable rather than producing a generic refusal. Two details worth their tests. An absent header and a present-but-empty one are deliberately keyed the same, since the origin sees no difference. And Vary: * is not reported as a named gap — it means uncacheable, which the eligibility gate handles, and reporting it would produce a nonsense instruction to configure a header called *. Verified: fmt, all six clippy targets, all four adapter suites, 1870 core tests.
Closes the loop the previous commit opened. VarySpec could detect drift but nothing called it, so the cache key remained free to under-cover the origin's Vary — the gap this plan's Step 4b recorded as open. c2_bypass_reason now takes the configured spec and reports VaryNotCovered, carrying the header names rather than a bare flag so a stale config is identifiable from the log line instead of requiring a bisect. It sits among the leak vectors rather than the eligibility checks, because storing under an under-covering key is cross-serving: a request differing only in the uncovered header would read that template. The spec is operator config, not a constant. The origin's Vary is a property of a particular deployment, and hardcoding one would be an invented value dressed as a default. Unset yields an empty spec, which covers nothing — so any Vary at all disqualifies and no template is cached. That is the intended default rather than a degenerate case: a deployment that has not stated what its origin varies on must not acquire a shared cache by omission, and every real origin varies on something, so fail-closed is the common path. C2BypassReason loses Copy, since it now carries the names. VaryGap is a newtype so the reason stays Display-able as one line. Four tests, two of which cover mistakes easy to make here: a Vary split across repeated headers must not hide names behind the first value, and a fully covered Vary must still be cacheable rather than the guard rejecting everything. Verified by mutation: reading only the first Vary value, and disabling the check entirely, each fail the new tests with the other ten gate tests still passing. Full gates green — fmt, six clippy targets, four adapter suites, 1874 core tests.
The cache had a backend and a gate but no call site, so nothing was ever written. This adds the store half. The gate now builds a key instead of only logging, and the key travels on the streaming params. Its presence is the store authorization — there is no second place that could disagree with the gate, and no path to the cache that has not passed it. store_template_if_authorized takes the key rather than borrowing it, so one request stores at most once even if the layered finalizers both call it. The gate moved below the content-encoding computation because the negotiated encoding belongs in the key. The pipeline pairs input encoding to output encoding, so a template stored as brotli must never be handed to a client that asked for gzip. The URL and the Vary-named request headers are captured before the request is consumed, reading the request as forwarded rather than as received: keying on a value the origin never saw would be keying on the wrong thing. Storing needs every transformed byte, and streaming hands bytes to the client as they are produced rather than collecting them. Shared modes therefore take the buffered finalizer, which already materializes the body. That branch keys on the store authorization rather than on the assembly mode, so Inline never reaches it and the spike cannot regress the shipped path by construction. The cost is that a C2 miss buffers, which is the right trade: a miss is already paying an origin fetch and a full transform, and what the spike measures is the hit, where there is no origin fetch to stream from at all. Store failures are logged and swallowed. A cache that cannot be written is a slower service, not a broken one, and C2's premise is that the response is reproducible without it. Three tests against a recording cache, covering the two ways this could silently break: storing without authorization, and storing twice for one request. Full gates green — fmt, six clippy targets, four adapter suites, 1877 core tests. Still open: the lookup. Nothing reads these templates yet.
Completes the chain. The store landed last commit; nothing read it back, so the cache was write-only and saved nothing. Wiring the lookup forced a correction to the key. It carried the content encoding the origin chose, which is unavailable at lookup time — the origin has not responded yet. Keying on it meant storing under `br` and looking up under `gzip, br`: a cache that never hits. The field is now the Accept-Encoding sent to the origin, renamed to say so. This is sound because origin negotiation is a function of what it was offered, so identical offers yield identical choices; the encoding actually chosen stays in the metadata and is what the served response declares. That change makes every key field request-derived, so the key is now built before the fetch and the response gate only authorizes storing it rather than constructing it. A key that needed the response could only ever authorize a store, never satisfy a read. The lookup re-checks the request-derived disqualifications and only those. The store gate is response-derived and cannot re-run here, but it does not need to: anything in the cache passed it on the way in. What must re-run are properties of the *reader* rather than of the bytes — an authenticated request must not be served a shared template even when that template is perfectly cacheable. Every response header on a hit is constructed, never replayed. The publisher path rewrites Cache-Control and strips validators after the send, so a replayed origin header would fight it, and constructing them means no origin header can reach a second visitor through the cache. Three end-to-end tests exercising the real finalizer, since the store only happens once the transform has produced every byte: a second request is served without touching the origin and is byte-identical; Inline never reads or writes; an authenticated request is refused the shared template. Verified by mutation. Disabling the lookup fails the hit test, so the hit is real rather than the fixture answering twice. Dropping the Authorization check fails the authenticated test, with the other two still passing in both cases. Full gates green — fmt, six clippy targets, four adapter suites, 1880 core tests.
The publisher tests use an in-memory double and the Fastly tests call the concrete type, so the seam between them was only ever type-checked: the publisher reaches the cache as a dyn PlatformTemplateCache behind RuntimeServices, which is what app.rs wires and what neither suite executed. Now round-tripped through the trait object under Viceroy against the real Core Cache. Records Task 3 as complete in the plan and the findings, including the three problems that only appeared once the code had to run — the Vary ordering, the encoding the origin chooses versus the one it is offered, and streaming not collecting the bytes a store needs. None were visible in the plan or in review, which is the same pattern as the earlier review findings arriving one layer down. Docs build verified, not just formatted.
The Esi arm emitted nothing at </body> because pointing an esi:include at /_ts/page-bids would splice raw JSON where an executable script belongs. This adds the script form and the marker that uses it. A format on the existing endpoint rather than a second path. Both forms carry the same data behind the same cross-site gate, so a new path would have duplicated that gate, the deprecation alias and the private, no-store header across four adapter routers for a difference in wrapping. As a format it is reachable on every adapter with no routing change at all. An unknown format is a 400, not a fall back to JSON. Defaulting would make a typo in an esi:include return 200 with a broken page and nothing in the logs pointing at the cause — the precise failure that kept this arm dark. The fragment reuses build_bids_script rather than formatting its own. If the two diverged, the A1-vs-A3 comparison would be measuring two script shapes instead of two delivery mechanisms and its number would mean nothing. Slots are not included: under a shared mode the head seam emits no tsjs.adSlots and the template already carries the slot markup, so the fragment supplies only what could not be shared. The marker carries no path. It is baked into a shared template, so every byte in it is a byte every reader of that template receives; the adapter's include dispatcher will supply the path from the live request. That also keeps a URL out of the cached bytes, so there is no escaping question at the seam. An existing test caught a real inconsistency this exposed. The root-auction gate asserted that dispatch usefulness tracks whether the seam emits bytes — true only while Esi emitted none. Under a Marker the seam emits bytes and reads nothing from ad_bids_state, because the fragment runs its own auction, so the old reading would have dispatched a root auction with no consumer: silent SSP spend, the exact waste that gate exists to prevent. The invariant now distinguishes emitting from consuming. Full gates green — fmt, six clippy targets, four adapter suites, 1889 core tests. Still open: the Fastly ESI processor is not wired, so the include is emitted and never resolved. Task 5.
Arm A3's mechanism, verified under Viceroy with the real esi 0.7 crate rather than argued from the docs: a template carrying the </body> seam's own esi:include comes back with the fragment spliced in its place and no unresolved tag left. The obstacle was that esi's fragment dispatcher is synchronous while this codebase's fragment producer is async, and calling async from inside the dispatcher means a nested executor, which panics. PendingFragmentContent::CompletedRequest is the way out: the dispatcher may hand back a response that was already built. So the caller resolves the fragment in the normal async flow and passes the bytes in, and the dispatcher performs no I/O at all — no subrequest, no backend, no self-call, nothing for Viceroy to stub. Five tests, each pinning something whose failure would be silent. The template is built from ESI_BIDS_INCLUDE rather than a hand-written tag, so a change to the seam's shape cannot leave this passing against a marker nothing emits. Position is asserted, because an assembler that appended rather than substituted would produce a page that parses and does nothing. An empty fragment must still consume the marker, since an auction returning no bids is normal and a leftover tag renders as visible text. And the verbatim splice is pinned, because that is exactly why the fragment endpoint has to return markup rather than JSON. derive_more added to the adapter, matching the workspace convention. mime avoided by setting the content type header directly rather than taking a dependency for one constant. allow(dead_code) at module scope, not expect: the tests exercise this code and the binary does not, and no single attribute can be both fulfilled and unfulfilled. Full gates green — fmt, six clippy targets, four adapter suites, 129 Fastly adapter tests, 1887 core tests. Still open: the request-path call site. Emitting the include and resolving it are now both proven; connecting them is Task 5's remainder.
The assembler used Configuration::default(). The plan's Task 5 Step 2 says explicitly not to, and reading the crate showed why that instruction exists. is_includes_cacheable defaults to true. A fragment here carries one visitor's bids, so letting the ESI layer cache it serves those bids to the next visitor — the exact per-user leak this whole design exists to prevent, arriving silently on a cache hit. That default fails open, in a pre-1.0 crate whose defaults can move in a patch release. Every safety-relevant field is now stated rather than inherited: - Fragment caching off, and includes_force_ttl left unset — it caches everything, ignoring private, no-store and Set-Cookie alike. - default_dca None and inherit_parent_dca false, so fragment bytes are never re-parsed as ESI. The fragment is a script built from auction data; parsing it as ESI would let bid content act as markup instructions. - max_include_depth 1. One include, no nesting. A template asking for more is not one this arm built. - Rendered caching and edge_control off. The publisher path sets private, no-store before any body byte is written, and headers cannot change once streaming starts on this adapter, so a Cache-Control computed from include TTLs would contradict it — and the contradiction would favour caching. Four tests assert the configuration rather than trusting it, plus one that proves the behaviour rather than the flag: a fragment containing its own esi:include is spliced as text, not dispatched, so auction data cannot drive fragment requests. Full gates green — fmt, six clippy targets, four adapter suites, 133 Fastly adapter tests.
Two findings worth keeping out of commit messages alone. The async/sync obstacle is dissolved rather than worked around: PendingFragmentContent::CompletedRequest means the dispatcher performs no I/O, so the plan no longer needs the self-referencing backend it assumed. And the plan's "call the setters, do not trust the defaults" instruction turned out to be load-bearing: is_includes_cacheable defaults to true, which caches per-user bid fragments and serves them to the next visitor. Docs build verified, not just formatted.
A cache hit returns before the origin fetch, and therefore before the point where the publisher path stamps private, no-store and strips validators. Nothing else set it, so a hit served HTML with no Cache-Control at all. That is not a safe default. HTML with no Cache-Control is heuristically cacheable by browsers and intermediaries, so an assembled per-user response was eligible to be stored and shared — the C3 the design forbids outright, reached by omission rather than by anything anyone wrote. The plan's Task 6 predicted exactly this class of miss. It says assert positively, because forbidding public, s-maxage and Surrogate-Control passes trivially when there is no Cache-Control to forbid. Checking for their absence would have reported this bug as safe. The hit path now stamps private, no-store first rather than last, and two tests assert it. One covers the returning visitor specifically: a first-visit response sets an EC cookie and the adapter's cookie-privacy net force-privatizes it regardless, so a test that only exercised first visits would pass on the backstop rather than on this code. A returning visitor sets no cookie, the net never fires, and this path is the only thing between the document and a shared cache. Verified by mutation: removing the stamp fails both new tests, with the hit and isolation tests still passing. Full gates green — fmt, six clippy targets, four adapter suites.
Closes the gap between emitting a marker and filling it. Both were proven separately; nothing connected them, so a shared-mode page returned 200 with a literal esi:include in it — no ads, no error, and every monitor reporting success. A design correction first. Esi's root auction was gated off on the premise that the fragment would run its own auction via a real subrequest. That premise made the arm strictly worse: a self-referencing backend, two auction code paths, and two auctions per pageview. It is also not what esi requires — CompletedRequest satisfies an include from bytes already in hand. So the auction already in flight *is* the fragment, and root_auction_is_useful(Esi) is now true. ClientFill stays false; the browser fetches its own bids, so a root auction there genuinely has no consumer. Assembly sits behind a platform trait, like the template cache, because the only implementation uses a Fastly-only crate. The default is UnavailableTemplateAssembler, which refuses rather than passing the template through: returning it unchanged is the tempting default and it produces exactly the silent no-ads page this commit exists to prevent. Core's tests use a plain substitution instead, which is what one constant marker reduces to — and which shows the seam is portable even though the crate is not. Two call sites, deliberately not one. The miss path assembles after the transform; the hit path assembles after reading the cache. Keeping them separate is what makes the store-before-assemble ordering visible rather than implied. That ordering also revealed a bug on the hit path: it returned before the pipeline that normally collects the auction, so a hit dropped its in-flight auction — billing the SSPs for a result nobody read, the exact waste the dispatch gate exists to prevent, reappearing on the one path that skips the pipeline. The hit path now collects and assembles. Two tests carry the load. One asserts the marker never reaches the browser on either path, checking both because only one call site running would still look like success on the other. The other asserts the cached template holds the marker and never a bids script. The second one earns its place: swapping store and assemble fails it and nothing else. Every other test still passes, including the marker test, because the served page looks correct — one visitor's bids would simply be in a cache shared with the next. Verified by running that mutation. Full gates green — fmt, six clippy targets, four adapter suites, 1887 core tests, 133 Fastly adapter tests.
Local testing found that C2 never engaged on any page where the ad stack runs — which is every page that matters. Two origin fetches for two requests, and the esi:include served unresolved. TS stamps its own `private, no-store` on the response when should_run_ad_stack is true. The C2 gate ran after that stamp, read it as the origin's declaration, concluded OriginNotShareable, and refused. The gate asks whether the *origin* said the response was shareable, so it now runs before TS writes anything. The test suite could not have caught this, and that is the more important half of the fix. Its fixture left the auction disabled and passed no dispatch slots, so should_run_ad_stack was false in every test, the stamp never fired, and the ordering was unobservable. Every assertion about C2 was therefore made against the one configuration where C2's hardest condition does not apply. The fixture now runs the ad stack for real: auction enabled, plus a slot in AuctionDispatch rather than only in settings, since should_run_ad_stack requires a matched slot and the two are different inputs. sec-fetch-mode: navigate added for the same reason. Verified by mutation both ways. With the old fixture, moving the gate back below the stamp passed all seven tests. With the corrected fixture it fails six. The bug is now observable, which it was not before. Also verified end to end under viceroy serve against a stub origin: one origin fetch for two requests, no unresolved marker on either path, a bids script present in both, private, no-store on the hit, and the cached template 353 bytes against 467 served — so the cache holds the pre-assembly template. Full gates green — fmt, six clippy targets, four adapter suites.
Cross-user leakage, stale revalidation, and transform failure. Each is a hard fail in the plan, independent of any performance result. Cross-user leakage is the one the design rests on. Two synthetic users differing in EC identity, consent jurisdiction and geo must store a byte-identical template — asserted as byte-identity rather than as a list of checks, because that does not depend on guessing which field might leak. Each user runs against a fresh cache, or the first user's entry would answer for the second and the comparison would prove nothing. The forbidden-substring assertions are the second layer: byte-identity would also hold if both templates leaked the same wrong thing. Transform failure covers a partial template reaching C2, which is the worst outcome available here — a truncated document served to every later visitor, indefinitely, with no error after the first request. Safe by construction, since the cap error propagates before the store; "by construction" is exactly the claim that stops holding after an unrelated refactor moves a line. The stale test needed rewriting because the first version passed for the wrong reason. A zero TTL produces an absent entry, not a stale one, so `is_stale()` was never reached — confirmed by reverting the staleness check and watching that version stay green. An entry is only present-and-stale with a stale_while_revalidate window, so the test now inserts one directly. With that fixed, the same mutation kills it. All three verified by mutation, which is the only reason to trust them: leaking adSlots through the head seam fails the leakage gate, storing before the cap check fails the transform gate, and serving stale fails the stale gate. Full gates green — fmt, six clippy targets, four adapter suites.
Task 6's local gates are marked done with what verified each. Two entries carry more than a checkbox. The C3 gate's wording is what caught a live bug, so the original wording is retained next to the result: checking for the absence of public/s-maxage/Surrogate-Control would have reported a hit serving with no Cache-Control at all as safe, because nothing was present to forbid. Request collapsing is left open rather than quietly dropped. Viceroy is single-threaded, so the concurrent cold-request case cannot be produced here; the racing-writer half is covered and the collapsing half is not. The findings document now records the local run and the bug it found — the C2 gate reading TS's own private, no-store as the origin's declaration, which disabled caching on every page that serves ads while every test passed. And the pattern across five bugs on this branch: each compiled, passed every existing test, and was wrong. Three were found by writing the test the plan asked for, one needed a running server, none by review — including my own review of the same gate, twice, in opposite directions. The stale-cache test is that failure in miniature: green while never reaching the branch it named, exposed only by mutation. Docs build verified, not just formatted.
The earlier finding said the </body> hold costs approximately nothing. That is true today and only today, for a reason that stops holding the moment the rest of this work lands: the auction hides behind a slow origin fetch, so the hold costs max(0, auction - origin). Make the root cacheable, the origin fetch disappears, and the auction becomes the entire remaining cost. So the two problems are coupled and neither fix shows a win alone. The issue bundles them as one blocker; they need different fixes. Bids-in-the-body makes the page uncacheable and is done. Holding the response for the auction makes it slow and is not. The current implementation relocates the hold rather than removing it: a C2 hit awaits the auction, then assembles, then returns fully buffered. On a hit that is worse than today in one respect, because there is no origin fetch left to hide the auction behind. Three facts settle the design, each verified in the codebase rather than assumed. The existing streaming path already implements stream-then-stall-at-the-seam and is shipping. EdgeBody::Stream is an async stream, so an await may sit between chunks with no nested executor. BodyCloseInjection::Marker already exists and the streaming finalizers already strip Content-Length. Three designs compared. Buffered assembly is what exists. Native ESI via PendingFragmentContent::PendingRequest is what the crate is built for and is Fastly-only — and it vindicates the original dispatch gate, since under it the fragment request runs the auction and the root must not. Dispatch-usefulness turns out to be a function of the delivery mechanism, which is the non-obvious coupling here. The recommendation is neither: cache the shell with an inert HTML comment sentinel at the seam, split on it at serve time, stream the article, stall only for the auction, then write the bids and the tail. A comment rather than an esi:include because a comment is inert, so a substitution failure degrades to no ads instead of visible text in the page. Two simplifications fall out: store the template decoded and encode at serve time, which removes accept_encoding from the key entirely; and stop setting Content-Length, which is unknowable before bids resolve. The consequence for #1009 is the part worth reading. Its gating question is whether Fastly-first is acceptable for the flagship perf path. This design makes that question unnecessary — the full win on all four adapters, no esi dependency on the render path, no self-referencing backend, no second rendering architecture. ESI is sufficient but unnecessary: for one insertion point at a known location its parsing generality buys nothing a byte split does not. That is the opposite of what the issue expected. Docs build verified, not just formatted.
The architecture doc argued that buffered assembly relocates the auction hold rather than removing it. This reproduces it on demand instead of arguing it. Local run under viceroy serve against a stub origin with a self-imposed slow bid endpoint. The cache demonstrably works — one origin fetch across three requests, one miss, one store, two hits. And time-to-first-byte equals total on every request, so nothing streams: the whole response lands at once, after the auction. On the cache hits the origin fetch is gone and first byte still tracks the injected bid delay exactly. So the reader waits just as long with the cache as without it, which is the whole point at issue. The numbers are synthetic — a delay I chose to make the effect observable — and are explicitly not a measurement of any deployment or comparable to publisher data. The harness also gives the proposed change a pass/fail shape rather than a vague improvement: under streaming assembly, TTFB must fall away from total by approximately the injected delay. That is a binary outcome, observable locally, and it does not need operator access. Docs build verified.
…age gap Both findings come from an independent review that demonstrated them by running code rather than by reading it. A POST to a path whose GET was cached was answered from the cache. handle_publisher_request is the `*`-method fallback route, so a publisher path that renders a page on GET and accepts a form or webhook on POST reaches it for both. The origin never saw the mutating request, the caller got 200 and a page, and nothing reported a problem. A POST is not entitled to a GET's representation. The method check goes at key construction because the key governs both the lookup and the store, so one None excludes non-GET from each. Verified by mutation: removing it fails the new test. The second finding was that ClientFill had no end-to-end coverage at all — every test in the module ran esi or inline, and the neutrality tests recompose the processor's inputs by hand instead of calling create_html_stream_processor. The reviewer reintroduced a diagnostics leak scoped to ClientFill and all 1889 tests passed. Investigating showed that specific mutation is not exploitable, and the reason was already documented on template_gpt_diagnostics: requires_private_no_store() is a strict superset of the condition under which diagnostics markup is emitted, and that stamp lands before the C2 gate reads response headers, so such a request never stores a template. The leak was unreachable — but only by a coincidence between two independent conditions, with the whole protection resting on it. So both halves are now covered. ClientFill runs the two-synthetic-user leakage test through the real pipeline, and a separate test pins the consequence the coincidence produces: an active-diagnostics reader never contributes to a shared cache. Mutation- verified by moving the diagnostics stamp after the gate, which fails it. One wrong turn worth recording: the first attempt gave every navigation an active diagnostics decision, which broke ten tests. That was the code being right — an active decision legitimately disables C2 — so the fixture is scoped to the one test that needs it rather than the invariant being weakened to accommodate it. Full gates green — fmt, six clippy targets, four adapter suites.
Two review findings, both about the cache refusing to cache anything real rather than about it caching too much. Vary: Accept-Encoding disqualified everything. The key has a dedicated accept_encoding field, so an origin declaring it is already keyed correctly — but the coverage check only consulted the operator-configured list, so it reported an uncovered gap and refused. Any compressing origin sends that header, so C2 would have stored nothing at all unless the operator redundantly listed a header the key already covers. Worse than a bug: the spike would have measured a hit rate near zero and read it as a result. Headers the key covers structurally are now excluded from the gap check. Cookies excluded essentially every repeat visitor. Any request carrying any cookie was disqualified in both directions, and TS sets its own identity cookie, so the population that could ever see a warm hit was roughly first-ever page views and cookie-less clients. The design notes called this the first-nav exception; it is the common case, not the exception. That one is a privacy gate, so it is opt-in and fail-closed rather than relaxed. origin_is_cookie_independent lets an operator assert their origin serves the same HTML with or without cookies. The assertion is not taken on trust: if the origin ever declares Vary: Cookie, the drift guard reports an uncovered header and the response is refused regardless of the flag. A wrong assertion is therefore caught whenever the origin is honest about it, and the flag only widens the window where an origin personalizes silently. cookie_disqualifies is computed once and used for both the lookup and the store, so the two cannot drift apart. Two doc comments corrected. One claimed a helper had three call sites when it has two, and missed that the third is a deliberate stricter duplicate — consolidating them would loosen the gate rather than tidy it. The other described a dispatcher supplying a path from the live request, a mechanism that stopped existing when assembly moved to CompletedRequest. One existing test had to change rather than being made to pass: it used Accept-Encoding as its example of an unstated Vary, which is now structurally covered, so it would have been testing the carve-out instead of the drift guard. Verified by mutation in both directions: ignoring the flag so cookies always disqualify fails the opt-in test, and ignoring it the other way so they never disqualify fails the default test. Full gates green — fmt, six clippy targets, four adapter suites.
Four findings, all demonstrated by running code rather than inferred. Two would have made the spike measure nothing and report it as a result: Vary: Accept-Encoding disqualified every compressing origin, and cookies excluded essentially every repeat visitor. One was a correctness bug — a POST answered from a cached GET. One was a coverage gap that let a leak-class mutation pass 1889 tests. The process note is worth as much as the findings. Every confirmed finding came from running something; the clean bills came with positive evidence rather than absence of findings, including proving that the leakage test has teeth by breaking the store/assemble order and watching it fail. Two of my own doc comments were wrong and were caught by checking rather than reading. Verified afterwards against a running server with the origin advertising Vary: Accept-Encoding: a cookie-bearing repeat visitor costs one origin fetch across two requests, and a POST still reaches the origin. Docs build verified.
prk-Jr
changed the title
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
per-reader assembly — so the root can be a cache hit while ad bids stay per-reader. All of it
sits behind
creative_opportunities.assembly_mode, which defaults toinline, today's shippedbehaviour; nothing changes until an operator opts in.
</body>holdcosts ~nothing today only because the auction hides behind a slow origin fetch; once the
root caches, the origin fetch disappears and the auction becomes the entire remaining cost.
The two are coupled, so neither fix shows a win alone — demonstrated locally, where a cache hit
skips the origin and the reader still waits exactly as long.
point at a known location its parsing generality buys nothing a byte split does not. That
matters for the issue's gating decision — "is Fastly-first acceptable for the flagship perf
path?" — because the portable design gets the same win on all four adapters, removing the
portability, dependency, and operational-weight objections the issue itself raises against ESI.
Changes
core/src/creative_opportunities.rsAssemblyMode(inline/client_fill/esi) plustemplate_cache_varyandorigin_is_cookie_independent; allOption+skip_serializing_ifso a rollback to an older binary still loads configcore/src/publisher.rscore/src/platform/template_cache.rsTemplateCacheKey(length-prefixed, so two keys cannot collide),TemplateMetadata, thePlatformTemplateCachetrait, andVarySpecwith its drift guardcore/src/platform/template_assembly.rsPlatformTemplateAssembler, defaulting to a null object that refuses rather than passing the template through unassembledcore/src/platform/types.rs,mod.rsRuntimeServices, defaulted so adapters without them degrade rather than fail to buildcore/src/html_processor.rsBodyCloseInjection— decouples what the</body>seam injects from whether the<head>seam injected anythingcore/src/response_privacy.rsCache-Controlpredicate; correct a doc comment that misdescribed its call sitescore/src/integrations/gpt_diagnostics.rsactive_for_tests()and a test pinning thatrequires_private_no_store()is a superset of the injection conditionadapter-fastly/src/template_cache.rsfastly::cache::corebacking, with a transactional insert so a cold key transforms onceadapter-fastly/src/esi_assembly.rsesi0.7 assembly with every safety-relevant setting stated explicitly —is_includes_cacheabledefaults totrue, which would cache one reader's bids and serve them to the nextadapter-fastly/src/app.rs,main.rs,Cargo.tomlesiandderive_moredocs/superpowers/**Closes
Refs #1009 — deliberately not
Closes. The issue asks whether edge assembly justifies aFastly-only rendering path. Answering that needs the client-fill arm to compare against, and it
does not exist yet, so there is no comparison and no decision. This PR makes the question
answerable; it does not answer it.
Test plan
cargo test-fastly && cargo test-axum(alsotest-cloudflare,test-spin)cargo clippy-fastly && cargo clippy-axum(alsocloudflare,cloudflare-wasm,spin-native,spin-wasm)cargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run— 7 tests pass; one test file fails to load on an ESM/CJS interop error insidenode_modules(@exodus/bytesviahtml-encoding-sniffer). Pre-existing and environmental: this branch changes zero JS files.cd crates/trusted-server-js/lib && npm run formatcd docs && npm run format(andnpm run build, which catches dead links thatformatdoes not)cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1viceroy servedirectly rather thanfastly compute serve(the Fastly CLI is not installed locally; Viceroy is what it wraps). Against a stub origin: a cache hit skips the origin entirely, no unresolved marker reaches the browser, the hit carriesprivate, no-store, a cookie-bearing repeat visitor shares the template under the opt-in, aPOSTstill reaches the origin, andinlineis unaffected.GET-only check, the cookie flag in both directions, theVarydrift guard, the stale-entry check, the transform-failure guard, and the C3 privacy stamp. One test was found to pass for the wrong reason this way and was rewritten.crates/trusted-server-integration-tests) — not runChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)Notes for review
Two things worth a reviewer's attention over the rest:
Ordering is enforced by statement position, not by types. Three correctness properties depend
on it — the gate must run before TS stamps its own
private, no-store, the store must precedeassembly, and headers must be final before the first body byte. Each is covered by a test that
fails loudly if reordered, but a well-meaning refactor consolidating adjacent blocks would break
them. A newtype returned only by the store and required by assembly would make one of these a
compile error instead.
An independent review found four issues, all fixed here, and all demonstrated by running code
rather than by reading it: a
POSTanswered from a cachedGET;Vary: Accept-Encodingdisqualifying every compressing origin (which would have made the cache store nothing and the
spike report a hit rate near zero as a result); cookies excluding essentially every repeat
visitor; and
client_fillhaving no end-to-end coverage at all.