Skip to content

Commit e03f903

Browse files
committed
fix(server): drain publisher+connection pending on WS disconnect
Two sibling bugs, same disconnect-drain class as the client/exposure fixes: 1. unregister_publisher() popped the PublisherRegistration but left publisher.pending unresolved. Non-streaming proxy_chat() futures then waited the full 60 s asyncio.wait_for timeout; streaming proxy_chat() queues blocked FOREVER on `await queue.get()` because no sentinel was ever put — the SSE event_stream inner loop has no timeout, only a None sentinel check. Result: streaming clients hung indefinitely after any publisher WS disconnect. 2. unregister_connection() popped the ConnectionRegistration but left conn.pending unresolved. invoke_capability() futures waited the full 60-second timeout instead of failing immediately on client disconnect. Fix: both methods now capture the registration before popping: - publisher: asyncio.Future → set LookupError("publisher disconnected"); asyncio.Queue → put_nowait(None) (existing stream sentinel) - connection: asyncio.Future → set LookupError("connection disconnected") The existing LookupError catch in the HTTP handlers converts these to clean 404s. The SSE event_stream exits the inner loop on None and emits [DONE] — stream terminates instead of hanging.
1 parent 5211cbb commit e03f903

2 files changed

Lines changed: 176 additions & 3 deletions

File tree

tests/test_hub_disconnect_drain.py

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
"""Hub-side pending drains on publisher/connection disconnect.
2+
3+
Two sibling bugs fixed in the same commit:
4+
5+
1. unregister_publisher() did not drain publisher.pending on WS disconnect.
6+
Non-streaming proxy_chat() futures waited 60 s (asyncio.wait_for timeout).
7+
Streaming proxy_chat() queues blocked FOREVER on queue.get() — no timeout.
8+
9+
2. unregister_connection() did not drain conn.pending on WS disconnect.
10+
invoke_capability() futures waited 60 s for the full timeout.
11+
12+
Post-fix: both methods capture the registration before popping, then:
13+
- publisher: Future → set LookupError; Queue → put_nowait(None) (stream sentinel)
14+
- connection: Future → set LookupError
15+
16+
These tests exercise the drain logic directly without spinning up a full hub.
17+
"""
18+
19+
import asyncio
20+
import time
21+
22+
import pytest
23+
24+
from zhub.server import ConnectionRegistration, PublisherRegistration
25+
26+
27+
# ---- helpers ----------------------------------------------------------------
28+
29+
def _make_pub(name: str = "test-ai") -> PublisherRegistration:
30+
pub = PublisherRegistration.__new__(PublisherRegistration)
31+
pub.name = name
32+
pub.manifest = {}
33+
pub.websocket = None # type: ignore[assignment]
34+
pub.api_key_hash = "abc"
35+
pub.created_at = time.time()
36+
pub.pending = {}
37+
return pub
38+
39+
40+
def _make_conn(name: str = "test-conn") -> ConnectionRegistration:
41+
conn = ConnectionRegistration.__new__(ConnectionRegistration)
42+
conn.connection_id = name
43+
conn.ai_name = "test-ai"
44+
conn.websocket = None # type: ignore[assignment]
45+
conn.client_manifest = {}
46+
conn.created_at = time.time()
47+
conn.pending = {}
48+
return conn
49+
50+
51+
def _drain_publisher(pub: PublisherRegistration) -> None:
52+
"""Mirrors the cleanup added to Hub.unregister_publisher()."""
53+
err = LookupError("publisher disconnected")
54+
for item in list(pub.pending.values()):
55+
if isinstance(item, asyncio.Future):
56+
if not item.done():
57+
item.set_exception(err)
58+
elif isinstance(item, asyncio.Queue):
59+
item.put_nowait(None)
60+
pub.pending.clear()
61+
62+
63+
def _drain_connection(conn: ConnectionRegistration) -> None:
64+
"""Mirrors the cleanup added to Hub.unregister_connection()."""
65+
err = LookupError("connection disconnected")
66+
for fut in list(conn.pending.values()):
67+
if not fut.done():
68+
fut.set_exception(err)
69+
conn.pending.clear()
70+
71+
72+
# ---- publisher tests --------------------------------------------------------
73+
74+
@pytest.mark.asyncio
75+
async def test_publisher_non_streaming_future_raises_on_disconnect():
76+
"""Non-streaming proxy_chat() future must raise LookupError immediately
77+
on publisher disconnect, not hang for the 60-second asyncio.wait_for timeout."""
78+
pub = _make_pub()
79+
loop = asyncio.get_running_loop()
80+
fut = loop.create_future()
81+
pub.pending["req-1"] = fut
82+
83+
_drain_publisher(pub)
84+
85+
assert not pub.pending
86+
assert fut.done()
87+
with pytest.raises(LookupError, match="disconnected"):
88+
fut.result()
89+
90+
91+
@pytest.mark.asyncio
92+
async def test_publisher_streaming_queue_gets_sentinel_on_disconnect():
93+
"""Streaming proxy_chat() queue must receive None (the stream-end sentinel)
94+
so event_stream's inner `while True: chunk = await queue.get()` loop exits
95+
instead of blocking forever on publisher disconnect."""
96+
pub = _make_pub()
97+
q: asyncio.Queue = asyncio.Queue()
98+
pub.pending["req-2"] = q # type: ignore[assignment]
99+
100+
_drain_publisher(pub)
101+
102+
assert not pub.pending
103+
sentinel = q.get_nowait()
104+
assert sentinel is None # this is what event_stream checks `if chunk is None: break`
105+
106+
107+
@pytest.mark.asyncio
108+
async def test_publisher_drain_skips_already_resolved_futures():
109+
"""Resolved futures must not raise InvalidStateError during drain."""
110+
pub = _make_pub()
111+
loop = asyncio.get_running_loop()
112+
fut = loop.create_future()
113+
fut.set_result({"text": "already done"})
114+
pub.pending["req-3"] = fut
115+
116+
_drain_publisher(pub) # must not raise
117+
118+
assert fut.result() == {"text": "already done"}
119+
120+
121+
# ---- connection tests -------------------------------------------------------
122+
123+
@pytest.mark.asyncio
124+
async def test_connection_invoke_future_raises_on_disconnect():
125+
"""invoke_capability() future must raise LookupError immediately on
126+
client disconnect, not wait for the 60-second timeout."""
127+
conn = _make_conn()
128+
loop = asyncio.get_running_loop()
129+
fut = loop.create_future()
130+
conn.pending["req-4"] = fut
131+
132+
_drain_connection(conn)
133+
134+
assert not conn.pending
135+
assert fut.done()
136+
with pytest.raises(LookupError, match="disconnected"):
137+
fut.result()
138+
139+
140+
@pytest.mark.asyncio
141+
async def test_connection_drain_skips_already_resolved_futures():
142+
"""Resolved futures must not raise InvalidStateError during drain."""
143+
conn = _make_conn()
144+
loop = asyncio.get_running_loop()
145+
fut = loop.create_future()
146+
fut.set_result({"ok": True, "result": "pong"})
147+
conn.pending["req-5"] = fut
148+
149+
_drain_connection(conn) # must not raise
150+
151+
assert fut.result() == {"ok": True, "result": "pong"}

