diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index 8fe8ad6..8fd9707 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -294,7 +294,7 @@ async def _run_horizon_materialization( end_date = ( today + datetime.timedelta(days=DEFAULT_HORIZON_DAYS) ).isoformat() - await _materialize_run(database, start_date, end_date) + await _materialize_run(database, start_date, end_date, today=today) def _register_day_rollover_listener( @@ -340,7 +340,7 @@ def _register_regenerate_service( def _unregister() -> None: if hass.services.has_service(DOMAIN, SERVICE_REGENERATE): - hass.services.async_unregister(DOMAIN, SERVICE_REGENERATE) + hass.services.async_remove(DOMAIN, SERVICE_REGENERATE) return _unregister @@ -402,6 +402,9 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: await listener_hass.config_entries.async_reload(listener_entry.entry_id) database = await _async_open_database(hass, await async_get_db_path(hass)) + remove_update_listener: Callable[[], Any] | None = None + remove_time_change_listener: Callable[[], Any] | None = None + remove_service_listener: Callable[[], Any] | None = None try: # Seed the admin allowlist on first setup so the owner is never # locked out: an empty database takes the persisted admin copy @@ -437,8 +440,29 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: await _run_horizon_materialization(hass, database) remove_service_listener = _register_regenerate_service(hass, database) except BaseException: + # A failure anywhere after the database is open must not leak the + # listeners already registered against that (now-closed) database: + # unwind EVERY remover we acquired before the failure, keeping the + # original error as the one raised. Removal is best-effort — a + # failing remover must not mask the setup error that triggered it. + for remove_listener in ( + remove_service_listener, + remove_time_change_listener, + remove_update_listener, + ): + if remove_listener is None: + continue + try: + result = remove_listener() + if inspect.isawaitable(result): + await result + except BaseException: + pass await database.close() raise + assert remove_update_listener is not None + assert remove_time_change_listener is not None + assert remove_service_listener is not None runtime_data = NestQuestRuntimeData( entry_id=entry.entry_id, options=dict(entry.options), diff --git a/custom_components/nestquest/dao_instances.py b/custom_components/nestquest/dao_instances.py index c948aa8..94f05b9 100644 --- a/custom_components/nestquest/dao_instances.py +++ b/custom_components/nestquest/dao_instances.py @@ -59,6 +59,34 @@ def _today() -> datetime.date: return datetime.date.today() +def _resolve_today(today: datetime.date | None) -> datetime.date: + """Return the caller-resolved "today", or the host date when absent. + + The no-past guard and the delete cutoffs must compare due dates + against the SAME calendar date the materialization horizon was built + from. Home Assistant's configured time zone can lag the host clock + around midnight, so when the caller has already resolved the local + date (``today``) that anchor is authoritative; ``None`` falls back to + the host clock (:func:`_today`), which is exactly the previous + behavior for callers with no HA-local date. + """ + return _today() if today is None else today + + +def _reject_past(due_date: str, today_iso: str) -> None: + """Raise unless ``due_date`` is on or after ``today_iso``. + + The single no-past rule shared by :meth:`QuestInstancesDao.upsert` + and :meth:`QuestInstancesDao.upsert_if_valid`: instances are never + generated in the past, measured against the resolved "today". + """ + if due_date < today_iso: + raise ValueError( + f"instances are never generated in the past: due_date " + f"{due_date!r} is before today {today_iso!r}" + ) + + def _validate_utc_timestamp(value: str, field: str) -> None: """Raise ValueError unless ``value`` is a strict UTC ISO-8601 stamp. @@ -178,6 +206,7 @@ class QuestInstancesDao: *, window: str, due_time: str | None = None, + today: datetime.date | None = None, ) -> QuestInstanceRecord: """Create the dated instance for the widened key idempotently. @@ -196,6 +225,10 @@ class QuestInstancesDao: - ``due_date`` must be TODAY or later: instances are never generated in the past (Feature 07). The caller's materialization horizon is future-dated by definition. + ``today`` optionally pins the resolved HA-local date the + guard compares against (falling back to the host clock when + omitted) so a household time zone behind the host around + midnight is not misread as "past". - ``window`` must be one of ``const.QUEST_WINDOWS``. - An existing instance WITH a completion event is IMMUTABLE: the conflict path must not rewrite its snapshot columns @@ -215,24 +248,20 @@ class QuestInstancesDao: # check runs after the lock is acquired, because a caller can # wait on the lock across midnight — a date that was "today" # at call time may be "yesterday" by the time the INSERT runs. - today = _today().isoformat() - if due_date < today: - raise ValueError( - f"instances are never generated in the past: due_date " - f"{due_date!r} is before today {today!r}" - ) + # A caller-supplied ``today`` (HA-local) is used for BOTH + # checks and never re-read, since it is already the anchor the + # batch runs against. + today_date = _resolve_today(today) + _reject_past(due_date, today_date.isoformat()) async with _connection_lock(self._database): async with self._database.transaction(): # Re-read "today" under the lock: this is the date the # insert actually executes on, so the no-past rule - # holds across midnight rollovers. - today = _today().isoformat() - if due_date < today: - raise ValueError( - f"instances are never generated in the past: " - f"due_date {due_date!r} is before today " - f"{today!r}" - ) + # holds across midnight rollovers. A pinned HA-local + # anchor is exempt from the re-read (see above). + if today is None: + today_date = _today() + _reject_past(due_date, today_date.isoformat()) definition = await self._database.fetch_one( "SELECT 1 FROM quest_definitions WHERE id = ?", (definition_id,), @@ -298,6 +327,7 @@ class QuestInstancesDao: *, window: str, due_time: str | None = None, + today: datetime.date | None = None, ) -> QuestInstanceRecord | None: """Insert one dated instance iff its tuple is still materializable. @@ -327,7 +357,8 @@ class QuestInstancesDao: failed — the tuple no longer materializable, which the walk SKIPS rather than raising, so the rest of the batch is unaffected. The no-past rule (instances are never generated in the past) is - preserved from :meth:`upsert` and still raises ValueError. An + preserved from :meth:`upsert` and still raises ValueError; ``today`` + optionally pins the HA-local anchor it compares against. An instance that ALREADY has a completion event is also skipped (returns ``None``) rather than raised: it is immutable, so the walk leaves it untouched instead of failing the batch — the @@ -338,20 +369,13 @@ class QuestInstancesDao: _validate_window(window) # Fail fast on obviously-past dates before queuing on the lock; # the authoritative check re-runs under the lock (see upsert). - today = _today().isoformat() - if due_date < today: - raise ValueError( - f"instances are never generated in the past: due_date " - f"{due_date!r} is before today {today!r}" - ) + today_date = _resolve_today(today) + _reject_past(due_date, today_date.isoformat()) async with _connection_lock(self._database): async with self._database.transaction(): - today = _today().isoformat() - if due_date < today: - raise ValueError( - f"instances are never generated in the past: " - f"due_date {due_date!r} is before today {today!r}" - ) + if today is None: + today_date = _today() + _reject_past(due_date, today_date.isoformat()) still_valid = await self._database.fetch_one( "SELECT 1 FROM quest_definitions d " "JOIN quest_definition_assignees a " @@ -463,7 +487,7 @@ class QuestInstancesDao: return [_instance_from_row(row) for row in rows] async def delete_future_uncompleted( - self, definition_id: int, cutoff_date: str + self, definition_id: int, cutoff_date: str, *, today: datetime.date | None = None ) -> int: """Delete the definition's open instances at/after cutoff. @@ -475,9 +499,11 @@ class QuestInstancesDao: is measured against TODAY (materialization regenerates the rolling horizon from now), so a caller cannot use this to rewrite past open instances either — the day-rollover missed - sweep (Feature 11) handles past-due instances instead. Any - instance with a completion event is never touched, regardless - of its date. Returns the number deleted. + sweep (Feature 11) handles past-due instances instead. ``today`` + optionally pins the HA-local anchor the clamp compares against + (falling back to the host clock when omitted). Any instance with + a completion event is never touched, regardless of its date. + Returns the number deleted. """ _validate_date(cutoff_date, "cutoff_date") async with _connection_lock(self._database): @@ -487,8 +513,8 @@ class QuestInstancesDao: # midnight cannot delete instances that became past # while it waited (backdating is clamped to the # execution date, not the call date). - today = _today().isoformat() - effective_cutoff = max(cutoff_date, today) + today_date = _resolve_today(today) + effective_cutoff = max(cutoff_date, today_date.isoformat()) result = await self._database.execute( "DELETE FROM quest_instances WHERE definition_id = ? " "AND due_date >= ? AND NOT EXISTS (" @@ -500,7 +526,7 @@ class QuestInstancesDao: return result.rowcount async def delete_future_uncompleted_for_child( - self, child_id: int, cutoff_date: str + self, child_id: int, cutoff_date: str, *, today: datetime.date | None = None ) -> tuple[int, str]: """Delete the child's open instances at/after cutoff, across every definition, returning ``(deleted, effective_cutoff)``. @@ -511,8 +537,10 @@ class QuestInstancesDao: one transaction — an instance that gains a completion event mid-flight cannot be deleted. The cutoff cannot be backdated below today, for the same reason as the definition-scoped method (see its - docstring). Any instance with a completion event is never touched, - regardless of its date. + docstring). ``today`` optionally pins the HA-local anchor the clamp + compares against (falling back to the host clock when omitted). + Any instance with a completion event is never touched, regardless + of its date. Returns the number deleted and the effective cutoff that was actually applied (``max(cutoff_date, today)``, read under the @@ -530,8 +558,8 @@ class QuestInstancesDao: # definition-scoped method: the DELETE executes on this # date, so a caller queued across midnight cannot delete # instances that became past while it waited. - today = _today().isoformat() - effective_cutoff = max(cutoff_date, today) + today_date = _resolve_today(today) + effective_cutoff = max(cutoff_date, today_date.isoformat()) result = await self._database.execute( "DELETE FROM quest_instances WHERE child_id = ? " "AND due_date >= ? AND NOT EXISTS (" diff --git a/custom_components/nestquest/materialize.py b/custom_components/nestquest/materialize.py index 342e626..3f22e07 100644 --- a/custom_components/nestquest/materialize.py +++ b/custom_components/nestquest/materialize.py @@ -64,7 +64,7 @@ from __future__ import annotations import datetime from .const import DEFAULT_HORIZON_DAYS -from .dao_instances import QuestInstancesDao, _today +from .dao_instances import QuestInstancesDao, _resolve_today from .dao_rules import ( _validate_date, load_materialization_input, @@ -127,6 +127,7 @@ async def materialize( end_date: str, *, child_ids: list[int] | None = None, + today: datetime.date | None = None, ) -> int: """Generate quest instances for the closed range [start_date, end_date]. @@ -142,6 +143,14 @@ async def materialize( after a presence change) cannot touch any unrelated child's rows. When omitted (or None) the walk covers every assignee as before. + ``today`` optionally pins the resolved HA-local date the walk's + no-past guard compares against; it is threaded into every + :meth:`~.dao_instances.QuestInstancesDao.upsert_if_valid` write. When + omitted the host clock (``datetime.date.today``) is used, which is the + historical default. A caller that computed the horizon in Home + Assistant's configured time zone MUST pass that same date here so a + time zone behind the host around midnight is not rejected as "past". + All inputs are read in ONE locked transaction; each tuple's write then goes through the atomic :meth:`~.dao_instances.QuestInstancesDao.upsert_if_valid`, which re-validates the tuple and its current ``due_time`` in the same @@ -195,6 +204,7 @@ async def materialize( generated_at, window=window.window, due_time=window.due_time, + today=today, ) if written is not None: count += 1 @@ -205,6 +215,8 @@ async def materialize( async def regenerate_for_definition( database: NestQuestDatabase, definition_id: int, + *, + today: datetime.date | None = None, ) -> int: """Regenerate a definition's future instances after a config change. @@ -221,17 +233,24 @@ async def regenerate_for_definition( :func:`~.dao_instances._today` — so the delete cutoff and the re-materialization range share one anchor, and the walk's ``upsert_if_valid`` no-past guard never rejects the regenerated - range. Returns the number of instances the re-materialization + range. ``today`` optionally pins a caller-resolved HA-local date + (threaded into both the delete cutoff and the re-materialization) + so a household time zone behind the host around midnight stays + consistent. Returns the number of instances the re-materialization upserted across the whole snapshot (idempotent — other active definitions' tuples refresh in place, never duplicate). """ - today = _today() - start_date = today.isoformat() - end_date = (today + datetime.timedelta(days=DEFAULT_HORIZON_DAYS)).isoformat() + today_date = _resolve_today(today) + start_date = today_date.isoformat() + end_date = ( + today_date + datetime.timedelta(days=DEFAULT_HORIZON_DAYS) + ).isoformat() instances = QuestInstancesDao(database) - await instances.delete_future_uncompleted(definition_id, start_date) - return await materialize(database, start_date, end_date) + await instances.delete_future_uncompleted( + definition_id, start_date, today=today + ) + return await materialize(database, start_date, end_date, today=today) # HOOK (Feature 09 presence services): ``regenerate_for_child`` is the @@ -250,6 +269,8 @@ async def regenerate_for_definition( async def regenerate_for_child( database: NestQuestDatabase, child_id: int, + *, + today: datetime.date | None = None, ) -> int: """Regenerate a child's future instances after a presence change. @@ -277,13 +298,16 @@ async def regenerate_for_child( :meth:`~.dao_instances.QuestInstancesDao.delete_future_uncompleted_for_child`), so the delete cutoff and the re-materialization window share ONE execution-day anchor even when the call is queued across midnight. - Returns the number of instances the re-materialization upserted for - the child across the whole snapshot. + ``today`` optionally pins a caller-resolved HA-local date, threaded + into the delete cutoff and the re-materialization for the same + host-vs-HA time-zone consistency. Returns the number of instances + the re-materialization upserted for the child across the whole + snapshot. """ if isinstance(child_id, bool) or not isinstance(child_id, int): raise ValueError(f"child_id must be an integer, got {child_id!r}") - today = _today() + today_date = _resolve_today(today) instances = QuestInstancesDao(database) # Use the delete's returned effective cutoff as the materialize anchor, @@ -293,10 +317,10 @@ async def regenerate_for_child( # walk's no-past guard and leave the child's future instances deleted # but not rebuilt. One anchor for both keeps them consistent. _, effective_cutoff = await instances.delete_future_uncompleted_for_child( - child_id, today.isoformat() + child_id, today_date.isoformat(), today=today ) start = datetime.date.fromisoformat(effective_cutoff) end_date = (start + datetime.timedelta(days=DEFAULT_HORIZON_DAYS)).isoformat() return await materialize( - database, start.isoformat(), end_date, child_ids=[child_id] + database, start.isoformat(), end_date, child_ids=[child_id], today=today ) diff --git a/tests/conftest.py b/tests/conftest.py index 0486d74..b6603e8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -311,11 +311,13 @@ class ServiceRegistry: """Models HA's ``hass.services`` service registry. ``async_register`` stores a handler under its (domain, service) key, - ``has_service`` reports registration, and ``async_unregister`` removes - it. ``async_remove`` is the deprecated HA alias, kept so either - spelling resolves. ``call`` invokes a stored handler the way HA's - service bus would — awaiting a coroutine result — so tests can exercise - a registered service end-to-end. + ``has_service`` reports registration, and ``async_remove`` removes it + — exactly the surface Home Assistant's ``ServiceRegistry`` exposes + (there is deliberately NO ``async_unregister`` alias here, so a caller + that uses the wrong method name fails instead of being silently + papered over). ``call`` invokes a stored handler the way HA's service + bus would — awaiting a coroutine result — so tests can exercise a + registered service end-to-end. """ def __init__(self, hass=None): @@ -329,13 +331,10 @@ class ServiceRegistry: def has_service(self, domain, service): return (domain, service) in self._services - def async_unregister(self, domain, service): + def async_remove(self, domain, service): self._services.pop((domain, service), None) return None - def async_remove(self, domain, service): - return self.async_unregister(domain, service) - async def call(self, domain, service, data=None): func = self._services.get((domain, service)) if func is None: diff --git a/tests/test_lifecycle.py b/tests/test_lifecycle.py index e68fa40..6fb41bf 100644 --- a/tests/test_lifecycle.py +++ b/tests/test_lifecycle.py @@ -346,7 +346,9 @@ async def test_day_rollover_listener_materializes_ha_local_horizon( await hass.time_change.fire() - fake.assert_awaited_once_with(database, expected_start, expected_end) + fake.assert_awaited_once_with( + database, expected_start, expected_end, today=today + ) def test_day_rollover_time_change_tracker_rejects_local_kwarg() -> None: @@ -479,4 +481,31 @@ async def test_unload_deregisters_regenerate_service(hass, make_entry) -> None: assert hass.services.has_service(DOMAIN, SERVICE_REGENERATE) assert await async_unload_entry(hass, entry) is True - assert not hass.services.has_service(DOMAIN, SERVICE_REGENERATE) \ No newline at end of file + assert not hass.services.has_service(DOMAIN, SERVICE_REGENERATE) + + +async def test_setup_cleans_listeners_when_materialization_raises( + hass, make_entry, monkeypatch +) -> None: + """A startup-materialization failure rolls back every registered listener. + + The daily and update listeners are installed before the startup + materialization runs; when that walk raises, the database is closed but + the remover callables must also be invoked so no listener survives + firing against a closed database. + """ + import custom_components.nestquest as nestquest + + async def _boom(*_args, **_kwargs): + raise RuntimeError("materialization failed") + + monkeypatch.setattr(nestquest, "_materialize_run", _boom) + + entry = _wire(make_entry(), hass.registry) + with pytest.raises(RuntimeError, match="materialization failed"): + await async_setup_entry(hass, entry) + + assert hass.time_change.size == 0 + assert hass.registry.size == 0 + assert not hass.services.has_service(DOMAIN, SERVICE_REGENERATE) + assert not hasattr(entry, "runtime_data") \ No newline at end of file diff --git a/tests/test_materialize.py b/tests/test_materialize.py index 62523fd..c20f226 100644 --- a/tests/test_materialize.py +++ b/tests/test_materialize.py @@ -340,7 +340,7 @@ async def _materialize_with_change( seen = {"n": 0} async def _gated(self, definition_id, child_id, due_date, generated_at, - *, window, due_time=None): + *, window, due_time=None, today=None): seen["n"] += 1 if seen["n"] == 1: seen["first"] = (definition_id, child_id, window) @@ -348,7 +348,7 @@ async def _materialize_with_change( await release.wait() return await original_upsert( self, definition_id, child_id, due_date, generated_at, - window=window, due_time=due_time, + window=window, due_time=due_time, today=today, ) QuestInstancesDao.upsert_if_valid = _gated @@ -630,6 +630,42 @@ def test_materialize_rejects_malformed_or_inverted_bounds(tmp_path) -> None: _with_db(tmp_path, "materialize-bounds.db")(_body) +def test_materialize_accepts_ha_local_today_behind_host(tmp_path) -> None: + """The no-past guard compares against a caller-supplied HA-local today. + + A household time zone behind the host clock around midnight resolves a + HA-local "today" one calendar day earlier than the host's + ``date.today()``. The host-clock guard would reject that date as past, + so the resolved HA-local date must be threaded through ``materialize`` + and accepted instead of raising. + """ + async def _body(database): + children = ChildrenDao(database) + child = await children.create("Ada", NOW) + ha_today = datetime.date.today() - datetime.timedelta(days=1) + start_iso = ha_today.isoformat() + end_iso = (ha_today + datetime.timedelta(days=14)).isoformat() + await create_quest_definition( + database, + "Daily chore", + ScheduleRule(rule_type=RuleType.DAILY, start_date=start_iso), + [child.id], + ["morning"], + ) + + # Without the pinned HA-local today, the host-clock guard rejects + # the horizon start (ha_today is "yesterday" on the host). + with pytest.raises(ValueError, match="never generated in the past"): + await materialize(database, start_iso, end_iso) + + # With today=ha_today, the same range is accepted and materialized. + count = await materialize(database, start_iso, end_iso, today=ha_today) + assert count == 15 + return count + + _with_db(tmp_path, "materialize-ha-local-today.db")(_body) + + def test_materialize_skips_inactive_definitions(tmp_path) -> None: async def _body(database): from custom_components.nestquest.quest_definitions import ( @@ -914,10 +950,8 @@ def test_regenerate_for_child_uses_single_anchor_across_midnight( return day0 if clock["reads"] == 1 else day1 import custom_components.nestquest.dao_instances as dao_instances_module - import custom_components.nestquest.materialize as materialize_module monkeypatch.setattr(dao_instances_module, "_today", _fake_today) - monkeypatch.setattr(materialize_module, "_today", _fake_today) async def _body(database): children = ChildrenDao(database)