Uh oh!
There was an error while loading. Please reload this page.
fix(providers): classify pydantic-ai translated errors as retryable - #458
Merged
Conversation
…454) pydantic-ai translates the Anthropic SDK's exceptions before Conductor sees them, so 429/5xx responses arrived as ModelHTTPError and transport failures as ModelAPIError, neither of which the existing retry classifier recognized -- Claude 429s were never retried on this path. _is_retryable_error now classifies both types directly, and _get_retry_after reads retry timing from ModelHTTPError's response body and, when headers are needed, from __cause__ (the untranslated SDK exception pydantic-ai's _map_api_errors preserves). Also stop clamping a user-configured delay_seconds below the provider default max_delay in both the pydantic-ai and Copilot retry configs -- an explicit delay_seconds: 60 now raises the cap instead of being silently capped at 30s.
…454 review) Blocking findings from the PR #458 review: - Validate a server-supplied retry-after value (from both the header path and the new ModelHTTPError body-parsing path) before using it as a sleep delay: reject non-finite (Infinity/NaN) and non-positive values, and clamp the result to retry_config.max_delay. Previously an unvalidated value could hang the workflow forever, crash the retry handler with a bare ValueError, or burn the whole retry budget in a hot loop. The __cause__ walk is now consulted before the body, since it carries the real HTTP header rather than an unconfirmed body convention Anthropic does not document. - Chain the retry-exhaustion ProviderError with `from last_error`, and for a translated ModelAPIError (transport failures whose own message is the SDK's hardcoded "Connection error.") walk __cause__ to name the real underlying failure (DNS, TLS, proxy, ...) in the suggestion text. - Correct the delay_seconds documentation in docs/workflow-syntax.md and the matching schema.py docstring: the cap is only *raised* above the 30s default when delay_seconds exceeds it, not "raised" unconditionally, and growth with the 2.0 default still reaches 30s. Also applied from the recommendations: - Rewrote the 60/60/60 delay test to route through _resolve_retry_config/execute_with_retry instead of constructing RetryConfig by hand (the original passed with the fix reverted). - Tightened the bare ModelAPIError retryable check to `type(...) is ModelAPIError` and added a canary test pinning ModelAPIError.__subclasses__() == [ModelHTTPError], since the arm is only safe while that holds. - Corrected stale docstrings/comments referencing a private pydantic-ai helper name and nonexistent ClaudeProvider methods; documented the exponential-backoff degeneration once delay_seconds >= 30s. - Added logging when a body-supplied retry_after is unparseable, and tests covering malformed retry-after values on both the header and body paths, plus exhaustion cause-chaining. Skipped (see PR report): recommendations requiring larger new test scaffolding or design changes (real anthropic.RateLimitError/ APIConnectionError integration tests, __cause__ cycle-guard tests, _retry_after_from_headers total-function hardening, 408/APITimeoutError reclassification, RetryConfig invariant normalization across three independent classes, retry-after provenance-tagged logging) were left for follow-up to keep this diff scoped to the review's blocking items. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
marked this pull request as ready for review
August 17, 2026 23:04
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
pydantic-ai translates the Anthropic SDK's exceptions before Conductor's Claude provider ever sees them, so 429/5xx responses arrive as
ModelHTTPErrorand transport failures asModelAPIError-- neither of which the existing retry classifier recognized. Claude rate-limit (429) errors were never retried on this path._is_retryable_errornow classifiesModelHTTPError(429/5xx) andModelAPIError(transport failures) directly._get_retry_afterreads retry timing fromModelHTTPError.bodyand, when headers are needed, walks__cause__to the untranslated SDK exception that_map_api_errorspreserves.delay_secondsbelow the provider's defaultmax_delay(both pydantic-ai and Copilot retry configs) -- an explicitdelay_seconds: 60now raises the cap instead of being capped at 30s.Closes#454
Testing
uv run pytest tests/test_providers/test_pydantic_ai_retry.py tests/test_per_agent_retry.py