fix: tolerate empty optional endpoints on factory-reset ISY-994 (#506)

Author: shbatm

pyisy/connection.py

@@ -218,7 +218,24 @@ async def request(
         ):
             _LOGGER.debug("ISY not ready or closed connection.")
         except aiohttp.ClientResponseError as err:
-            # Malformed framing/protocol error — retrying won't recover; bail.
+            # Malformed framing/protocol error — retrying won't recover.
+            # When the caller already opted into ``ok404=True`` we treat it
+            # as another flavor of "feature not present": ISY-994 firmware
+            # on a factory-reset / un-configured controller responds to
+            # missing optional resources (``/CONF/STATE.VAR``,
+            # ``/CONF/NET/RES.CFG``) with a real 404 whose framing trips
+            # aiohttp's parser when the connection is reused — the next
+            # request on the kept-alive socket reads the prior 404's HTML
+            # body where an HTTP status line should be. Demote that to a
+            # debug log and return ``""`` so the optional manager's
+            # "no resource configured" path handles it cleanly.
+            if ok404:
+                _LOGGER.debug(
+                    "ISY response framing error on optional endpoint %s: %s",
+                    url,
+                    err.message,
+                )
+                return ""
             _LOGGER.error(
                 "Client Response Error from ISY: %s %s.",
                 err.status,
@@ -289,13 +306,25 @@ async def get_status(self) -> str | None:
         return await self.request(req_url)
 
     async def get_variable_defs(self) -> list[str | BaseException] | None:
-        """Fetch the list of variables from the ISY."""
+        """Fetch the list of variables from the ISY.
+
+        ``ok404=True`` because both endpoints legitimately 404 on a
+        factory-reset / un-configured ISY-994 (``/CONF/INTEGER.VAR not
+        found`` / ``/CONF/STATE.VAR not found``); the ``Variables``
+        parser already handles those bodies and ``None`` as
+        "no variables defined" (see ``EMPTY_VARIABLE_RESPONSES``).
+        Without ``ok404`` the request path emits ERROR-level log spam
+        for what is really an empty-config success.
+        """
         req_list = [
             [URL_VARIABLES, URL_DEFINITIONS, VAR_INTEGER],
             [URL_VARIABLES, URL_DEFINITIONS, VAR_STATE],
         ]
         req_urls = [self.compile_url(req) for req in req_list]
-        return await asyncio.gather(*[self.request(req_url) for req_url in req_urls], return_exceptions=True)
+        return await asyncio.gather(
+            *[self.request(req_url, ok404=True) for req_url in req_urls],
+            return_exceptions=True,
+        )
 
     async def get_variables(self) -> str | None:
         """Fetch the variable details from the ISY to update local copy."""

pyisy/variables/__init__.py

@@ -104,8 +104,11 @@ def parse_definitions(self, xmls: list[str]) -> bool:
         valid_definitions = False
         for ind in range(2):
             # parse definitions
-            if xmls[ind] is None or xmls[ind] in EMPTY_VARIABLE_RESPONSES:
-                # No variables of this type defined.
+            if not xmls[ind] or xmls[ind] in EMPTY_VARIABLE_RESPONSES:
+                # No variables of this type defined. ``not xmls[ind]``
+                # catches both ``None`` (request returned nothing) and
+                # ``""`` (the ``ok404`` / framing-desync path in
+                # ``Connection.request``).
                 _LOGGER.info("No Type %s variables defined", ind + 1)
                 continue
             try:

tests/test_connection_extras.py

@@ -184,6 +184,19 @@ async def test_get_variable_defs_returns_two_responses(conn: Connection) -> None
     assert conn.request.await_count == 2
 
 
+async def test_get_variable_defs_passes_ok404(conn: Connection) -> None:
+    """Both per-type requests must be issued with ``ok404=True`` so a
+    factory-reset / un-configured ISY-994 doesn't surface its
+    ``/CONF/INTEGER.VAR not found`` 404 as ERROR-level log spam — the
+    Variables parser already treats that body as "no variables
+    defined"."""
+    conn.request = AsyncMock(return_value="")
+    await conn.get_variable_defs()
+    assert conn.request.await_count == 2
+    for call in conn.request.await_args_list:
+        assert call.kwargs.get("ok404") is True
+
+
 async def test_get_variables_concatenates_and_strips_inner_boundary(
     conn: Connection,
 ) -> None:
@@ -259,6 +272,32 @@ async def test_request_client_response_error_returns_none(conn: Connection) -> N
     assert result is None
 
 
+async def test_request_client_response_error_with_ok404_returns_empty(
+    conn: Connection,
+) -> None:
+    """Regression: ISY-994 firmware on a factory-reset controller answers
+    missing optional resources (``/CONF/STATE.VAR``,
+    ``/CONF/NET/RES.CFG``) with a real 404 whose framing desyncs the
+    keep-alive connection — aiohttp's parser then raises
+    ``ClientResponseError("Expected HTTP/, RTSP/ or ICE/:")`` on the
+    *next* request that reuses the socket. Callers that already opted
+    into ``ok404=True`` (variable defs, network resources) should see
+    that absorbed as ``""`` rather than an ERROR-level log + None."""
+    url = conn.compile_url(["vars", "definitions", "1"])
+    with aioresponses() as mocked:
+        mocked.get(
+            url,
+            exception=aiohttp.ClientResponseError(
+                request_info=None,
+                history=(),
+                status=400,
+                message="Expected HTTP/, RTSP/ or ICE/:",
+            ),
+        )
+        result = await conn.request(url, ok404=True)
+    assert result == ""
+
+
 async def test_request_non_rest_url_does_not_crash(conn: Connection) -> None:
     """Regression for #488: ``request()`` derives its debug-log endpoint
     from the URL by splitting on ``"rest"``. ``get_description()`` builds