diff --git a/server/services/utils/http.go b/server/services/utils/http.go index 711b665b6..6aad37e6f 100644 --- a/server/services/utils/http.go +++ b/server/services/utils/http.go @@ -21,12 +21,17 @@ import ( "crypto/ed25519" "crypto/tls" "encoding/json" + "errors" "fmt" "io" + "net" "net/http" "net/url" + "strings" "time" + "github.com/cenkalti/backoff/v5" + "github.com/rs/zerolog/log" "github.com/yaronf/httpsign" host_matcher "go.woodpecker-ci.org/woodpecker/v3/server/services/utils/hostmatcher" @@ -89,49 +94,146 @@ func NewHTTPClient(privateKey crypto.PrivateKey, allowedHostList string) (*Clien }, nil } -// Send makes an http request to the given endpoint, writing the input -// to the request body and un-marshaling the output from the response body. +// Send makes an http request with retry logic. func (e *Client) Send(ctx context.Context, method, path string, in, out any) (int, error) { + // Maximum number of retries + const maxRetries = 3 + + log.Debug().Msgf("HTTP request: %s %s, retries enabled (max: %d)", method, path, maxRetries) + + // Prepare request body bytes for possible retries + var bodyBytes []byte + if in != nil { + buf := new(bytes.Buffer) + if err := json.NewEncoder(buf).Encode(in); err != nil { + return 0, err + } + bodyBytes = buf.Bytes() + } + + // Parse URI once uri, err := url.Parse(path) if err != nil { return 0, err } - // if we are posting or putting data, we need to write it to the body of the request. - var buf io.ReadWriter - if in != nil { - buf = new(bytes.Buffer) - jsonErr := json.NewEncoder(buf).Encode(in) - if jsonErr != nil { - return 0, jsonErr + // Create backoff configuration + exponentialBackoff := backoff.NewExponentialBackOff() + + // Execute with backoff retry + return backoff.Retry(ctx, func() (int, error) { + // Check if context is already canceled + if ctx.Err() != nil { + return 0, ctx.Err() } - } - // creates a new http request to the endpoint. - req, err := http.NewRequestWithContext(ctx, method, uri.String(), buf) - if err != nil { - return 0, err - } - if in != nil { - req.Header.Set("Content-Type", "application/json") - } + // Create request body for this attempt + var body io.Reader + if len(bodyBytes) > 0 { + body = bytes.NewReader(bodyBytes) + } - resp, err := e.Do(req) - if err != nil { - return 0, err - } - defer resp.Body.Close() - - if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { - body, err := io.ReadAll(resp.Body) + // Create new request for each attempt + req, err := http.NewRequestWithContext(ctx, method, uri.String(), body) if err != nil { - return resp.StatusCode, err + return 0, err + } + if in != nil { + req.Header.Set("Content-Type", "application/json") } - return resp.StatusCode, fmt.Errorf("response: %s", string(body)) + // Send request + resp, err := e.Do(req) + if err != nil { + // Check if this is a retryable error + if !isRetryableError(err) { + log.Error().Err(err).Msgf("HTTP request failed (not retryable): %s %s", method, path) + return 0, backoff.Permanent(err) + } + return 0, err + } + + statusCode := resp.StatusCode + // Read body immediately to ensure proper resource cleanup for retries + respBody, readErr := io.ReadAll(resp.Body) + resp.Body.Close() + if readErr != nil { + // Check if this is a retryable error + if !isRetryableError(readErr) { + log.Error().Err(readErr).Msgf("HTTP response read failed (not retryable): %s %s", method, path) + return statusCode, backoff.Permanent(readErr) + } + return statusCode, readErr + } + + // Check if status code is retryable + if isRetryableStatusCode(statusCode) { + return statusCode, fmt.Errorf("response: %d", statusCode) + } + + // If status code is client error (4xx), don't retry + if statusCode >= http.StatusBadRequest && statusCode < http.StatusInternalServerError { + log.Debug().Int("status", statusCode).Msgf("HTTP request returned client error (not retryable): %s %s", method, path) + return statusCode, backoff.Permanent(fmt.Errorf("response: %s", string(respBody))) + } + + // If status code is OK (2xx), parse and return response + if statusCode >= http.StatusOK && statusCode < http.StatusMultipleChoices { + if out != nil { + err = json.NewDecoder(bytes.NewReader(respBody)).Decode(out) + // Check for EOF error during response body parsing + if err != nil && (errors.Is(err, io.EOF) || strings.Contains(err.Error(), "unexpected EOF")) { + return statusCode, err + } + if err != nil { + log.Error().Err(err).Msgf("HTTP response parsing failed (not retryable): %s %s", method, path) + return statusCode, backoff.Permanent(err) + } + } + log.Debug().Int("status", statusCode).Msgf("HTTP request succeeded: %s %s", method, path) + return statusCode, nil + } + + // For any other status code, don't retry + log.Error().Int("status", statusCode).Msgf("HTTP request returned unexpected status code (not retryable): %s %s", method, path) + return statusCode, backoff.Permanent(fmt.Errorf("response: %s", string(respBody))) + }, backoff.WithBackOff(exponentialBackoff), backoff.WithMaxTries(maxRetries), + backoff.WithNotify(func(err error, delay time.Duration) { + // Log retry attempts + log.Debug().Err(err).Msgf("HTTP request failed, retrying in %v: %s %s", delay, method, path) + }), + ) +} + +// isRetryableError checks if an error is transient and suitable for retry. +func isRetryableError(err error) bool { + // Check for network-related errors + var netErr net.Error + if errors.As(err, &netErr) { + // Retry on timeout errors + if netErr.Timeout() { + return true + } } - // if no other errors parse and return the json response. - err = json.NewDecoder(resp.Body).Decode(out) - return resp.StatusCode, err + // Check for specific error types + switch { + case errors.Is(err, net.ErrClosed), + errors.Is(err, io.EOF), + errors.Is(err, io.ErrUnexpectedEOF): + return true + } + + // Check for error strings that indicate retryable conditions + errStr := err.Error() + return strings.Contains(errStr, "connection refused") || + strings.Contains(errStr, "connection reset by peer") || + strings.Contains(errStr, "no such host") || + strings.Contains(errStr, "TLS handshake timeout") +} + +// isRetryableStatusCode checks if an HTTP status code is suitable for retry. +func isRetryableStatusCode(statusCode int) bool { + // Retry on server errors (5xx) + return statusCode >= http.StatusInternalServerError && statusCode < http.StatusNetworkAuthenticationRequired } diff --git a/server/services/utils/http_test.go b/server/services/utils/http_test.go index dc6cd3d91..a6603a6fd 100644 --- a/server/services/utils/http_test.go +++ b/server/services/utils/http_test.go @@ -24,6 +24,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/yaronf/httpsign" "go.woodpecker-ci.org/woodpecker/v3/server/services/utils" @@ -72,3 +73,35 @@ func TestSignClient(t *testing.T) { assert.Equal(t, http.StatusOK, rr.StatusCode) } + +func TestRetry(t *testing.T) { + _, privEd25519Key, err := ed25519.GenerateKey(rand.Reader) + require.NoError(t, err) + + numRetry := 0 + body := []byte("{\"foo\":\"bar\"}") + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + numRetry++ + if numRetry >= 6 { + w.WriteHeader(http.StatusNoContent) + } else { + w.WriteHeader(http.StatusInternalServerError) + } + })) + + client, err := utils.NewHTTPClient(privEd25519Key, "loopback") + require.NoError(t, err) + + // first time: retry fails all the times + _, err = client.Send(t.Context(), http.MethodGet, server.URL+"/", bytes.NewBuffer(body), nil) + assert.Error(t, err) + assert.Equal(t, 3, numRetry) + + // second time: retry succeeds after two failed times + rr, err := client.Send(t.Context(), http.MethodGet, server.URL+"/", bytes.NewBuffer(body), nil) + assert.NoError(t, err) + + assert.Equal(t, http.StatusNoContent, rr) + assert.Equal(t, 6, numRetry) +}