diff --git a/docs/consumers/pkg-scriptorium.md b/docs/consumers/pkg-scriptorium.md index b90b2f1..973fcff 100644 --- a/docs/consumers/pkg-scriptorium.md +++ b/docs/consumers/pkg-scriptorium.md @@ -20,12 +20,17 @@ fields: | `PromptDir` | Prompt-definition directory, required unless a prompt source option is supplied. | | `ProfileDir` | Optional custom profile directory over built-ins. | | `SchemaDir` | Schema directory; empty uses `.`. | -| `Timeout` | Default timeout for the built-in OpenAI-compatible client. | -| `HTTPClient` | Optional HTTP client for that built-in client. | +| `Timeout` | Base timeout for the built-in OpenAI-compatible client when `HTTPClient` is absent or has a zero timeout. A non-positive value uses the internal default. | +| `HTTPClient` | Optional HTTP client for that built-in client. It is cloned; a non-zero `Timeout` on it takes precedence over `Config.Timeout` as the base timeout. | Nil options are ignored. Invalid construction, including `WithLLMClient(nil)`, returns an error matching `ErrInvalidConfig`. +An effective positive `timeout_seconds` replaces the base timeout. An explicit +request override of zero disables the HTTP-client timeout. The timeout is +otherwise inherited from the supplied client, `Config.Timeout`, or the internal +default in that order. + Source options replace their matching directory source: - prompts: `WithPromptFS(fsys, root)`, `WithPromptFile(path)`; diff --git a/docs/integrations/openai-compatible-chat.md b/docs/integrations/openai-compatible-chat.md index 18f81bc..a8d8cae 100644 --- a/docs/integrations/openai-compatible-chat.md +++ b/docs/integrations/openai-compatible-chat.md @@ -77,9 +77,12 @@ resolves the configured non-empty `api_key_env` at request time and sends the same header. If neither mechanism supplies a key, it sends no `Authorization` header. -The configured client timeout applies by default. A positive effective -`timeout_seconds` replaces it. An explicit request override of zero disables -the HTTP-client timeout; negative values are rejected before a request is sent. +The client base timeout is chosen at construction: a non-zero timeout on a +supplied `http.Client` takes precedence over `Config.Timeout`; otherwise a +positive `Config.Timeout` is used, then the internal default. A positive +effective `timeout_seconds` replaces that base. An explicit request override +of zero disables the HTTP-client timeout; negative values are rejected before a +request is sent. ## Response Subset And Failures diff --git a/docs/internal/llm.md b/docs/internal/llm.md index bb97ad4..bbcea3f 100644 --- a/docs/internal/llm.md +++ b/docs/internal/llm.md @@ -10,13 +10,14 @@ format and protocol behavior. ## Construction `NewOpenAICompatibleClient` validates a non-empty configured base URL, records -an optional default model, and establishes the default timeout. A non-positive -configured timeout uses the internal default. +an optional default model, and resolves one base timeout. A supplied client with +a non-zero timeout supplies that base; otherwise a positive configured timeout +is used, then the internal default. When callers supply an `http.Client`, construction clones it rather than mutating the caller's instance. A supplied client with no timeout receives the -resolved default in the clone; a supplied non-zero timeout is retained. The -client stores the trimmed base URL, default model, timeout, and cloned client. +resolved base timeout in the clone. The client stores the trimmed base URL, +default model, and cloned client. ## Generate Flow @@ -28,7 +29,7 @@ client stores the trimmed base URL, default model, timeout, and cloned client. request; 4. prefer a direct API key, otherwise resolve the configured key environment variable; -5. derive a request HTTP client when an explicit timeout changes the configured +5. derive a request HTTP client only when an explicit timeout changes the base client; 6. execute the request, reject non-success status responses without returning provider response bodies; and diff --git a/internal/llm/openai_compatible_client.go b/internal/llm/openai_compatible_client.go index 257768d..5fdaa1c 100644 --- a/internal/llm/openai_compatible_client.go +++ b/internal/llm/openai_compatible_client.go @@ -36,7 +36,6 @@ type OpenAICompatibleConfig struct { type OpenAICompatibleClient struct { baseURL string defaultModel string - timeout time.Duration httpClient *http.Client } @@ -67,7 +66,6 @@ func NewOpenAICompatibleClient(cfg OpenAICompatibleConfig) (*OpenAICompatibleCli return &OpenAICompatibleClient{ baseURL: strings.TrimRight(baseURL, "/"), defaultModel: cfg.Model, - timeout: timeout, httpClient: client, }, nil } @@ -116,19 +114,20 @@ func (c *OpenAICompatibleClient) Generate(ctx context.Context, req domain.Genera httpReq.Header.Set("Authorization", "Bearer "+apiKey) } - effectiveTimeout := c.timeout - if req.Target.TimeoutSeconds > 0 { - effectiveTimeout = time.Duration(req.Target.TimeoutSeconds) * time.Second - } else if req.TargetPresence.TimeoutSeconds { - effectiveTimeout = 0 - } - httpClient := c.httpClient if httpClient == nil { - httpClient = &http.Client{Timeout: effectiveTimeout} - } else if httpClient.Timeout != effectiveTimeout { + httpClient = &http.Client{Timeout: defaults.LLMRequestTimeoutDefault} + } + if req.Target.TimeoutSeconds > 0 { + effectiveTimeout := time.Duration(req.Target.TimeoutSeconds) * time.Second + if httpClient.Timeout != effectiveTimeout { + cloned := *httpClient + cloned.Timeout = effectiveTimeout + httpClient = &cloned + } + } else if req.TargetPresence.TimeoutSeconds && httpClient.Timeout != 0 { cloned := *httpClient - cloned.Timeout = effectiveTimeout + cloned.Timeout = 0 httpClient = &cloned } diff --git a/internal/llm/openai_compatible_client_test.go b/internal/llm/openai_compatible_client_test.go index a205361..49d6ae3 100644 --- a/internal/llm/openai_compatible_client_test.go +++ b/internal/llm/openai_compatible_client_test.go @@ -31,9 +31,6 @@ func TestNewOpenAICompatibleClientDoesNotMutateSuppliedZeroTimeoutClient(t *test if client.httpClient == supplied { t.Fatal("expected constructed client to use a cloned HTTP client") } - if client.httpClient.Timeout != client.timeout { - t.Fatalf("expected cloned client timeout %v, got %v", client.timeout, client.httpClient.Timeout) - } if client.httpClient.Timeout <= 0 { t.Fatalf("expected constructed client to use a positive default timeout, got %v", client.httpClient.Timeout) } @@ -674,8 +671,9 @@ func TestOpenAICompatibleClientExplicitZeroTimeoutDisablesClientTimeout(t *testi defer ts.Close() client, err := NewOpenAICompatibleClient(OpenAICompatibleConfig{ - BaseURL: ts.URL + "/v1", - Timeout: time.Nanosecond, + BaseURL: ts.URL + "/v1", + Timeout: time.Nanosecond, + HTTPClient: &http.Client{Timeout: time.Nanosecond}, }) if err != nil { t.Fatal(err) @@ -718,6 +716,34 @@ func TestOpenAICompatibleClientOmittedTimeoutUsesClientTimeout(t *testing.T) { } } +func TestOpenAICompatibleClientSuppliedHTTPClientTimeoutOverridesConfigTimeout(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(25 * time.Millisecond) + _, _ = w.Write([]byte(`{"choices":[{"message":{"content":"ok"}}]}`)) + })) + defer ts.Close() + + client, err := NewOpenAICompatibleClient(OpenAICompatibleConfig{ + BaseURL: ts.URL + "/v1", + Model: "model", + Timeout: 5 * time.Millisecond, + HTTPClient: &http.Client{Timeout: 100 * time.Millisecond}, + }) + if err != nil { + t.Fatal(err) + } + + resp, err := client.Generate(context.Background(), domain.GenerateRequest{ + Prompt: domain.RenderedPrompt{Messages: []domain.RenderedMessage{{Role: "user", Content: "hi"}}}, + }) + if err != nil { + t.Fatalf("expected supplied client timeout to allow the request, got %v", err) + } + if resp.Content != "ok" { + t.Fatalf("unexpected response content: %q", resp.Content) + } +} + func TestOpenAICompatibleClientRejectsInvalidExtraParamsBeforeProviderCall(t *testing.T) { tests := []struct { name string @@ -1011,9 +1037,10 @@ func TestOpenAICompatibleClientRequestTimeoutOverride(t *testing.T) { defer ts.Close() client, err := NewOpenAICompatibleClient(OpenAICompatibleConfig{ - BaseURL: ts.URL + "/v1", - Model: "m", - Timeout: 50 * time.Millisecond, + BaseURL: ts.URL + "/v1", + Model: "m", + Timeout: 5 * time.Millisecond, + HTTPClient: &http.Client{Timeout: 50 * time.Millisecond}, }) if err != nil { t.Fatal(err)