Commit ee0c5e7
Eric Bower
·
2026-08-13 10:17:32 -0400 EDT
parent 538c3e9
fix(httpscache): flaky test
2 files changed,
+26,
-14
+17,
-13
| ... | ... | @@ -729,19 +729,22 @@ func TestCacheExpires(t *testing.T) { | |
| 729 | 729 | // RFC 9111 4.3.4 304 Not Modified | |
| 730 | 730 | // https://www.rfc-editor.org/rfc/rfc9111.html#section-4.3.4 | |
| 731 | 731 | // When a cached entry is validated and the origin responds with 304, the cache: | |
| 732 | - | // - Returns 304 to the client | |
| 733 | 732 | // - Updates header metadata from the 304 response | |
| 734 | 733 | // - Retains the cached body for subsequent requests. | |
| 734 | + | // If the client's conditional headers no longer match the updated cache entry, | |
| 735 | + | // the cache serves 200 with the full cached body. | |
| 735 | 736 | func TestCache304NotModifiedMerge(t *testing.T) { | |
| 736 | 737 | originCalls := 0 | |
| 737 | 738 | ||
| 738 | - | // Validation handler: returns 304 when ETag matches, 200 otherwise | |
| 739 | + | // Validation handler: returns 304 when ETag matches, 200 otherwise. | |
| 740 | + | // The 304 response includes an updated ETag header (set before WriteHeader). | |
| 739 | 741 | validationMux := http.NewServeMux() | |
| 740 | 742 | validationMux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { | |
| 741 | 743 | originCalls++ | |
| 742 | 744 | if r.Header.Get("If-None-Match") == "\"abc\"" { | |
| 743 | - | w.WriteHeader(http.StatusNotModified) | |
| 745 | + | // Set headers before WriteHeader (correct HTTP practice). | |
| 744 | 746 | w.Header().Set("etag", "\"abc-updated\"") | |
| 747 | + | w.WriteHeader(http.StatusNotModified) | |
| 745 | 748 | return | |
| 746 | 749 | } | |
| 747 | 750 | w.Header().Set("etag", "\"abc\"") |
| ... | ... | @@ -766,22 +769,23 @@ func TestCache304NotModifiedMerge(t *testing.T) { | |
| 766 | 769 | cacheData, _ := json.Marshal(staleCv) | |
| 767 | 770 | handler.Cache.Add(cacheKey, cacheData) | |
| 768 | 771 | ||
| 769 | - | // First request with If-None-Match triggers validation; origin returns 304. | |
| 770 | - | // Client sent conditional headers, so if they still match the updated cache | |
| 771 | - | // entry, the client gets 304. Here the upstream updated the ETag to "abc-updated" | |
| 772 | - | // so the client's If-None-Match "abc" no longer matches — serve cached body as 200. | |
| 772 | + | // First request with If-None-Match triggers validation; origin returns 304 | |
| 773 | + | // with an updated ETag "abc-updated". | |
| 774 | + | // The cache merges the new ETag into the stored entry, then re-evaluates | |
| 775 | + | // the client's If-None-Match "abc" against the updated ETag "abc-updated". | |
| 776 | + | // They no longer match, so the cache serves the full cached body as 200. | |
| 773 | 777 | resp1, _ := tc.DoWithHeaders(req, map[string][]string{ | |
| 774 | 778 | "If-None-Match": {"\"abc\""}, | |
| 775 | 779 | }) | |
| 776 | - | if resp1.StatusCode != http.StatusNotModified { | |
| 777 | - | t.Errorf("expected 304 (ETag changed after revalidation), got %d", resp1.StatusCode) | |
| 780 | + | if resp1.StatusCode != http.StatusOK { | |
| 781 | + | t.Errorf("expected 200 (updated ETag no longer matches client If-None-Match), got %d", resp1.StatusCode) | |
| 778 | 782 | } | |
| 779 | 783 | status := resp1.Header.Get("cache-status") | |
| 780 | - | if !strings.Contains(status, "fwd=stale") { | |
| 781 | - | t.Errorf("expected cache-status hit, got %s", status) | |
| 784 | + | if !strings.Contains(status, "hit") { | |
| 785 | + | t.Errorf("expected cache-status to contain 'hit', got %s", status) | |
| 782 | 786 | } | |
| 783 | 787 | ||
| 784 | - | // Second request without conditional headers should still serve the cached body | |
| 788 | + | // Second request without conditional headers should still serve the cached body. | |
| 785 | 789 | resp2, _ := tc.Do(req) | |
| 786 | 790 | if resp2.StatusCode != http.StatusOK { | |
| 787 | 791 | t.Errorf("expected 200, got %d", resp2.StatusCode) |
| ... | ... | @@ -797,7 +801,7 @@ func TestCache304NotModifiedMerge(t *testing.T) { | |
| 797 | 801 | t.Errorf("expected cache-status hit on second request, got %s", status2) | |
| 798 | 802 | } | |
| 799 | 803 | ||
| 800 | - | // Origin should have been called exactly once (304 validation only) | |
| 804 | + | // Origin should have been called exactly once (304 validation only). | |
| 801 | 805 | if originCalls != 1 { | |
| 802 | 806 | t.Errorf("expected 1 origin call, got %d", originCalls) | |
| 803 | 807 | } |
+9,
-1
| ... | ... | @@ -145,11 +145,19 @@ func (c *HttpCache) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |
| 145 | 145 | ||
| 146 | 146 | // Merge non-forbidden headers from the 304 response into the cached entry. | |
| 147 | 147 | // Normalize keys to lowercase to avoid case-sensitivity issues. | |
| 148 | + | // Delete any existing case-insensitive duplicates first so that getHeader | |
| 149 | + | // cannot find both the old and new values on random map iteration. | |
| 148 | 150 | for key, values := range wrapped.Header() { | |
| 149 | 151 | if isForbiddenHeader(key) { | |
| 150 | 152 | continue | |
| 151 | 153 | } | |
| 152 | - | cacheValue.Header[strings.ToLower(key)] = values | |
| 154 | + | normKey := strings.ToLower(key) | |
| 155 | + | for existing := range cacheValue.Header { | |
| 156 | + | if strings.EqualFold(existing, normKey) { | |
| 157 | + | delete(cacheValue.Header, existing) | |
| 158 | + | } | |
| 159 | + | } | |
| 160 | + | cacheValue.Header[normKey] = values | |
| 153 | 161 | } | |
| 154 | 162 | // Revalidation refreshes the entry -- reset CreatedAt so it's fresh again. | |
| 155 | 163 | cacheValue.CreatedAt = time.Now() |