Repository navigation
fix(google): bound chunk silence, not the whole request - #7994
iamdgarcia wants to merge 3 commits into
Conversation
conn_options.timeout went to genai's HttpOptions.timeout, which aiohttp turns into a whole-request deadline (ClientTimeout(total=...)) and Vertex enforces again as X-Server-Timeout. Any healthy Gemini answer streaming longer than the read timeout died mid-sentence with a 504 or a CancelledError, and chunks already sent meant no failover. Force the httpx transport so the timeout keeps the per-phase read semantics the other plugins have, and preset X-Server-Timeout with the long-decode ceiling so genai does not re-derive a server deadline from the read timeout. (livekit#7987)
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
genai reads async_client_args from client-level options only (_use_aiohttp consults the ApiClient's _http_options), so the request-level injection was a no-op at runtime. Move it to Client construction and pass client_http_options through. Found by Devin review on PR livekit#7994.
|
Good catch, and confirmed: _use_aiohttp reads the transport from the ApiClient's client-level _http_options, so the per-request injection never reached the code path that picks aiohttp. Fixed in 7049b17: the plugin now injects the httpx transport into the http_options passed to the genai.Client constructor (a copy, so the caller's object is untouched), and the request path only presets X-Server-Timeout. Test now asserts the transport on llm._client._api_client._http_options. Full google plugin suite 172 passed. |
| client_async_args = dict(client_http_options.async_client_args or {}) | ||
| if "transport" not in client_async_args: | ||
| client_async_args["transport"] = httpx.AsyncHTTPTransport() | ||
| client_http_options.async_client_args = client_async_args |
There was a problem hiding this comment.
🔴 Google requests bypass configured proxies
When a deployment relies on HTTPS_PROXY, the inserted AsyncHTTPTransport makes httpx ignore environment proxies. Google requests connect directly and fail where the provider is reachable only through that proxy.
Learn more
httpx builds environment-proxy mounts only when no custom transport is supplied to its async client. Supplying AsyncHTTPTransport() forces the desired per-read timeout behavior, but it also turns off proxy auto-discovery. Deployments that can access Gemini or Vertex only through HTTPS_PROXY now attempt direct connections instead.
Example: A worker runs with HTTPS_PROXY=http://proxy.internal:8080 and blocks direct external traffic. Constructing the Google LLM with default HTTP options installs a custom transport; generating an answer attempts a direct connection and fails.
Recommended fix: Preserve proxy behavior when forcing httpx, for example by supplying a proxy-aware transport or propagating the applicable proxy configuration to the HTTP client. Cover both default and caller-supplied HTTP options in a proxy environment test.
Was this helpful? React with 👍 or 👎 to provide feedback.
longcw
left a comment
longcw
left a comment
There was a problem hiding this comment.
left some comments, and please shorten the comments to one line each
| # so request-level transport injection is a no-op at runtime (#7987). Forcing httpx here | ||
| # makes the ms timeout a per-phase read timeout instead of aiohttp's ClientTimeout(total) | ||
| # whole-request deadline, matching the openai/inference plugins' semantics. | ||
| client_http_options = http_options if is_given(http_options) else types.HttpOptions() |
There was a problem hiding this comment.
The transport block in LLM.__init__ is in the diff twice. The first copy changes the caller's HttpOptions in place (no model_copy()), so please delete it and keep only the second one.
| # it as a WHOLE-REQUEST deadline, killing healthy answers that stream longer | ||
| # than the read timeout (#7987). Presetting the header suppresses that | ||
| # derivation; the read timeout below still bounds per-chunk silence. | ||
| headers["X-Server-Timeout"] = str(int(LONG_DECODE_HTTP_TIMEOUT_MS / 1000)) |
There was a problem hiding this comment.
Preset X-Server-Timeout only when the plugin set the timeout itself. If a user passes http_options.timeout, they probably want that deadline on the server too.
- drop the duplicated transport block that mutated the caller's HttpOptions in place; the remaining copy uses model_copy() - preset X-Server-Timeout only when the plugin chose the timeout itself, so a user-set http_options.timeout keeps its whole-request deadline on the server as well - compress explanatory comments to one line each - close LLM instances deterministically in the google tests (forced httpx transport makes genai's client close nondeterministically at GC)
|
All three nits are in (05a0a1e): deleted the first transport copy, kept the model_copy one; X-Server-Timeout is now preset only when the plugin picked the timeout, with a test asserting a user-set http_options.timeout leaves the header untouched; comments shortened to one line each. CI is green on the new head, including unit-tests. |
This branch has not been deployed
2 participants
The Google plugin turned
conn_options.timeoutinto two whole-request deadlines: genai derivesX-Server-Timeoutfrom it (Vertex enforces it with 504), and aiohttp'sClientTimeout(total=...)surfaces as CancelledError. Any healthy answer streaming longer than the read timeout dies mid-sentence with no failover, since chunks were already sent.Two changes in the request path:
async_client_args={"transport": httpx.AsyncHTTPTransport()}forces the httpx path (genai's_use_aiohttpdefers to any user transport), where the ms timeout is a per-phase read timeout - the same semantics the openai/inference plugins get fromhttpx.Timeout.X-Server-Timeoutwith the long-decode ceiling; genai only fills that header when absent, so the server-side deadline is no longer derived from the read timeout.Fixes #7987. Test:
test_timeout_is_read_timeout_not_whole_request_deadlineasserts the transport type and the header on the outgoing config; full plugin suite 40/40 with google-genai 2.31.0.