Repository: superfly/flyctl (go)

Issue:
Test flake: testRetryBackoffContextTimeout
*Please only report specific issues with `flyctl` behavior*. Anything like a support request for your application should go to https://community.fly.io. More people watch that space and can help you faster!

**Describe the bug**
Briefly, describe what broke and provide the following details:

When upgrading flyctl to v0.3.50 via the Arch AUR package, the test run failed with the following:

```
--- FAIL: TestRetry (0.35s)
    --- FAIL: TestRetry/testRetryBackoffContextTimeout (0.20s)
        retry_test.go:135:
                Error Trace:    github.com/superfly/flyctl/retry/retry_test.go:135
                Error:          "199.99232ms" is not greater than or equal to "200ms"
                Test:           TestRetry/testRetryBackoffContextTimeout
                Messages:       elapsed time should be at least the timeout duration
        retry_test.go:137: RetryBackoff - Attempts made: 3, Elapsed time: 199.99232ms
FAIL
FAIL    github.com/superfly/flyctl/retry        0.355s
```

The package is running the tests using the following:
`go test ./... -ldflags="-X 'github.com/superfly/flyctl/internal/buildinfo.buildDate=2024-12-16T18:44:07Z'" --run=`

Looks like this might be a flaky test given the comparison failed by just under 8ms? From a quick look, the `context.WithTimeout` is starting a 200ms timer before the startTime is captured. Then it is expected that the time delta between startTime and the `time.Since` is  going to be longer than the WithTimeout but there is an arbitrary amount of time that could pass between the WithTimeout and the call to startTime. There might be other possible situations which could result in the test failure for this code. This is a racy assertion.

* Operating system

Arch linux
```
$ uname -a
Linux name 6.12.4-arch1-1 #1 SMP PREEMPT_DYNAMIC Mon, 09 Dec 2024 14:31:57 +0000 x86_64 GNU/Linux
$ go version
go version go1.23.4 linux/amd64
```

Pull request:
refactor: remove flyctl/retry
testRetryBackoffContextTimeout has been flaky.

Instead of fixing, this change removes our own retry package. We don't have to write our own.

Fixes #4124

### Change Summary

What and Why:

How:

Related to:

---

### Documentation

- [ ] Fresh Produce
- [ ] In superfly/docs, or asked for help from docs team
- [ ] n/a


