Retry should be limited by the max backoff if no maximum number of tries is specified (#2102)

* fix retry bug, use default retry for invocation db update

* add tests, implement floor for max retries

* table-based tests

Author: tempoz

server/backends/invocationdb/invocationdb.go

@@ -111,15 +111,14 @@ func (d *InvocationDB) CreateInvocation(ctx context.Context, ti *tables.Invocati
 func (d *InvocationDB) UpdateInvocation(ctx context.Context, ti *tables.Invocation) (bool, error) {
 	updated := false
 	var err error
-	retryOptions := &retry.Options{InitialBackoff: time.Second, Multiplier: 2.0, MaxRetries: 5}
-	for i, r := 1, retry.New(ctx, retryOptions); r.Next(); i++ {
+	for r := retry.DefaultWithContext(ctx); r.Next(); {
 		err = d.h.TransactionWithOptions(ctx, db.Opts().WithQueryName("update_invocation"), func(tx *db.DB) error {
 			result := tx.Where("`invocation_id` = ? AND `attempt` = ?", ti.InvocationID, ti.Attempt).Updates(ti)
 			updated = result.RowsAffected > 0
 			return result.Error
 		})
 		if d.h.IsDeadlockError(err) {
-			log.Warningf("Encountered deadlock when attempting to update invocation table for invocation %s, attempt %d of %d", ti.InvocationID, i, retryOptions.MaxRetries)
+			log.Warningf("Encountered deadlock when attempting to update invocation table for invocation %s, attempt %d of %d", ti.InvocationID, r.AttemptNumber(), r.MaxAttempts())
 			continue
 		}
 		break

server/util/retry/BUILD

@@ -1,8 +1,17 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "retry",
     srcs = ["retry.go"],
     importpath = "github.com/buildbuddy-io/buildbuddy/server/util/retry",
     visibility = ["//visibility:public"],
 )
+
+go_test(
+    name = "retry_test",
+    srcs = ["retry_test.go"],
+    deps = [
+        ":retry",
+        "@com_github_stretchr_testify//assert",
+    ],
+)

server/util/retry/retry.go

@@ -12,21 +12,36 @@ type Options struct {
 	InitialBackoff time.Duration // How long to wait after the first request
 	MaxBackoff     time.Duration // Max amount of time to wait for a single request
 	Multiplier     float64       // Next backoff is this * previous backoff
-	MaxRetries     int           // Max number of retries; 0 is inf
+	MaxRetries     int           // Max number of retries; 0 is based on time
 }
 
 type Retry struct {
 	opts *Options
 	ctx  context.Context
 
 	currentAttempt int
+	maxAttempts    int
 	isReset        bool
 }
 
 func New(ctx context.Context, opts *Options) *Retry {
+	maxAttempts := opts.MaxRetries
+	if maxAttempts <= 0 {
+		// always try at least once
+		maxAttempts = 1
+		if opts.Multiplier > 1 && opts.MaxBackoff > opts.InitialBackoff {
+			maxAttempts = 1 + int(math.Ceil(
+				math.Log(
+					float64(opts.MaxBackoff)/float64(opts.InitialBackoff),
+				)/math.Log(opts.Multiplier),
+			))
+		}
+	}
+
 	r := &Retry{
-		ctx:  ctx,
-		opts: opts,
+		ctx:         ctx,
+		opts:        opts,
+		maxAttempts: maxAttempts,
 	}
 	r.Reset()
 	return r
@@ -40,17 +55,15 @@ func New(ctx context.Context, opts *Options) *Retry {
 //   doSomething()
 // }
 func DefaultWithContext(ctx context.Context) *Retry {
-	r := &Retry{
-		ctx: ctx,
-		opts: &Options{
+	return New(
+		ctx,
+		&Options{
 			InitialBackoff: 50 * time.Millisecond,
 			MaxBackoff:     3 * time.Second,
 			Multiplier:     2,
 			MaxRetries:     0, // unlimited, time based max by default.
 		},
-	}
-	r.Reset()
-	return r
+	)
 }
 
 func (r *Retry) Reset() {
@@ -74,7 +87,7 @@ func (r *Retry) Next() bool {
 	}
 
 	// If we're out of retries, exit.
-	if r.opts.MaxRetries > 0 && r.currentAttempt == r.opts.MaxRetries {
+	if r.currentAttempt >= r.maxAttempts {
 		return false
 	}
 
@@ -86,3 +99,11 @@ func (r *Retry) Next() bool {
 		return false
 	}
 }
+
+func (r *Retry) AttemptNumber() int {
+	return r.currentAttempt + 1
+}
+
+func (r *Retry) MaxAttempts() int {
+	return r.maxAttempts
+}

server/util/retry/retry_test.go

@@ -0,0 +1,85 @@
+package retry_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/buildbuddy-io/buildbuddy/server/util/retry"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestMaxRetryFromBackoff(t *testing.T) {
+	tests := map[string]struct {
+		inputOptions        *retry.Options
+		expectedMaxAttempts int
+	}{
+		"exact backoff boundary": {
+			inputOptions: &retry.Options{
+				InitialBackoff: 3,
+				MaxBackoff:     24,
+				Multiplier:     2,
+				MaxRetries:     0,
+			},
+			expectedMaxAttempts: 4,
+		},
+		"above exact backoff boundary": {
+			inputOptions: &retry.Options{
+				InitialBackoff: 3,
+				MaxBackoff:     25,
+				Multiplier:     2,
+				MaxRetries:     0,
+			},
+			expectedMaxAttempts: 5,
+		},
+		"below exact backoff boundary": {
+			inputOptions: &retry.Options{
+				InitialBackoff: 3,
+				MaxBackoff:     23,
+				Multiplier:     2,
+				MaxRetries:     0,
+			},
+			expectedMaxAttempts: 4,
+		},
+		"manually set maximum above calculated backoff": {
+			inputOptions: &retry.Options{
+				InitialBackoff: 3,
+				MaxBackoff:     23,
+				Multiplier:     2,
+				MaxRetries:     7,
+			},
+			expectedMaxAttempts: 7,
+		},
+		"manually set maximum below calculated backoff": {
+			inputOptions: &retry.Options{
+				InitialBackoff: 3,
+				MaxBackoff:     23,
+				Multiplier:     2,
+				MaxRetries:     2,
+			},
+			expectedMaxAttempts: 2,
+		},
+		"initial backoff greater than maximum": {
+			inputOptions: &retry.Options{
+				InitialBackoff: 3,
+				MaxBackoff:     1,
+				Multiplier:     2,
+				MaxRetries:     0,
+			},
+			expectedMaxAttempts: 1,
+		},
+		"multiplier less than or equal to one": {
+			inputOptions: &retry.Options{
+				InitialBackoff: 3,
+				MaxBackoff:     7,
+				Multiplier:     0.5,
+				MaxRetries:     0,
+			},
+			expectedMaxAttempts: 1,
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			assert.Equal(t, tc.expectedMaxAttempts, retry.New(context.Background(), tc.inputOptions).MaxAttempts())
+		})
+	}
+}