mirror of
https://github.com/lukaszraczylo/graphql-monitoring-proxy.git
synced 2026-10-22 06:38:13 +00:00
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.
This commit is contained in:
@@ -510,10 +510,17 @@ func executeProxyAttempt(c fiber.Ctx, proxyURL string) error {
|
|||||||
return retry.Unrecoverable(proxyErr)
|
return retry.Unrecoverable(proxyErr)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check if this is a retryable HTTP error (e.g., 503)
|
// The server responded with an HTTP status code. Decide retry policy
|
||||||
// These indicate the server responded but with an error status
|
// 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") {
|
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 {
|
if poolMgr != nil {
|
||||||
poolMgr.RecordConnectionFailure()
|
poolMgr.RecordConnectionFailure()
|
||||||
}
|
}
|
||||||
@@ -600,6 +607,12 @@ func performProxyRequestWithEnhancedRetries(c fiber.Ctx, proxyURL string, backen
|
|||||||
}),
|
}),
|
||||||
retry.LastErrorOnly(true),
|
retry.LastErrorOnly(true),
|
||||||
retry.RetryIf(func(err error) bool {
|
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
|
// Don't retry if context is cancelled or context is nil
|
||||||
if c == nil {
|
if c == nil {
|
||||||
return false
|
return false
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ package main
|
|||||||
import (
|
import (
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
|
"sync/atomic"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
"github.com/valyala/fasthttp"
|
"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")
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user