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.
This commit is contained in:
Joshua King
@@ -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
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user