Review fix: serialize create_child's duplicate scan with the insert
Review round 3 (Codex gpt-5.6-terra) found two concurrent creates of the same normalised name could both pass the duplicate scan before either INSERT committed, inserting a duplicate with no warning. The scan and INSERT now run inside the shared connection-scoped lock, so the second create always sees the first's committed row and warns. A gated concurrent-create test proves exactly one warning fires; a sequential-duplicate warning assertion rode along. Suite: 851 passed.
This commit is contained in:
@@ -116,23 +116,27 @@ async def create_child(
|
||||
|
||||
The display name is required, trimmed, and rejected when empty.
|
||||
A duplicate display name (case-insensitive) is allowed but logged
|
||||
as a warning. The duplicate scan and the INSERT are deliberately
|
||||
not one transaction: the warning is advisory, so serialising
|
||||
creates to protect it would buy nothing.
|
||||
as a warning. The scan and the INSERT run inside the
|
||||
connection-scoped lock shared with the DAOs, so a concurrent
|
||||
create of the same normalised name queues behind the first insert
|
||||
and sees it — the duplicate still warns; it can never slip through
|
||||
unlogged.
|
||||
"""
|
||||
name = _validate_text(display_name, "display_name", required=True)
|
||||
assert name is not None
|
||||
colour_value = _validate_text(colour, "colour")
|
||||
avatar_value = _validate_text(avatar_ref, "avatar_ref")
|
||||
order = _validate_sort_order(sort_order)
|
||||
await _warn_on_duplicate_name(database, name)
|
||||
return await ChildrenDao(database).create(
|
||||
name,
|
||||
_now_stamp(),
|
||||
colour=colour_value,
|
||||
avatar_ref=avatar_value,
|
||||
sort_order=order,
|
||||
)
|
||||
async with _connection_lock(database):
|
||||
await _warn_on_duplicate_name(database, name)
|
||||
created = await ChildrenDao(database).create(
|
||||
name,
|
||||
_now_stamp(),
|
||||
colour=colour_value,
|
||||
avatar_ref=avatar_value,
|
||||
sort_order=order,
|
||||
)
|
||||
return created
|
||||
|
||||
|
||||
async def edit_child(
|
||||
|
||||
@@ -320,6 +320,68 @@ def test_edit_child_rename_to_own_name_casing_alone_warns_not(
|
||||
_with_db(tmp_path, "edit-rename-self.db")(_body)
|
||||
|
||||
|
||||
def test_create_child_concurrent_same_name_still_warns(tmp_path) -> None:
|
||||
"""Racing creates of the same normalised name cannot slip through
|
||||
unlogged: the scan and INSERT are serialized on the connection
|
||||
lock, so the second create sees the first's committed row and
|
||||
warns. Deterministic gate on task A's INSERT while task B queues
|
||||
behind the lock. Without the lock, B's scan would run before A's
|
||||
INSERT commits and no warning would fire."""
|
||||
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 / "create-race.db")
|
||||
try:
|
||||
await apply_migrations(database)
|
||||
logger = logging.getLogger(
|
||||
"custom_components.nestquest.children"
|
||||
)
|
||||
seen: list[logging.LogRecord] = []
|
||||
|
||||
class _Capture(logging.Handler):
|
||||
def emit(self, record: logging.LogRecord) -> None:
|
||||
seen.append(record)
|
||||
|
||||
handler = _Capture()
|
||||
logger.addHandler(handler)
|
||||
try:
|
||||
armed["active"] = True
|
||||
task_a = asyncio.ensure_future(
|
||||
create_child(database, "Ada")
|
||||
)
|
||||
await a_started.wait()
|
||||
armed["active"] = False
|
||||
task_b = asyncio.ensure_future(
|
||||
create_child(database, " ada ")
|
||||
)
|
||||
await asyncio.sleep(0)
|
||||
gate_open.set()
|
||||
first, second = await asyncio.gather(task_a, task_b)
|
||||
finally:
|
||||
logger.removeHandler(handler)
|
||||
|
||||
# Both creates land; the duplicate is allowed.
|
||||
assert first.display_name == "Ada"
|
||||
assert second.display_name == "ada"
|
||||
names = [c.display_name for c in await list_children(database)]
|
||||
assert names == ["Ada", "ada"]
|
||||
# Exactly one warning: B's scan saw A's committed row.
|
||||
warnings = [
|
||||
r for r in seen if "duplicates existing child" in r.getMessage()
|
||||
]
|
||||
assert len(warnings) == 1, (
|
||||
"the queued duplicate create must warn exactly once"
|
||||
)
|
||||
assert "Ada" in warnings[0].getMessage()
|
||||
finally:
|
||||
await database.close()
|
||||
|
||||
_run(_main())
|
||||
|
||||
|
||||
def test_edit_child_case_only_edit_colliding_with_other_child_warns(
|
||||
tmp_path, caplog
|
||||
) -> None:
|
||||
|
||||
Reference in New Issue
Block a user