mirror of
https://github.com/coder/coder.git
synced 2026-09-22 05:05:20 +08:00
fix(coderd/x/chatd): respect provider Retry-After headers in chat retry loop (#23351)
> **PR Stack** > 1. **#23351** ← `#23282` *(you are here)* > 2. #23282 ← `#23275` > 3. #23275 ← `#23349` > 4. #23349 ← `main` --- ## Summary `chatretry.Retry()` used pure exponential backoff (1 s, 2 s, 4 s, …) and never consulted provider `Retry-After` headers. Fantasy's `ProviderError` carries `ResponseHeaders` including `Retry-After`, but `chaterror.Classify()` only parsed error text and silently dropped the structured transport metadata. This makes `Retry-After` a first-class signal in the classification → retry pipeline. <img width="853" height="346" alt="image" src="https://github.com/user-attachments/assets/65f012b6-8173-43d2-957e-ab9faddea525" /> ## Changes ### `coderd/chatd/chaterror/classify.go` - Added `RetryAfter time.Duration` field to `ClassifiedError` — a normalized minimum retry delay derived from provider response metadata. - `Classify()` now calls `extractProviderErrorDetails()` before falling back to text heuristics. Structured `ProviderError.StatusCode` takes priority over regex extraction. - `normalizeClassification()` preserves and clamps `RetryAfter`. ### `coderd/chatd/chaterror/provider_error.go` (new) Provider-specific extraction, isolated from the text-based classification logic: - `extractProviderErrorDetails()` unwraps `*fantasy.ProviderError` from the error chain via `errors.As`. - `retryAfterFromHeaders()` parses headers in priority order: 1. `retry-after-ms` (OpenAI-specific, millisecond precision) 2. `retry-after` (standard HTTP — integer seconds or HTTP-date) - Case-insensitive header key lookup. ### `coderd/chatd/chatretry/chatretry.go` - `effectiveDelay(attempt, classified)` computes `max(Delay(attempt), classified.RetryAfter)` — the provider hint acts as a floor without weakening the local exponential backoff. - `Retry()` now uses `effectiveDelay` and passes the effective delay to both `onRetry(...)` and the sleep timer, so downstream payloads, logs, and the frontend countdown stay aligned automatically. ### Tests - `classify_test.go`: Structured provider status + `Retry-After` extraction, `retry-after-ms` priority, HTTP-date parsing, invalid header fallback, `WithProvider` preservation. - `chatretry_test.go`: Retry-after-as-floor semantics — longer hint wins, shorter hint keeps base delay. ## Design notes - **No SDK/API/frontend changes needed.** `codersdk.ChatStreamRetry` already carries `DelayMs` and `RetryingAt`, and the frontend already consumes them. The fix is purely in the server-side delay computation. - **Existing retryability rules unchanged.** This fixes *when* we sleep, not *whether* an error is retryable. - **Provider hint is a floor:** `max(baseDelay, RetryAfter)` ensures we never retry earlier than the provider asks, and never weaken our own backoff curve.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user