Standardize retry mechanism (#7027)

internal/codespaces/api/api.go

@@ -40,6 +40,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/cenkalti/backoff/v4"
 	"github.com/cli/cli/v2/api"
 	"github.com/opentracing/opentracing-go"
 )
@@ -1082,16 +1083,16 @@ func (a *API) setHeaders(req *http.Request) {
 
 // withRetry takes a generic function that sends an http request and retries
 // only when the returned response has a >=500 status code.
-func (a *API) withRetry(f func() (*http.Response, error)) (resp *http.Response, err error) {
-	for i := 0; i < 5; i++ {
-		resp, err = f()
+func (a *API) withRetry(f func() (*http.Response, error)) (*http.Response, error) {
+	bo := backoff.NewConstantBackOff(a.retryBackoff)
+	return backoff.RetryWithData(func() (*http.Response, error) {
+		resp, err := f()
 		if err != nil {
-			return nil, err
+			return nil, backoff.Permanent(err)
 		}
 		if resp.StatusCode < 500 {
-			break
+			return resp, nil
 		}
-		time.Sleep(a.retryBackoff * (time.Duration(i) + 1))
-	}
-	return resp, err
+		return nil, errors.New("retry")
+	}, backoff.WithMaxRetries(bo, 3))
 }

internal/codespaces/api/api_test.go

@@ -185,8 +185,9 @@ func TestCreateCodespaces_Pending(t *testing.T) {
 	defer svr.Close()
 
 	api := API{
-		githubAPI: svr.URL,
-		client:    &http.Client{},
+		githubAPI:    svr.URL,
+		client:       &http.Client{},
+		retryBackoff: 0,
 	}
 
 	ctx := context.TODO()

internal/codespaces/codespaces.go

@@ -37,7 +37,7 @@ type logger interface {
 
 // ConnectToLiveshare waits for a Codespace to become running,
 // and connects to it using a Live Share session.
-func ConnectToLiveshare(ctx context.Context, progress progressIndicator, sessionLogger logger, apiClient apiClient, codespace *api.Codespace) (sess *liveshare.Session, err error) {
+func ConnectToLiveshare(ctx context.Context, progress progressIndicator, sessionLogger logger, apiClient apiClient, codespace *api.Codespace) (*liveshare.Session, error) {
 	if codespace.State != api.CodespaceStateAvailable {
 		progress.StartProgressIndicatorWithLabel("Starting codespace")
 		defer progress.StopProgressIndicator()
@@ -45,25 +45,33 @@ func ConnectToLiveshare(ctx context.Context, progress progressIndicator, session
 			return nil, fmt.Errorf("error starting codespace: %w", err)
 		}
 	}
-	expBackoff := backoff.NewExponentialBackOff()
 
-	expBackoff.Multiplier = 1.1
-	expBackoff.MaxInterval = 10 * time.Second
-	expBackoff.MaxElapsedTime = 5 * time.Minute
+	if !connectionReady(codespace) {
+		expBackoff := backoff.NewExponentialBackOff()
+		expBackoff.Multiplier = 1.1
+		expBackoff.MaxInterval = 10 * time.Second
+		expBackoff.MaxElapsedTime = 5 * time.Minute
 
-	for retries := 0; !connectionReady(codespace); retries++ {
-		if retries > 1 {
-			duration := expBackoff.NextBackOff()
-			time.Sleep(duration)
-		}
+		err := backoff.Retry(func() error {
+			var err error
+			codespace, err = apiClient.GetCodespace(ctx, codespace.Name, true)
+			if err != nil {
+				return backoff.Permanent(fmt.Errorf("error getting codespace: %w", err))
+			}
 
-		if expBackoff.GetElapsedTime() >= expBackoff.MaxElapsedTime {
-			return nil, errors.New("timed out while waiting for the codespace to start")
-		}
+			if connectionReady(codespace) {
+				return nil
+			}
 
-		codespace, err = apiClient.GetCodespace(ctx, codespace.Name, true)
+			return errors.New("codespace not ready yet")
+		}, backoff.WithContext(expBackoff, ctx))
 		if err != nil {
-			return nil, fmt.Errorf("error getting codespace: %w", err)
+			var permErr *backoff.PermanentError
+			if errors.As(err, &permErr) {
+				return nil, err
+			}
+
+			return nil, errors.New("timed out while waiting for the codespace to start")
 		}
 	}
 

pkg/cmd/pr/create/create.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/MakeNowJust/heredoc"
+	"github.com/cenkalti/backoff/v4"
 	"github.com/cli/cli/v2/api"
 	ghContext "github.com/cli/cli/v2/context"
 	"github.com/cli/cli/v2/git"
@@ -741,27 +742,23 @@ func handlePush(opts CreateOptions, ctx CreateContext) error {
 	// automatically push the branch if it hasn't been pushed anywhere yet
 	if ctx.IsPushEnabled {
 		pushBranch := func() error {
-			pushTries := 0
-			maxPushTries := 3
-			for {
-				w := NewRegexpWriter(opts.IO.ErrOut, gitPushRegexp, "")
-				defer w.Flush()
-				gitClient := ctx.GitClient
-				ref := fmt.Sprintf("HEAD:%s", ctx.HeadBranch)
-				if err := gitClient.Push(context.Background(), headRemote.Name, ref, git.WithStderr(w)); err != nil {
-					if didForkRepo && pushTries < maxPushTries {
-						pushTries++
-						// first wait 2 seconds after forking, then 4s, then 6s
-						waitSeconds := 2 * pushTries
-						fmt.Fprintf(opts.IO.ErrOut, "waiting %s before retrying...\n", text.Pluralize(waitSeconds, "second"))
-						time.Sleep(time.Duration(waitSeconds) * time.Second)
-						continue
+			w := NewRegexpWriter(opts.IO.ErrOut, gitPushRegexp, "")
+			defer w.Flush()
+			gitClient := ctx.GitClient
+			ref := fmt.Sprintf("HEAD:%s", ctx.HeadBranch)
+			bo := backoff.NewConstantBackOff(2 * time.Second)
+			ctx := context.Background()
+			return backoff.Retry(func() error {
+				if err := gitClient.Push(ctx, headRemote.Name, ref, git.WithStderr(w)); err != nil {
+					// Only retry if we have forked the repo else the push should succeed the first time.
+					if didForkRepo {
+						fmt.Fprintf(opts.IO.ErrOut, "waiting 2 seconds before retrying...\n")
+						return err
 					}
-					return err
+					return backoff.Permanent(err)
 				}
-				break
-			}
-			return nil
+				return nil
+			}, backoff.WithContext(backoff.WithMaxRetries(bo, 3), ctx))
 		}
 
 		err := pushBranch()

pkg/cmd/release/shared/upload.go

@@ -13,6 +13,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/cenkalti/backoff/v4"
 	"github.com/cli/cli/v2/api"
 	"golang.org/x/sync/errgroup"
 )
@@ -117,10 +118,6 @@ func ConcurrentUpload(httpClient httpDoer, uploadURL string, numWorkers int, ass
 	return g.Wait()
 }
 
-var retryInterval = time.Millisecond * 200
-
-const maxRetries = 3
-
 func shouldRetry(err error) bool {
 	var networkError errNetwork
 	if errors.As(err, &networkError) {
@@ -130,26 +127,23 @@ func shouldRetry(err error) bool {
 	return errors.As(err, &httpError) && httpError.StatusCode >= 500
 }
 
+// Allow injecting backoff interval in tests.
+var retryInterval = time.Millisecond * 200
+
 func uploadWithDelete(ctx context.Context, httpClient httpDoer, uploadURL string, a AssetForUpload) error {
 	if a.ExistingURL != "" {
 		if err := deleteAsset(ctx, httpClient, a.ExistingURL); err != nil {
 			return err
 		}
 	}
-
-	retries := 0
-	for {
+	bo := backoff.NewConstantBackOff(retryInterval)
+	return backoff.Retry(func() error {
 		_, err := uploadAsset(ctx, httpClient, uploadURL, a)
-		if err == nil || retries == maxRetries || !shouldRetry(err) {
+		if err == nil || shouldRetry(err) {
 			return err
 		}
-		retries++
-		select {
-		case <-ctx.Done():
-			return ctx.Err()
-		case <-time.After(time.Duration(retries) * retryInterval):
-		}
-	}
+		return backoff.Permanent(err)
+	}, backoff.WithContext(backoff.WithMaxRetries(bo, 3), ctx))
 }
 
 func uploadAsset(ctx context.Context, httpClient httpDoer, uploadURL string, asset AssetForUpload) (*ReleaseAsset, error) {

pkg/cmd/release/shared/upload_test.go

@@ -7,7 +7,6 @@ import (
 	"io"
 	"net/http"
 	"testing"
-	"time"
 )
 
 func Test_typeForFilename(t *testing.T) {
@@ -77,7 +76,7 @@ func Test_typeForFilename(t *testing.T) {
 }
 
 func Test_uploadWithDelete_retry(t *testing.T) {
-	retryInterval = time.Millisecond
+	retryInterval = 0
 	ctx := context.Background()
 
 	tries := 0

pkg/cmd/repo/fork/fork.go

@@ -326,7 +326,7 @@ func forkRun(opts *ForkOptions) error {
 		if cloneDesired {
 			// Allow injecting alternative BackOff in tests.
 			if opts.BackOff == nil {
-				bo := backoff.NewConstantBackOff(3 * time.Second)
+				bo := backoff.NewConstantBackOff(2 * time.Second)
 				opts.BackOff = bo
 			}