From 1dc50f6338ebe98334a870b2469143394375026e Mon Sep 17 00:00:00 2001 From: Inhyuk Andy Cho Date: Tue, 20 Jan 2026 18:46:28 +0900 Subject: [PATCH 1/4] fix: return error immediately on roundtrip error --- internal/validationresponsehandler.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/internal/validationresponsehandler.go b/internal/validationresponsehandler.go index 428c370..a39234f 100644 --- a/internal/validationresponsehandler.go +++ b/internal/validationresponsehandler.go @@ -77,7 +77,12 @@ func (r *validationResponseHandler) HandleValidationResponse( resp *http.Response, err error, ) (*http.Response, error) { - if err == nil && req.Method == http.MethodGet && resp.StatusCode == http.StatusNotModified { + + if err != nil { + return nil, err + } + + if req.Method == http.MethodGet && resp.StatusCode == http.StatusNotModified { // RFC 9111 §4.3.3 Handling Validation Responses (304 Not Modified) // RFC 9111 §4.3.4 Freshening Stored Responses upon Validation mergeResponseHeaders(ctx.Stored.Data, resp.Header) @@ -99,7 +104,7 @@ func (r *validationResponseHandler) HandleValidationResponse( ccResp CCResponseDirectives ccRespOnce bool ) - if (err != nil || isStaleErrorAllowed(resp.StatusCode)) && req.Method == http.MethodGet { + if isStaleErrorAllowed(resp.StatusCode) && req.Method == http.MethodGet { ccResp = ParseCCResponseDirectives(resp.Header) ccRespOnce = true if r.siep.CanStaleOnError(ctx.Freshness, ccResp) { @@ -112,10 +117,6 @@ func (r *validationResponseHandler) HandleValidationResponse( } } - if err != nil { - return nil, err - } - if !ccRespOnce { ccResp = ParseCCResponseDirectives(resp.Header) } From f517dc6d5b18baa7735b7ef70118c9d7be4140a5 Mon Sep 17 00:00:00 2001 From: Inhyuk Andy Cho Date: Tue, 20 Jan 2026 19:58:19 +0900 Subject: [PATCH 2/4] refactor: no more error to validation handler --- internal/validationresponsehandler.go | 7 ------- roundtripper.go | 7 +++++-- roundtripper_test.go | 26 +++++++++++++------------- 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/internal/validationresponsehandler.go b/internal/validationresponsehandler.go index a39234f..04227ee 100644 --- a/internal/validationresponsehandler.go +++ b/internal/validationresponsehandler.go @@ -24,7 +24,6 @@ type ValidationResponseHandler interface { ctx RevalidationContext, req *http.Request, resp *http.Response, - err error, ) (*http.Response, error) } @@ -75,13 +74,7 @@ func (r *validationResponseHandler) HandleValidationResponse( ctx RevalidationContext, req *http.Request, resp *http.Response, - err error, ) (*http.Response, error) { - - if err != nil { - return nil, err - } - if req.Method == http.MethodGet && resp.StatusCode == http.StatusNotModified { // RFC 9111 §4.3.3 Handling Validation Responses (304 Not Modified) // RFC 9111 §4.3.4 Freshening Stored Responses upon Validation diff --git a/roundtripper.go b/roundtripper.go index f2d9ab1..dba6579 100644 --- a/roundtripper.go +++ b/roundtripper.go @@ -340,6 +340,9 @@ func (r *transport) handleCacheHit( revalidate: req = withConditionalHeaders(req, stored.Data.Header) resp, start, end, err := r.roundTripTimed(req) + if err != nil { + return nil, err + } ctx := internal.RevalidationContext{ URLKey: urlKey, Start: start, @@ -350,7 +353,7 @@ revalidate: RefIndex: refIndex, Freshness: freshness, } - return r.vrh.HandleValidationResponse(ctx, req, resp, err) + return r.vrh.HandleValidationResponse(ctx, req, resp) } func (r *transport) serveFromCache( @@ -441,7 +444,7 @@ func (r *transport) backgroundRevalidate( Freshness: freshness, } //nolint:bodyclose // The response is not used, so we don't need to close it. - _, err = r.vrh.HandleValidationResponse(revalCtx, req, resp, nil) + _, err = r.vrh.HandleValidationResponse(revalCtx, req, resp) errc <- err }() diff --git a/roundtripper_test.go b/roundtripper_test.go index 06d3820..1756dff 100644 --- a/roundtripper_test.go +++ b/roundtripper_test.go @@ -65,8 +65,8 @@ func mockTransport(fields func(rt *transport)) *transport { ci: &internal.MockCacheInvalidator{}, rs: &internal.MockResponseStorer{}, vrh: &internal.MockValidationResponseHandler{ - HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response, err error) (*http.Response, error) { - return resp, err + HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response) (*http.Response, error) { + return resp, nil }, }, clock: &internal.MockClock{NowResult: time.Now()}, @@ -225,9 +225,9 @@ func Test_transport_CacheHit_MustRevalidate_Stale(t *testing.T) { }, } rt.vrh = &internal.MockValidationResponseHandler{ - HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response, err error) (*http.Response, error) { + HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response) (*http.Response, error) { mockVHCalled = true - return resp, err + return resp, nil }, } }) @@ -260,9 +260,9 @@ func Test_transport_CacheHit_NoCacheUnqualified(t *testing.T) { }, } rt.vrh = &internal.MockValidationResponseHandler{ - HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response, err error) (*http.Response, error) { + HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response) (*http.Response, error) { mockVHCalled = true - return resp, err + return resp, nil }, } }) @@ -540,10 +540,10 @@ func Test_transport_RevalidationPath(t *testing.T) { }, } rt.vrh = &internal.MockValidationResponseHandler{ - HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response, err error) (*http.Response, error) { + HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response) (*http.Response, error) { mockVHCalled = true internal.CacheStatusRevalidated.ApplyTo(resp.Header) - return resp, err + return resp, nil }, } }) @@ -597,9 +597,9 @@ func Test_transport_SWR_NormalPath(t *testing.T) { rt.clock = &internal.MockClock{NowResult: base.Add(5 * time.Second), SinceResult: 0} rt.siep = &internal.MockStaleIfErrorPolicy{} rt.vrh = &internal.MockValidationResponseHandler{ - HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response, err error) (*http.Response, error) { + HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response) (*http.Response, error) { revalidateCalled <- struct{}{} // Signal that revalidation was called - return resp, err + return resp, nil }, } rt.swrTimeout = DefaultSWRTimeout @@ -670,7 +670,7 @@ func Test_transport_SWR_NormalPathAndError(t *testing.T) { rt.clock = &internal.MockClock{NowResult: base.Add(5 * time.Second), SinceResult: 0} rt.swrTimeout = swrTimeout rt.vrh = &internal.MockValidationResponseHandler{ - HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response, err error) (*http.Response, error) { + HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response) (*http.Response, error) { defer func() { revalidateCalled <- struct{}{} }() // Signal that revalidation was called return nil, errors.New("revalidation error") }, @@ -737,9 +737,9 @@ func Test_transport_SWR_Timeout(t *testing.T) { rt.clock = &internal.MockClock{NowResult: base.Add(5 * time.Second), SinceResult: 0} rt.swrTimeout = swrTimeout rt.vrh = &internal.MockValidationResponseHandler{ - HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response, err error) (*http.Response, error) { + HandleValidationResponseFunc: func(ctx internal.RevalidationContext, req *http.Request, resp *http.Response) (*http.Response, error) { revalidateCalled <- struct{}{} // Signal that revalidation was called - return resp, err + return resp, nil }, } rt.upstream = &internal.MockRoundTripper{ From 1135d1085f47ffe667e9902649917de68e92ce0b Mon Sep 17 00:00:00 2001 From: Inhyuk Andy Cho Date: Tue, 20 Jan 2026 19:59:11 +0900 Subject: [PATCH 3/4] test: interface changed --- internal/mocks.go | 5 ++- internal/validationresponsehandler_test.go | 42 +++++----------------- 2 files changed, 11 insertions(+), 36 deletions(-) diff --git a/internal/mocks.go b/internal/mocks.go index 1b8b984..90512fb 100644 --- a/internal/mocks.go +++ b/internal/mocks.go @@ -183,16 +183,15 @@ func (m *MockCacheInvalidator) InvalidateCache( var _ ValidationResponseHandler = (*MockValidationResponseHandler)(nil) type MockValidationResponseHandler struct { - HandleValidationResponseFunc func(ctx RevalidationContext, req *http.Request, resp *http.Response, err error) (*http.Response, error) + HandleValidationResponseFunc func(ctx RevalidationContext, req *http.Request, resp *http.Response) (*http.Response, error) } func (m *MockValidationResponseHandler) HandleValidationResponse( ctx RevalidationContext, req *http.Request, resp *http.Response, - err error, ) (*http.Response, error) { - return m.HandleValidationResponseFunc(ctx, req, resp, err) + return m.HandleValidationResponseFunc(ctx, req, resp) } var _ VaryMatcher = (*MockVaryMatcher)(nil) diff --git a/internal/validationresponsehandler_test.go b/internal/validationresponsehandler_test.go index 9c6aafd..7189242 100644 --- a/internal/validationresponsehandler_test.go +++ b/internal/validationresponsehandler_test.go @@ -15,7 +15,6 @@ package internal import ( - "errors" "log/slog" "net/http" "net/url" @@ -48,9 +47,8 @@ func Test_validationResponseHandler_HandleValidationResponse(t *testing.T) { } type args struct { - req *http.Request - resp *http.Response - inputErr error + req *http.Request + resp *http.Response } tests := []struct { @@ -107,31 +105,7 @@ func Test_validationResponseHandler_HandleValidationResponse(t *testing.T) { }, }, { - name: "GET with error, stale allowed", - handler: &validationResponseHandler{ - l: noopLogger, - siep: &MockStaleIfErrorPolicy{ - CanStaleOnErrorFunc: func(*Freshness, ...StaleIfErrorer) bool { return true }, - }, - clock: &MockClock{NowResult: base}, - }, - setup: func(tt *testing.T, handler *validationResponseHandler) args { - return args{ - req: &http.Request{Method: http.MethodGet}, - resp: &http.Response{ - StatusCode: http.StatusInternalServerError, - Header: http.Header{"Cache-Control": {"stale-if-error=60"}}, - }, - inputErr: errors.New("network error"), - } - }, - assert: func(tt *testing.T, got *http.Response, err error) { - testutil.RequireNoError(tt, err) - testutil.AssertEqual(tt, http.StatusOK, got.StatusCode) - }, - }, - { - name: "GET with error, stale not allowed", + name: "GET with error status, stale not allowed", handler: &validationResponseHandler{ l: noopLogger, siep: &MockStaleIfErrorPolicy{ @@ -140,18 +114,20 @@ func Test_validationResponseHandler_HandleValidationResponse(t *testing.T) { clock: &MockClock{NowResult: base}, }, setup: func(tt *testing.T, handler *validationResponseHandler) args { + handler.ce = CacheabilityEvaluatorFunc( + func(*http.Response, CCRequestDirectives, CCResponseDirectives) bool { return false }, + ) return args{ req: &http.Request{Method: http.MethodGet}, resp: &http.Response{ StatusCode: http.StatusInternalServerError, Header: http.Header{}, }, - inputErr: errors.New("network error"), } }, assert: func(tt *testing.T, got *http.Response, err error) { - testutil.RequireError(tt, err) - testutil.AssertNil(tt, got) + testutil.RequireNoError(tt, err) + testutil.AssertEqual(tt, "BYPASS", got.Header.Get(CacheStatusHeader)) }, }, { @@ -210,7 +186,7 @@ func Test_validationResponseHandler_HandleValidationResponse(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { a := tt.setup(t, tt.handler) - got, err := tt.handler.HandleValidationResponse(ctx, a.req, a.resp, a.inputErr) + got, err := tt.handler.HandleValidationResponse(ctx, a.req, a.resp) tt.assert(t, got, err) }) } From e890a98cc3424f04058d9c3c3484b9f915b704f2 Mon Sep 17 00:00:00 2001 From: Bart Venter <72999113+bartventer@users.noreply.github.com> Date: Thu, 12 Mar 2026 17:11:48 +0000 Subject: [PATCH 4/4] chore: suppress `cyclop` linting error --- roundtripper.go | 1 + 1 file changed, 1 insertion(+) diff --git a/roundtripper.go b/roundtripper.go index dba6579..a353f0d 100644 --- a/roundtripper.go +++ b/roundtripper.go @@ -288,6 +288,7 @@ func (r *transport) handleCacheMiss( return resp, nil } +//nolint:cyclop // The complexity of this function is justified by the need to handle multiple caching scenarios according to RFC 9111. func (r *transport) handleCacheHit( req *http.Request, stored *internal.Response,