Add shared root caching with hybrid ESI and streaming assembly - #1013
Open
prk-Jr wants to merge 68 commits into
Open
Add shared root caching with hybrid ESI and streaming assembly#1013prk-Jr wants to merge 68 commits into
prk-Jr wants to merge 68 commits into
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.
The previous fix worked and cost far too much. Forcing Accept-Encoding: identity on the origin request made the origin send ~674KB where it would have sent ~100KB, which showed up on a real deployment as 2.82s waiting for the server. I called it an accepted spike cost and moved on; it is not acceptable, and it was the wrong layer. Only the assembled response needs to be text. The fetch should stay compressed. So the origin request is left alone, and the transform's output is decoded once, at the point where the shared-template path starts treating bytes as text — finding the seam marker, splitting on it, inserting a script. None of that works on compressed bytes: the marker is not there to find, from_utf8 fails, and a spliced gzip stream is undecodable in the browser. Only shared modes pay the decode; Inline's bytes go out exactly as the encoder produced them. Two places had to agree. The response stops claiming an encoding it no longer carries, and the stored metadata records identity rather than the origin's encoding — otherwise a cache hit would declare Content-Encoding: gzip over plaintext, which is the same undecodable response one layer along. The harness gained an assertion that the origin fetch stays compressed, since that is precisely the regression being undone. Getting it to pass exposed three harness requests that were not sending Accept-Encoding at all — the headers-only check and the POST — so they were silently exercising the plaintext path. Also added: assembly is now proven byte-faithful over React Suspense markers (<!--$-->, <!--/$-->, <!--$?-->, <!--$!-->) and escaped inline scripts. Hydration depends on those comments surviving, the ESI parser runs over the publisher's whole document, and nothing checked it — every previous test used a five-line fixture. Verified by mutation: skipping the decode reproduces the 502 and the missing seam marker, failing five harness assertions. Full gates green, both harness modes pass.
A review found the truncation diagnosis correct but "one change closes it" wrong. Six findings, all confirmed by reading the code rather than taken on trust. The worst was silent. Shared modes suppress the head slot script, and the seam carried only bids — so tsjs.adSlots stayed at its [] default, adInit defined nothing, and the page rendered perfectly while serving zero TS ads. My own comment supplied the alibi, claiming "the template already carries the slot markup", which conflated the publisher's div with TS's slot configuration. The harness asserted window.tsjs was present, not that adSlots was populated, so it passed green through exactly this. It now checks both, and refuses an empty array. Slots therefore travel on the seam with the bids, which is what the comment already claimed. That is also the only place they can go: they are request-gated, so baking them into a template shared between readers would decide for all of them. The remaining five: - Assembly no longer uses the esi crate. It loses content inside any element larger than its 16KB chunk_size, which is every Next.js RSC payload script. The byte split the hit path already used handles the real 1.4MB page intact. - Assembly is gated on the authorization, not the configured mode, and the authorization is read before the store consumes it. A bypassed response carries no marker, so the earlier draft turned an ordinary bypass — the common case — into a 500. - No slots means no seam at all rather than an empty one. Emitting an empty-slot seam still calls scheduleInitialAdInit, scheduling adInit for bots, prefetches and consent-denied requests: exactly the traffic that opted out. - Vary: * is refused. VarySpec::uncovered_by filters the wildcard out with a comment saying the eligibility gate handles it; nothing did, so a response the origin said no key can select was shareable. - Every assembled response is private, not only those where should_run_ad_stack is true. A suppressed request can still assemble an empty-bids document and would have kept the origin's public caching directives for a downstream cache to serve on. - Origin policy headers survive a hit. Reconstructing headers keeps Set-Cookie and caching directives out of a shared cache and also silently dropped Content-Security-Policy and framing protection. An allowlist stores the per-URL policy headers with the template, so anything per-reader or cache-controlling is excluded by construction. Marker validation is strict: missing or repeated is an error, since splicing the first of several would leave the rest in the page as visible text. The oversized-script test is inverted rather than deleted. It asserts the defect, because that defect is the reason the render path no longer uses the crate; if it starts failing, the crate has been fixed and the decision deserves revisiting. Full gates green, both harness modes pass (esi 11, inline 5). Known defect, deliberately not fixed here: current_bid_map recovers bids by un-escaping the rendered script and only reverses the two angle-bracket escapes, so a bid containing any other escaped character is lost. Every fixture has empty bids, so nothing catches it. The fix is to carry the bid map from write_bids_to_state rather than reconstruct it, which is a change in the auction collection path.
Two independent reviews found eight defects on top of a41d3e6. Every one had shipped past a fully green suite, because a fixture never reached the branch under test. The worst: ESI mode served zero bids. current_bid_map recovered them by un-escaping the rendered script, but html_escape_for_script escapes quotes, so any non-empty map was invalid JSON, from_str failed, and unwrap_or_default swallowed it into {}. Only the empty map survived — which is every fixture there was. AdBidsState now holds the rendered script and the structured map, both derived from one map in one call, so they cannot disagree. The rest: - Assembly is a byte split, never the esi crate. The crate loses content inside any element larger than its 16KB chunk_size, and Next.js streams its RSC payload as a few enormous scripts — a real 1.4MB page came back at 697KB. - Both hit finalizers emitted an empty-slot seam when the ad stack had not run, which still calls scheduleInitialAdInit and schedules adInit for bots, prefetches and consent-denied readers. Absent is not empty; all three call sites now share one helper. - The seam no longer assigns adSlots ahead of the navigation-generation guard, where a committed SPA route could clobber it. - The marker is an inert HTML comment, validated as exactly-one before a template is stored and before any response header commits, with TEMPLATE_SCHEMA_VERSION bumped so entries holding the old marker are never read back. - Cache-Control is read across all header lines, so a private on a second line disqualifies. The cache key's fingerprint folds in the integrations config, so reconfiguring one no longer reuses a template built under the old config. - client_fill's cache had never hit: the seam check was unconditional and client_fill emits no marker, so every hit failed as Missing and refetched. The requirement is now mode-aware via an exhaustive match, so a new mode must state its answer. Coverage was the actual failure. Two gaps are closed because reverting the fix left the suite green: the fingerprint was tested as a function but never through the call site into a real cache key, and client_fill's test used a fresh cache per reader with one request each — proving the stored bytes neutral, never that a second request is served from cache. Both now assert on what the cache actually did. The harness was lying too. It grepped gzipped bytes, so every content assertion passed for free; it counted log lines Viceroy emits twice; and it had no client_fill mode at all. It now gunzips before asserting, compares distinct values, runs all three modes, and checks a real winning bid reaches the page rather than accepting an empty auction. Verified by mutation throughout: each fix reverted, the test watched to fail, the fix restored. Full gates green — fmt, six clippy targets, four adapter suites, parity, 569 JS tests. Harness: inline 6/6, client_fill 12/12, esi 14/14. Two caveats recorded rather than fixed. client_fill now caches correctly but its client-side bid fetch lives only in the SPA navigation hook, so it likely still renders no ads on initial load. And the JS bundle is not content-hashed and is served max-age=300, so for a few minutes after a deploy an old one-argument scheduler can meet a new two-argument seam and silently drop slots; the schema version protects the document side, nothing versions the client side. assembly_mode still defaults to inline, and inline behaviour is unchanged.
prk-Jr
requested review from
ChristianPavilonis and
aram356
and removed request for
aram356
August 12, 2026 14:28
prk-Jr
marked this pull request as ready for review
August 12, 2026 14:28
7 tasks
prk-Jr
changed the title
prk-Jr
added a commit
that referenced
this pull request
Aug 14, 2026
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
2 participants
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
creative_opportunities.assembly_mode, which continues to default toinline.slots, bids, consent-gated behavior, and auction state per request.
stackpop/esiparser on an authorized cold fill. Warm C2 hits retainthe lower-latency streaming byte seam, so the article prefix reaches the reader while
the auction finishes near
</body>.Final architecture
x-ts-c2-cache: bypass-*miss-stored+esi-parserhit+byte-seambyte-seam-fallbackC2 stores one inert
<!--ts-ad-seam-->comment. Executable ESI exists only in a privateworking copy: the Fastly adapter converts that comment to one synthetic include and
resolves it from the already-collected reader state with
PendingFragmentContent::CompletedRequest. There is no browser-visible fragment request,self-referencing backend, or final assembled-response cache.
The parser is pinned to
stackpop/esi@4c53feab,which preserves incomplete elements across read chunks. Warm hits deliberately do not run
the parser because its current adapter API buffers the complete document and would lose
the fast first byte.
Safety contract
GET/200HTML representations withexplicit remaining shared freshness.
Authorization,Set-Cookie, private/no-store responses,Vary: *, uncovered Varyfields, conditional/range requests, diagnostics-private requests, and malformed cache
metadata fail open to the origin path.
origin_is_cookie_independent = trueis an explicit operator assertion; configuredorigin Vary fields are included in the length-prefixed cache key.
<esi:directives never enter C2 or the parser.Truncation or any unrelated mutation triggers the complete byte-seam fallback.
ad stack is gated off emit no seam script and do not schedule GPT initialization.
private, no-store; replayed policy headers areallowlisted and repeated values are preserved.
Configuration and rollout
template_cache_max_age_secondscan cap the origin's remaining shared freshness. Rollbackis a configuration change back to
assembly_mode = "inline"; the default remains inline,so merging this PR does not enable C2 for any publisher.
Verification
cargo fmt --all -- --checkcargo test-fastlycargo test-axumcargo test-cloudflarecargo test-spinpermutations, include resolution after a large script, and truncated EOF behavior
Live verification through the local Fastly stack against the real Autoblog origin produced
the intended sequence:
no-cacherequest:x-ts-c2-cache: bypass-requestmiss-stored+esi-parserhit+byte-seam, 9.23 ms curl TTFBseam marker or synthetic ESI include in the served document
Review focus
The highest-value review areas are the authorization/store/assembly ordering, the
reader-neutral cache-key assumptions, the terminal privacy stamp, and the deliberately
restricted ESI dispatcher/output validation.
Closes #1009