zhub/server.py

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -335,13 +335,27 @@ async def register_publisher(self, name: str, manifest: dict[str, Any],
335335

336336
async def unregister_publisher(self, name: str) -> None:
337337
async with self.lock:
338-
self.publishers.pop(name, None)
338+
publisher = self.publishers.pop(name, None)
339339
for k, v in list(self.api_keys.items()):
340340
if v == name:
341341
self.api_keys.pop(k, None)
342342
self.connections_by_ai.pop(name, None)
343343
self._rate_windows.pop(name, None)
344-
log.info("publisher unregistered: %s", name)
344+
if publisher is not None:
345+
# Fail/terminate any in-flight proxy_chat() calls immediately on
346+
# disconnect rather than waiting for their full timeout:
347+
# - non-streaming futures: raise LookupError to the HTTP handler
348+
# - streaming queues: put None (the existing stream-end sentinel)
349+
# so the SSE event_stream loop exits instead of blocking forever
350+
err = LookupError("publisher disconnected")
351+
for item in list(publisher.pending.values()):
352+
if isinstance(item, asyncio.Future):
353+
if not item.done():
354+
item.set_exception(err)
355+
elif isinstance(item, asyncio.Queue):
356+
item.put_nowait(None)
357+
publisher.pending.clear()
358+
log.info("publisher unregistered: %s", name)
345359

346360
def lookup_by_api_key(self, api_key: str) -> Optional[str]:
347361
return self.api_keys.get(api_key)
@@ -392,12 +406,20 @@ async def register_connection(self, ai_name: str, api_key: str,
392406

393407
async def unregister_connection(self, ai_name: str, connection_id: str) -> None:
394408
async with self.lock:
395-
self.connections_by_ai.get(ai_name, {}).pop(connection_id, None)
409+
conn = self.connections_by_ai.get(ai_name, {}).pop(connection_id, None)
396410
if ai_name in self.publishers:
397411
await self._send_to_publisher(
398412
ai_name,
399413
connection_event("disconnected", connection_id, None),
400414
)
415+
if conn is not None:
416+
# Fail any in-flight invoke_capability() calls immediately on
417+
# client disconnect instead of hanging for the full 60-second timeout.
418+
err = LookupError("connection disconnected")
419+
for fut in list(conn.pending.values()):
420+
if not fut.done():
421+
fut.set_exception(err)
422+
conn.pending.clear()
401423
log.info("connection unregistered: %s", connection_id)
402424

403425
# exposures (Phase 7.0) --------------------------------------------

0 commit comments

Comments
 (0)