Guard against stale due_time overwrite (Feature 07 fix) (#87)

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

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: