mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd/x/chatd): retry provider stream cancellations (#26010)
Closes CODAGT-541. ## Problem An Agents chat stream could die with a terminal `context cancelled` error and surface to the user as a permanent chat failure, even when no context in our process had actually been canceled. The cancellation was a provider-returned error value (HTTP/2 RST_STREAM mid-body surfacing as `context.Canceled` from Go's net/http2), not a real caller cancel. The chain that produced the bug: - fantasy passed the provider's `context.Canceled` through unchanged. - `chaterror.Classify` short-circuited any `errors.Is(err, context.Canceled)` (or `"context canceled"` text) as terminal generic, before checking HTTP status codes or other retry signals. - `chatretry.Retry` did not retry. - The frontend rendered `type:"error"` and the chat was dead. The same short-circuit also masked retryable 5xx responses whose underlying transport error happened to wrap `context.Canceled`. ## Approach `context.Canceled` has no inherent intent. The same error value can mean a user pressing Stop, a server shutdown, the silence guard firing, or a provider-side stream reset. The only layer that can disambiguate is the one holding both the returned error and the caller context. That is `chatretry`. This PR centralizes the policy there and keeps `chaterror` context-free. ## Changes `coderd/x/chatd/chaterror/classify.go` - Add `ErrProviderTransportReset` sentinel to explicitly mark provider-side stream cancellations. - Remove the broad `context.Canceled` / `"context canceled"` short-circuit so status codes and other retry signals can win. - Classify `ErrProviderTransportReset` (with no status code) as a retryable timeout. - Keep a fallback that classifies bare `context.Canceled` as terminal-generic when no other signal is present, so legitimate caller cancels still terminate cleanly. `coderd/x/chatd/chatretry/chatretry.go` - Add `contextError(ctx)` that returns `context.Cause(ctx)` when set, falling back to `ctx.Err()`, so caller-owned cancel causes (`ErrInterrupted`, `errStreamSilenceTimeout`, server shutdown sentinels) propagate cleanly out of the retry loop. - Add `classifyProviderAttemptError(err)` that wraps a bare `context.Canceled` in `ErrProviderTransportReset` and reclassifies. Errors that already classify as retryable or carry a status code are left alone. - Restructure `Retry` so the policy is explicit and readable: check caller cancellation before attempting, run the attempt, check caller cancellation again before normalizing the provider error, then classify and retry. ## End-to-end behavior - Provider returns `context.Canceled` while caller context is healthy: classified as a retryable timeout, retried, the user sees a brief `type:"retry"` event and the chat continues. - User presses Stop: `contextError(ctx)` returns `ErrInterrupted`. Retry stops. `chatloop` flushes partial content and persists. - Stream-silence guard fires: `attemptCtx` is canceled with `errStreamSilenceTimeout`, `guardedStream` produces a classified retryable error, retry proceeds normally on the still-alive parent. - Server shutdown: parent context's cause propagates out, retry stops.
This commit is contained in:
@@ -795,6 +795,74 @@ func TestRun_HTTP2TransportErrorClassifiedAsRetryableTimeout(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestRun_RetriesProviderContextCanceledStreamError(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
attempts := 0
|
||||
retryErrs := make(chan error, chatretry.MaxAttempts)
|
||||
retries := make(chan chatretry.ClassifiedError, chatretry.MaxAttempts)
|
||||
var persisted []fantasy.Content
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
model := &chattest.FakeModel{
|
||||
ProviderName: "openai",
|
||||
StreamFn: func(_ context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) {
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
return streamFromParts([]fantasy.StreamPart{
|
||||
{Type: fantasy.StreamPartTypeTextStart, ID: "text-1"},
|
||||
{Type: fantasy.StreamPartTypeTextDelta, ID: "text-1", Delta: "partial"},
|
||||
{Type: fantasy.StreamPartTypeError, Error: context.Canceled},
|
||||
}), nil
|
||||
}
|
||||
return streamFromParts([]fantasy.StreamPart{
|
||||
{Type: fantasy.StreamPartTypeTextStart, ID: "text-2"},
|
||||
{Type: fantasy.StreamPartTypeTextDelta, ID: "text-2", Delta: "done"},
|
||||
{Type: fantasy.StreamPartTypeTextEnd, ID: "text-2"},
|
||||
{Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonStop},
|
||||
}), nil
|
||||
},
|
||||
}
|
||||
|
||||
err := Run(ctx, RunOptions{
|
||||
Model: model,
|
||||
MaxSteps: 1,
|
||||
ContextLimitFallback: 4096,
|
||||
PersistStep: func(_ context.Context, step PersistedStep) error {
|
||||
persisted = append([]fantasy.Content(nil), step.Content...)
|
||||
return nil
|
||||
},
|
||||
OnRetry: func(
|
||||
_ int,
|
||||
retryErr error,
|
||||
classified chatretry.ClassifiedError,
|
||||
_ time.Duration,
|
||||
) {
|
||||
retryErrs <- retryErr
|
||||
retries <- classified
|
||||
},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, 2, attempts)
|
||||
require.Len(t, retryErrs, 1)
|
||||
require.Len(t, retries, 1)
|
||||
retryErr := testutil.RequireReceive(ctx, t, retryErrs)
|
||||
classified := testutil.RequireReceive(ctx, t, retries)
|
||||
require.ErrorIs(t, retryErr, chaterror.ErrProviderTransportReset)
|
||||
require.ErrorIs(t, retryErr, context.Canceled)
|
||||
require.Equal(t, codersdk.ChatErrorKindTimeout, classified.Kind)
|
||||
require.True(t, classified.Retryable)
|
||||
require.Equal(t, "openai", classified.Provider)
|
||||
require.Equal(t, "OpenAI is temporarily unavailable.", classified.Message)
|
||||
|
||||
text := requireTextContent(t, persisted, "done")
|
||||
require.Equal(t, "done", text.Text)
|
||||
for _, block := range persisted {
|
||||
if text, ok := fantasy.AsContentType[fantasy.TextContent](block); ok {
|
||||
require.NotContains(t, text.Text, "partial")
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestRun_RetriesSilenceTimeoutBeforeFirstPart(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
@@ -577,24 +577,30 @@ func TestRun_StreamRetry_RecordsMetric(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
// TestRun_StreamRetry_CanceledDoesNotIncrement pins the invariant
|
||||
// that canceled streams never increment stream_retries_total.
|
||||
// chaterror.Classify routes context.Canceled to
|
||||
// ClassifiedError{Retryable: false}, so chatretry.Retry returns
|
||||
// immediately without calling onRetry. This test guards against
|
||||
// future classification changes that could silently introduce
|
||||
// misleading retry samples.
|
||||
func TestRun_StreamRetry_CanceledDoesNotIncrement(t *testing.T) {
|
||||
// TestRun_StreamRetry_ContextCanceledTransportResetIncrements pins the
|
||||
// invariant that provider-originated context cancellation is counted as
|
||||
// a retryable transport reset when the chat context is still alive.
|
||||
func TestRun_StreamRetry_ContextCanceledTransportResetIncrements(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
reg := prometheus.NewRegistry()
|
||||
metrics := chatloop.NewMetrics(reg)
|
||||
|
||||
attempts := 0
|
||||
model := &chattest.FakeModel{
|
||||
ProviderName: "test-provider",
|
||||
ModelName: "test-model",
|
||||
StreamFn: func(_ context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) {
|
||||
return nil, context.Canceled
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
return nil, context.Canceled
|
||||
}
|
||||
return func(yield func(fantasy.StreamPart) bool) {
|
||||
_ = yield(fantasy.StreamPart{
|
||||
Type: fantasy.StreamPartTypeFinish,
|
||||
FinishReason: fantasy.FinishReasonStop,
|
||||
})
|
||||
}, nil
|
||||
},
|
||||
}
|
||||
|
||||
@@ -607,19 +613,15 @@ func TestRun_StreamRetry_CanceledDoesNotIncrement(t *testing.T) {
|
||||
},
|
||||
Metrics: metrics,
|
||||
})
|
||||
// Expect an error (the stream failed); we don't care which error
|
||||
// kind as long as no retry was recorded.
|
||||
require.Error(t, err)
|
||||
|
||||
families, err := reg.Gather()
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, 2, attempts)
|
||||
|
||||
for _, f := range families {
|
||||
if f.GetName() == "coderd_chatd_stream_retries_total" {
|
||||
assert.Empty(t, f.GetMetric(),
|
||||
"stream_retries_total should have no samples after a canceled stream")
|
||||
}
|
||||
}
|
||||
requireCounter(t, reg, "coderd_chatd_stream_retries_total", 1, map[string]string{
|
||||
"provider": "test-provider",
|
||||
"model": "test-model",
|
||||
"kind": string(codersdk.ChatErrorKindTimeout),
|
||||
"chain_broken": "false",
|
||||
})
|
||||
}
|
||||
|
||||
func TestRun_ToolError_RecordsMetric(t *testing.T) {
|
||||
|
||||
Reference in New Issue
Block a user