Patch:
diff --git a/go.mod b/go.mod
index de0bc4bcd..17329537b 100644
--- a/go.mod
+++ b/go.mod
@@ -21,7 +21,7 @@ require (
 	github.com/briandowns/spinner v1.23.1
 	github.com/buildpacks/pack v0.36.1
 	github.com/cavaliergopher/grab/v3 v3.0.1
-	github.com/cenkalti/backoff v2.2.1+incompatible
+	github.com/cenkalti/backoff/v5 v5.0.0
 	github.com/chzyer/readline v1.5.1
 	github.com/cli/safeexec v1.0.1
 	github.com/coder/websocket v1.8.12
diff --git a/go.sum b/go.sum
index c69c3d49d..64e6ebf59 100644
--- a/go.sum
+++ b/go.sum
@@ -166,10 +166,10 @@ github.com/buildpacks/pack v0.36.1/go.mod h1:oM4TCRUir43xU8bwiapa6VPHs29HuznNxPg
 github.com/bwesterb/go-ristretto v1.2.3/go.mod h1:fUIoIZaG73pV5biE2Blr2xEzDoMj7NFEuV9ekS419A0=
 github.com/cavaliergopher/grab/v3 v3.0.1 h1:4z7TkBfmPjmLAAmkkAZNX/6QJ1nNFdv3SdIHXju0Fr4=
 github.com/cavaliergopher/grab/v3 v3.0.1/go.mod h1:1U/KNnD+Ft6JJiYoYBAimKH2XrYptb8Kl3DFGmsjpq4=
-github.com/cenkalti/backoff v2.2.1+incompatible h1:tNowT99t7UNflLxfYYSlKYsBpXdEet03Pg2g16Swow4=
-github.com/cenkalti/backoff v2.2.1+incompatible/go.mod h1:90ReRw6GdpyfrHakVjL/QHaoyV4aDUVVkXQJJJ3NXXM=
 github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8=
 github.com/cenkalti/backoff/v4 v4.3.0/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE=
+github.com/cenkalti/backoff/v5 v5.0.0 h1:4ziwFuaVJicDO1ah1Nz1aXXV1caM28PFgf1V5TTFXew=
+github.com/cenkalti/backoff/v5 v5.0.0/go.mod h1:rkhZdG3JZukswDf7f0cwqPNk4K0sa+F97BxZthm/crw=
 github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
 github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/chrismellard/docker-credential-acr-env v0.0.0-20230304212654-82a0ddb27589 h1:krfRl01rzPzxSxyLyrChD+U+MzsBXbm0OwYYB67uF+4=
diff --git a/internal/build/imgsrc/depot.go b/internal/build/imgsrc/depot.go
index 23ecb077f..9b2362b9c 100644
--- a/internal/build/imgsrc/depot.go
+++ b/internal/build/imgsrc/depot.go
@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"time"
 
+	"github.com/cenkalti/backoff/v5"
 	depotbuild "github.com/depot/depot-go/build"
 	depotmachine "github.com/depot/depot-go/machine"
 	"github.com/moby/buildkit/client"
@@ -25,7 +26,6 @@ import (
 	"github.com/superfly/flyctl/internal/render"
 	"github.com/superfly/flyctl/internal/tracing"
 	"github.com/superfly/flyctl/iostreams"
-	"github.com/superfly/flyctl/retry"
 	"github.com/superfly/flyctl/terminal"
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/trace"
@@ -208,24 +208,22 @@ func initBuilder(ctx context.Context, buildState *build, appName string, streams
 	}
 
 	// Set the buildErr to any error that represents the build failing.
-	var buildErr error
 	var finalBuildErr error
 
 	span.AddEvent("Acquiring Depot machine")
-	var buildkit *depotmachine.Machine
 
 	timeoutCtx, cancel := context.WithTimeout(ctx, 25*time.Second)
 	defer cancel()
 
-	finalBuildErr = retry.Retry(timeoutCtx, func() error {
-		buildkit, buildErr = depotmachine.Acquire(ctx, build.ID, build.Token, "amd64")
-		if buildErr != nil {
-			span.RecordError(buildErr)
-			return buildErr
+	buildkit, finalBuildErr := backoff.Retry(timeoutCtx, func() (*depotmachine.Machine, error) {
+		machine, err := depotmachine.Acquire(ctx, build.ID, build.Token, "amd64")
+		if err != nil {
+			span.RecordError(err)
+			return nil, err
 		}
 
-		return nil
-	}, 2)
+		return machine, nil
+	}, backoff.WithMaxTries(2))
 
 	if finalBuildErr != nil {
 		streams.StopProgressIndicator()
diff --git a/internal/build/imgsrc/resolver.go b/internal/build/imgsrc/resolver.go
index 052661569..ec30f82ec 100644
--- a/internal/build/imgsrc/resolver.go
+++ b/internal/build/imgsrc/resolver.go
@@ -12,6 +12,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/cenkalti/backoff/v5"
 	"github.com/pkg/errors"
 	"github.com/vektah/gqlparser/v2/gqlerror"
 	"go.opentelemetry.io/otel/attribute"
@@ -27,7 +28,6 @@ import (
 	"github.com/superfly/flyctl/internal/sentry"
 	"github.com/superfly/flyctl/internal/tracing"
 	"github.com/superfly/flyctl/iostreams"
-	"github.com/superfly/flyctl/retry"
 
 	"github.com/superfly/flyctl/terminal"
 )
@@ -675,9 +675,9 @@ func (r *Resolver) StartHeartbeat(ctx context.Context) (*StopSignal, error) {
 	terminal.Debugf("Sending remote builder heartbeat pulse to %s...\n", heartbeatUrl)
 
 	span.AddEvent("sending first heartbeat")
-	err = retry.Retry(ctx, func() error {
-		return r.heartbeatFn(ctx, dockerClient, heartbeatReq)
-	}, 3)
+	_, err = backoff.Retry(ctx, func() (any, error) {
+		return nil, r.heartbeatFn(ctx, dockerClient, heartbeatReq)
+	}, backoff.WithMaxTries(3))
 	if err != nil {
 		var h *httpError
 		if errors.As(err, &h) {
diff --git a/internal/command/deploy/machines_deploymachinesapp.go b/internal/command/deploy/machines_deploymachinesapp.go
index dc994c538..561fc52ed 100644
--- a/internal/command/deploy/machines_deploymachinesapp.go
+++ b/internal/command/deploy/machines_deploymachinesapp.go
@@ -12,7 +12,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/cenkalti/backoff"
+	"github.com/cenkalti/backoff/v5"
 	"github.com/miekg/dns"
 	"github.com/samber/lo"
 	"github.com/sourcegraph/conc/pool"
@@ -1367,9 +1367,8 @@ func (md *machineDeployment) checkDNS(ctx context.Context) error {
 		b := backoff.NewExponentialBackOff()
 		b.InitialInterval = 1 * time.Second
 		b.MaxInterval = 5 * time.Second
-		b.MaxElapsedTime = 60 * time.Second
 
-		return backoff.Retry(func() error {
+		_, err = backoff.Retry(ctx, func() (any, error) {
 			m := new(dns.Msg)
 
 			var numIPv4, numIPv6 int
@@ -1389,11 +1388,11 @@ func (md *machineDeployment) checkDNS(ctx context.Context) error {
 			answerv4, _, err := c.Exchange(m, "8.8.8.8:53")
 			if err != nil {
 				tracing.RecordError(span, err, "failed to exchange v4")
-				return err
+				return nil, err
 			} else if len(answerv4.Answer) != numIPv4 {
 				span.SetAttributes(attribute.String("v4_answer", answerv4.String()))
 				tracing.RecordError(span, errors.New("v4 response count mismatch"), "v4 response count mismatch")
-				return fmt.Errorf("expected %d A records for %s, got %d", numIPv4, fqdn, len(answerv4.Answer))
+				return nil, fmt.Errorf("expected %d A records for %s, got %d", numIPv4, fqdn, len(answerv4.Answer))
 			}
 
 			m.SetQuestion(fqdn, dns.TypeAAAA)
@@ -1401,16 +1400,16 @@ func (md *machineDeployment) checkDNS(ctx context.Context) error {
 			answerv6, _, err := c.Exchange(m, "8.8.8.8:53")
 			if err != nil {
 				tracing.RecordError(span, err, "failed to exchange v4")
-				return err
+				return nil, err
 			} else if len(answerv6.Answer) != numIPv6 {
 				span.SetAttributes(attribute.String("v6_answer", answerv6.String()))
 				tracing.RecordError(span, errors.New("v6 response count mismatch"), "v6 response count mismatch")
-				return fmt.Errorf("expected %d AAAA records for %s, got %d", numIPv6, fqdn, len(answerv6.Answer))
+				return nil, fmt.Errorf("expected %d AAAA records for %s, got %d", numIPv6, fqdn, len(answerv6.Answer))
 			}
 
-			return nil
-		}, backoff.WithContext(b, ctx))
-
+			return nil, nil
+		}, backoff.WithBackOff(b), backoff.WithMaxElapsedTime(60*time.Second))
+		return err
 	} else {
 		return nil
 	}
diff --git a/retry/retry.go b/retry/retry.go
deleted file mode 100644
index 1b89a106c..000000000
--- a/retry/retry.go
+++ /dev/null
@@ -1,49 +0,0 @@
-package retry
-
-import (
-	"context"
-	"time"
-
-	"github.com/jpillora/backoff"
-)
-
-// Retry attempts to execute the provided function up to 'attempts' times,
-// respecting the context for cancellation and timeout
-func Retry(ctx context.Context, fn func() error, attempts uint) (err error) {
-	for i := attempts; i > 0; i-- {
-
-		if ctx.Err() != nil {
-			return ctx.Err()
-		}
-
-		err = fn()
-		if err == nil {
-			return nil
-		}
-	}
-
-	return err
-}
-
-// Retry attempts to execute the provided function up to 'attempts' times with an
-// exponential backoff strategy, respecting the context for cancellation and timeout
-func RetryBackoff(ctx context.Context, fn func() error, attempts uint, backoffStrategy *backoff.Backoff) (err error) {
-	for i := attempts; i > 0; i-- {
-		if ctx.Err() != nil {
-			return ctx.Err()
-		}
-
-		err = fn()
-		if err == nil {
-			return nil
-		}
-
-		select {
-		case <-time.After(backoffStrategy.Duration()):
-		case <-ctx.Done():
-			return ctx.Err()
-		}
-	}
-
-	return err
-}


Test patch:
diff --git a/internal/command/deploy/machinebasedtest.go b/internal/command/deploy/machinebasedtest.go
index 533c176fd..889bb61b5 100644
--- a/internal/command/deploy/machinebasedtest.go
+++ b/internal/command/deploy/machinebasedtest.go
@@ -5,7 +5,7 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/cenkalti/backoff"
+	"github.com/cenkalti/backoff/v5"
 	"github.com/samber/lo"
 	fly "github.com/superfly/fly-go"
 	"github.com/superfly/flyctl/internal/appconfig"
@@ -153,19 +153,19 @@ func (md *machineDeployment) waitForLogs(ctx context.Context, mach *fly.Machine,
 	b := backoff.NewExponentialBackOff()
 	b.InitialInterval = 1 * time.Second
 	b.MaxInterval = 10 * time.Second
-	b.MaxElapsedTime = timeout
 
-	return backoff.Retry(func() error {
+	_, err := backoff.Retry(ctx, func() ([]fly.LogEntry, error) {
 		logs, _, err := md.apiClient.GetAppLogs(ctx, md.app.Name, "", md.appConfig.PrimaryRegion, mach.ID)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		if len(logs) == 0 {
-			return fmt.Errorf(ErrNoLogsFound)
+			return nil, fmt.Errorf(ErrNoLogsFound)
 		}
 
-		return nil
-	}, backoff.WithContext(b, ctx))
+		return logs, nil
+	}, backoff.WithBackOff(b), backoff.WithMaxElapsedTime(timeout))
+	return err
 }
 
 func (md *machineDeployment) createTestMachine(ctx context.Context, svc *appconfig.ServiceMachineCheck, machineToTest *fly.Machine, sl statuslogger.StatusLine) (*fly.Machine, error) {
diff --git a/retry/retry_test.go b/retry/retry_test.go
deleted file mode 100644
index c81af7918..000000000
--- a/retry/retry_test.go
+++ /dev/null
@@ -1,138 +0,0 @@
-package retry
-
-import (
-	"context"
-	"errors"
-	"testing"
-	"time"
-
-	"github.com/jpillora/backoff"
-	"github.com/stretchr/testify/assert"
-)
-
-var errFail = errors.New("fail")
-
-func TestRetry(t *testing.T) {
-	t.Parallel()
-	t.Run("testSuccess", testSuccess)
-	t.Run("testFail1", testFail1)
-	t.Run("testFail2", testFail2)
-	t.Run("testFailAll", testFailAll)
-	t.Run("testContextTimeout", testContextTimeout)           // Added test
-	t.Run("testRetryBackoffContextTimeout", testRetryBackoff) // Test for RetryBackoff
-}
-
-func testSuccess(t *testing.T) {
-	var count int
-
-	fn := func() error {
-		count++
-		return nil
-	}
-
-	err := Retry(context.Background(), fn, 3)
-	assert.NoError(t, err)
-	assert.Equal(t, 1, count)
-}
-
-func testFail1(t *testing.T) {
-	var count int
-
-	fn := func() error {
-		count++
-		if count == 1 {
-			return errors.New("1")
-		}
-		return nil
-	}
-
-	err := Retry(context.Background(), fn, 3)
-	assert.NoError(t, err)
-	assert.Equal(t, 2, count)
-}
-
-func testFail2(t *testing.T) {
-	var count int
-
-	fn := func() error {
-		count++
-		if count <= 2 {
-			return errFail
-		}
-		return nil
-	}
-
-	err := Retry(context.Background(), fn, 3)
-	assert.NoError(t, err)
-	assert.Equal(t, 3, count)
-}
-
-func testFailAll(t *testing.T) {
-	var count int
-
-	fn := func() error {
-		count++
-		return errFail
-	}
-
-	err := Retry(context.Background(), fn, 3)
-	assert.ErrorIs(t, err, errFail)
-	assert.Equal(t, 3, count)
-}
-
-func testContextTimeout(t *testing.T) {
-	var count int
-
-	fn := func() error {
-		count++
-		time.Sleep(50 * time.Millisecond)
-		return errFail
-	}
-
-	timeoutDuration := 100 * time.Millisecond
-	ctx, cancel := context.WithTimeout(context.Background(), timeoutDuration)
-	defer cancel()
-
-	startTime := time.Now()
-
-	err := Retry(ctx, fn, 10)
-	elapsed := time.Since(startTime)
-
-	assert.ErrorIs(t, err, context.DeadlineExceeded)
-	assert.LessOrEqual(t, count, 3)
-	assert.GreaterOrEqual(t, elapsed, timeoutDuration)
-}
-
-func testRetryBackoff(t *testing.T) {
-	var count int
-
-	fn := func() error {
-		count++
-		time.Sleep(50 * time.Millisecond)
-		return errFail
-	}
-
-	timeoutDuration := 200 * time.Millisecond
-	ctx, cancel := context.WithTimeout(context.Background(), timeoutDuration)
-	defer cancel()
-
-	b := &backoff.Backoff{
-		Min:    10 * time.Millisecond,
-		Max:    50 * time.Millisecond,
-		Factor: 2,
-	}
-
-	startTime := time.Now()
-
-	err := RetryBackoff(ctx, fn, 10, b)
-
-	elapsed := time.Since(startTime)
-
-	assert.ErrorIs(t, err, context.DeadlineExceeded, "expected context deadline exceeded error")
-
-	assert.LessOrEqual(t, count, 4, "count should not exceed the number of attempts before timeout")
-
-	assert.GreaterOrEqual(t, elapsed, timeoutDuration, "elapsed time should be at least the timeout duration")
-
-	t.Logf("RetryBackoff - Attempts made: %d, Elapsed time: %v", count, elapsed)
-}