fix(providers): retry transient OpenAI stream errors - #506
Conversation
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Two blocking issues, both in retry.py around the new openai.APIError classification, plus six recommended items on tests, docs, and error handling. The core problem: the retryable set doesn't match what OpenAI (or any OpenAI-compatible endpoint) actually sends, so this fixes less than the PR title claims, and the module docstring now asserts something the code no longer does.
Blocking:
src/conductor/providers/_pydantic_ai/retry.py:176— retryable set uses Anthropic'srate_limit_errorstring instead of OpenAI's actual markers, so streamed rate limits still fail on the first attempt.src/conductor/providers/_pydantic_ai/retry.py:175— module docstring claims this is untouched by SDK internals; the new branch depends directly onopenai.APIErrorstaying untranslated by pydantic-ai, and nothing pins that assumption.
Two findings had no line to anchor to, so they're written out in full below.
RECOMMENDED — Provider parity check is incomplete (no location)
AGENTS.md requires checking every provider when retry/error classification changes in one of them. The PR description covers Anthropic but doesn't mention hermes, which also targets OpenAI-compatible endpoints. hermes.py wraps exceptions in ProviderError with no status_code or is_retryable, so retryability falls back to a "connection" in message or "timeout" in message substring check (exceptions.py:275) — a stream error message like "upstream overloaded" would be classified non-retryable there too, the same gap this PR closes for openai. Whether hermes-agent actually surfaces openai.APIError from its stream handling couldn't be confirmed (the package isn't installed here), but the PR body should say one way or the other, the way it already does for Anthropic. copilot, claude_agent_sdk, and aca aren't affected — none of them go through the OpenAI SDK directly.
RECOMMENDED — No CHANGELOG entry (CHANGELOG.md)
This changes when a configured retry: policy kicks in against OpenAI-compatible endpoints — a run that used to fail after one attempt will now retry. ## [Unreleased] currently has no ### Fixed heading, and four of the last five fix(...) commits (d3c43a1, 9760b72, 812a7dc, 134b735) updated the changelog alongside the code. Add an entry, and given how narrow the current allowlist is (see the blocking finding above), it's worth being specific about which stream errors now retry and which still don't — otherwise a user hitting a rate-limit stream error will assume this PR covered them when it didn't.
| # an SSE response has started. Unlike HTTP APIStatusError subclasses, that | ||
| # exception has no status code; retry only server/rate-limit payload types. | ||
| if openai is not None and type(exception) is openai.APIError: | ||
| return exception.type in {"server_error", "rate_limit_error"} |
There was a problem hiding this comment.
BLOCKING: The retryable set doesn't match OpenAI's actual error payloads, so streamed rate limits still fail on the first attempt.
{"server_error", "rate_limit_error"} — "rate_limit_error" is Anthropic's vocabulary (anthropic/types/beta_rate_limit_error.py), not OpenAI's, and Anthropic never even reaches this branch: its SSE error path raises a status-bearing APIStatusError (anthropic/_streaming.py:162) that pydantic-ai translates to ModelHTTPError, which the earlier branch already handles. So this entry only matches a third-party gateway that happens to imitate Anthropic's error shape.
OpenAI's own rate-limit errors use "type": "requests" or "tokens" with "code": "rate_limit_exceeded" (see the SDK's own literals in openai/types/beta/threads/run.py:42). I ran the classifier against realistic mid-stream payloads:
body (openai/_streaming.py passes body=data["error"]) |
.type |
retryable |
|---|---|---|
{"type": "server_error"} |
'server_error' |
True |
{"type": "requests", "code": "rate_limit_exceeded"} (real OpenAI 429) |
'requests' |
False |
{"type": "overloaded_error"} |
'overloaded_error' |
False |
{"type": "api_error"} |
'api_error' |
False |
{"code": "429"} (Azure shape, no type) |
None |
False |
"model is overloaded" (Ollama/vLLM string body) |
None |
False |
So this makes exactly one payload type retryable, while the commit title and the comment above this line both say rate limits are covered. That gap matters more here than usual — openai.py explicitly targets OpenAI-compatible endpoints (Ollama, vLLM, LM Studio, Azure, per AGENTS.md), and those are the bottom four rows.
One more thing worth knowing: APIError.type is built via construct_type(type_=str, value=body.get("type")), which doesn't coerce — a payload of {"type": 500} yields the int 500. Any comparison here needs to not assume the value is a string.
| return exception.type in {"server_error", "rate_limit_error"} | |
| if openai is not None and type(exception) is openai.APIError: | |
| payload_type = getattr(exception, "type", None) | |
| payload_code = getattr(exception, "code", None) | |
| retryable_markers = { | |
| "server_error", # OpenAI mid-stream 5xx | |
| "rate_limit_exceeded", # OpenAI / most gateways | |
| "requests", # OpenAI 429 (requests-per-min) | |
| "tokens", # OpenAI 429 (tokens-per-min) | |
| "overloaded_error", # Anthropic-compatible gateways | |
| "api_error", # Anthropic-compatible gateways | |
| "internal_server_error", | |
| } | |
| return payload_type in retryable_markers or payload_code in retryable_markers |
Whatever set you land on, add a comment citing where the strings come from. APIError.type is a free-form passthrough with no SDK enum behind it, so right now there's no way for a reader to tell if this list is exhaustive or a guess.
There was a problem hiding this comment.
Fixed in 42eeb73. The marker set now matches what OpenAI and Anthropic-shaped gateways actually send: server_error / internal_server_error (mid-stream 5xx), requests / tokens + code rate_limit_exceeded (OpenAI 429, per the literals you pointed at), and rate_limit_error / overloaded_error / api_error (gateway-proxied Anthropic vocabulary) — with sources cited in the comment above the set.
The non-coercion point is handled too: the comparison is in-based, so a {"type": 500} payload (arriving as an int) just misses the set and stays fatal rather than crashing — pinned by test_raw_openai_api_error_with_non_string_type_is_fatal. And every marker is now asserted in both the type and code positions by test_every_documented_stream_marker_is_retryable_as_type_and_code, so trimming the set later is a loud failure, not a silent one. I verified that by deleting individual markers and watching the suite fail.
| # The OpenAI SDK raises a bare APIError for error objects delivered after | ||
| # an SSE response has started. Unlike HTTP APIStatusError subclasses, that | ||
| # exception has no status code; retry only server/rate-limit payload types. | ||
| if openai is not None and type(exception) is openai.APIError: |
There was a problem hiding this comment.
BLOCKING: The module docstring now asserts the opposite of what this branch does, and the assumption it waves off is actually load-bearing.
Three claims in the docstring (lines 11-25) no longer hold:
- "Those are the exception types actually observed on this path, not the SDK's own classes" —
openai.APIErroris the SDK's own class and is now classified directly. - "Only the public
ModelHTTPError/ModelAPIErrortypes are relied on at runtime" —openai.APIErrorand its.typeattribute are now load-bearing. - "a change to the private translator degrades this comment, not the code" — this one is now false, and it's the dangerous one.
I confirmed the branch is actually reachable: pydantic_ai/models/openai.py:216-226 (_map_api_errors) only catches APIStatusError and APIConnectionError, so a bare APIError propagates untranslated, and Conductor does stream (interrupt.py:230 → agent.iter → _stream_node_events → node.stream(...)), so openai/_streaming.py:188/205 raises directly into this function. Which means this branch's correctness depends on pydantic-ai continuing not to catch bare APIError.
If pydantic-ai adds except APIError to _map_api_errors — a plausible one-line upstream change — this branch goes dead and the same errors arrive as ModelAPIError, which line 169-170 makes unconditionally retryable. That silently reverses the fatal classification the new test at tests/test_providers/test_pydantic_ai_retry.py:231 is supposed to pin: an invalid_request_error delivered mid-stream would start retrying. Right now the docstring tells the next maintainer that translator changes are safe to ignore, which is exactly backwards.
Also stale while you're in there: the parenthetical about "the 1.44.0 floor pinned in pyproject.toml" — the project now pins pydantic-ai-slim[anthropic,openai]>=2.28.0,<3, so that branch is unreachable under any supported version.
| if openai is not None and type(exception) is openai.APIError: | |
| The OpenAI path is translated by the same-named ``_map_api_errors`` in | |
| ``pydantic_ai.models.openai``, which catches only ``APIStatusError`` | |
| (-> ``ModelHTTPError``) and ``APIConnectionError`` (-> ``ModelAPIError``). | |
| It deliberately does not catch the bare ``openai.APIError`` that | |
| ``openai/_streaming.py`` raises for an ``error`` object embedded in an SSE | |
| body, so that one SDK class does reach Conductor untranslated and is | |
| classified directly in ``_is_retryable_error``. If pydantic-ai widens that | |
| ``except`` clause, this branch goes dead and mid-stream errors arrive as | |
| ``ModelAPIError`` -- which is unconditionally retryable, reversing the | |
| fatal classification for client-side payload types. |
The ModelAPIError.__subclasses__() canary already in this file (tests/test_providers/test_pydantic_ai_retry.py:190-196) is the existing pattern for turning this kind of upstream assumption into a failing test instead of a comment — worth doing the same here.
There was a problem hiding this comment.
Fixed in 42eeb73. The module docstring now states plainly that _map_api_errors in pydantic_ai.models.openai catches only APIStatusError/APIConnectionError, that the bare APIError from openai/_streaming.py reaches Conductor untranslated, and — your dangerous case — that a widened except upstream would silently reverse the fatal classification via the unconditional ModelAPIError arm. The stale 1.44.0-floor parenthetical is gone (the pin is pydantic-ai-slim[anthropic,openai]>=2.28.0,<3 now).
The assumption is pinned with the canary pattern you suggested: test_pydantic_ai_does_not_translate_bare_openai_api_error raises a bare APIError inside the real _map_api_errors and asserts it propagates untranslated, so an upstream widening fails the suite instead of going dead quietly.
| # an SSE response has started. Unlike HTTP APIStatusError subclasses, that | ||
| # exception has no status code; retry only server/rate-limit payload types. | ||
| if openai is not None and type(exception) is openai.APIError: | ||
| return exception.type in {"server_error", "rate_limit_error"} |
There was a problem hiding this comment.
RECOMMENDED: A payload with no parseable type gets silently classified fatal, and this branch shadows everything below it.
APIError.__init__ (openai/_exceptions.py:71-79) only sets .type when is_dict(body); otherwise it's None. And openai/_streaming.py:87-99 only checks is_mapping(error) to build the message — it raises with body=data["error"] regardless of that value's shape. So a gateway sending {"error": "upstream overloaded"} produces .type is None, and this line returns False.
Because line 176 is an unconditional return, every bare APIError short-circuits the rest of the function — it never reaches the name-based retryable_names check or the APIStatusError handling below. That's fine for an exact-type match, but "unknown or absent type" is a real classification decision being made by omission, with no comment and no test behind it.
Worth deciding explicitly rather than defaulting: a stream that's already started means auth and request validation both passed, so an error arriving mid-stream with no recognizable type looks more like the transport failures this function retries unconditionally (line 169-170) than a genuine client error. Either way, put the reasoning in the comment — right now it reads as a filter over a known vocabulary and doesn't say that a type-less payload gets swallowed as fatal.
No crash risk here, for what it's worth — .type is set on both branches of __init__, and the exact-type check guarantees the instance went through it.
| return exception.type in {"server_error", "rate_limit_error"} | |
| if error_type is None: | |
| # A mid-stream error whose payload carries no `type` (common from | |
| # OpenAI-compatible gateways, and from a non-object `error` value) | |
| # is indistinguishable from a broken stream; treat it like the | |
| # transport failures above rather than burning the run. | |
| return True |
Or keep it fatal and say so explicitly — "A payload whose error value isn't a JSON object leaves .type as None and is treated as fatal" — with a test pinning the choice.
There was a problem hiding this comment.
Fixed in 42eeb73, taking your first option: a payload with no parseable type now returns True explicitly, with the reasoning in the comment — by the time a stream has started, auth and request validation have passed, so a type-less mid-stream error is indistinguishable from a broken stream and is treated like the transport failures above rather than burning the run.
Pinned by test_raw_openai_api_error_without_payload_type_is_retryable over None, "upstream overloaded", [1, 2], and {"code": "x"} — the shapes most likely from a non-OpenAI endpoint — and end-to-end by the test_sdk_stream_error_with_non_object_error_value canary through a real openai.Stream.
| assert _is_retryable_error(_make_openai_status_error(503)) is True | ||
| assert _is_retryable_error(_make_openai_status_error(429)) is True | ||
|
|
||
| def test_raw_openai_stream_validation_error_is_fatal(self) -> None: |
There was a problem hiding this comment.
RECOMMENDED: The new tests don't pin the guard they depend on, and leave half the retryable set unasserted.
Three gaps, all cheap to close:
- Nothing asserts
type(...) isinstead ofisinstance(...). Every existingopenai.APIStatusErrortest constructs withbody=None, so.typeisNoneacross the board andNone not in {"server_error", "rate_limit_error"}gives the same answer either way. Swapping the new line toisinstance(exception, openai.APIError)fails zero tests, even though the exact-type check is the entire reason the comment above this line exists. "rate_limit_error"is asserted nowhere. It appears exactly once in the whole repo, atretry.py:176. Deleting it from the set is a silent, green-suite change.body=Noneand non-dict bodies aren't tested — the shapes most likely from a non-OpenAI endpoint, and the ones that make the fatal-by-default decision above invisible.
Also: test_raw_openai_stream_validation_error_is_fatal passes unchanged against origin/main (a bare APIError was already fatal there), so it's a forward guard only, not a regression test. test_real_openai_stream_server_error_retries_once_then_succeeds is the one test in this PR that actually catches a regression.
| def test_raw_openai_stream_validation_error_is_fatal(self) -> None: | |
| def test_raw_openai_stream_rate_limit_error_is_retryable(self) -> None: | |
| """Requirement: the second half of the retryable set is load-bearing.""" | |
| error = openai.APIError( | |
| "slow down", _make_http_request(), | |
| body={"type": "rate_limit_error", "code": "rate_limit_exceeded"}, | |
| ) | |
| assert _is_retryable_error(error) is True | |
| def test_openai_status_subclasses_are_not_captured_by_the_bare_arm(self) -> None: | |
| """Requirement: status wins over payload type for APIStatusError subclasses.""" | |
| request = _make_http_request() | |
| assert _is_retryable_error( | |
| openai.BadRequestError( | |
| "bad", response=httpx.Response(400, request=request), | |
| body={"type": "server_error"}, | |
| ) | |
| ) is False | |
| assert _is_retryable_error( | |
| openai.InternalServerError( | |
| "boom", response=httpx.Response(500, request=request), | |
| body={"type": "invalid_request_error"}, | |
| ) | |
| ) is True | |
| @pytest.mark.parametrize("body", [None, "upstream overloaded", [1, 2], {"code": "x"}]) | |
| def test_raw_openai_api_error_without_payload_type(self, body: object) -> None: | |
| """Requirement: a bare APIError with no parseable `type` must not crash.""" | |
| assert _is_retryable_error( | |
| openai.APIError("boom", _make_http_request(), body=body) | |
| ) is False # flip if the decision above goes the other way |
The first BadRequestError assertion is the one that flips under the isinstance mutation — it's the one actually guarding the exact-type check.
There was a problem hiding this comment.
All three gaps closed in 42eeb73:
test_openai_status_subclasses_are_not_captured_by_the_bare_arm(adapted from your suggestion) — I ran the mutation: swappingtype(...) isforisinstancenow fails this test.test_every_documented_stream_marker_is_retryable_as_type_and_codepins every marker in the set in bothtypeandcodepositions, so deleting any entry is a loud failure (verified by deleting markers and watching the suite go red).- Type-less and non-dict bodies are covered by
test_raw_openai_api_error_without_payload_type_is_retryable, and the non-string-typecase bytest_raw_openai_api_error_with_non_string_type_is_fatal.
On test_raw_openai_stream_validation_error_is_fatal passing against main: fair — it was a forward guard. With the widened marker set it now does real work as the fatal-side pin, and the translator canary documents what reverses it.
| # The OpenAI SDK raises a bare APIError for error objects delivered after | ||
| # an SSE response has started. Unlike HTTP APIStatusError subclasses, that | ||
| # exception has no status code; retry only server/rate-limit payload types. | ||
| if openai is not None and type(exception) is openai.APIError: |
There was a problem hiding this comment.
RECOMMENDED: Under a narrowed retry_on, a newly-retryable stream error escapes as a raw openai.APIError instead of a ProviderError.
This is a behavior change the PR introduces, not a pre-existing gap. execute_with_retry has a retry_on gate:
if retry_config.retry_on is not None:
error_category = _classify_error(e)
if error_category not in retry_config.retry_on:
raiseThat's a bare raise — no log, no event, no ProviderError wrapper. retry_on: ["timeout"] is valid workflow config (config/schema.py:665).
Before this PR, a mid-stream server_error was non-retryable and exited through the earlier branch as a wrapped ProviderError. After this PR, it's retryable, reaches the retry_on gate, fails the category check (_classify_error returns "provider_error"), and escapes as a raw openai.APIError. Any caller catching ProviderError now misses it — confirmed end-to-end, isinstance(e, ProviderError) is False. The wider the retryable set gets (see the first finding), the more error shapes take this path.
| if openai is not None and type(exception) is openai.APIError: | |
| if error_category not in retry_config.retry_on: | |
| raise ProviderError( | |
| f"Pydantic AI call failed: {e}", | |
| suggestion=( | |
| f"Error category {error_category!r} is not in retry_on=" | |
| f"{retry_config.retry_on}; widen retry_on to retry it." | |
| ), | |
| status_code=_extract_status_code(e), | |
| is_retryable=False, | |
| ) from e |
There was a problem hiding this comment.
Fixed in 4ab0868. The retry_on gate now wraps a declined error in a non-retryable ProviderError naming the category, chained to the original — no more raw SDK exceptions escaping past ProviderError catchers. This also changes the pre-existing path for other retryable types under a narrowed retry_on (they took the same bare raise), which the function's docstring had already promised (ProviderError: On fatal or exhausted retry errors); test_retry_on_filter_honored was pinning the old raw-escape behavior and now pins the wrap and the __cause__ chain.
Your exact scenario is covered by test_retry_on_filtered_stream_error_surfaces_as_provider_error: a server_error stream error under retry_on=["timeout"] surfaces as ProviderError, not a raw openai.APIError.
| if type(exception) is ModelAPIError: | ||
| return True | ||
|
|
||
| # The OpenAI SDK raises a bare APIError for error objects delivered after |
There was a problem hiding this comment.
RECOMMENDED: A declined retry leaves no trace, and the resulting error tells the user to check their API key.
The only record of the classification decision is logger.debug(...). Conductor installs no logging handlers — documented at cli/run.py:1972 — so that line hits logging.lastResort, which is level WARNING, and is never emitted in a default run. The asymmetry is total: a retry taken gets a logger.warning plus an agent_retry event to the dashboard and JSONL log. A retry declined gets nothing.
Here's what an operator actually sees, reproduced through the real execute_with_retry:
ProviderError: Pydantic AI call failed: upstream overloaded
Suggestion: Check API key, model name, and request parameters
The API key was fine — the suggestion points at the wrong thing entirely. _extract_status_code returns None for a bare APIError (no status_code attribute), so the message carries no status, no .type, no .code, no body. And when the error's message field is missing, openai/_streaming.py:76 substitutes the literal "An error occurred during streaming", so the final message is "Pydantic AI call failed: An error occurred during streaming. Check API key, model name, and request parameters." The user configured three attempts, got one, and nothing anywhere says a retry policy existed and was declined.
This is pre-existing behavior, but this PR makes it the primary failure mode for this error class — given how narrow the allowlist is, most real transient stream errors will land here.
| # The OpenAI SDK raises a bare APIError for error objects delivered after | |
| if not is_retryable: | |
| logger.warning( | |
| "[No retry] %s classified non-retryable after attempt %s/%s: %s", | |
| type(e).__name__, attempt, retry_config.max_attempts, e, | |
| ) |
For a bare APIError specifically, include .type / .code / .body in the ProviderError message — str(e) demonstrably doesn't carry them.
There was a problem hiding this comment.
Fixed in 4ab0868. A declined retry now logs at warning level with the attempt count, mirroring the retry-taken path — pinned by test_declined_retry_logs_a_warning.
For a fatal bare APIError, the ProviderError message now carries the payload type/code (and a non-object body) via _describe_stream_error, precisely because str(e) doesn't — pinned by test_fatal_stream_error_message_carries_payload_details.
On the misleading suggestion: with the type-less → retryable decision from your earlier finding, the upstream overloaded scenario now retries instead of surfacing as fatal, so it never reaches that suggestion. The remaining fatal bare-APIError cases are recognized client-side payload types (e.g. invalid_request_error), where "check request parameters" is actually the right advice.
| mock_sleep.assert_called_once_with(2.0) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_real_openai_stream_server_error_retries_once_then_succeeds(self) -> None: |
There was a problem hiding this comment.
RECOMMENDED: The tests describe an SSE stream but never touch one, so the payload-shape assumption the whole PR rests on is unpinned.
Both # Requirement: comments say the error is "inside an SSE stream," but both tests hand-construct openai.APIError(...) directly — the async one raises it from a plain async def factory(). The chosen body shape does match reality (openai/_streaming.py passes body=data["error"], so .type sits at the top level, same as what the tests build), but nothing asserts that against actual SDK output. If a future SDK release nests the payload one level deeper (body=data instead of body=data["error"]), .type becomes None, retries silently stop happening in production, and both of these tests keep passing.
This is the same class of assumption the file already guards elsewhere with the ModelAPIError.__subclasses__() canary at line 190-196.
| async def test_real_openai_stream_server_error_retries_once_then_succeeds(self) -> None: | |
| def test_sdk_stream_error_payload_shape_is_what_we_classify(self) -> None: | |
| """Requirement: the SDK puts the error object itself in `body`. | |
| If this ever nests one level deeper, `.type` becomes None and the | |
| retry branch silently stops matching. | |
| """ | |
| # drive openai.Stream over: data: {"error": {"type": "server_error"}} | |
| # and assert the raised APIError has .type == "server_error" |
Or just soften the comments to describe what's actually asserted ("an openai.APIError shaped like the one the SDK raises from an SSE body...") if you'd rather not add the integration-style test.
There was a problem hiding this comment.
Done in 42eeb73 — I added the integration-style canary rather than softening: test_sdk_stream_error_payload_shape_is_what_we_classify drives a real openai.Stream over data: {"error": {"type": "server_error", "message": "boom"}} bytes and asserts the SDK puts the error object itself into body (so .type sits at the top level) and that the result classifies retryable. A second canary, test_sdk_stream_error_with_non_object_error_value, pins the {"error": "upstream overloaded"} shape end-to-end: .type is None, string body, and the SDK's "An error occurred during streaming" fallback message.
The two hand-constructed tests keep their place as unit tests, but their comments now describe what's actually asserted and point at the canary for the payload-shape guarantee.
| if type(exception) is ModelAPIError: | ||
| return True | ||
|
|
||
| # The OpenAI SDK raises a bare APIError for error objects delivered after |
There was a problem hiding this comment.
RECOMMENDED: _is_retryable_error's docstring still claims every raw-SDK branch is unreachable in production.
It reads: "...the retryable_names and anthropic.APIStatusError fallback below is unreachable in production but kept intentionally." That was true before this PR, but the enumeration is now incomplete — it lists the translated types and the unreachable fallback, but skips the third category this PR adds: a raw SDK class (openai.APIError) that is reachable in production. A reader who trusts this docstring walks away thinking the function only ever sees translated types, which is exactly the wrong mental model now.
For what it's worth, the existing claim about the retryable_names tail is still accurate — I checked models/openai.py:216 and models/anthropic.py:321, both fully absorb APIStatusError/APIConnectionError, so that part of the docstring doesn't need to change.
| # The OpenAI SDK raises a bare APIError for error objects delivered after | |
| plus the bare ``openai.APIError`` the OpenAI SDK raises for an error object | |
| embedded in an SSE body, which pydantic-ai does **not** translate and which | |
| is therefore reachable in production. |
There was a problem hiding this comment.
Fixed in 42eeb73 — the _is_retryable_error docstring now names the third category (a bare openai.APIError that pydantic-ai does not translate and that is therefore reachable in production) alongside the translated types and the intentionally-unreachable fallback. Confirmed your read on the retryable_names/anthropic.APIStatusError tail and left that claim untouched.
…nAI payloads
The retryable set for a bare openai.APIError used Anthropic's
"rate_limit_error" string, which a real OpenAI 429 never sends — its
mid-stream rate limits arrive as type "requests"/"tokens" with code
"rate_limit_exceeded", so they still failed on the first attempt.
Widen the marker set to the vocabulary OpenAI and Anthropic-shaped
gateways actually emit, with sources cited, and treat a payload with no
parseable type (a non-object error value from an Ollama/vLLM gateway,
or an Azure-style {"code": ...} shape) like a broken stream: by the
time a stream has started, auth and request validation have passed.
The module docstring also claimed only the translated ModelHTTPError/
ModelAPIError types are relied on at runtime — now false, since the
bare-APIError branch depends on pydantic-ai continuing not to translate
it. Rewrite it to say so, drop the stale 1.44.0 floor note, and pin the
upstream assumptions with canary tests: pydantic-ai's _map_api_errors
leaves a bare APIError untranslated, and a real openai.Stream puts the
SSE error object itself into APIError.body. Every marker in the set is
now pinned in both payload positions, and the exact-type guard has a
test that fails under an isinstance mutation.
Three gaps in how execute_with_retry reports a failure the retry policy did not take: - A retryable error declined by a narrowed retry_on filter escaped through a bare raise, so a raw SDK exception (e.g. a mid-stream openai.APIError) flew past callers that catch ProviderError. The gate now wraps it in a non-retryable ProviderError naming the declined category, chained to the original. - A retry taken logs a warning and emits agent_retry; a retry declined logged only at debug, which Conductor's default no-handler logging never emits. The decline is now logged at warning level too. - A bare openai.APIError's str() carries neither the payload type/code nor a non-object body, and the SDK substitutes a generic "An error occurred during streaming" message, so a fatal stream error told the user to check their API key with no evidence attached. The ProviderError message now includes the payload details via _describe_stream_error.
|
Addressing the two findings that had no line to anchor to: Provider parity / hermes. Verified from Conductor's side: CHANGELOG. Added under Both blocking findings are addressed in 42eeb73; the error-surfacing recommendations in 4ab0868. Ready for another look. |
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM, thanks for contributing!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
=======================================
Coverage ? 91.89%
=======================================
Files ? 164
Lines ? 26553
Branches ? 0
=======================================
Hits ? 24402
Misses ? 2151
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
server_error/internal_server_error), OpenAI rate limits (typerequests/tokenswith coderate_limit_exceeded), gateway-proxied Anthropic errors (rate_limit_error/overloaded_error/api_error), and stream errors with no parseable payloadtype(non-objecterrorvalues from Ollama/vLLM-style gateways, Azure-style{"code": ...}shapes), which are treated like broken streamsinvalid_request_error) and HTTP 4xx failures fatalretry_on-declined errors inProviderErrorinstead of letting raw SDK exceptions escape, log declined retries at warning level, and include payloadtype/codein fatal bare-APIErrormessagesAPIErrorand the SDK's SSE error payload shapeWhy
The OpenAI SDK raises a bare
openai.APIErrorwhen an error object arrives after an SSE response has started. That exception has no HTTP status, so Conductor previously skipped the configured retry policy and failed after the first attempt.Provider parity:
APIStatusErrorsubclasses, which the existing classifier already handles.hermes-agentlibrary, which owns its own OpenAI client. Conductor never sees a rawopenai.APIErroron that path — library failures arrive wrapped inProviderErrorwith no status, so retryability falls back to the message-substring heuristic inexceptions.py. Mid-stream retry classification there belongs tohermes-agent, not to Conductor, and is out of scope for this PR. (Whether the library itself surfacesopenai.APIErrorcould not be confirmed — the package isn't installed in this environment — but either way the fix would have to land there.)Verification
uv run pytest tests/test_providers/test_pydantic_ai_retry.py(88 passed)make lintmake typecheckisinstancefails the subclass-separation test; deleting any marker from the retryable set fails its pinning testopenai.APIError(type=server_error): result=ok, attempts=2