Merge pull request #7258 from astokes-docker/source-http-retry

source/http: retry transient request failures

source/http/retry_test.go

@@ -0,0 +1,72 @@
+package http
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestDoRequestRetriesDroppedConnection covers a connection dropped before any
+// response is written, which reaches the client as EOF. A solve with many HTTP
+// sources hits this regularly on a lossy network, and before the retry a
+// single occurrence failed the whole build.
+func TestDoRequestRetriesDroppedConnection(t *testing.T) {
+	t.Parallel()
+
+	var attempts atomic.Int32
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if attempts.Add(1) <= 2 {
+			hj, ok := w.(http.Hijacker)
+			if !ok {
+				return
+			}
+			conn, _, err := hj.Hijack()
+			if err != nil {
+				return
+			}
+			conn.Close()
+			return
+		}
+		w.Write([]byte("content"))
+	}))
+	defer server.Close()
+
+	ctx := t.Context()
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, server.URL, nil)
+	require.NoError(t, err)
+
+	resp, err := doRequest(ctx, server.Client(), req)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+	require.Equal(t, int32(3), attempts.Load())
+}
+
+// TestDoRequestDoesNotRetryStatusErrors asserts the retry does not change how
+// HTTP status codes are handled: the response is returned to the caller on the
+// first attempt, exactly as before.
+func TestDoRequestDoesNotRetryStatusErrors(t *testing.T) {
+	t.Parallel()
+
+	var attempts atomic.Int32
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts.Add(1)
+		w.WriteHeader(http.StatusNotFound)
+	}))
+	defer server.Close()
+
+	ctx := t.Context()
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, server.URL, nil)
+	require.NoError(t, err)
+
+	resp, err := doRequest(ctx, server.Client(), req)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+
+	require.Equal(t, http.StatusNotFound, resp.StatusCode)
+	require.Equal(t, int32(1), attempts.Load())
+}

source/http/source.go

@@ -31,6 +31,7 @@ import (
 	"github.com/moby/buildkit/util/bklog"
 	"github.com/moby/buildkit/util/cachedigest"
 	"github.com/moby/buildkit/util/pgpsign"
+	"github.com/moby/buildkit/util/resolver/retryhandler"
 	"github.com/moby/buildkit/util/tracing"
 	"github.com/moby/buildkit/util/urlutil"
 	"github.com/moby/buildkit/version"
@@ -215,6 +216,24 @@ func (hs *httpSourceHandler) client(g session.Group) *http.Client {
 	return &http.Client{Transport: newTransport(hs.transport, hs.sm, g)}
 }
 
+// doRequest performs req, retrying transient network failures with
+// exponential backoff.
+//
+// A solve can contain thousands of independent HTTP sources, each fetched over
+// its own connection and with no connection reuse between them. Without a
+// retry, one dropped connection anywhere fails the entire solve. Registry
+// fetches already retry this class of error through retryhandler; HTTP sources
+// did not.
+//
+// Only the request is retried. A failure part way through reading the response
+// body is not covered here, and neither is a non-2xx response: those are left
+// to the caller so that status handling is unchanged.
+func doRequest(ctx context.Context, client *http.Client, req *http.Request) (*http.Response, error) {
+	return retryhandler.WithRetry(ctx, nil, func(ctx context.Context) (*http.Response, error) {
+		return client.Do(req.Clone(ctx))
+	})
+}
+
 // urlHash is internal hash the etag is stored by that doesn't leak outside
 // this package.
 func (hs *httpSourceHandler) urlHash() (digest.Digest, error) {
@@ -408,6 +427,7 @@ func (hs *httpSourceHandler) resolveMetadataRef(ctx context.Context, jobCtx solv
 		// we need to add accept-encoding header manually because stdlib only adds it to GET requests
 		// some servers will return different etags if Accept-Encoding header is different
 		req.Header.Set("Accept-Encoding", "gzip")
+		// Best-effort: on any error we fall through to the GET below, which is retried.
 		resp, err := client.Do(req)
 		if err == nil {
 			if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusNotModified {
@@ -454,7 +474,7 @@ func (hs *httpSourceHandler) resolveMetadataRef(ctx context.Context, jobCtx solv
 		req.Header.Del("Accept-Encoding")
 	}
 
-	resp, err := client.Do(req)
+	resp, err := doRequest(ctx, client, req)
 	if err != nil {
 		return nil, err
 	}
@@ -889,7 +909,7 @@ func (hs *httpSourceHandler) Snapshot(ctx context.Context, jobCtx solver.JobCont
 
 	client := hs.client(g)
 
-	resp, err := client.Do(req)
+	resp, err := doRequest(ctx, client, req)
 	if err != nil {
 		return nil, err
 	}