Merge pull request LF2b2w#51 from geofffranks/main

geofffranks · web-flow · commit caa2942c88a5 · 2026-05-26T09:50:30.000-04:00
fix: stop entry from getting stuck in failed_unload after OAuth refresh
diff --git a/custom_components/u_tec/__init__.py b/custom_components/u_tec/__init__.py
@@ -128,10 +128,18 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
         "coordinator": coordinator,
         "auth_data": auth_data,
         "webhook_handler": webhook_handler,
+        # Track previous push_enabled so async_update_options can detect a real
+        # change. entry.options reflects current state; without a stored prior
+        # we can't tell a toggle from a no-op data update (e.g. OAuth refresh).
+        "push_enabled": push_enabled,
     }
 
     await hass.config_entries.async_forward_entry_setups(entry, _PLATFORMS)
-    # Unload the entry if the user disables push notifications
+    # Listener fires on ANY entry update (data or options). The handler is
+    # responsible for filtering down to actual option changes — see
+    # async_update_options. Calling async_reload from this listener on every
+    # update would loop on OAuth token refreshes, which the integration cannot
+    # recover from without a working async_unload_entry.
     entry.async_on_unload(entry.add_update_listener(async_update_options))
     # Unregister the webhook when the entry is unloaded
     entry.async_on_unload(webhook_handler.unregister_webhook)
@@ -141,29 +149,37 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
     return True
 
 
+async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
+    """Unload a config entry."""
+    unload_ok = await hass.config_entries.async_unload_platforms(entry, _PLATFORMS)
+    if unload_ok:
+        hass.data.get(DOMAIN, {}).pop(entry.entry_id, None)
+    return unload_ok
+
+
 async def async_update_options(hass: HomeAssistant, entry: ConfigEntry) -> None:
-    """Handle options update."""
-    # Get the webhook handler and coordinator
-    webhook_handler = hass.data[DOMAIN][entry.entry_id]["webhook_handler"]
-    coordinator = hass.data[DOMAIN][entry.entry_id]["coordinator"]
-    auth_data = hass.data[DOMAIN][entry.entry_id]["auth_data"]
-
-    # Check if push notification setting has changed
-    old_push_enabled = entry.data.get("options", {}).get(CONF_PUSH_ENABLED, True)
+    """Handle options update.
+
+    Reconciles webhook registration and push_devices inline based on the new
+    options. Deliberately does NOT call async_reload — token refreshes update
+    entry.data and would otherwise trigger a reload on every refresh.
+    """
+    entry_data = hass.data[DOMAIN][entry.entry_id]
+    webhook_handler = entry_data["webhook_handler"]
+    coordinator = entry_data["coordinator"]
+    auth_data = entry_data["auth_data"]
+
+    old_push_enabled = entry_data.get("push_enabled", True)
     new_push_enabled = entry.options.get(CONF_PUSH_ENABLED, True)
 
-    # Update push devices in coordinator
     coordinator.push_devices = entry.options.get(CONF_PUSH_DEVICES, [])
 
-    # Handle webhook registration/unregistration if needed
     if old_push_enabled != new_push_enabled:
         if new_push_enabled:
-            # Register webhook
             await webhook_handler.async_register_webhook(auth_data)
         else:
-            # Unregister webhook
-            webhook_handler.unregister_webhook()
-    await hass.config_entries.async_reload(entry.entry_id)
+            await webhook_handler.unregister_webhook()
+        entry_data["push_enabled"] = new_push_enabled
 
 
 async def async_migrate_entry(hass: HomeAssistant, config_entry: ConfigEntry) -> bool:
diff --git a/custom_components/u_tec/api.py b/custom_components/u_tec/api.py
@@ -119,15 +119,20 @@ async def async_register_webhook(self, auth_data) -> bool:
             _LOGGER.error("Failed to register webhook with U-Tec API: %s", err)
             return False
 
