From 6e32761b0c6504e0dd2fc4dadefb4d1ae1a852fd Mon Sep 17 00:00:00 2001 From: Joshua Date: Thu, 17 Sep 2026 17:17:57 -0400 Subject: [PATCH] Guard against stale due_time overwrite (Feature 07 fix) (#87) --- custom_components/nestquest/dao_instances.py | 25 +++++---- tests/test_materialize.py | 57 +++++++++++++++++--- 2 files changed, 66 insertions(+), 16 deletions(-) diff --git a/custom_components/nestquest/dao_instances.py b/custom_components/nestquest/dao_instances.py index 94f05b9..e483d6c 100644 --- a/custom_components/nestquest/dao_instances.py +++ b/custom_components/nestquest/dao_instances.py @@ -344,14 +344,20 @@ class QuestInstancesDao: - the (definition, child) assignment link still exists (a removed assignment is skipped); - the window is still declared on the definition (a removed window - is skipped). + is skipped); + - the window's live ``due_time`` still EQUALS the caller-supplied + ``due_time`` (NULL-safe: NULL == NULL counts as equal; a window + with no due time matches a snapshot with no due time). A + window-time edit landed after the input snapshot makes the + caller's ``due_time`` stale, and writing it would overwrite the + newer due_time a concurrent regeneration already upserted — so + the tuple is skipped instead of written. - The live checks above ONLY decide whether the tuple is still - valid; they never read the window's ``due_time``. The instance is - written with the caller-supplied ``due_time`` — the value snapshotted - from the coherent input read — so a window-time edit landing - mid-walk cannot leak a newer time into a batch that was already - snapshotted against older rule/presence/assignment decisions. + The due_time check closes the one check-then-insert gap this + method previously left open: it re-validated every OTHER + precondition but then wrote the snapshot's ``due_time`` + unconditionally, so a stale materialization could clobber a + newer, concurrently-regenerated due_time. Returns the stored instance, or ``None`` when a precondition failed — the tuple no longer materializable, which the walk SKIPS @@ -384,8 +390,9 @@ class QuestInstancesDao: "JOIN quest_definition_windows w " "ON w.definition_id = d.id " "WHERE d.id = ? AND a.child_id = ? AND w.window = ? " - "AND d.is_active = 1 AND c.is_active = 1 LIMIT 1", - (definition_id, child_id, window), + "AND d.is_active = 1 AND c.is_active = 1 " + "AND w.due_time IS ? LIMIT 1", + (definition_id, child_id, window, due_time), ) if still_valid is None: # A config change invalidated the tuple since the diff --git a/tests/test_materialize.py b/tests/test_materialize.py index 2135559..3b1ea2f 100644 --- a/tests/test_materialize.py +++ b/tests/test_materialize.py @@ -545,7 +545,7 @@ def test_materialize_uses_single_presence_snapshot_mid_walk(tmp_path) -> None: _with_db(tmp_path, "materialize-presence-snapshot.db")(_body) -def test_materialize_stores_snapshot_due_time_not_live(tmp_path) -> None: +def test_materialize_skips_stale_due_time_mid_walk(tmp_path) -> None: async def _body(database): children = ChildrenDao(database) child = await children.create("Ada", NOW) @@ -566,21 +566,64 @@ def test_materialize_stores_snapshot_due_time_not_live(tmp_path) -> None: ) # The window's due_time is edited to 10:15 between the input - # snapshot and the insert. The walk must store the SNAPSHOT's - # 09:00, not the live 10:15 — the batch is governed entirely by - # the coherent snapshot it read. + # snapshot and the insert. The walk's snapshotted 09:00 no + # longer matches the live 10:15, so the tuple is SKIPPED rather + # than writing a due_time that has already been superseded. count = await _materialize_with_change( database, start_iso, end_iso, _edit_due_time ) - assert count == 3 + assert count == 0 + assert await QuestInstancesDao(database).list_by_date_range( + child.id, start_iso, end_iso + ) == [] + return count + + _with_db(tmp_path, "materialize-stale-due-time.db")(_body) + + +def test_materialize_does_not_overwrite_regenerated_due_time(tmp_path) -> None: + async def _body(database): + children = ChildrenDao(database) + child = await children.create("Ada", NOW) + + today = datetime.date.today() + start_iso = today.isoformat() + end_iso = (today + datetime.timedelta(days=2)).isoformat() + + created = await create_quest_definition( + database, + "Daily chore", + ScheduleRule(rule_type=RuleType.DAILY, start_date=start_iso), + [child.id], + [("morning", "09:00")], + ) + + async def _edit_and_regenerate(): + # A window-time edit + regeneration land after the stale walk + # read its input snapshot: the window moves to 10:15 and the + # regeneration upserts the newer due_time across the horizon. + await QuestDefinitionsDao(database).upsert_window( + created.definition.id, "morning", due_time="10:15" + ) + await regenerate_for_definition(database, created.definition.id) + + # The stale walk (snapshot due_time 09:00) then resumes. It must + # SKIP these tuples — its snapshotted 09:00 no longer matches the + # live 10:15 — so the regeneration's newer due_time survives and + # is NOT overwritten by the stale value. + count = await _materialize_with_change( + database, start_iso, end_iso, _edit_and_regenerate + ) + assert count == 0 + records = await QuestInstancesDao(database).list_by_date_range( child.id, start_iso, end_iso ) assert len(records) == 3 - assert {r.due_time for r in records} == {"09:00"} + assert {r.due_time for r in records} == {"10:15"} return records - _with_db(tmp_path, "materialize-snapshot-due-time.db")(_body) + _with_db(tmp_path, "materialize-regenerated-due-time.db")(_body) def test_materialize_honours_overrides(tmp_path) -> None: