From ca16d1fe5940af01e9c8b986f4b633c90746d8b9 Mon Sep 17 00:00:00 2001 From: Matysh Date: Fri, 28 Aug 2026 13:49:57 +0300 Subject: [PATCH] Fix vacuum trail lifecycle persistence Issue: #335 User-Visible: yes --- custom_components/houseplan/__init__.py | 7 +- custom_components/houseplan/trails.py | 82 ++++++++++--- custom_components/houseplan/websocket_api.py | 39 +++--- docs/CHANGELOG.md | 6 + docs/CHANGELOG.ru.md | 6 + docs/TESTING.md | 5 + docs/VACUUM.md | 6 +- tests_backend/test_ha_websocket.py | 68 +++++++++++ tests_backend/test_trail_recorder.py | 121 +++++++++++++++++++ 9 files changed, 295 insertions(+), 45 deletions(-) diff --git a/custom_components/houseplan/__init__.py b/custom_components/houseplan/__init__.py index 874a1e92..9ee4ba7d 100755 --- a/custom_components/houseplan/__init__.py +++ b/custom_components/houseplan/__init__.py @@ -239,12 +239,9 @@ async def async_setup_entry(hass: HomeAssistant, entry: HouseplanConfigEntry) -> hass.bus.async_fire("houseplan_config_updated", {"rev": optimize_revs[0]}) hass.bus.async_fire("houseplan_layout_updated", {"rev": optimize_revs[1]}) if recovered_import: - await recorder.async_refresh() current = (await data.config_store.async_load() or {}).get("config") or {} - live_ids = {str(marker.get("id")) for marker in current.get("markers") or []} - for marker_id in list(recorder.book.data): - if marker_id not in live_ids: - await recorder.async_delete(marker_id) + await recorder.async_purge_orphans(current) + await recorder.async_refresh() await async_check_plan_files(hass, entry) diff --git a/custom_components/houseplan/trails.py b/custom_components/houseplan/trails.py index ca17cd6f..fc29bf91 100755 --- a/custom_components/houseplan/trails.py +++ b/custom_components/houseplan/trails.py @@ -187,8 +187,11 @@ class TrailRecorder: # just finished calibrating) must start recording NOW, not at the # next state change — otherwise the first seconds of the path are # lost. + now = time.time() + changed = False for src in self.pairs: - self._sample(src, time.time()) + changed |= self._sample(src, now) + self._handle_sample_change(changed, now) def _source_failure_reason(self, source: str) -> str | None: """Classify only refresh-time health evidence. @@ -244,25 +247,70 @@ class TrailRecorder: async def async_delete(self, marker: str) -> bool: """Stop and erase one marker without racing subscription refresh/save.""" + return bool(await self._async_delete_many({marker})) + + async def async_purge_orphans(self, config: dict[str, Any]) -> int: + """Erase trails whose marker is absent or a removal tombstone. + + A tombstone deliberately stays in config so discovery cannot resurrect + a deleted device. For live tracking and trail ownership it is absent: + this is the same boundary used by ``async_refresh`` above. + """ + live_marker_ids = { + str(marker.get("id")) + for marker in config.get("markers") or [] + if marker.get("id") is not None and marker.get("removed") is not True + } + orphan_ids = set(self.book.data) - live_marker_ids + if not orphan_ids: + return 0 + try: + return await self._async_delete_many(orphan_ids) + except Exception: # noqa: BLE001 — config commit already succeeded + _LOGGER.exception( + "House Plan: removing orphan vacuum trails failed: markers=%s", + sorted(orphan_ids), + ) + return 0 + + async def _async_delete_many(self, markers: set[str]) -> int: + """Delete one or more books with one subscription/store transaction.""" async with self._refresh_lock: # The trail book owns deletion. When it has no such marker, this # is a no-op and must not silently damage the live tracking graph. - removed = self.book.delete(marker) + removed = { + marker: self.book.data.pop(marker) + for marker in markers + if marker in self.book.data + } if not removed: - return False - for src in list(self.pairs): - kept = [pair for pair in self.pairs[src] if pair[0] != marker] - if kept: - self.pairs[src] = kept - else: - del self.pairs[src] - self._resubscribe() - if self._unsub_save: - self._unsub_save() - self._unsub_save = None - await self.store.async_save(self.book.data) + return 0 + previous_pairs = {src: list(pairs) for src, pairs in self.pairs.items()} + had_pending_save = self._unsub_save is not None + try: + for src in list(self.pairs): + kept = [pair for pair in self.pairs[src] if pair[0] not in removed] + if kept: + self.pairs[src] = kept + else: + del self.pairs[src] + self._resubscribe() + if self._unsub_save: + self._unsub_save() + self._unsub_save = None + await self.store.async_save(self.book.data) + except Exception: + # The store is the durable authority. Restore the in-memory + # owner graph so the next successful config sync can retry + # instead of leaving an orphan on disk forever (#335). + self.book.data.update(removed) + self.pairs = previous_pairs + self._resubscribe() + if had_pending_save: + self._schedule_save() + raise self.hass.bus.async_fire("houseplan_trail_updated", {}) - return True + return len(removed) def _resubscribe(self) -> None: """Replace the state subscription for the current pair graph.""" @@ -344,6 +392,10 @@ class TrailRecorder: for src, pair_list in self.pairs.items(): if eid == src or any(eid == vac for _, vac in pair_list): changed |= self._sample(src, now) + self._handle_sample_change(changed, now) + + def _handle_sample_change(self, changed: bool, now: float) -> None: + """Persist and announce one logical sampling pass when it changed.""" if changed: self._schedule_save() if now - self._last_fire >= FIRE_THROTTLE_S: diff --git a/custom_components/houseplan/websocket_api.py b/custom_components/houseplan/websocket_api.py index 38b4aef0..1361cf12 100755 --- a/custom_components/houseplan/websocket_api.py +++ b/custom_components/houseplan/websocket_api.py @@ -493,24 +493,14 @@ async def ws_import_apply(hass: HomeAssistant, connection, msg: dict[str, Any]) hass.bus.async_fire("houseplan_config_updated", {"rev": new_config_rev}) hass.bus.async_fire("houseplan_layout_updated", {"rev": new_layout_rev}) - _refresh_trail_recorder(hass) if kind == "full": - recorder = hass.data.get(DOMAIN, {}).get("trail_recorder") - live_marker_ids = { - str(marker.get("id")) for marker in target_config.get("markers") or [] - } - if recorder is not None: - for marker_id in list(getattr(getattr(recorder, "book", None), "data", {})): - if marker_id not in live_marker_ids: - try: - await recorder.async_delete(marker_id) - except Exception: # noqa: BLE001 - _LOGGER.exception("House Plan: removing orphan import trail failed") + await _purge_trail_recorder(hass, target_config) entry = get_entry(hass) if entry is not None: from .repairs import async_check_plan_files hass.async_create_task(async_check_plan_files(hass, entry)) + _refresh_trail_recorder(hass) connection.send_result(msg["id"], { "ok": True, "kind": kind, @@ -1412,6 +1402,11 @@ async def ws_config_set(hass: HomeAssistant, connection, msg: dict[str, Any]) -> await hass.async_add_executor_job(_collect) except Exception: # noqa: BLE001 — see above: the commit stands regardless _LOGGER.exception("House Plan: collecting superseded files failed") + # The config is already durable, so trail cleanup is best-effort and + # cannot turn this accepted write into a retryable client failure. + # Keep it under write_lock: a later config/set must not resurrect a + # marker between this commit and the ownership decision (#335). + await _purge_trail_recorder(hass, msg["config"]) hass.bus.async_fire("houseplan_config_updated", {"rev": new_rev}) _refresh_trail_recorder(hass) # refresh repair issues (broken plan references) without waiting for a restart @@ -1867,24 +1862,14 @@ async def ws_plan_optimize_undo(hass: HomeAssistant, connection, msg: dict[str, hass.bus.async_fire("houseplan_config_updated", {"rev": new_config_rev}) hass.bus.async_fire("houseplan_layout_updated", {"rev": new_layout_rev}) - _refresh_trail_recorder(hass) if restored_kind == "import": - recorder = hass.data.get(DOMAIN, {}).get("trail_recorder") - live_marker_ids = { - str(marker.get("id")) for marker in restored_config.get("markers") or [] - } - if recorder is not None: - for marker_id in list(getattr(getattr(recorder, "book", None), "data", {})): - if marker_id not in live_marker_ids: - try: - await recorder.async_delete(marker_id) - except Exception: # noqa: BLE001 - _LOGGER.exception("House Plan: removing orphan undo trail failed") + await _purge_trail_recorder(hass, restored_config) entry = get_entry(hass) if entry is not None: from .repairs import async_check_plan_files hass.async_create_task(async_check_plan_files(hass, entry)) + _refresh_trail_recorder(hass) connection.send_result(msg["id"], { "ok": True, "config_rev": new_config_rev, @@ -1968,6 +1953,12 @@ def _refresh_trail_recorder(hass: HomeAssistant) -> None: hass.async_create_task(rec.async_refresh()) +async def _purge_trail_recorder(hass: HomeAssistant, config: dict[str, Any]) -> int: + """Reconcile durable trails with the live marker set after a config commit.""" + rec = hass.data.get(DOMAIN, {}).get("trail_recorder") + return await rec.async_purge_orphans(config) if rec else 0 + + @websocket_api.websocket_command({vol.Required("type"): "houseplan/trail/get"}) @websocket_api.async_response async def ws_trail_get(hass: HomeAssistant, connection: websocket_api.ActiveConnection, msg: dict) -> None: diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index ebcb7664..4021a036 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -7,6 +7,12 @@ the inner-face convergence accounts for both thicknesses, not just the larger one ([#339](https://github.com/Matysh/houseplan-card/issues/339)). +- Vacuum trails now reconcile with ordinary plan edits on the server: deleting + a marker also removes its stored runs even if the browser-side cleanup was + interrupted, and a position sampled during Home Assistant startup is saved + instead of remaining memory-only + ([#335](https://github.com/Matysh/houseplan-card/issues/335)). + ## v1.69.0-beta.1 — 2026-08-28 - House Plan now has a complete German interface. `Deutsch` can be selected diff --git a/docs/CHANGELOG.ru.md b/docs/CHANGELOG.ru.md index 8882c503..806603a8 100755 --- a/docs/CHANGELOG.ru.md +++ b/docs/CHANGELOG.ru.md @@ -13,6 +13,12 @@ смыкания внутренних граней учитывает обе толщины, а не одну наибольшую ([#339](https://github.com/Matysh/houseplan-card/issues/339)). +- Серверные трейлы пылесосов теперь согласуются с обычным редактированием + плана: удаление маркера удаляет и сохранённые маршруты, даже если клиентская + очистка прервалась, а точка, полученная при запуске Home Assistant, + сохраняется на диск и не остаётся только в памяти + ([#335](https://github.com/Matysh/houseplan-card/issues/335)). + ## v1.69.0-beta.1 — 2026-08-28 - В House Plan появилась полная немецкая локализация. `Deutsch` можно выбрать diff --git a/docs/TESTING.md b/docs/TESTING.md index 6c380c63..710ffb2b 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -2153,6 +2153,11 @@ require hands on real hardware — they remain for the human pass. source/vacuum pair stays subscribed. A successful delete removes only that marker's pairs and immediately rebuilds the subscription [backend: test_trail_recorder.py]. +- A successful config edit purges trails for both a missing marker and its + `removed: true` tombstone, but keeps live/hidden markers; a semantic no-op + performs no surprise cleanup. A startup refresh that samples a new point + schedules one save and one throttled update event + [backend: test_ha_websocket.py + test_trail_recorder.py]. - Trail style: cartography casing (dark halo 2.25 + light core 0.9), readable over any room fill. - Hidden marker: neither puck nor trail. Uncalibrated active map: no puck. diff --git a/docs/VACUUM.md b/docs/VACUUM.md index 0f939145..b7dcead8 100644 --- a/docs/VACUUM.md +++ b/docs/VACUUM.md @@ -143,7 +143,11 @@ marker.vacuum = { All fields are optional and old plans remain readable. Hiding retains the configuration. Deleting a vacuum marker removes its layout and server trails, creates the normal removal tombstone and makes the HA device available for a -fresh add without resurrecting old runs. +fresh add without resurrecting old runs. The backend reconciles both a removal +tombstone and a completely absent marker with the trail store after every +successful config change, so an interrupted browser-side cleanup is repaired. +An initial position sampled during integration startup follows the same +debounced persistence and live-update path as a later state event. ## Troubleshooting diff --git a/tests_backend/test_ha_websocket.py b/tests_backend/test_ha_websocket.py index 7b3d6748..36fc0c01 100644 --- a/tests_backend/test_ha_websocket.py +++ b/tests_backend/test_ha_websocket.py @@ -367,6 +367,74 @@ async def test_trail_delete_rejects_non_admin_without_mutation( assert "m1" in recorder.book.data +async def test_config_set_purges_tombstoned_and_absent_trails_durably( + hass: HomeAssistant, hass_ws_client: WebSocketGenerator +) -> None: + """#335 AC1/AC2: the durable config owns the durable trail book.""" + await _setup(hass) + client = await hass_ws_client(hass) + initial = { + "spaces": [], + "markers": [ + {"id": "tombstone", "binding": "entity:vacuum.tombstone"}, + {"id": "hard_drop", "binding": "entity:vacuum.hard_drop"}, + {"id": "live", "binding": "entity:vacuum.live"}, + { + "id": "hidden", "binding": "entity:vacuum.hidden", + "hidden": True, + }, + ], + "settings": {}, + } + await client.send_json_auto_id({ + "type": "houseplan/config/set", "config": initial, "expected_rev": 0, + }) + first = await client.receive_json() + assert first["success"] + await hass.async_block_till_done() + + recorder = hass.data[DOMAIN]["trail_recorder"] + recorder.book.data = { + marker_id: {"current": {"points": [[index, index + 1]]}} + for index, marker_id in enumerate(("tombstone", "hard_drop", "live", "hidden")) + } + await recorder.store.async_save(copy.deepcopy(recorder.book.data)) + + candidate = copy.deepcopy(initial) + candidate["markers"] = [ + { + "id": "tombstone", "binding": "entity:vacuum.tombstone", + "removed": True, "hidden": True, + }, + {"id": "live", "binding": "entity:vacuum.live"}, + { + "id": "hidden", "binding": "entity:vacuum.hidden", + "hidden": True, + }, + ] + await client.send_json_auto_id({ + "type": "houseplan/config/set", "config": candidate, + "expected_rev": first["result"]["rev"], + }) + removed = await client.receive_json() + assert removed["success"] + assert set(recorder.book.data) == {"live", "hidden"} + assert set(await recorder.store.async_load() or {}) == {"live", "hidden"} + + # A semantic no-op is not a lifecycle transition and must not perform a + # surprise cleanup. A later real config commit will reconcile this orphan. + recorder.book.data["late_orphan"] = {"current": {"points": [[9, 10]]}} + await recorder.store.async_save(copy.deepcopy(recorder.book.data)) + await client.send_json_auto_id({ + "type": "houseplan/config/set", "config": candidate, + "expected_rev": removed["result"]["rev"], + }) + noop = await client.receive_json() + assert noop["success"] and noop["result"]["rev"] == removed["result"]["rev"] + assert "late_orphan" in recorder.book.data + assert "late_orphan" in (await recorder.store.async_load() or {}) + + async def test_config_rev_conflict(hass: HomeAssistant, hass_ws_client: WebSocketGenerator) -> None: await _setup(hass) client = await hass_ws_client(hass) diff --git a/tests_backend/test_trail_recorder.py b/tests_backend/test_trail_recorder.py index 73e28a3c..7943b8ab 100644 --- a/tests_backend/test_trail_recorder.py +++ b/tests_backend/test_trail_recorder.py @@ -119,6 +119,49 @@ def test_sample_seeds_a_run_already_in_progress(): assert rec.book.data["m1"]["current"]["points"] == [[1000.0, -500.0]] +def test_refresh_persists_and_announces_a_new_startup_sample_once(): + scheduled = [] + saved = [] + old_call_later = trails.async_call_later + trails.async_call_later = lambda hass, delay, cb: ( + scheduled.append((delay, cb)) or (lambda: None) + ) + try: + class CS: + async def async_load(self): + return {"config": {"markers": [{ + "id": "m1", + "binding": "entity:vacuum.x50", + "vacuum": {"source": "camera.map"}, + }]}} + + class RT: + config_store = CS() + + class TrailStore: + async def async_save(self, data): + saved.append(json.loads(json.dumps(data))) + + rec, hass, _states = _rec() + rec.rt = RT() + rec.store = TrailStore() + rec.pairs = {} + + _run_isolated(rec.async_refresh()) + assert len(scheduled) == 1 + assert scheduled[0][0] == trails.SAVE_DELAY_S + assert hass.bus.fired == [("houseplan_trail_updated", {})] + _run_isolated(scheduled[0][1](None)) + assert saved[-1]["m1"]["current"]["points"] == [[1000.0, -500.0]] + + # The same point is a true no-op: no second save or live-card event. + _run_isolated(rec.async_refresh()) + assert len(scheduled) == 1 + assert hass.bus.fired == [("houseplan_trail_updated", {})] + finally: + trails.async_call_later = old_call_later + + def test_junk_position_ignored(): rec, _hass, states = _rec() states["camera.map"] = S("idle", {"vacuum_position": {"x": "nope", "y": 1}}) @@ -173,6 +216,84 @@ def test_trail_delete_prunes_pair_and_replaces_subscription(): trails.async_track_state_change_event = old_track +def test_orphan_purge_treats_removed_as_absent_and_batches_one_store_write(): + rec, hass, _states = _rec() + rec.book.data = { + "tombstone": {"current": {"points": [[1, 2]]}}, + "hard_drop": {"current": {"points": [[3, 4]]}}, + "live": {"current": {"points": [[5, 6]]}}, + "hidden": {"current": {"points": [[7, 8]]}}, + } + rec.pairs = { + "camera.map": [ + ("tombstone", "vacuum.x50"), + ("hard_drop", "vacuum.x50"), + ("live", "vacuum.x50"), + ("hidden", "vacuum.x50"), + ] + } + saved = [] + + class TrailStore: + async def async_save(self, data): + saved.append(json.loads(json.dumps(data))) + + rec.store = TrailStore() + removed = _run_isolated(rec.async_purge_orphans({"markers": [ + {"id": "tombstone", "removed": True}, + {"id": "live"}, + {"id": "hidden", "hidden": True}, + ]})) + + assert removed == 2 + assert set(rec.book.data) == {"live", "hidden"} + assert rec.pairs == { + "camera.map": [("live", "vacuum.x50"), ("hidden", "vacuum.x50")] + } + assert len(saved) == 1 + assert set(saved[0]) == {"live", "hidden"} + assert hass.bus.fired == [("houseplan_trail_updated", {})] + + +def test_failed_orphan_store_write_rolls_back_and_can_be_retried(): + rec, hass, _states = _rec() + rec.book.data = { + "orphan": {"current": {"points": [[1, 2]]}}, + "live": {"current": {"points": [[3, 4]]}}, + } + rec.pairs = { + "camera.map": [ + ("orphan", "vacuum.x50"), + ("live", "vacuum.x50"), + ] + } + + class FailingStore: + async def async_save(self, _data): + raise OSError("disk full") + + rec.store = FailingStore() + assert _run_isolated(rec.async_purge_orphans({"markers": [{"id": "live"}]})) == 0 + assert set(rec.book.data) == {"orphan", "live"} + assert rec.pairs["camera.map"] == [ + ("orphan", "vacuum.x50"), + ("live", "vacuum.x50"), + ] + assert hass.bus.fired == [] + + saved = [] + + class WorkingStore: + async def async_save(self, data): + saved.append(json.loads(json.dumps(data))) + + rec.store = WorkingStore() + assert _run_isolated(rec.async_purge_orphans({"markers": [{"id": "live"}]})) == 1 + assert set(rec.book.data) == {"live"} + assert set(saved[-1]) == {"live"} + assert hass.bus.fired == [("houseplan_trail_updated", {})] + + def test_object_style_position_is_read(): # Tasshack in-memory attributes hold a Point OBJECT, not a dict class Point: