fix: cancel both listeners independently on unload (NQ-80-001)

custom_components/nestquest/__init__.py

@@ -2,19 +2,30 @@
 from __future__ import annotations
 
 import asyncio
+import datetime
 import inspect
 import sqlite3
 from collections.abc import Callable
 from dataclasses import dataclass
 from pathlib import Path
 from typing import Any
+from zoneinfo import ZoneInfo
 
 from homeassistant.config_entries import ConfigEntry
 from homeassistant.core import HomeAssistant
 from homeassistant.exceptions import ConfigEntryNotReady
+from homeassistant.helpers.event import async_track_time_change
 
-from .const import CONF_ADMIN_USER_IDS, DOMAIN, LOGGER
+from .const import (
+    CONF_ADMIN_USER_IDS,
+    CONF_DAY_ROLLOVER_TIME,
+    DEFAULT_DAY_ROLLOVER_TIME,
+    DEFAULT_HORIZON_DAYS,
+    DOMAIN,
+    LOGGER,
+)
 from .db import NestQuestDatabase
+from .materialize import materialize as _materialize_run
 from .migrations import apply_migrations
 from .store import async_get_db_path
 from .admin_allowlist import seed_setup_admin
@@ -224,6 +235,7 @@ class NestQuestRuntimeData:
     options: dict[str, Any]
     database: NestQuestDatabase
     remove_update_listener: Callable[[], Any]
+    remove_time_change_listener: Callable[[], Any]
 
 
 async def _async_owner_user_ids(hass: HomeAssistant) -> list[str]:
@@ -248,6 +260,48 @@ async def _async_owner_user_ids(hass: HomeAssistant) -> list[str]:
     return owners
 
 
+def _day_rollover_hour_minute(options: dict[str, Any]) -> tuple[int, int]:
+    """Return the (hour, minute) the daily rollover fires at from ``options``.
+
+    The options flow validates ``day_rollover_time`` as a strict ``HH:MM``
+    string, so a stored value never needs re-validation here; a missing key
+    falls back to :data:`~.const.DEFAULT_DAY_ROLLOVER_TIME`.
+    """
+    rollover = options.get(CONF_DAY_ROLLOVER_TIME, DEFAULT_DAY_ROLLOVER_TIME)
+    hour, _, minute = rollover.partition(":")
+    return int(hour), int(minute)
+
+
+def _register_day_rollover_listener(
+    hass: HomeAssistant,
+    database: NestQuestDatabase,
+    options: dict[str, Any],
+) -> Callable[[], Any]:
+    """Register the daily materialization listener and return its remover.
+
+    Fires once per day at the configured local ``day_rollover_time`` and
+    materializes the rolling horizon ``[today, today + DEFAULT_HORIZON_DAYS]``.
+    "today" is computed in HA local time (``hass.config.time_zone``) rather
+    than the system clock, so the horizon tracks the household's own day.
+    The walk itself runs through :func:`~.materialize.materialize`, whose DB
+    access is executor-only.
+    """
+    hour, minute = _day_rollover_hour_minute(options)
+
+    async def _run_materialization(_now: datetime.datetime) -> None:
+        time_zone = ZoneInfo(hass.config.time_zone)
+        today = datetime.datetime.now(time_zone).date()
+        start_date = today.isoformat()
+        end_date = (
+            today + datetime.timedelta(days=DEFAULT_HORIZON_DAYS)
+        ).isoformat()
+        await _materialize_run(database, start_date, end_date)
+
+    return async_track_time_change(
+        hass, _run_materialization, hour=hour, minute=minute
+    )
+
+
 async def async_setup(hass: HomeAssistant, config: dict[str, Any]) -> bool:
     """Set up the NestQuest integration. YAML configuration is not used, returns True."""
     return True
@@ -330,6 +384,9 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
             owner_ids=owner_ids,
         )
         remove_update_listener = entry.add_update_listener(_async_update_listener)
+        remove_time_change_listener = _register_day_rollover_listener(
+            hass, database, dict(entry.options)
+        )
     except BaseException:
         await database.close()
         raise
@@ -338,6 +395,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
         options=dict(entry.options),
         database=database,
         remove_update_listener=remove_update_listener,
+        remove_time_change_listener=remove_time_change_listener,
     )
     entry.runtime_data = runtime_data
     hass.data[DOMAIN][entry.entry_id] = runtime_data
@@ -354,14 +412,29 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
         runtime_data = getattr(entry, "runtime_data", None)
     if runtime_data is not None:
         remove_update_listener = getattr(runtime_data, "remove_update_listener", None)
+        remove_time_change_listener = getattr(
+            runtime_data, "remove_time_change_listener", None
+        )
         unload_error: BaseException | None = None
         try:
