Skip to content

Commit 1313e0b

Browse files
committed
client: drain _pending and _streams on WS disconnect
In-flight chat() and chat_stream() calls on a ZhubConnection were silently waiting up to 60 s (their timeout) when the underlying WebSocket dropped — because _serve_one_session exited without notifying them. Add a finally block that: - sets ZhubConnectionError on every unresolved future in _pending - sends a done/error sentinel to every queue in _streams - clears both dicts and nulls _ws Callers now raise immediately on disconnect instead of blocking until the per-request timeout fires. Cover the cleanup logic in tests/test_client_disconnect_drain.py.
1 parent 16d4c45 commit 1313e0b

2 files changed

Lines changed: 142 additions & 31 deletions

File tree

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
"""ZhubConnection._pending + _streams are drained on WS disconnect.
2+
3+
Pre-fix: when _serve_one_session exited (WS drop / hub restart), any in-flight
4+
chat() or chat_stream() calls silently waited up to 60 s before their
5+
asyncio.wait_for timeout fired.
6+
7+
Post-fix: a finally block in _serve_one_session fails all pending futures and
8+
sends done-with-error sentinels to all stream queues, so callers raise
9+
immediately.
10+
11+
These tests exercise the cleanup logic directly on ZhubConnection state,
12+
without spinning up a real hub.
13+
"""
14+
15+
import asyncio
16+
17+
import pytest
18+
19+
from zhub.client import ZhubConnection
20+
from zhub.errors import ConnectionError as ZhubConnectionError
21+
from zhub.manifest import Manifest
22+
23+
24+
def _make_conn() -> ZhubConnection:
25+
return ZhubConnection(
26+
ai_name="ai",
27+
api_key="zk_test",
28+
hub_url="ws://localhost",
29+
client_manifest=Manifest(name="ai-client"),
30+
capabilities={},
31+
)
32+
33+
34+
def _run_disconnect_cleanup(conn: ZhubConnection) -> None:
35+
"""Mirrors the finally block added to _serve_one_session in client.py."""
36+
err = ZhubConnectionError("connection closed")
37+
for fut in list(conn._pending.values()):
38+
if not fut.done():
39+
fut.set_exception(err)
40+
conn._pending.clear()
41+
for q in list(conn._streams.values()):
42+
q.put_nowait({"done": True, "error": "connection closed"})
43+
conn._streams.clear()
44+
conn._ws = None
45+
46+
47+
@pytest.mark.asyncio
48+
async def test_pending_future_raises_on_disconnect():
49+
"""A future in _pending must raise ZhubConnectionError, not hang."""
50+
conn = _make_conn()
51+
loop = asyncio.get_running_loop()
52+
fut = loop.create_future()
53+
conn._pending["req-1"] = fut
54+
55+
_run_disconnect_cleanup(conn)
56+
57+
assert not conn._pending, "pending should be empty after cleanup"
58+
assert fut.done(), "future should be resolved"
59+
with pytest.raises(ZhubConnectionError):
60+
fut.result()
61+
62+
63+
@pytest.mark.asyncio
64+
async def test_stream_queue_gets_done_sentinel_on_disconnect():
65+
"""A stream queue in _streams must receive the done sentinel so
66+
chat_stream() exits instead of blocking on the next chunk forever."""
67+
conn = _make_conn()
68+
q: asyncio.Queue = asyncio.Queue()
69+
conn._streams["req-2"] = q
70+
71+
_run_disconnect_cleanup(conn)
72+
73+
assert not conn._streams, "streams should be empty after cleanup"
74+
item = q.get_nowait()
75+
assert item.get("done") is True
76+
assert "error" in item
77+
78+
79+
@pytest.mark.asyncio
80+
async def test_cleanup_skips_already_done_futures():
81+
"""Already-resolved futures must not raise InvalidStateError."""
82+
conn = _make_conn()
83+
loop = asyncio.get_running_loop()
84+
fut = loop.create_future()
85+
fut.set_result({"text": "done already"})
86+
conn._pending["req-3"] = fut
87+
88+
# Must not raise
89+
_run_disconnect_cleanup(conn)
90+
assert fut.result() == {"text": "done already"}
91+
92+
93+
@pytest.mark.asyncio
94+
async def test_ws_is_none_after_cleanup():
95+
conn = _make_conn()
96+
conn._ws = object() # type: ignore[assignment]
97+
_run_disconnect_cleanup(conn)
98+
assert conn._ws is None

zhub/client.py

Lines changed: 44 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -497,37 +497,50 @@ def connect(
497497
)
498498

499499
async def _serve_one_session(url: str) -> None:
500-
async with websockets.connect(url, max_size=10_000_000) as ws:
501-
conn._ws = ws
502-
await ws.send(register_connection(ai_name, api_key, cm.to_dict()).to_json())
503-
504-
async for raw in ws:
505-
env = Envelope.from_json(raw)
506-
507-
if env.type == "registered":
508-
log.info("client registered to %s", ai_name)
509-
510-
elif env.type == "chat-response":
511-
fut = conn._pending.get(env.request_id)
512-
if fut and not fut.done():
513-
fut.set_result(env.payload)
514-
queue = conn._streams.get(env.request_id)
515-
if queue is not None:
516-
await queue.put({"delta": env.payload.get("text", ""), "done": False})
517-
await queue.put({"done": True})
518-
519-
elif env.type == "chat-chunk":
520-
queue = conn._streams.get(env.request_id)
521-
if queue is not None:
522-
await queue.put(env.payload)
523-
524-
elif env.type == "invoke-request":
525-
asyncio.create_task(_handle_invoke(conn, ws, env))
526-
527-
elif env.type == "error":
528-
log.warning("hub error: %s", env.payload)
529-
if env.payload.get("code") == "register_failed":
530-
raise AuthError(env.payload.get("message", "register failed"))
500+
try:
501+
async with websockets.connect(url, max_size=10_000_000) as ws:
502+
conn._ws = ws
503+
await ws.send(register_connection(ai_name, api_key, cm.to_dict()).to_json())
504+
505+
async for raw in ws:
506+
env = Envelope.from_json(raw)
507+
508+
if env.type == "registered":
509+
log.info("client registered to %s", ai_name)
510+
511+
elif env.type == "chat-response":
512+
fut = conn._pending.get(env.request_id)
513+
if fut and not fut.done():
514+
fut.set_result(env.payload)
515+
queue = conn._streams.get(env.request_id)
516+
if queue is not None:
517+
await queue.put({"delta": env.payload.get("text", ""), "done": False})
518+
await queue.put({"done": True})
519+
520+
elif env.type == "chat-chunk":
521+
queue = conn._streams.get(env.request_id)
522+
if queue is not None:
523+
await queue.put(env.payload)
524+
525+
elif env.type == "invoke-request":
526+
asyncio.create_task(_handle_invoke(conn, ws, env))
527+
528+
elif env.type == "error":
529+
log.warning("hub error: %s", env.payload)
530+
if env.payload.get("code") == "register_failed":
531+
raise AuthError(env.payload.get("message", "register failed"))
532+
finally:
533+
# Fail any in-flight chat() calls so callers get an immediate error
534+
# instead of hanging until their 60s timeout when the WS drops.
535+
err = ZhubConnectionError("connection closed")
536+
for fut in list(conn._pending.values()):
537+
if not fut.done():
538+
fut.set_exception(err)
539+
conn._pending.clear()
540+
for q in list(conn._streams.values()):
541+
await q.put({"done": True, "error": "connection closed"})
542+
conn._streams.clear()
543+
conn._ws = None
531544

532545
async def runner() -> None:
533546
url = _to_ws_url(hub_url, "/ws/connect")

0 commit comments

Comments
 (0)