diff --git a/custom_components/ninlight/update.py b/custom_components/ninlight/update.py index 782db24..9219efc 100644 --- a/custom_components/ninlight/update.py +++ b/custom_components/ninlight/update.py @@ -74,6 +74,7 @@ class NinlightUpdateEntity(NinlightEntity, UpdateEntity): self._offer: FirmwareImage | None = None self._evaluated_for: tuple[str, str, str] | None = None self._eval_lock = asyncio.Lock() + self._install_lock = asyncio.Lock() self._attr_in_progress = False @property @@ -168,6 +169,16 @@ class NinlightUpdateEntity(NinlightEntity, UpdateEntity): ) async def async_install(self, version: str | None, backup: bool, **kwargs: Any) -> None: + # Only the call that owns this lock may touch in_progress/ota_in_progress below: without + # this check, a second concurrent call would sail through HA's own in-progress guard (it + # only rejects installed == latest) and its `finally` would clear the flags out from under + # the first call while that one is still flashing the lamp. + if self._install_lock.locked(): + raise _error("update_busy") + async with self._install_lock: + await self._async_install_locked() + + async def _async_install_locked(self) -> None: shared = self.hass.data[DATA_SHARED] image = self._offer if image is None: @@ -176,8 +187,12 @@ class NinlightUpdateEntity(NinlightEntity, UpdateEntity): if not await self.hass.async_add_executor_job(shared.catalog.verify, image): raise _error("image_invalid") - # Re-check right before sending: settings may have changed since the offer. + # Re-check right before sending: the lamp's firmware may have changed since the offer was + # computed (e.g. a stale offer surviving a failed evaluation - see _async_evaluate) and + # must never be flashed if it is no longer actually newer than what is running now. info = await self.coordinator.async_call(lambda c: c.get_info()) + if not _firmware_is_newer(image.version, str(info.get("fw_version"))): + raise _error("no_update") settings = await self.coordinator.async_call(lambda c: c.get_settings()) if info.get("device_type") != image.device_type: raise _error("image_invalid") diff --git a/tests/test_update.py b/tests/test_update.py index e952030..8f071f3 100644 --- a/tests/test_update.py +++ b/tests/test_update.py @@ -2,6 +2,8 @@ from __future__ import annotations +import asyncio +from copy import deepcopy from pathlib import Path from typing import Any from unittest.mock import MagicMock @@ -38,6 +40,7 @@ BASE = "http://192.168.1.50:8123" INSTALLED = "0.3.8-36-gca9bb" NEWER = "0.3.8-47-g71a57" NEWEST = "0.3.9-rc1" +EVEN_NEWER = "0.3.9-rc2" # newer than NEWEST @pytest.fixture @@ -328,6 +331,98 @@ async def test_install_rejects_changed_file( mock_client.fw_update.assert_not_awaited() +async def test_install_rejects_downgrade( + hass: HomeAssistant, + init_integration, + mock_client: MagicMock, + lamp_info: dict[str, Any], + firmware_dir: Path, +) -> None: + """The lamp may have been reflashed by another route since the offer was computed.""" + put(firmware_dir, "a.img", version=NEWER, content=CONTENT_WITH_SSID) + await rescan(hass) + entity_id = entity_id_for(hass, "update", "firmware") + assert hass.states.get(entity_id).attributes[ATTR_LATEST_VERSION] == NEWER + + # A newer image also appears on disk, and the lamp is reflashed by another route to a version + # newer than our stale offer - but the follow-up evaluation (triggered by the catalog scan) + # fails at get_settings and returns early, so the stale NEWER offer survives untouched. + put(firmware_dir, "b.img", version=EVEN_NEWER, content=CONTENT_WITH_SSID) + mock_client.get_settings.side_effect = NinlightConnectionError("down") + lamp_info["fw_version"] = NEWEST + await rescan(hass) + assert hass.states.get(entity_id).attributes[ATTR_LATEST_VERSION] == NEWER + + with pytest.raises(HomeAssistantError) as raised: + await install(hass) + assert raised.value.translation_key == "no_update" + mock_client.fw_update.assert_not_awaited() + + +async def test_install_rejects_concurrent_calls( + hass: HomeAssistant, + init_integration, + mock_client: MagicMock, + lamp_info: dict[str, Any], + firmware_dir: Path, +) -> None: + """A second call arriving in the pre-flag window (before in_progress is set) must be + rejected by our own lock rather than allowed to touch OTA state. + + HA's own service-level "already in progress" guard only helps once `_attr_in_progress` is + already True (it is a `cached_property` that HA invalidates whenever `_attr_in_progress` is + set, so it is never stale) - by the time that guard would fire, a second concurrent call is + already blocked by HA itself with translation_key "update_in_progress", not "update_busy". + The real gap I2 describes is *before* that: `verify`, `get_info` and `get_settings` are all + awaited before we ever set the flags, and HA's guard does not protect that window. This test + blocks the first `get_info()` re-check inside that window to reach it deterministically. + """ + put(firmware_dir, "a.img", version=NEWER, content=CONTENT_WITH_SSID) + await rescan(hass) + entity_id = entity_id_for(hass, "update", "firmware") + + entered = asyncio.Event() + release = asyncio.Event() + first_call = True + + async def blocking_get_info() -> dict[str, Any]: + nonlocal first_call + if first_call: + first_call = False + entered.set() + await release.wait() + return deepcopy(lamp_info) + + async def lamp_flashes(url: str) -> None: + lamp_info["fw_version"] = NEWER + lamp_info["uptime"] = 4 + + mock_client.get_info.side_effect = blocking_get_info + mock_client.fw_update.side_effect = lamp_flashes + + task1 = hass.async_create_task(install(hass), name="install #1") + async with asyncio.timeout(5): + await entered.wait() + + # Install #1 is blocked inside its pre-flag get_info() re-check: the lock is held, but + # in_progress/ota_in_progress are still untouched. + assert init_integration.runtime_data.ota_in_progress is False + assert hass.states.get(entity_id).attributes[ATTR_IN_PROGRESS] is False + + with pytest.raises(HomeAssistantError) as raised: + await install(hass) + assert raised.value.translation_key == "update_busy" + assert init_integration.runtime_data.ota_in_progress is False + assert hass.states.get(entity_id).attributes[ATTR_IN_PROGRESS] is False + + release.set() + async with asyncio.timeout(5): + await task1 + assert init_integration.runtime_data.ota_in_progress is False + assert hass.states.get(entity_id).attributes[ATTR_IN_PROGRESS] is False + assert hass.states.get(entity_id).attributes[ATTR_INSTALLED_VERSION] == NEWER + + async def test_removing_the_entry_clears_the_wifi_issue( hass: HomeAssistant, init_integration, firmware_dir: Path ) -> None: