[Internal] Add internal/retrier package (#5746)

## Summary

The repository has several ways to implement simple retry loops. This PR
adds a new `internal/retrier` package that captures the common
denominator across all of them.

This PR already opportunistically migrates `common/retry.go` to use this
new package internally. Subsequent PRs will migrate existing call sites
to it, removing unnecessary middleware.

Design choices:

- **Value-aware predicate.** `Retrier[V].IsRetriable(V, error)` sees
both the polled value and any error, so state-driven polling (the
dominant pattern across the provider's waiters) is supported natively
without sentinel-error wrapping.
- **Factory-per-Run.** `Run` takes a `func() Retrier[V]` and invokes it
once per call. Each invocation gets its own retrier instance with
independent backoff state, making `Run` safe to use from multiple
goroutines with the same factory without locking.
- **Deterministic exponential backoff, no jitter.** The Databricks SDK
already absorbs transient errors, and state-polling waiters here do not
need cross-client decorrelation. The package is intended to sit on top
of the SDK for longer polls (e.g. resource state transitions); jitter
adds complexity without a corresponding benefit in that scenario. This
is not a one-way door; it can be revisited if a use case calls for it.

## Test plan

- [x] `go test ./internal/retrier/...` passes (12 cases across
BackoffPolicy initialization and exponential growth, `Run`
retry/value-aware/ctx-cancellation paths, `RunErr`, `RetryIf` wiring and
factory independence, `RetryIfErr` forwarding, `sleep` cancellation).
- [x] `go tool staticcheck ./internal/retrier/...` clean.
- [x] `make fmt lint ws` clean.
- [x] CI green.

Author: renaudhartert-db

NEXT_CHANGELOG.md

@@ -16,6 +16,5 @@
 
 ### Internal Changes
 
+* Add `internal/retrier` package for unified retry and backoff handling ([#5746](https://github.com/databricks/terraform-provider-databricks/pull/5746)).
 * Pass `excludedAttributes=entitlements` on SCIM `/Me` requests ([#5725](https://github.com/databricks/terraform-provider-databricks/pull/5725)).
-
-  The provider only needs identity fields (`userName`, `id`, `externalId`) from `/Me`, never entitlements. Skipping the entitlement computation avoids an expensive `getEffectivePermissions` traversal on the SCIM backend, which has caused incidents on workspaces with large grant counts.

common/retry.go

@@ -4,40 +4,47 @@ import (
 	"context"
 	"errors"
 	"regexp"
+	"time"
 
 	"github.com/databricks/databricks-sdk-go/apierr"
-	"github.com/databricks/databricks-sdk-go/logger"
-	"github.com/databricks/databricks-sdk-go/retries"
+	"github.com/databricks/terraform-provider-databricks/internal/retrier"
 )
 
-var timeoutRegex = regexp.MustCompile(`request timed out after .* of inactivity`)
-
+// RetryOnTimeout retries f while it returns an SDK inactivity-timeout error.
+//
+// TODO: Deprecate this function in favor of retrier.Run.
 func RetryOnTimeout[T any](ctx context.Context, f func(context.Context) (*T, error)) (*T, error) {
-	r := retries.New[T](retries.WithRetryFunc(func(err error) bool {
-		msg := err.Error()
-		isTimeout := timeoutRegex.MatchString(msg)
-		if isTimeout {
-			logger.Debugf(ctx, "Retrying due to timeout: %s", msg)
-		}
-		return isTimeout
-	}))
-	return r.Run(ctx, func(ctx context.Context) (*T, error) {
-		return f(ctx)
-	})
+	return retrier.Run(ctx, retryOnErr[*T](isTimeoutError), f)
 }
 
-// RetryOn504 returns a [retries.Retrier] that calls the given method
-// until it either succeeds or returns an error that is different from
-// [apierr.ErrDeadlineExceeded].
+// RetryOn504 retries f while it returns an error wrapping [apierr.ErrDeadlineExceeded].
+//
+// TODO: Deprecate this function in favor of retrier.Run.
 func RetryOn504[T any](ctx context.Context, f func(context.Context) (*T, error)) (*T, error) {
-	r := retries.New[T](retries.WithTimeout(-1), retries.WithRetryFunc(func(err error) bool {
-		if !errors.Is(err, apierr.ErrDeadlineExceeded) {
-			return false
-		}
-		logger.Debugf(ctx, "Retrying on error 504")
-		return true
-	}))
-	return r.Run(ctx, func(ctx context.Context) (*T, error) {
-		return f(ctx)
+	return retrier.Run(ctx, retryOnErr[*T](is504Error), f)
+}
+
+// retryOnErr adapts an error-only predicate to a value-aware retrier factory
+// using the transient backoff policy.
+func retryOnErr[V any](isRetriable func(error) bool) func() retrier.Retrier[V] {
+	return retrier.RetryIf(transientBackoff(), func(_ V, err error) bool {
+		return isRetriable(err)
 	})
 }
+
+// transientBackoff returns a fresh backoff tuned for transient errors:
+// start fast, cap quickly.
+func transientBackoff() retrier.BackoffPolicy {
+	return retrier.BackoffPolicy{Initial: time.Second, Maximum: 30 * time.Second}
+}
+
+// TODO: Replace the regex check with type-aware error inspection.
+var timeoutRegex = regexp.MustCompile(`request timed out after .* of inactivity`)
+
+func isTimeoutError(err error) bool {
+	return err != nil && timeoutRegex.MatchString(err.Error())
+}
+
+func is504Error(err error) bool {
+	return errors.Is(err, apierr.ErrDeadlineExceeded)
+}

common/retry_test.go

@@ -3,132 +3,116 @@ package common
 import (
 	"context"
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/databricks/databricks-sdk-go/apierr"
-	"github.com/databricks/databricks-sdk-go/experimental/mocks"
-	"github.com/databricks/databricks-sdk-go/service/workspace"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/mock"
 )
 
-func TestRetryOnTimeout_NoError(t *testing.T) {
-	w := mocks.NewMockWorkspaceClient(t)
-	expected := &workspace.ObjectInfo{}
-	api := w.GetMockWorkspaceAPI().EXPECT()
-	api.GetStatusByPath(mock.Anything, mock.Anything).Return(expected, nil)
-	res, err := RetryOnTimeout(context.Background(), func(ctx context.Context) (*workspace.ObjectInfo, error) {
-		return w.WorkspaceClient.Workspace.GetStatusByPath(ctx, "path")
-	})
-	assert.NoError(t, err)
-	assert.Equal(t, expected, res)
+func TestRetryOnTimeout(t *testing.T) {
+	timeoutErr := errors.New("request failed: request timed out after 1m0s of inactivity")
+	otherErr := errors.New("non-retriable")
+
+	testCases := []struct {
+		name      string
+		callErrs  []error
+		wantErr   error
+		wantCalls int
+	}{
+		{
+			name:      "success on first call",
+			callErrs:  []error{nil},
+			wantCalls: 1,
+		},
+		{
+			name:      "timeout then succeed",
+			callErrs:  []error{timeoutErr, nil},
+			wantCalls: 2,
+		},
+		{
+			name:      "non-timeout halts",
+			callErrs:  []error{otherErr},
+			wantErr:   otherErr,
+			wantCalls: 1,
+		},
+		{
+			name:      "timeout then non-timeout halts",
+			callErrs:  []error{timeoutErr, otherErr},
+			wantErr:   otherErr,
+			wantCalls: 2,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			calls := 0
+			_, err := RetryOnTimeout(context.Background(), func(ctx context.Context) (*struct{}, error) {
+				e := tc.callErrs[calls]
+				calls++
+				return nil, e
+			})
+			if calls != tc.wantCalls {
+				t.Errorf("call count = %d, want %d", calls, tc.wantCalls)
+			}
+			if !errors.Is(err, tc.wantErr) {
+				t.Errorf("err = %v, want %v", err, tc.wantErr)
+			}
+		})
+	}
 }
 
-func TestRetryOnTimeout_OneError(t *testing.T) {
-	w := mocks.NewMockWorkspaceClient(t)
-	expected := &workspace.ObjectInfo{}
-	api := w.GetMockWorkspaceAPI().EXPECT()
-	call1 := api.GetStatusByPath(mock.Anything, mock.Anything).Return(nil, errors.New("request failed: request timed out after 1m0s of inactivity"))
-	call1.Repeatability = 1
-	api.GetStatusByPath(mock.Anything, mock.Anything).Return(expected, nil)
-	res, err := RetryOnTimeout(context.Background(), func(ctx context.Context) (*workspace.ObjectInfo, error) {
-		return w.WorkspaceClient.Workspace.GetStatusByPath(ctx, "path")
-	})
-	assert.NoError(t, err)
-	assert.Equal(t, expected, res)
-}
-
-func TestRetryOnTimeout_NonRetriableError(t *testing.T) {
-	w := mocks.NewMockWorkspaceClient(t)
-	expected := errors.New("request failed: non-retriable error")
-	api := w.GetMockWorkspaceAPI().EXPECT()
-	api.GetStatusByPath(mock.Anything, mock.Anything).Return(nil, expected)
-	_, err := RetryOnTimeout(context.Background(), func(ctx context.Context) (*workspace.ObjectInfo, error) {
-		return w.WorkspaceClient.Workspace.GetStatusByPath(ctx, "path")
-	})
-	assert.ErrorIs(t, err, expected)
-}
-
-func TestRetryOn504_noError(t *testing.T) {
-	wantErr := error(nil)
-	wantRes := (*workspace.ObjectInfo)(nil)
-	wantCalls := 1
-
-	w := mocks.NewMockWorkspaceClient(t)
-	api := w.GetMockWorkspaceAPI().EXPECT()
-	api.GetStatusByPath(mock.Anything, mock.Anything).Return(wantRes, wantErr)
-
-	gotCalls := 0
-	gotRes, gotErr := RetryOn504(context.Background(), func(ctx context.Context) (*workspace.ObjectInfo, error) {
-		gotCalls += 1
-		return w.WorkspaceClient.Workspace.GetStatusByPath(ctx, "path")
-	})
-
-	assert.ErrorIs(t, gotErr, wantErr)
-	assert.Equal(t, gotRes, wantRes)
-	assert.Equal(t, gotCalls, wantCalls)
-}
-
-func TestRetryOn504_errorNot504(t *testing.T) {
-	wantErr := errors.New("test error")
-	wantRes := (*workspace.ObjectInfo)(nil)
-	wantCalls := 1
-
-	w := mocks.NewMockWorkspaceClient(t)
-	api := w.GetMockWorkspaceAPI().EXPECT()
-	api.GetStatusByPath(mock.Anything, mock.Anything).Return(wantRes, wantErr)
-
-	gotCalls := 0
-	gotRes, gotErr := RetryOn504(context.Background(), func(ctx context.Context) (*workspace.ObjectInfo, error) {
-		gotCalls += 1
-		return w.WorkspaceClient.Workspace.GetStatusByPath(ctx, "path")
-	})
-
-	assert.ErrorIs(t, gotErr, wantErr)
-	assert.Equal(t, gotRes, wantRes)
-	assert.Equal(t, gotCalls, wantCalls)
-}
-
-func TestRetryOn504_error504ThenFail(t *testing.T) {
-	wantErr := errors.New("test error")
-	wantRes := (*workspace.ObjectInfo)(nil)
-	wantCalls := 2
-
-	w := mocks.NewMockWorkspaceClient(t)
-	api := w.GetMockWorkspaceAPI().EXPECT()
-	call := api.GetStatusByPath(mock.Anything, mock.Anything).Return(nil, apierr.ErrDeadlineExceeded)
-	call.Repeatability = 1
-	api.GetStatusByPath(mock.Anything, mock.Anything).Return(wantRes, wantErr)
-
-	gotCalls := 0
-	gotRes, gotErr := RetryOn504(context.Background(), func(ctx context.Context) (*workspace.ObjectInfo, error) {
-		gotCalls++
-		return w.WorkspaceClient.Workspace.GetStatusByPath(ctx, "path")
-	})
-
-	assert.ErrorIs(t, gotErr, wantErr)
-	assert.Equal(t, gotRes, wantRes)
-	assert.Equal(t, gotCalls, wantCalls)
-}
-
-func TestRetryOn504_error504ThenSuccess(t *testing.T) {
-	wantErr := error(nil)
-	wantRes := &workspace.ObjectInfo{}
-	wantCalls := 2
-
-	w := mocks.NewMockWorkspaceClient(t)
-	api := w.GetMockWorkspaceAPI().EXPECT()
-	call := api.GetStatusByPath(mock.Anything, mock.Anything).Return(nil, apierr.ErrDeadlineExceeded)
-	call.Repeatability = 1
-	api.GetStatusByPath(mock.Anything, mock.Anything).Return(wantRes, wantErr)
-
-	gotCalls := 0
-	gotRes, gotErr := RetryOn504(context.Background(), func(ctx context.Context) (*workspace.ObjectInfo, error) {
-		gotCalls++
-		return w.WorkspaceClient.Workspace.GetStatusByPath(ctx, "path")
-	})
-
-	assert.ErrorIs(t, gotErr, wantErr)
-	assert.Equal(t, gotRes, wantRes)
-	assert.Equal(t, gotCalls, wantCalls)
+func TestRetryOn504(t *testing.T) {
+	otherErr := errors.New("not 504")
+
+	testCases := []struct {
+		name      string
+		callErrs  []error
+		wantErr   error
+		wantCalls int
+	}{
+		{
+			name:      "success on first call",
+			callErrs:  []error{nil},
+			wantCalls: 1,
+		},
+		{
+			name:      "504 then succeed",
+			callErrs:  []error{apierr.ErrDeadlineExceeded, nil},
+			wantCalls: 2,
+		},
+		{
+			name:      "wrapped 504 then succeed",
+			callErrs:  []error{fmt.Errorf("got 504: %w", apierr.ErrDeadlineExceeded), nil},
+			wantCalls: 2,
+		},
+		{
+			name:      "non-504 halts",
+			callErrs:  []error{otherErr},
+			wantErr:   otherErr,
+			wantCalls: 1,
+		},
+		{
+			name:      "504 then non-504 halts",
+			callErrs:  []error{apierr.ErrDeadlineExceeded, otherErr},
+			wantErr:   otherErr,
+			wantCalls: 2,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			calls := 0
+			_, err := RetryOn504(context.Background(), func(ctx context.Context) (*struct{}, error) {
+				e := tc.callErrs[calls]
+				calls++
+				return nil, e
+			})
+			if calls != tc.wantCalls {
+				t.Errorf("call count = %d, want %d", calls, tc.wantCalls)
+			}
+			if !errors.Is(err, tc.wantErr) {
+				t.Errorf("err = %v, want %v", err, tc.wantErr)
+			}
+		})
+	}
 }