-        # Register HA-side webhook handler (only once)
+        # Register HA-side webhook handler (only once). webhook.async_register
+        # returns None, so we use _unregister_webhook as a boolean flag — True
+        # once registered. Was previously assigned the return value, which left
+        # it None and broke this guard on every re-entry (notably the 24h
+        # re-register timer), raising "Handler is already defined!".
         if not self._unregister_webhook:
-            self._unregister_webhook = webhook.async_register(
+            webhook.async_register(
                 self.hass,
                 DOMAIN,
                 WEBHOOK_HANDLER,
                 self.webhook_id,
                 self._handle_webhook,
             )
+            self._unregister_webhook = True
 
         self.webhook_url = webhook_url
 
diff --git a/tests/test_setup.py b/tests/test_setup.py
@@ -6,7 +6,11 @@
 
 import pytest
 
-from custom_components.u_tec import async_setup_entry, async_update_options
+from custom_components.u_tec import (
+    async_setup_entry,
+    async_unload_entry,
+    async_update_options,
+)
 from custom_components.u_tec.const import CONF_PUSH_ENABLED, DOMAIN
 from tests.common import make_config_entry
 
@@ -119,6 +123,145 @@ async def _reload_side_effect(_entry_id):
         await async_update_options(hass, entry)
         await hass.async_block_till_done()
 
-    # Exactly two registrations: initial setup (push=False so not called) + reload (push=True).
-    # The initial setup is push=False, so only the reload should have registered.
+    # Initial setup is push=False (no register). Flipping to push=True via the
+    # options listener registers the webhook inline — no reload required.
     assert mock_register.await_count == 1
+
+
+async def test_async_update_options_toggles_webhook_off(hass, patched_uhomeapi):
+    """Disabling push while integration is running unregisters the webhook."""
+    entry = make_config_entry(options={CONF_PUSH_ENABLED: True})
+    entry.add_to_hass(hass)
+
+    with _patched_setup_env(hass), patch(
+        "custom_components.u_tec.api.AsyncPushUpdateHandler.async_register_webhook",
+        new=AsyncMock(return_value=True),
+    ), patch(
+        "custom_components.u_tec.api.AsyncPushUpdateHandler.unregister_webhook",
+        new=AsyncMock(return_value=None),
+    ) as mock_unregister, patch.object(
+        hass.config_entries, "async_reload", new=AsyncMock(),
+    ):
+        await async_setup_entry(hass, entry)
+
+        object.__setattr__(entry, "options", MappingProxyType({CONF_PUSH_ENABLED: False}))
+        await async_update_options(hass, entry)
+
+    mock_unregister.assert_awaited_once()
+
+
+async def test_async_update_options_re_enable_after_disable_registers(hass, patched_uhomeapi):
+    """Push True → False → True actually re-registers (regression for entry.data lookup bug)."""
+    entry = make_config_entry(options={CONF_PUSH_ENABLED: True})
+    entry.add_to_hass(hass)
+
+    with _patched_setup_env(hass), patch(
+        "custom_components.u_tec.api.AsyncPushUpdateHandler.async_register_webhook",
+        new=AsyncMock(return_value=True),
+    ) as mock_register, patch(
+        "custom_components.u_tec.api.AsyncPushUpdateHandler.unregister_webhook",
+        new=AsyncMock(return_value=None),
+    ), patch.object(
+        hass.config_entries, "async_reload", new=AsyncMock(),
+    ):
+        await async_setup_entry(hass, entry)
+        # setup with push=True → 1 register call
+        assert mock_register.await_count == 1
+
+        object.__setattr__(entry, "options", MappingProxyType({CONF_PUSH_ENABLED: False}))
+        await async_update_options(hass, entry)
+        # off — still 1 register call
+        assert mock_register.await_count == 1
+
+        object.__setattr__(entry, "options", MappingProxyType({CONF_PUSH_ENABLED: True}))
+        await async_update_options(hass, entry)
+        # back on — should re-register
+        assert mock_register.await_count == 2
+
+
+async def test_async_update_options_does_not_reload(hass, patched_uhomeapi):
+    """OAuth token refresh path: listener fires with options unchanged. Must not call async_reload."""
+    entry = make_config_entry(options={CONF_PUSH_ENABLED: False})
+    entry.add_to_hass(hass)
+
+    with _patched_setup_env(hass), patch.object(
+        hass.config_entries, "async_reload", new=AsyncMock(),
+    ) as mock_reload:
+        await async_setup_entry(hass, entry)
+        # Listener fires after async_update_entry — options identical to setup.
+        await async_update_options(hass, entry)
+
+    mock_reload.assert_not_called()
+
+
+async def test_unload_entry_returns_true_and_clears_hass_data(hass, patched_uhomeapi):
+    """async_unload_entry must exist and clean up hass.data[DOMAIN][entry_id]."""
+    entry = make_config_entry(options={CONF_PUSH_ENABLED: False})
+    entry.add_to_hass(hass)
+
+    with _patched_setup_env(hass), patch.object(
+        hass.config_entries,
+        "async_unload_platforms",
+        new=AsyncMock(return_value=True),
+    ) as mock_unload_platforms:
+        await async_setup_entry(hass, entry)
+        assert entry.entry_id in hass.data[DOMAIN]
+
+        result = await async_unload_entry(hass, entry)
+
+    assert result is True
+    mock_unload_platforms.assert_awaited_once()
+    assert entry.entry_id not in hass.data.get(DOMAIN, {})
+
+
+async def test_unload_entry_leaves_data_when_platform_unload_fails(
+    hass, patched_uhomeapi,
+):
+    """If platforms fail to unload, hass.data must stay intact for next attempt."""
+    entry = make_config_entry(options={CONF_PUSH_ENABLED: False})
+    entry.add_to_hass(hass)
+
+    with _patched_setup_env(hass), patch.object(
+        hass.config_entries,
+        "async_unload_platforms",
+        new=AsyncMock(return_value=False),
+    ):
+        await async_setup_entry(hass, entry)
+        result = await async_unload_entry(hass, entry)
+
+    assert result is False
+    assert entry.entry_id in hass.data[DOMAIN]
+
+
+async def test_oauth_token_refresh_does_not_unload_entry(hass, patched_uhomeapi):
+    """End-to-end: real async_update_entry → real update listener → no reload.
+
+    Exercises HA's full update-listener wiring rather than calling
+    async_update_options directly. This is the exact path an OAuth refresh
+    takes: OAuth2Session.async_ensure_token_valid → async_update_entry(data=...)
+    → fires entry.update_listeners → async_update_options. Before the fix, the
+    listener called async_reload, which silently transitioned the entry to
+    FAILED_UNLOAD because async_unload_entry was missing.
+    """
+    entry = make_config_entry(options={CONF_PUSH_ENABLED: False})
+    entry.add_to_hass(hass)
+
+    with _patched_setup_env(hass), patch.object(
+        hass.config_entries, "async_reload", new=AsyncMock(),
+    ) as mock_reload:
+        await async_setup_entry(hass, entry)
+        await hass.async_block_till_done()
+
+        # Simulate OAuth refresh writing a new token to entry.data — same call
+        # OAuth2Session.async_ensure_token_valid makes after a token refresh.
+        new_token = {**entry.data["token"], "access_token": "refreshed-token"}
+        hass.config_entries.async_update_entry(
+            entry, data={**entry.data, "token": new_token},
+        )
+        await hass.async_block_till_done()
+
+    mock_reload.assert_not_called()
+    # async_reload would have triggered async_unload_platforms; verify the
+    # entry's coordinator + webhook handler are still the same instances.
+    assert entry.entry_id in hass.data[DOMAIN]
+    assert hass.data[DOMAIN][entry.entry_id]["push_enabled"] is False
diff --git a/tests/test_webhook_registration.py b/tests/test_webhook_registration.py
@@ -1,8 +1,11 @@
 """Tests for AsyncPushUpdateHandler.async_register_webhook — URL resolution."""
 
+from datetime import timedelta
 from unittest.mock import MagicMock, patch
 
 from homeassistant.helpers.network import NoURLAvailableError
+from homeassistant.util import dt as dt_util
+from pytest_homeassistant_custom_component.common import async_fire_time_changed
 
 from custom_components.u_tec.api import AsyncPushUpdateHandler
 
@@ -141,3 +144,74 @@ async def test_unregister_noop_when_nothing_registered(hass, mock_uhome_api):
     h = AsyncPushUpdateHandler(hass, mock_uhome_api, entry_id="e1")
     # Neither _unregister_webhook nor _cancel_reregister is set — should not raise
     await h.unregister_webhook()
+
+
+async def test_register_twice_does_not_re_register_handler(hass, mock_uhome_api):
+    """webhook.async_register returns None — guard must still skip on second call.
+
+    Regression: storing the None return value as the "registered" flag broke the
+    guard, so the next call (e.g. the 24h re-register timer) re-entered the
+    block and webhook.async_register raised ValueError("Handler is already defined!").
+    """
+    h = AsyncPushUpdateHandler(hass, mock_uhome_api, entry_id="e1")
+
+    with patch(
+        "custom_components.u_tec.api.network.get_url",
+        return_value="https://ha.example.com",
+    ), patch(
+        "custom_components.u_tec.api.webhook.async_generate_url",
+        return_value="https://ha.example.com/api/webhook/x",
+    ), patch(
+        "custom_components.u_tec.api.webhook.async_register",
+        return_value=None,
+    ) as mock_register, patch(
+        "custom_components.u_tec.api.async_track_time_interval",
+        return_value=MagicMock(),
+    ):
+        await h.async_register_webhook(auth_data=MagicMock())
+        await h.async_register_webhook(auth_data=MagicMock())
+
+    assert mock_register.call_count == 1
+
+
+async def test_24h_reregister_timer_does_not_raise(hass, mock_uhome_api):
+    """Advance time by 24h and verify the re-register timer fires cleanly.
+
+    Before the fix, the scheduled _async_reregister callback would call
+    webhook.async_register again (because the guard's flag had been set to None),
+    raising ValueError("Handler is already defined!"). With the boolean-flag
+    fix, the guard correctly skips re-registration on the second cycle.
+    """
+    h = AsyncPushUpdateHandler(hass, mock_uhome_api, entry_id="e1")
+
+    with patch(
+        "custom_components.u_tec.api.network.get_url",
+        return_value="https://ha.example.com",
+    ), patch(
+        "custom_components.u_tec.api.webhook.async_generate_url",
+        return_value="https://ha.example.com/api/webhook/x",
+    ), patch(
+        "custom_components.u_tec.api.webhook.async_register",
+        return_value=None,
+    ) as mock_register, patch(
+        "custom_components.u_tec.api.webhook.async_unregister",
+    ):
+        # Real async_track_time_interval scheduler — no mock — so the 24h timer
+        # actually arms.
+        await h.async_register_webhook(auth_data=MagicMock())
+
+        # 24h + 1 minute later, the daily re-register callback fires.
+        async_fire_time_changed(hass, dt_util.utcnow() + timedelta(hours=24, minutes=1))
+        await hass.async_block_till_done()
+
+        # Exactly one HA-side registration despite the timer cycling. If the
+        # guard were broken, this would be 2 and the second call would raise.
+        assert mock_register.call_count == 1
+        # set_push_status fires for both the initial and the daily re-register —
+        # confirms the timer callback actually ran (the test would otherwise
+        # silently pass because the timer was never scheduled).
+        assert mock_uhome_api.set_push_status.await_count == 2
+
+        # Clean up the (now re-armed) timer so the lingering-timer guard in
+        # pytest-homeassistant-custom-component's teardown doesn't fail us.
+        await h.unregister_webhook()
