From 4ac2bf105066060cd7e486b8759803be9b53f6ec Mon Sep 17 00:00:00 2001 From: Joshua King Date: Mon, 14 Sep 2026 08:03:55 -0400 Subject: [PATCH] fix: unambiguous get_by_child, deterministic gated-executor race test, exact-line probe exemption - get_by_child raises ValueError for unknown children: None means ONLY 'existing child, no schedule' (present every day), never 'nonexistent child silently present' - concurrent-upsert test uses a gated async executor that pauses task A's first transactional job after the connection lock is held, then schedules task B so it provably queues on the lock; deterministic, not timing-dependent - leak guard's probe exemption narrowed from whole-function spans to the exact string-literal lines holding the two sanctioned COUNT(*) queries --- custom_components/nestquest/dao_presence.py | 12 +++- tests/test_dao_presence.py | 79 +++++++++++++++++---- 2 files changed, 76 insertions(+), 15 deletions(-) diff --git a/custom_components/nestquest/dao_presence.py b/custom_components/nestquest/dao_presence.py index e90ccb9..b05fc7f 100644 --- a/custom_components/nestquest/dao_presence.py +++ b/custom_components/nestquest/dao_presence.py @@ -134,9 +134,17 @@ class PresenceSchedulesDao: ) -> PresenceScheduleRecord | None: """Return the child's schedule, or None when the child has none. - None is meaningful: a child with no schedule row is present - every day (Feature 05 guardrail). + None is meaningful and UNAMBIGUOUS here: the child exists and + has no schedule row, i.e. the child is present every day + (Feature 05 guardrail). An unknown child id raises ValueError + instead of returning None, because "nonexistent child" must + never be silently read as "present every day". """ + child = await self._database.fetch_one( + "SELECT 1 FROM children WHERE id = ?", (child_id,) + ) + if child is None: + raise ValueError(f"child {child_id} does not exist") row = await self._database.fetch_one( f"SELECT {_SCHEDULE_COLUMNS} FROM presence_schedules " "WHERE child_id = ?", diff --git a/tests/test_dao_presence.py b/tests/test_dao_presence.py index df32253..0c6e5d1 100644 --- a/tests/test_dao_presence.py +++ b/tests/test_dao_presence.py @@ -155,28 +155,63 @@ def test_upsert_two_children_have_distinct_rows(tmp_path) -> None: def test_upsert_concurrent_same_child_serializes(tmp_path) -> None: """Racing upserts for one child both succeed; exactly one row. - Without serialization the UNIQUE(child_id) would make the second - plain INSERT raise; the serialized ON CONFLICT upserts both land. + Deterministic interleaving: a controllable executor gates task A's + job AFTER it has acquired the connection lock and begun its + transaction, so task B is created while A provably owns the + transaction and must queue on the lock. Both upserts land, and + the table holds exactly one row (the ON CONFLICT upsert, not two + plain INSERTs, is what avoids a UNIQUE violation). """ async def _main(): - database = NestQuestDatabase(_make_hass_mock()) + from unittest.mock import MagicMock + + hass = MagicMock() + # Gate exactly ONE job: the first executor job issued after + # arming, which is task A's child-existence SELECT inside its + # transaction (A holds the connection lock from that moment). + # Everything before arming (open, migrations, child create) + # and everything after (B's jobs, A's remaining jobs, reads) + # pass straight through. + armed = {"active": False, "gated_once": False} + gate_open = asyncio.Event() + a_started = asyncio.Event() + + async def _gated_executor(fn, *args): + if armed["active"] and not armed["gated_once"]: + armed["gated_once"] = True + a_started.set() + await gate_open.wait() + return fn(*args) + + hass.async_add_executor_job = _gated_executor + database = NestQuestDatabase(hass) await database.open(tmp_path / "upsert-race.db") try: await apply_migrations(database) children = ChildrenDao(database) schedules = PresenceSchedulesDao(database) child = await children.create("Ada", NOW) + + armed["active"] = True task_a = asyncio.ensure_future( schedules.upsert_by_child( child.id, 2, "2026-09-07", "0,2|1,3" ) ) - await asyncio.sleep(0) + # Wait until task A's first transactional job is paused + # (A holds the connection lock, transaction open), then + # schedule task B so it must queue behind A. + await a_started.wait() + armed["active"] = False task_b = asyncio.ensure_future( schedules.upsert_by_child( child.id, 2, "2026-09-14", "1,3|0,2" ) ) + await asyncio.sleep(0) + # Release A; B's upsert must wait for A's commit and then + # land as the ON CONFLICT update path. + gate_open.set() record_a, record_b = await asyncio.gather(task_a, task_b) assert record_a.id == record_b.id fetched = await schedules.get_by_child(child.id) @@ -205,11 +240,17 @@ def test_get_by_child_missing_schedule_returns_none(tmp_path) -> None: assert _with_db(tmp_path, "get-none.db")(_body) is None -def test_get_by_child_unknown_child_returns_none(tmp_path) -> None: +def test_get_by_child_unknown_child_raises(tmp_path) -> None: + """None must mean ONLY 'no schedule row': an unknown child id is a + caller bug, and silently reading a nonexistent child as 'present + every day' would be wrong — so get raises instead. + """ async def _body(database, schedules, overrides, children, child): - return await schedules.get_by_child(999) + with pytest.raises(ValueError, match="does not exist"): + await schedules.get_by_child(999) + return None - assert _with_db(tmp_path, "get-unknown.db")(_body) is None + _with_db(tmp_path, "get-unknown.db")(_body) def test_delete_removes_and_returns_true_then_false(tmp_path) -> None: @@ -513,9 +554,11 @@ def test_presence_sql_lives_only_in_dao_module() -> None: def test_dao_presence_test_file_uses_dao_not_raw_table_sql() -> None: """This test module must exercise the DAO, not raw SQL, for these - tables — except the upsert row-count probe (COUNT(*) of the whole - table, needed to prove exactly-one-row) and the guard functions' - own spans. Scanned as ONE blob so multiline SQL cannot evade it. + tables. Exemptions are EXACT lines, not whole-function spans: the + two single-statement COUNT(*) row probes (the only honest way to + prove 'exactly one row exists' without a listing method) and the + guard functions' own regex spans. Any other raw SQL added anywhere + in this file fails the guard. """ import ast import re @@ -529,16 +572,26 @@ def test_dao_presence_test_file_uses_dao_not_raw_table_sql() -> None: "test_presence_sql_lives_only_in_dao_module", "test_dao_presence_test_file_uses_dao_not_raw_table_sql", } - probe_names = {"test_upsert_creates_then_updates_same_row", - "test_upsert_concurrent_same_child_serializes"} excluded: set[int] = set() for node in ast.walk(tree): if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): continue - if node.name in guard_names or node.name in probe_names: + if node.name in guard_names: excluded.update( range(node.lineno, (node.end_lineno or node.lineno) + 1) ) + continue + # Narrow probe exemption: only the string-literal line holding + # the sanctioned COUNT(*) query, not the enclosing function. + for sub in ast.walk(node): + if ( + isinstance(sub, ast.Constant) + and isinstance(sub.value, str) + and sub.value.strip() + == "SELECT COUNT(*) FROM presence_schedules" + and sub.lineno is not None + ): + excluded.add(sub.lineno) remaining = "".join( line