Add ESI cacheable-root design, Stage 0 plan, and Step A findings - #1013
Draft
prk-Jr wants to merge 16 commits into
Draft
Add ESI cacheable-root design, Stage 0 plan, and Step A findings#1013prk-Jr wants to merge 16 commits into
prk-Jr wants to merge 16 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.
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
origin → lol_html → shared transformed-template cache → finalize headers → per-request body assembly; Fastly provides native cache and in-process purge primitives, so no external KV/template service is structurally required.FINAL PASSis recorded.This PR changes documentation only. It does not implement or validate ESI.
closes #1009
Current status
Changes
docs/superpowers/specs/2026-08-08-esi-cacheable-root-validation-design.mdlol_html → cache → header finalization → ESIpipeline, distinguishes C1/C2/C3, and records the request-neutral template invariant.docs/superpowers/plans/2026-08-08-1009-measurement-and-stage-0.mdFINAL PASS, and the C1 rollback constraints.docs/superpowers/plans/2026-08-08-1009-measurement-findings.mdPROVISIONAL PASS, its untested conditions, the randomized-slot finding, and the still-unrun Steps B/C.docs/superpowers/plans/2026-08-10-1009-esi-validation-spike.mdVerification
cd docs && npm run formatcd docs && npm run buildgit diff --check main...HEADFollow-up
The cheapest technical falsifier remains Task 1 of the spike: add
esi = "0.7"to the Fastly adapter and runcargo check-fastly. That is follow-up implementation work, not evidence supplied by this documentation PR.