Skip to content

Commit b39ef1b

Browse files
fix: surface WebSocket close to tool calls instead of hanging (#1773)
* fix: surface WebSocket close to tool calls instead of hanging (#1721) When the WebSocket to Home Assistant closed while a command awaited its response, the pending future was cancelled; the resulting CancelledError escaped every except-Exception handler and reached the MCP SDK, which suppressed the response entirely. The MCP client then hung until its own timeout (4 minutes in Claude clients) with only a bare 'WebSocket connection closed' log line to go on. - Fail pending request/event futures with HomeAssistantConnectionError (carrying the close code/reason) instead of cancelling them, so tools raise a structured error immediately. - Log the close code and reason (e.g. 1009 message-too-big) instead of discarding them; WARNING for abnormal closes. - Raise the client max_size 20MB -> 64MB, matching the Supervisor's own Core-connection limit: config/entity_registry/list arrives as one frame that scales with entity count (no pagination in the HA WS API) and overflowed 20MB on a ~6.4k-entity instance. - Chunk config/entity_registry/get_entries alias enrichment (500 ids per call) so its response frame stays bounded on large instances. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: use future.result() to re-raise, satisfying py/ineffectual-statement Bare 'await request_future' statements tripped CodeQL's py/ineffectual-statement (await of a Name reads as an expression with no effect). The futures are already done after reset_connection sets the exception, so result() re-raises the stored exception directly -- a call expression the rule does not flag, and a more direct assertion of the state contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: retrieve orphaned event-future exception; surface alias-fetch warnings Review-driven hardening on top of the #1721 fix: - send_command_with_event: a connection drop during the result phase now retrieves event_future's exception (reset_connection fails BOTH futures; only result_future was awaited), preventing asyncio's ERROR-level 'Future exception was never retrieved' on the exact path this PR makes diagnosable. Regression-tested red/green via asyncio's retrieved-flag. - _fetch_entity_aliases now returns (aliases_map, warnings): failed get_entries chunks surface as a top-level warning in search results instead of silently returning success with missing aliases. - Close the untested branches flagged by review: self-initiated close (sent 1009 -- the literal #1721 trigger), WARNING-vs-INFO level split, abrupt no-close-frame drop, generic handler exception teardown, max_size wiring into websockets.connect, CancelledError propagation in the chunk loop, malformed success payload tolerance. - Docstring: drop hardcoded 500 duplicating _GET_ENTRIES_CHUNK_SIZE. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 97ec188 commit b39ef1b

4 files changed

Lines changed: 678 additions & 39 deletions

File tree

src/ha_mcp/client/websocket_client.py

Lines changed: 72 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,15 @@
3030

3131
logger = logging.getLogger(__name__)
3232

33+
# Matches the Supervisor's own Core-connection receive limit
34+
# (MAX_MESSAGE_SIZE_FROM_CORE in home-assistant/supervisor, see supervisor
35+
# issue #4392). On the add-on path frames above that limit die at the
36+
# Supervisor proxy anyway, so a larger client-side cap adds nothing there.
37+
# Registry list responses scale with entity count and arrive as ONE frame
38+
# (the HA WebSocket API has no pagination); a ~6.4k-entity instance
39+
# overflowed the previous 20MB cap (#1721).
40+
MAX_WS_MESSAGE_BYTES = 64 * 1024 * 1024
41+
3342

3443
class WebSocketConnectionState:
3544
"""Encapsulates mutable state used by the WebSocket client."""
@@ -109,20 +118,43 @@ def consume_auth_message(self, message_type: str) -> dict[str, Any] | None:
109118
"""Retrieve and remove an authentication message if present."""
110119
return self._auth_messages.pop(message_type, None)
111120

112-
def reset_connection(self) -> None:
113-
"""Reset connection-specific state while preserving handlers."""
121+
def reset_connection(self, close_reason: str | None = None) -> None:
122+
"""Reset connection-specific state while preserving handlers.
123+
124+
Args:
125+
close_reason: Human-readable description of why the connection
126+
went away (e.g. a close code/reason pair), included in the
127+
error surfaced to any request still awaiting a response.
128+
"""
114129
self.connected = False
115130
self.authenticated = False
116131
self._message_id = 0
117132

133+
# ``future.cancel()`` makes awaiters see ``asyncio.CancelledError``,
134+
# a BaseException that skips every ``except Exception`` handler in
135+
# the tool layer and reaches the MCP SDK, which treats it as a
136+
# client-initiated cancellation, suppresses the response entirely,
137+
# and leaves the MCP client hanging until its own timeout (#1721).
138+
# ``set_exception`` with a normal exception propagates through
139+
# ``except Exception`` as expected instead.
140+
message = (
141+
"WebSocket connection to Home Assistant closed while waiting for a response"
142+
)
143+
if close_reason:
144+
message = f"{message} ({close_reason})"
145+
118146
for future in self._pending_requests.values():
119147
if not future.done():
120-
future.cancel()
148+
# A fresh instance per future: sharing one exception object
149+
# across multiple ``set_exception`` calls means the second
150+
# raise attaches a traceback to an exception already
151+
# associated with another future's stack.
152+
future.set_exception(HomeAssistantConnectionError(message))
121153
self._pending_requests.clear()
122154

123155
for future in self._event_responses.values():
124156
if not future.done():
125-
future.cancel()
157+
future.set_exception(HomeAssistantConnectionError(message))
126158
self._event_responses.clear()
127159

128160
# Drop any subscription queues — readers wake on the close signal
@@ -144,9 +176,9 @@ def mark_authenticated(self) -> None:
144176
"""Mark the socket as authenticated and ready for commands."""
145177
self.authenticated = True
146178

147-
def mark_disconnected(self) -> None:
179+
def mark_disconnected(self, close_reason: str | None = None) -> None:
148180
"""Reset connection state when the socket is closed."""
149-
self.reset_connection()
181+
self.reset_connection(close_reason)
150182

151183
@property
152184
def is_ready(self) -> bool:
@@ -293,9 +325,7 @@ async def connect(self) -> bool:
293325
ping_timeout=10,
294326
additional_headers={"Authorization": f"Bearer {self.token}"},
295327
ssl=ssl_ctx,
296-
# Increase max message size to 20MB for large responses
297-
# (e.g., HACS repository list can be 2MB+)
298-
max_size=20 * 1024 * 1024,
328+
max_size=MAX_WS_MESSAGE_BYTES,
299329
)
300330
self._state.mark_connected()
301331

@@ -390,6 +420,11 @@ async def _message_handler(self) -> None:
390420
"""Background task to handle incoming WebSocket messages."""
391421
if not self.websocket:
392422
raise Exception("WebSocket not connected")
423+
# None means a clean exit (the async-for loop simply ended); the
424+
# except blocks below fill this in so pending futures — and the
425+
# log — carry *why* the connection went away instead of a bare
426+
# "WebSocket connection closed" that gave no lead on #1721.
427+
close_reason: str | None = None
393428
try:
394429
async for message in self.websocket:
395430
try:
@@ -400,12 +435,30 @@ async def _message_handler(self) -> None:
400435
logger.error(f"Invalid JSON received: {e}")
401436
except Exception as e:
402437
logger.error(f"Error processing message: {e}")
403-
except websockets.exceptions.ConnectionClosed:
404-
logger.info("WebSocket connection closed")
438+
except websockets.exceptions.ConnectionClosed as e:
439+
# Prefer the frame we received (the peer closed on us); fall
440+
# back to the frame we sent (we failed the connection
441+
# ourselves, e.g. an over-max_size frame produces a sent
442+
# Close(1009, ...) with no frame received at all).
443+
close = e.rcvd if e.rcvd is not None else e.sent
444+
if close is not None:
445+
verb = "received" if e.rcvd is not None else "sent"
446+
close_reason = f"{verb} close code {close.code}"
447+
if close.reason:
448+
close_reason = f"{close_reason} ({close.reason})"
449+
else:
450+
close_reason = "connection dropped without a close frame"
451+
452+
log_message = f"WebSocket connection closed ({close_reason})"
453+
if close is None or close.code not in (1000, 1001):
454+
logger.warning(log_message)
455+
else:
456+
logger.info(log_message)
405457
except Exception as e:
458+
close_reason = str(e)
406459
logger.error(f"WebSocket message handler error: {e}")
407460
finally:
408-
self._state.mark_disconnected()
461+
self._state.mark_disconnected(close_reason)
409462

410463
async def _process_message(self, data: dict[str, Any]) -> None:
411464
"""Process incoming WebSocket message."""
@@ -649,9 +702,15 @@ async def send_command_with_event(
649702
result_response = await asyncio.wait_for(
650703
result_future, timeout=wait_timeout
651704
)
652-
except TimeoutError:
705+
except BaseException:
653706
self.cancel_pending_response(message_id)
654707
self.cancel_event_response(message_id)
708+
# A connection drop fails BOTH futures via reset_connection;
709+
# only result_future gets awaited on this path, so retrieve
710+
# event_future's exception too or asyncio logs an ERROR-level
711+
# "Future exception was never retrieved" when it is GC'd.
712+
if event_future.done() and not event_future.cancelled():
713+
event_future.exception()
655714
raise
656715

657716
if not result_response.get("success"):

src/ha_mcp/tools/smart_search/_entities.py

Lines changed: 80 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@
1212

1313
logger = logging.getLogger(__name__)
1414

15+
# Bounds the per-frame response size of config/entity_registry/get_entries
16+
# (extended entries, ~1KB typical each) so alias enrichment can't produce an
17+
# over-cap WebSocket frame on large instances (#1721).
18+
_GET_ENTRIES_CHUNK_SIZE = 500
19+
1520

1621
class EntitySearchMixin(_SearchBase):
1722
"""``smart_entity_search`` and ``get_entities_by_area`` plus helpers."""
@@ -144,41 +149,89 @@ def _filter_hidden_entities(
144149

145150
async def _fetch_entity_aliases(
146151
self, survivor_ids: list[str]
147-
) -> dict[str, list[str]]:
152+
) -> tuple[dict[str, list[str]], list[str]]:
148153
"""Batch-fetch full registry entries for aliases.
149154
150155
``config/entity_registry/list`` deliberately omits ``aliases``;
151-
``get_entries`` includes them. One extra round-trip enriches the
152-
survivor set without N+1 fan-out.
156+
``get_entries`` includes them. Survivors are split into
157+
``_GET_ENTRIES_CHUNK_SIZE``-sized chunks fetched concurrently: a
158+
bounded number of round-trips (one per chunk), not N+1 fan-out, and
159+
each response frame stays bounded regardless of instance size.
160+
Enrichment is best-effort per chunk — one chunk failing does not
161+
drop aliases fetched by the others; failures are reported in the
162+
returned warnings so the caller can tell "no aliases exist" apart
163+
from "the alias fetch failed".
164+
165+
Returns:
166+
``(aliases_map, warnings)`` — warnings has one entry when any
167+
chunk failed, empty otherwise.
153168
"""
154169
aliases_map: dict[str, list[str]] = {}
155170
if not survivor_ids:
156-
return aliases_map
157-
try:
158-
entries_resp = await self.client.send_websocket_message(
159-
{
160-
"type": "config/entity_registry/get_entries",
161-
"entity_ids": survivor_ids,
162-
}
163-
)
164-
if isinstance(entries_resp, dict) and entries_resp.get("success"):
165-
for eid, entry in (entries_resp.get("result", {}) or {}).items():
166-
if isinstance(entry, dict):
167-
aliases_map[eid] = entry.get("aliases", []) or []
168-
else:
171+
return aliases_map, []
172+
173+
chunks: list[list[str]] = [
174+
survivor_ids[i : i + _GET_ENTRIES_CHUNK_SIZE]
175+
for i in range(0, len(survivor_ids), _GET_ENTRIES_CHUNK_SIZE)
176+
]
177+
responses: list[Any] = await asyncio.gather(
178+
*(
179+
self.client.send_websocket_message(
180+
{
181+
"type": "config/entity_registry/get_entries",
182+
"entity_ids": chunk,
183+
}
184+
)
185+
for chunk in chunks
186+
),
187+
return_exceptions=True,
188+
)
189+
190+
failed_chunks = 0
191+
for chunk, entries_resp in zip(chunks, responses, strict=True):
192+
# Same convention as _fetch_search_entities: a captured
193+
# CancelledError means the surrounding task is being torn
194+
# down — propagate it instead of degrading to a warning.
195+
if isinstance(entries_resp, asyncio.CancelledError):
196+
raise entries_resp
197+
if isinstance(entries_resp, BaseException):
198+
failed_chunks += 1
169199
logger.warning(
170-
"alias_enrichment_failed: get_entries returned non-success "
171-
"for %d entities (resp=%r)",
172-
len(survivor_ids),
200+
"alias_enrichment_failed: get_entries chunk of %d entities "
201+
"raised (err=%r)",
202+
len(chunk),
173203
entries_resp,
174204
)
175-
except (KeyError, TypeError, AttributeError) as alias_err:
176-
logger.warning(
177-
"alias_enrichment_failed: malformed payload for %d entities (err=%r)",
178-
len(survivor_ids),
179-
alias_err,
205+
continue
206+
try:
207+
if isinstance(entries_resp, dict) and entries_resp.get("success"):
208+
for eid, entry in (entries_resp.get("result", {}) or {}).items():
209+
if isinstance(entry, dict):
210+
aliases_map[eid] = entry.get("aliases", []) or []
211+
else:
212+
failed_chunks += 1
213+
logger.warning(
214+
"alias_enrichment_failed: get_entries returned non-success "
215+
"for a chunk of %d entities (resp=%r)",
216+
len(chunk),
217+
entries_resp,
218+
)
219+
except (KeyError, TypeError, AttributeError) as alias_err:
220+
failed_chunks += 1
221+
logger.warning(
222+
"alias_enrichment_failed: malformed payload for a chunk of "
223+
"%d entities (err=%r)",
224+
len(chunk),
225+
alias_err,
226+
)
227+
warnings: list[str] = []
228+
if failed_chunks:
229+
warnings.append(
230+
f"Alias enrichment incomplete: {failed_chunks} of {len(chunks)} "
231+
"entity-registry lookups failed; alias-based matches may be "
232+
"missing from these results."
180233
)
181-
return aliases_map
234+
return aliases_map, warnings
182235

183236
async def _fetch_search_entities(
184237
self, domain_filter: str | None, include_hidden: bool
@@ -223,7 +276,8 @@ async def _fetch_search_entities(
223276
survivor_ids, survivor_states = self._filter_hidden_entities(
224277
entities, registry_slim, include_hidden, visibility_hidden
225278
)
226-
aliases_map = await self._fetch_entity_aliases(survivor_ids)
279+
aliases_map, alias_warnings = await self._fetch_entity_aliases(survivor_ids)
280+
visibility_warnings = [*visibility_warnings, *alias_warnings]
227281

228282
# Enrich with aliases + hidden_by for the fuzzy layer. Shallow copy +
229283
# private-prefixed keys so downstream consumers that round-trip these

0 commit comments

Comments
 (0)