From 9a87d21e25333839547e0070a629a3cc3c09f536 Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Fri, 14 Aug 2026 12:43:55 -0500 Subject: [PATCH 1/2] Implement caching logic for conditional requests and add tests for cacheable statuses --- internal/cache/handlers.go | 46 +++++++++++- internal/cache/handlers_test.go | 84 ++++++++++++++++----- proxy_test.go | 125 ++++++++++++++++++++++++++++++-- 3 files changed, 228 insertions(+), 27 deletions(-) diff --git a/internal/cache/handlers.go b/internal/cache/handlers.go index 0e882e6..c73603c 100644 --- a/internal/cache/handlers.go +++ b/internal/cache/handlers.go @@ -62,6 +62,15 @@ var ignoreHeaders = map[string]struct{}{ "X-Pub-Os": {}, "X-Pub-Reason": {}, "X-Pub-Session-Id": {}, + // Conditional-request validators: collapse a conditional request onto the + // same key as its unconditional twin so it can be served from an already + // cached full 200 body instead of going upstream. We never cache a 304 + // (see cacheableStatus), so this key only ever holds a full response; a + // conditional request therefore either hits that body or passes through + // unchanged. Excluding them from the hash never forces a re-download of a + // body the client already has. + "If-None-Match": {}, + "If-Modified-Since": {}, } // generates the key used in the DB, includes a hash of the body @@ -190,6 +199,27 @@ func bodyless(status int, method string) bool { } } +// cacheableStatus reports whether a response with the given status code is worth +// persisting in the cache. Informational (1xx), no-content (204/205) and +// conditional (304 Not Modified) responses carry no reusable body, so caching +// them saves nothing. Worse, caching a 304 is actively harmful for a dependency +// updater: replaying a stale "Not Modified" hides upstream package updates that +// the client would otherwise fetch, and a client whose local copy no longer +// matches (e.g. pnpm) fails with ERR_PNPM_CACHE_MISSING_AFTER_304. These +// responses are always passed straight through instead. +func cacheableStatus(status int) bool { + switch { + case status >= 100 && status < 200: + return false + case status == http.StatusNoContent, + status == http.StatusResetContent, + status == http.StatusNotModified: + return false + default: + return true + } +} + // OnRequest checks to see if the response is cached, if so responds with the cached data. func (d *DB) OnRequest(r *http.Request, proxyCtx *goproxy.ProxyCtx) (*http.Request, *http.Response) { if d == nil { @@ -303,12 +333,24 @@ func (d *DB) OnResponse(resp *http.Response, proxyCtx *goproxy.ProxyCtx) *http.R // restore http.NoBody so goproxy sees an unmodified empty body and frames // the response without a chunked terminator. Leaving a non-NoBody body in // place would make goproxy stamp a "0\r\n\r\n" the client never reads, - // desyncing the keep-alive MITM tunnel. ContentLength is left untouched so - // a HEAD still advertises the length its GET would return. + // desyncing the keep-alive MITM tunnel. This normalization runs whether + // or not we go on to cache the response. if resp.Body != nil && resp.Body != http.NoBody { _ = resp.Body.Close() resp.Body = http.NoBody } + + if !cacheableStatus(resp.StatusCode) { + // Never cache conditional / no-content responses (1xx/204/205/304). + // A cached 304 would replay a stale "Not Modified" and hide upstream + // package updates from the updater; there is no body to reuse anyway. + // Pass it straight through, correctly framed. + return resp + } + + // A HEAD response for an otherwise cacheable status (e.g. 200) has no + // body but carries useful headers (notably Content-Length). Cache it + // headers-only so a later HEAD hit reproduces the same metadata. d.cacheDB[key] = &Entry{ Status: resp.StatusCode, ResponseHeaders: resp.Header, diff --git a/internal/cache/handlers_test.go b/internal/cache/handlers_test.go index 74d5b28..f75e890 100644 --- a/internal/cache/handlers_test.go +++ b/internal/cache/handlers_test.go @@ -148,28 +148,53 @@ func Test_bodyless(t *testing.T) { } } +func Test_cacheableStatus(t *testing.T) { + cases := []struct { + name string + status int + want bool + }{ + {"200 OK is cacheable", 200, true}, + {"203 Non-Authoritative is cacheable", 203, true}, + {"301 Moved Permanently is cacheable", 301, true}, + {"404 Not Found is cacheable", 404, true}, + {"100 Continue is not cacheable", 100, false}, + {"204 No Content is not cacheable", 204, false}, + {"205 Reset Content is not cacheable", 205, false}, + {"304 Not Modified is not cacheable", 304, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, cacheableStatus(tc.status)) + }) + } +} + // TestCache_BodylessResponses is a regression test for the keep-alive desync // introduced by goproxy v1.9.0. Bodyless responses (1xx/204/304/HEAD) must keep // Body == http.NoBody through both cache paths; otherwise goproxy stamps a // chunked terminator the client never reads, desyncing the reused MITM tunnel. +// Conditional / no-content statuses (1xx/204/205/304) are additionally never +// cached, so replaying them can't hide upstream updates; only a HEAD for a +// cacheable status is stored (headers-only). func TestCache_BodylessResponses(t *testing.T) { cases := []struct { name string method string status int contentLength string + wantCached bool wantHitContentLen int64 }{ - {"304 Not Modified", http.MethodGet, http.StatusNotModified, "", 0}, - {"204 No Content", http.MethodGet, http.StatusNoContent, "", 0}, - {"205 Reset Content", http.MethodGet, http.StatusResetContent, "", 0}, - {"HEAD 200", http.MethodHead, http.StatusOK, "", 0}, + {"304 Not Modified", http.MethodGet, http.StatusNotModified, "", false, 0}, + {"204 No Content", http.MethodGet, http.StatusNoContent, "", false, 0}, + {"205 Reset Content", http.MethodGet, http.StatusResetContent, "", false, 0}, + {"HEAD 200", http.MethodHead, http.StatusOK, "", true, 0}, // A HEAD advertises the length the equivalent GET would return; a cache // hit must report that same length, not force it to zero. - {"HEAD 200 with Content-Length", http.MethodHead, http.StatusOK, "5", 5}, - // A non-HEAD bodyless status must stay zero: a non-zero ContentLength - // with an empty body would fail resp.Write for GET responses. - {"304 with Content-Length", http.MethodGet, http.StatusNotModified, "5", 0}, + {"HEAD 200 with Content-Length", http.MethodHead, http.StatusOK, "5", true, 5}, + // A 304 is never cached even when it carries a Content-Length. + {"304 with Content-Length", http.MethodGet, http.StatusNotModified, "5", false, 0}, } for _, tc := range cases { @@ -198,7 +223,20 @@ func TestCache_BodylessResponses(t *testing.T) { resp = cacher.OnResponse(resp, missCtx) assert.True(t, resp.Body == http.NoBody, "OnResponse must leave bodyless Body as http.NoBody (not tee-wrapped)") - assert.Len(t, cacher.cacheDB, 1, "bodyless response should still be cached") + + if !tc.wantCached { + assert.Empty(t, cacher.cacheDB, + "conditional/no-content responses must not be cached") + + // --- A later identical request must be a miss (passed through). --- + hitReq := httptest.NewRequestWithContext(t.Context(), tc.method, URL, nil) + hitCtx := &goproxy.ProxyCtx{Req: hitReq} + _, hit := cacher.OnRequest(hitReq, hitCtx) + assert.Nil(t, hit, "uncached conditional response must not produce a cache hit") + return + } + + assert.Len(t, cacher.cacheDB, 1, "cacheable HEAD response should be cached") // --- Cache hit: OnRequest must serve a bodyless response. --- hitReq := httptest.NewRequestWithContext(t.Context(), tc.method, URL, nil) @@ -216,18 +254,21 @@ func TestCache_BodylessResponses(t *testing.T) { // TestCache_BodylessResponseWithWrappedBodyIsRestored verifies that when an // earlier response handler has replaced a bodyless response's http.NoBody with // another (empty) ReadCloser — as PythonIndexHandler does — OnResponse restores -// http.NoBody before caching. Otherwise goproxy sees resp.Body != http.NoBody, -// stamps a chunked terminator, and desyncs the keep-alive MITM tunnel. +// http.NoBody. Otherwise goproxy sees resp.Body != http.NoBody, stamps a chunked +// terminator, and desyncs the keep-alive MITM tunnel. This restoration happens +// whether or not the response is cached: conditional/no-content statuses are not +// cached, but their bodies must still be normalized before pass-through. func TestCache_BodylessResponseWithWrappedBodyIsRestored(t *testing.T) { cases := []struct { - name string - method string - status int + name string + method string + status int + wantCached bool }{ - {"HEAD 200", http.MethodHead, http.StatusOK}, - {"204 No Content", http.MethodGet, http.StatusNoContent}, - {"205 Reset Content", http.MethodGet, http.StatusResetContent}, - {"304 Not Modified", http.MethodGet, http.StatusNotModified}, + {"HEAD 200", http.MethodHead, http.StatusOK, true}, + {"204 No Content", http.MethodGet, http.StatusNoContent, false}, + {"205 Reset Content", http.MethodGet, http.StatusResetContent, false}, + {"304 Not Modified", http.MethodGet, http.StatusNotModified, false}, } for _, tc := range cases { @@ -257,7 +298,12 @@ func TestCache_BodylessResponseWithWrappedBodyIsRestored(t *testing.T) { assert.True(t, resp.Body == http.NoBody, "OnResponse must restore http.NoBody when a bodyless response was wrapped") assert.True(t, closed, "the replaced wrapper must be closed to avoid leaks") - assert.Len(t, cacher.cacheDB, 1, "bodyless response should still be cached") + if tc.wantCached { + assert.Len(t, cacher.cacheDB, 1, "cacheable HEAD response should be cached") + } else { + assert.Empty(t, cacher.cacheDB, + "conditional/no-content responses must not be cached") + } }) } } diff --git a/proxy_test.go b/proxy_test.go index 02034ae..22a1575 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -222,9 +222,11 @@ func TestProxyHTTPSMITMHeadCacheHitPreservesContentLength(t *testing.T) { require.Equal(t, int32(1), proxyDials.Load(), "both requests must reuse one tunnel") } -// TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse verifies that a -// 304 response served through the cache does not corrupt the tunnel, and that a -// later unconditional request still returns the original cached 200 body. +// TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse verifies that once +// a full 200 is cached, both conditional and unconditional requests for the same +// resource are served that cached body over a healthy reused tunnel, without +// extra upstream round-trips. The conditional request collapses onto the same +// cache key (see ignoreHeaders) rather than producing a replayed 304. func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { const ( etag = `"v1"` @@ -265,6 +267,7 @@ func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { return resp } + // Unconditional request populates the cache with the full 200 body. resp := request("") responseBody, err := io.ReadAll(resp.Body) require.NoError(t, err) @@ -272,13 +275,16 @@ func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, body, string(responseBody)) + // Conditional request collapses onto the same key and is served the cached + // full body instead of a 304, with no upstream round-trip. resp = request(etag) responseBody, err = io.ReadAll(resp.Body) require.NoError(t, err) require.NoError(t, resp.Body.Close()) - assert.Equal(t, http.StatusNotModified, resp.StatusCode) - assert.Empty(t, responseBody) + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, body, string(responseBody)) + // A later unconditional request is likewise served from cache. resp = request("") responseBody, err = io.ReadAll(resp.Body) require.NoError(t, err) @@ -287,10 +293,117 @@ func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { assert.Equal(t, body, string(responseBody)) assert.Equal(t, etag, resp.Header.Get("ETag")) - assert.Equal(t, int32(2), upstreamRequests.Load()) + // Only the first request reached upstream; the reused tunnel stayed healthy. + assert.Equal(t, int32(1), upstreamRequests.Load()) + assert.Equal(t, int32(1), proxyDials.Load()) +} + +// TestProxyHTTPSConditionalNotModifiedIsNotCached verifies that a 304 Not +// Modified is never stored in the cache. Replaying a cached 304 would let a +// stale "Not Modified" mask upstream package updates from the dependency +// updater, so every conditional request must reach upstream. It also confirms +// the passthrough 304 keeps the reused MITM tunnel healthy across requests. +func TestProxyHTTPSConditionalNotModifiedIsNotCached(t *testing.T) { + const etag = `"v1"` + var conditionalRequests atomic.Int32 + upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", etag) + if r.Header.Get("If-None-Match") == etag { + conditionalRequests.Add(1) + w.WriteHeader(http.StatusNotModified) + return + } + _, err := io.WriteString(w, "body") + assert.NoError(t, err) + })) + defer upstream.Close() + + t.Setenv("PROXY_CACHE", "true") + client, proxy := testProxyServer(t, testProxyConfig, nil, upstream.Certificate()) + defer proxy.Close() + transport := client.Transport.(*http.Transport) + var proxyDials atomic.Int32 + dialer := &net.Dialer{} + transport.DialContext = func(ctx context.Context, network, address string) (net.Conn, error) { + proxyDials.Add(1) + return dialer.DialContext(ctx, network, address) + } + + for i := 0; i < 3; i++ { + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, upstream.URL, nil) + require.NoError(t, err) + req.Header.Set("If-None-Match", etag) + resp, err := client.Do(req) + require.NoError(t, err) + responseBody, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusNotModified, resp.StatusCode) + assert.Empty(t, responseBody) + } + + // Every conditional request must reach upstream: a cached 304 is never + // replayed. A single dial proves the reused tunnel stayed in sync. + assert.Equal(t, int32(3), conditionalRequests.Load()) assert.Equal(t, int32(1), proxyDials.Load()) } +// TestProxyHTTPSConditionalRequestServedFromCachedBody verifies the performance +// path: once an unconditional GET has cached a full 200, a later *conditional* +// request (If-None-Match) for the same resource is served that cached body +// instead of going upstream. This is safe because we only ever serve a full +// response we already fetched — the client never receives a stale 304, and we +// never force a re-download of a body it already has. +func TestProxyHTTPSConditionalRequestServedFromCachedBody(t *testing.T) { + const ( + etag = `"v1"` + body = "cached response" + ) + var upstreamRequests atomic.Int32 + upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamRequests.Add(1) + w.Header().Set("ETag", etag) + if r.Header.Get("If-None-Match") == etag { + w.WriteHeader(http.StatusNotModified) + return + } + _, err := io.WriteString(w, body) + assert.NoError(t, err) + })) + defer upstream.Close() + + t.Setenv("PROXY_CACHE", "true") + client, proxy := testProxyServer(t, testProxyConfig, nil, upstream.Certificate()) + defer proxy.Close() + + // First request is unconditional and populates the cache with the full 200. + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, upstream.URL, nil) + require.NoError(t, err) + resp, err := client.Do(req) + require.NoError(t, err) + responseBody, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, body, string(responseBody)) + + // A later conditional request collapses onto the same cache key and is + // served the cached full 200 body without another upstream round-trip. + condReq, err := http.NewRequestWithContext(t.Context(), http.MethodGet, upstream.URL, nil) + require.NoError(t, err) + condReq.Header.Set("If-None-Match", etag) + condResp, err := client.Do(condReq) + require.NoError(t, err) + condBody, err := io.ReadAll(condResp.Body) + require.NoError(t, err) + require.NoError(t, condResp.Body.Close()) + assert.Equal(t, http.StatusOK, condResp.StatusCode) + assert.Equal(t, body, string(condBody)) + + // Only the first (cache-populating) request reached upstream. + assert.Equal(t, int32(1), upstreamRequests.Load()) +} + // TestProxyUpstreamCloseIsNotCachedAsBodylessResponse verifies that an upstream // connection failure (goproxy returns no response, synthesizes a 500) is logged // distinctly and NOT cached as if it were a valid bodyless response, so a later From 04b9c6f767bdf0ce7d2d36b6e0eaea7225b73592 Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Fri, 14 Aug 2026 13:24:34 -0500 Subject: [PATCH 2/2] Address review: keep conditional validators in cache key; filter persisted bodyless entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revert the header-collapsing change (adding If-None-Match/If-Modified-Since to ignoreHeaders): collapsing a conditional request onto the unconditional key could serve a cached representation older than the one the client already holds, bypassing revalidation and masking upstream updates. Conditional validators now stay in the key, so a conditional request is never short-circuited to a stale cached 200. Also apply cacheableStatus when loading db.yaml so conditional/no-content entries (1xx/204/205/304) persisted by an older proxy are dropped on load and never replayed after upgrade — cacheableStatus previously only gated new writes. Update tests accordingly and add coverage for the persisted-entry filter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f21dba67-58ce-4aa3-a3ac-f33fcb5d086d --- internal/cache/handlers.go | 17 ++++--- internal/cache/handlers_test.go | 37 +++++++++++++++ proxy_test.go | 81 ++++++--------------------------- 3 files changed, 58 insertions(+), 77 deletions(-) diff --git a/internal/cache/handlers.go b/internal/cache/handlers.go index c73603c..d352ddc 100644 --- a/internal/cache/handlers.go +++ b/internal/cache/handlers.go @@ -62,15 +62,6 @@ var ignoreHeaders = map[string]struct{}{ "X-Pub-Os": {}, "X-Pub-Reason": {}, "X-Pub-Session-Id": {}, - // Conditional-request validators: collapse a conditional request onto the - // same key as its unconditional twin so it can be served from an already - // cached full 200 body instead of going upstream. We never cache a 304 - // (see cacheableStatus), so this key only ever holds a full response; a - // conditional request therefore either hits that body or passes through - // unchanged. Excluding them from the hash never forces a re-download of a - // body the client already has. - "If-None-Match": {}, - "If-Modified-Since": {}, } // generates the key used in the DB, includes a hash of the body @@ -159,6 +150,14 @@ func New(enabled bool, cacheDir string) (*DB, error) { return db, nil } for i := range in { + if in[i].Entry != nil && !cacheableStatus(in[i].Entry.Status) { + // Skip conditional / no-content entries (1xx/204/205/304) persisted + // by an older proxy. cacheableStatus only gates new writes, so + // without this filter such entries would still be loaded here and + // replayed by OnRequest after upgrade, reintroducing the stale-304 + // problem this change is meant to fix. + continue + } db.cacheDB[in[i].Key] = in[i].Entry } // prevent successive runs from overwriting previous cache entries diff --git a/internal/cache/handlers_test.go b/internal/cache/handlers_test.go index f75e890..f8c510c 100644 --- a/internal/cache/handlers_test.go +++ b/internal/cache/handlers_test.go @@ -18,6 +18,7 @@ import ( "github.com/elazarl/goproxy" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) // None of these tests should make network calls @@ -413,6 +414,42 @@ func TestCache_MissingCacheFileIsNotCountedAsHit(t *testing.T) { assert.False(t, WasResponseCached(hitCtx), "request must not be tagged as cached when the file is missing") } +// TestCache_PersistedNonCacheableEntriesAreNotLoaded verifies that conditional / +// no-content entries (1xx/204/205/304) persisted in a db.yaml by an older proxy +// are dropped on load, so they are never served by OnRequest after upgrade. +func TestCache_PersistedNonCacheableEntriesAreNotLoaded(t *testing.T) { + cacheDir := filepath.Join(os.TempDir(), strconv.Itoa(time.Now().Nanosecond())) + require.NoError(t, os.MkdirAll(cacheDir, 0o750)) + defer os.RemoveAll(cacheDir) + + // A backing file for the cacheable (200) entry. + bodyFile := filepath.Join(cacheDir, "000001-body") + require.NoError(t, os.WriteFile(bodyFile, []byte("hello"), 0o600)) + + persisted := []Out{ + {Key: Key{Method: http.MethodGet, URL: "https://example.test/ok"}, + Entry: &Entry{Status: http.StatusOK, FilePath: bodyFile}}, + {Key: Key{Method: http.MethodGet, URL: "https://example.test/not-modified"}, + Entry: &Entry{Status: http.StatusNotModified}}, + {Key: Key{Method: http.MethodGet, URL: "https://example.test/no-content"}, + Entry: &Entry{Status: http.StatusNoContent}}, + } + data, err := yaml.Marshal(persisted) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(cacheDir, "db.yaml"), data, 0o600)) + + cacher, err := New(true, cacheDir) + require.NoError(t, err) + + assert.Len(t, cacher.cacheDB, 1, "only the cacheable 200 entry should be loaded") + _, ok := cacher.cacheDB[Key{Method: http.MethodGet, URL: "https://example.test/ok"}] + assert.True(t, ok, "the 200 entry must be loaded") + _, ok = cacher.cacheDB[Key{Method: http.MethodGet, URL: "https://example.test/not-modified"}] + assert.False(t, ok, "a persisted 304 entry must not be loaded") + _, ok = cacher.cacheDB[Key{Method: http.MethodGet, URL: "https://example.test/no-content"}] + assert.False(t, ok, "a persisted 204 entry must not be loaded") +} + func Test_sanitize(t *testing.T) { var tests = []struct { Input, Expected string diff --git a/proxy_test.go b/proxy_test.go index 22a1575..63690fa 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -222,11 +222,12 @@ func TestProxyHTTPSMITMHeadCacheHitPreservesContentLength(t *testing.T) { require.Equal(t, int32(1), proxyDials.Load(), "both requests must reuse one tunnel") } -// TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse verifies that once -// a full 200 is cached, both conditional and unconditional requests for the same -// resource are served that cached body over a healthy reused tunnel, without -// extra upstream round-trips. The conditional request collapses onto the same -// cache key (see ignoreHeaders) rather than producing a replayed 304. +// TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse verifies that a +// 304 response passed through the cache does not corrupt the reused tunnel, and +// that a later unconditional request still returns the original cached 200 body. +// Conditional validators stay in the cache key, so a conditional request is +// never short-circuited to a cached 200 (which could serve content older than +// the client already holds). func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { const ( etag = `"v1"` @@ -275,16 +276,17 @@ func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, body, string(responseBody)) - // Conditional request collapses onto the same key and is served the cached - // full body instead of a 304, with no upstream round-trip. + // Conditional request keeps its own key, misses the cache, and reaches + // upstream, which returns a bodyless 304 (never cached, correctly framed). resp = request(etag) responseBody, err = io.ReadAll(resp.Body) require.NoError(t, err) require.NoError(t, resp.Body.Close()) - assert.Equal(t, http.StatusOK, resp.StatusCode) - assert.Equal(t, body, string(responseBody)) + assert.Equal(t, http.StatusNotModified, resp.StatusCode) + assert.Empty(t, responseBody) - // A later unconditional request is likewise served from cache. + // The unconditional entry is untouched: a later unconditional request is + // still served the original cached 200 body over the same healthy tunnel. resp = request("") responseBody, err = io.ReadAll(resp.Body) require.NoError(t, err) @@ -293,8 +295,7 @@ func TestProxyHTTPSConditionalNotModifiedPreservesCachedResponse(t *testing.T) { assert.Equal(t, body, string(responseBody)) assert.Equal(t, etag, resp.Header.Get("ETag")) - // Only the first request reached upstream; the reused tunnel stayed healthy. - assert.Equal(t, int32(1), upstreamRequests.Load()) + assert.Equal(t, int32(2), upstreamRequests.Load()) assert.Equal(t, int32(1), proxyDials.Load()) } @@ -348,62 +349,6 @@ func TestProxyHTTPSConditionalNotModifiedIsNotCached(t *testing.T) { assert.Equal(t, int32(1), proxyDials.Load()) } -// TestProxyHTTPSConditionalRequestServedFromCachedBody verifies the performance -// path: once an unconditional GET has cached a full 200, a later *conditional* -// request (If-None-Match) for the same resource is served that cached body -// instead of going upstream. This is safe because we only ever serve a full -// response we already fetched — the client never receives a stale 304, and we -// never force a re-download of a body it already has. -func TestProxyHTTPSConditionalRequestServedFromCachedBody(t *testing.T) { - const ( - etag = `"v1"` - body = "cached response" - ) - var upstreamRequests atomic.Int32 - upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - upstreamRequests.Add(1) - w.Header().Set("ETag", etag) - if r.Header.Get("If-None-Match") == etag { - w.WriteHeader(http.StatusNotModified) - return - } - _, err := io.WriteString(w, body) - assert.NoError(t, err) - })) - defer upstream.Close() - - t.Setenv("PROXY_CACHE", "true") - client, proxy := testProxyServer(t, testProxyConfig, nil, upstream.Certificate()) - defer proxy.Close() - - // First request is unconditional and populates the cache with the full 200. - req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, upstream.URL, nil) - require.NoError(t, err) - resp, err := client.Do(req) - require.NoError(t, err) - responseBody, err := io.ReadAll(resp.Body) - require.NoError(t, err) - require.NoError(t, resp.Body.Close()) - assert.Equal(t, http.StatusOK, resp.StatusCode) - assert.Equal(t, body, string(responseBody)) - - // A later conditional request collapses onto the same cache key and is - // served the cached full 200 body without another upstream round-trip. - condReq, err := http.NewRequestWithContext(t.Context(), http.MethodGet, upstream.URL, nil) - require.NoError(t, err) - condReq.Header.Set("If-None-Match", etag) - condResp, err := client.Do(condReq) - require.NoError(t, err) - condBody, err := io.ReadAll(condResp.Body) - require.NoError(t, err) - require.NoError(t, condResp.Body.Close()) - assert.Equal(t, http.StatusOK, condResp.StatusCode) - assert.Equal(t, body, string(condBody)) - - // Only the first (cache-populating) request reached upstream. - assert.Equal(t, int32(1), upstreamRequests.Load()) -} - // TestProxyUpstreamCloseIsNotCachedAsBodylessResponse verifies that an upstream // connection failure (goproxy returns no response, synthesizes a 500) is logged // distinctly and NOT cached as if it were a valid bodyless response, so a later