From 6fd8f99b7e4a9002357af78e482600ded2d6d10d Mon Sep 17 00:00:00 2001
From: Lukasz Raczylo <lukasz@raczylo.com>
Date: Thu, 13 Aug 2026 00:58:27 +0100
Subject: [PATCH] fix(proxy): stop retrying non-retryable 4xx responses

doProxyRequestWithTimeout returns an error for every non-200 status and
executeProxyAttempt returned it as a plain, retryable error, so 4xx
client errors (400/404/405/...) were retried with exponential backoff
and the 4xx branch of isRetryableStatusCode was dead code.

Additionally, retry-go's finite-attempt loop only consults the custom
RetryIf, which overrode the default IsRecoverable gate, so retry.
Unrecoverable errors (non-retryable 4xx AND timeouts) were still
retried. Honor IsRecoverable in RetryIf so these never retry.

Now 5xx/429 are retried, other 4xx and timeouts fail fast.

Adds Test_proxyRetryPolicyByStatusCode locking in the policy.
---
 proxy.go      | 19 ++++++++++++++---
 proxy_test.go | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/proxy.go b/proxy.go
index a639f05..ea5d5e1 100644
--- a/proxy.go
+++ b/proxy.go
@@ -510,10 +510,17 @@ func executeProxyAttempt(c fiber.Ctx, proxyURL string) error {
 			return retry.Unrecoverable(proxyErr)
 		}
 
-		// Check if this is a retryable HTTP error (e.g., 503)
-		// These indicate the server responded but with an error status
+		// The server responded with an HTTP status code. Decide retry policy
+		// from the status: 5xx and 429 are retryable; other 4xx client
+		// errors fail fast instead of burning backoff retries.
+		// doProxyRequestWithTimeout already copied the status into c.Response().
 		if strings.Contains(proxyErr.Error(), "non-200 response") {
-			// Track as a failure for retryable HTTP errors
+			statusCode := c.Response().StatusCode()
+			shouldRetry, _ := isRetryableStatusCode(statusCode)
+			if !shouldRetry {
+				return retry.Unrecoverable(proxyErr)
+			}
+			// Track as a failure for retryable HTTP errors (5xx, 429)
 			if poolMgr != nil {
 				poolMgr.RecordConnectionFailure()
 			}
@@ -600,6 +607,12 @@ func performProxyRequestWithEnhancedRetries(c fiber.Ctx, proxyURL string, backen
 		}),
 		retry.LastErrorOnly(true),
 		retry.RetryIf(func(err error) bool {
+			// Unrecoverable errors (non-retryable 4xx, timeouts) must not be
+			// retried. retry-go's finite-attempt loop only consults RetryIf,
+			// so without this the default IsRecoverable gate is lost.
+			if !retry.IsRecoverable(err) {
+				return false
+			}
 			// Don't retry if context is cancelled or context is nil
 			if c == nil {
 				return false
diff --git a/proxy_test.go b/proxy_test.go
index 00f854a..b6549c9 100644
--- a/proxy_test.go
+++ b/proxy_test.go
@@ -3,6 +3,7 @@ package main
 import (
 	"net/http"
 	"net/http/httptest"
+	"sync/atomic"
 	"time"
 
 	"github.com/valyala/fasthttp"
@@ -286,3 +287,60 @@ func (suite *Tests) Test_proxyTheRequestWithTimeouts() {
 		})
 	}
 }
+
+// Test_proxyRetryPolicyByStatusCode locks in the HTTP-status retry policy:
+// 4xx client errors must fail fast (no backoff retries), 5xx must be retried.
+func (suite *Tests) Test_proxyRetryPolicyByStatusCode() {
+	originalCoalescing := cfg.RequestCoalescing.Enable
+	originalCB := cfg.CircuitBreaker.Enable
+	cfg.RequestCoalescing.Enable = false
+	cfg.CircuitBreaker.Enable = false
+	defer func() {
+		cfg.RequestCoalescing.Enable = originalCoalescing
+		cfg.CircuitBreaker.Enable = originalCB
+	}()
+
+	doRequest := func(server *httptest.Server) error {
+		cfg.Server.HostGraphQL = server.URL
+		cfg.Client.ClientTimeout = 5
+		cfg.Client.FastProxyClient = createFasthttpClient(cfg)
+
+		reqCtx := &fasthttp.RequestCtx{}
+		reqCtx.Request.SetRequestURI("/graphql")
+		reqCtx.Request.Header.SetMethod("POST")
+		reqCtx.Request.Header.Set("Content-Type", "application/json")
+		reqCtx.Request.SetBody([]byte(`{"query":"query { test }"}`))
+
+		ctx := suite.app.AcquireCtx(reqCtx)
+		err := proxyTheRequest(ctx, cfg.Server.HostGraphQL)
+		suite.app.ReleaseCtx(ctx)
+		return err
+	}
+
+	suite.Run("client_error_4xx_not_retried", func() {
+		var hits atomic.Int32
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			hits.Add(1)
+			w.WriteHeader(http.StatusBadRequest)
+			_, _ = w.Write([]byte(`{"errors":[{"message":"bad request"}]}`))
+		}))
+		defer server.Close()
+
+		err := doRequest(server)
+		suite.Error(err, "4xx should surface an error")
+		suite.Equal(int32(1), hits.Load(), "4xx must be served without retries")
+	})
+
+	suite.Run("server_error_5xx_retried", func() {
+		var hits atomic.Int32
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			hits.Add(1)
+			w.WriteHeader(http.StatusInternalServerError)
+			_, _ = w.Write([]byte(`{"errors":[{"message":"error"}]}`))
+		}))
+		defer server.Close()
+
+		_ = doRequest(server)
+		suite.Greater(hits.Load(), int32(1), "5xx should trigger retries")
+	})
+}