From 5578507ce898a28942e709a948247b151d43c6d0 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 14:13:48 -0400 Subject: [PATCH] Review fix: edit_child/set_child_active update+readback atomic Review round 2 (Codex gpt-5.6-terra) found both functions returned a readback that a concurrent writer could overwrite between the UPDATE and the SELECT, so the returned record could be the other caller's values. Both now run existence-check, duplicate scan, UPDATE and readback inside the DAOs' shared connection-scoped lock. Two gated concurrency tests prove the contract deterministically: task A paused at its UPDATE while task B queues, each returns its own values, final stored state is the last writer's. Suite: 850 passed. --- custom_components/nestquest/children.py | 86 +++++++++------- tests/test_children_registry.py | 125 ++++++++++++++++++++++++ 2 files changed, 176 insertions(+), 35 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index c0b6790..e3439aa 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -28,7 +28,7 @@ from __future__ import annotations import datetime import logging -from .dao_children import ChildRecord, ChildrenDao +from .dao_children import ChildRecord, ChildrenDao, _connection_lock from .db import NestQuestDatabase LOGGER = logging.getLogger(__name__) @@ -155,38 +155,49 @@ async def edit_child( being edited, so a case-only edit colliding with another child's normalised name still warns. Raises ValueError when the child does not exist. + + Atomicity: the existence check, the duplicate scan, the UPDATE and + the readback all run inside the connection-scoped lock shared with + the DAOs, so another edit cannot slip between this call's UPDATE + and its readback — the returned record is exactly the one this + edit produced, not a later writer's. """ - dao = ChildrenDao(database) - existing = await dao.get(child_id) - if existing is None: - raise ValueError(f"child {child_id} does not exist") - updates: dict[str, object] = {} - if display_name is not _UNSET: - name = _validate_text(display_name, "display_name", required=True) - assert name is not None - await _warn_on_duplicate_name(database, name, exclude_child_id=child_id) - updates["display_name"] = name - if colour is not _UNSET: - value = _validate_text(colour, "colour") - if value is None: - raise ValueError( - "clearing colour is not supported; pass a replacement " - "value or omit the field" + async with _connection_lock(database): + dao = ChildrenDao(database) + existing = await dao.get(child_id) + if existing is None: + raise ValueError(f"child {child_id} does not exist") + updates: dict[str, object] = {} + if display_name is not _UNSET: + name = _validate_text( + display_name, "display_name", required=True ) - updates["colour"] = value - if avatar_ref is not _UNSET: - value = _validate_text(avatar_ref, "avatar_ref") - if value is None: - raise ValueError( - "clearing avatar_ref is not supported; pass a " - "replacement value or omit the field" + assert name is not None + await _warn_on_duplicate_name( + database, name, exclude_child_id=child_id ) - updates["avatar_ref"] = value - if sort_order is not _UNSET: - updates["sort_order"] = _validate_sort_order(sort_order) - if updates: - await dao.update(child_id, **updates) - edited = await dao.get(child_id) + updates["display_name"] = name + if colour is not _UNSET: + value = _validate_text(colour, "colour") + if value is None: + raise ValueError( + "clearing colour is not supported; pass a replacement " + "value or omit the field" + ) + updates["colour"] = value + if avatar_ref is not _UNSET: + value = _validate_text(avatar_ref, "avatar_ref") + if value is None: + raise ValueError( + "clearing avatar_ref is not supported; pass a " + "replacement value or omit the field" + ) + updates["avatar_ref"] = value + if sort_order is not _UNSET: + updates["sort_order"] = _validate_sort_order(sort_order) + if updates: + await dao.update(child_id, **updates) + edited = await dao.get(child_id) assert edited is not None return edited @@ -199,12 +210,17 @@ async def set_child_active( Deactivation hides the child from the panel without deleting their history (feature guardrail); this is the only removal path. Raises ValueError when the child does not exist. + + Atomicity: existence check, UPDATE and readback run inside the + connection-scoped lock, so a concurrent opposite transition cannot + make this call report the other caller's value. """ - dao = ChildrenDao(database) - if await dao.get(child_id) is None: - raise ValueError(f"child {child_id} does not exist") - await dao.set_active(child_id, is_active) - updated = await dao.get(child_id) + async with _connection_lock(database): + dao = ChildrenDao(database) + if await dao.get(child_id) is None: + raise ValueError(f"child {child_id} does not exist") + await dao.set_active(child_id, is_active) + updated = await dao.get(child_id) assert updated is not None return updated diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index 3aa874d..e2af6cf 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -379,6 +379,131 @@ def test_set_child_active_missing_raises(tmp_path) -> None: _with_db(tmp_path, "set-active-missing.db")(_body) +# --------------------------------------------------------------------------- +# Concurrency: edit/set_active are atomic under the connection lock +# --------------------------------------------------------------------------- + + +def _gated_hass(armed: dict, gate_open: asyncio.Event, a_started: asyncio.Event): + """A hass mock whose executor gates the FIRST `_execute` job after + arming — the mutate statement of whichever business call starts + first — proving the caller holds the connection lock at that + moment and the second caller must queue behind it.""" + from unittest.mock import MagicMock + + hass = MagicMock() + + async def _gated_executor(fn, *args): + if armed["active"]: + if getattr(fn, "__name__", "") == "_execute" and not armed["gated"]: + armed["gated"] = True + a_started.set() + await gate_open.wait() + return fn(*args) + + hass.async_add_executor_job = _gated_executor + return hass + + +def test_edit_child_concurrent_opposite_edits_return_own_values( + tmp_path, +) -> None: + """Two racing edits cannot cross-contaminate each other's readback. + + Deterministic gate: task A is paused at its UPDATE (it provably + holds the connection lock), task B is then created and must queue; + A's readback runs before B's edit, so A returns A's value and B + returns B's. Without the lock, A's readback would see B's write. + """ + async def _main(): + armed = {"active": False, "gated": False} + gate_open = asyncio.Event() + a_started = asyncio.Event() + hass = _gated_hass(armed, gate_open, a_started) + database = NestQuestDatabase(hass) + await database.open(tmp_path / "edit-race.db") + try: + await apply_migrations(database) + child = await create_child( + database, "Ada", colour="#111111", sort_order=0 + ) + + armed["active"] = True + task_a = asyncio.ensure_future( + edit_child(database, child.id, colour="#AAAAAA", sort_order=1) + ) + await a_started.wait() + armed["active"] = False + task_b = asyncio.ensure_future( + edit_child( + database, child.id, colour="#BBBBBB", sort_order=2 + ) + ) + await asyncio.sleep(0) + gate_open.set() + record_a, record_b = await asyncio.gather(task_a, task_b) + + # Each caller gets back exactly what it wrote. + assert record_a.colour == "#AAAAAA" + assert record_a.sort_order == 1 + assert record_b.colour == "#BBBBBB" + assert record_b.sort_order == 2 + # B landed last: the stored row is B's. + row = await database.fetch_one( + "SELECT colour, sort_order FROM children WHERE id = ?", + (child.id,), + ) + assert row == ("#BBBBBB", 2) + finally: + await database.close() + + _run(_main()) + + +def test_set_child_active_concurrent_opposite_transitions_return_own( + tmp_path, +) -> None: + """Racing activate/deactivate calls each report their own request. + + Same deterministic gate on task A's UPDATE; task B queues behind + the connection lock and lands the opposite transition after A. + """ + async def _main(): + armed = {"active": False, "gated": False} + gate_open = asyncio.Event() + a_started = asyncio.Event() + hass = _gated_hass(armed, gate_open, a_started) + database = NestQuestDatabase(hass) + await database.open(tmp_path / "active-race.db") + try: + await apply_migrations(database) + child = await create_child(database, "Ada") + + armed["active"] = True + task_a = asyncio.ensure_future( + set_child_active(database, child.id, False) + ) + await a_started.wait() + armed["active"] = False + task_b = asyncio.ensure_future( + set_child_active(database, child.id, True) + ) + await asyncio.sleep(0) + gate_open.set() + record_a, record_b = await asyncio.gather(task_a, task_b) + + assert record_a.is_active is False + assert record_b.is_active is True + row = await database.fetch_one( + "SELECT is_active FROM children WHERE id = ?", (child.id,) + ) + assert row == (1,) + finally: + await database.close() + + _run(_main()) + + def test_list_children_orders_by_sort_order_then_id(tmp_path) -> None: async def _body(database): second = await create_child(database, "Bo", sort_order=1)