Repository navigation
Report ESPHome transport failures as connection errors - #142
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #142 +/- ##
==========================================
+ Coverage 94.29% 94.34% +0.05%
==========================================
Files 25 25
Lines 4399 4405 +6
==========================================
+ Hits 4148 4156 +8
+ Misses 251 249 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Upstream issue to use ConnectionNotEstablishedAPIError for disconnected clients: esphome/aioesphomeapi#1948 |
There was a problem hiding this comment.
This review was written by a bot (zigpy-review-bot) and still needs to be checked by a maintainer.
The offline-API case from home-assistant/core#186950 is fixed: an already-disconnected API now raises ConnectionError. One gap remains, and the last commit is what reopened it.
A graceful disconnect still becomes SerialException
cef89ca dropped the type(exc) is APIConnectionError branch from 08b772e because ConnectionNotEstablishedAPIError now covers "not connected". That only covers an API that is already down when the call starts. When the connection closes cleanly, aioesphomeapi 46.21.0 still fails every in-flight request with a bare APIConnectionError("Connection closed") (connection.py:475-482). This happens when the device sends DisconnectRequest, which ESPHome does on every reboot and OTA, and when the owner calls api.disconnect() on a shared client. Both fall through to except APIConnectionError and become SerialException. A TCP drop (FIN/RST) is classified correctly, but the clean shutdown a rebooting proxy actually sends is not.
Restoring the exact-type check fixes it (verified below). The (APIConnectionError("Unclassified API error"), SerialException) row in test_open_api_error_classification would flip, and a test that fails a pending request with APIConnectionError("Connection closed") would cover the reboot case. The PR description's "removes the need for an exact-type check" should then be adjusted.
Details: where the bare exception comes from, reproduction, other bare raises
for fut in self._read_exception_futures:
if not fut.done():
err = self._fatal_exception or APIConnectionError("Connection closed")_fatal_exception is unset in exactly two cases. One is the device sending DisconnectRequest, which ESPHome sends to every client in APIServer::on_shutdown() (the "Processing expected disconnect" line in the HA log). The other is the owner calling api.disconnect(), for example HA's ESPHome integration reloading while a serial open is in flight. Mid-session this also clashes with the closed callback, which marks the port broken with OSError(EIO) for the same event.
I reproduced this with a fake plaintext ESPHome server (aioesphomeapi 46.21.0, API 1.15) on the PR head:
| Scenario | PR head | With the 08b772e branch restored |
|---|---|---|
Device sends DisconnectRequest during hello |
SerialException |
ConnectionError |
Device sends DisconnectRequest during the subscribe |
SerialException |
ConnectionError |
Owner calls api.disconnect() during the subscribe (external API) |
SerialException |
ConnectionError |
Device sends DisconnectRequest during flush() on an open transport |
SerialException |
ConnectionError |
| FIN or RST at any of those points | ConnectionError |
ConnectionError |
The exact-type check also covers the noise frame helper's close(), which fails a pending encrypted handshake with a bare APIConnectionError("Connection closed") (_frame_helper/noise.py:128-130). The other bare raises in this code path are "Already connected" (client.py:410, which _async_open can't hit on a client it creates) and "Incompatible API version" for API major > 2 (connection.py:711). The latter is permanent, so as a ConnectionError it would make HA retry with backoff forever. That only matters if ESPHome ever ships API 3.x. If you'd rather keep it as SerialException, you could map the bare class to ConnectionError only when the client is no longer connected. Longer term, aioesphomeapi could raise a dedicated subclass from _cleanup(), the same way esphome/aioesphomeapi#1960 did for _get_connection().
Smaller points (non-blocking)
- Socket errors during the encrypted handshake. aioesphomeapi wraps a raw
OSErroron the handshake future asHandshakeAPIError(connection.py:653-655). A reset right after the client hello is relabelled earlier, by the noise helper itself, asEncryptionHelloAPIErrorwith theConnectionResetErroras cause (noise.py:134-143). I reproduced that case, and it becomesSerialException. aioesphomeapi labels it on purpose as a likely encryption mismatch, so keeping it is defensible. If you want to cover both, map aHandshakeAPIErrorwhose__cause__is anOSErrortoConnectionError. A wrong key (InvalidEncryptionKeyAPIError, which never carries anOSErrorcause) would staySerialException. - Downstream wrappers. samsung-exlink (whose wrapper was the library-side fix for the HA issue), lg-rs232-tv, denon-rs232 and sony-tv-rs232 wrap
open_serial_connection()inexcept serialx.SerialExceptiononly. An offline ESPHome proxy now gets past those wrappers as a plainConnectionError. That matches how a missing local port already behaves (FileNotFoundError), and the HA integration in the report also catchesConnectionError/OSError, so it still retries. Still, it's worth a line in the release notes so those libraries can widen their wrappers to(serialx.SerialException, OSError). The pyserial compat layer is unaffected (maybe_wrap_exceptionscatchesException). - Nit:
test_open_disconnected_api_raises_connection_errorusespytest.raises(OSError, ...)followed byisinstance(..., ConnectionError), which could just bepytest.raises(ConnectionError, ...). Itsconnection_exists=Trueleg relies on the privateAPIClient._create_connection(), so it may need an update when aioesphomeapi changes internally.
Verification
- The classification matches
docs/how-to/pyserial-migration.md(I/O failures asOSError, backend/protocol failures asserialx.SerialException) and theOSError(EIO)thatconnection_closed_error()already uses mid-session. - The
exceptorder matches the aioesphomeapi 46.21.0 hierarchy.ResolveTimeoutAPIError(a subclass of bothResolveAPIErrorandasyncio.TimeoutError) is caught first and becomesTimeoutError.SocketClosedAPIErroris covered throughSocketAPIError. Auth, encryption-key, protocol and name/MAC mismatches staySerialException. - Live sockets: a closed port and an unresolvable host now raise
ConnectionError(causeSocketAPIError/ResolveAPIError); ondevthey raisedSerialException. uv.lockonly movesaioesphomeapi46.12.0 → 46.21.0.- Full suite on Python 3.14: 1112 passed, 216 skipped, 4 xfailed.
tests/test_serial_esphome.py: 72 passed, 19 skipped, matching the description. ruff and mypy are clean, and CI is green.
For later: overlap with pending fix branches (no action needed in this PR)
Two not-yet-opened branches on TheJulianJES/serialx (listed in this overview) touch the code around the raise … "ESPHome API connection closed while connecting" in ESPHomeSerialTransport._connect(). tjj/esphome-connect-timeout re-indents that raise under a new _connect_deadline() block, and its moved copy still raises SerialException, so it needs updating to ConnectionError when it is rebased onto this PR. tjj/esphome-unplug-during-open merges cleanly onto this PR and keeps the ConnectionError. Its direction matches: it reports an empty or removed port on open as OSError(ENXIO).
Map ESPHome transport failures to
ConnectionErrorso callers can retry after a proxy reboot. Preserve authentication/protocol errors asSerialException.Requires aioesphomeapi 46.21.0, which includes esphome/aioesphomeapi#1960. The new
ConnectionNotEstablishedAPIErrorremoves the need for an exact-type check.Original report: home-assistant/core#186950.
Validation: 72 tests passed against aioesphomeapi 46.21.0; 19 skipped without the ESPHome host binary. Repository checks passed.