From bcdea85a2dccf0e79972177a9c5facbb6fdc7880 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 10:49:46 -0400 Subject: [PATCH] Wire retrying Provider into presets.FromSettings FromSettings now returns llm.NewRetrying around the configured adapter, so every Provider it builds retries transient Generate failures and gets the default timeout; ListModels and Verify pass through. The test recording transport now answers 400 instead of failing the round trip: the ollama adapter reports a transport failure as ErrTimeout, which is retried, so the helper would see three requests. Tests: a 429 then 200 succeeds on the second request; exhausted retries return ErrProvider after MaxRetries+1 requests with no key in any error. Maestro task: 1a882923-fdb4-4576-ab82-9529e9974b26 Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/presets/presets.go | 5 +- internal/ai/llm/presets/presets_test.go | 100 ++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 9 deletions(-) diff --git a/internal/ai/llm/presets/presets.go b/internal/ai/llm/presets/presets.go index 43b0d85..da86eda 100644 --- a/internal/ai/llm/presets/presets.go +++ b/internal/ai/llm/presets/presets.go @@ -178,13 +178,14 @@ var all = map[string]Preset{ // // The Provider sends s.Temperature with every request whose Temperature is // nil; a request that sets one keeps it. Neither is sent to a model the -// preset's OmitTemperature matches. +// preset's OmitTemperature matches. It is wrapped in llm.NewRetrying, so +// Generate retries transient vendor failures and has the default timeout. func FromSettings(s llm.Settings, key string) (llm.Provider, error) { cp, err := fromSettings(s, key) if err != nil { return nil, redact(err, key) } - return cp, nil + return llm.NewRetrying(cp), nil } func fromSettings(s llm.Settings, key string) (*configured, error) { diff --git a/internal/ai/llm/presets/presets_test.go b/internal/ai/llm/presets/presets_test.go index 142c564..79cc0d1 100644 --- a/internal/ai/llm/presets/presets_test.go +++ b/internal/ai/llm/presets/presets_test.go @@ -22,10 +22,8 @@ type captured struct { body map[string]json.RawMessage } -var errStop = errors.New("recorded") - -// recordingTransport records each request and fails it, so no request -// leaves the process. +// recordingTransport records each request and answers it 400, so no +// request leaves the process and the Provider never retries it. type recordingTransport struct{ reqs []captured } func (rt *recordingTransport) RoundTrip(r *http.Request) (*http.Response, error) { @@ -40,7 +38,12 @@ func (rt *recordingTransport) RoundTrip(r *http.Request) (*http.Response, error) } } rt.reqs = append(rt.reqs, c) - return nil, errStop + return &http.Response{ + StatusCode: http.StatusBadRequest, + Header: http.Header{"Content-Type": {"application/json"}}, + Body: io.NopCloser(strings.NewReader(`{}`)), + Request: r, + }, nil } // generate builds the provider s describes and returns the request its @@ -327,7 +330,7 @@ func TestNegativeTemperatureRejectedForRejectingModels(t *testing.T) { } req := plainRequest() req.Temperature = ptr(-0.5) - if _, err := p.Generate(context.Background(), req); err == nil || errors.Is(err, errStop) || !strings.Contains(err.Error(), "temperature") { + if _, err := p.Generate(context.Background(), req); err == nil || errors.Is(err, llm.ErrProvider) || !strings.Contains(err.Error(), "temperature") { t.Errorf("%s: Generate = %v, want a temperature validation error", s.Model, err) } } @@ -465,7 +468,7 @@ func TestGenerateValidationErrorsNeverLeakKey(t *testing.T) { req := plainRequest() req.Temperature = ptr(-0.5) _, err = p.Generate(context.Background(), req) - if err == nil || errors.Is(err, errStop) { + if err == nil || errors.Is(err, llm.ErrProvider) { t.Fatalf("%s: Generate = %v, want a validation error", key, err) } for e := error(err); e != nil; e = errors.Unwrap(e) { @@ -518,3 +521,86 @@ 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. +type scriptedTransport struct { + statuses []int + key string + sent int +} + +func (st *scriptedTransport) RoundTrip(r *http.Request) (*http.Response, error) { + if st.sent == len(st.statuses) { + return nil, fmt.Errorf("unscripted request %d", st.sent+1) + } + status := st.statuses[st.sent] + st.sent++ + header := http.Header{"Content-Type": {"application/json"}} + body := `{"error":{"message":"bad key ` + st.key + `"}}` + switch status { + case http.StatusOK: + body = `{"model":"m","choices":[{"message":{"content":"tacos"},"finish_reason":"stop"}]}` + case http.StatusTooManyRequests: + header.Set("Retry-After", "1") + } + return &http.Response{ + StatusCode: status, + Header: header, + Body: io.NopCloser(strings.NewReader(body)), + Request: r, + }, nil +} + +// useTransport makes the adapters send through rt for the rest of the test. +func useTransport(t *testing.T, rt http.RoundTripper) { + t.Helper() + orig := http.DefaultTransport + http.DefaultTransport = rt + t.Cleanup(func() { http.DefaultTransport = orig }) +} + +// The Provider FromSettings returns retries a rate-limited Generate. +func TestFromSettingsRetriesRateLimited(t *testing.T) { + st := &scriptedTransport{statuses: []int{http.StatusTooManyRequests, 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) + } +} + +// When every retry fails, the error FromSettings's Provider returns keeps +// its sentinel and never holds the key the vendor echoed. +func TestFromSettingsRetryErrorNeverLeaksKey(t *testing.T) { + st := &scriptedTransport{statuses: []int{http.StatusTooManyRequests, http.StatusTooManyRequests, http.StatusServiceUnavailable}, key: testKey} + useTransport(t, st) + + p, err := FromSettings(llm.Settings{Provider: "openai", Model: "m"}, testKey) + if err != nil { + t.Fatalf("FromSettings: %v", err) + } + _, err = p.Generate(context.Background(), plainRequest()) + if !errors.Is(err, llm.ErrProvider) { + t.Fatalf("Generate = %v, want %v", err, llm.ErrProvider) + } + if st.sent != llm.MaxRetries+1 { + t.Errorf("sent %d requests, want %d", st.sent, llm.MaxRetries+1) + } + for e := error(err); e != nil; e = errors.Unwrap(e) { + if s := e.Error() + fmt.Sprintf("%#v", e); strings.Contains(s, testKey) { + t.Fatalf("error leaks the key: %s", s) + } + } +}