diff --git a/coderd/x/chatd/chaterror/classify.go b/coderd/x/chatd/chaterror/classify.go index 3cbdb1eeb8..66ba58bb75 100644 --- a/coderd/x/chatd/chaterror/classify.go +++ b/coderd/x/chatd/chaterror/classify.go @@ -4,6 +4,7 @@ import ( "context" "errors" "strings" + "time" ) // ClassifiedError is the normalized, user-facing view of an @@ -14,6 +15,10 @@ type ClassifiedError struct { Provider string Retryable bool StatusCode int + + // RetryAfter is a normalized minimum retry delay derived from + // provider response metadata when available. + RetryAfter time.Duration } // WithProvider returns a copy of the classification using an explicit @@ -71,13 +76,17 @@ func Classify(err error) ClassifiedError { return normalizeClassification(wrapped.classified) } + structured := extractProviderErrorDetails(err) message := strings.TrimSpace(err.Error()) - if message == "" { + if message == "" && structured.statusCode == 0 && structured.retryAfter <= 0 { return ClassifiedError{} } lower := strings.ToLower(message) - statusCode := extractStatusCode(lower) + statusCode := structured.statusCode + if statusCode == 0 { + statusCode = extractStatusCode(lower) + } provider := detectProvider(lower) canceled := errors.Is(err, context.Canceled) || strings.Contains(lower, "context canceled") interrupted := containsAny(lower, interruptedPatterns...) @@ -87,6 +96,7 @@ func Classify(err error) ClassifiedError { Kind: KindGeneric, Provider: provider, StatusCode: statusCode, + RetryAfter: structured.retryAfter, }) } @@ -157,6 +167,7 @@ func Classify(err error) ClassifiedError { Provider: provider, Retryable: rule.retryable, StatusCode: statusCode, + RetryAfter: structured.retryAfter, }) } @@ -164,6 +175,7 @@ func Classify(err error) ClassifiedError { Kind: KindGeneric, Provider: provider, StatusCode: statusCode, + RetryAfter: structured.retryAfter, }) } @@ -171,8 +183,14 @@ func normalizeClassification(classified ClassifiedError) ClassifiedError { classified.Message = strings.TrimSpace(classified.Message) classified.Kind = strings.TrimSpace(classified.Kind) classified.Provider = normalizeProvider(classified.Provider) + if classified.RetryAfter < 0 { + classified.RetryAfter = 0 + } if classified.Kind == "" && classified.Message == "" { - return ClassifiedError{} + if classified.StatusCode == 0 && classified.RetryAfter <= 0 { + return ClassifiedError{} + } + classified.Kind = KindGeneric } if classified.Kind == "" { classified.Kind = KindGeneric diff --git a/coderd/x/chatd/chaterror/classify_test.go b/coderd/x/chatd/chaterror/classify_test.go index c7d0a9e204..577e120e72 100644 --- a/coderd/x/chatd/chaterror/classify_test.go +++ b/coderd/x/chatd/chaterror/classify_test.go @@ -2,8 +2,11 @@ package chaterror_test import ( "context" + "net/http" "testing" + "time" + "charm.land/fantasy" "github.com/stretchr/testify/require" "golang.org/x/xerrors" @@ -338,3 +341,94 @@ func TestWithProviderAddsProviderWhenUnknown(t *testing.T) { StatusCode: 429, }, enriched) } + +func TestClassify_UsesStructuredProviderStatusAndRetryAfter(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(testProviderError( + "", + 429, + map[string]string{"Retry-After": "30"}, + )) + + require.Equal(t, chaterror.ClassifiedError{ + Message: "The AI provider is rate limiting requests (HTTP 429).", + Kind: chaterror.KindRateLimit, + Provider: "", + Retryable: true, + StatusCode: 429, + RetryAfter: 30 * time.Second, + }, classified) +} + +func TestClassify_PrefersRetryAfterMsOverRetryAfter(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(testProviderError( + "upstream failed", + 429, + map[string]string{ + "Retry-After": "30", + "ReTrY-AfTeR-Ms": "1500", + }, + )) + + require.Equal(t, 429, classified.StatusCode) + require.Equal(t, 1500*time.Millisecond, classified.RetryAfter) +} + +func TestClassify_ParsesRetryAfterHTTPDate(t *testing.T) { + t.Parallel() + + retryAt := time.Now().Add(3 * time.Second).UTC().Format(http.TimeFormat) + classified := chaterror.Classify(testProviderError( + "upstream failed", + 429, + map[string]string{"Retry-After": retryAt}, + )) + + require.Equal(t, 429, classified.StatusCode) + require.GreaterOrEqual(t, classified.RetryAfter, 2*time.Second) + require.LessOrEqual(t, classified.RetryAfter, 4*time.Second) +} + +func TestClassify_IgnoresInvalidRetryAfter(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(testProviderError( + "upstream failed", + 429, + map[string]string{"Retry-After": "definitely not a delay"}, + )) + + require.Zero(t, classified.RetryAfter) +} + +func TestWithProviderPreservesRetryAfter(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(testProviderError( + "upstream failed", + 429, + map[string]string{"Retry-After": "30"}, + )) + + enriched := classified.WithProvider("openai") + require.Equal(t, 30*time.Second, enriched.RetryAfter) + require.Equal(t, chaterror.ClassifiedError{ + Message: "OpenAI is rate limiting requests (HTTP 429).", + Kind: chaterror.KindRateLimit, + Provider: "openai", + Retryable: true, + StatusCode: 429, + RetryAfter: 30 * time.Second, + }, enriched) +} + +func testProviderError(message string, statusCode int, headers map[string]string) error { + return &fantasy.ProviderError{ + Message: message, + StatusCode: statusCode, + ResponseHeaders: headers, + } +} diff --git a/coderd/x/chatd/chaterror/provider_error.go b/coderd/x/chatd/chaterror/provider_error.go new file mode 100644 index 0000000000..68b350d610 --- /dev/null +++ b/coderd/x/chatd/chaterror/provider_error.go @@ -0,0 +1,65 @@ +package chaterror + +import ( + "errors" + "net/http" + "strconv" + "strings" + "time" + + "charm.land/fantasy" +) + +type providerErrorDetails struct { + statusCode int + retryAfter time.Duration +} + +func extractProviderErrorDetails(err error) providerErrorDetails { + var providerErr *fantasy.ProviderError + if !errors.As(err, &providerErr) { + return providerErrorDetails{} + } + + return providerErrorDetails{ + statusCode: providerErr.StatusCode, + retryAfter: retryAfterFromHeaders(providerErr.ResponseHeaders), + } +} + +func retryAfterFromHeaders(headers map[string]string) time.Duration { + if len(headers) == 0 { + return 0 + } + + // Prefer retry-after-ms (OpenAI convention, milliseconds) + // over the standard retry-after (seconds or HTTP-date). + for key, value := range headers { + if strings.EqualFold(key, "retry-after-ms") { + ms, err := strconv.ParseFloat(strings.TrimSpace(value), 64) + if err == nil && ms > 0 { + return time.Duration(ms * float64(time.Millisecond)) + } + } + } + + for key, value := range headers { + if strings.EqualFold(key, "retry-after") { + v := strings.TrimSpace(value) + if seconds, err := strconv.ParseFloat(v, 64); err == nil { + if seconds > 0 { + return time.Duration(seconds * float64(time.Second)) + } + return 0 + } + if retryAt, err := http.ParseTime(v); err == nil { + if d := time.Until(retryAt); d > 0 { + return d + } + } + return 0 + } + } + + return 0 +} diff --git a/coderd/x/chatd/chatretry/chatretry.go b/coderd/x/chatd/chatretry/chatretry.go index ea573b0ecb..10e2d7e806 100644 --- a/coderd/x/chatd/chatretry/chatretry.go +++ b/coderd/x/chatd/chatretry/chatretry.go @@ -1,6 +1,6 @@ // Package chatretry provides retry logic for transient LLM provider -// errors. It classifies errors as retryable or permanent and -// implements exponential backoff matching the behavior of coder/mux. +// errors. It classifies errors as retryable or permanent and uses +// exponential backoff with provider retry hints when available. package chatretry import ( @@ -50,6 +50,16 @@ func Delay(attempt int) time.Duration { return d } +// effectiveDelay returns the delay for the given 0-indexed attempt +// while honoring any provider-supplied minimum retry delay. +func effectiveDelay(attempt int, classified ClassifiedError) time.Duration { + delay := Delay(attempt) + if classified.RetryAfter > delay { + return classified.RetryAfter + } + return delay +} + // RetryFn is the function to retry. It receives a context and returns // an error. The context may be a child of the original with adjusted // deadlines for individual attempts. @@ -62,7 +72,8 @@ type OnRetryFn func(attempt int, err error, classified ClassifiedError, delay ti // Retry calls fn repeatedly until it succeeds, returns a // non-retryable error, ctx is canceled, or MaxAttempts is reached. -// Retries use exponential backoff capped at MaxDelay. +// Retries use exponential backoff capped at MaxDelay, unless the +// normalized error includes a longer provider Retry-After hint. // // The onRetry callback (if non-nil) is called before each retry // attempt, giving the caller a chance to reset state, log, or @@ -94,7 +105,7 @@ func Retry(ctx context.Context, fn RetryFn, onRetry OnRetryFn) error { ) } - delay := Delay(attempt - 1) + delay := effectiveDelay(attempt-1, classified) if onRetry != nil { onRetry(attempt, err, classified, delay) diff --git a/coderd/x/chatd/chatretry/chatretry_test.go b/coderd/x/chatd/chatretry/chatretry_test.go index 67ced0180d..8d5f517339 100644 --- a/coderd/x/chatd/chatretry/chatretry_test.go +++ b/coderd/x/chatd/chatretry/chatretry_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "charm.land/fantasy" "github.com/stretchr/testify/require" "golang.org/x/xerrors" @@ -255,3 +256,64 @@ func TestRetry_OnRetryNilDoesNotPanic(t *testing.T) { t.Fatalf("expected nil error, got %v", err) } } + +func TestRetry_UsesRetryAfterAsDelayFloor(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + headers map[string]string + wantDelay time.Duration + wantRetryAfter time.Duration + }{ + { + name: "LongerThanBaseDelay", + headers: map[string]string{"Retry-After": "3"}, + wantDelay: 3 * time.Second, + wantRetryAfter: 3 * time.Second, + }, + { + name: "ShorterThanBaseDelay", + headers: map[string]string{"Retry-After-Ms": "500"}, + wantDelay: chatretry.Delay(0), + wantRetryAfter: 500 * time.Millisecond, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + calls := 0 + var gotClassified chatretry.ClassifiedError + var gotDelay time.Duration + err := chatretry.Retry(ctx, func(_ context.Context) error { + calls++ + return &fantasy.ProviderError{ + Message: "upstream failed", + StatusCode: 429, + ResponseHeaders: tt.headers, + } + }, func( + _ int, + _ error, + classified chatretry.ClassifiedError, + delay time.Duration, + ) { + gotClassified = classified + gotDelay = delay + cancel() + }) + + require.ErrorIs(t, err, context.Canceled) + require.Equal(t, 1, calls) + require.True(t, gotClassified.Retryable) + require.Equal(t, 429, gotClassified.StatusCode) + require.Equal(t, tt.wantRetryAfter, gotClassified.RetryAfter) + require.Equal(t, tt.wantDelay, gotDelay) + }) + } +} diff --git a/enterprise/coderd/x/chatd/chatd_test.go b/enterprise/coderd/x/chatd/chatd_test.go index f54b9952d8..cc588f8705 100644 --- a/enterprise/coderd/x/chatd/chatd_test.go +++ b/enterprise/coderd/x/chatd/chatd_test.go @@ -565,7 +565,7 @@ func TestSubscribeRetryEventAcrossInstances(t *testing.T) { require.Greater(t, retryEvent.DelayMs, int64(0)) require.Equal(t, "rate_limit", retryEvent.Kind) require.Equal(t, "openai", retryEvent.Provider) - require.Equal(t, 0, retryEvent.StatusCode) + require.Equal(t, 429, retryEvent.StatusCode) require.Contains(t, retryEvent.Error, "rate limiting requests") require.False(t, assistantMessageBeforeRetry) require.False(t, waitingBeforeRetry)