From 7f7bd7346e831b2f42c0b6ae7246af4ba6c71b6e Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Sat, 3 Oct 2026 11:16:24 +0000 Subject: [PATCH] Retry when the device or cloud is briefly unavailable Since passing on the error messages of the device and the cloud, every error response raised a LaMetricError that was never retried. A brief 502, 503 or 504 from the cloud now failed right away, where it used to be retried. Home Assistant only treats connection errors as "cannot connect", so people got an unknown error instead. Those three statuses are connection errors again. Reads retry them, changes do not, as the device may already have acted on them. aiohttp's ConnectionTimeoutError now also counts as never sent, as it is only raised while the connection is still being made. --- src/demetriek/cloud.py | 10 +++++- src/demetriek/const.py | 9 ++++++ src/demetriek/device.py | 17 +++++++++-- tests/test_cloud.py | 25 +++++++++++++++ tests/test_lametric.py | 67 ++++++++++++++++++++++++++++++++++++----- 5 files changed, 117 insertions(+), 11 deletions(-) diff --git a/src/demetriek/cloud.py b/src/demetriek/cloud.py index 2f875dcb..28dfe9a4 100644 --- a/src/demetriek/cloud.py +++ b/src/demetriek/cloud.py @@ -20,6 +20,7 @@ ) from yarl import URL +from .const import TRANSIENT_HTTP_STATUSES from .exceptions import ( LaMetricAuthenticationError, LaMetricConnectionError, @@ -103,10 +104,17 @@ async def _request( body = await response.text() # The cloud did answer, so this is not a connection problem and - # retrying will not help. An expired token must surface as an + # retrying will not help, unless it says it is briefly unable to + # answer at all. An expired token must surface as an # authentication error, so the caller can ask for a new one. if response.status >= HTTPStatus.BAD_REQUEST: reason = error_message(body) or response.reason + if response.status in TRANSIENT_HTTP_STATUSES: + msg = ( + "The LaMetric cloud is temporarily unavailable" + f" ({response.status}): {reason}" + ) + raise LaMetricConnectionError(msg) if response.status in [HTTPStatus.UNAUTHORIZED, HTTPStatus.FORBIDDEN]: msg = f"Authentication to the LaMetric cloud failed: {reason}" raise LaMetricAuthenticationError(msg) diff --git a/src/demetriek/const.py b/src/demetriek/const.py index 92f745b6..51cd3635 100644 --- a/src/demetriek/const.py +++ b/src/demetriek/const.py @@ -1,6 +1,7 @@ """Asynchronous Python client for LaMetric TIME devices.""" from enum import IntEnum, StrEnum +from http import HTTPStatus # The model the device reports is a hardware identifier, not a product name. # LaMetric TIME reports "LM 37X8" on firmware 2.x and "sa8" on 3.x. @@ -10,6 +11,14 @@ "sa8": "TIME", } +# Statuses that say the server is briefly unable to answer, rather than that +# something is wrong with the request. Those are worth another try. +TRANSIENT_HTTP_STATUSES = { + HTTPStatus.BAD_GATEWAY, + HTTPStatus.SERVICE_UNAVAILABLE, + HTTPStatus.GATEWAY_TIMEOUT, +} + class BrightnessMode(StrEnum): """Enum holding the available brightness modes.""" diff --git a/src/demetriek/device.py b/src/demetriek/device.py index 4d48ba42..0b5962a4 100644 --- a/src/demetriek/device.py +++ b/src/demetriek/device.py @@ -22,7 +22,12 @@ ) from yarl import URL -from .const import ScreensaverMode, StreamFillType, StreamRenderMode +from .const import ( + TRANSIENT_HTTP_STATUSES, + ScreensaverMode, + StreamFillType, + StreamRenderMode, +) from .exceptions import ( LaMetricAuthenticationError, LaMetricConnectionError, @@ -76,7 +81,8 @@ def _can_retry(retry_state: RetryCallState) -> bool: return True return isinstance( - exception.__cause__, (aiohttp.ClientConnectorError, socket.gaierror) + exception.__cause__, + (aiohttp.ClientConnectorError, aiohttp.ConnectionTimeoutError, socket.gaierror), ) @@ -150,8 +156,15 @@ async def _request( # The device did answer, so this is not a connection problem and # retrying will not help. Pass on what the device says is wrong. + # Unless it says it is briefly unable to answer at all. if response.status >= HTTPStatus.BAD_REQUEST: reason = error_message(body) or response.reason + if response.status in TRANSIENT_HTTP_STATUSES: + msg = ( + f"The LaMetric device at {self.host} is temporarily" + f" unavailable ({response.status}): {reason}" + ) + raise LaMetricConnectionError(msg) if response.status in [HTTPStatus.UNAUTHORIZED, HTTPStatus.FORBIDDEN]: msg = ( f"Authentication to the LaMetric device at {self.host}" diff --git a/tests/test_cloud.py b/tests/test_cloud.py index 6daec32c..6e841676 100644 --- a/tests/test_cloud.py +++ b/tests/test_cloud.py @@ -127,6 +127,31 @@ async def test_http_error401( assert len(next(iter(responses.requests.values()))) == 1 +@pytest.mark.parametrize("status", [502, 503, 504]) +async def test_temporarily_unavailable_retried( + responses: aioresponses, cloud: LaMetricCloud, status: int +) -> None: + """Test a request is retried when the cloud is briefly unable to answer.""" + responses.get(f"{CLOUD_URL}/", status=status, body="") + responses.get(f"{CLOUD_URL}/", status=200, body='{"status": "ok"}') + + response = await cloud._request("/") + + assert response["status"] == "ok" + + +async def test_temporarily_unavailable_gives_up( + responses: aioresponses, cloud: LaMetricCloud +) -> None: + """Test a cloud that stays unavailable raises a connection error.""" + responses.get(f"{CLOUD_URL}/", status=503, body="", repeat=True) + + with pytest.raises(LaMetricConnectionError, match="temporarily unavailable"): + await cloud._request("/") + + assert len(next(iter(responses.requests.values()))) == 3 + + async def test_http_error_not_retried( responses: aioresponses, cloud: LaMetricCloud ) -> None: diff --git a/tests/test_lametric.py b/tests/test_lametric.py index a7d97bf2..aa0649db 100644 --- a/tests/test_lametric.py +++ b/tests/test_lametric.py @@ -95,6 +95,10 @@ async def test_timeout(responses: aioresponses, device: LaMetricDevice) -> None: assert len(next(iter(responses.requests.values()))) == 3 +CHANGE_METHODS = ["POST", "PUT", "DELETE"] + + +@pytest.mark.parametrize("method", CHANGE_METHODS) @pytest.mark.parametrize( "exception", [ @@ -103,40 +107,87 @@ async def test_timeout(responses: aioresponses, device: LaMetricDevice) -> None: ], ) async def test_change_not_retried_once_sent( - responses: aioresponses, device: LaMetricDevice, exception: Exception + responses: aioresponses, + device: LaMetricDevice, + exception: Exception, + method: str, ) -> None: """Test a change is not repeated when it may have reached the device. Repeating it could, for example, show the same notification twice. """ - responses.post(f"{DEVICE_URL}/", exception=exception, repeat=True) + responses.add(f"{DEVICE_URL}/", method, exception=exception, repeat=True) with pytest.raises(LaMetricConnectionError): - await device._request("/", method="POST") + await device._request("/", method=method) assert len(next(iter(responses.requests.values()))) == 1 +@pytest.mark.parametrize("method", CHANGE_METHODS) @pytest.mark.parametrize( "exception", [ aiohttp.ClientConnectorError(MagicMock(), OSError()), + aiohttp.ConnectionTimeoutError(), socket.gaierror(), ], ) async def test_change_retried_when_not_sent( - responses: aioresponses, device: LaMetricDevice, exception: Exception + responses: aioresponses, + device: LaMetricDevice, + exception: Exception, + method: str, ) -> None: """Test a change is retried when it never reached the device.""" - responses.post(f"{DEVICE_URL}/", exception=exception) - responses.post(f"{DEVICE_URL}/", exception=exception) - responses.post(f"{DEVICE_URL}/", status=200, body='{"status": "ok"}') + responses.add(f"{DEVICE_URL}/", method, exception=exception) + responses.add(f"{DEVICE_URL}/", method, exception=exception) + responses.add(f"{DEVICE_URL}/", method, status=200, body='{"status": "ok"}') + + response = await device._request("/", method=method) + + assert response["status"] == "ok" - response = await device._request("/", method="POST") + +@pytest.mark.parametrize("status", [502, 503, 504]) +async def test_temporarily_unavailable_read_retried( + responses: aioresponses, device: LaMetricDevice, status: int +) -> None: + """Test reading is retried when the device is briefly unable to answer.""" + responses.get(f"{DEVICE_URL}/", status=status, body="") + responses.get(f"{DEVICE_URL}/", status=200, body='{"status": "ok"}') + + response = await device._request("/") assert response["status"] == "ok" +@pytest.mark.parametrize("method", CHANGE_METHODS) +async def test_temporarily_unavailable_change_not_retried( + responses: aioresponses, device: LaMetricDevice, method: str +) -> None: + """Test a change is not repeated, the device may have acted on it.""" + responses.add(f"{DEVICE_URL}/", method, status=503, body="", repeat=True) + + with pytest.raises(LaMetricConnectionError, match="temporarily unavailable"): + await device._request("/", method=method) + + assert len(next(iter(responses.requests.values()))) == 1 + + +async def test_server_error_not_retried( + responses: aioresponses, device: LaMetricDevice +) -> None: + """Test an error that is not transient is raised right away.""" + responses.get(f"{DEVICE_URL}/", status=500, body="", repeat=True) + + with pytest.raises(LaMetricError) as error: + await device._request("/") + + assert not isinstance(error.value, LaMetricConnectionError) + assert len(next(iter(responses.requests.values()))) == 1 + + async def test_http_error404(responses: aioresponses, device: LaMetricDevice) -> None: """Test HTTP 404 response handling.""" responses.get(