From 2a4eec658fa4669156543db99c667cf075181563 Mon Sep 17 00:00:00 2001 From: orchestrator-agent Date: Sat, 19 Sep 2026 12:03:42 -0400 Subject: [PATCH] Decide event transitions under the completion lock complete/uncomplete return a CompletionResult str subclass carrying appended (decided with the write under the lock), so duplicate concurrent calls fire one transition event; day-complete only evaluates for an instance due today. --- custom_components/nestquest/completion.py | 58 ++++++++++----- custom_components/nestquest/events.py | 4 ++ custom_components/nestquest/services.py | 19 ++--- tests/test_events.py | 88 +++++++++++++++++++++++ 4 files changed, 140 insertions(+), 29 deletions(-) diff --git a/custom_components/nestquest/completion.py b/custom_components/nestquest/completion.py index 034741d..7d163b5 100644 --- a/custom_components/nestquest/completion.py +++ b/custom_components/nestquest/completion.py @@ -366,6 +366,25 @@ async def instance_state( return _derived_state(instance, latest, today_date) +class CompletionResult(str): + """The derived instance state, carrying the append verdict. + + A ``str`` subclass so every existing ``result == "done"`` contract + keeps working; ``appended`` reports whether THIS call actually + 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. + """ + + appended: bool + + def __new__(cls, state: str, *, appended: bool) -> "CompletionResult": + obj = super().__new__(cls, state) + obj.appended = appended + return obj + + async def complete_instance( database: NestQuestDatabase, instance_id: int, @@ -375,14 +394,14 @@ async def complete_instance( actor_child_id: int | None = None, now: datetime.datetime | None = None, today: datetime.date | None = None, -) -> str: +) -> CompletionResult: """Append a completed event unless the instance is already done. - Returns the derived state ``done``. An already-done instance is a - no-op: nothing is appended. Unknown ``instance_id`` raises - ValueError naming the field. Existence, latest-event, and the - append run under one connection lock so a concurrent complete - cannot double-append. + Returns the derived state ``done``; ``appended`` reports whether + THIS call appended (False on an already-done no-op). Unknown + ``instance_id`` raises ValueError naming the field. Existence, + latest-event, and the append run under one connection lock so a + concurrent complete cannot double-append. ``was_on_time`` is computed from the HA-local completion moment (threaded ``now`` / ``today``) against the instance ``due_date`` @@ -404,7 +423,7 @@ async def complete_instance( instance_id ) if latest is not None and latest.event_type == EVENT_COMPLETED: - return "done" + return CompletionResult("done", appended=False) if latest is not None and latest.event_type != EVENT_UNCOMPLETED: raise ValueError( f"event_type must be '{EVENT_COMPLETED}' or " @@ -421,7 +440,7 @@ async def complete_instance( was_on_time=_was_on_time(instance, now, today), now=now, ) - return "done" + return CompletionResult("done", appended=True) async def uncomplete_instance( @@ -433,16 +452,16 @@ async def uncomplete_instance( actor_child_id: int | None = None, now: datetime.datetime | None = None, today: datetime.date | None = None, -) -> str: +) -> CompletionResult: """Append an uncompleted event unless the instance is already open. Returns the derived state ``open``, or ``missed`` when the instance - is past due (``due_date`` before HA-local ``today``). An - already-open instance is a no-op: nothing is appended. Unknown - ``instance_id`` raises ValueError naming the field. Existence, - latest-event, and the append run under one connection lock so a - concurrent uncomplete cannot double-append. The original completed - row is not modified. + is past due (``due_date`` before HA-local ``today``); ``appended`` + reports whether THIS call appended (False on an already-open + no-op). Unknown ``instance_id`` raises ValueError naming the + field. Existence, latest-event, and the append run under one + connection lock so a concurrent uncomplete cannot double-append. + The original completed row is not modified. """ instance_id = _validate_int_id(instance_id, "instance_id") actor_source, actor_user_id, actor_child_id = _validate_actor( @@ -459,7 +478,10 @@ async def uncomplete_instance( instance_id ) if latest is None or latest.event_type == EVENT_UNCOMPLETED: - return _derived_state(instance, latest, today_date) + return CompletionResult( + _derived_state(instance, latest, today_date), + appended=False, + ) if latest.event_type != EVENT_COMPLETED: raise ValueError( f"event_type must be '{EVENT_COMPLETED}' or " @@ -476,7 +498,9 @@ async def uncomplete_instance( was_on_time=None, now=now, ) - return _derived_state(instance, latest, today_date) + return CompletionResult( + _derived_state(instance, latest, today_date), appended=True + ) async def list_missed_for_child( diff --git a/custom_components/nestquest/events.py b/custom_components/nestquest/events.py index d8394c6..9798340 100644 --- a/custom_components/nestquest/events.py +++ b/custom_components/nestquest/events.py @@ -91,7 +91,11 @@ async def fire_quest_completed( # Day-complete: quests were owed today and none remain (the same # rule as the all-done binary sensor; a zero-quest day never fires). + # Only a completion of TODAY'S instance evaluates it: completing a + # back-dated or future instance must not announce a cleared day. today = datetime.datetime.now(ZoneInfo(hass.config.time_zone)).date() + if payload["due_date"] != today.isoformat(): + return instances = await QuestInstancesDao(database).list_by_date_range( payload["child_id"], today.isoformat(), today.isoformat() ) diff --git a/custom_components/nestquest/services.py b/custom_components/nestquest/services.py index 3ee9903..c03150d 100644 --- a/custom_components/nestquest/services.py +++ b/custom_components/nestquest/services.py @@ -295,17 +295,15 @@ def _complete_quest( async def _handler(call: Any) -> None: database = _require_runtime(hass, find_runtime).database instance_id = call.data["instance_id"] - # Events fire on an ACTUAL transition only: an already-done - # instance is a no-op below and must not re-announce itself. - state_before = await completion_layer.instance_state( - database, instance_id - ) - await completion_layer.complete_instance( + # ``appended`` is decided under the same lock as the write, so + # a concurrent duplicate call (a double tap) can not produce a + # second transition event: the loser appends nothing. + result = await completion_layer.complete_instance( database, instance_id, **_actor_kwargs(call), ) - if state_before != "done": + if result.appended: await fire_quest_completed(hass, database, instance_id) return _with_errors(_handler) @@ -317,15 +315,12 @@ def _uncomplete_quest( async def _handler(call: Any) -> None: database = _require_runtime(hass, find_runtime).database instance_id = call.data["instance_id"] - state_before = await completion_layer.instance_state( - database, instance_id - ) - await completion_layer.uncomplete_instance( + result = await completion_layer.uncomplete_instance( database, instance_id, **_actor_kwargs(call), ) - if state_before == "done": + if result.appended: await fire_quest_uncompleted(hass, database, instance_id) return _with_errors(_handler) diff --git a/tests/test_events.py b/tests/test_events.py index ee3d79b..ad019be 100644 --- a/tests/test_events.py +++ b/tests/test_events.py @@ -190,3 +190,91 @@ async def test_uncompleted_fires_on_reversal_only(hass, make_entry) -> None: def test_missed_event_name_is_documented_contract() -> None: """Feature 10 defines the missed event Feature 11's sweep fires.""" assert EVENT_QUEST_MISSED == "nestquest_quest_missed" + + +async def test_appended_verdict_is_atomic_under_duplicate_calls( + hass, make_entry +) -> None: + """Concurrent duplicate completes decide appended under the same + lock as the write: exactly one fires the transition event.""" + import asyncio + + entry, child, instances = await _setup_and_seed(hass, make_entry) + instance_id = instances[0].id + calls = [ + hass.services.call( + DOMAIN, + SERVICE_COMPLETE_QUEST, + { + "instance_id": instance_id, + "actor": "panel", + "actor_child_id": child.id, + }, + ) + for _ in range(2) + ] + await asyncio.gather(*calls) + assert len(hass.bus.fired(EVENT_QUEST_COMPLETED)) == 1 + + +async def test_day_complete_ignored_for_backdated_completion( + hass, make_entry +) -> None: + """Completing an instance NOT due today never evaluates the + child's day-complete, so a cleared day is announced at most once + per day and only by that day's completions.""" + import datetime + + from custom_components.nestquest.completion import ( + complete_instance as _complete, + ) + + entry, child, instances = await _setup_and_seed(hass, make_entry) + database = entry.runtime_data.database + # A future instance (tomorrow) completed after today's day is + # already clear. + today = datetime.date.today() + end = (today + datetime.timedelta(days=3)).isoformat() + all_instances = await QuestInstancesDao(database).list_by_date_range( + child.id, today.isoformat(), end + ) + today_ids = {instance.id for instance in instances} + tomorrow_instance = next( + instance + for instance in all_instances + if instance.id not in today_ids + ) + for instance in instances: + await hass.services.call( + DOMAIN, + SERVICE_COMPLETE_QUEST, + { + "instance_id": instance.id, + "actor": "panel", + "actor_child_id": child.id, + }, + ) + assert len(hass.bus.fired(EVENT_CHILD_DAY_COMPLETE)) == 1 + + await _complete( + database, + tomorrow_instance.id, + actor_source="user", + actor_user_id="admin-1", + ) + await hass.services.call( + DOMAIN, + SERVICE_COMPLETE_QUEST, + { + "instance_id": tomorrow_instance.id, + "actor": "panel", + "actor_child_id": child.id, + }, + ) + assert len(hass.bus.fired(EVENT_CHILD_DAY_COMPLETE)) == 1, ( + "a back-dated/future completion must not re-announce today" + ) + # The direct business-layer call fired nothing (only the service + # path fires), and the follow-up service call on the now-done + # future instance was a no-op: today's single completion stands. + assert len(hass.bus.fired(EVENT_QUEST_COMPLETED)) == len(instances)