From 58f5ddab7214e2ace46d2a4adb7505fe20a18006 Mon Sep 17 00:00:00 2001 From: Joshua Date: Thu, 17 Sep 2026 09:17:35 -0400 Subject: [PATCH] fix: return assignee roster within the lock (NQ-69-01) --- custom_components/nestquest/dao_rules.py | 9 ++- .../nestquest/quest_definitions.py | 8 +-- tests/test_quest_definitions.py | 57 +++++++++++++++++++ 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/custom_components/nestquest/dao_rules.py b/custom_components/nestquest/dao_rules.py index c15f585..d1338e4 100644 --- a/custom_components/nestquest/dao_rules.py +++ b/custom_components/nestquest/dao_rules.py @@ -962,14 +962,18 @@ class QuestDefinitionsDao: async def add_assignee( self, definition_id: int, child_id: int - ) -> None: + ) -> list[ChildRecord]: """Assign ``child_id`` to the definition (idempotent). Validates the assignee inside the same serialized transaction as the INSERT (existing and active AT ASSIGNMENT TIME), so a child deactivated concurrently cannot become an assignee. Assigning an already-assigned child is a no-op, mirroring the - composite primary key's storage-level guarantee. Changes + composite primary key's storage-level guarantee. Returns the + definition's assignees read back INSIDE the same transaction + that performed the write, so the returned roster is a + consistent post-assignment snapshot — a concurrent assign or + unassign cannot race it after the write commits. Changes future instances only: already-generated instances and completion history are never rewritten here. """ @@ -990,6 +994,7 @@ class QuestDefinitionsDao: "ON CONFLICT (definition_id, child_id) DO NOTHING", (definition_id, child_id), ) + return await self.list_assignees(definition_id) async def remove_assignee( self, definition_id: int, child_id: int diff --git a/custom_components/nestquest/quest_definitions.py b/custom_components/nestquest/quest_definitions.py index 901a4d8..b438a03 100644 --- a/custom_components/nestquest/quest_definitions.py +++ b/custom_components/nestquest/quest_definitions.py @@ -348,8 +348,9 @@ async def assign_child( active/exists check inside its transaction, and also confirms the definition exists. Assigning an already-assigned child is a no-op (no duplicate row). Returns the definition's assignees read back - through the DAO, a consistent post-assignment roster. Raises - ValueError on any rejected argument. Only the + inside the DAO's same transaction, a consistent post-assignment + roster (a concurrent assign/unassign cannot race it after the + write). Raises ValueError on any rejected argument. Only the ``quest_definition_assignees`` link is written; existing instances and completion history are never touched. """ @@ -357,7 +358,7 @@ async def assign_child( _validate_child_id(child_id) dao = QuestDefinitionsDao(database) try: - await dao.add_assignee(definition_id, child_id) + return await dao.add_assignee(definition_id, child_id) except ValueError as error: # The DAO names only the child/definition; re-raise so the # public contract names the field the caller actually passed. @@ -367,7 +368,6 @@ async def assign_child( if message.startswith("quest definition "): raise ValueError(f"definition_id: {message}") from error raise - return await dao.list_assignees(definition_id) async def unassign_child( diff --git a/tests/test_quest_definitions.py b/tests/test_quest_definitions.py index c3ba262..2206daa 100644 --- a/tests/test_quest_definitions.py +++ b/tests/test_quest_definitions.py @@ -1093,3 +1093,60 @@ def test_assign_and_unassign_leave_instances_and_history_alone( return None _with_db(tmp_path, "assign-unassign-history.db")(_body) + + +def test_concurrent_assign_unassign_returns_consistent_roster( + tmp_path, +) -> None: + """A concurrent unassign cannot strip the just-assigned child from + the roster that ``assign_child`` returns. + + ``assign_child`` returns the roster read inside the same locked + transaction as the INSERT, so the child it assigned is always present + in its result — even when an unassign of that same child races. The + pause hook holds ``list_assignees`` mid-flight so the unassign is + deterministically queued against the assignment's lock. + """ + async def _body(database, children, rules, definitions, child): + created = await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], ["morning"] + ) + bo = await children.create("Bo", NOW) + + import custom_components.nestquest.dao_rules as dao_rules + + original_list = dao_rules.QuestDefinitionsDao.list_assignees + started = asyncio.Event() + release = asyncio.Event() + + async def _pausing_list(self, definition_id): + if not started.is_set(): + started.set() + await release.wait() + return await original_list(self, definition_id) + + dao_rules.QuestDefinitionsDao.list_assignees = _pausing_list + try: + assign_task = asyncio.ensure_future( + assign_child(database, created.definition.id, bo.id) + ) + await started.wait() + unassign_task = asyncio.ensure_future( + unassign_child(database, created.definition.id, bo.id) + ) + await asyncio.sleep(0) + release.set() + assignees, removed = await asyncio.gather( + assign_task, unassign_task + ) + finally: + dao_rules.QuestDefinitionsDao.list_assignees = original_list + + assert bo.id in [c.id for c in assignees], ( + "assign_child must return a roster containing the child it " + "just assigned, even under a concurrent unassign" + ) + assert removed is True + return assignees + + _with_db(tmp_path, "assign-unassign-concurrent.db")(_body)