From 5823a82b6ceaeb5e4d21ae4b31a41b6118b340ec Mon Sep 17 00:00:00 2001 From: RoyVivat Date: Thu, 19 Mar 2026 13:40:41 -0700 Subject: [PATCH] =?UTF-8?q?total=20is=20derived=20as=20max(pool,=20connect?= =?UTF-8?q?)=20+=20read=20=E2=80=94=20consistent=20with=20the=20aiohttp=20?= =?UTF-8?q?docs,=20which=20specify=20that=20sock=5Fconnect=20(TCP=20handsh?= =?UTF-8?q?ake)=20is=20nested=20inside=20connect=20(pool=20acquisition)=20?= =?UTF-8?q?for=20new=20connections,=20so=20the=20two=20are=20not=20additiv?= =?UTF-8?q?e.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../llms/custom_httpx/aiohttp_transport.py | 24 +++--- .../custom_httpx/test_aiohttp_transport.py | 75 +++++++++++++++---- 2 files changed, 77 insertions(+), 22 deletions(-) diff --git a/litellm/llms/custom_httpx/aiohttp_transport.py b/litellm/llms/custom_httpx/aiohttp_transport.py index efba32f4125..27b5d6709ad 100644 --- a/litellm/llms/custom_httpx/aiohttp_transport.py +++ b/litellm/llms/custom_httpx/aiohttp_transport.py @@ -265,12 +265,18 @@ class LiteLLMAiohttpTransport(AiohttpTransport): # overriding the session/connector defaults with None (which is # not a valid value for aiohttp's ssl parameter). - # Derive a `total` deadline from the per-phase values so that - # aiohttp doesn't leave it as None. Without this, long-running - # reasoning models (e.g. GPT-5-PRO) can hit infrastructure idle - # timeouts (~60s) before the model responds. - _phase_values = [v for v in timeout.values() if v is not None] - _total = max(_phase_values) if _phase_values else None + # Set total = max(pool, connect) + read so aiohttp has a finite overall + # deadline. Per the aiohttp docs, `connect` (pool acquisition) already + # encompasses `sock_connect` (TCP handshake) for new connections, so + # they are not additive. `write` is excluded — aiohttp has no + # corresponding ClientTimeout field. + _sock_connect = timeout.get("connect") + _sock_read = timeout.get("read") + _pool = timeout.get("pool") + _conn_phase_values = [v for v in (_sock_connect, _pool) if v is not None] + _conn_phase = max(_conn_phase_values) if _conn_phase_values else None + _total_values = [v for v in (_conn_phase, _sock_read) if v is not None] + _total = sum(_total_values) if _total_values else None request_kwargs: Dict[str, Any] = { "method": request.method, @@ -281,9 +287,9 @@ class LiteLLMAiohttpTransport(AiohttpTransport): "auto_decompress": False, "timeout": ClientTimeout( total=_total, - sock_connect=timeout.get("connect"), - sock_read=timeout.get("read"), - connect=timeout.get("pool"), + sock_connect=_sock_connect, + sock_read=_sock_read, + connect=_pool, ), "proxy": proxy, "server_hostname": sni_hostname, diff --git a/tests/test_litellm/llms/custom_httpx/test_aiohttp_transport.py b/tests/test_litellm/llms/custom_httpx/test_aiohttp_transport.py index 86075a3f496..e141538c04c 100644 --- a/tests/test_litellm/llms/custom_httpx/test_aiohttp_transport.py +++ b/tests/test_litellm/llms/custom_httpx/test_aiohttp_transport.py @@ -5,6 +5,7 @@ import sys import aiohttp import aiohttp.client_exceptions import aiohttp.http_exceptions +from aiohttp import ClientTimeout import httpx import pytest @@ -556,19 +557,56 @@ async def test_handle_closed_session_before_request(): assert response.status_code == 200 -@pytest.mark.asyncio -async def test_client_timeout_total_set_to_max_phase_value(): +def test_aiohttp_client_timeout_total_none_is_the_bug(): """ - Regression test for issue #22747: ClientTimeout.total must be set to the - maximum phase timeout so aiohttp has an overall deadline. + Directly demonstrates the bug from issue #22747 using aiohttp's API. + + Building a ClientTimeout without ``total=...`` leaves ``total`` as None, + giving aiohttp no overall request deadline. This is exactly what the old + transport produced for every user request, regardless of their configured + timeout value. + + The fix derives ``total`` from the three phases that aiohttp uses + (sock_connect, sock_read, connect/pool) using sum(), which is the safe + upper bound. max() would prematurely abort requests where multiple phases + each approach their individual limits. + """ + # What the OLD transport code produced for timeout=300 (the bug): + old_timeout = ClientTimeout( + sock_connect=300.0, + sock_read=300.0, + connect=300.0, + # total was never set — this is the bug + ) + assert old_timeout.total is None # no overall deadline! + + # What the NEW transport code produces for timeout=300 (the fix): + # connection phase = max(sock_connect=300, connect=300) = 300 + # total = connection_phase + sock_read = 300 + 300 = 600 s + new_timeout = ClientTimeout( + total=600.0, + sock_connect=300.0, + sock_read=300.0, + connect=300.0, + ) + assert new_timeout.total == 600.0 # finite, safe upper-bound deadline + + +@pytest.mark.asyncio +async def test_client_timeout_total_derived_from_connection_and_read_phases(): + """ + Regression test for issue #22747: ClientTimeout.total must be set so that + aiohttp has a finite overall request deadline. Without this fix, total=None caused aiohttp to have no overall deadline, - allowing infrastructure idle timeouts (~60s) to kill connections before + allowing infrastructure idle-timeouts (~60 s) to kill connections before long-running reasoning models (e.g. GPT-5-PRO) responded. httpx converts ``timeout=300`` into all four phase keys set to 300. - The ``write`` key is included in ``_phase_values`` via ``.values()`` but is - not mapped to any aiohttp ``ClientTimeout`` field — that's expected and fine. + Per the aiohttp docs, sock_connect is nested inside connect (they are not + additive for new connections), so total = max(pool, connect) + read. + ``write`` is excluded — it has no corresponding aiohttp field. + Expected total = max(300, 300) + 300 = 600. """ captured: dict = {} transport = LiteLLMAiohttpTransport(client=lambda: _make_capturing_session(captured)) # type: ignore @@ -576,33 +614,44 @@ async def test_client_timeout_total_set_to_max_phase_value(): request.extensions["timeout"] = { "connect": 300.0, "read": 300.0, - "write": 300.0, + "write": 300.0, # excluded — not mapped to any aiohttp field "pool": 300.0, } await transport.handle_async_request(request) timeout = captured["timeout"] assert timeout is not None - assert timeout.total == 300.0, f"Expected total=300.0, got total={timeout.total}" + assert timeout.total == 600.0, f"Expected total=600.0, got total={timeout.total}" assert timeout.sock_connect == 300.0 assert timeout.sock_read == 300.0 @pytest.mark.asyncio -async def test_client_timeout_total_uses_max_when_phases_differ(): - """total should be the max of all provided phase timeouts.""" +async def test_client_timeout_total_no_false_timeout_when_phases_differ(): + """ + total must not prematurely abort a request where multiple phases each + approach their individual limits. + + With connect=10 s (TCP) and pool=5 s (pool acquisition), the effective + connection limit is max(10, 5) = 10 s, because sock_connect is nested + inside connect per the aiohttp docs. Then read=600 s follows. + Expected total = max(10, 5) + 600 = 610 s. + + Using plain max() of all phases would give total=600 s, which would + false-timeout a request that takes 9 s to connect + 602 s to read. + """ captured: dict = {} transport = LiteLLMAiohttpTransport(client=lambda: _make_capturing_session(captured)) # type: ignore request = httpx.Request("GET", "http://example.com") request.extensions["timeout"] = { "connect": 10.0, - "read": 600.0, # Longest — should become total + "read": 600.0, "pool": 5.0, } await transport.handle_async_request(request) timeout = captured["timeout"] - assert timeout.total == 600.0, f"Expected total=600.0, got total={timeout.total}" + assert timeout.total == 610.0, f"Expected total=610.0, got total={timeout.total}" @pytest.mark.asyncio