fix(matcher): do not cache responses that freshly assign a sticky variant - #1216
fix(matcher): do not cache responses that freshly assign a sticky variant#1216guitavano wants to merge 1 commit into
Conversation
…iant A sticky matcher (e.g. the `random` A/B matcher) draws a variant on the first request and persists it in a `deco_matcher_*` / `deco_segment` cookie. The previous approach mirrored that Set-Cookie into an inline `<script>` and marked the page cacheable, assuming the CDN would key the cache by the cookie. That assumption only holds for visitors who ALREADY carry the cookie: every cold visitor shares the empty-cookie cache entry, so the first cold draw gets baked into the shared response and served to everyone within the cache window. The traffic split (e.g. 50/50) collapses and the A/B test is statistically invalid. Fix: treat a framework `Set-Cookie` on the response as a fresh, non-deterministic assignment and force `no-store`. Only responses whose variant was read back from an existing request cookie (no Set-Cookie emitted) are cacheable, and their `Deco-Cache-Vary-Cookies` hint is now derived from the request cookies the visitor already carries. Net effect: - Clean pages / no matchers: cache first visit as before (unchanged). - Returning, already-bucketed visitors: served from cache, varied by their variant cookie. - The single request that performs the random draw: no-store, so each cold visitor gets an independent draw and the split is preserved. This makes the client-cookie mirror mechanism obsolete, so runtime/clientCookies.ts and its tests are removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughFramework cookie cache decisions now derive variation from request cookies, mark fresh framework cookie assignments as ChangesFramework cookie cache behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant middleware
participant applyPageCacheDecision
participant Response
Request->>middleware: incoming framework cookies
middleware->>applyPageCacheDecision: requestFrameworkCookies
applyPageCacheDecision->>Response: cache headers
middleware->>Response: original response body
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/middleware.ts">
<violation number="1" location="runtime/middleware.ts:194">
P2: A response that freshly sets a framework cookie can still retain an existing public `Cache-Control` header when `isPageCacheAllowed` is false, because this check is reached only after the opt-in early return. Applying framework `Set-Cookie` disqualification before that return would preserve the no-cache-on-cookie-birth invariant for responses with pre-existing cache headers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // variant within the cache window). Only responses whose variant was read | ||
| // back from an existing request cookie (no framework Set-Cookie emitted) are | ||
| // safe to share from cache. | ||
| if (frameworkSetCookieNames(headers).length > 0) { |
There was a problem hiding this comment.
P2: A response that freshly sets a framework cookie can still retain an existing public Cache-Control header when isPageCacheAllowed is false, because this check is reached only after the opt-in early return. Applying framework Set-Cookie disqualification before that return would preserve the no-cache-on-cookie-birth invariant for responses with pre-existing cache headers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/middleware.ts, line 194:
<comment>A response that freshly sets a framework cookie can still retain an existing public `Cache-Control` header when `isPageCacheAllowed` is false, because this check is reached only after the opt-in early return. Applying framework `Set-Cookie` disqualification before that return would preserve the no-cache-on-cookie-birth invariant for responses with pre-existing cache headers.</comment>
<file context>
@@ -182,12 +183,27 @@ export const applyPageCacheDecision = (
+ // variant within the cache window). Only responses whose variant was read
+ // back from an existing request cookie (no framework Set-Cookie emitted) are
+ // safe to share from cache.
+ if (frameworkSetCookieNames(headers).length > 0) {
+ headers.set("Cache-Control", NO_STORE);
+ return;
</file context>
Problem
The
randomA/B matcher (website/matchers/random.tsinapps) declarescacheable = trueand is sticky-on-session: it draws a variant once (Math.random() < traffic) and persists it in adeco_matcher_*cookie.Recent work (#1203, #1204, #1205) made such pages cacheable by mirroring the framework
Set-Cookieinto an inline<script>document.cookie=…</script>and marking the response cacheable — assuming the CDN would key the cache by that cookie so each variant gets a distinct entry.That assumption only holds for visitors who already carry the cookie. A cold visitor has no cookie, so all cold visitors share the single empty-cookie cache entry. The first cold visitor's coin flip gets baked into that shared HTML (variant + the cookie-setting script) and is served to everyone within the cache window. The result:
This isn't a privacy/correctness bug (no user data leaks); it's a soundness bug for any per-user matcher. A random draw and a shared cached response are fundamentally incompatible.
Fix
Treat a framework
Set-Cookieon the response as a fresh, non-deterministic assignment and forceno-store. Only responses whose variant was read back from an existing request cookie (noSet-Cookieemitted) are cacheable, and theirDeco-Cache-Vary-Cookieshint is now derived from the request cookies the visitor already carries.The invariant: never cache the exact request where a variant cookie is being born.
Net effect
Cold visitors now take an origin miss (unavoidable to keep the draw independent without edge-side randomization), but returning traffic — the bulk of session pageviews — still hits cache.
The client-cookie mirror mechanism becomes obsolete (a cacheable response never carries a framework
Set-Cookieanymore), soruntime/clientCookies.tsand its tests are removed. Cold-visitSet-Cookieheaders now flow to the client normally (the response isno-store, so no CDN strips them) — this also restores stickiness for no-JS clients.Note:
apps/website/matchers/random.tsneeds no change —cacheable = trueis now safe because the framework refuses to cache the assignment request. Its comment is slightly outdated and can be updated separately.Tests
runtime/middleware.test.tsupdated: fresh-assignment → no-store, returning-visitor → public + vary hint from request cookies, plus the existing safety cases. All 10 pass.🤖 Generated with Claude Code
Summary by cubic
Prevents caching of responses that assign a fresh sticky variant so A/B splits stay correct. Only returning visitors are cached (varied by their existing cookies), and the client-cookie mirror code is removed.
Bug Fixes
Refactors
Written for commit 784f8fa. Summary will update on new commits.
Summary by CodeRabbit