Fix async error path in acompletion/aresponses to invoke fallback/mapping/telemetry#3355
Fix async error path in acompletion/aresponses to invoke fallback/mapping/telemetry#3355malhotra5 wants to merge 5 commits into
Conversation
…ping/telemetry In acompletion() and aresponses(), the async retry loop was outside the try/except that calls _ahandle_error. This meant that on retry exhaustion or non-retryable provider errors, the async path: - Never invoked fallback_strategy (backup LLM silently skipped) - Never mapped litellm exceptions to SDK types via map_provider_exception - Never called telemetry.on_error Most critically, ContextWindowExceededError escaped unmapped so the agent's astep() recovery (except LLMContextWindowExceedError -> CondensationRequest) never triggered, erroring out the conversation. Fix: wrap the async retry loop inside the same try/except scope as the sync path, for both acompletion and aresponses. Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — the async paths now match the sync error-handling scope, and the targeted fallback tests pass locally (uv run pytest tests/sdk/llm/test_llm_fallback.py -q).
No code issues found. Per repo policy, this touches LLM/agent recovery behavior, so I’m leaving a COMMENT rather than approving; please have a human maintainer decide after lightweight eval/benchmark validation.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM. Small, well-tested error-path fix, but it changes production LLM fallback/mapping/condensation behavior.
This review was created by an AI agent (OpenHands) on behalf of the user.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26247340286
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review — Fix async error path in acompletion/aresponses
Overall: The fix is correct and well-motivated. The root cause (retry loop outside try/except) was a clear structural bug causing the async paths to silently bypass fallback invocation, exception mapping, and telemetry. The solution — expanding the try block upward to enclose the entire retry loop — mirrors the sync completion()/responses() pattern and is the minimal, right change. The new tests confirm both fallback dispatch and exception-type mapping work correctly for both async methods.
Two minor observations below.
This review was generated by an AI agent (OpenHands) on behalf of the user via OpenHands Automation.
…onnection error test - Move @pytest.mark.asyncio above @patch decorators on all async tests for conventional ordering and readability - Add test_aresponses_maps_connection_error for full mapping parity between acompletion and aresponses Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The async LLM error paths now behave as described: fallback, SDK exception mapping, and telemetry error callbacks all activate for acompletion() and aresponses().
Does this PR achieve its stated goal?
Yes. I reproduced the base-branch failure by calling the SDK async LLM APIs with controlled provider transport failures: raw LiteLLM exceptions escaped, fallback was not attempted, and telemetry did not record the error. On the PR branch, the same SDK usage returned fallback responses and mapped provider errors to SDK exceptions while invoking telemetry.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully; no tests/linters were run locally. |
| CI Status | unresolved-review-threads failed and several Run tests/Agent Server jobs were still in progress; many checks had passed. |
| Functional Verification | ✅ Before/after SDK script verified async fallback + mapping + telemetry for chat and Responses API paths. |
Functional Verification
Test 1: Async LLM provider error handling before/after
I exercised the SDK as a library user would: a short script imported LLM, FallbackStrategy, LLMProfileStore, Message, and TextContent; configured a primary LLM plus a real fallback profile; then called await llm.acompletion(...) and await llm.aresponses(...) with deterministic simulated provider transport failures. The script filtered output to its QA-* result lines.
Step 1 — Reproduce / establish baseline (without the fix):
Ran git checkout main && git rev-parse --short HEAD && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_async_llm_behavior.py 2>&1 | grep '^QA-':
8f406a88
QA-branch=main sha=8f406a88
QA-acompletion-fallback=FAIL raised=APIConnectionError async_calls=1 fallback_calls=0
QA-acompletion-context-map-telemetry=FAIL raised=ContextWindowExceededError telemetry=[] async_calls=1
QA-acompletion-connection-map=FAIL raised=APIConnectionError async_calls=1
QA-aresponses-fallback=FAIL raised=APIConnectionError async_calls=1 fallback_calls=0
QA-aresponses-context-map-telemetry=FAIL raised=ContextWindowExceededError telemetry=[] async_calls=1
This confirms the reported bug exists on main: async transport errors escape as raw LiteLLM exceptions, fallback is skipped (fallback_calls=0), and telemetry does not see the provider error (telemetry=[]).
Step 2 — Apply the PR's changes:
Checked out fix/async-error-path-fallback-mapping-telemetry at 490b626151bcda2b41a6e18fee5f31e200eb0098.
Step 3 — Re-run with the fix in place:
Ran git checkout fix/async-error-path-fallback-mapping-telemetry && git rev-parse --short HEAD && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_async_llm_behavior.py 2>&1 | grep '^QA-':
490b6261
QA-branch=fix/async-error-path-fallback-mapping-telemetry sha=490b6261
QA-acompletion-fallback=PASS text='chat fallback ok' async_calls=1 fallback_calls=1
QA-acompletion-context-map-telemetry=PASS raised=LLMContextWindowExceedError telemetry=['ContextWindowExceededError'] async_calls=1
QA-acompletion-connection-map=PASS raised=LLMServiceUnavailableError async_calls=1
QA-aresponses-fallback=PASS text='responses fallback ok' async_calls=1 fallback_calls=1
QA-aresponses-context-map-telemetry=PASS raised=LLMContextWindowExceedError telemetry=['ContextWindowExceededError'] async_calls=1
This shows the fix works for the stated behavior: both async paths invoke fallback, context-window provider errors map to LLMContextWindowExceedError, connection errors map to LLMServiceUnavailableError, and telemetry observes the original provider error.
Issues Found
None.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
Co-authored-by: Vasco Schiavo <115561717+VascoSch92@users.noreply.github.com>
|
@OpenHands address the unresolved review comments leave the comment that outlines improvements for a future pr alone |
|
@malhotra5 your session has expired. Please login again at OpenHands Cloud and try again. |
… streaming loop Merge the litellm_modify_params/catch_warnings context managers into a single parenthesized with across aresponses, sync responses, and both transport calls for consistency. Convert the aresponses streaming delta checks to continue guard clauses to reduce nesting.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Verified the async LLM error path by running real SDK LLM.acompletion() and LLM.aresponses() calls against local OpenAI-compatible endpoints; fallback, SDK exception mapping, and telemetry error hooks now fire where main did not.
Does this PR achieve its stated goal?
Yes. The PR set out to make async acompletion()/aresponses() match the sync error path by invoking fallback, mapping provider exceptions to SDK exceptions, and calling telemetry on provider errors. In a before/after run, main skipped fallback and surfaced raw LiteLLM errors, while this PR returned fallback responses, raised LLMContextWindowExceedError / LLMServiceUnavailableError, and recorded telemetry.on_error for both async paths.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed; SDK imported and ran from the repo environment. |
| CI Status | ✅ Completed checks observed green; current qa-changes/pr-review jobs were still in progress when checked. |
| Functional Verification | ✅ Real SDK calls exercised acompletion() and aresponses() before/after the fix. |
Functional Verification
Test 1: Async fallback after primary transport failure
Step 1 — Reproduce / establish baseline (without the fix):
Checked out origin/main and ran:
OPENHANDS_SUPPRESS_BANNER=1 uv run python -u /tmp/qa_async_llm_error_paths.pyObserved baseline output excerpt:
{
"acompletion_fallback": "ERROR: InternalServerError: litellm.InternalServerError: InternalServerError: OpenAIException - Connection error.",
"aresponses_fallback": "ERROR: InternalServerError: litellm.InternalServerError: InternalServerError: OpenAIException - Cannot connect to host 127.0.0.1:59805 ssl:<ssl.SSLContext object at 0x7ff557a7ee90> [Connect call failed ('127.0.0.1', 59805)]"
}This confirms the bug exists on main: the primary transport error escapes and the fallback profile is not invoked.
Step 2 — Apply the PR's changes:
Checked out PR head 49ad6bfc7e2057702b791c9fcc964853a9d366bc.
Step 3 — Re-run with the fix in place:
Ran the same command:
OPENHANDS_SUPPRESS_BANNER=1 uv run python -u /tmp/qa_async_llm_error_paths.pyObserved PR output excerpt:
{
"acompletion_fallback": "fallback ok via chat",
"aresponses_fallback": "fallback ok via responses"
}This shows the async fallback strategy now loads the fallback profile and returns successful responses for both Chat Completions and Responses API paths.
Test 2: Async provider exceptions are mapped to SDK exception types
Step 1 — Reproduce / establish baseline (without the fix):
On origin/main, the same script used a local provider response shaped like an OpenAI context-window failure and a closed local port for connection failure. Observed baseline output excerpt:
{
"acompletion_connection_error": "InternalServerError (sdk_mapped=False)",
"acompletion_context_error": "ContextWindowExceededError (sdk_mapped=False)",
"aresponses_connection_error": "InternalServerError (sdk_mapped=False)",
"aresponses_context_error": "ContextWindowExceededError (sdk_mapped=False)"
}This confirms async errors bypassed the SDK mapping layer on main.
Step 2 — Apply the PR's changes:
Checked out PR head 49ad6bfc7e2057702b791c9fcc964853a9d366bc.
Step 3 — Re-run with the fix in place:
Observed PR output excerpt:
{
"acompletion_connection_error": "LLMServiceUnavailableError (sdk_mapped=True)",
"acompletion_context_error": "LLMContextWindowExceedError (sdk_mapped=True)",
"aresponses_connection_error": "LLMServiceUnavailableError (sdk_mapped=True)",
"aresponses_context_error": "LLMContextWindowExceedError (sdk_mapped=True)"
}This shows the async paths now raise the SDK exception types callers expect.
Test 3: Async telemetry on provider error
Step 1 — Reproduce / establish baseline (without the fix):
On origin/main, the script attached a minimal telemetry sink and triggered context-window errors through real async SDK calls. Observed baseline output excerpt:
{
"acompletion_telemetry_on_context_error": {
"errors": [],
"raised": "ContextWindowExceededError",
"requests": 1,
"responses": 0
},
"aresponses_telemetry_on_context_error": {
"errors": [],
"raised": "ContextWindowExceededError",
"requests": 1,
"responses": 0
}
}This confirms the request was attempted but telemetry.on_error was not called on main.
Step 2 — Apply the PR's changes:
Checked out PR head 49ad6bfc7e2057702b791c9fcc964853a9d366bc.
Step 3 — Re-run with the fix in place:
Observed PR output excerpt:
{
"acompletion_telemetry_on_context_error": {
"errors": ["ContextWindowExceededError"],
"raised": "LLMContextWindowExceedError",
"requests": 1,
"responses": 0
},
"aresponses_telemetry_on_context_error": {
"errors": ["ContextWindowExceededError"],
"raised": "LLMContextWindowExceedError",
"requests": 1,
"responses": 0
}
}This shows telemetry.on_error is now invoked before the mapped SDK exception reaches the caller.
Issues Found
None.
Verified by an AI agent (OpenHands) on behalf of the user.
Final verdict: PASS
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — no actionable code issues found. The async paths now match the sync error-handling scope, and targeted tests pass locally (uv run pytest tests/sdk/llm/test_llm_fallback.py -q).
[RISK ASSESSMENT] 🟡 MEDIUM: small, well-tested LLM error-path fix, but it affects async agent recovery/fallback behavior; per repo policy, I’m commenting rather than approving pending human/eval validation.
This review was created by an AI agent (OpenHands) on behalf of the user.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26414675097
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review — Fix async error path in acompletion/aresponses
Overall: ✅ Correct fix, well-tested, all prior threads resolved.
The structural bug was clear: the async for attempt in self.async_retry(...) loop sat outside the try/except that calls _ahandle_error, so transport errors from the retry machinery escaped unmapped and without triggering fallback or telemetry. The fix — expanding the try block upward to enclose the entire retry loop — is the minimal, right change and mirrors the sync completion()/responses() pattern exactly.
All six previous review threads (decorator ordering, missing test_aresponses_maps_connection_error, context-manager merging, guard-clause refactoring, raw_response parameter, and the structural note) are resolved in the current HEAD. The six new async tests provide solid coverage of fallback dispatch and exception-type mapping for both acompletion and aresponses.
One minor style issue below.
This review was generated by an AI agent (OpenHands) on behalf of the user via OpenHands Automation.
| @patch("openhands.sdk.llm.llm.litellm_aresponses", new_callable=AsyncMock) | ||
| async def test_aresponses_fallback_on_transport_error(mock_aresp, mock_resp): | ||
| """aresponses must invoke fallback when the primary transport raises.""" | ||
| from litellm.types.llms.openai import ResponsesAPIResponse |
There was a problem hiding this comment.
🟡 Suggestion: ResponsesAPIResponse is imported inside the function body. Per PEP 8, module-level imports should be at the top of the file. Since it's only needed in this one test, move it alongside the litellm.types.utils imports at the top of the file and remove this local import.
4 participants
Why
In
acompletion()andaresponses(), theasync forretry loop was outside thetry/exceptblock that calls_ahandle_error. This meant that on retry exhaustion or non-retryable provider errors, the async path:fallback_strategy— backup LLM was silently skippedmap_provider_exceptiontelemetry.on_errorMost critically,
ContextWindowExceededErrorescaped unmapped, so the agent'sastep()recovery (except LLMContextWindowExceedError → CondensationRequest) never triggered — the conversation errored out instead of auto-condensing. Same forLLMMalformedConversationHistoryError.The sync
completion()andresponses()methods were correct — they wrap the entire retry-decorated_one_attempt()call insidetry/except.Summary
async forretry loop inside the sametry/exceptscope as the sync path, for bothacompletionandaresponsesHow to Test
All 15 tests pass (10 original sync + 5 new async):
test_acompletion_fallback_on_transport_error— verifies fallback fires onAPIConnectionErrortest_acompletion_maps_context_window_error— verifiesContextWindowExceededError→LLMContextWindowExceedErrortest_acompletion_maps_connection_error— verifiesAPIConnectionError→LLMServiceUnavailableErrortest_aresponses_fallback_on_transport_error— verifies fallback fires for the Responses API pathtest_aresponses_maps_context_window_error— verifies context window mapping for Responses APIFull LLM test suite (744 tests) also passes:
uv run pytest tests/sdk/llm/ -xEvidence the fix works
Before the fix (async retry loop outside try/except):
Transport errors (APIConnectionError, ContextWindowExceededError, etc.) raised from the retry loop would propagate up unmapped and without fallback.
After the fix (async retry loop inside try/except, matching sync):
The new async tests (e.g.,
test_acompletion_fallback_on_transport_error) would fail on the old code becauseAPIConnectionErrorwould escape without invoking fallback, andtest_acompletion_maps_context_window_errorwould fail becauseContextWindowExceededErrorwould propagate raw instead of being mapped toLLMContextWindowExceedError.Type
Notes
This PR was created by an AI agent (OpenHands) on behalf of the user.
@malhotra5 can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:49ad6bf-pythonRun
All tags pushed for this build
About Multi-Architecture Support
49ad6bf-python) is a multi-arch manifest supporting both amd64 and arm6449ad6bf-python-amd64) are also available if needed