repos / pico

commit

d758032

parent

272d185

author

Eric Bower

date

2026-04-19 11:32:48 -0400 EDT

fix(pgs): 304 send correct status and headers

 1@@ -765,12 +765,15 @@ func TestCache304NotModifiedMerge(t *testing.T) {
 2 	cacheData, _ := json.Marshal(staleCv)
 3 	handler.Cache.Add(cacheKey, cacheData)
 4 
 5-	// First request with If-None-Match triggers validation; origin returns 304
 6+	// First request with If-None-Match triggers validation; origin returns 304.
 7+	// Client sent conditional headers, so if they still match the updated cache
 8+	// entry, the client gets 304. Here the upstream updated the ETag to "abc-updated"
 9+	// so the client's If-None-Match "abc" no longer matches — serve cached body as 200.
10 	resp1, _ := tc.DoWithHeaders(req, map[string][]string{
11 		"If-None-Match": {"\"abc\""},
12 	})
13-	if resp1.StatusCode != http.StatusNotModified {
14-		t.Errorf("expected 304, got %d", resp1.StatusCode)
15+	if resp1.StatusCode != http.StatusOK {
16+		t.Errorf("expected 200 (ETag changed after revalidation), got %d", resp1.StatusCode)
17 	}
18 	status := resp1.Header.Get("cache-status")
19 	if !strings.Contains(status, "hit") {
20@@ -889,14 +892,15 @@ func TestCache304NoBody(t *testing.T) {
21 	cacheData, _ := json.Marshal(cv)
22 	handler.Cache.Add(cacheKey, cacheData)
23 
24-	// Trigger revalidation — upstream returns 304 with a spurious body
25+	// Trigger revalidation — upstream returns 304 with a spurious body.
26+	// Client request is unconditional, so cache serves the stored body as 200.
27 	resp, _ := tc.Do(req)
28-	if resp.StatusCode != http.StatusNotModified {
29-		t.Fatalf("expected 304, got %d", resp.StatusCode)
30+	if resp.StatusCode != http.StatusOK {
31+		t.Fatalf("expected 200, got %d", resp.StatusCode)
32 	}
33 	body, _ := readBody(resp)
34-	if body != "" {
35-		t.Errorf("expected empty body for 304 response, got %q", body)
36+	if body != "original body" {
37+		t.Errorf("expected cached body 'original body', got %q", body)
38 	}
39 }
40 
41@@ -1075,8 +1079,10 @@ func TestCacheMustRevalidateRevalidationHeaders(t *testing.T) {
42 
43 			resp, _ := tc.Do(req)
44 
45-			if resp.StatusCode != http.StatusNotModified {
46-				t.Errorf("expected 304, got %d", resp.StatusCode)
47+			// Client request is unconditional — after upstream 304, cache
48+			// serves the stored body as 200.
49+			if resp.StatusCode != http.StatusOK {
50+				t.Errorf("expected 200, got %d", resp.StatusCode)
51 			}
52 			status := resp.Header.Get("cache-status")
53 			if !strings.Contains(status, "hit") {

  1@@ -90,6 +90,12 @@ func (c *HttpCache) ServeHTTP(w http.ResponseWriter, r *http.Request) {
  2 
  3 	// RFC 9111 4.2.4 + 4.3.1/4.3.2: stale must-revalidate entries must be
  4 	// revalidated with conditional headers derived from the stored response.
  5+	// Preserve original client conditional headers so we can evaluate them
  6+	// after revalidation to decide whether the client gets 304 or 200.
  7+	clientIfNoneMatch := r.Header.Get("If-None-Match")
  8+	clientIfModifiedSince := r.Header.Get("If-Modified-Since")
  9+	clientConditional := clientIfNoneMatch != "" || clientIfModifiedSince != ""
 10+
 11 	if err.Error() == "cache is stale and must-revalidate requires revalidation" {
 12 		if cachedData, exists := c.Cache.Get(cacheKey); exists {
 13 			var cachedValue CacheValue
 14@@ -114,22 +120,58 @@ func (c *HttpCache) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 15 	if wrapped.StatusCode() == http.StatusNotModified {
 16 		log.Info("304 not modified, updating cached headers")
 17 		existingData, exists := c.Cache.Get(cacheKey)
 18-		if exists {
 19-			var cacheValue CacheValue
 20-			if json.Unmarshal(existingData, &cacheValue) == nil {
 21-				// Merge headers from the 304 response into the cached entry.
 22-				for key, values := range wrapped.Header() {
 23-					cacheValue.Header[key] = values
 24+		if !exists {
 25+			// Cache entry vanished; forward the 304 as-is.
 26+			wrapped.Send()
 27+			return
 28+		}
 29+
 30+		var cacheValue CacheValue
 31+		if json.Unmarshal(existingData, &cacheValue) != nil {
 32+			wrapped.Send()
 33+			return
 34+		}
 35+
 36+		// Merge non-forbidden headers from the 304 response into the cached entry.
 37+		for key, values := range wrapped.Header() {
 38+			if isForbiddenHeader(key) {
 39+				continue
 40+			}
 41+			cacheValue.Header[key] = values
 42+		}
 43+		// Revalidation refreshes the entry — reset CreatedAt so it's fresh again.
 44+		cacheValue.CreatedAt = time.Now()
 45+		enc, _ := json.Marshal(cacheValue)
 46+		c.Cache.Add(cacheKey, enc)
 47+		c.AddCacheItem(float64(len(enc)))
 48+
 49+		if clientConditional {
 50+			// Client sent conditional headers — re-evaluate against the
 51+			// updated cached entry and return 304 if it still matches.
 52+			r.Header.Set("If-None-Match", clientIfNoneMatch)
 53+			r.Header.Set("If-Modified-Since", clientIfModifiedSince)
 54+			valid, status := c.handleValidation(r, &cacheValue)
 55+			if valid {
 56+				hdr := w.Header()
 57+				for key, values := range cacheValue.Header {
 58+					if isForbiddenHeader(key) {
 59+						continue
 60+					}
 61+					for _, value := range values {
 62+						hdr.Add(key, value)
 63+					}
 64 				}
 65-				// Revalidation refreshes the entry — reset CreatedAt so it's fresh again.
 66-				cacheValue.CreatedAt = time.Now()
 67-				enc, _ := json.Marshal(cacheValue)
 68-				c.Cache.Add(cacheKey, enc)
 69-				c.AddCacheItem(float64(len(enc)))
 70+				ageDur := calcAge(cacheValue.CreatedAt)
 71+				hdr.Set("age", strconv.Itoa(int(ageDur.Seconds())+1))
 72+				hdr.Set("cache-status", cacheStatusHit(cacheKey, c.Ttl.Seconds()))
 73+				w.WriteHeader(status)
 74+				return
 75 			}
 76 		}
 77-		wrapped.Header().Set("cache-status", cacheStatusHit(cacheKey, c.Ttl.Seconds()))
 78-		wrapped.Send()
 79+
 80+		// Client request was unconditional (or conditional but no longer matches) —
 81+		// serve the full cached response.
 82+		serveCache(w, c.Ttl, cacheKey, &cacheValue)
 83 		return
 84 	}
 85 
 86@@ -154,8 +196,8 @@ func (c *HttpCache) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 87 func isForbiddenHeader(key string) bool {
 88 	switch strings.ToLower(key) {
 89 	case "connection", "keep-alive", "proxy-authenticate", "proxy-authorization",
 90-		"teardown", "transfer-encoding", "upgrade", "proxy-connection",
 91-		"www-authenticate", "proxy-authentication-info":
 92+		"te", "trailer", "transfer-encoding", "upgrade", "proxy-connection",
 93+		"proxy-authentication-info":
 94 		return true
 95 	default:
 96 		return false
 97@@ -178,6 +220,9 @@ func serveCache(w http.ResponseWriter, freshness time.Duration, cacheKey string,
 98 	age := ageDur.Seconds()
 99 	hdr.Add("age", strconv.Itoa(int(age)+1))
100 	hdr.Add("cache-status", cacheStatusHit(cacheKey, freshness.Seconds()))
101+	if cacheValue.StatusCode != 0 && cacheValue.StatusCode != http.StatusOK {
102+		w.WriteHeader(cacheValue.StatusCode)
103+	}
104 	_, _ = w.Write(cacheValue.Body)
105 }
106 
107@@ -605,6 +650,8 @@ func (c *HttpCache) maybeUseCache(cacheKey string, w http.ResponseWriter, r *htt
108 				hdr.Add(key, value)
109 			}
110 		}
111+		ageDur := calcAge(cacheValue.CreatedAt)
112+		hdr.Set("age", strconv.Itoa(int(ageDur.Seconds())+1))
113 		hdr.Set("cache-status", cacheStatusHit(cacheKey, c.Ttl.Seconds()))
114 		w.WriteHeader(status)
115 		return nil