fix: bound the async image download to the same 5s as sync - #197
Open
pucedoteth wants to merge 1 commit into
Open
fix: bound the async image download to the same 5s as sync#197pucedoteth wants to merge 1 commit into
pucedoteth wants to merge 1 commit into
Conversation
`ImageResponse.image` downloads the image when the API returns a URL. The sync path passes `timeout=5` to `requests.get`; the async path passes no timeout to `aiohttp.request`, which falls back to aiohttp's default of `ClientTimeout(total=300)`. So the same call is bounded at 5s on the sync client and 300s on the async one — 60x apart on an operation the sync side already decided should give up after five seconds. On a stalled CDN connection an async caller blocks for five minutes. Both now read `IMAGE_DOWNLOAD_TIMEOUT_SECONDS` from the shared `xai_sdk.image` module rather than each hardcoding its own, so the two transports cannot drift again. The sync value is unchanged. These are the only two outbound HTTP calls in the SDK; everything else goes over gRPC.
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.
Problem
ImageResponse.imagedownloads the image when the API returns a URL rather than base64. The two transports disagree on how long that may take:sync/image.py:362requests.get(..., timeout=5)aio/image.py:362aiohttp.request("GET", ...)aiohttp.requestwith notimeoutfalls back to aiohttp's default, which I confirmed against the pinned version:So the same operation is bounded 60× more loosely on the async client than the sync one already decided was right. On a stalled CDN connection,
await response.imageblocks for five minutes — in an async program, usually while holding whatever task the caller expected to be short.Fix
Both paths now read
IMAGE_DOWNLOAD_TIMEOUT_SECONDSfrom the sharedxai_sdk.imagemodule instead of each carrying its own literal, so they cannot drift apart again. The sync behaviour is unchanged — it just stops hardcoding the number.These are the only two outbound HTTP calls in the SDK (
grepforrequests./aiohttp.acrosssrc/xai_sdkreturns exactly these two); everything else is gRPC, so the scope really is this pair.Tests
One test per suite, asserting the timeout that actually reaches the transport:
tests/sync/image_test.py::test_url_download_uses_the_shared_timeouttests/aio/image_test.py::test_url_download_uses_the_shared_timeoutWith the two handlers reverted, the async one fails with
KeyError: 'timeout'— no timeout was passed. The sync one passes either way, which is correct: the sync path was never the bug, and that test is there to keep it from becoming one.Checks
All four from CONTRIBUTING, run locally:
uv run ruff format --checkuv run ruff checkuv run pyrightuv run pytest -n auto