Implement retry logic in HTTP Send method (#5857)

Co-authored-by: qwerty287 <80460567+qwerty287@users.noreply.github.com>
Co-authored-by: qwerty287 <qwerty287@posteo.de>

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
 }

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)
+}