diff --git a/internal/ai/llm/anthropic/anthropic.go b/internal/ai/llm/anthropic/anthropic.go index 37b1239..c41c0f1 100644 --- a/internal/ai/llm/anthropic/anthropic.go +++ b/internal/ai/llm/anthropic/anthropic.go @@ -381,7 +381,7 @@ func (p *Provider) statusError(resp *http.Response, body []byte) error { case http.StatusTooManyRequests, statusOverloaded: return llm.NewRateLimitedError(retryAfter(resp.Header.Get("Retry-After"), time.Now()), p.apiKey) case http.StatusRequestTimeout: - return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrTimeout) + return llm.NewTimeoutStatusError(resp.StatusCode, p.apiKey) default: return llm.NewProviderError(resp.StatusCode, body, p.apiKey) } diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index e4f1bd2..b4c6480 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -137,3 +137,32 @@ func (e *ProviderError) GoString() string { return e.text } // Is reports whether target is ErrProvider. func (e *ProviderError) Is(target error) bool { return target == ErrProvider } + +// TimeoutStatusError is an HTTP status, such as 408, by which the vendor +// said the request timed out. It matches ErrTimeout, and Status keeps it +// apart from a transport timeout, so a caller can tell the vendor answered. +// Build it with NewTimeoutStatusError, which redacts its text. +type TimeoutStatusError struct { + status int + text string +} + +// NewTimeoutStatusError returns the error for a timeout HTTP status. +// secret is the credential sent with the request, or "" when the request +// sent none; every occurrence of a non-empty secret is removed from the +// error text, and the secret itself is not kept. +func NewTimeoutStatusError(status int, secret string) *TimeoutStatusError { + text := fmt.Sprintf("HTTP %d: %s", status, ErrTimeout) + return &TimeoutStatusError{status: status, text: Redact(text, secret)} +} + +// Status returns the HTTP status code. +func (e *TimeoutStatusError) Status() int { return e.status } + +func (e *TimeoutStatusError) Error() string { return e.text } + +// GoString keeps %#v to the redacted Error text instead of the fields. +func (e *TimeoutStatusError) GoString() string { return e.text } + +// Unwrap returns ErrTimeout. +func (e *TimeoutStatusError) Unwrap() error { return ErrTimeout } diff --git a/internal/ai/llm/errors_test.go b/internal/ai/llm/errors_test.go index 5ac5fa7..37406df 100644 --- a/internal/ai/llm/errors_test.go +++ b/internal/ai/llm/errors_test.go @@ -196,3 +196,17 @@ func TestTypedErrorsDoNotKeepSecret(t *testing.T) { } } } + +func TestTimeoutStatusError(t *testing.T) { + err := fmt.Errorf("openai: %w", NewTimeoutStatusError(408, "HTTP")) + if !errors.Is(err, ErrTimeout) || errors.Is(err, ErrProvider) { + t.Fatalf("%v: want ErrTimeout only", err) + } + var ts *TimeoutStatusError + if !errors.As(err, &ts) || ts.Status() != 408 { + t.Fatalf("TimeoutStatusError from %v", err) + } + if s := ts.Error() + fmt.Sprintf("%#v", ts); strings.Contains(s, "HTTP") { + t.Fatalf("text holds the secret: %s", s) + } +} diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 70c94e9..d8bf66a 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -453,7 +453,7 @@ func (p *Provider) statusError(resp *http.Response, body []byte) error { case http.StatusTooManyRequests: return llm.NewRateLimitedError(retryAfter(resp.Header.Get("Retry-After"), time.Now()), p.apiKey) case http.StatusRequestTimeout: - return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrTimeout) + return llm.NewTimeoutStatusError(resp.StatusCode, p.apiKey) default: return llm.NewProviderError(resp.StatusCode, body, p.apiKey) } diff --git a/internal/ai/llm/presets/presets_test.go b/internal/ai/llm/presets/presets_test.go index 79cc0d1..41e4b3f 100644 --- a/internal/ai/llm/presets/presets_test.go +++ b/internal/ai/llm/presets/presets_test.go @@ -523,7 +523,8 @@ func TestConfigErrorsNeverLeakKey(t *testing.T) { } // scriptedTransport answers each request with the next scripted status and -// body, echoing the key in every failure body, and counts the requests. +// body, echoing the key in every failure body, and counts the requests. A +// scripted status of 0 is a transport timeout instead of a response. type scriptedTransport struct { statuses []int key string @@ -536,6 +537,9 @@ func (st *scriptedTransport) RoundTrip(r *http.Request) (*http.Response, error) } status := st.statuses[st.sent] st.sent++ + if status == 0 { + return nil, transportTimeout{} + } header := http.Header{"Content-Type": {"application/json"}} body := `{"error":{"message":"bad key ` + st.key + `"}}` switch status { @@ -552,6 +556,13 @@ func (st *scriptedTransport) RoundTrip(r *http.Request) (*http.Response, error) }, nil } +// transportTimeout is a net.Error that timed out. +type transportTimeout struct{} + +func (transportTimeout) Error() string { return "i/o timeout" } +func (transportTimeout) Timeout() bool { return true } +func (transportTimeout) Temporary() bool { return true } + // useTransport makes the adapters send through rt for the rest of the test. func useTransport(t *testing.T, rt http.RoundTripper) { t.Helper() @@ -604,3 +615,48 @@ func TestFromSettingsRetryErrorNeverLeaksKey(t *testing.T) { } } } + +// An HTTP 408 from the vendor is a 4xx answer and is not retried, though it +// matches llm.ErrTimeout. +func TestFromSettingsDoesNotRetryHTTP408(t *testing.T) { + for _, provider := range []string{"openai", "anthropic"} { + t.Run(provider, func(t *testing.T) { + st := &scriptedTransport{statuses: []int{http.StatusRequestTimeout}, key: testKey} + useTransport(t, st) + + p, err := FromSettings(llm.Settings{Provider: provider, Model: "m"}, testKey) + if err != nil { + t.Fatalf("FromSettings: %v", err) + } + _, err = p.Generate(context.Background(), plainRequest()) + var ts *llm.TimeoutStatusError + if !errors.Is(err, llm.ErrTimeout) || !errors.As(err, &ts) || ts.Status() != http.StatusRequestTimeout { + t.Fatalf("Generate = %v, want an HTTP 408 ErrTimeout", err) + } + if st.sent != 1 { + t.Errorf("sent %d requests, want 1", st.sent) + } + }) + } +} + +// A transport timeout carries no status and is retried. +func TestFromSettingsRetriesTransportTimeout(t *testing.T) { + st := &scriptedTransport{statuses: []int{0, http.StatusOK}, key: testKey} + useTransport(t, st) + + p, err := FromSettings(llm.Settings{Provider: "openai", Model: "m"}, testKey) + if err != nil { + t.Fatalf("FromSettings: %v", err) + } + resp, err := p.Generate(context.Background(), plainRequest()) + if err != nil { + t.Fatalf("Generate: %v", err) + } + if resp.Text != "tacos" { + t.Errorf("Text = %q, want %q", resp.Text, "tacos") + } + if st.sent != 2 { + t.Errorf("sent %d requests, want 2", st.sent) + } +} diff --git a/internal/ai/llm/retry.go b/internal/ai/llm/retry.go index bb724ab..ec50fa5 100644 --- a/internal/ai/llm/retry.go +++ b/internal/ai/llm/retry.go @@ -22,11 +22,12 @@ const ( ) // Retrying is a Provider that retries transient Generate failures of the -// Provider it wraps. It retries ErrRateLimited, ErrTimeout and a 5xx -// ErrProvider at most MaxRetries times, and nothing else. A rate-limited -// retry waits for the vendor's RetryAfter, capped at MaxRetryAfter; every -// other retry, and a rate limit that names no wait, waits a jittered -// exponential backoff. The Request's Timeout, or DefaultTimeout, is one +// Provider it wraps. It retries ErrRateLimited, a transport ErrTimeout and +// a 5xx ErrProvider at most MaxRetries times, and nothing else: an error +// carrying a 4xx HTTP status other than a rate limit, such as an HTTP 408 +// TimeoutStatusError, is not retried. A rate-limited retry waits for the +// vendor's RetryAfter, capped at MaxRetryAfter; every other retry, and a +// rate limit that names no wait, waits a jittered exponential backoff. The Request's Timeout, or DefaultTimeout, is one // deadline for the whole call, retries and waits included; a caller's // earlier context deadline wins. A retry whose wait would not end before // that deadline is not made: the last error is returned at once. @@ -91,6 +92,10 @@ func (r *Retrying) Generate(ctx context.Context, req Request) (Response, error) // wait returns how long to wait before retrying after err, the retry-th // retry counting from 0, and false when err is not retried. func (r *Retrying) wait(err error, retry int) (time.Duration, bool) { + var st interface{ Status() int } + if errors.As(err, &st) && st.Status() >= 400 && st.Status() <= 499 { + return 0, false + } var rl *RateLimitedError if errors.As(err, &rl) && rl.RetryAfter > 0 { return min(rl.RetryAfter, MaxRetryAfter), true diff --git a/internal/ai/llm/retry_test.go b/internal/ai/llm/retry_test.go index 1c0a7b7..2095616 100644 --- a/internal/ai/llm/retry_test.go +++ b/internal/ai/llm/retry_test.go @@ -77,7 +77,7 @@ func TestRetryingRateLimitedWaitIsCapped(t *testing.T) { if _, err := r.Generate(context.Background(), retryReq); err != nil { t.Fatal(err) } - if want := []time.Duration{MaxRetryAfter}; !slices.Equal(c.sleeps, want) { + if want := []time.Duration{20 * time.Second}; !slices.Equal(c.sleeps, want) { t.Errorf("sleeps = %v, want %v", c.sleeps, want) } assertConsumed(t, f, 2) @@ -149,11 +149,13 @@ func TestRetrying5xxThreeTimesFails(t *testing.T) { func TestRetryingDoesNotRetryPermanentErrors(t *testing.T) { for name, err := range map[string]error{ - "401 auth": ErrAuth, - "400": NewProviderError(400, []byte("bad request"), ""), - "404": NewProviderError(404, []byte("no such model"), ""), - "bad output": ErrBadOutput, - "other": errors.New("llm: request has no messages"), + "401 auth": ErrAuth, + "400": NewProviderError(400, []byte("bad request"), ""), + "404": NewProviderError(404, []byte("no such model"), ""), + "408": NewTimeoutStatusError(408, ""), + "wrapped 408": fmt.Errorf("openai: generate: %w", NewTimeoutStatusError(408, "")), + "bad output": ErrBadOutput, + "other": errors.New("llm: request has no messages"), } { t.Run(name, func(t *testing.T) { r, f, c := newTestRetrying() @@ -170,6 +172,18 @@ func TestRetryingDoesNotRetryPermanentErrors(t *testing.T) { } } +// A transport timeout matches ErrTimeout like an HTTP 408, but carries no +// status, so it is retried. +func TestRetryingTransportTimeoutRetried(t *testing.T) { + r, f, _ := newTestRetrying() + f.QueueGenerate(fail(fmt.Errorf("openai: generate: %w", ErrTimeout)), ok("dinner")) + + if _, err := r.Generate(context.Background(), retryReq); err != nil { + t.Fatal(err) + } + assertConsumed(t, f, 2) +} + func TestRetryingDeadlineShorterThanRetryAfter(t *testing.T) { r, f, c := newTestRetrying() rl := NewRateLimitedError(10*time.Second, "") @@ -250,7 +264,7 @@ func TestRetryingTimeoutBoundsWholeCall(t *testing.T) { timeout time.Duration want time.Duration }{ - {"default", 0, DefaultTimeout}, + {"default", 0, 60 * time.Second}, {"custom", 10 * time.Second, 10 * time.Second}, } { t.Run(tc.name, func(t *testing.T) {