diff --git a/sdk/integrations/github/retry.go b/sdk/integrations/github/retry.go index a30ea77..3248439 100644 --- a/sdk/integrations/github/retry.go +++ b/sdk/integrations/github/retry.go @@ -7,71 +7,33 @@ import ( "strconv" "strings" "time" + + retryutil "github.com/qf-studio/studio-sdk/sdk/util/retry" ) -// RetryOptions configures retry behavior -type RetryOptions struct { - MaxRetries int // Maximum number of retries (default: 3) - BaseDelay time.Duration // Initial delay between retries (default: 1s) - MaxDelay time.Duration // Maximum delay between retries (default: 30s) -} +// RetryOptions configures retry behavior. The backoff/context-cancellation +// engine lives in sdk/util/retry; this alias keeps the existing github API +// surface (Client.retryOpts, tests) unchanged. +type RetryOptions = retryutil.RetryOptions -// DefaultRetryOptions returns sensible defaults for retry behavior +// DefaultRetryOptions returns sensible defaults for retry behavior. func DefaultRetryOptions() RetryOptions { - return RetryOptions{ - MaxRetries: 3, - BaseDelay: 1 * time.Second, - MaxDelay: 30 * time.Second, - } + return retryutil.DefaultRetryOptions() } // WithRetry executes an operation with exponential backoff retry. -// It respects context cancellation and GitHub's Retry-After header. +// It respects context cancellation and GitHub's Retry-After header. Error +// classification defaults to GitHub's own isRetryableError/extractRetryAfter +// (provider-specific: 429/403-rate-limit, 5xx, network errors, GraphQL +// RATE_LIMITED) unless the caller already set Classify/ExtractRetryAfter. func WithRetry[T any](ctx context.Context, op func() (T, error), opts RetryOptions) (T, error) { - var result T - var lastErr error - - for attempt := 0; attempt <= opts.MaxRetries; attempt++ { - result, lastErr = op() - if lastErr == nil { - return result, nil - } - - // Don't retry non-retryable errors - if !isRetryableError(lastErr) { - return result, lastErr - } - - // Don't retry if we've exhausted retries - if attempt >= opts.MaxRetries { - return result, lastErr - } - - // Calculate delay with exponential backoff: 1s, 2s, 4s, 8s... - delay := opts.BaseDelay * time.Duration(1< opts.MaxDelay { - delay = opts.MaxDelay - } - - // Check for Retry-After header in rate limit errors; cap at MaxDelay so - // a runaway "Retry-After: 3600" can't stall the worker for an hour. - if retryAfter := extractRetryAfter(lastErr); retryAfter > 0 { - if retryAfter > opts.MaxDelay { - retryAfter = opts.MaxDelay - } - delay = retryAfter - } - - // Wait with context cancellation support - select { - case <-ctx.Done(): - return result, ctx.Err() - case <-time.After(delay): - // Continue to next retry attempt - } + if opts.Classify == nil { + opts.Classify = isRetryableError } - - return result, lastErr + if opts.ExtractRetryAfter == nil { + opts.ExtractRetryAfter = extractRetryAfter + } + return retryutil.WithRetry(ctx, op, opts) } // WithRetryVoid is like WithRetry but for operations that don't return a value. diff --git a/sdk/util/retry/retry.go b/sdk/util/retry/retry.go new file mode 100644 index 0000000..7b5bd33 --- /dev/null +++ b/sdk/util/retry/retry.go @@ -0,0 +1,165 @@ +// Package retry provides a provider-agnostic retry engine (exponential +// backoff, context cancellation, Retry-After honoring) plus the two typed +// errors every connector needs to drive it: RateLimitError and AuthError. +// +// Error classification is deliberately NOT centralized here. Each connector +// knows how its provider signals "rate limited" or "auth rejected" (status +// codes, headers, GraphQL error types, ...) and maps those onto typed errors +// or its own RetryOptions.Classify/ExtractRetryAfter functions. This package +// only understands the typed RateLimitError/AuthError by default; it does not +// pattern-match provider error strings. +// +// This is a leaf package with no dependencies beyond the Go stdlib, so any +// connector can import it without creating a cycle. +package retry + +import ( + "context" + "errors" + "fmt" + "time" +) + +// RateLimitError signals that an operation was rejected because of API rate +// limiting. RetryAfter carries the provider's requested backoff when known; +// zero means the caller should fall back to exponential backoff. +type RateLimitError struct { + RetryAfter time.Duration + Message string +} + +func (e *RateLimitError) Error() string { + return fmt.Sprintf("rate limited: %s", e.Message) +} + +// AuthError signals that the credentials themselves were rejected (e.g. HTTP +// 401). It is never retryable — retrying with the same token cannot help, so +// callers should park the operation immediately instead of burning attempts. +type AuthError struct { + Message string +} + +func (e *AuthError) Error() string { + return fmt.Sprintf("auth error: %s", e.Message) +} + +// RetryOptions configures WithRetry / WithRetryVoid. +// +// Classify and ExtractRetryAfter are per-connector hooks: each integration +// maps its provider's error responses onto typed errors (or its own richer +// error types) and supplies matching functions here. Both default to typed- +// error-only behavior when left nil — no status-code string matching. +type RetryOptions struct { + MaxRetries int // Maximum number of retries (default: 3) + BaseDelay time.Duration // Initial delay between retries (default: 1s) + MaxDelay time.Duration // Maximum delay between retries (default: 30s) + + // Classify reports whether err is transient and worth retrying. Defaults + // to DefaultClassify when nil. + Classify func(err error) bool + + // ExtractRetryAfter returns the provider-requested backoff for err, or 0 + // if none applies. Defaults to DefaultExtractRetryAfter when nil. + ExtractRetryAfter func(err error) time.Duration +} + +// DefaultRetryOptions returns sensible defaults for retry behavior. +func DefaultRetryOptions() RetryOptions { + return RetryOptions{ + MaxRetries: 3, + BaseDelay: 1 * time.Second, + MaxDelay: 30 * time.Second, + } +} + +// DefaultClassify retries *RateLimitError and refuses *AuthError; any other +// error is treated as non-retryable. Connectors with richer transient-error +// detection (5xx, network failures, provider-specific GraphQL errors, ...) +// should supply their own Classify via RetryOptions. +func DefaultClassify(err error) bool { + if err == nil { + return false + } + var rlErr *RateLimitError + if errors.As(err, &rlErr) { + return true + } + var authErr *AuthError + if errors.As(err, &authErr) { + return false + } + return false +} + +// DefaultExtractRetryAfter returns RateLimitError.RetryAfter when err (or +// something it wraps) is a *RateLimitError, else 0. +func DefaultExtractRetryAfter(err error) time.Duration { + var rlErr *RateLimitError + if errors.As(err, &rlErr) { + return rlErr.RetryAfter + } + return 0 +} + +// WithRetry executes op with exponential backoff, honoring context +// cancellation and any provider-requested Retry-After delay reported via +// opts.ExtractRetryAfter. +func WithRetry[T any](ctx context.Context, op func() (T, error), opts RetryOptions) (T, error) { + classify := opts.Classify + if classify == nil { + classify = DefaultClassify + } + extractRetryAfter := opts.ExtractRetryAfter + if extractRetryAfter == nil { + extractRetryAfter = DefaultExtractRetryAfter + } + + var result T + var lastErr error + + for attempt := 0; attempt <= opts.MaxRetries; attempt++ { + result, lastErr = op() + if lastErr == nil { + return result, nil + } + + if !classify(lastErr) { + return result, lastErr + } + if attempt >= opts.MaxRetries { + return result, lastErr + } + + // Exponential backoff: BaseDelay, 2x, 4x, 8x... capped at MaxDelay. + delay := opts.BaseDelay * time.Duration(1< opts.MaxDelay { + delay = opts.MaxDelay + } + + // A provider-requested Retry-After overrides backoff, but is still + // capped at MaxDelay so a runaway "Retry-After: 3600" can't stall + // the caller for an hour. + if retryAfter := extractRetryAfter(lastErr); retryAfter > 0 { + if retryAfter > opts.MaxDelay { + retryAfter = opts.MaxDelay + } + delay = retryAfter + } + + select { + case <-ctx.Done(): + return result, ctx.Err() + case <-time.After(delay): + } + } + + return result, lastErr +} + +// WithRetryVoid is like WithRetry but for operations that don't return a value. +func WithRetryVoid(ctx context.Context, op func() error, opts RetryOptions) error { + _, err := WithRetry(ctx, func() (struct{}, error) { + return struct{}{}, op() + }, opts) + return err +} diff --git a/sdk/util/retry/retry_test.go b/sdk/util/retry/retry_test.go new file mode 100644 index 0000000..58faace --- /dev/null +++ b/sdk/util/retry/retry_test.go @@ -0,0 +1,326 @@ +package retry + +import ( + "context" + "errors" + "testing" + "time" +) + +func TestWithRetry_SuccessOnFirstAttempt(t *testing.T) { + calls := 0 + result, err := WithRetry(context.Background(), func() (string, error) { + calls++ + return "success", nil + }, DefaultRetryOptions()) + + if err != nil { + t.Errorf("expected no error, got: %v", err) + } + if result != "success" { + t.Errorf("expected 'success', got: %s", result) + } + if calls != 1 { + t.Errorf("expected 1 call, got: %d", calls) + } +} + +func TestWithRetry_SuccessAfterRetries(t *testing.T) { + calls := 0 + result, err := WithRetry(context.Background(), func() (string, error) { + calls++ + if calls < 3 { + return "", &RateLimitError{Message: "rate limited"} + } + return "success", nil + }, RetryOptions{ + MaxRetries: 3, + BaseDelay: 1 * time.Millisecond, + MaxDelay: 10 * time.Millisecond, + }) + + if err != nil { + t.Errorf("expected no error, got: %v", err) + } + if result != "success" { + t.Errorf("expected 'success', got: %s", result) + } + if calls != 3 { + t.Errorf("expected 3 calls, got: %d", calls) + } +} + +func TestWithRetry_ExhaustsRetries(t *testing.T) { + calls := 0 + _, err := WithRetry(context.Background(), func() (string, error) { + calls++ + return "", &RateLimitError{Message: "still limited"} + }, RetryOptions{ + MaxRetries: 3, + BaseDelay: 1 * time.Millisecond, + MaxDelay: 10 * time.Millisecond, + }) + + if err == nil { + t.Error("expected error after exhausting retries") + } + if calls != 4 { + t.Errorf("expected 4 calls (1 + 3 retries), got: %d", calls) + } +} + +func TestWithRetry_NonRetryableError(t *testing.T) { + calls := 0 + _, err := WithRetry(context.Background(), func() (string, error) { + calls++ + return "", errors.New("boom: not a typed error") + }, RetryOptions{ + MaxRetries: 3, + BaseDelay: 1 * time.Millisecond, + MaxDelay: 10 * time.Millisecond, + }) + + if err == nil { + t.Error("expected error for non-retryable failure") + } + if calls != 1 { + t.Errorf("expected 1 call (no retries for untyped error), got: %d", calls) + } +} + +func TestWithRetry_AuthErrorShortCircuits(t *testing.T) { + calls := 0 + _, err := WithRetry(context.Background(), func() (string, error) { + calls++ + return "", &AuthError{Message: "bad credentials"} + }, RetryOptions{ + MaxRetries: 5, + BaseDelay: 1 * time.Millisecond, + MaxDelay: 10 * time.Millisecond, + }) + + var authErr *AuthError + if !errors.As(err, &authErr) { + t.Fatalf("expected *AuthError, got %T", err) + } + if calls != 1 { + t.Errorf("expected 1 call (AuthError must not retry), got: %d", calls) + } +} + +func TestWithRetry_ContextCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + calls := 0 + + go func() { + time.Sleep(5 * time.Millisecond) + cancel() + }() + + _, err := WithRetry(ctx, func() (string, error) { + calls++ + return "", &RateLimitError{Message: "rate limited"} + }, RetryOptions{ + MaxRetries: 10, + BaseDelay: 50 * time.Millisecond, + MaxDelay: 100 * time.Millisecond, + }) + + if !errors.Is(err, context.Canceled) { + t.Errorf("expected context.Canceled, got: %v", err) + } + if calls > 2 { + t.Errorf("expected at most 2 calls before cancellation, got: %d", calls) + } +} + +func TestWithRetry_RetryAfterHonored(t *testing.T) { + start := time.Now() + calls := 0 + _, _ = WithRetry(context.Background(), func() (string, error) { + calls++ + if calls == 1 { + return "", &RateLimitError{RetryAfter: 20 * time.Millisecond, Message: "rate limited"} + } + return "ok", nil + }, RetryOptions{ + MaxRetries: 3, + BaseDelay: 1 * time.Millisecond, + MaxDelay: 1 * time.Second, + }) + elapsed := time.Since(start) + + if elapsed < 20*time.Millisecond { + t.Errorf("Retry-After was not honored: elapsed %v (expected >= 20ms)", elapsed) + } + if calls != 2 { + t.Errorf("expected 2 calls, got %d", calls) + } +} + +func TestWithRetry_RetryAfterCappedAtMaxDelay(t *testing.T) { + start := time.Now() + calls := 0 + _, _ = WithRetry(context.Background(), func() (string, error) { + calls++ + if calls == 1 { + return "", &RateLimitError{RetryAfter: 60 * time.Second, Message: "rate limited"} + } + return "ok", nil + }, RetryOptions{ + MaxRetries: 3, + BaseDelay: 1 * time.Millisecond, + MaxDelay: 10 * time.Millisecond, + }) + elapsed := time.Since(start) + + if elapsed > 500*time.Millisecond { + t.Errorf("Retry-After was not capped: elapsed %v (expected <500ms)", elapsed) + } + if calls != 2 { + t.Errorf("expected 2 calls, got %d", calls) + } +} + +func TestWithRetry_ExponentialBackoff(t *testing.T) { + delays := []time.Duration{} + lastCall := time.Now() + + calls := 0 + _, _ = WithRetry(context.Background(), func() (string, error) { + now := time.Now() + if calls > 0 { + delays = append(delays, now.Sub(lastCall)) + } + lastCall = now + calls++ + if calls <= 3 { + return "", &RateLimitError{Message: "rate limited"} + } + return "done", nil + }, RetryOptions{ + MaxRetries: 3, + BaseDelay: 10 * time.Millisecond, + MaxDelay: 100 * time.Millisecond, + }) + + if len(delays) != 3 { + t.Fatalf("expected 3 delays, got %d", len(delays)) + } + + expectedDelays := []time.Duration{10 * time.Millisecond, 20 * time.Millisecond, 40 * time.Millisecond} + tolerance := 5 * time.Millisecond + + for i, expected := range expectedDelays { + if delays[i] < expected-tolerance || delays[i] > expected+2*tolerance { + t.Errorf("delay[%d] = %v, expected ~%v (tolerance %v)", i, delays[i], expected, tolerance) + } + } +} + +func TestWithRetryVoid_Success(t *testing.T) { + calls := 0 + err := WithRetryVoid(context.Background(), func() error { + calls++ + return nil + }, DefaultRetryOptions()) + + if err != nil { + t.Errorf("expected no error, got: %v", err) + } + if calls != 1 { + t.Errorf("expected 1 call, got: %d", calls) + } +} + +func TestWithRetryVoid_RetriesOnError(t *testing.T) { + calls := 0 + err := WithRetryVoid(context.Background(), func() error { + calls++ + if calls < 2 { + return &RateLimitError{Message: "rate limited"} + } + return nil + }, RetryOptions{ + MaxRetries: 3, + BaseDelay: 1 * time.Millisecond, + MaxDelay: 10 * time.Millisecond, + }) + + if err != nil { + t.Errorf("expected no error, got: %v", err) + } + if calls != 2 { + t.Errorf("expected 2 calls, got: %d", calls) + } +} + +func TestWithRetry_CustomClassifyOverridesDefault(t *testing.T) { + calls := 0 + _, err := WithRetry(context.Background(), func() (string, error) { + calls++ + return "", errors.New("status 503: service unavailable") + }, RetryOptions{ + MaxRetries: 2, + BaseDelay: 1 * time.Millisecond, + MaxDelay: 10 * time.Millisecond, + Classify: func(err error) bool { + return err != nil // treat every error as retryable for this test + }, + }) + + if err == nil { + t.Error("expected error after exhausting retries") + } + if calls != 3 { + t.Errorf("expected 3 calls (1 + 2 retries) with custom Classify, got: %d", calls) + } +} + +func TestDefaultClassify(t *testing.T) { + tests := []struct { + name string + err error + retryable bool + }{ + {"nil error", nil, false}, + {"rate limit error", &RateLimitError{Message: "limited"}, true}, + {"auth error", &AuthError{Message: "bad token"}, false}, + {"untyped error", errors.New("something went wrong"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := DefaultClassify(tt.err); got != tt.retryable { + t.Errorf("DefaultClassify(%v) = %v, want %v", tt.err, got, tt.retryable) + } + }) + } +} + +func TestDefaultExtractRetryAfter(t *testing.T) { + if got := DefaultExtractRetryAfter(nil); got != 0 { + t.Errorf("expected 0 for nil error, got %v", got) + } + if got := DefaultExtractRetryAfter(errors.New("plain")); got != 0 { + t.Errorf("expected 0 for untyped error, got %v", got) + } + got := DefaultExtractRetryAfter(&RateLimitError{RetryAfter: 17 * time.Second}) + if got != 17*time.Second { + t.Errorf("expected 17s, got %v", got) + } +} + +func TestDefaultRetryOptions(t *testing.T) { + opts := DefaultRetryOptions() + + if opts.MaxRetries != 3 { + t.Errorf("expected MaxRetries=3, got %d", opts.MaxRetries) + } + if opts.BaseDelay != 1*time.Second { + t.Errorf("expected BaseDelay=1s, got %v", opts.BaseDelay) + } + if opts.MaxDelay != 30*time.Second { + t.Errorf("expected MaxDelay=30s, got %v", opts.MaxDelay) + } +}