mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix: classify HTTP/2 transport failures as retryable timeouts (#24502)
Modifies chatloop error classification behaviour to treat the following as retryable:
* HTTP/2 `force closed`
* GOAWAY
* use of closed network connection
* Modfies user-facing retry banner to show "<provider> is temporarily
unavailable."
Relates to CODAGT-212.
> 🤖
This commit is contained in:
Cian Johnston
@@ -221,6 +221,10 @@ func TestClassify_PatternCoverage(t *testing.T) {
|
||||
{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: "ClientConnLiteral", err: "client conn", wantKind: chaterror.KindTimeout, wantRetry: true},
|
||||
{name: "GOAWAYLiteral", err: "goaway", wantKind: chaterror.KindTimeout, wantRetry: true},
|
||||
{name: "HTTP2StreamClosedLiteral", err: "http2: stream closed", wantKind: chaterror.KindTimeout, wantRetry: true},
|
||||
{name: "UseOfClosedNetworkConnectionLiteral", err: "use of closed network connection", 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},
|
||||
@@ -289,6 +293,168 @@ func TestClassify_TransportFailuresUseBroaderRetryMessage(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestClassify_HTTP2TransportErrors checks HTTP/2 transport errors
|
||||
// classify as retryable KindTimeout. Split into two sub-tables so a
|
||||
// bug in transport matching cannot be masked by provider detection
|
||||
// (and vice versa).
|
||||
func TestClassify_HTTP2TransportErrors(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// Transport patterns, no provider hint. Provider stays empty and
|
||||
// Message uses the generic subject.
|
||||
transportOnly := []struct {
|
||||
name string
|
||||
err string
|
||||
}{
|
||||
{
|
||||
name: "HTTP2ClientConnForceClosed",
|
||||
err: "http2: client connection force closed via ClientConn.Close",
|
||||
},
|
||||
{
|
||||
name: "HTTP2TransportGOAWAY",
|
||||
err: "http2: Transport received Server's graceful shutdown GOAWAY",
|
||||
},
|
||||
{
|
||||
name: "HTTP2ServerGOAWAY",
|
||||
err: "http2: server sent GOAWAY and closed the connection",
|
||||
},
|
||||
{
|
||||
name: "HTTP2StreamClosed",
|
||||
err: "http2: stream closed",
|
||||
},
|
||||
{
|
||||
name: "UseOfClosedNetworkConnectionOnPOST",
|
||||
err: `Post "https://example.com/v1/messages": use of closed network connection`,
|
||||
},
|
||||
{
|
||||
name: "HTTP2ClientConnIsClosed",
|
||||
err: "http2: client conn is closed",
|
||||
},
|
||||
{
|
||||
name: "HTTP2ClientConnNotUsable",
|
||||
err: "http2: client conn not usable",
|
||||
},
|
||||
{
|
||||
name: "HTTP2ClientConnNotEstablished",
|
||||
err: "http2: client conn could not be established",
|
||||
},
|
||||
{
|
||||
name: "HTTP2ClientConnectionLost",
|
||||
err: "http2: client connection lost",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range transportOnly {
|
||||
t.Run("TransportOnly/"+tt.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
classified := chaterror.Classify(xerrors.New(tt.err))
|
||||
require.Equal(t, chaterror.KindTimeout, classified.Kind, "Kind")
|
||||
require.True(t, classified.Retryable, "Retryable")
|
||||
require.Equal(t, "", classified.Provider, "Provider")
|
||||
require.Equal(t,
|
||||
"The AI provider is temporarily unavailable.",
|
||||
classified.Message,
|
||||
"Message",
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
// Same transport signature with a provider host in the URL so
|
||||
// detectProvider can stamp Provider.
|
||||
providerDetection := []struct {
|
||||
name string
|
||||
err string
|
||||
provider string
|
||||
wantMessage string
|
||||
}{
|
||||
{
|
||||
name: "CustomerRegressionAnthropic",
|
||||
err: `stream response: Post "https://api.anthropic.com/v1/messages": http2: client connection force closed via ClientConn.Close`,
|
||||
provider: "anthropic",
|
||||
wantMessage: "Anthropic is temporarily unavailable.",
|
||||
},
|
||||
{
|
||||
name: "OpenAIForceClosed",
|
||||
err: `stream response: Post "https://api.openai.com/v1/chat/completions": http2: client connection force closed via ClientConn.Close`,
|
||||
provider: "openai",
|
||||
wantMessage: "OpenAI is temporarily unavailable.",
|
||||
},
|
||||
{
|
||||
name: "GoogleGOAWAY",
|
||||
err: `stream response: Post "https://generativelanguage.googleapis.com/v1beta/models/gemini-pro:streamGenerateContent": http2: server sent GOAWAY and closed the connection`,
|
||||
provider: "google",
|
||||
wantMessage: "Google is temporarily unavailable.",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range providerDetection {
|
||||
t.Run("ProviderDetection/"+tt.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
classified := chaterror.Classify(xerrors.New(tt.err))
|
||||
require.Equal(t, chaterror.KindTimeout, classified.Kind, "Kind")
|
||||
require.True(t, classified.Retryable, "Retryable")
|
||||
require.Equal(t, tt.provider, classified.Provider, "Provider")
|
||||
require.Equal(t, tt.wantMessage, classified.Message, "Message")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestClassify_StatusCodeBeatsHTTP2Transport ensures explicit status
|
||||
// codes still win over the new HTTP/2 patterns.
|
||||
func TestClassify_StatusCodeBeatsHTTP2Transport(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
err string
|
||||
wantKind string
|
||||
wantRetryable bool
|
||||
wantStatus int
|
||||
}{
|
||||
{
|
||||
name: "HTTP2With429",
|
||||
err: "http2: server error 429 Too Many Requests",
|
||||
wantKind: chaterror.KindRateLimit,
|
||||
wantRetryable: true,
|
||||
wantStatus: 429,
|
||||
},
|
||||
{
|
||||
name: "HTTP2With401",
|
||||
err: "http2: 401 unauthorized",
|
||||
wantKind: chaterror.KindAuth,
|
||||
wantRetryable: false,
|
||||
wantStatus: 401,
|
||||
},
|
||||
{
|
||||
name: "ClientConnWith429RateLimitWins",
|
||||
err: "http2: client conn is closed: status 429 Too Many Requests",
|
||||
wantKind: chaterror.KindRateLimit,
|
||||
wantRetryable: true,
|
||||
wantStatus: 429,
|
||||
},
|
||||
{
|
||||
name: "GOAWAYWith401AuthWins",
|
||||
err: "http2: server sent GOAWAY: status 401 unauthorized",
|
||||
wantKind: chaterror.KindAuth,
|
||||
wantRetryable: false,
|
||||
wantStatus: 401,
|
||||
},
|
||||
}
|
||||
|
||||
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, "Kind")
|
||||
require.Equal(t, tt.wantRetryable, classified.Retryable, "Retryable")
|
||||
require.Equal(t, tt.wantStatus, classified.StatusCode, "StatusCode")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestClassify_StartupTimeoutWrappedClassificationWins(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
@@ -0,0 +1,99 @@
|
||||
package chaterror_test
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/xerrors"
|
||||
|
||||
"github.com/coder/coder/v2/coderd/x/chatd/chaterror"
|
||||
)
|
||||
|
||||
// TestTerminalMessage covers the per-provider "temporarily
|
||||
// unavailable" copy, the startup-timeout copy, and the generic
|
||||
// fallback string for its intended (unclassified, non-retryable)
|
||||
// path.
|
||||
func TestTerminalMessage(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
kind string
|
||||
provider string
|
||||
retryable bool
|
||||
statusCode int
|
||||
want string
|
||||
}{
|
||||
{
|
||||
name: "Timeout_Retryable_Anthropic",
|
||||
kind: chaterror.KindTimeout,
|
||||
provider: "anthropic",
|
||||
retryable: true,
|
||||
want: "Anthropic is temporarily unavailable.",
|
||||
},
|
||||
{
|
||||
name: "Timeout_Retryable_OpenAI",
|
||||
kind: chaterror.KindTimeout,
|
||||
provider: "openai",
|
||||
retryable: true,
|
||||
want: "OpenAI is temporarily unavailable.",
|
||||
},
|
||||
{
|
||||
name: "Timeout_Retryable_UnknownProvider",
|
||||
kind: chaterror.KindTimeout,
|
||||
provider: "",
|
||||
retryable: true,
|
||||
want: "The AI provider is temporarily unavailable.",
|
||||
},
|
||||
{
|
||||
name: "Timeout_NotRetryable_NoStatus",
|
||||
kind: chaterror.KindTimeout,
|
||||
provider: "",
|
||||
retryable: false,
|
||||
want: "The request timed out before it completed.",
|
||||
},
|
||||
{
|
||||
name: "StartupTimeout_Anthropic",
|
||||
kind: chaterror.KindStartupTimeout,
|
||||
provider: "anthropic",
|
||||
retryable: true,
|
||||
want: "Anthropic did not start responding in time.",
|
||||
},
|
||||
{
|
||||
name: "StartupTimeout_OpenAI",
|
||||
kind: chaterror.KindStartupTimeout,
|
||||
provider: "openai",
|
||||
retryable: true,
|
||||
want: "OpenAI did not start responding in time.",
|
||||
},
|
||||
{
|
||||
// Generic fallback reserved for genuinely
|
||||
// unclassified non-retryable failures.
|
||||
name: "Generic_NotRetryable_NoStatus",
|
||||
kind: chaterror.KindGeneric,
|
||||
provider: "",
|
||||
retryable: false,
|
||||
want: "The chat request failed unexpectedly.",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
classified := chaterror.ClassifiedError{
|
||||
Kind: tt.kind,
|
||||
Provider: tt.provider,
|
||||
Retryable: tt.retryable,
|
||||
StatusCode: tt.statusCode,
|
||||
}
|
||||
// terminalMessage is unexported; round-trip through
|
||||
// WithClassification + Classify to exercise it.
|
||||
wrapped := chaterror.WithClassification(
|
||||
xerrors.New(tt.name),
|
||||
classified,
|
||||
)
|
||||
require.Equal(t, tt.want, chaterror.Classify(wrapped).Message)
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -37,6 +37,17 @@ var (
|
||||
"broken pipe",
|
||||
"bad gateway",
|
||||
"gateway timeout",
|
||||
// "client conn" covers all of the stdlib http2 ClientConn errors:
|
||||
// "client conn is closed", "client conn not usable",
|
||||
// "client conn could not be established",
|
||||
// "client connection force closed via ClientConn.Close",
|
||||
// and "client connection lost".
|
||||
"client conn",
|
||||
// Transport-layer failures (HTTP/2 force-closed streams,
|
||||
// GOAWAY, closed network connections) so we retry.
|
||||
"goaway",
|
||||
"http2: stream closed",
|
||||
"use of closed network connection",
|
||||
}
|
||||
authStrongPatterns = []string{
|
||||
"authentication",
|
||||
|
||||
@@ -491,6 +491,85 @@ func TestRun_RetriesStartupTimeoutWhileOpeningStream(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestRun_HTTP2TransportErrorClassifiedAsRetryableTimeout proves the
|
||||
// provider comes from Model.Provider() (not from sniffing the error
|
||||
// text) by using an error string with no provider hint and running
|
||||
// the same assertion across two providers.
|
||||
func TestRun_HTTP2TransportErrorClassifiedAsRetryableTimeout(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
providers := []string{"anthropic", "openai"}
|
||||
for _, provider := range providers {
|
||||
t.Run(provider, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
const startupTimeout = 5 * time.Millisecond
|
||||
|
||||
ctx, cancel := context.WithTimeout(
|
||||
context.Background(),
|
||||
testutil.WaitShort,
|
||||
)
|
||||
defer cancel()
|
||||
|
||||
mClock := quartz.NewMock(t)
|
||||
trap := mClock.Trap().AfterFunc("startupGuard")
|
||||
defer trap.Close()
|
||||
|
||||
attempts := 0
|
||||
var retries []chatretry.ClassifiedError
|
||||
model := &chattest.FakeModel{
|
||||
ProviderName: provider,
|
||||
StreamFn: func(_ context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) {
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
// Bare transport error; Provider must
|
||||
// come from Model.Provider().
|
||||
return nil, xerrors.New(
|
||||
"http2: client connection force closed via ClientConn.Close",
|
||||
)
|
||||
}
|
||||
return streamFromParts([]fantasy.StreamPart{{
|
||||
Type: fantasy.StreamPartTypeFinish,
|
||||
FinishReason: fantasy.FinishReasonStop,
|
||||
}}), nil
|
||||
},
|
||||
}
|
||||
|
||||
done := make(chan error, 1)
|
||||
go func() {
|
||||
done <- Run(context.Background(), RunOptions{
|
||||
Model: model,
|
||||
MaxSteps: 1,
|
||||
StartupTimeout: startupTimeout,
|
||||
Clock: mClock,
|
||||
PersistStep: func(_ context.Context, _ PersistedStep) error {
|
||||
return nil
|
||||
},
|
||||
OnRetry: func(
|
||||
_ int,
|
||||
_ error,
|
||||
classified chatretry.ClassifiedError,
|
||||
_ time.Duration,
|
||||
) {
|
||||
retries = append(retries, classified)
|
||||
},
|
||||
})
|
||||
}()
|
||||
|
||||
// One guard per attempt.
|
||||
trap.MustWait(ctx).MustRelease(ctx)
|
||||
trap.MustWait(ctx).MustRelease(ctx)
|
||||
|
||||
require.NoError(t, awaitRunResult(ctx, t, done))
|
||||
require.Equal(t, 2, attempts)
|
||||
require.Len(t, retries, 1)
|
||||
require.Equal(t, chaterror.KindTimeout, retries[0].Kind, "Kind")
|
||||
require.True(t, retries[0].Retryable, "Retryable")
|
||||
require.Equal(t, provider, retries[0].Provider, "Provider")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestRun_RetriesStartupTimeoutBeforeFirstPart(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
@@ -317,3 +317,24 @@ func TestRetry_UsesRetryAfterAsDelayFloor(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestRetry_HTTP2TransportErrorKeepsRetrying proves a bare HTTP/2
|
||||
// transport error is treated as retryable, so Retry drives one more
|
||||
// attempt instead of returning on the first call.
|
||||
func TestRetry_HTTP2TransportErrorKeepsRetrying(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
calls := 0
|
||||
err := chatretry.Retry(context.Background(), func(_ context.Context) error {
|
||||
calls++
|
||||
if calls == 1 {
|
||||
return xerrors.New(
|
||||
"http2: client connection force closed via ClientConn.Close",
|
||||
)
|
||||
}
|
||||
return nil
|
||||
}, nil)
|
||||
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, 2, calls, "expected one retry after an HTTP/2 transport failure")
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@ import { LiveStreamTailContent } from "./LiveStreamTail";
|
||||
import {
|
||||
buildLiveStatus,
|
||||
buildReconnectState,
|
||||
buildRetryState,
|
||||
buildStreamRenderState,
|
||||
FIXTURE_NOW,
|
||||
textResponseStreamParts,
|
||||
@@ -103,6 +104,91 @@ export const TerminalOverloadedError: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* Transport timeouts render the per-provider "temporarily
|
||||
* unavailable" copy with a "Request timed out" heading rather than
|
||||
* the generic "Request failed" fallback.
|
||||
*/
|
||||
export const TerminalTimeoutErrorAnthropic: Story = {
|
||||
args: {
|
||||
...defaultArgs,
|
||||
liveStatus: buildLiveStatus({
|
||||
streamError: {
|
||||
kind: "timeout",
|
||||
message: "Anthropic is temporarily unavailable.",
|
||||
provider: "anthropic",
|
||||
retryable: false,
|
||||
},
|
||||
}),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
expect(
|
||||
canvas.getByRole("heading", { name: /request timed out/i }),
|
||||
).toBeVisible();
|
||||
expect(
|
||||
canvas.getByText(/anthropic is temporarily unavailable/i),
|
||||
).toBeVisible();
|
||||
// Guard against the pre-fix generic fallback.
|
||||
expect(
|
||||
canvas.queryByText(/the chat request failed unexpectedly/i),
|
||||
).not.toBeInTheDocument();
|
||||
},
|
||||
};
|
||||
|
||||
/** Transport timeout with an unknown provider uses the generic subject. */
|
||||
export const TerminalTimeoutErrorUnknownProvider: Story = {
|
||||
args: {
|
||||
...defaultArgs,
|
||||
liveStatus: buildLiveStatus({
|
||||
streamError: {
|
||||
kind: "timeout",
|
||||
message: "The AI provider is temporarily unavailable.",
|
||||
retryable: false,
|
||||
},
|
||||
}),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
expect(
|
||||
canvas.getByRole("heading", { name: /request timed out/i }),
|
||||
).toBeVisible();
|
||||
expect(
|
||||
canvas.getByText(/the ai provider is temporarily unavailable/i),
|
||||
).toBeVisible();
|
||||
},
|
||||
};
|
||||
|
||||
/** Retrying a transport timeout shows attempt + countdown. */
|
||||
export const RetryingTimeoutAnthropic: Story = {
|
||||
args: {
|
||||
...defaultArgs,
|
||||
liveStatus: buildLiveStatus({
|
||||
retryState: buildRetryState({
|
||||
attempt: 2,
|
||||
kind: "timeout",
|
||||
error: "Anthropic is temporarily unavailable.",
|
||||
provider: "anthropic",
|
||||
}),
|
||||
}),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
expect(
|
||||
canvas.getByRole("heading", { name: /request timed out/i }),
|
||||
).toBeVisible();
|
||||
expect(
|
||||
canvas.getByText(/anthropic is temporarily unavailable/i),
|
||||
).toBeVisible();
|
||||
expect(canvas.getByText(/attempt 2/i)).toBeVisible();
|
||||
// StatusCountdown renders label and seconds as separate text
|
||||
// nodes, so match against the element's combined textContent.
|
||||
await waitFor(() => {
|
||||
expect(canvasElement).toHaveTextContent(/retrying in \d+s/i);
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
/** Terminal startup timeouts get a specific heading without provider metadata. */
|
||||
export const TerminalStartupTimeoutError: Story = {
|
||||
args: {
|
||||
|
||||
Reference in New Issue
Block a user