diff options
| author | Paul Buetow <paul@buetow.org> | 2026-06-11 08:19:24 +0300 |
|---|---|---|
| committer | Paul Buetow <paul@buetow.org> | 2026-06-11 08:19:24 +0300 |
| commit | 8135a6b851b420597c22cfa5e58e6d69e894a00d (patch) | |
| tree | 7f70ac1325e4908139fc60af665dfb573275530b | |
| parent | b4f06c656d8d8733a9bf2e2e2a5eb2a7a48389bb (diff) | |
Consolidate LLM timeout/retry/breaker values into policy package
The per-request HTTP timeouts (30s chat, 120s research), retry policy
(attempts, backoff, jitter), and circuit-breaker tuning (threshold,
cooldown) were previously scattered as bare literals across each provider
constructor and resilience.go/circuitbreaker.go, making the operational
policy hard to discover and prone to drift.
Introduce internal/llm/policy with documented named constants as the single
source of truth, and reference them from all provider constructors, the
default retry policy, and the shared circuit breaker. Update comments to
explain the policy and reasoning. Add tests validating the values and their
internal consistency.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| -rw-r--r-- | internal/llm/anthropic.go | 4 | ||||
| -rw-r--r-- | internal/llm/ollama.go | 4 | ||||
| -rw-r--r-- | internal/llm/openai.go | 4 | ||||
| -rw-r--r-- | internal/llm/openrouter.go | 4 | ||||
| -rw-r--r-- | internal/llm/policy/policy.go | 68 | ||||
| -rw-r--r-- | internal/llm/policy/policy_test.go | 67 | ||||
| -rw-r--r-- | internal/llm/provider.go | 2 | ||||
| -rw-r--r-- | internal/llm/resilience.go | 18 | ||||
| -rw-r--r-- | internal/llm/yousearch.go | 6 |
9 files changed, 164 insertions, 13 deletions
diff --git a/internal/llm/anthropic.go b/internal/llm/anthropic.go index 3f7a616..00af17a 100644 --- a/internal/llm/anthropic.go +++ b/internal/llm/anthropic.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "codeberg.org/snonux/hexai/internal/llm/policy" "codeberg.org/snonux/hexai/internal/logging" ) @@ -116,7 +117,8 @@ func newAnthropicWithTimeout(baseURL, model, apiKey string, defaultTemp *float64 model = "claude-3-5-sonnet-20240620" } if timeoutSec <= 0 { - timeoutSec = 30 + // Fall back to the shared default chat timeout from the policy package. + timeoutSec = policy.DefaultRequestTimeoutSeconds } return anthropicClient{ httpClient: &http.Client{Timeout: time.Duration(timeoutSec) * time.Second}, diff --git a/internal/llm/ollama.go b/internal/llm/ollama.go index d8f911f..83823bc 100644 --- a/internal/llm/ollama.go +++ b/internal/llm/ollama.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "codeberg.org/snonux/hexai/internal/llm/policy" "codeberg.org/snonux/hexai/internal/logging" ) @@ -79,7 +80,8 @@ func newOllamaWithTimeout(baseURL, model, apiKey string, defaultTemp *float64, t model = "gemma4:31b-cloud" } if timeoutSec <= 0 { - timeoutSec = 30 + // Fall back to the shared default chat timeout from the policy package. + timeoutSec = policy.DefaultRequestTimeoutSeconds } return ollamaClient{ httpClient: &http.Client{Timeout: time.Duration(timeoutSec) * time.Second}, diff --git a/internal/llm/openai.go b/internal/llm/openai.go index d475de0..983723a 100644 --- a/internal/llm/openai.go +++ b/internal/llm/openai.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "codeberg.org/snonux/hexai/internal/llm/policy" "codeberg.org/snonux/hexai/internal/logging" ) @@ -125,7 +126,8 @@ func newOpenAIWithTimeout(baseURL, model, apiKey string, defaultTemp *float64, t model = "gpt-4.1" } if timeoutSec <= 0 { - timeoutSec = 30 + // Fall back to the shared default chat timeout from the policy package. + timeoutSec = policy.DefaultRequestTimeoutSeconds } return openAIClient{ httpClient: &http.Client{Timeout: time.Duration(timeoutSec) * time.Second}, diff --git a/internal/llm/openrouter.go b/internal/llm/openrouter.go index d728275..970ec29 100644 --- a/internal/llm/openrouter.go +++ b/internal/llm/openrouter.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "codeberg.org/snonux/hexai/internal/llm/policy" "codeberg.org/snonux/hexai/internal/logging" ) @@ -56,7 +57,8 @@ func newOpenRouterWithTimeout(baseURL, model, apiKey string, defaultTemp *float6 model = "openrouter/auto" } if timeoutSec <= 0 { - timeoutSec = 30 + // Fall back to the shared default chat timeout from the policy package. + timeoutSec = policy.DefaultRequestTimeoutSeconds } return openRouterClient{ httpClient: &http.Client{Timeout: time.Duration(timeoutSec) * time.Second}, diff --git a/internal/llm/policy/policy.go b/internal/llm/policy/policy.go new file mode 100644 index 0000000..e133e27 --- /dev/null +++ b/internal/llm/policy/policy.go @@ -0,0 +1,68 @@ +// Package policy centralizes the timeout, retry, and circuit-breaker tuning +// values used throughout the LLM HTTP call path. Previously these durations and +// counts were scattered as bare literals across each provider constructor and +// across resilience.go / circuitbreaker.go, which made the operational policy +// hard to discover and easy to drift out of sync. +// +// Consolidating them here gives a single, documented source of truth: anyone +// adjusting how aggressively Hexai times out, retries, or sheds load against an +// upstream LLM API only has to look in one place, and every provider inherits +// the same defaults automatically. +package policy + +import "time" + +// Request timeouts. +// +// DefaultRequestTimeout is the per-request HTTP client timeout applied to chat +// providers (Anthropic, OpenAI, OpenRouter, Ollama) when the user has not +// configured an explicit RequestTimeout. Thirty seconds is generous enough for +// normal completions yet short enough that a hung connection fails fast rather +// than blocking interactive use indefinitely. +// +// ResearchRequestTimeout applies to the You.com Research provider, whose +// multi-step research pipeline routinely runs much longer than a single chat +// completion. It therefore gets a substantially larger default so legitimate +// long-running research is not cut off prematurely. +const ( + DefaultRequestTimeout = 30 * time.Second + ResearchRequestTimeout = 120 * time.Second +) + +// DefaultRequestTimeoutSeconds and ResearchRequestTimeoutSeconds expose the +// timeouts as whole seconds, matching the int-seconds shape that provider +// constructors and the user-facing Config.RequestTimeout field use. +const ( + DefaultRequestTimeoutSeconds = int(DefaultRequestTimeout / time.Second) + ResearchRequestTimeoutSeconds = int(ResearchRequestTimeout / time.Second) +) + +// Retry policy. +// +// These govern how transient LLM HTTP failures (network resets, 5xx, 429 rate +// limits) are retried with exponential backoff plus jitter. The defaults are +// intentionally conservative: a handful of fast attempts that smooth over brief +// upstream blips without hammering the API or stalling interactive use. +// +// - RetryMaxAttempts: total tries including the first; <=1 disables retries. +// - RetryBaseDelay: delay before the first retry; doubles each subsequent attempt. +// - RetryMaxDelay: upper bound on any single backoff so growth stays bounded. +// - RetryJitterFraction: +/- fraction of random jitter to avoid thundering-herd retries. +const ( + RetryMaxAttempts = 3 + RetryBaseDelay = 200 * time.Millisecond + RetryMaxDelay = 2 * time.Second + RetryJitterFraction = 0.2 +) + +// Circuit breaker. +// +// The shared circuit breaker guards the whole LLM HTTP path. After +// CircuitFailureThreshold consecutive transient failures it trips open and +// rejects requests for CircuitCooldown, giving an unhealthy upstream time to +// recover instead of forcing every caller to wait out full retry+timeout +// cycles. +const ( + CircuitFailureThreshold = 5 + CircuitCooldown = 30 * time.Second +) diff --git a/internal/llm/policy/policy_test.go b/internal/llm/policy/policy_test.go new file mode 100644 index 0000000..976b41e --- /dev/null +++ b/internal/llm/policy/policy_test.go @@ -0,0 +1,67 @@ +package policy + +import ( + "testing" + "time" +) + +// TestRequestTimeouts verifies the documented default and research timeouts and +// that their whole-second mirrors stay in sync with the duration constants. +func TestRequestTimeouts(t *testing.T) { + if DefaultRequestTimeout != 30*time.Second { + t.Fatalf("DefaultRequestTimeout = %v, want 30s", DefaultRequestTimeout) + } + if ResearchRequestTimeout != 120*time.Second { + t.Fatalf("ResearchRequestTimeout = %v, want 120s", ResearchRequestTimeout) + } + if DefaultRequestTimeoutSeconds != 30 { + t.Fatalf("DefaultRequestTimeoutSeconds = %d, want 30", DefaultRequestTimeoutSeconds) + } + if ResearchRequestTimeoutSeconds != 120 { + t.Fatalf("ResearchRequestTimeoutSeconds = %d, want 120", ResearchRequestTimeoutSeconds) + } + // The integer mirrors must equal the duration constants converted to seconds. + if got := int(DefaultRequestTimeout / time.Second); got != DefaultRequestTimeoutSeconds { + t.Fatalf("DefaultRequestTimeoutSeconds out of sync: %d vs %d", DefaultRequestTimeoutSeconds, got) + } + if got := int(ResearchRequestTimeout / time.Second); got != ResearchRequestTimeoutSeconds { + t.Fatalf("ResearchRequestTimeoutSeconds out of sync: %d vs %d", ResearchRequestTimeoutSeconds, got) + } + // Research must allow a longer window than a normal chat completion. + if ResearchRequestTimeout <= DefaultRequestTimeout { + t.Fatalf("ResearchRequestTimeout (%v) must exceed DefaultRequestTimeout (%v)", ResearchRequestTimeout, DefaultRequestTimeout) + } +} + +// TestRetryPolicyConstants verifies the retry tuning values stay within sane, +// documented bounds. +func TestRetryPolicyConstants(t *testing.T) { + if RetryMaxAttempts != 3 { + t.Fatalf("RetryMaxAttempts = %d, want 3", RetryMaxAttempts) + } + if RetryBaseDelay != 200*time.Millisecond { + t.Fatalf("RetryBaseDelay = %v, want 200ms", RetryBaseDelay) + } + if RetryMaxDelay != 2*time.Second { + t.Fatalf("RetryMaxDelay = %v, want 2s", RetryMaxDelay) + } + if RetryBaseDelay > RetryMaxDelay { + t.Fatalf("RetryBaseDelay (%v) must not exceed RetryMaxDelay (%v)", RetryBaseDelay, RetryMaxDelay) + } + if RetryJitterFraction <= 0 || RetryJitterFraction >= 1 { + t.Fatalf("RetryJitterFraction = %v, want in (0,1)", RetryJitterFraction) + } +} + +// TestCircuitBreakerConstants verifies the circuit-breaker tuning values. +func TestCircuitBreakerConstants(t *testing.T) { + if CircuitFailureThreshold != 5 { + t.Fatalf("CircuitFailureThreshold = %d, want 5", CircuitFailureThreshold) + } + if CircuitCooldown != 30*time.Second { + t.Fatalf("CircuitCooldown = %v, want 30s", CircuitCooldown) + } + if CircuitFailureThreshold <= 0 { + t.Fatalf("CircuitFailureThreshold must be positive to trip the breaker") + } +} diff --git a/internal/llm/provider.go b/internal/llm/provider.go index dbfa603..27d8cc2 100644 --- a/internal/llm/provider.go +++ b/internal/llm/provider.go @@ -75,7 +75,7 @@ func WithStop(stop ...string) RequestOption { // Config defines provider configuration read from the Hexai config file. type Config struct { Provider string - RequestTimeout int // seconds; 0 means use default (30s) + RequestTimeout int // seconds; 0 means use the provider default (see internal/llm/policy) // OpenAI options OpenAIBaseURL string OpenAIModel string diff --git a/internal/llm/resilience.go b/internal/llm/resilience.go index 4d82153..ea6a32b 100644 --- a/internal/llm/resilience.go +++ b/internal/llm/resilience.go @@ -9,6 +9,7 @@ import ( "net/http" "time" + "codeberg.org/snonux/hexai/internal/llm/policy" "codeberg.org/snonux/hexai/internal/logging" ) @@ -37,15 +38,17 @@ type retryPolicy struct { randFloat func() float64 } -// defaultRetryPolicy returns the policy used for all LLM HTTP calls. Three -// total attempts (two retries) with 200ms base backoff keeps recovery fast for +// defaultRetryPolicy returns the policy used for all LLM HTTP calls. The +// concrete tuning values (attempts, backoff, jitter) live in the policy package +// so the whole resilience policy has a single source of truth. Three total +// attempts (two retries) with a 200ms base backoff keeps recovery fast for // transient errors while staying well within typical request timeouts. func defaultRetryPolicy() retryPolicy { return retryPolicy{ - maxAttempts: 3, - baseDelay: 200 * time.Millisecond, - maxDelay: 2 * time.Second, - jitterFraction: 0.2, + maxAttempts: policy.RetryMaxAttempts, + baseDelay: policy.RetryBaseDelay, + maxDelay: policy.RetryMaxDelay, + jitterFraction: policy.RetryJitterFraction, sleep: sleepWithContext, randFloat: rand.Float64, } @@ -54,7 +57,8 @@ func defaultRetryPolicy() retryPolicy { // sharedBreaker guards the whole LLM HTTP path. It trips after several // consecutive transient failures and stays open briefly so a single unhealthy // upstream does not cause every caller to wait out full retry+timeout cycles. -var sharedBreaker = newCircuitBreaker(5, 30*time.Second) +// Its threshold and cooldown come from the policy package. +var sharedBreaker = newCircuitBreaker(policy.CircuitFailureThreshold, policy.CircuitCooldown) // sleepWithContext sleeps for d unless ctx is cancelled first, in which case it // returns ctx.Err(). This keeps backoff waits responsive to cancellation and diff --git a/internal/llm/yousearch.go b/internal/llm/yousearch.go index 7539e8d..66cfb35 100644 --- a/internal/llm/yousearch.go +++ b/internal/llm/yousearch.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "codeberg.org/snonux/hexai/internal/llm/policy" "codeberg.org/snonux/hexai/internal/logging" ) @@ -52,7 +53,10 @@ func youSearchProviderFactory(cfg Config, keys ProviderKeys) (Client, error) { } timeoutSec := cfg.RequestTimeout if timeoutSec <= 0 { - timeoutSec = 120 + // The Research API runs a long multi-step pipeline, so it uses the + // larger research timeout from the policy package rather than the + // default chat timeout. + timeoutSec = policy.ResearchRequestTimeoutSeconds } return youSearchClient{ httpClient: &http.Client{Timeout: time.Duration(timeoutSec) * time.Second}, |
