From 70f031d793f0cde79f48acc2bd070a0abb7a1655 Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Wed, 25 Mar 2026 13:47:54 +1100 Subject: [PATCH] feat(coderd/chatd): structured chat error classification and retry hardening (#23275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > **PR Stack** > 1. #23351 ← `#23282` > 2. #23282 ← `#23275` > 3. **#23275** ← `#23349` *(you are here)* > 4. #23349 ← `main` --- ## Summary Extracts a structured error classification subsystem for agent chat (`chatd`) so that retry and error payloads carry machine-readable metadata — error kind, provider name, HTTP status code, and retryability — instead of raw error strings. This is the **backend half** of the error-handling work. The frontend counterpart is in #23282. ## Changes ### New package: `coderd/chatd/chaterror/` Canonical error classification — extracts error kind, provider, status code, and user-facing message from raw provider errors. One source of truth that drives both retry policy and stream payloads. - **`kind.go`**: Error kind enum (`rate_limit`, `timeout`, `auth`, `config`, `overloaded`, `unknown`). - **`signals.go`**: Signal extraction — parses provider name, HTTP status code, and retryability from error strings and wrapped types. - **`classify.go`**: Classification logic — maps extracted signals to an error kind. - **`message.go`**: User-facing message templates keyed by kind + signals. - **`payload.go`**: Projectors that build `ChatStreamError` and `ChatStreamRetry` payloads from a classified error. ### Modified - **`codersdk/chats.go`**: Added `Kind`, `Provider`, `Retryable`, `StatusCode` fields to `ChatStreamError` and `ChatStreamRetry`. - **`coderd/chatd/chatretry/`**: Thinned to retry-policy only; classification logic moved to `chaterror`. - **`coderd/chatd/chatloop/`**: Added per-attempt first-chunk timeout (60 s) via `guardedStream` wrapper — produces retryable `startup_timeout` errors instead of hanging forever. - **`coderd/chatd/chatd.go`**: Publishes normalized retry/error payloads via `chaterror` projectors. --- coderd/pubsub/chatstreamnotify.go | 8 +- coderd/x/chatd/chatd.go | 66 +-- coderd/x/chatd/chatd_internal_test.go | 96 ++++- coderd/x/chatd/chaterror/classify.go | 184 ++++++++ coderd/x/chatd/chaterror/classify_test.go | 340 +++++++++++++++ coderd/x/chatd/chaterror/export_test.go | 13 + coderd/x/chatd/chaterror/kind.go | 13 + coderd/x/chatd/chaterror/message.go | 122 ++++++ coderd/x/chatd/chaterror/payload.go | 39 ++ coderd/x/chatd/chaterror/payload_test.go | 58 +++ coderd/x/chatd/chaterror/signals.go | 104 +++++ coderd/x/chatd/chaterror/signals_test.go | 69 +++ coderd/x/chatd/chatloop/chatloop.go | 159 ++++++- coderd/x/chatd/chatloop/chatloop_test.go | 400 ++++++++++++++++++ coderd/x/chatd/chatretry/chatretry.go | 106 +---- coderd/x/chatd/chatretry/chatretry_test.go | 317 +++----------- codersdk/chats.go | 17 +- enterprise/coderd/x/chatd/chatd_test.go | 49 ++- site/src/api/typesGenerated.ts | 33 +- .../AgentDetail/ChatContext.test.tsx | 10 +- 20 files changed, 1789 insertions(+), 414 deletions(-) create mode 100644 coderd/x/chatd/chaterror/classify.go create mode 100644 coderd/x/chatd/chaterror/classify_test.go create mode 100644 coderd/x/chatd/chaterror/export_test.go create mode 100644 coderd/x/chatd/chaterror/kind.go create mode 100644 coderd/x/chatd/chaterror/message.go create mode 100644 coderd/x/chatd/chaterror/payload.go create mode 100644 coderd/x/chatd/chaterror/payload_test.go create mode 100644 coderd/x/chatd/chaterror/signals.go create mode 100644 coderd/x/chatd/chaterror/signals_test.go diff --git a/coderd/pubsub/chatstreamnotify.go b/coderd/pubsub/chatstreamnotify.go index d1a0179e4b..b39ca2cda7 100644 --- a/coderd/pubsub/chatstreamnotify.go +++ b/coderd/pubsub/chatstreamnotify.go @@ -37,7 +37,13 @@ type ChatStreamNotifyMessage struct { // from the database. Retry *codersdk.ChatStreamRetry `json:"retry,omitempty"` - // Error is set when a processing error occurs. + // ErrorPayload carries a structured error event for cross-replica + // live delivery. Keep Error for backward compatibility with older + // replicas during rolling deploys. + ErrorPayload *codersdk.ChatStreamError `json:"error_payload,omitempty"` + + // Error is the legacy string-only error payload kept for mixed- + // version compatibility during rollout. Error string `json:"error,omitempty"` // QueueUpdate is set when the queued messages change. diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 8d6e16df19..22a3aab186 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -30,9 +30,11 @@ import ( "github.com/coder/coder/v2/coderd/webpush" "github.com/coder/coder/v2/coderd/workspacestats" "github.com/coder/coder/v2/coderd/x/chatd/chatcost" + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" "github.com/coder/coder/v2/coderd/x/chatd/chatloop" "github.com/coder/coder/v2/coderd/x/chatd/chatprompt" "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" + "github.com/coder/coder/v2/coderd/x/chatd/chatretry" "github.com/coder/coder/v2/coderd/x/chatd/chattool" "github.com/coder/coder/v2/coderd/x/chatd/mcpclient" "github.com/coder/coder/v2/codersdk" @@ -2075,7 +2077,17 @@ func (p *Server) Subscribe( }: } } - if notify.Error != "" { + if notify.ErrorPayload != nil { + select { + case <-mergedCtx.Done(): + return + case mergedEvents <- codersdk.ChatStreamEvent{ + Type: codersdk.ChatStreamEventTypeError, + ChatID: chatID, + Error: notify.ErrorPayload, + }: + } + } else if notify.Error != "" { select { case <-mergedCtx.Done(): return @@ -2294,30 +2306,31 @@ func (p *Server) publishRetry(chatID uuid.UUID, payload *codersdk.ChatStreamRetr }) } -func (p *Server) publishError(chatID uuid.UUID, message string) { - message = strings.TrimSpace(message) - if message == "" { +func (p *Server) publishError(chatID uuid.UUID, classified chaterror.ClassifiedError) { + payload := chaterror.StreamErrorPayload(classified) + if payload == nil { return } p.publishEvent(chatID, codersdk.ChatStreamEvent{ Type: codersdk.ChatStreamEventTypeError, - Error: &codersdk.ChatStreamError{Message: message}, + Error: payload, }) p.publishChatStreamNotify(chatID, coderdpubsub.ChatStreamNotifyMessage{ - Error: message, + ErrorPayload: payload, + Error: payload.Message, }) } -func processingFailureReason(err error) (string, bool) { +func processingFailure(err error) (chaterror.ClassifiedError, bool) { if err == nil { - return "", false + return chaterror.ClassifiedError{}, false } - reason := strings.TrimSpace(err.Error()) - if reason == "" { - return "", false + classified := chaterror.Classify(err) + if classified.Message == "" { + return chaterror.ClassifiedError{}, false } - return reason, true + return classified, true } func panicFailureReason(recovered any) string { @@ -2654,7 +2667,10 @@ func (p *Server) processChat(ctx context.Context, chat database.Chat) { if r := recover(); r != nil { logger.Error(cleanupCtx, "panic during chat processing", slog.F("panic", r)) lastError = panicFailureReason(r) - p.publishError(chat.ID, lastError) + p.publishError(chat.ID, chaterror.ClassifiedError{ + Message: lastError, + Kind: chaterror.KindGeneric, + }) status = database.ChatStatusError } @@ -2763,9 +2779,9 @@ func (p *Server) processChat(ctx context.Context, chat database.Chat) { return } logger.Error(ctx, "failed to process chat", slog.Error(err)) - if reason, ok := processingFailureReason(err); ok { - lastError = reason - p.publishError(chat.ID, lastError) + if classified, ok := processingFailure(err); ok { + lastError = classified.Message + p.publishError(chat.ID, classified) } status = database.ChatStatusError return @@ -3540,7 +3556,12 @@ func (p *Server) runChat( chainModeActive = false }, - OnRetry: func(attempt int, retryErr error, delay time.Duration) { + OnRetry: func( + attempt int, + retryErr error, + classified chatretry.ClassifiedError, + delay time.Duration, + ) { if val, ok := p.chatStreams.Load(chat.ID); ok { if rs, ok := val.(*chatStreamState); ok { rs.mu.Lock() @@ -3554,12 +3575,8 @@ func (p *Server) runChat( slog.F("delay", delay.String()), slog.Error(retryErr), ) - p.publishRetry(chat.ID, &codersdk.ChatStreamRetry{ - Attempt: attempt, - DelayMs: delay.Milliseconds(), - Error: retryErr.Error(), - RetryingAt: time.Now().Add(delay), - }) + payload := chaterror.StreamRetryPayload(attempt, delay, classified) + p.publishRetry(chat.ID, payload) }, OnInterruptedPersistError: func(err error) { @@ -3567,7 +3584,8 @@ func (p *Server) runChat( }, }) if err != nil { - return result, err + classified := chaterror.Classify(err).WithProvider(model.Provider()) + return result, chaterror.WithClassification(err, classified) } result.FinalAssistantText = finalAssistantText return result, nil diff --git a/coderd/x/chatd/chatd_internal_test.go b/coderd/x/chatd/chatd_internal_test.go index 13f8b8471b..2946d29e65 100644 --- a/coderd/x/chatd/chatd_internal_test.go +++ b/coderd/x/chatd/chatd_internal_test.go @@ -18,6 +18,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbmock" dbpubsub "github.com/coder/coder/v2/coderd/database/pubsub" coderdpubsub "github.com/coder/coder/v2/coderd/pubsub" + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/workspacesdk" "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" @@ -451,7 +452,10 @@ func TestSubscribeDeliversRetryEventViaPubsubOnce(t *testing.T) { expected := &codersdk.ChatStreamRetry{ Attempt: 1, DelayMs: (1500 * time.Millisecond).Milliseconds(), - Error: "rate limit exceeded", + Error: "OpenAI is rate limiting requests (HTTP 429). Please try again later.", + Kind: chaterror.KindRateLimit, + Provider: "openai", + StatusCode: 429, RetryingAt: retryingAt, } @@ -462,6 +466,81 @@ func TestSubscribeDeliversRetryEventViaPubsubOnce(t *testing.T) { requireNoStreamEvent(t, events, 200*time.Millisecond) } +func TestSubscribePrefersStructuredErrorPayloadViaPubsub(t *testing.T) { + t.Parallel() + + ctx, cancelCtx := context.WithCancel(context.Background()) + defer cancelCtx() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + + chatID := uuid.New() + chat := database.Chat{ID: chatID, Status: database.ChatStatusPending} + + gomock.InOrder( + db.EXPECT().GetChatMessagesByChatID(gomock.Any(), database.GetChatMessagesByChatIDParams{ + ChatID: chatID, + AfterID: 0, + }).Return(nil, nil), + db.EXPECT().GetChatQueuedMessages(gomock.Any(), chatID).Return(nil, nil), + db.EXPECT().GetChatByID(gomock.Any(), chatID).Return(chat, nil), + ) + + server := newSubscribeTestServer(t, db) + _, events, cancel, ok := server.Subscribe(ctx, chatID, nil, 0) + require.True(t, ok) + defer cancel() + + classified := chaterror.ClassifiedError{ + Message: "OpenAI is rate limiting requests (HTTP 429). Please try again later.", + Kind: chaterror.KindRateLimit, + Provider: "openai", + Retryable: true, + StatusCode: 429, + } + server.publishError(chatID, classified) + + event := requireStreamErrorEvent(t, events) + require.Equal(t, chaterror.StreamErrorPayload(classified), event.Error) + requireNoStreamEvent(t, events, 200*time.Millisecond) +} + +func TestSubscribeFallsBackToLegacyErrorStringViaPubsub(t *testing.T) { + t.Parallel() + + ctx, cancelCtx := context.WithCancel(context.Background()) + defer cancelCtx() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + + chatID := uuid.New() + chat := database.Chat{ID: chatID, Status: database.ChatStatusPending} + + gomock.InOrder( + db.EXPECT().GetChatMessagesByChatID(gomock.Any(), database.GetChatMessagesByChatIDParams{ + ChatID: chatID, + AfterID: 0, + }).Return(nil, nil), + db.EXPECT().GetChatQueuedMessages(gomock.Any(), chatID).Return(nil, nil), + db.EXPECT().GetChatByID(gomock.Any(), chatID).Return(chat, nil), + ) + + server := newSubscribeTestServer(t, db) + _, events, cancel, ok := server.Subscribe(ctx, chatID, nil, 0) + require.True(t, ok) + defer cancel() + + server.publishChatStreamNotify(chatID, coderdpubsub.ChatStreamNotifyMessage{ + Error: "legacy error only", + }) + + event := requireStreamErrorEvent(t, events) + require.Equal(t, &codersdk.ChatStreamError{Message: "legacy error only"}, event.Error) + requireNoStreamEvent(t, events, 200*time.Millisecond) +} + func newSubscribeTestServer(t *testing.T, db database.Store) *Server { t.Helper() @@ -502,6 +581,21 @@ func requireStreamRetryEvent(t *testing.T, events <-chan codersdk.ChatStreamEven } } +func requireStreamErrorEvent(t *testing.T, events <-chan codersdk.ChatStreamEvent) codersdk.ChatStreamEvent { + t.Helper() + + select { + case event, ok := <-events: + require.True(t, ok, "chat stream closed before delivering an event") + require.Equal(t, codersdk.ChatStreamEventTypeError, event.Type) + require.NotNil(t, event.Error) + return event + case <-time.After(time.Second): + t.Fatal("timed out waiting for chat stream error event") + return codersdk.ChatStreamEvent{} + } +} + func requireNoStreamEvent(t *testing.T, events <-chan codersdk.ChatStreamEvent, wait time.Duration) { t.Helper() diff --git a/coderd/x/chatd/chaterror/classify.go b/coderd/x/chatd/chaterror/classify.go new file mode 100644 index 0000000000..522a6c5f31 --- /dev/null +++ b/coderd/x/chatd/chaterror/classify.go @@ -0,0 +1,184 @@ +package chaterror + +import ( + "context" + "errors" + "strings" +) + +// ClassifiedError is the normalized, user-facing view of an +// underlying provider or runtime error. +type ClassifiedError struct { + Message string + Kind string + Provider string + Retryable bool + StatusCode int +} + +// WithProvider returns a copy of the classification using an explicit +// provider hint. Explicit provider hints are trusted over provider names +// heuristically parsed from the error text. +func (c ClassifiedError) WithProvider(provider string) ClassifiedError { + hint := normalizeProvider(provider) + if hint == "" { + return normalizeClassification(c) + } + if c.Provider == hint && strings.TrimSpace(c.Message) != "" { + return normalizeClassification(c) + } + updated := c + updated.Provider = hint + updated.Message = "" + return normalizeClassification(updated) +} + +// WithClassification wraps err so future calls to Classify return +// classified instead of re-deriving it from err.Error(). +func WithClassification(err error, classified ClassifiedError) error { + if err == nil { + return nil + } + return &classifiedError{ + cause: err, + classified: normalizeClassification(classified), + } +} + +type classifiedError struct { + cause error + classified ClassifiedError +} + +func (e *classifiedError) Error() string { + return e.cause.Error() +} + +func (e *classifiedError) Unwrap() error { + return e.cause +} + +// Classify normalizes err into a stable, user-facing payload used for +// retry handling, streamed terminal errors, and persisted last_error +// values. +func Classify(err error) ClassifiedError { + if err == nil { + return ClassifiedError{} + } + + var wrapped *classifiedError + if errors.As(err, &wrapped) { + return normalizeClassification(wrapped.classified) + } + + message := strings.TrimSpace(err.Error()) + if message == "" { + return ClassifiedError{} + } + + lower := strings.ToLower(message) + statusCode := extractStatusCode(lower) + provider := detectProvider(lower) + canceled := errors.Is(err, context.Canceled) || strings.Contains(lower, "context canceled") + interrupted := containsAny(lower, interruptedPatterns...) + if canceled || interrupted { + return normalizeClassification(ClassifiedError{ + Message: "The request was canceled before it completed.", + Kind: KindGeneric, + Provider: provider, + StatusCode: statusCode, + }) + } + + deadline := errors.Is(err, context.DeadlineExceeded) || strings.Contains(lower, "context deadline exceeded") + overloadedMatch := statusCode == 529 || containsAny(lower, overloadedPatterns...) + authStrong := statusCode == 401 || containsAny(lower, authStrongPatterns...) + configMatch := containsAny(lower, configPatterns...) + authWeak := statusCode == 403 || containsAny(lower, authWeakPatterns...) + rateLimitMatch := statusCode == 429 || containsAny(lower, rateLimitPatterns...) + timeoutMatch := deadline || statusCode == 408 || statusCode == 502 || + statusCode == 503 || statusCode == 504 || + containsAny(lower, timeoutPatterns...) + genericRetryableMatch := statusCode == 500 || containsAny(lower, genericRetryablePatterns...) + + // Config signals should beat ambiguous wrapper signals so + // transient-looking errors like "503 invalid model" fail fast. + // Overloaded stays ahead because 529/overloaded is a dedicated + // provider saturation signal, not a common transport wrapper. + // Strong auth still stays above config because bad credentials are + // the root cause when both signals appear. + rules := []struct { + match bool + kind string + retryable bool + }{ + { + match: overloadedMatch, + kind: KindOverloaded, + retryable: true, + }, + { + match: authStrong, + kind: KindAuth, + retryable: false, + }, + { + match: authWeak && !configMatch, + kind: KindAuth, + retryable: false, + }, + { + match: rateLimitMatch && !configMatch, + kind: KindRateLimit, + retryable: true, + }, + { + match: timeoutMatch && !configMatch, + kind: KindTimeout, + retryable: !deadline, + }, + { + match: configMatch, + kind: KindConfig, + retryable: false, + }, + { + match: genericRetryableMatch, + kind: KindGeneric, + retryable: true, + }, + } + for _, rule := range rules { + if !rule.match { + continue + } + return normalizeClassification(ClassifiedError{ + Kind: rule.kind, + Provider: provider, + Retryable: rule.retryable, + StatusCode: statusCode, + }) + } + + return normalizeClassification(ClassifiedError{ + Kind: KindGeneric, + Provider: provider, + StatusCode: statusCode, + }) +} + +func normalizeClassification(classified ClassifiedError) ClassifiedError { + classified.Message = strings.TrimSpace(classified.Message) + classified.Kind = strings.TrimSpace(classified.Kind) + classified.Provider = normalizeProvider(classified.Provider) + if classified.Kind == "" && classified.Message == "" { + return ClassifiedError{} + } + if classified.Kind == "" { + classified.Kind = KindGeneric + } + if classified.Message == "" { + classified.Message = userFacingMessage(classified) + } + return classified +} diff --git a/coderd/x/chatd/chaterror/classify_test.go b/coderd/x/chatd/chaterror/classify_test.go new file mode 100644 index 0000000000..16aacd7b87 --- /dev/null +++ b/coderd/x/chatd/chaterror/classify_test.go @@ -0,0 +1,340 @@ +package chaterror_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" +) + +func TestClassify(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want chaterror.ClassifiedError + }{ + { + name: "AmbiguousOverloadKeepsProviderUnknown", + err: xerrors.New("status 529 from upstream"), + want: chaterror.ClassifiedError{ + Message: "The AI provider is temporarily overloaded (HTTP 529). Please try again later.", + Kind: chaterror.KindOverloaded, + Provider: "", + Retryable: true, + StatusCode: 529, + }, + }, + { + name: "ExplicitAnthropicOverload", + err: xerrors.New("anthropic overloaded_error"), + want: chaterror.ClassifiedError{ + Message: "Anthropic is temporarily overloaded. Please try again later.", + Kind: chaterror.KindOverloaded, + Provider: "anthropic", + Retryable: true, + StatusCode: 0, + }, + }, + { + name: "AuthBeatsConfig", + err: xerrors.New("authentication failed: invalid model"), + want: chaterror.ClassifiedError{ + Message: "Authentication with the AI provider failed. Check the API key, permissions, and billing settings.", + Kind: chaterror.KindAuth, + Provider: "", + Retryable: false, + StatusCode: 0, + }, + }, + { + name: "PureConfig", + err: xerrors.New("invalid model"), + want: chaterror.ClassifiedError{ + Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Kind: chaterror.KindConfig, + Provider: "", + Retryable: false, + StatusCode: 0, + }, + }, + { + name: "BareForbiddenClassifiesAsAuth", + err: xerrors.New("forbidden"), + want: chaterror.ClassifiedError{ + Message: "Authentication with the AI provider failed. Check the API key, permissions, and billing settings.", + Kind: chaterror.KindAuth, + Provider: "", + Retryable: false, + StatusCode: 0, + }, + }, + { + name: "ExplicitStatus401ClassifiesAsAuth", + err: xerrors.New("status 401 from upstream"), + want: chaterror.ClassifiedError{ + Message: "Authentication with the AI provider failed. Check the API key, permissions, and billing settings.", + Kind: chaterror.KindAuth, + Provider: "", + Retryable: false, + StatusCode: 401, + }, + }, + { + name: "ExplicitStatus403ClassifiesAsAuth", + err: xerrors.New("status 403 from upstream"), + want: chaterror.ClassifiedError{ + Message: "Authentication with the AI provider failed. Check the API key, permissions, and billing settings.", + Kind: chaterror.KindAuth, + Provider: "", + Retryable: false, + StatusCode: 403, + }, + }, + { + name: "ForbiddenContextLengthClassifiesAsConfig", + err: xerrors.New("forbidden: context length exceeded"), + want: chaterror.ClassifiedError{ + Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Kind: chaterror.KindConfig, + Provider: "", + Retryable: false, + StatusCode: 0, + }, + }, + { + name: "ExplicitStatus429ClassifiesAsRateLimit", + err: xerrors.New("status 429 from upstream"), + want: chaterror.ClassifiedError{ + Message: "The AI provider is rate limiting requests (HTTP 429). Please try again later.", + Kind: chaterror.KindRateLimit, + Provider: "", + Retryable: true, + StatusCode: 429, + }, + }, + { + name: "RateLimitDoesNotBeatConfig", + err: xerrors.New("status 429: invalid model"), + want: chaterror.ClassifiedError{ + Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Kind: chaterror.KindConfig, + Provider: "", + Retryable: false, + StatusCode: 429, + }, + }, + { + name: "ServiceUnavailableClassifiesAsRetryableTimeout", + err: xerrors.New("service unavailable"), + want: chaterror.ClassifiedError{ + Message: "The AI provider is temporarily unavailable. Please try again later.", + Kind: chaterror.KindTimeout, + Provider: "", + Retryable: true, + StatusCode: 0, + }, + }, + { + name: "TimeoutDoesNotBeatConfigViaStatusCode", + err: xerrors.New("status 503: invalid model"), + want: chaterror.ClassifiedError{ + Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Kind: chaterror.KindConfig, + Provider: "", + Retryable: false, + StatusCode: 503, + }, + }, + { + name: "TimeoutDoesNotBeatConfigViaMessage", + err: xerrors.New("service unavailable: model not found"), + want: chaterror.ClassifiedError{ + Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Kind: chaterror.KindConfig, + Provider: "", + Retryable: false, + StatusCode: 0, + }, + }, + { + name: "ConnectionRefusedUnsupportedModelClassifiesAsConfig", + err: xerrors.New("connection refused: unsupported model"), + want: chaterror.ClassifiedError{ + Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Kind: chaterror.KindConfig, + Provider: "", + Retryable: false, + StatusCode: 0, + }, + }, + { + name: "DeadlineExceededStaysNonRetryableTimeout", + err: context.DeadlineExceeded, + want: chaterror.ClassifiedError{ + Message: "The request timed out before it completed. Please try again.", + Kind: chaterror.KindTimeout, + Provider: "", + Retryable: false, + StatusCode: 0, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, chaterror.Classify(tt.err)) + }) + } +} + +func TestClassify_PatternCoverage(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err string + wantKind string + wantRetry bool + }{ + {name: "OverloadedLiteral", err: "overloaded", wantKind: chaterror.KindOverloaded, wantRetry: true}, + {name: "RateLimitLiteral", err: "rate limit", wantKind: chaterror.KindRateLimit, wantRetry: true}, + {name: "RateLimitUnderscoreLiteral", err: "rate_limit", wantKind: chaterror.KindRateLimit, wantRetry: true}, + {name: "RateLimitedLiteral", err: "rate limited", wantKind: chaterror.KindRateLimit, wantRetry: true}, + {name: "RateLimitedHyphenLiteral", err: "rate-limited", wantKind: chaterror.KindRateLimit, wantRetry: true}, + {name: "TooManyRequestsLiteral", err: "too many requests", wantKind: chaterror.KindRateLimit, wantRetry: true}, + {name: "TimeoutLiteral", err: "timeout", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "TimedOutLiteral", err: "timed out", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "ServiceUnavailableLiteral", err: "service unavailable", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "UnavailableLiteral", err: "unavailable", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "ConnectionResetLiteral", err: "connection reset", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "ConnectionRefusedLiteral", err: "connection refused", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "EOFLiteral", err: "eof", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "BrokenPipeLiteral", err: "broken pipe", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "BadGatewayLiteral", err: "bad gateway", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "GatewayTimeoutLiteral", err: "gateway timeout", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "AuthenticationLiteral", err: "authentication", wantKind: chaterror.KindAuth, wantRetry: false}, + {name: "UnauthorizedLiteral", err: "unauthorized", wantKind: chaterror.KindAuth, wantRetry: false}, + {name: "InvalidAPIKeyLiteral", err: "invalid api key", wantKind: chaterror.KindAuth, wantRetry: false}, + {name: "InvalidAPIKeyUnderscoreLiteral", err: "invalid_api_key", wantKind: chaterror.KindAuth, wantRetry: false}, + {name: "QuotaLiteral", err: "quota", wantKind: chaterror.KindAuth, wantRetry: false}, + {name: "BillingLiteral", err: "billing", wantKind: chaterror.KindAuth, wantRetry: false}, + {name: "InsufficientQuotaLiteral", err: "insufficient_quota", wantKind: chaterror.KindAuth, wantRetry: false}, + {name: "PaymentRequiredLiteral", err: "payment required", wantKind: chaterror.KindAuth, wantRetry: false}, + {name: "ForbiddenLiteral", err: "forbidden", wantKind: chaterror.KindAuth, wantRetry: false}, + {name: "InvalidModelLiteral", err: "invalid model", wantKind: chaterror.KindConfig, wantRetry: false}, + {name: "ModelNotFoundLiteral", err: "model not found", wantKind: chaterror.KindConfig, wantRetry: false}, + {name: "ModelNotFoundUnderscoreLiteral", err: "model_not_found", wantKind: chaterror.KindConfig, wantRetry: false}, + {name: "UnsupportedModelLiteral", err: "unsupported model", wantKind: chaterror.KindConfig, wantRetry: false}, + {name: "ContextLengthExceededLiteral", err: "context length exceeded", wantKind: chaterror.KindConfig, wantRetry: false}, + {name: "ContextExceededLiteral", err: "context_exceeded", wantKind: chaterror.KindConfig, wantRetry: false}, + {name: "MaximumContextLengthLiteral", err: "maximum context length", wantKind: chaterror.KindConfig, wantRetry: false}, + {name: "MalformedConfigLiteral", err: "malformed config", wantKind: chaterror.KindConfig, wantRetry: false}, + {name: "MalformedConfigurationLiteral", err: "malformed configuration", wantKind: chaterror.KindConfig, wantRetry: false}, + {name: "ServerErrorLiteral", err: "server error", wantKind: chaterror.KindGeneric, wantRetry: true}, + {name: "InternalServerErrorLiteral", err: "internal server error", wantKind: chaterror.KindGeneric, wantRetry: true}, + {name: "ChatInterruptedLiteral", err: "chat interrupted", wantKind: chaterror.KindGeneric, wantRetry: false}, + {name: "RequestInterruptedLiteral", err: "request interrupted", wantKind: chaterror.KindGeneric, wantRetry: false}, + {name: "OperationInterruptedLiteral", err: "operation interrupted", wantKind: chaterror.KindGeneric, wantRetry: false}, + {name: "Status408", err: "status 408", wantKind: chaterror.KindTimeout, wantRetry: true}, + {name: "Status500", err: "status 500", wantKind: chaterror.KindGeneric, wantRetry: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(xerrors.New(tt.err)) + require.Equal(t, tt.wantKind, classified.Kind) + require.Equal(t, tt.wantRetry, classified.Retryable) + }) + } +} + +func TestClassify_TransportFailuresUseBroaderRetryMessage(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err string + }{ + {name: "TimeoutLiteral", err: "timeout"}, + {name: "EOFLiteral", err: "eof"}, + {name: "BrokenPipeLiteral", err: "broken pipe"}, + {name: "ConnectionResetLiteral", err: "connection reset"}, + {name: "ConnectionRefusedLiteral", err: "connection refused"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(xerrors.New(tt.err)) + require.Equal(t, chaterror.KindTimeout, classified.Kind) + require.True(t, classified.Retryable) + require.Equal( + t, + "The AI provider is temporarily unavailable. Please try again later.", + classified.Message, + ) + }) + } +} + +func TestClassify_StartupTimeoutWrappedClassificationWins(t *testing.T) { + t.Parallel() + + wrapped := chaterror.WithClassification( + xerrors.New("context canceled"), + chaterror.ClassifiedError{ + Kind: chaterror.KindStartupTimeout, + Provider: "openai", + Retryable: true, + }, + ) + + require.Equal(t, chaterror.ClassifiedError{ + Message: "OpenAI did not start responding in time. Please try again.", + Kind: chaterror.KindStartupTimeout, + Provider: "openai", + Retryable: true, + StatusCode: 0, + }, chaterror.Classify(wrapped)) +} + +func TestWithProviderUsesExplicitHint(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(xerrors.New("openai received status 429 from upstream")) + require.Equal(t, "openai", classified.Provider) + + enriched := classified.WithProvider("azure openai") + require.Equal(t, chaterror.ClassifiedError{ + Message: "Azure OpenAI is rate limiting requests (HTTP 429). Please try again later.", + Kind: chaterror.KindRateLimit, + Provider: "azure", + Retryable: true, + StatusCode: 429, + }, enriched) +} + +func TestWithProviderAddsProviderWhenUnknown(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(xerrors.New("received status 429 from upstream")) + require.Empty(t, classified.Provider) + + enriched := classified.WithProvider("openai") + require.Equal(t, chaterror.ClassifiedError{ + Message: "OpenAI is rate limiting requests (HTTP 429). Please try again later.", + Kind: chaterror.KindRateLimit, + Provider: "openai", + Retryable: true, + StatusCode: 429, + }, enriched) +} diff --git a/coderd/x/chatd/chaterror/export_test.go b/coderd/x/chatd/chaterror/export_test.go new file mode 100644 index 0000000000..db532be96c --- /dev/null +++ b/coderd/x/chatd/chaterror/export_test.go @@ -0,0 +1,13 @@ +package chaterror + +// ExtractStatusCodeForTest lets external-package tests pin signal extraction +// behavior without exposing the helper in production builds. +func ExtractStatusCodeForTest(lower string) int { + return extractStatusCode(lower) +} + +// DetectProviderForTest lets external-package tests cover provider-detection +// ordering without opening the production API surface. +func DetectProviderForTest(lower string) string { + return detectProvider(lower) +} diff --git a/coderd/x/chatd/chaterror/kind.go b/coderd/x/chatd/chaterror/kind.go new file mode 100644 index 0000000000..502155a6f9 --- /dev/null +++ b/coderd/x/chatd/chaterror/kind.go @@ -0,0 +1,13 @@ +// Package chaterror classifies provider/runtime failures into stable, +// user-facing chat error payloads. +package chaterror + +const ( + KindOverloaded = "overloaded" + KindRateLimit = "rate_limit" + KindTimeout = "timeout" + KindStartupTimeout = "startup_timeout" + KindAuth = "auth" + KindConfig = "config" + KindGeneric = "generic" +) diff --git a/coderd/x/chatd/chaterror/message.go b/coderd/x/chatd/chaterror/message.go new file mode 100644 index 0000000000..f038643901 --- /dev/null +++ b/coderd/x/chatd/chaterror/message.go @@ -0,0 +1,122 @@ +package chaterror + +import ( + "fmt" + "strings" +) + +func userFacingMessage(classified ClassifiedError) string { + subject := providerSubject(classified.Provider) + switch classified.Kind { + case KindOverloaded: + return optionalStatusMessage( + subject, + classified.StatusCode, + "%s is temporarily overloaded (HTTP %d). Please try again later.", + "%s is temporarily overloaded. Please try again later.", + ) + case KindRateLimit: + return optionalStatusMessage( + subject, + classified.StatusCode, + "%s is rate limiting requests (HTTP %d). Please try again later.", + "%s is rate limiting requests. Please try again later.", + ) + case KindTimeout: + if classified.StatusCode > 0 { + return fmt.Sprintf( + "%s is temporarily unavailable (HTTP %d). Please try again later.", + subject, + classified.StatusCode, + ) + } + if classified.Retryable { + return fmt.Sprintf("%s is temporarily unavailable. Please try again later.", subject) + } + return "The request timed out before it completed. Please try again." + case KindStartupTimeout: + return fmt.Sprintf("%s did not start responding in time. Please try again.", subject) + case KindAuth: + if displayName := providerDisplayName(classified.Provider); displayName != "" { + return fmt.Sprintf( + "Authentication with %s failed. Check the API key, permissions, and billing settings.", + displayName, + ) + } + return "Authentication with the AI provider failed. Check the API key, permissions, and billing settings." + case KindConfig: + return fmt.Sprintf( + "%s rejected the model configuration. Check the selected model and provider settings.", + subject, + ) + default: + if classified.StatusCode > 0 { + suffix := " Please try again." + if classified.Retryable { + suffix = " Please try again later." + } + return fmt.Sprintf( + "%s returned an unexpected error (HTTP %d).%s", + subject, + classified.StatusCode, + suffix, + ) + } + if classified.Retryable { + return fmt.Sprintf( + "%s returned an unexpected error. Please try again later.", + subject, + ) + } + return "The chat request failed unexpectedly. Please try again." + } +} + +func optionalStatusMessage(subject string, statusCode int, withStatus string, withoutStatus string) string { + if statusCode > 0 { + return fmt.Sprintf(withStatus, subject, statusCode) + } + return fmt.Sprintf(withoutStatus, subject) +} + +func providerSubject(provider string) string { + if displayName := providerDisplayName(provider); displayName != "" { + return displayName + } + return "The AI provider" +} + +func providerDisplayName(provider string) string { + switch normalizeProvider(provider) { + case "anthropic": + return "Anthropic" + case "azure": + return "Azure OpenAI" + case "bedrock": + return "AWS Bedrock" + case "google": + return "Google" + case "openai": + return "OpenAI" + case "openai-compat": + return "OpenAI Compatible" + case "openrouter": + return "OpenRouter" + case "vercel": + return "Vercel AI Gateway" + default: + return "" + } +} + +func normalizeProvider(provider string) string { + normalized := strings.ToLower(strings.TrimSpace(provider)) + switch normalized { + case "azure openai", "azure-openai": + return "azure" + case "openai compat", "openai compatible", "openai_compat": + return "openai-compat" + default: + return normalized + } +} diff --git a/coderd/x/chatd/chaterror/payload.go b/coderd/x/chatd/chaterror/payload.go new file mode 100644 index 0000000000..462cdba979 --- /dev/null +++ b/coderd/x/chatd/chaterror/payload.go @@ -0,0 +1,39 @@ +package chaterror + +import ( + "time" + + "github.com/coder/coder/v2/codersdk" +) + +func StreamErrorPayload(classified ClassifiedError) *codersdk.ChatStreamError { + if classified.Message == "" { + return nil + } + return &codersdk.ChatStreamError{ + Message: classified.Message, + Kind: classified.Kind, + Provider: classified.Provider, + Retryable: classified.Retryable, + StatusCode: classified.StatusCode, + } +} + +func StreamRetryPayload( + attempt int, + delay time.Duration, + classified ClassifiedError, +) *codersdk.ChatStreamRetry { + if classified.Message == "" { + return nil + } + return &codersdk.ChatStreamRetry{ + Attempt: attempt, + DelayMs: delay.Milliseconds(), + Error: classified.Message, + Kind: classified.Kind, + Provider: classified.Provider, + StatusCode: classified.StatusCode, + RetryingAt: time.Now().Add(delay), + } +} diff --git a/coderd/x/chatd/chaterror/payload_test.go b/coderd/x/chatd/chaterror/payload_test.go new file mode 100644 index 0000000000..836a221c99 --- /dev/null +++ b/coderd/x/chatd/chaterror/payload_test.go @@ -0,0 +1,58 @@ +package chaterror_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" + "github.com/coder/coder/v2/codersdk" +) + +func TestStreamErrorPayloadUsesNormalizedClassification(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify( + xerrors.New("azure openai received status 429 from upstream"), + ) + payload := chaterror.StreamErrorPayload(classified) + + require.Equal(t, &codersdk.ChatStreamError{ + Message: "Azure OpenAI is rate limiting requests (HTTP 429). Please try again later.", + Kind: chaterror.KindRateLimit, + Provider: "azure", + Retryable: true, + StatusCode: 429, + }, payload) +} + +func TestStreamErrorPayloadNilForEmptyClassification(t *testing.T) { + t.Parallel() + + require.Nil(t, chaterror.StreamErrorPayload(chaterror.ClassifiedError{})) +} + +func TestStreamRetryPayloadUsesNormalizedClassification(t *testing.T) { + t.Parallel() + + delay := 3 * time.Second + startedAt := time.Now() + payload := chaterror.StreamRetryPayload(2, delay, chaterror.ClassifiedError{ + Message: "retry me", + Kind: chaterror.KindGeneric, + Provider: "openai", + Retryable: true, + StatusCode: 503, + }) + + require.NotNil(t, payload) + require.Equal(t, 2, payload.Attempt) + require.Equal(t, delay.Milliseconds(), payload.DelayMs) + require.Equal(t, "retry me", payload.Error) + require.Equal(t, chaterror.KindGeneric, payload.Kind) + require.Equal(t, "openai", payload.Provider) + require.Equal(t, 503, payload.StatusCode) + require.WithinDuration(t, startedAt.Add(delay), payload.RetryingAt, time.Second) +} diff --git a/coderd/x/chatd/chaterror/signals.go b/coderd/x/chatd/chaterror/signals.go new file mode 100644 index 0000000000..eb09a6d620 --- /dev/null +++ b/coderd/x/chatd/chaterror/signals.go @@ -0,0 +1,104 @@ +package chaterror + +import ( + "regexp" + "strconv" + "strings" +) + +type providerHint struct { + provider string + patterns []string +} + +var ( + statusCodePattern = regexp.MustCompile(`(?:status(?:\s+code)?|http)\s*[:=]?\s*(\d{3})`) + standaloneStatusPattern = regexp.MustCompile(`\b(?:401|403|408|429|500|502|503|504|529)\b`) + providerHints = []providerHint{ + {provider: "openai-compat", patterns: []string{"openai-compat", "openai compatible"}}, + {provider: "azure", patterns: []string{"azure openai", "azure-openai"}}, + {provider: "openrouter", patterns: []string{"openrouter"}}, + {provider: "bedrock", patterns: []string{"aws bedrock", "bedrock"}}, + {provider: "vercel", patterns: []string{"vercel ai gateway", "vercel"}}, + {provider: "anthropic", patterns: []string{"anthropic", "claude"}}, + {provider: "google", patterns: []string{"google", "gemini", "vertex"}}, + {provider: "openai", patterns: []string{"openai"}}, + } + overloadedPatterns = []string{"overloaded"} + rateLimitPatterns = []string{"rate limit", "rate_limit", "rate limited", "rate-limited", "too many requests"} + timeoutPatterns = []string{ + "timeout", + "timed out", + "service unavailable", + "unavailable", + "connection reset", + "connection refused", + "eof", + "broken pipe", + "bad gateway", + "gateway timeout", + } + authStrongPatterns = []string{ + "authentication", + "unauthorized", + "invalid api key", + "invalid_api_key", + "quota", + "billing", + "insufficient_quota", + "payment required", + } + authWeakPatterns = []string{"forbidden"} + configPatterns = []string{ + "invalid model", + "model not found", + "model_not_found", + "unsupported model", + "context length exceeded", + "context_exceeded", + "maximum context length", + "malformed config", + "malformed configuration", + } + genericRetryablePatterns = []string{"server error", "internal server error"} + interruptedPatterns = []string{"chat interrupted", "request interrupted", "operation interrupted"} +) + +func extractStatusCode(lower string) int { + if matches := statusCodePattern.FindStringSubmatch(lower); len(matches) == 2 { + if code, err := strconv.Atoi(matches[1]); err == nil { + return code + } + return 0 + } + for _, loc := range standaloneStatusPattern.FindAllStringIndex(lower, -1) { + // Skip values in host:port text. A later standalone status code in the + // same message may still be valid, so keep scanning. + if loc[0] > 0 && lower[loc[0]-1] == ':' { + continue + } + if code, err := strconv.Atoi(lower[loc[0]:loc[1]]); err == nil { + return code + } + return 0 + } + return 0 +} + +func detectProvider(lower string) string { + for _, hint := range providerHints { + if containsAny(lower, hint.patterns...) { + return hint.provider + } + } + return "" +} + +func containsAny(lower string, patterns ...string) bool { + for _, pattern := range patterns { + if strings.Contains(lower, pattern) { + return true + } + } + return false +} diff --git a/coderd/x/chatd/chaterror/signals_test.go b/coderd/x/chatd/chaterror/signals_test.go new file mode 100644 index 0000000000..799bc1033d --- /dev/null +++ b/coderd/x/chatd/chaterror/signals_test.go @@ -0,0 +1,69 @@ +package chaterror_test + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" +) + +func TestExtractStatusCode(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + want int + }{ + {name: "Status", input: "received status 429 from upstream", want: 429}, + {name: "StatusCode", input: "status code: 503", want: 503}, + {name: "HTTP", input: "http 502 bad gateway", want: 502}, + {name: "Standalone", input: "got 504 from upstream", want: 504}, + {name: "MultipleStandaloneCodesReturnFirstMatch", input: "retrying 503 after 429", want: 503}, + {name: "MixedCaseViaCallerLowering", input: "HTTP 503 bad gateway", want: 503}, + {name: "PortNumberIPIsNotStatus", input: "dial tcp 10.0.0.1:503: connection refused", want: 0}, + {name: "PortNumberHostIsNotStatus", input: "proxy.internal:502 unreachable", want: 0}, + {name: "PortNumberDialIsNotStatus", input: "dial tcp 172.16.0.5:429: refused", want: 0}, + {name: "PortThenRealStatusReturnsRealStatus", input: "proxy at 10.0.0.1:500 returned 503", want: 503}, + {name: "NoFabricatedOverloadStatus", input: "anthropic overloaded_error", want: 0}, + {name: "NoFabricatedRateLimitStatus", input: "too many requests", want: 0}, + {name: "NoFabricatedBadGatewayStatus", input: "bad gateway", want: 0}, + {name: "NoFabricatedServiceUnavailableStatus", input: "service unavailable", want: 0}, + {name: "NoStatus", input: "boom", want: 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, chaterror.ExtractStatusCodeForTest(strings.ToLower(tt.input))) + }) + } +} + +func TestDetectProvider(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + want string + }{ + {name: "OpenAICompatBeatsOpenAI", input: "openai-compat upstream error", want: "openai-compat"}, + {name: "OpenAICompatibleAlias", input: "openai compatible proxy", want: "openai-compat"}, + {name: "AzureOpenAI", input: "azure openai rate limited", want: "azure"}, + {name: "OpenAI", input: "openai rate limited", want: "openai"}, + {name: "Anthropic", input: "anthropic overloaded", want: "anthropic"}, + {name: "GoogleGemini", input: "gemini timeout", want: "google"}, + {name: "Vercel", input: "vercel ai gateway 503", want: "vercel"}, + {name: "Unknown", input: "local provider error", want: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, chaterror.DetectProviderForTest(strings.ToLower(tt.input))) + }) + } +} diff --git a/coderd/x/chatd/chatloop/chatloop.go b/coderd/x/chatd/chatloop/chatloop.go index 1e72ded6a5..011c34ca61 100644 --- a/coderd/x/chatd/chatloop/chatloop.go +++ b/coderd/x/chatd/chatloop/chatloop.go @@ -17,6 +17,7 @@ import ( "charm.land/fantasy/schema" "golang.org/x/xerrors" + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" "github.com/coder/coder/v2/coderd/x/chatd/chatprompt" "github.com/coder/coder/v2/coderd/x/chatd/chatretry" "github.com/coder/coder/v2/codersdk" @@ -24,15 +25,24 @@ import ( const ( interruptedToolResultErrorMessage = "tool call was interrupted before it produced a result" - // maxCompactionRetries limits how many times the post-run // compaction safety net can re-enter the step loop. This // prevents infinite compaction loops when the model keeps // hitting the context limit after summarization. maxCompactionRetries = 3 + // defaultStartupTimeout bounds how long an individual + // model attempt may spend starting to respond before + // the attempt is canceled and retried. + defaultStartupTimeout = 60 * time.Second ) -var ErrInterrupted = xerrors.New("chat interrupted") +var ( + ErrInterrupted = xerrors.New("chat interrupted") + + errStartupTimeout = xerrors.New( + "chat response did not start before the startup timeout", + ) +) // PersistedStep contains the full content of a completed or // interrupted agent step. Content includes both assistant blocks @@ -57,6 +67,11 @@ type RunOptions struct { Messages []fantasy.Message Tools []fantasy.AgentTool MaxSteps int + // StartupTimeout bounds how long each model attempt may + // spend opening the provider stream and waiting for its + // first stream part before the attempt is canceled and + // retried. Zero uses the production default. + StartupTimeout time.Duration ActiveTools []string ContextLimitFallback int64 @@ -88,10 +103,11 @@ type RunOptions struct { // OnRetry is called before each retry attempt when the LLM // stream fails with a retryable error. It provides the attempt - // number, error, and backoff delay so callers can publish status - // events to connected clients. Callers should also clear any - // buffered stream state from the failed attempt in this callback - // to avoid sending duplicated content. + // number, raw error, normalized classification, and backoff + // delay so callers can publish status events to connected + // clients. Callers should also clear any buffered stream state + // from the failed attempt in this callback to avoid sending + // duplicated content. OnRetry chatretry.OnRetryFn OnInterruptedPersistError func(error) @@ -234,6 +250,9 @@ func Run(ctx context.Context, opts RunOptions) error { if opts.MaxSteps <= 0 { opts.MaxSteps = 1 } + if opts.StartupTimeout <= 0 { + opts.StartupTimeout = defaultStartupTimeout + } publishMessagePart := func(role codersdk.ChatMessageRole, part codersdk.ChatMessagePart) { if opts.PublishMessagePart == nil { @@ -306,19 +325,37 @@ func Run(ctx context.Context, opts RunOptions) error { var result stepResult err := chatretry.Retry(ctx, func(retryCtx context.Context) error { - stream, streamErr := opts.Model.Stream(retryCtx, call) + attempt, streamErr := guardedStream( + retryCtx, + opts.Model.Provider(), + opts.StartupTimeout, + func(attemptCtx context.Context) (fantasy.StreamResponse, error) { + return opts.Model.Stream(attemptCtx, call) + }, + ) if streamErr != nil { return streamErr } + defer attempt.release() var processErr error - result, processErr = processStepStream(retryCtx, stream, publishMessagePart) - return processErr - }, func(attempt int, retryErr error, delay time.Duration) { + result, processErr = processStepStream( + attempt.ctx, + attempt.stream, + publishMessagePart, + ) + return attempt.finish(processErr) + }, func( + attempt int, + retryErr error, + classified chatretry.ClassifiedError, + delay time.Duration, + ) { // Reset result from the failed attempt so the next // attempt starts clean. result = stepResult{} if opts.OnRetry != nil { - opts.OnRetry(attempt, retryErr, delay) + classified = classified.WithProvider(opts.Model.Provider()) + opts.OnRetry(attempt, retryErr, classified, delay) } }) if err != nil { @@ -514,6 +551,105 @@ func Run(ctx context.Context, opts RunOptions) error { return nil } +// guardedAttempt owns an attempt-scoped context and startup guard +// around a provider stream. release is idempotent and frees the +// attempt-scoped timer/context. finish canonicalizes startup timeout +// errors before the retry loop classifies them. +type guardedAttempt struct { + ctx context.Context + stream fantasy.StreamResponse + release func() + finish func(error) error +} + +// startupGuard arbitrates whether an attempt times out during +// stream startup. Exactly one outcome wins: the timer cancels +// the attempt, or the first-part path disarms the timer. +type startupGuard struct { + timer *time.Timer + cancel context.CancelCauseFunc + once sync.Once +} + +func newStartupGuard( + timeout time.Duration, + cancel context.CancelCauseFunc, +) *startupGuard { + guard := &startupGuard{cancel: cancel} + guard.timer = time.AfterFunc(timeout, guard.onTimeout) + return guard +} + +func (g *startupGuard) onTimeout() { + g.once.Do(func() { + g.cancel(errStartupTimeout) + }) +} + +func (g *startupGuard) Disarm() { + g.once.Do(func() { + g.timer.Stop() + }) +} + +func classifyStartupTimeout( + attemptCtx context.Context, + provider string, + err error, +) error { + if !errors.Is(context.Cause(attemptCtx), errStartupTimeout) { + return err + } + if err == nil { + err = errStartupTimeout + } + return chaterror.WithClassification(err, chaterror.ClassifiedError{ + Kind: chaterror.KindStartupTimeout, + Provider: provider, + Retryable: true, + }) +} + +func guardedStream( + parent context.Context, + provider string, + timeout time.Duration, + openStream func(context.Context) (fantasy.StreamResponse, error), +) (guardedAttempt, error) { + attemptCtx, cancelAttempt := context.WithCancelCause(parent) + guard := newStartupGuard(timeout, cancelAttempt) + var releaseOnce sync.Once + release := func() { + releaseOnce.Do(func() { + guard.Disarm() + cancelAttempt(nil) + }) + } + + stream, err := openStream(attemptCtx) + if err != nil { + err = classifyStartupTimeout(attemptCtx, provider, err) + release() + return guardedAttempt{}, err + } + + return guardedAttempt{ + ctx: attemptCtx, + stream: fantasy.StreamResponse(func(yield func(fantasy.StreamPart) bool) { + for part := range stream { + guard.Disarm() + if !yield(part) { + return + } + } + }), + release: release, + finish: func(err error) error { + return classifyStartupTimeout(attemptCtx, provider, err) + }, + }, nil +} + // processStepStream consumes a fantasy StreamResponse and // accumulates all content into a stepResult. Callbacks fire // inline and their errors propagate directly. @@ -703,7 +839,6 @@ func processStepStream( ) return result, ErrInterrupted } - hasLocalToolCalls := false for _, tc := range result.toolCalls { if !tc.ProviderExecuted { diff --git a/coderd/x/chatd/chatloop/chatloop_test.go b/coderd/x/chatd/chatloop/chatloop_test.go index b0e7e46aa2..add8117302 100644 --- a/coderd/x/chatd/chatloop/chatloop_test.go +++ b/coderd/x/chatd/chatloop/chatloop_test.go @@ -6,6 +6,7 @@ import ( "iter" "strings" "sync" + "sync/atomic" "testing" "time" @@ -14,6 +15,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" + "github.com/coder/coder/v2/coderd/x/chatd/chatretry" + "github.com/coder/coder/v2/codersdk" ) const activeToolName = "read_file" @@ -81,6 +86,401 @@ func TestRun_ActiveToolsPrepareBehavior(t *testing.T) { require.True(t, hasAnthropicEphemeralCacheControl(capturedCall.Prompt[4])) } +func TestRun_OnRetryEnrichesProvider(t *testing.T) { + t.Parallel() + + type retryRecord struct { + attempt int + errMsg string + classified chatretry.ClassifiedError + delay time.Duration + } + + var records []retryRecord + calls := 0 + model := &loopTestModel{ + provider: "openai", + streamFn: func(_ context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + calls++ + if calls == 1 { + return nil, xerrors.New("received status 429 from upstream") + } + return streamFromParts([]fantasy.StreamPart{{ + Type: fantasy.StreamPartTypeFinish, + FinishReason: fantasy.FinishReasonStop, + }}), nil + }, + } + + err := Run(context.Background(), RunOptions{ + Model: model, + MaxSteps: 1, + ContextLimitFallback: 4096, + PersistStep: func(_ context.Context, _ PersistedStep) error { + return nil + }, + OnRetry: func( + attempt int, + retryErr error, + classified chatretry.ClassifiedError, + delay time.Duration, + ) { + records = append(records, retryRecord{ + attempt: attempt, + errMsg: retryErr.Error(), + classified: classified, + delay: delay, + }) + }, + }) + require.NoError(t, err) + require.Len(t, records, 1) + require.Equal(t, 1, records[0].attempt) + require.Equal(t, "received status 429 from upstream", records[0].errMsg) + require.Equal(t, chatretry.Delay(0), records[0].delay) + require.Equal(t, "openai", records[0].classified.Provider) + require.Equal(t, chaterror.KindRateLimit, records[0].classified.Kind) + require.True(t, records[0].classified.Retryable) + require.Equal(t, 429, records[0].classified.StatusCode) + require.Equal( + t, + "OpenAI is rate limiting requests (HTTP 429). Please try again later.", + records[0].classified.Message, + ) +} + +func TestStartupGuard_DisarmAndFireRace(t *testing.T) { + t.Parallel() + + for range 128 { + var cancels atomic.Int32 + guard := newStartupGuard(time.Hour, func(err error) { + if errors.Is(err, errStartupTimeout) { + cancels.Add(1) + } + }) + + start := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + <-start + guard.onTimeout() + }() + + go func() { + defer wg.Done() + <-start + guard.Disarm() + }() + + close(start) + wg.Wait() + + guard.onTimeout() + guard.Disarm() + + require.LessOrEqual(t, cancels.Load(), int32(1)) + } +} + +func TestStartupGuard_DisarmPreservesPermanentError(t *testing.T) { + t.Parallel() + + attemptCtx, cancelAttempt := context.WithCancelCause(context.Background()) + defer cancelAttempt(nil) + + guard := newStartupGuard(time.Hour, cancelAttempt) + guard.Disarm() + guard.onTimeout() + + classified := chaterror.Classify(classifyStartupTimeout( + attemptCtx, + "openai", + xerrors.New("invalid model"), + )) + require.Equal(t, chaterror.KindConfig, classified.Kind) + require.False(t, classified.Retryable) + require.Nil(t, context.Cause(attemptCtx)) +} + +func TestRun_RetriesStartupTimeoutWhileOpeningStream(t *testing.T) { + t.Parallel() + + const startupTimeout = 5 * time.Millisecond + + attempts := 0 + attemptCause := make(chan error, 1) + var retries []chatretry.ClassifiedError + model := &loopTestModel{ + provider: "openai", + streamFn: func(ctx context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + attempts++ + if attempts == 1 { + <-ctx.Done() + attemptCause <- context.Cause(ctx) + return nil, ctx.Err() + } + return streamFromParts([]fantasy.StreamPart{{ + Type: fantasy.StreamPartTypeFinish, + FinishReason: fantasy.FinishReasonStop, + }}), nil + }, + } + + err := Run(context.Background(), RunOptions{ + Model: model, + MaxSteps: 1, + StartupTimeout: startupTimeout, + PersistStep: func(_ context.Context, _ PersistedStep) error { + return nil + }, + OnRetry: func( + _ int, + _ error, + classified chatretry.ClassifiedError, + _ time.Duration, + ) { + retries = append(retries, classified) + }, + }) + require.NoError(t, err) + require.Equal(t, 2, attempts) + require.Len(t, retries, 1) + require.Equal(t, chaterror.KindStartupTimeout, retries[0].Kind) + require.True(t, retries[0].Retryable) + require.Equal(t, "openai", retries[0].Provider) + require.Equal( + t, + "OpenAI did not start responding in time. Please try again.", + retries[0].Message, + ) + require.ErrorIs(t, <-attemptCause, errStartupTimeout) +} + +func TestRun_RetriesStartupTimeoutBeforeFirstPart(t *testing.T) { + t.Parallel() + + const startupTimeout = 5 * time.Millisecond + + attempts := 0 + attemptCause := make(chan error, 1) + var retries []chatretry.ClassifiedError + model := &loopTestModel{ + provider: "openai", + streamFn: func(ctx context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + attempts++ + if attempts == 1 { + return iter.Seq[fantasy.StreamPart](func(yield func(fantasy.StreamPart) bool) { + <-ctx.Done() + attemptCause <- context.Cause(ctx) + _ = yield(fantasy.StreamPart{ + Type: fantasy.StreamPartTypeError, + Error: ctx.Err(), + }) + }), nil + } + return streamFromParts([]fantasy.StreamPart{{ + Type: fantasy.StreamPartTypeFinish, + FinishReason: fantasy.FinishReasonStop, + }}), nil + }, + } + + err := Run(context.Background(), RunOptions{ + Model: model, + MaxSteps: 1, + StartupTimeout: startupTimeout, + PersistStep: func(_ context.Context, _ PersistedStep) error { + return nil + }, + OnRetry: func( + _ int, + _ error, + classified chatretry.ClassifiedError, + _ time.Duration, + ) { + retries = append(retries, classified) + }, + }) + require.NoError(t, err) + require.Equal(t, 2, attempts) + require.Len(t, retries, 1) + require.Equal(t, chaterror.KindStartupTimeout, retries[0].Kind) + require.True(t, retries[0].Retryable) + require.Equal(t, "openai", retries[0].Provider) + require.Equal( + t, + "OpenAI did not start responding in time. Please try again.", + retries[0].Message, + ) + require.ErrorIs(t, <-attemptCause, errStartupTimeout) +} + +func TestRun_FirstPartDisarmsStartupTimeout(t *testing.T) { + t.Parallel() + + const startupTimeout = 5 * time.Millisecond + + attempts := 0 + retried := false + model := &loopTestModel{ + provider: "openai", + streamFn: func(ctx context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + attempts++ + return iter.Seq[fantasy.StreamPart](func(yield func(fantasy.StreamPart) bool) { + if !yield(fantasy.StreamPart{Type: fantasy.StreamPartTypeTextStart, ID: "text-1"}) { + return + } + + timer := time.NewTimer(startupTimeout * 2) + defer timer.Stop() + + select { + case <-ctx.Done(): + _ = yield(fantasy.StreamPart{ + Type: fantasy.StreamPartTypeError, + Error: ctx.Err(), + }) + return + case <-timer.C: + } + + parts := []fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeTextDelta, ID: "text-1", Delta: "done"}, + {Type: fantasy.StreamPartTypeTextEnd, ID: "text-1"}, + {Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonStop}, + } + for _, part := range parts { + if !yield(part) { + return + } + } + }), nil + }, + } + + err := Run(context.Background(), RunOptions{ + Model: model, + MaxSteps: 1, + StartupTimeout: startupTimeout, + PersistStep: func(_ context.Context, _ PersistedStep) error { + return nil + }, + OnRetry: func( + _ int, + _ error, + _ chatretry.ClassifiedError, + _ time.Duration, + ) { + retried = true + }, + }) + require.NoError(t, err) + require.Equal(t, 1, attempts) + require.False(t, retried) +} + +func TestRun_PanicInPublishMessagePartReleasesAttempt(t *testing.T) { + t.Parallel() + + attemptReleased := make(chan struct{}) + model := &loopTestModel{ + provider: "openai", + streamFn: func(ctx context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + go func() { + <-ctx.Done() + close(attemptReleased) + }() + return streamFromParts([]fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeTextStart, ID: "text-1"}, + {Type: fantasy.StreamPartTypeTextDelta, ID: "text-1", Delta: "boom"}, + }), nil + }, + } + + defer func() { + r := recover() + require.NotNil(t, r) + select { + case <-attemptReleased: + case <-time.After(time.Second): + t.Fatal("attempt context was not released after panic") + } + }() + + _ = Run(context.Background(), RunOptions{ + Model: model, + MaxSteps: 1, + ContextLimitFallback: 4096, + PersistStep: func(_ context.Context, _ PersistedStep) error { + return nil + }, + PublishMessagePart: func(codersdk.ChatMessageRole, codersdk.ChatMessagePart) { + panic("publish panic") + }, + }) + + t.Fatal("expected Run to panic") +} + +func TestRun_RetriesStartupTimeoutWhenStreamClosesSilently(t *testing.T) { + t.Parallel() + + const startupTimeout = 5 * time.Millisecond + + attempts := 0 + attemptCause := make(chan error, 1) + var retries []chatretry.ClassifiedError + model := &loopTestModel{ + provider: "openai", + streamFn: func(ctx context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + attempts++ + if attempts == 1 { + return iter.Seq[fantasy.StreamPart](func(yield func(fantasy.StreamPart) bool) { + <-ctx.Done() + attemptCause <- context.Cause(ctx) + }), nil + } + return streamFromParts([]fantasy.StreamPart{{ + Type: fantasy.StreamPartTypeFinish, + FinishReason: fantasy.FinishReasonStop, + }}), nil + }, + } + + err := Run(context.Background(), RunOptions{ + Model: model, + MaxSteps: 1, + StartupTimeout: startupTimeout, + PersistStep: func(_ context.Context, _ PersistedStep) error { + return nil + }, + OnRetry: func( + _ int, + _ error, + classified chatretry.ClassifiedError, + _ time.Duration, + ) { + retries = append(retries, classified) + }, + }) + require.NoError(t, err) + require.Equal(t, 2, attempts) + require.Len(t, retries, 1) + require.Equal(t, chaterror.KindStartupTimeout, retries[0].Kind) + require.True(t, retries[0].Retryable) + require.Equal(t, "openai", retries[0].Provider) + require.Equal( + t, + "OpenAI did not start responding in time. Please try again.", + retries[0].Message, + ) + require.ErrorIs(t, <-attemptCause, errStartupTimeout) +} + func TestRun_InterruptedStepPersistsSyntheticToolResult(t *testing.T) { t.Parallel() diff --git a/coderd/x/chatd/chatretry/chatretry.go b/coderd/x/chatd/chatretry/chatretry.go index 6b51916f93..ea573b0ecb 100644 --- a/coderd/x/chatd/chatretry/chatretry.go +++ b/coderd/x/chatd/chatretry/chatretry.go @@ -5,11 +5,11 @@ package chatretry import ( "context" - "errors" - "strings" "time" "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" ) const ( @@ -28,90 +28,12 @@ const ( MaxAttempts = 25 ) -// nonRetryablePatterns are substrings that indicate a permanent error -// which should not be retried. These are checked first so that -// ambiguous messages (e.g. "bad request: rate limit") are correctly -// classified as non-retryable. -var nonRetryablePatterns = []string{ - "context canceled", - "context deadline exceeded", - "authentication", - "unauthorized", - "forbidden", - "invalid api key", - "invalid_api_key", - "invalid model", - "model not found", - "model_not_found", - "context length exceeded", - "context_exceeded", - "maximum context length", - "quota", - "billing", -} - -// retryablePatterns are substrings that indicate a transient error -// worth retrying. -var retryablePatterns = []string{ - "overloaded", - "rate limit", - "rate_limit", - "too many requests", - "server error", - "status 500", - "status 502", - "status 503", - "status 529", - "connection reset", - "connection refused", - "eof", - "broken pipe", - "timeout", - "unavailable", - "service unavailable", -} +type ClassifiedError = chaterror.ClassifiedError // IsRetryable determines whether an error from an LLM provider is -// transient and worth retrying. It inspects the error message and -// any wrapped HTTP status codes for known retryable patterns. +// transient and worth retrying. func IsRetryable(err error) bool { - if err == nil { - return false - } - - // context.Canceled is always non-retryable regardless of - // wrapping. - if errors.Is(err, context.Canceled) { - return false - } - - lower := strings.ToLower(err.Error()) - - // Check non-retryable patterns first so they take precedence. - for _, p := range nonRetryablePatterns { - if strings.Contains(lower, p) { - return false - } - } - - for _, p := range retryablePatterns { - if strings.Contains(lower, p) { - return true - } - } - - return false -} - -// StatusCodeRetryable returns true for HTTP status codes that -// indicate a transient failure worth retrying. -func StatusCodeRetryable(code int) bool { - switch code { - case 429, 500, 502, 503, 529: - return true - default: - return false - } + return chaterror.Classify(err).Retryable } // Delay returns the backoff duration for the given 0-indexed attempt. @@ -134,9 +56,9 @@ func Delay(attempt int) time.Duration { type RetryFn func(ctx context.Context) error // OnRetryFn is called before each retry attempt with the attempt -// number (1-indexed), the error that triggered the retry, and the -// delay before the next attempt. -type OnRetryFn func(attempt int, err error, delay time.Duration) +// number (1-indexed), the raw error that triggered the retry, the +// normalized error payload, and the delay before the next attempt. +type OnRetryFn func(attempt int, err error, classified ClassifiedError, delay time.Duration) // Retry calls fn repeatedly until it succeeds, returns a // non-retryable error, ctx is canceled, or MaxAttempts is reached. @@ -153,8 +75,9 @@ func Retry(ctx context.Context, fn RetryFn, onRetry OnRetryFn) error { return nil } - if !IsRetryable(err) { - return err + classified := chaterror.Classify(err) + if !classified.Retryable { + return chaterror.WithClassification(err, classified) } // If the caller's context is already done, return the @@ -165,13 +88,16 @@ func Retry(ctx context.Context, fn RetryFn, onRetry OnRetryFn) error { attempt++ if attempt >= MaxAttempts { - return xerrors.Errorf("max retry attempts (%d) exceeded: %w", MaxAttempts, err) + return chaterror.WithClassification( + xerrors.Errorf("max retry attempts (%d) exceeded: %w", MaxAttempts, err), + classified, + ) } delay := Delay(attempt - 1) if onRetry != nil { - onRetry(attempt, err, delay) + onRetry(attempt, err, classified, delay) } timer := time.NewTimer(delay) diff --git a/coderd/x/chatd/chatretry/chatretry_test.go b/coderd/x/chatd/chatretry/chatretry_test.go index df4659d23f..67ced0180d 100644 --- a/coderd/x/chatd/chatretry/chatretry_test.go +++ b/coderd/x/chatd/chatretry/chatretry_test.go @@ -8,12 +8,14 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" "golang.org/x/xerrors" + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" "github.com/coder/coder/v2/coderd/x/chatd/chatretry" ) -func TestIsRetryable(t *testing.T) { +func TestIsRetryableDelegatesToClassification(t *testing.T) { t.Parallel() tests := []struct { @@ -21,220 +23,36 @@ func TestIsRetryable(t *testing.T) { err error retryable bool }{ - // Retryable errors. - { - name: "Overloaded", - err: xerrors.New("model is overloaded, please try again"), - retryable: true, - }, - { - name: "RateLimit", - err: xerrors.New("rate limit exceeded"), - retryable: true, - }, - { - name: "RateLimitUnderscore", - err: xerrors.New("rate_limit: too many requests"), - retryable: true, - }, - { - name: "TooManyRequests", - err: xerrors.New("too many requests"), - retryable: true, - }, - { - name: "HTTP429InMessage", - err: xerrors.New("received status 429 from upstream"), - retryable: false, // "429" alone is not a pattern; needs matching text. - }, - { - name: "HTTP529InMessage", - err: xerrors.New("received status 529 from upstream"), - retryable: true, - }, - { - name: "ServerError500", - err: xerrors.New("status 500: internal server error"), - retryable: true, - }, - { - name: "ServerErrorGeneric", - err: xerrors.New("server error"), - retryable: true, - }, - { - name: "ConnectionReset", - err: xerrors.New("read tcp: connection reset by peer"), - retryable: true, - }, - { - name: "ConnectionRefused", - err: xerrors.New("dial tcp: connection refused"), - retryable: true, - }, - { - name: "EOF", - err: xerrors.New("unexpected EOF"), - retryable: true, - }, - { - name: "BrokenPipe", - err: xerrors.New("write: broken pipe"), - retryable: true, - }, - { - name: "NetworkTimeout", - err: xerrors.New("i/o timeout"), - retryable: true, - }, - { - name: "ServiceUnavailable", - err: xerrors.New("service unavailable"), - retryable: true, - }, - { - name: "Unavailable", - err: xerrors.New("the service is currently unavailable"), - retryable: true, - }, - { - name: "Status502", - err: xerrors.New("status 502: bad gateway"), - retryable: true, - }, - { - name: "Status503", - err: xerrors.New("status 503"), - retryable: true, - }, - - // Non-retryable errors. - { - name: "Nil", - err: nil, - retryable: false, - }, - { - name: "ContextCanceled", - err: context.Canceled, - retryable: false, - }, - { - name: "ContextCanceledWrapped", - err: xerrors.Errorf("operation failed: %w", context.Canceled), - retryable: false, - }, - { - name: "ContextCanceledMessage", - err: xerrors.New("context canceled"), - retryable: false, - }, - { - name: "ContextDeadlineExceeded", - err: xerrors.New("context deadline exceeded"), - retryable: false, - }, - { - name: "Authentication", - err: xerrors.New("authentication failed"), - retryable: false, - }, - { - name: "Unauthorized", - err: xerrors.New("401 Unauthorized"), - retryable: false, - }, - { - name: "Forbidden", - err: xerrors.New("403 Forbidden"), - retryable: false, - }, - { - name: "InvalidAPIKey", - err: xerrors.New("invalid api key"), - retryable: false, - }, - { - name: "InvalidAPIKeyUnderscore", - err: xerrors.New("invalid_api_key"), - retryable: false, - }, - { - name: "InvalidModel", - err: xerrors.New("invalid model: gpt-5-turbo"), - retryable: false, - }, - { - name: "ModelNotFound", - err: xerrors.New("model not found"), - retryable: false, - }, - { - name: "ModelNotFoundUnderscore", - err: xerrors.New("model_not_found"), - retryable: false, - }, - { - name: "ContextLengthExceeded", - err: xerrors.New("context length exceeded"), - retryable: false, - }, - { - name: "ContextExceededUnderscore", - err: xerrors.New("context_exceeded"), - retryable: false, - }, - { - name: "MaximumContextLength", - err: xerrors.New("maximum context length"), - retryable: false, - }, - { - name: "QuotaExceeded", - err: xerrors.New("quota exceeded"), - retryable: false, - }, - { - name: "BillingError", - err: xerrors.New("billing issue: payment required"), - retryable: false, - }, - - // Wrapped errors preserve retryability. - { - name: "WrappedRetryable", - err: xerrors.Errorf("provider call failed: %w", xerrors.New("service unavailable")), - retryable: true, - }, - { - name: "WrappedNonRetryable", - err: xerrors.Errorf("provider call failed: %w", xerrors.New("invalid api key")), - retryable: false, - }, + {name: "Nil", err: nil, retryable: false}, + {name: "RetryableExplicitStatus429", err: xerrors.New("received status 429 from upstream"), retryable: true}, + {name: "RetryableTimeout", err: xerrors.New("service unavailable"), retryable: true}, + {name: "NonRetryableAuth", err: xerrors.New("invalid api key"), retryable: false}, + {name: "NonRetryableGeneric", err: xerrors.New("boom"), retryable: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := chatretry.IsRetryable(tt.err) - if got != tt.retryable { - t.Errorf("IsRetryable(%v) = %v, want %v", tt.err, got, tt.retryable) - } + + require.Equal(t, tt.retryable, chatretry.IsRetryable(tt.err)) + require.Equal(t, chaterror.Classify(tt.err).Retryable, chatretry.IsRetryable(tt.err)) }) } } -func TestStatusCodeRetryable(t *testing.T) { +func TestRetryabilityFromClassifyStatusCodes(t *testing.T) { t.Parallel() tests := []struct { code int retryable bool }{ + {408, true}, {429, true}, {500, true}, {502, true}, {503, true}, + {504, true}, {529, true}, {200, false}, {400, false}, @@ -246,10 +64,11 @@ func TestStatusCodeRetryable(t *testing.T) { for _, tt := range tests { t.Run(fmt.Sprintf("Status%d", tt.code), func(t *testing.T) { t.Parallel() - got := chatretry.StatusCodeRetryable(tt.code) - if got != tt.retryable { - t.Errorf("StatusCodeRetryable(%d) = %v, want %v", tt.code, got, tt.retryable) - } + + err := xerrors.Errorf("status %d from upstream", tt.code) + classified := chaterror.Classify(err) + require.Equal(t, tt.retryable, classified.Retryable) + require.Equal(t, classified.Retryable, chatretry.IsRetryable(err)) }) } } @@ -267,8 +86,8 @@ func TestDelay(t *testing.T) { {3, 8 * time.Second}, {4, 16 * time.Second}, {5, 32 * time.Second}, - {6, 60 * time.Second}, // Capped at MaxDelay. - {10, 60 * time.Second}, // Still capped. + {6, 60 * time.Second}, + {10, 60 * time.Second}, {100, 60 * time.Second}, } @@ -291,12 +110,8 @@ func TestRetry_SuccessOnFirstTry(t *testing.T) { calls++ return nil }, nil) - if err != nil { - t.Fatalf("expected nil error, got %v", err) - } - if calls != 1 { - t.Fatalf("expected fn called once, got %d", calls) - } + require.NoError(t, err) + require.Equal(t, 1, calls) } func TestRetry_TransientThenSuccess(t *testing.T) { @@ -310,12 +125,8 @@ func TestRetry_TransientThenSuccess(t *testing.T) { } return nil }, nil) - if err != nil { - t.Fatalf("expected nil error, got %v", err) - } - if calls != 2 { - t.Fatalf("expected fn called twice, got %d", calls) - } + require.NoError(t, err) + require.Equal(t, 2, calls) } func TestRetry_MultipleTransientThenSuccess(t *testing.T) { @@ -329,12 +140,8 @@ func TestRetry_MultipleTransientThenSuccess(t *testing.T) { } return nil }, nil) - if err != nil { - t.Fatalf("expected nil error, got %v", err) - } - if calls != 4 { - t.Fatalf("expected fn called 4 times, got %d", calls) - } + require.NoError(t, err) + require.Equal(t, 4, calls) } func TestRetry_NonRetryableError(t *testing.T) { @@ -346,15 +153,14 @@ func TestRetry_NonRetryableError(t *testing.T) { return xerrors.New("invalid api key") }, nil) - if err == nil { - t.Fatal("expected error, got nil") - } - if err.Error() != "invalid api key" { - t.Fatalf("expected 'invalid api key', got %q", err.Error()) - } - if calls != 1 { - t.Fatalf("expected fn called once, got %d", calls) - } + require.Error(t, err) + require.EqualError(t, err, "invalid api key") + require.Equal(t, 1, calls) + require.Equal( + t, + chaterror.Classify(xerrors.New("invalid api key")), + chaterror.Classify(err), + ) } func TestRetry_ContextCanceledDuringWait(t *testing.T) { @@ -365,8 +171,6 @@ func TestRetry_ContextCanceledDuringWait(t *testing.T) { calls := 0 err := chatretry.Retry(ctx, func(_ context.Context) error { calls++ - // Cancel after the first retryable error so the wait - // select picks up the cancellation. if calls == 1 { cancel() } @@ -385,8 +189,6 @@ func TestRetry_ContextCanceledDuringFn(t *testing.T) { err := chatretry.Retry(ctx, func(_ context.Context) error { cancel() - // Return a retryable error; the loop should detect that - // ctx is done and return the context error. return xerrors.New("overloaded") }, nil) @@ -399,9 +201,10 @@ func TestRetry_OnRetryCalledWithCorrectArgs(t *testing.T) { t.Parallel() type retryRecord struct { - attempt int - errMsg string - delay time.Duration + attempt int + errMsg string + classified chatretry.ClassifiedError + delay time.Duration } var records []retryRecord @@ -409,31 +212,33 @@ func TestRetry_OnRetryCalledWithCorrectArgs(t *testing.T) { err := chatretry.Retry(context.Background(), func(_ context.Context) error { calls++ if calls <= 2 { - return xerrors.New("rate limit exceeded") + return xerrors.New("received status 429 from upstream") } return nil - }, func(attempt int, err error, delay time.Duration) { + }, func( + attempt int, + err error, + classified chatretry.ClassifiedError, + delay time.Duration, + ) { records = append(records, retryRecord{ - attempt: attempt, - errMsg: err.Error(), - delay: delay, + attempt: attempt, + errMsg: err.Error(), + classified: classified, + delay: delay, }) }) - if err != nil { - t.Fatalf("expected nil error, got %v", err) - } - if len(records) != 2 { - t.Fatalf("expected 2 onRetry calls, got %d", len(records)) - } - if records[0].attempt != 1 { - t.Errorf("first onRetry attempt = %d, want 1", records[0].attempt) - } - if records[1].attempt != 2 { - t.Errorf("second onRetry attempt = %d, want 2", records[1].attempt) - } - if records[0].errMsg != "rate limit exceeded" { - t.Errorf("first onRetry error = %q, want 'rate limit exceeded'", records[0].errMsg) - } + require.NoError(t, err) + require.Len(t, records, 2) + + expected := chaterror.Classify(xerrors.New("received status 429 from upstream")) + require.Equal(t, 1, records[0].attempt) + require.Equal(t, 2, records[1].attempt) + require.Equal(t, "received status 429 from upstream", records[0].errMsg) + require.Equal(t, expected, records[0].classified) + require.Equal(t, expected, records[1].classified) + require.Equal(t, chatretry.Delay(0), records[0].delay) + require.Equal(t, chatretry.Delay(1), records[1].delay) } func TestRetry_OnRetryNilDoesNotPanic(t *testing.T) { diff --git a/codersdk/chats.go b/codersdk/chats.go index ecc26d6045..1e6d712da3 100644 --- a/codersdk/chats.go +++ b/codersdk/chats.go @@ -778,7 +778,16 @@ type ChatStreamStatus struct { // ChatStreamError represents an error event in the stream. type ChatStreamError struct { + // Message is the normalized, user-facing error message. Message string `json:"message"` + // Kind classifies the error for consistent client rendering. + Kind string `json:"kind,omitempty"` + // Provider identifies the upstream model provider when known. + Provider string `json:"provider,omitempty"` + // Retryable reports whether the underlying error is transient. + Retryable bool `json:"retryable"` + // StatusCode is the best-effort upstream HTTP status code. + StatusCode int `json:"status_code,omitempty"` } // ChatStreamRetry represents an auto-retry status event in the stream. @@ -788,8 +797,14 @@ type ChatStreamRetry struct { Attempt int `json:"attempt"` // DelayMs is the backoff delay in milliseconds before the retry. DelayMs int64 `json:"delay_ms"` - // Error is the error message from the failed attempt. + // Error is the normalized error message from the failed attempt. Error string `json:"error"` + // Kind classifies the retry reason for consistent client rendering. + Kind string `json:"kind,omitempty"` + // Provider identifies the upstream model provider when known. + Provider string `json:"provider,omitempty"` + // StatusCode is the best-effort upstream HTTP status code. + StatusCode int `json:"status_code,omitempty"` // RetryingAt is the timestamp when the retry will be attempted. RetryingAt time.Time `json:"retrying_at" format:"date-time"` } diff --git a/enterprise/coderd/x/chatd/chatd_test.go b/enterprise/coderd/x/chatd/chatd_test.go index eef216b143..f54b9952d8 100644 --- a/enterprise/coderd/x/chatd/chatd_test.go +++ b/enterprise/coderd/x/chatd/chatd_test.go @@ -63,6 +63,28 @@ func newTestServer( return server } +func newActiveWorkerServer( + t *testing.T, + db database.Store, + ps dbpubsub.Pubsub, + replicaID uuid.UUID, +) *osschatd.Server { + t.Helper() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + server := osschatd.New(osschatd.Config{ + Logger: logger, + Database: db, + ReplicaID: replicaID, + Pubsub: ps, + PendingChatAcquireInterval: 10 * time.Millisecond, + InFlightChatStaleAfter: testutil.WaitSuperLong, + }) + t.Cleanup(func() { + require.NoError(t, server.Close()) + }) + return server +} + // seedChatDependencies creates a user and chat model config in the // database for use in relay tests. func seedChatDependencies( @@ -99,28 +121,6 @@ func seedChatDependencies( return user, model } -func newActiveWorkerServer( - t *testing.T, - db database.Store, - ps dbpubsub.Pubsub, - replicaID uuid.UUID, -) *osschatd.Server { - t.Helper() - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - server := osschatd.New(osschatd.Config{ - Logger: logger, - Database: db, - ReplicaID: replicaID, - Pubsub: ps, - PendingChatAcquireInterval: 10 * time.Millisecond, - InFlightChatStaleAfter: testutil.WaitSuperLong, - }) - t.Cleanup(func() { - require.NoError(t, server.Close()) - }) - return server -} - func setOpenAIProviderBaseURL( ctx context.Context, t *testing.T, @@ -563,7 +563,10 @@ func TestSubscribeRetryEventAcrossInstances(t *testing.T) { require.NotNil(t, retryEvent) require.Equal(t, 1, retryEvent.Attempt) require.Greater(t, retryEvent.DelayMs, int64(0)) - require.Contains(t, retryEvent.Error, "Rate limit exceeded") + require.Equal(t, "rate_limit", retryEvent.Kind) + require.Equal(t, "openai", retryEvent.Provider) + require.Equal(t, 0, retryEvent.StatusCode) + require.Contains(t, retryEvent.Error, "rate limiting requests") require.False(t, assistantMessageBeforeRetry) require.False(t, waitingBeforeRetry) require.GreaterOrEqual(t, streamCalls.Load(), int32(2)) diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 83b08eb51a..918ac53313 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1763,7 +1763,26 @@ export const ChatStatuses: ChatStatus[] = [ * ChatStreamError represents an error event in the stream. */ export interface ChatStreamError { + /** + * Message is the normalized, user-facing error message. + */ readonly message: string; + /** + * Kind classifies the error for consistent client rendering. + */ + readonly kind?: string; + /** + * Provider identifies the upstream model provider when known. + */ + readonly provider?: string; + /** + * Retryable reports whether the underlying error is transient. + */ + readonly retryable: boolean; + /** + * StatusCode is the best-effort upstream HTTP status code. + */ + readonly status_code?: number; } // From codersdk/chats.go @@ -1823,9 +1842,21 @@ export interface ChatStreamRetry { */ readonly delay_ms: number; /** - * Error is the error message from the failed attempt. + * Error is the normalized error message from the failed attempt. */ readonly error: string; + /** + * Kind classifies the retry reason for consistent client rendering. + */ + readonly kind?: string; + /** + * Provider identifies the upstream model provider when known. + */ + readonly provider?: string; + /** + * StatusCode is the best-effort upstream HTTP status code. + */ + readonly status_code?: number; /** * RetryingAt is the timestamp when the retry will be attempted. */ diff --git a/site/src/pages/AgentsPage/components/AgentDetail/ChatContext.test.tsx b/site/src/pages/AgentsPage/components/AgentDetail/ChatContext.test.tsx index 8730423572..ebfc94e412 100644 --- a/site/src/pages/AgentsPage/components/AgentDetail/ChatContext.test.tsx +++ b/site/src/pages/AgentsPage/components/AgentDetail/ChatContext.test.tsx @@ -1570,7 +1570,7 @@ describe("useChatStore", () => { mockSocket.emitData({ type: "error", chat_id: chatID, - error: { message: "Rate limit exceeded" }, + error: { message: "Rate limit exceeded", retryable: true }, }); }); @@ -1629,7 +1629,7 @@ describe("useChatStore", () => { mockSocket.emitData({ type: "error", chat_id: chatID, - error: { message: " " }, + error: { message: " ", retryable: false }, }); }); @@ -1953,7 +1953,7 @@ describe("useChatStore", () => { mockSocket.emitData({ type: "error", chat_id: chatID, - error: { message: "Rate limit exceeded" }, + error: { message: "Rate limit exceeded", retryable: true }, }); }); @@ -2750,7 +2750,7 @@ describe("updateSidebarChat via stream events", () => { mockSocket.emitData({ type: "error", chat_id: chatID, - error: { message: "something went wrong" }, + error: { message: "something went wrong", retryable: false }, }); }); @@ -3026,7 +3026,7 @@ describe("updateSidebarChat via stream events", () => { mockSocket.emitData({ type: "error", chat_id: chatID, - error: { message: "something broke" }, + error: { message: "something broke", retryable: false }, }); });