diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index 2331a3a8a..0f7c71aa9 100644 --- a/internal/codespaces/api/api.go +++ b/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)) } diff --git a/internal/codespaces/api/api_test.go b/internal/codespaces/api/api_test.go index 51bb04637..e8fab74f1 100644 --- a/internal/codespaces/api/api_test.go +++ b/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() diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 5c85ae616..13b27c3da 100644 --- a/internal/codespaces/codespaces.go +++ b/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") } } diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index ca10bafea..d8f58638e 100644 --- a/pkg/cmd/pr/create/create.go +++ b/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() diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 939deffe7..38add7425 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/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) { diff --git a/pkg/cmd/release/shared/upload_test.go b/pkg/cmd/release/shared/upload_test.go index f2b623312..26bed11c0 100644 --- a/pkg/cmd/release/shared/upload_test.go +++ b/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 diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 8334cf0e5..087d7219e 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/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 }