amadan

carlos / platform Public

edge: per-entry Vary storage — mixed Vary schemes coexist per URL (#20)

* edge: the Vary names belong to the entry, not to the URL

One Vary scheme was recorded per URL, so an app that emits Vary
conditionally — CORS middleware adding Vary: Origin only for
Origin-bearing requests, content negotiation adding Vary: Accept-Language
only when it negotiates — rewrote that record on every flip and swept
every variant stored under the previous scheme.

The thrash was the smaller half. With the record cleared by an
intervening non-varying response, a language-negotiating request resolved
to the EMPTY variant and was served the no-Vary body: the negotiation the
app performed silently disappeared, for everyone, until the next flip.

Store the Vary field value with each entry, as part of its key, and let
each candidate evaluate its own names against the incoming request
(RFC 9111 §4.1). Mixed schemes now coexist under one URL. Vary is in the
key rather than merely on the entry because varyValue renders "Name=value;"
pairs, so `A: b;C=d` under Vary: A renders what `A: b` + `C: d` renders
under Vary: A, C — with Vary out of the key that collision would let one
response overwrite the other's body while the first's names stayed on the
entry.

Among several matching variants the most specific wins, then the most
recently stored. RFC 9111 §4's freshness rule assumes one scheme per URI;
under mixed schemes it would hand the non-varying entry to everybody the
moment it was newest, which is a CORS response cached without its
Access-Control-Allow-Origin and served to the requests that needed it.

The per-URL cap now bounds a URL across every scheme it holds — one
allowance, not one per scheme — and is also what keeps the lookup scan
short. GetFor takes the lock once instead of twice.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* edge cache: review fix round — canonical Vary, injective VaryValue, deterministic selection

Seven review findings on the per-entry-Vary change. Five needed code; two are
answered in the doc comments and pinned by tests.

- Stale shadowing (most-specific-first can serve an older specific variant
  while a newer general one sits beside it) is KEPT: it is bounded by the
  shadowing entry's own TTL, whereas "newest wins" is unbounded wrongness — the
  non-varying entry served to requests whose response depended on their
  headers, for as long as it stays newest. selectLocked's doc comment now names
  the cost and the argument; the behaviour is pinned both ways (shadowed while
  live, general entry served once the specific one's TTL runs out).

- varyValue is now injective: each value is length-prefixed, "Name=<len>:<v>;".
  The separators were forgeable from inside a header value — under Vary: A, B a
  request sending A: "u;B=v", B: "w" rendered byte-identically to A: "u",
  B: "v;B=w", so two different requests shared one slot in a cache shared by
  every visitor of the host. Store and lookup still render through this one
  function. cacheFullKey's doc now states the guarantee precisely: canonical
  name list in Vary, injective-over-values VaryValue for a given Vary.

- Vary is canonicalized before it becomes a key component or drives varyValue:
  split across lines, trimmed, empty tokens dropped, case-canonicalized,
  deduped, sorted. "accept-encoding" and "Accept-Encoding" are one slot; so are
  "A, B" and "B, A". Specificity is the count of canonical names, so a Vary of
  only separators is specificity 0 and takes the no-Vary path exactly rather
  than out-ranking real variants with four empty names. Canonicalization lives
  inside canonicalVaryNames, which both Put and varyValue call, so the store
  and lookup paths cannot canonicalize differently. The Vary: * / Vary: Cookie
  refusals still read the RAW field value and still compare case-insensitively
  across every line — unchanged, and now pinned against lower/upper/mixed
  spellings too.

- Cache.Get is unexported (get). No caller outside the package existed; one
  activator test moved to the exported GetFor, which is the request-time API.

- better breaks a specificity+StoredAt tie by key rather than by Go's
  randomized map iteration order, so one request cannot resolve two ways.

Two existing tests updated, both because they hard-coded a VaryValue in the old
un-prefixed rendering and re-derived it at lookup time
(TestCacheMixedVarySchemesCoexistForOneURL,
TestCacheCollidingVaryValuesUnderDifferentSchemesStaySeparate). Values only —
neither assertion changed. Every other test passes unmodified.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 6846d9af4f69a7c08a4fbb1b12838b472c7fef88 parent 3c7c7bc
6 files changed, +717 −143
  • internal/activator/activator_test.go +9 −2
  • internal/edge/cache.go +185 −84
  • internal/edge/cache_test.go +346 −32
  • internal/edge/edge.go +81 −19
  • internal/edge/edge_test.go +94 −4
  • internal/edge/purge_test.go +2 −2
diff --git a/internal/activator/activator_test.go b/internal/activator/activator_test.go
index 164c9d137..92756add4 100644
--- a/internal/activator/activator_test.go
+++ b/internal/activator/activator_test.go
@@ -2250,10 +2250,17 @@ func TestExecBackendStartPurgesHostCacheOnSpawn(t *testing.T) {
defer proc.Stop(context.Background())
}
- if _, ok := cache.Get(stale); ok {
+ // GetFor is the cache's request-time lookup (the by-VaryValue one is
+ // package-internal); both entries were stored from responses with no Vary,
+ // so any request for the URL reaches them.
+ cached := func(k edge.CacheKey) bool {
+ _, ok := cache.GetFor(k, httptest.NewRequest("GET", "http://"+k.Host+k.Path, nil))
+ return ok
+ }
+ if cached(stale) {
t.Fatal("spawning a fresh process for a host must drop that host's cached responses")
}
- if _, ok := cache.Get(otherHost); !ok {
+ if !cached(otherHost) {
t.Fatal("another host's cached responses must survive a spawn")
}
}
diff --git a/internal/edge/cache.go b/internal/edge/cache.go
index ed4f07c47..0769b7ede 100644
--- a/internal/edge/cache.go
+++ b/internal/edge/cache.go
@@ -17,7 +17,10 @@ import (
//
// Two caps, because the two floods land on different key components:
// - maxVaryVariantsPerURL bounds the Vary variants of one exact URL
-// (host+method+path+query).
+// (host+method+path+query) — every variant of it, across every Vary scheme
+// its responses have used, so a URL that varies conditionally gets one
+// allowance rather than one per scheme. It is also what keeps the lookup
+// scan in selectLocked short: a handful of candidates, never a search.
// - maxEntriesPerPath bounds everything under one path regardless of query
// string or variant, which is what actually bounds a query-string flood.
//
@@ -60,23 +63,6 @@ type Cache struct {
entries map[cacheFullKey]*list.Element
byURL map[cacheURLKey]map[cacheFullKey]*list.Element // urlKey -> its Vary variants
byPath map[cachePathKey]map[cacheFullKey]*list.Element // pathKey -> every entry under that path
- // varyNames records, per URL, the raw Vary header value of the most recent
- // response stored for that URL. It is what makes the two-phase lookup
- // possible: at request time the response's Vary header isn't known yet, so
- // GetFor replays the last one seen to compute the same VaryValue Put
- // stored the entry under. Dropped when a URL's last entry goes away, so it
- // can't grow without bound.
- //
- // "Most recent", emphatically, not "first ever". A URL whose responses
- // vary conditionally (CORS middleware emitting Vary: Origin only for
- // Origin-bearing requests) would otherwise be poisoned by a single
- // request: the remembered names outlive the response that set them, later
- // non-varying responses are stored under the empty variant, and every
- // lookup keeps resolving against names nothing is stored under — a
- // permanent miss on that URL, which on a hibernating route is a permanent
- // wake per request. Put rewrites this record on every store, so a URL that
- // stops varying stops being resolved against stale names.
- varyNames map[cacheURLKey]string
}
type cacheURLKey struct {
@@ -87,8 +73,28 @@ type cachePathKey struct {
Host, Method, Path string
}
+// cacheFullKey identifies one stored variant. It carries BOTH halves of the
+// Vary contract — which headers the response was selected on and the values
+// those names resolved to for the request that elicited it — because a variant
+// is only reusable for a request that agrees on both. RFC 9111 §4.1.
+//
+// What the two components guarantee, precisely:
+//
+// - Vary holds the CANONICAL name list (canonicalVary: trimmed, deduped,
+// case-canonicalized, sorted), so two responses selecting on the same
+// headers share one slot however they spelled or ordered them, and two
+// responses selecting on DIFFERENT headers never share one.
+// - VaryValue is injective over request-header values FOR A GIVEN Vary: the
+// length prefix varyValue writes ahead of each value means a distinct tuple
+// of values always renders a distinct string, since no value's content can
+// be read as the separator structure around it (see varyValue).
+//
+// Injectivity is stated per-Vary because that is all the key needs: Vary is a
+// key component in its own right, so agreement on the rendered values is only
+// ever consulted between entries that already agree on the name list.
type cacheFullKey struct {
cacheURLKey
+ Vary string // canonicalVary of the response's Vary field value; "" if it carried none
VaryValue string
}
@@ -97,6 +103,30 @@ type cacheEntry struct {
resp *CachedResponse
}
+// matches reports whether this stored variant is reusable for r: every header
+// named by the response's own Vary field resolves, for r, to the same values
+// the stored response was selected under.
+//
+// An entry stored from a response with no Vary matches every request for the
+// URL — that is precisely what "no Vary" means. The empty-Vary branch is
+// explicit rather than left to varyValue (which renders "" for an empty name
+// list, and so would agree) so that an entry claiming no Vary can never match
+// on a non-empty VaryValue it should not be holding.
+func (e *cacheEntry) matches(r *http.Request) bool {
+ if e.key.Vary == "" {
+ return e.key.VaryValue == ""
+ }
+ return varyValue(r, e.key.Vary) == e.key.VaryValue
+}
+
+// specificity counts the headers a variant was selected on: the canonical name
+// list, so duplicate, empty and differently-spelled tokens cannot inflate it
+// past the number of headers actually consulted. It orders candidates when
+// several match one request — see selectLocked.
+func (e *cacheEntry) specificity() int {
+ return len(canonicalVaryNames(e.key.Vary))
+}
+
// NewCache creates a new cache with the given capacity (max entries) and max body size.
func NewCache(maxEntries, maxBodyBytes int) *Cache {
return &Cache{
@@ -106,7 +136,6 @@ func NewCache(maxEntries, maxBodyBytes int) *Cache {
entries: make(map[cacheFullKey]*list.Element),
byURL: make(map[cacheURLKey]map[cacheFullKey]*list.Element),
byPath: make(map[cachePathKey]map[cacheFullKey]*list.Element),
- varyNames: make(map[cacheURLKey]string),
}
}
@@ -114,60 +143,148 @@ func (k CacheKey) urlKey() cacheURLKey {
return cacheURLKey{Host: k.Host, Method: k.Method, Path: k.Path, RawQuery: k.RawQuery}
}
-func (k CacheKey) fullKey() cacheFullKey {
- return cacheFullKey{cacheURLKey: k.urlKey(), VaryValue: k.VaryValue}
+func (k CacheKey) fullKey(vary string) cacheFullKey {
+ return cacheFullKey{cacheURLKey: k.urlKey(), Vary: vary, VaryValue: k.VaryValue}
}
func (k cacheURLKey) pathKey() cachePathKey {
return cachePathKey{Host: k.Host, Method: k.Method, Path: k.Path}
}
-// Get retrieves a cached response if it exists and has not expired.
-// Returns (nil, false) for a miss or an expired entry.
-// Expired entries are evicted lazily on lookup, not swept on a timer.
+// get retrieves a cached variant by an already-resolved VaryValue, if one
+// exists and has not expired. Returns (nil, false) for a miss or an expired
+// entry. Expired entries are evicted lazily on lookup, not swept on a timer.
//
-// Get takes the VaryValue already resolved by the caller. Request handling
-// goes through GetFor instead, which resolves it first — a request can't know
-// the response's Vary header until the response exists.
-func (c *Cache) Get(key CacheKey) (*CachedResponse, bool) {
+// Deliberately unexported: every request-handling caller must go through
+// GetFor, which matches on the stored Vary names rather than a VaryValue the
+// caller had to guess — a request cannot know the response's Vary header until
+// the response exists, so an exported by-VaryValue lookup is an invitation to
+// guess "" and silently miss (or, worse, hit the wrong variant). get exists for
+// the package's own probes and tests, which already hold the exact variant
+// string; when several Vary schemes happen to render the same VaryValue it
+// picks between them by selectLocked's ordering.
+func (c *Cache) get(key CacheKey) (*CachedResponse, bool) {
c.mu.Lock()
defer c.mu.Unlock()
- fk := key.fullKey()
- el, ok := c.entries[fk]
- if !ok {
- return nil, false
+ return c.selectLocked(key.urlKey(), func(e *cacheEntry) bool {
+ return e.key.VaryValue == key.VaryValue
+ })
+}
+
+// GetFor is the request-time lookup. baseKey carries everything known before
+// the response exists (host, method, path, query) with VaryValue left empty;
+// which variant r belongs to is decided by the Vary names each stored entry
+// recorded for itself.
+//
+// The Vary header names live on the RESPONSE, so they cannot be known from r
+// alone — which is why they travel with the entry (RFC 9111 §4.1). Every
+// candidate for this URL evaluates its OWN names against r, so responses that
+// vary by different headers, or that vary only sometimes, coexist under one
+// URL instead of overwriting each other's notion of how the URL varies.
+//
+// The scan is bounded by maxVaryVariantsPerURL, so it is a handful of string
+// comparisons, not a search. Both this and maybeStore render the variant
+// through varyValue, so store and lookup cannot drift apart — that drift
+// (storing under varyValue(request, vary) while looking up under "") is the
+// bug that once made every Vary-carrying response a permanent miss.
+func (c *Cache) GetFor(baseKey CacheKey, r *http.Request) (*CachedResponse, bool) {
+ c.mu.Lock()
+ defer c.mu.Unlock()
+ return c.selectLocked(baseKey.urlKey(), func(e *cacheEntry) bool {
+ return e.matches(r)
+ })
+}
+
+// selectLocked returns the best live entry for uk that satisfies want, evicting
+// any expired entry it walks past. Caller must hold c.mu.
+//
+// "Best", because several stored variants can satisfy one request: an entry
+// from a response with no Vary matches every request for the URL, so it
+// overlaps every varying entry beside it. RFC 9111 §4 leaves the choice among
+// suitable responses to freshness order; that rule assumes one Vary scheme per
+// URI, and under mixed schemes it would hand the non-varying entry to
+// everybody the moment it was the newest — a CORS response cached without its
+// Access-Control-Allow-Origin, served to the cross-origin requests that needed
+// it. So the ordering is most-specific-first: among matches, the entry
+// selected on the most headers wins, because it is the one whose stored
+// response actually depended on this request's headers. Equal specificity
+// falls back to the most recently stored, which is RFC 9111 §4's rule.
+//
+// The accepted cost of that ordering is stale shadowing: a more-specific
+// variant outranks a strictly NEWER general one that also matches, so an older
+// `Vary: Accept-Encoding` entry keeps being served to gzip-accepting requests
+// even after a fresher no-Vary response for the same URL has been stored beside
+// it. Pinned by TestCacheOlderSpecificVariantShadowsNewerGeneralUntilItExpires.
+//
+// This is knowingly kept, and no recency heuristic is layered on top:
+//
+// - It is bounded by the shadowing entry's own TTL. maybeStore clamps every
+// TTL to DefaultCacheMaxTTL, so the window is short and self-healing — the
+// stale entry is dropped by the expiry sweep in this very loop, after which
+// the general entry is the only match left. Bounded staleness is the whole
+// bargain of a cache, and PurgeURL/PurgeHost cut it short on deploy.
+// - The alternative is unbounded WRONGNESS. "Newest wins" hands the
+// non-varying entry to every request the moment it is newest, including the
+// cross-origin ones whose stored response carried an
+// Access-Control-Allow-Origin the new one does not — a correctness failure
+// no TTL bounds, because the newest entry keeps being the newest.
+//
+// Serving a slightly older response that was selected on this request's actual
+// headers is the safe side of that trade.
+func (c *Cache) selectLocked(uk cacheURLKey, want func(*cacheEntry) bool) (*CachedResponse, bool) {
+ var best *list.Element
+ var bestEntry *cacheEntry
+ var expired []*list.Element
+ for _, el := range c.byURL[uk] {
+ e := el.Value.(*cacheEntry)
+ if time.Since(e.resp.StoredAt) > e.resp.TTL {
+ expired = append(expired, el)
+ continue
+ }
+ if !want(e) {
+ continue
+ }
+ if bestEntry == nil || better(e, bestEntry) {
+ best, bestEntry = el, e
+ }
}
- e := el.Value.(*cacheEntry)
- if time.Since(e.resp.StoredAt) > e.resp.TTL {
+ // Removal is deferred out of the loop: removeLocked deletes from the very
+ // map being ranged over, and can drop c.byURL[uk] entirely once its last
+ // entry goes.
+ for _, el := range expired {
c.removeLocked(el)
+ }
+ if best == nil {
return nil, false
}
- c.order.MoveToFront(el)
- return e.resp, true
+ c.order.MoveToFront(best)
+ return bestEntry.resp, true
}
-// GetFor is the request-time lookup: phase one resolves which Vary variant r
-// belongs to, phase two does the ordinary Get. baseKey carries everything
-// known before the response exists (host, method, path, query) with VaryValue
-// left empty.
+// better reports whether candidate outranks incumbent as the variant to serve:
+// more selecting headers first, then more recently stored, and finally by key
+// so the answer never depends on map iteration order.
//
-// The Vary header names live on the RESPONSE, so at lookup time the only
-// honest source for them is what the last response stored for this URL said
-// (c.varyNames). With none on record — no response yet, or the last one
-// carried no Vary — the lookup uses an empty VaryValue,
-// which is exactly the no-Vary case and keeps that path byte-for-byte as it
-// was. Both this and maybeStore compute the variant through varyValue, so
-// store and lookup can never drift apart — the bug this method exists to fix
-// was precisely that drift: storing under varyValue(request, vary) while
-// looking up under "", so a Vary-carrying response never hit.
-func (c *Cache) GetFor(baseKey CacheKey, r *http.Request) (*CachedResponse, bool) {
- c.mu.Lock()
- names := c.varyNames[baseKey.urlKey()]
- c.mu.Unlock()
- if names != "" {
- baseKey.VaryValue = varyValue(r, names)
+// That last step is not cosmetic. selectLocked ranges over a Go map, whose
+// order is randomized per iteration, so two entries tied on both specificity
+// and StoredAt — same-millisecond stores, or timestamps a test or a restore
+// path set by hand — would otherwise serve a different body from one request to
+// the next for no visible reason. A cache that answers the same question two
+// ways is unreproducible in exactly the situation where it matters. Comparing
+// the key components (which are unique per entry: two entries agreeing on both
+// are the same map key) makes selection a pure function of what is stored.
+func better(candidate, incumbent *cacheEntry) bool {
+ cs, is := candidate.specificity(), incumbent.specificity()
+ if cs != is {
+ return cs > is
+ }
+ if !candidate.resp.StoredAt.Equal(incumbent.resp.StoredAt) {
+ return candidate.resp.StoredAt.After(incumbent.resp.StoredAt)
}
- return c.Get(baseKey)
+ if candidate.key.VaryValue != incumbent.key.VaryValue {
+ return candidate.key.VaryValue < incumbent.key.VaryValue
+ }
+ return candidate.key.Vary < incumbent.key.Vary
}
// Put stores a response in the cache. Silently drops a response whose body
@@ -183,37 +300,25 @@ func (c *Cache) GetFor(baseKey CacheKey, r *http.Request) (*CachedResponse, bool
// "no Vary": a response may vary on a header the request never sent, which
// computes an empty value from a very real Vary header.
//
-// Put records vary as this URL's Vary scheme so GetFor can resolve the same
-// variant on the next request, and — this is the load-bearing half — clears
-// or replaces that record when vary changes, including changing to "".
+// vary is stored WITH the entry, as part of its key, in the canonical form
+// canonicalVary produces — the same normalization varyValue applied when it
+// rendered key.VaryValue, so the two halves of the key agree by construction
+// and a scheme spelled "accept-encoding" cannot open a second slot beside the
+// one "Accept-Encoding" already holds. A URL whose responses vary
+// conditionally — CORS middleware emitting Vary: Origin only for
+// Origin-bearing requests, content negotiation emitting Vary: Accept-Language
+// only when it negotiates — therefore accumulates one variant per DISTINCT
+// scheme rather than having each flip invalidate the last, and each request is
+// matched against the scheme its own shape produced. The per-URL cap still
+// bounds the total, now across schemes rather than within one.
func (c *Cache) Put(key CacheKey, vary string, resp *CachedResponse) {
if len(resp.Body) > c.maxBodyBytes {
return
}
c.mu.Lock()
defer c.mu.Unlock()
- fk := key.fullKey()
+ fk := key.fullKey(canonicalVary(vary))
uk := fk.cacheURLKey
- if vary != c.varyNames[uk] {
- // This URL's Vary scheme changed: it stopped varying, started, or
- // started varying by different headers. Every entry already stored for
- // it was keyed under the OLD scheme, while GetFor will now resolve
- // against the new one — none of them can ever be reached again, so
- // they are pure garbage holding LRU and per-URL-cap space. Dropping
- // them is what keeps a conditionally-varying URL from slowly filling
- // the cache with unreachable variants.
- //
- // The sweep runs before the record is rewritten because removeLocked
- // deletes varyNames[uk] once the URL's last entry goes.
- for _, el := range elementsOf(c.byURL[uk]) {
- c.removeLocked(el)
- }
- if vary == "" {
- delete(c.varyNames, uk)
- } else {
- c.varyNames[uk] = vary
- }
- }
if el, ok := c.entries[fk]; ok {
el.Value.(*cacheEntry).resp = resp
c.order.MoveToFront(el)
@@ -275,10 +380,6 @@ func (c *Cache) removeLocked(el *list.Element) {
delete(variants, e.key)
if len(variants) == 0 {
delete(c.byURL, uk)
- // No entries left for this URL, so the remembered Vary names have
- // nothing to resolve against — dropping them keeps varyNames
- // bounded by the number of live URLs.
- delete(c.varyNames, uk)
}
}
pk := uk.pathKey()
diff --git a/internal/edge/cache_test.go b/internal/edge/cache_test.go
index a3b52d633..eb9eb087b 100644
--- a/internal/edge/cache_test.go
+++ b/internal/edge/cache_test.go
@@ -3,13 +3,14 @@ package edge
import (
"fmt"
"net/http"
+ "net/http/httptest"
"testing"
"time"
)
func TestCacheGetMissOnEmpty(t *testing.T) {
c := NewCache(10, 1<<20)
- if _, ok := c.Get(CacheKey{Host: "a", Method: "GET", Path: "/"}); ok {
+ if _, ok := c.get(CacheKey{Host: "a", Method: "GET", Path: "/"}); ok {
t.Fatal("expected miss on empty cache")
}
}
@@ -19,7 +20,7 @@ func TestCachePutThenGetHits(t *testing.T) {
key := CacheKey{Host: "a", Method: "GET", Path: "/event"}
resp := &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte("hi"), StoredAt: time.Now(), TTL: time.Minute}
c.Put(key, "", resp)
- got, ok := c.Get(key)
+ got, ok := c.get(key)
if !ok {
t.Fatal("expected hit after put")
}
@@ -33,7 +34,7 @@ func TestCacheEntryExpiresAfterTTL(t *testing.T) {
key := CacheKey{Host: "a", Method: "GET", Path: "/event"}
resp := &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte("hi"), StoredAt: time.Now().Add(-2 * time.Minute), TTL: time.Minute}
c.Put(key, "", resp)
- if _, ok := c.Get(key); ok {
+ if _, ok := c.get(key); ok {
t.Fatal("expected miss on expired entry")
}
}
@@ -43,7 +44,7 @@ func TestCachePutOverMaxBodyBytesIsDropped(t *testing.T) {
key := CacheKey{Host: "a", Method: "GET", Path: "/event"}
resp := &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte("way too big"), StoredAt: time.Now(), TTL: time.Minute}
c.Put(key, "", resp)
- if _, ok := c.Get(key); ok {
+ if _, ok := c.get(key); ok {
t.Fatal("expected oversized body to never be cached")
}
}
@@ -56,15 +57,15 @@ func TestCacheEvictsLeastRecentlyUsedAtCapacity(t *testing.T) {
}
put("/one")
put("/two")
- c.Get(mk("/one")) // touch /one so /two becomes the LRU victim
+ c.get(mk("/one")) // touch /one so /two becomes the LRU victim
put("/three")
- if _, ok := c.Get(mk("/two")); ok {
+ if _, ok := c.get(mk("/two")); ok {
t.Fatal("expected /two to have been evicted as least-recently-used")
}
- if _, ok := c.Get(mk("/one")); !ok {
+ if _, ok := c.get(mk("/one")); !ok {
t.Fatal("expected /one to survive (recently touched)")
}
- if _, ok := c.Get(mk("/three")); !ok {
+ if _, ok := c.get(mk("/three")); !ok {
t.Fatal("expected /three to have been inserted")
}
}
@@ -79,41 +80,354 @@ func varyOf(k CacheKey) string {
return "Accept-Language"
}
-// Put records the Vary scheme of the LATEST response for a URL, not the first
-// one ever seen. When a URL stops varying, the old per-variant entries are
-// unreachable — GetFor resolves against the new (empty) scheme — so they must
-// go rather than sit in the LRU forever, and the remembered names must go with
-// them or every later lookup resolves against names nothing is stored under.
-func TestCachePutWithoutVaryClearsRememberedNamesAndStaleVariants(t *testing.T) {
+// The Vary names travel with the entry, so one URL holds as many schemes as
+// its responses produce. A response that carries no Vary no longer invalidates
+// the variants stored from Vary-carrying ones: both stay reachable, and each
+// request is matched against the scheme its own shape produced.
+//
+// This replaces a test that pinned the opposite — the old design kept ONE Vary
+// scheme per URL, so storing a non-varying response swept every varying entry
+// beside it.
+func TestCacheMixedVarySchemesCoexistForOneURL(t *testing.T) {
c := NewCache(10, 1<<20)
uk := CacheKey{Host: "a", Method: "GET", Path: "/page"}.urlKey()
mkResp := func(b string) *CachedResponse {
return &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte(b), StoredAt: time.Now(), TTL: time.Minute}
}
- varied := CacheKey{Host: "a", Method: "GET", Path: "/page", VaryValue: "Origin=https://x;"}
+ // The VaryValue is spelled the way varyValue renders it: each value carries
+ // its byte length ahead of it ("Origin=" + len("https://x") + ":" + value).
+ varied := CacheKey{Host: "a", Method: "GET", Path: "/page", VaryValue: "Origin=9:https://x;"}
c.Put(varied, "Origin", mkResp("cors"))
+ plain := CacheKey{Host: "a", Method: "GET", Path: "/page"}
+ c.Put(plain, "", mkResp("plain"))
+
c.mu.Lock()
- names := c.varyNames[uk]
+ variants := len(c.byURL[uk])
c.mu.Unlock()
- if names != "Origin" {
- t.Fatalf("varyNames = %q after a Vary-carrying store, want %q", names, "Origin")
+ if variants != 2 {
+ t.Fatalf("URL holds %d entries, want 2 — a response with no Vary must not evict the variants stored from Vary-carrying ones", variants)
}
- plain := CacheKey{Host: "a", Method: "GET", Path: "/page"}
- c.Put(plain, "", mkResp("plain"))
+ // An Origin-bearing request resolves to the Origin variant; anything else
+ // to the non-varying one.
+ corsReq := httptest.NewRequest("GET", "http://a/page", nil)
+ corsReq.Header.Set("Origin", "https://x")
+ if got, ok := c.GetFor(plain, corsReq); !ok || string(got.Body) != "cors" {
+ t.Fatalf("GetFor(Origin: https://x) = %v, %v; want the cors body — the varying entry is the one selected on this request's headers", got, ok)
+ }
+ if got, ok := c.GetFor(plain, httptest.NewRequest("GET", "http://a/page", nil)); !ok || string(got.Body) != "plain" {
+ t.Fatalf("GetFor(no Origin) = %v, %v; want the plain body", got, ok)
+ }
+}
+
+// Two responses that named different Vary headers must never share a slot,
+// even when their rendered VaryValues collide. Length-prefixing now stops real
+// requests from colliding in the first place (see the varyValue tests below),
+// so this test forces the collision directly — both entries stored under one
+// hand-written VaryValue — to pin the structural guarantee behind it: Vary is a
+// key component, so agreement on the rendered values is only ever consulted
+// between entries that already agree on the name list. Without that, the second
+// store would overwrite the first's body while leaving the first's names on the
+// entry, and the next request matching those names would be served a body
+// computed for entirely different headers.
+func TestCacheCollidingVaryValuesUnderDifferentSchemesStaySeparate(t *testing.T) {
+ c := NewCache(10, 1<<20)
+ mkResp := func(b string) *CachedResponse {
+ return &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte(b), StoredAt: time.Now(), TTL: time.Minute}
+ }
+ // What a request sending A: "b;C=d" renders under Vary: A — 5 is the length
+ // of that value, so the lookup below reaches the entry stored under it.
+ const collide = "A=5:b;C=d;"
+ key := CacheKey{Host: "a", Method: "GET", Path: "/page", VaryValue: collide}
+ c.Put(key, "A", mkResp("selected-on-A"))
+ c.Put(key, "A, C", mkResp("selected-on-A-and-C"))
+
c.mu.Lock()
- names, remembered := c.varyNames[uk]
- variants := len(c.byURL[uk])
+ variants := len(c.byURL[key.urlKey()])
c.mu.Unlock()
- if remembered {
- t.Fatalf("varyNames still holds %q after a response with no Vary — every later lookup would resolve against names nothing is stored under", names)
+ if variants != 2 {
+ t.Fatalf("URL holds %d entries, want 2 — different Vary schemes rendering the same VaryValue are different variants", variants)
+ }
+
+ // The request that only matches Vary: A must get that response, not the
+ // one computed for a request carrying both headers.
+ req := httptest.NewRequest("GET", "http://a/page", nil)
+ req.Header.Set("A", "b;C=d")
+ if got, ok := c.GetFor(CacheKey{Host: "a", Method: "GET", Path: "/page"}, req); !ok || string(got.Body) != "selected-on-A" {
+ t.Fatalf("GetFor(A: b;C=d) = %v, %v; want selected-on-A", got, ok)
+ }
+}
+
+// A variant whose stored response has expired must not be returned, and must
+// not shadow a live variant that also matches. The scan evicts what it walks
+// past, so the expired entry is gone afterwards too.
+func TestCacheGetForSkipsAndEvictsExpiredVariants(t *testing.T) {
+ c := NewCache(10, 1<<20)
+ base := CacheKey{Host: "a", Method: "GET", Path: "/page"}
+ stale := CacheKey{Host: "a", Method: "GET", Path: "/page", VaryValue: "Accept-Language=en;"}
+ c.Put(stale, "Accept-Language", &CachedResponse{
+ StatusCode: 200, Header: http.Header{}, Body: []byte("stale-en"),
+ StoredAt: time.Now().Add(-2 * time.Minute), TTL: time.Minute,
+ })
+ c.Put(base, "", &CachedResponse{
+ StatusCode: 200, Header: http.Header{}, Body: []byte("plain"),
+ StoredAt: time.Now(), TTL: time.Minute,
+ })
+
+ req := httptest.NewRequest("GET", "http://a/page", nil)
+ req.Header.Set("Accept-Language", "en")
+ if got, ok := c.GetFor(base, req); !ok || string(got.Body) != "plain" {
+ t.Fatalf("GetFor = %v, %v; want the live non-varying entry — an expired variant must neither be served nor shadow a live match", got, ok)
}
+ c.mu.Lock()
+ variants := len(c.byURL[base.urlKey()])
+ c.mu.Unlock()
if variants != 1 {
- t.Fatalf("URL holds %d entries, want 1 — the old Origin variant is unreachable once the URL stops varying and must not linger in the LRU", variants)
+ t.Fatalf("URL holds %d entries after the lookup, want 1 — the expired variant should have been evicted on the way past", variants)
}
- if got, ok := c.Get(plain); !ok || string(got.Body) != "plain" {
- t.Fatalf("Get(no-Vary key) = %v, %v; want the freshly stored body", got, ok)
+}
+
+// The per-URL cap bounds a URL's variants across every Vary scheme it holds,
+// not per scheme: an app flip-flopping between schemes must not be able to
+// multiply its own allowance.
+func TestCacheVariantCapSpansVarySchemes(t *testing.T) {
+ c := NewCache(1000, 1<<20)
+ uk := CacheKey{Host: "a", Method: "GET", Path: "/page"}.urlKey()
+ for i := 0; i < 4*maxVaryVariantsPerURL; i++ {
+ // Alternate two schemes, each with its own fresh value every round.
+ vary := "Accept-Language"
+ if i%2 == 1 {
+ vary = "Origin"
+ }
+ k := CacheKey{Host: "a", Method: "GET", Path: "/page", VaryValue: fmt.Sprintf("%s=v%d;", vary, i)}
+ c.Put(k, vary, &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte("x"), StoredAt: time.Now(), TTL: time.Minute})
+ }
+ c.mu.Lock()
+ n := len(c.byURL[uk])
+ c.mu.Unlock()
+ if n > maxVaryVariantsPerURL {
+ t.Fatalf("one URL holds %d cached variants across two Vary schemes, want at most %d", n, maxVaryVariantsPerURL)
+ }
+}
+
+// The documented cost of most-specific-first selection, pinned so that anyone
+// who "fixes" it has to change this test on purpose: a more-specific variant
+// outranks a strictly NEWER general one that also matches, so the older
+// Vary: Accept-Encoding entry keeps being served to gzip-accepting requests
+// while a fresher no-Vary entry sits beside it — and only its own TTL ends
+// that. The trade is deliberate (selectLocked's doc comment): this staleness is
+// bounded by TTL, whereas "newest wins" would hand the non-varying entry to
+// requests whose stored response actually depended on their headers, for as
+// long as it stayed the newest.
+func TestCacheOlderSpecificVariantShadowsNewerGeneralUntilItExpires(t *testing.T) {
+ c := NewCache(10, 1<<20)
+ base := CacheKey{Host: "a", Method: "GET", Path: "/page"}
+ gz := httptest.NewRequest("GET", "http://a/page", nil)
+ gz.Header.Set("Accept-Encoding", "gzip")
+
+ // The specific entry is stored FIRST and expires soon; the general one is
+ // stored after it and stays live for a minute.
+ specific := base
+ specific.VaryValue = varyValue(gz, "Accept-Encoding")
+ c.Put(specific, "Accept-Encoding", &CachedResponse{
+ StatusCode: 200, Header: http.Header{}, Body: []byte("older-gzip"),
+ StoredAt: time.Now().Add(-10 * time.Millisecond), TTL: 100 * time.Millisecond,
+ })
+ c.Put(base, "", &CachedResponse{
+ StatusCode: 200, Header: http.Header{}, Body: []byte("newer-general"),
+ StoredAt: time.Now(), TTL: time.Minute,
+ })
+
+ if got, ok := c.GetFor(base, gz); !ok || string(got.Body) != "older-gzip" {
+ t.Fatalf("GetFor(Accept-Encoding: gzip) = %v, %v; want older-gzip — the accepted cost: the more-specific variant wins even though the general one is newer", got, ok)
+ }
+
+ // ...until the shadowing entry's own TTL runs out, which is what bounds it.
+ time.Sleep(150 * time.Millisecond)
+ if got, ok := c.GetFor(base, gz); !ok || string(got.Body) != "newer-general" {
+ t.Fatalf("GetFor after the specific variant's TTL = %v, %v; want newer-general — the shadowing is bounded by that TTL, not permanent", got, ok)
+ }
+}
+
+// Two requests that differ only in which side of a Vary pair holds a given
+// substring must not share a cache slot. Under Vary: A, B the unprefixed
+// rendering wrote "A=u;B=v;B=w;" for BOTH `A: "u;B=v", B: "w"` and
+// `A: "u", B: "v;B=w"` — the separators were forgeable from inside a header
+// value, so whichever request stored first had its body served to the other out
+// of a cache shared by every visitor of the host. Length-prefixing each value
+// makes the rendering injective for a given Vary: the reader takes exactly len
+// bytes, so no content can be read as structure.
+func TestVaryValueLengthPrefixSeparatesRequestsThatOnceCollided(t *testing.T) {
+ const vary = "A, B"
+ first := httptest.NewRequest("GET", "http://a/page", nil)
+ first.Header.Set("A", "u;B=v")
+ first.Header.Set("B", "w")
+ second := httptest.NewRequest("GET", "http://a/page", nil)
+ second.Header.Set("A", "u")
+ second.Header.Set("B", "v;B=w")
+
+ if varyValue(first, vary) == varyValue(second, vary) {
+ t.Fatalf("varyValue rendered both requests as %q — a header value must not be able to forge the separators around it", varyValue(first, vary))
+ }
+
+ c := NewCache(10, 1<<20)
+ base := CacheKey{Host: "a", Method: "GET", Path: "/page"}
+ put := func(r *http.Request, body string) {
+ k := base
+ k.VaryValue = varyValue(r, vary)
+ c.Put(k, vary, &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte(body), StoredAt: time.Now(), TTL: time.Minute})
+ }
+ put(first, "for-first")
+ put(second, "for-second")
+
+ c.mu.Lock()
+ variants := len(c.byURL[base.urlKey()])
+ c.mu.Unlock()
+ if variants != 2 {
+ t.Fatalf("URL holds %d entries, want 2 — the two requests select on different values and must occupy different slots", variants)
+ }
+ if got, ok := c.GetFor(base, first); !ok || string(got.Body) != "for-first" {
+ t.Fatalf("GetFor(first) = %v, %v; want for-first", got, ok)
+ }
+ if got, ok := c.GetFor(base, second); !ok || string(got.Body) != "for-second" {
+ t.Fatalf("GetFor(second) = %v, %v; want for-second — the second request must never be served the first's body", got, ok)
+ }
+}
+
+// Header field names are case-insensitive (RFC 9110 §5.1), so a response
+// spelling its Vary "accept-encoding" names the same scheme as one spelling it
+// "Accept-Encoding". They must share one slot — otherwise two middlewares
+// disagreeing on capitalization quietly halve a URL's variant allowance and
+// double the misses.
+func TestCacheVaryNameCaseIsCanonicalizedToOneVariant(t *testing.T) {
+ c := NewCache(10, 1<<20)
+ base := CacheKey{Host: "a", Method: "GET", Path: "/page"}
+ req := httptest.NewRequest("GET", "http://a/page", nil)
+ req.Header.Set("Accept-Encoding", "gzip")
+ put := func(vary, body string) {
+ k := base
+ k.VaryValue = varyValue(req, vary)
+ c.Put(k, vary, &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte(body), StoredAt: time.Now(), TTL: time.Minute})
+ }
+ put("accept-encoding", "lowercase")
+ put("Accept-Encoding", "canonical")
+
+ c.mu.Lock()
+ variants := len(c.byURL[base.urlKey()])
+ c.mu.Unlock()
+ if variants != 1 {
+ t.Fatalf("URL holds %d entries, want 1 — Vary: accept-encoding and Vary: Accept-Encoding are the same scheme", variants)
+ }
+ if got, ok := c.GetFor(base, req); !ok || string(got.Body) != "canonical" {
+ t.Fatalf("GetFor = %v, %v; want canonical — the second store must have replaced the first, not landed beside it", got, ok)
+ }
+}
+
+// The Vary list is a set, not a sequence (RFC 9110 §12.5.5): "Vary: B, A" and
+// "Vary: A, B" select on the same headers. Canonical sorting means they share
+// one slot AND render values in the same order, so an app that emits its Vary
+// through two different middlewares does not split its own cache.
+func TestCacheVaryNameOrderIsCanonicalizedToOneVariant(t *testing.T) {
+ c := NewCache(10, 1<<20)
+ base := CacheKey{Host: "a", Method: "GET", Path: "/page"}
+ req := httptest.NewRequest("GET", "http://a/page", nil)
+ req.Header.Set("A", "one")
+ req.Header.Set("B", "two")
+ if varyValue(req, "B, A") != varyValue(req, "A, B") {
+ t.Fatalf("varyValue(B, A) = %q, varyValue(A, B) = %q; want identical — the Vary list has no ordering meaning", varyValue(req, "B, A"), varyValue(req, "A, B"))
+ }
+ put := func(vary, body string) {
+ k := base
+ k.VaryValue = varyValue(req, vary)
+ c.Put(k, vary, &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte(body), StoredAt: time.Now(), TTL: time.Minute})
+ }
+ put("B, A", "stored-as-B-A")
+ put("A, B", "stored-as-A-B")
+
+ c.mu.Lock()
+ variants := len(c.byURL[base.urlKey()])
+ c.mu.Unlock()
+ if variants != 1 {
+ t.Fatalf("URL holds %d entries, want 1 — Vary: B, A and Vary: A, B are the same scheme", variants)
+ }
+ if got, ok := c.GetFor(base, req); !ok || string(got.Body) != "stored-as-A-B" {
+ t.Fatalf("GetFor = %v, %v; want stored-as-A-B", got, ok)
+ }
+}
+
+// A Vary that is nothing but separators names no headers, so it selects on
+// nothing — which is what "no Vary" means. It must therefore take the no-Vary
+// path exactly: specificity 0, one shared slot with a genuinely Vary-less
+// response, and a match for every request. Left uncanonicalized it rendered
+// four empty names ("=;=;=;=;"), giving a scheme that also matched everything
+// but outranked real single-header variants with a specificity of 4.
+func TestCacheVaryOfOnlySeparatorsIsTreatedAsNoVary(t *testing.T) {
+ c := NewCache(10, 1<<20)
+ base := CacheKey{Host: "a", Method: "GET", Path: "/page"}
+ req := httptest.NewRequest("GET", "http://a/page", nil)
+ if vv := varyValue(req, ", , ,"); vv != "" {
+ t.Fatalf("varyValue(%q) = %q, want \"\" — a Vary naming no headers selects on nothing", ", , ,", vv)
+ }
+ c.Put(base, ", , ,", &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte("separators"), StoredAt: time.Now(), TTL: time.Minute})
+
+ c.mu.Lock()
+ var stored *cacheEntry
+ for _, el := range c.byURL[base.urlKey()] {
+ stored = el.Value.(*cacheEntry)
+ }
+ c.mu.Unlock()
+ if stored == nil {
+ t.Fatal("nothing cached")
+ }
+ if stored.key.Vary != "" {
+ t.Fatalf("stored under Vary %q, want \"\" — a Vary of only separators canonicalizes to no Vary at all", stored.key.Vary)
+ }
+ if s := stored.specificity(); s != 0 {
+ t.Fatalf("specificity = %d, want 0 — it must not outrank variants selected on real headers", s)
+ }
+
+ // Same slot as a genuinely Vary-less response, and a match for any request.
+ c.Put(base, "", &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte("no-vary"), StoredAt: time.Now(), TTL: time.Minute})
+ c.mu.Lock()
+ variants := len(c.byURL[base.urlKey()])
+ c.mu.Unlock()
+ if variants != 1 {
+ t.Fatalf("URL holds %d entries, want 1 — an empty Vary scheme is the no-Vary scheme", variants)
+ }
+ other := httptest.NewRequest("GET", "http://a/page", nil)
+ other.Header.Set("Accept-Language", "fr")
+ if got, ok := c.GetFor(base, other); !ok || string(got.Body) != "no-vary" {
+ t.Fatalf("GetFor = %v, %v; want no-vary for any request shape", got, ok)
+ }
+}
+
+// selectLocked ranges over a map, whose iteration order Go randomizes. Two
+// variants tied on both specificity and StoredAt must still resolve the same
+// way every time: a shared cache that answers one request two ways, run to run,
+// is unreproducible exactly when that matters.
+func TestCacheSelectionIsDeterministicWhenSpecificityAndAgeTie(t *testing.T) {
+ base := CacheKey{Host: "a", Method: "GET", Path: "/page"}
+ // Neither header is sent, so both entries match; both were stored at the
+ // same instant on one selecting header each.
+ req := httptest.NewRequest("GET", "http://a/page", nil)
+ stored := time.Now()
+
+ for i := 0; i < 50; i++ {
+ c := NewCache(10, 1<<20)
+ for _, v := range []struct{ vary, body string }{{"A", "selected-on-A"}, {"B", "selected-on-B"}} {
+ k := base
+ k.VaryValue = varyValue(req, v.vary)
+ c.Put(k, v.vary, &CachedResponse{StatusCode: 200, Header: http.Header{}, Body: []byte(v.body), StoredAt: stored, TTL: time.Minute})
+ }
+ got, ok := c.GetFor(base, req)
+ if !ok {
+ t.Fatal("expected a hit")
+ }
+ // The tie-break is by key, so the A variant wins every round.
+ if string(got.Body) != "selected-on-A" {
+ t.Fatalf("round %d served %q, want selected-on-A every round — a tie must not resolve by map iteration order", i, got.Body)
+ }
}
}
@@ -142,11 +456,11 @@ func TestCachePurgeHostRemovesEveryEntryForThatHost(t *testing.T) {
t.Fatalf("PurgeHost removed %d entries, want %d", n, len(keys))
}
for _, k := range keys {
- if _, ok := c.Get(k); ok {
+ if _, ok := c.get(k); ok {
t.Fatalf("entry %+v survived PurgeHost", k)
}
}
- if _, ok := c.Get(other); !ok {
+ if _, ok := c.get(other); !ok {
t.Fatal("PurgeHost(\"a\") must leave host b's entries untouched")
}
if n := c.PurgeHost("nobody"); n != 0 {
@@ -190,13 +504,13 @@ func TestCachePurgeURLRemovesAllVaryVariants(t *testing.T) {
if n != 2 {
t.Fatalf("purged %d entries, want 2", n)
}
- if _, ok := c.Get(en); ok {
+ if _, ok := c.get(en); ok {
t.Fatal("expected en variant purged")
}
- if _, ok := c.Get(fr); ok {
+ if _, ok := c.get(fr); ok {
t.Fatal("expected fr variant purged")
}
- if _, ok := c.Get(other); !ok {
+ if _, ok := c.get(other); !ok {
t.Fatal("expected unrelated URL to survive purge")
}
}
diff --git a/internal/edge/edge.go b/internal/edge/edge.go
index 3e9f2f30e..28a182909 100644
--- a/internal/edge/edge.go
+++ b/internal/edge/edge.go
@@ -26,9 +26,11 @@ import (
"net"
"net/http"
"net/http/httputil"
+ "net/textproto"
"os"
"os/signal"
"path/filepath"
+ "slices"
"strconv"
"strings"
"sync/atomic"
@@ -291,10 +293,11 @@ func (e *Edge) ServeHTTP(w http.ResponseWriter, r *http.Request) {
var key CacheKey
if cacheable {
key = CacheKey{Host: host, Method: r.Method, Path: r.URL.Path, RawQuery: r.URL.RawQuery}
- // GetFor, not Get: the variant this request belongs to depends on the
- // response's Vary header, which doesn't exist yet — GetFor resolves it
- // from the Vary names the last response for this URL recorded, so the
- // lookup key matches the key maybeStore stored under.
+ // GetFor, not get: the variant this request belongs to depends on the
+ // response's Vary header, which doesn't exist yet — GetFor matches r
+ // against the Vary names each stored variant recorded for itself, so
+ // the entry it returns is one maybeStore stored under exactly these
+ // request headers.
if resp, hit := e.cache.GetFor(key, r); hit {
writeCached(w, resp)
return
@@ -394,6 +397,10 @@ func (e *Edge) maybeStore(key CacheKey, rec *recordingResponseWriter) {
if ttl > DefaultCacheMaxTTL {
ttl = DefaultCacheMaxTTL
}
+ // The refusals below read the RAW joined field value, before any
+ // canonicalization, and compare case-insensitively: whatever normalizing
+ // happens downstream, "Vary: *" and "Vary: cookie" must be refused in every
+ // spelling and on every line they can arrive in.
vary := joinHeader(rec.Header(), "Vary")
if vary != "" {
for _, h := range strings.Split(vary, ",") {
@@ -403,12 +410,16 @@ func (e *Edge) maybeStore(key CacheKey, rec *recordingResponseWriter) {
}
}
// The same computation GetFor replays at lookup time — both call
- // varyValue so the store key and the lookup key can never drift.
+ // varyValue, which canonicalizes the name list itself, so the store key
+ // and the lookup key can never drift on either count.
key.VaryValue = varyValue(rec.request, vary)
}
- // vary is handed to Put as well as consumed here: it is what Put records
- // (or, when empty, clears) as this URL's Vary scheme, so the next request's
- // GetFor resolves the very variant this store used.
+ // vary is handed to Put as well as consumed here: Put canonicalizes it the
+ // same way varyValue just did and stores it with the entry, as part of its
+ // key, so a later GetFor can replay this exact selection against a later
+ // request — and so a response that varies by different headers, or doesn't
+ // vary at all, becomes a variant beside this one rather than a replacement
+ // for it.
e.cache.Put(key, vary, &CachedResponse{
StatusCode: rec.status,
Header: rec.Header().Clone(),
@@ -435,22 +446,73 @@ func joinHeader(h http.Header, name string) string {
return strings.Join(h.Values(name), ", ")
}
-// varyValue concatenates r's values for each header named in the response's
-// Vary list, in the order Vary names them — the same raw string always
-// produces the same bucket, which is all correctness requires (standard HTTP
-// Vary semantics; RFC 7234 §4.1).
+// canonicalVaryNames reduces a Vary field value to the set of header names it
+// selects on, in one canonical form: split on commas (a Vary may be comma-
+// joined from several lines — see joinHeader), trimmed, empty tokens dropped,
+// case-canonicalized, deduplicated and sorted.
//
-// vary itself may be a comma-joined multi-line field (see joinHeader); the
-// split-and-trim below handles that form identically to a single line. The
-// request side reads all of a header's lines too, so two requests that differ
-// only in how they split a varied-on header still land in different buckets.
-func varyValue(r *http.Request, vary string) string {
- var b strings.Builder
+// Everything downstream — the Vary component of the cache key, the specificity
+// count, and the order varyValue renders values in — is derived from THIS list
+// rather than from the raw field value, so spellings that mean the same thing
+// share one cache slot instead of splitting the URL's variant allowance into
+// near-duplicates: "Vary: accept-encoding" and "Vary: Accept-Encoding" are one
+// scheme, and so are "Vary: A, B" and "Vary: B, A" (RFC 9110 §5.1: field names
+// are case-insensitive; §12.5.5 gives the Vary list no ordering meaning).
+//
+// A Vary of only separators ("Vary: , , ,") yields no names at all, which is
+// exactly a response that selects on nothing: it canonicalizes to "", takes the
+// no-Vary path through Put and matches(), and has specificity 0.
+func canonicalVaryNames(vary string) []string {
+ var names []string
for _, h := range strings.Split(vary, ",") {
h = strings.TrimSpace(h)
+ if h == "" {
+ continue
+ }
+ h = textproto.CanonicalMIMEHeaderKey(h)
+ if slices.Contains(names, h) {
+ continue
+ }
+ names = append(names, h)
+ }
+ slices.Sort(names)
+ return names
+}
+
+// canonicalVary renders canonicalVaryNames back as a field value. This is the
+// form stored in cacheFullKey.Vary, so key equality is spelling-independent.
+func canonicalVary(vary string) string {
+ return strings.Join(canonicalVaryNames(vary), ", ")
+}
+
+// varyValue concatenates r's values for each header the response's Vary list
+// names, in canonical name order, so the same selecting headers with the same
+// request values always produce the same bucket (standard HTTP Vary semantics;
+// RFC 7234 §4.1). Both the store path (maybeStore) and the lookup path
+// (cacheEntry.matches) render through this one function, so the two can never
+// disagree about either the canonicalization or the rendering.
+//
+// Each value is LENGTH-PREFIXED — "Name=<len>:<value>;" — which is what makes
+// the rendering injective for a given name list. Without the prefix the
+// separators were forgeable from inside a header value: under Vary: A, B a
+// request sending A: "u;B=v", B: "w" rendered "A=u;B=v;B=w;", byte-identical to
+// A: "u", B: "v;B=w" — two different requests sharing one cache slot, so
+// whichever stored first had its body served to the other. A length prefix
+// cannot be forged from within the value it measures: the reader takes exactly
+// len bytes, so no content can be mistaken for structure.
+//
+// The request side reads all of a header's lines (joinHeader), so two requests
+// that differ only in how they split a varied-on header still land in the same
+// bucket, which is what RFC 9110 §5.3 says they mean.
+func varyValue(r *http.Request, vary string) string {
+ var b strings.Builder
+ for _, h := range canonicalVaryNames(vary) {
+ v := joinHeader(r.Header, h)
b.WriteString(h)
b.WriteByte('=')
- b.WriteString(joinHeader(r.Header, h))
+ b.WriteString(strconv.Itoa(len(v)))
+ b.WriteByte(':')
+ b.WriteString(v)
b.WriteByte(';')
}
return b.String()
diff --git a/internal/edge/edge_test.go b/internal/edge/edge_test.go
index 08a1f2fd3..651fd1df7 100644
--- a/internal/edge/edge_test.go
+++ b/internal/edge/edge_test.go
@@ -576,7 +576,7 @@ func TestServeHTTPNeverCachesNon2xx(t *testing.T) {
e.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "http://err.example.com/event", nil))
- if _, ok := e.cache.Get(CacheKey{Host: "err.example.com", Method: "GET", Path: "/event"}); ok {
+ if _, ok := e.cache.get(CacheKey{Host: "err.example.com", Method: "GET", Path: "/event"}); ok {
t.Fatal("a 500 response must never be cached")
}
}
@@ -723,7 +723,7 @@ func cacheProbeEdge(t *testing.T, host string, hdr func(path string, h http.Head
}
func cachedAt(e *Edge, host, path string) bool {
- _, ok := e.cache.Get(CacheKey{Host: host, Method: "GET", Path: path})
+ _, ok := e.cache.get(CacheKey{Host: host, Method: "GET", Path: path})
return ok
}
@@ -807,7 +807,7 @@ func TestServeHTTPClampsMaxAgeToCeiling(t *testing.T) {
})
e.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "http://ttl.example.com/event", nil))
- resp, ok := e.cache.Get(CacheKey{Host: "ttl.example.com", Method: "GET", Path: "/event"})
+ resp, ok := e.cache.get(CacheKey{Host: "ttl.example.com", Method: "GET", Path: "/event"})
if !ok {
t.Fatal("expected the response to be cached")
}
@@ -837,7 +837,7 @@ func TestServeHTTPRefusesVaryCookie(t *testing.T) {
e.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "http://cookie.example.com/event", nil))
- if _, ok := e.cache.Get(CacheKey{Host: "cookie.example.com", Method: "GET", Path: "/event"}); ok {
+ if _, ok := e.cache.get(CacheKey{Host: "cookie.example.com", Method: "GET", Path: "/event"}); ok {
t.Fatal("a response Vary-ing on Cookie must never be cached — it would defeat per-visitor correctness")
}
}
@@ -912,6 +912,40 @@ func TestServeHTTPRefusesVaryCookieSplitAcrossHeaderLines(t *testing.T) {
}
}
+// The Vary refusals read the raw field value and compare case-insensitively,
+// so they fire on every spelling a response can use — including the ones the
+// canonicalization downstream would have tidied up anyway. Pinned separately
+// because canonicalization is the kind of change that invites moving the
+// refusals behind it, where a name the canonicalizer does not recognise (or
+// declines to rewrite) would slip past.
+func TestServeHTTPRefusesVaryCookieAndStarInAnySpelling(t *testing.T) {
+ e := cacheProbeEdge(t, "spell.example.com", func(path string, h http.Header) {
+ h.Set("Cache-Control", "max-age=60")
+ switch path {
+ case "/lower":
+ h.Set("Vary", "accept-encoding, cookie")
+ case "/upper":
+ h.Add("Vary", "Accept-Language")
+ h.Add("Vary", "COOKIE")
+ case "/star":
+ h.Set("Vary", "Accept-Encoding , * ")
+ case "/ok":
+ h.Set("Vary", "accept-encoding")
+ }
+ })
+ for _, path := range []string{"/lower", "/upper", "/star", "/ok"} {
+ e.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "http://spell.example.com"+path, nil))
+ }
+ for _, path := range []string{"/lower", "/upper", "/star"} {
+ if n := cachedVariants(e, "spell.example.com", path); n != 0 {
+ t.Errorf("%s: %d variants cached, want 0 — Vary: Cookie and Vary: * must be refused whatever their spelling or line", path, n)
+ }
+ }
+ if n := cachedVariants(e, "spell.example.com", "/ok"); n != 1 {
+ t.Errorf("/ok: %d variants cached, want 1 — an ordinary Vary in lower case must still be cached", n)
+ }
+}
+
// The same root cause on Cache-Control: with max-age=600 on one line and
// private on the next, Header.Get saw only max-age and the private response
// was cached at the shared edge — the cross-visitor leak the private rule
@@ -1088,6 +1122,62 @@ func TestServeHTTPRefusesVaryStar(t *testing.T) {
}
}
+// An app that emits Vary conditionally — Vary: Accept-Language when the
+// request negotiates a language, no Vary at all when it doesn't — used to
+// thrash its own cache entries. One Vary scheme was recorded per URL, so every
+// flip between the two response shapes rewrote that record and swept every
+// variant stored under the previous scheme. Worse than a miss: with the record
+// cleared, a language-negotiating request resolved to the EMPTY variant and was
+// served the no-Vary body, so the language the app negotiated silently
+// disappeared for everyone.
+//
+// With the Vary names stored per entry, the two schemes coexist: each request
+// shape matches the entry its own shape produced, both stay live, and only the
+// first of each is a miss.
+func TestServeHTTPConditionalVaryKeepsBothVariantsLive(t *testing.T) {
+ var hits atomic.Int64
+ backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ hits.Add(1)
+ w.Header().Set("Cache-Control", "max-age=60")
+ lang := r.Header.Get("Accept-Language")
+ if lang != "" {
+ w.Header().Set("Vary", "Accept-Language")
+ }
+ w.WriteHeader(200)
+ w.Write([]byte("lang=" + lang))
+ }))
+ t.Cleanup(backend.Close)
+
+ reg := openReg(t)
+ if err := reg.Upsert(registry.Route{Host: "mixedvary.example.com", Kind: "instance", Addr: backend.Listener.Addr().String(), Status: "ready"}); err != nil {
+ t.Fatalf("upsert: %v", err)
+ }
+ e, err := New(reg, nil)
+ if err != nil {
+ t.Fatalf("New: %v", err)
+ }
+ e.EnableCache(NewCache(DefaultCacheMaxEntries, DefaultCacheMaxBodyBytes))
+ const url = "http://mixedvary.example.com/page"
+
+ // Alternate the two shapes. The first of each is a legitimate miss; every
+ // repeat after that must be served from cache, with its OWN body.
+ for i := 0; i < 6; i++ {
+ if got := getLang(t, e, url, "en"); got != "lang=en" {
+ t.Fatalf("round %d: language-negotiating request served %q, want %q", i, got, "lang=en")
+ }
+ if got := getLang(t, e, url, ""); got != "lang=" {
+ t.Fatalf("round %d: plain request served %q, want %q", i, got, "lang=")
+ }
+ }
+
+ if n := hits.Load(); n != 2 {
+ t.Fatalf("backend invoked %d times across 12 requests alternating two Vary schemes, want 2 — after one miss each, both variants must stay live and hit", n)
+ }
+ if n := cachedVariants(e, "mixedvary.example.com", "/page"); n != 2 {
+ t.Fatalf("URL holds %d cached variants, want 2 — the varying and the non-varying response must coexist, not evict each other", n)
+ }
+}
+
// hijackableRecorder embeds httptest.ResponseRecorder (which deliberately
// does NOT implement http.Hijacker) and adds a no-op Hijack, standing in for
// a real connection's ResponseWriter — the minimum shape needed to prove
diff --git a/internal/edge/purge_test.go b/internal/edge/purge_test.go
index f3b39c6f3..60d86f8b1 100644
--- a/internal/edge/purge_test.go
+++ b/internal/edge/purge_test.go
@@ -48,7 +48,7 @@ func TestListenPurgeRemovesCachedEntry(t *testing.T) {
t.Fatalf("purge status = %d, want 200", resp.StatusCode)
}
- if _, ok := cache.Get(CacheKey{Host: "a.example", Method: "GET", Path: "/event"}); ok {
+ if _, ok := cache.get(CacheKey{Host: "a.example", Method: "GET", Path: "/event"}); ok {
t.Fatal("expected entry to be purged")
}
}
@@ -72,7 +72,7 @@ func TestListenPurgeCannotAffectAnotherHost(t *testing.T) {
resp, _ := client.Post("http://unix/purge", "application/json", bytes.NewReader(body))
resp.Body.Close()
- if _, ok := cache.Get(CacheKey{Host: "other.example", Method: "GET", Path: "/event"}); !ok {
+ if _, ok := cache.get(CacheKey{Host: "other.example", Method: "GET", Path: "/event"}); !ok {
t.Fatal("a purge listener bound to a.example must never affect other.example's cache entries")
}
}