From 35e66bd9e65951392fe70514768baee47a7ef3cd Mon Sep 17 00:00:00 2001 From: orchestrator-agent Date: Sat, 19 Sep 2026 13:29:25 -0400 Subject: [PATCH] Carry completion timing under the lock; shut down before close The completed event's was_on_time travels on the CompletionResult (decided with the write) instead of re-reading latest state a racing reversal could replace; unload clears coordinator listeners before the database close. --- custom_components/nestquest/__init__.py | 13 +++++++---- custom_components/nestquest/completion.py | 23 +++++++++++++++---- custom_components/nestquest/events.py | 18 ++++++++------- custom_components/nestquest/services.py | 7 +++++- tests/test_entity_lifecycle.py | 28 +++++++++++++++++++++++ 5 files changed, 71 insertions(+), 18 deletions(-) diff --git a/custom_components/nestquest/__init__.py b/custom_components/nestquest/__init__.py index 33f1449..5c66d8c 100644 --- a/custom_components/nestquest/__init__.py +++ b/custom_components/nestquest/__init__.py @@ -615,16 +615,19 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: if unload_error is None: unload_error = err finally: - database = getattr(runtime_data, "database", None) - if database is not None: - await database.close() - # Tear the shared coordinator down after the DB close: no - # listener may outlive the connection it reads. + # The coordinator's entity listeners must stop BEFORE the + # database closes: a scheduled refresh starting during the + # close await would touch a cleared connection. (The + # platform unload above already removed the entities; this + # clears the coordinator's own remaining listeners.) coordinator = getattr(runtime_data, "coordinator", None) if coordinator is not None: shutdown = getattr(coordinator, "async_shutdown", None) if shutdown is not None: await shutdown() + database = getattr(runtime_data, "database", None) + if database is not None: + await database.close() # Domain-global services are torn down only once the FINAL entry # unloads (``hass.data[DOMAIN]`` is now empty), never when a sibling # entry is still loaded. diff --git a/custom_components/nestquest/completion.py b/custom_components/nestquest/completion.py index 7d163b5..bb6becb 100644 --- a/custom_components/nestquest/completion.py +++ b/custom_components/nestquest/completion.py @@ -374,14 +374,25 @@ class CompletionResult(str): appended an event (decided under the same lock as the append), so a caller — the Feature 09 service path firing bus events — announces a transition exactly once even under a concurrent - duplicate call where the loser's write is a no-op. + duplicate call where the loser's write is a no-op. A completion + also carries the ``was_on_time`` verdict computed under that same + lock, so the event payload never re-reads mutable latest state a + racing reversal could have replaced. """ appended: bool + was_on_time: bool | None - def __new__(cls, state: str, *, appended: bool) -> "CompletionResult": + def __new__( + cls, + state: str, + *, + appended: bool, + was_on_time: bool | None = None, + ) -> "CompletionResult": obj = super().__new__(cls, state) obj.appended = appended + obj.was_on_time = was_on_time return obj @@ -429,6 +440,10 @@ async def complete_instance( f"event_type must be '{EVENT_COMPLETED}' or " f"'{EVENT_UNCOMPLETED}', got {latest.event_type!r}" ) + # Computed under the completion lock and carried on the result: + # the bus event must report THIS completion's timing, never a + # re-read of the latest event a racing reversal could replace. + on_time = _was_on_time(instance, now, today) await append_event( database, instance.id, @@ -437,10 +452,10 @@ async def complete_instance( actor_source=actor_source, actor_user_id=actor_user_id, actor_child_id=actor_child_id, - was_on_time=_was_on_time(instance, now, today), + was_on_time=on_time, now=now, ) - return CompletionResult("done", appended=True) + return CompletionResult("done", appended=True, was_on_time=on_time) async def uncomplete_instance( diff --git a/custom_components/nestquest/events.py b/custom_components/nestquest/events.py index 9798340..129af0b 100644 --- a/custom_components/nestquest/events.py +++ b/custom_components/nestquest/events.py @@ -71,22 +71,24 @@ async def _instance_payload( async def fire_quest_completed( - hass, database: NestQuestDatabase, instance_id: int + hass, + database: NestQuestDatabase, + instance_id: int, + *, + was_on_time: bool | None, ) -> None: """Fire ``nestquest_quest_completed`` and, when the completion cleared the child's whole day, ``nestquest_child_day_complete``. Called by the complete_quest service handler AFTER an actual - completion transition; a no-op re-complete never reaches here. + completion transition, with the completion's ``was_on_time`` — + computed under the completion lock and carried on the result — so + the payload never re-reads mutable latest state a racing reversal + could have replaced. A no-op re-complete never reaches here. """ now_stamp = _now_stamp() payload = await _instance_payload(database, instance_id, now_stamp) - latest = await CompletionEventsDao(database).get_latest_for_instance( - instance_id - ) - payload["was_on_time"] = ( - latest.was_on_time if latest is not None else None - ) + payload["was_on_time"] = was_on_time hass.bus.async_fire(EVENT_QUEST_COMPLETED, payload) # Day-complete: quests were owed today and none remain (the same diff --git a/custom_components/nestquest/services.py b/custom_components/nestquest/services.py index f8c322b..0c66635 100644 --- a/custom_components/nestquest/services.py +++ b/custom_components/nestquest/services.py @@ -317,7 +317,12 @@ def _complete_quest( **_actor_kwargs(call), ) if result.appended: - await fire_quest_completed(hass, runtime.database, instance_id) + await fire_quest_completed( + hass, + runtime.database, + instance_id, + was_on_time=result.was_on_time, + ) await _refresh_entities(runtime) return _with_errors(_handler) diff --git a/tests/test_entity_lifecycle.py b/tests/test_entity_lifecycle.py index a85c265..48f9b84 100644 --- a/tests/test_entity_lifecycle.py +++ b/tests/test_entity_lifecycle.py @@ -157,3 +157,31 @@ async def test_restart_equivalent_restores_all_entity_states( assert hass.entities[ "nestquest_household_quests_completed_today" ].native_value == 1 + + +async def test_unload_shuts_coordinator_before_closing_database( + hass, make_entry +) -> None: + """The coordinator's listeners are cleared while the database is + still connected: a scheduled refresh must never race the close.""" + entry = wire_entry_to_registry(make_entry(), hass.registry) + assert await async_setup_entry(hass, entry) is True + coordinator = entry.runtime_data.coordinator + database = entry.runtime_data.database + seen_connected = [] + + async def _probe_shutdown(): + seen_connected.append(database.connected) + + original_close = database.close + + async def _recording_close(): + seen_connected.append("closing") + await original_close() + + coordinator.async_shutdown = _probe_shutdown + database.close = _recording_close + assert await async_unload_entry(hass, entry) is True + assert seen_connected == [True, "closing"], ( + "coordinator shutdown must precede the database close" + )