-            if remove_update_listener is not None:
+            for remove_listener in (
-                result = remove_update_listener()
+                remove_update_listener,
-                if inspect.isawaitable(result):
+                remove_time_change_listener,
-                    await result
+            ):
-        except BaseException as err:
+                if remove_listener is None:
-            unload_error = err
+                    continue
+                try:
+                    result = remove_listener()
+                    if inspect.isawaitable(result):
+                        await result
+                except BaseException as err:
+                    # Cancel EVERY listener even when an earlier remover
+                    # raises: a skipped remover would leave a daily
+                    # callback firing against a closed database after
+                    # unload.  The first error is retained and re-raised
+                    # only after both removers (and the DB close) ran.
+                    if unload_error is None:
+                        unload_error = err
         finally:
             database = getattr(runtime_data, "database", None)
             if database is not None:

tests/conftest.py

@@ -44,6 +44,7 @@ _HA_MODULES = (
     "homeassistant.exceptions",
     "homeassistant.helpers",
     "homeassistant.helpers.config_validation",
+    "homeassistant.helpers.event",
 )
 
 def _find_spec(name: str):
@@ -83,6 +84,7 @@ for _parent, _child in (
     ("homeassistant", "exceptions"),
     ("homeassistant", "helpers"),
     ("homeassistant.helpers", "config_validation"),
+    ("homeassistant.helpers", "event"),
 ):
     setattr(sys.modules[_parent], _child, sys.modules[f"{_parent}.{_child}"])
 
@@ -242,6 +244,69 @@ class ListenerRegistry:
         return sum(len(listeners) for listeners in self._listeners.values())
 
 
+class TimeChangeRegistry:
+    """Models HA's time-change listener bookkeeping.
+
+    ``track`` mirrors ``homeassistant.helpers.event.async_track_time_change``:
+    each call appends a record (action + hour/minute/second) and returns
+    a remove callable that reverses the registration.  Tests inspect
+    ``registrations`` and exercise the newest action through :meth:`fire`.
+    """
+
+    def __init__(self, hass=None):
+        self._hass = hass
+        self._registrations: list[dict] = []
+
+    def track(self, action, hour=None, minute=None, second=None):
+        record = {
+            "action": action,
+            "hour": hour,
+            "minute": minute,
+            "second": second,
+        }
+
+        def _remove():
+            self._registrations.remove(record)
+
+        record["remove"] = _remove
+        self._registrations.append(record)
+        return _remove
+
+    @property
+    def registrations(self):
+        """Return a copy of the live registrations."""
+        return list(self._registrations)
+
+    @property
+    def size(self) -> int:
+        """Number of live registrations."""
+        return len(self._registrations)
+
+    async def fire(self, now=None, index: int = -1):
+        """Run a registered action as HA's scheduler would, awaiting coroutines."""
+        result = self._registrations[index]["action"](now)
+        if inspect.isawaitable(result):
+            await result
+
+
+def _async_track_time_change(hass, action, hour=None, minute=None, second=None):
+    """Mirror homeassistant.helpers.event.async_track_time_change.
+
+    Exact HA 2024.6 signature — no ``local`` keyword: the local-time
+    wrapper is already this function.  Routes to the registry attached to
+    ``hass`` (created on demand), so tests reach the registrations via
+    ``hass.time_change``.
+    """
+    registry = getattr(hass, "time_change", None)
+    if registry is None:
+        registry = TimeChangeRegistry(hass)
+        hass.time_change = registry
+    return registry.track(action, hour=hour, minute=minute, second=second)
+
+
+_ha_mock("homeassistant.helpers.event").async_track_time_change = _async_track_time_change
+
+
 def make_config_entry(
     entry_id: str = "test_entry",
     options: dict | None = None,
@@ -303,6 +368,10 @@ def make_hass() -> tuple:
 
     hass.async_add_executor_job = _run_executor_job
     registry = ListenerRegistry(hass)
+    hass.time_change = TimeChangeRegistry(hass)
+    # A valid IANA time zone (the day-rollover listener reads it to
+    # compute "today" in HA local time).
+    hass.config.time_zone = "UTC"
 
     async def _async_reload(entry_id: str) -> None:
         registry.reloaded.append(entry_id)

tests/test_lifecycle.py

@@ -8,7 +8,11 @@ import pytest
 from conftest import make_config_entry, make_hass, wire_entry_to_registry
 
 from custom_components.nestquest import DOMAIN, async_setup_entry, async_unload_entry
-from custom_components.nestquest.const import DOMAIN as DOMAIN_CONST
+from custom_components.nestquest.const import (
+    CONF_DAY_ROLLOVER_TIME,
+    DEFAULT_HORIZON_DAYS,
+    DOMAIN as DOMAIN_CONST,
+)
 
 
 def _wire(entry, registry):
@@ -237,6 +241,128 @@ async def test_unload_closes_database_even_when_listener_removal_raises() -> Non
     assert database.connected is False, "database leaked despite remover error"
     assert entry.runtime_data is None
     assert DOMAIN not in hass.data
+    assert hass.time_change.size == 0, "time-change listener leaked"
+
+
+async def test_unload_cancels_time_listener_even_when_update_remover_raises() -> None:
+    """A raising update-listener remover must not skip the time-change cancel.
+
+    Both removers are independent: the update-listener remover raising must
+    still cancel the daily time-change callback, whose action would otherwise
+    fire after unload against a closed database.
+    """
+
+    class _RemoveBoom(Exception):
+        pass
+
+    def _remover():
+        raise _RemoveBoom()
+
+    entry = make_config_entry(entry_id="update-boom-time-cancel")
+    entry.add_update_listener = lambda listener: _remover
+    hass, _registry = make_hass()
+    await async_setup_entry(hass, entry)
+    assert hass.time_change.size == 1
+    with pytest.raises(_RemoveBoom):
+        await async_unload_entry(hass, entry)
+    assert hass.time_change.size == 0, "time-change listener not cancelled"
+    assert entry.runtime_data is None
+    assert DOMAIN not in hass.data
+
+
+async def test_setup_registers_day_rollover_listener(hass, make_entry) -> None:
+    """Setup registers a daily rollover listener and stores its cancel fn."""
+    entry = _wire(make_entry(), hass.registry)
+    assert await async_setup_entry(hass, entry) is True
+    assert hass.time_change.size == 1
+    registration = hass.time_change.registrations[0]
+    assert registration["hour"] == 0
+    assert registration["minute"] == 0
+    assert callable(entry.runtime_data.remove_time_change_listener)
+
+
+async def test_setup_registers_day_rollover_at_configured_time(hass, make_entry) -> None:
+    """The listener fires at the entry option's rollover time, not the default."""
+    entry = _wire(make_entry(options={CONF_DAY_ROLLOVER_TIME: "06:30"}), hass.registry)
+    await async_setup_entry(hass, entry)
+    registration = hass.time_change.registrations[0]
+    assert registration["hour"] == 6
+    assert registration["minute"] == 30
+
+
+async def test_unload_cancels_day_rollover_listener(hass, make_entry) -> None:
+    """Unload removes the time-change listener alongside the update listener."""
+    entry = _wire(make_entry(), hass.registry)
+    await async_setup_entry(hass, entry)
+    assert hass.time_change.size == 1
+    assert await async_unload_entry(hass, entry) is True
+    assert hass.time_change.size == 0
+
+
+async def test_rollover_time_change_reregisters_listener_at_new_time(
+    hass, make_entry
+) -> None:
+    """A rollover-time options change re-registers the listener at the new time."""
+    entry = _wire(
+        make_entry(options={CONF_DAY_ROLLOVER_TIME: "00:00"}), hass.registry
+    )
+    await async_setup_entry(hass, entry)
+    assert (hass.time_change.registrations[0]["hour"], hass.time_change.registrations[0]["minute"]) == (0, 0)
+
+    # The options flow writes the new rollover time, then HA reloads the
+    # entry: unload tears down the old listener, setup re-registers it.
+    entry.options = {CONF_DAY_ROLLOVER_TIME: "06:30"}
+    await async_unload_entry(hass, entry)
+    assert hass.time_change.size == 0
+
+    assert await async_setup_entry(hass, entry) is True
+    assert hass.time_change.size == 1
+    registration = hass.time_change.registrations[0]
+    assert (registration["hour"], registration["minute"]) == (6, 30)
+
+
+async def test_day_rollover_listener_materializes_ha_local_horizon(
+    hass, make_entry, monkeypatch
+) -> None:
+    """Firing the listener materializes today..today+horizon in HA local time."""
+    import datetime
+    from unittest.mock import AsyncMock
+    from zoneinfo import ZoneInfo
+
+    import custom_components.nestquest as nestquest
+
+    entry = _wire(make_entry(), hass.registry)
+    await async_setup_entry(hass, entry)
+    database = entry.runtime_data.database
+
+    fake = AsyncMock()
+    monkeypatch.setattr(nestquest, "_materialize_run", fake)
+
+    time_zone = ZoneInfo(hass.config.time_zone)
+    today = datetime.datetime.now(time_zone).date()
+    expected_start = today.isoformat()
+    expected_end = (today + datetime.timedelta(days=DEFAULT_HORIZON_DAYS)).isoformat()
+
+    await hass.time_change.fire()
+
+    fake.assert_awaited_once_with(database, expected_start, expected_end)
+
+
+def test_day_rollover_time_change_tracker_rejects_local_kwarg() -> None:
+    """The fake async_track_time_change mirrors HA 2024.6 (no ``local`` kwarg).
+
+    Setup drives the local-time wrapper directly, so passing ``local=`` must
+    raise TypeError — otherwise a regression in the production call would be
+    silently masked by the fake.
+    """
+    import inspect as _inspect
+
+    from homeassistant.helpers.event import async_track_time_change
+
+    params = _inspect.signature(async_track_time_change).parameters
+    assert list(params) == ["hass", "action", "hour", "minute", "second"]
+    with pytest.raises(TypeError):
+        async_track_time_change(None, lambda _now: None, hour=0, minute=0, local=True)
 
 
 async def test_setup_applies_all_v1_tables(hass, make_entry) -> None: