Retry when the device or cloud is briefly unavailable

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)

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."""

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}"

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:

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(