fix: add default HTTP timeout and retry transient connection errors#52
Merged
Conversation
Two related reliability fixes: 1. Default HTTP timeout. All four provider clients (OpenAI, Anthropic, Gemini, Grok) previously defaulted to no timeout at all - reqwest's default - so a hung connection could block materialize() forever. Clients now default to a 5-minute total request timeout (DEFAULT_REQUEST_TIMEOUT) plus a 30-second connect timeout (DEFAULT_CONNECT_TIMEOUT), built once via a shared build_http_client() helper so the default actually reaches the reqwest client (previously the timeout was only applied when .timeout() was explicitly called). The .timeout() builder override still works and now also preserves the connect timeout. 2. Retryable connection errors. RStructorError::is_retryable() returned false for HttpError, so connection-reset/refused/DNS failures were treated as fatal even though they are exactly as transient as the 5xx errors that DO retry. is_retryable() (and retry_delay()) now return true / Some(1s) for HttpError where the underlying reqwest error is_connect() or is_timeout(). Body/decode errors remain non-retryable. New tests cover: connection-refused classifying as retryable after conversion, the request timeout actually firing through the built client, non-transient HTTP errors staying non-retryable, and default vs explicitly-set timeouts on all four provider configs. Co-Authored-By: Claude Fable 5 <[email protected]>
3c3ad50 to
e88bfad
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two related reliability hazards in all four provider clients (OpenAI, Anthropic, Gemini, Grok):
1. No default HTTP timeout — a hung connection blocked
materialize()foreverEvery client defaulted to
timeout: None, and reqwest's default is no timeout at all, so a stalled connection (half-open TCP, unresponsive proxy, blackholed route) would hang amaterialize()/generate()call indefinitely. Worse, the configured timeout only reached the reqwest client when.timeout()was explicitly called — the constructors built a barereqwest::Client::new().Now:
DEFAULT_REQUEST_TIMEOUT = 300s— generous enough for reasoning models) plus a 30-second connect timeout (DEFAULT_CONNECT_TIMEOUT = 30s).build_http_client()helper (and re-exported asrstructor::DEFAULT_REQUEST_TIMEOUT/rstructor::DEFAULT_CONNECT_TIMEOUT), used by every constructor so the default actually reaches the reqwest client..timeout()builder still overrides the request timeout, and now also preserves the connect timeout when it rebuilds the client.base_urlpaths use the same client and inherit the timeouts;MockClientis offline and unaffected.2. Transient connection errors were fatal while 502s retried
RStructorError::is_retryable()returnedtruefor retryableApiErrorkinds andTimeout, butfalsefor everything else. reqwest connection-reset/refused/DNS failures map toRStructorError::HttpErrorand were therefore never retried — even though they are exactly as transient as the 5xx errors that DO retry.Now:
is_retryable()(andretry_delay()) returntrue/Some(1s)forHttpError(e)wheree.is_connect() || e.is_timeout(). Body/decode errors remain non-retryable. The existing retry loop ingenerate_with_retry_with_initial_messagesconsultsis_retryable()and picks these up automatically.Requests that previously could run (or hang) indefinitely now fail with
RStructorError::Timeoutafter 5 minutes (30s if the connection can't even be established). Restore the old behavior with an explicit long.timeout(...)if needed. Additionally, transient connection failures are now retried by the built-in retry loop instead of failing fast.Tests
connection_refused_error_is_retryable— local bind-then-drop ephemeral port; asserts the converted error staysHttpError, classifies retryable, and carries a retry delay (no external network).request_timeout_applies_and_maps_to_timeout_error— local listener that accepts but never responds; proves the default-built client's timeout actually fires and maps toRStructorError::Timeout.non_transient_http_error_is_not_retryable— a non-connect/non-timeout reqwest error remains non-retryable.default_config_has_default_timeout/explicit_timeout_overrides_defaulton all four provider clients.--all-features;cargo clippy --all-targets(default features) and--all-featuresare clean; doctests pass. No new tests hit live APIs.🤖 Generated with Claude Code