diff --git a/source/http/retry_test.go b/source/http/retry_test.go new file mode 100644 index 000000000..fba0c4c79 --- /dev/null +++ b/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()) +} diff --git a/source/http/source.go b/source/http/source.go index 858805358..2813b69bc 100644 --- a/source/http/source.go +++ b/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 }