From c00048611e3486b2507cc88b4a86068f10365203 Mon Sep 17 00:00:00 2001 From: Matysh Date: Tue, 28 Jul 2026 22:51:07 +0300 Subject: [PATCH] HP-1470-02: only refuse a plan reference that is NEW and already broken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught what the local pure suite cannot run. Four HA-harness tests store a plan url whose file is not there — and so, sooner or later, will a user: files disappear from outside Home Assistant, and one of them is what the 'broken plan' repair exists to report. Refusing every write that names a missing file would have locked the owner out of every edit, including detaching it. So the check compares against the stored configuration and only refuses names it has not seen before, which is exactly the pick-then-delete window it was written for. The repairs test now attaches a real plan and removes the file behind it; the quota test budgets from what the shared test config directory already holds instead of assuming an empty folder. --- custom_components/houseplan/websocket_api.py | 37 +++++++++++++++---- tests_backend/test_ha_websocket.py | 39 ++++++++++++++++++-- 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/custom_components/houseplan/websocket_api.py b/custom_components/houseplan/websocket_api.py index 5242b08..28eaa56 100755 --- a/custom_components/houseplan/websocket_api.py +++ b/custom_components/houseplan/websocket_api.py @@ -477,12 +477,11 @@ async def ws_config_get(hass: HomeAssistant, connection, msg: dict[str, Any]) -> -def _missing_internal_plans(plans_dir: Path, config: dict[str, Any]) -> set[str]: - """Plan files a configuration names that are not on disk. +def _internal_plan_names(config: dict[str, Any]) -> set[str]: + """Plan file names a configuration names through OUR urls. - Only OUR urls are checked — `/api/houseplan/content/plans/_/` and the - legacy static path. Anything else belongs to the user and may point wherever - they like. + Only `/api/houseplan/content/plans/_/` and the legacy static path + count. Anything else belongs to the user and may point wherever they like. """ out: set[str] = set() for space in (config or {}).get("spaces") or []: @@ -492,11 +491,32 @@ def _missing_internal_plans(plans_dir: Path, config: dict[str, Any]) -> set[str] if not (url.startswith(CONTENT_URL + "/plans/") or url.startswith(PLANS_URL + "/")): continue name = plan_basename(url) - if name and not (plans_dir / name).is_file(): + if name: out.add(name) return out +def _missing_internal_plans( + plans_dir: Path, config: dict[str, Any], previous: dict[str, Any] | None = None +) -> set[str]: + """Newly named plan files that are not on disk. + + Guards the pick-then-save window: another client may delete a plan between + the moment this one chose it and the moment it saves, which would otherwise + store a url with nothing behind it (HP-1470-02). + + A name the stored configuration already carries is deliberately let through. + It is already broken — repairs says so — and refusing the write would lock + the owner out of every other edit, including the one that detaches it. + """ + known = _internal_plan_names(previous or {}) + return { + name + for name in _internal_plan_names(config) + if name not in known and not (plans_dir / name).is_file() + } + + @websocket_api.websocket_command( { vol.Required("type"): "houseplan/config/set", @@ -552,7 +572,10 @@ async def ws_config_set(hass: HomeAssistant, connection, msg: dict[str, Any]) -> # nothing about whether the file survived (HP-1470-02). External and # legacy urls are not ours to check and are left alone. missing = await hass.async_add_executor_job( - _missing_internal_plans, Path(hass.config.path(PLANS_DIR)), msg["config"] + _missing_internal_plans, + Path(hass.config.path(PLANS_DIR)), + msg["config"], + data.get("config"), ) if missing: connection.send_error( diff --git a/tests_backend/test_ha_websocket.py b/tests_backend/test_ha_websocket.py index 4a63b7c..967aa99 100644 --- a/tests_backend/test_ha_websocket.py +++ b/tests_backend/test_ha_websocket.py @@ -784,8 +784,22 @@ async def test_repair_issue_goes_when_its_space_does( client = await hass_ws_client(hass) registry = ir.async_get(hass) - gone = "/api/houseplan/content/plans/_/nosuchfile.png" - rev = (await _save(client, await _cfg([{"id": "r7", "plan_url": gone}]), 0))["result"]["rev"] + # A reference can only go bad AFTER it is stored — config/set refuses a new + # one that is already broken (HP-1470-02). So: attach a real plan, then let + # the file disappear the way it does in life, from outside Home Assistant. + import os + + from custom_components.houseplan.const import PLANS_DIR + + url, name = await _upload(client, "r7", b"PLAN") + rev = (await _save(client, await _cfg([{"id": "r7", "plan_url": url}]), 0))["result"]["rev"] + await hass.async_block_till_done() + assert registry.async_get_issue(HP_DOMAIN, "broken_plan_r7") is None, "the file is there" + + await hass.async_add_executor_job( + os.remove, os.path.join(hass.config.path(PLANS_DIR), name) + ) + rev = (await _save(client, await _cfg([{"id": "r7", "plan_url": url}]), rev))["result"]["rev"] await hass.async_block_till_done() assert registry.async_get_issue(HP_DOMAIN, "broken_plan_r7") is not None @@ -940,6 +954,9 @@ async def test_startup_sweep_collects_what_no_commit_will( await _setup(hass) client = await hass_ws_client(hass) files, _plans, p = _paths(hass) + # the plan it names has to exist: config/set refuses a NEW broken + # reference (HP-1470-02). The orphans still arrive after the save. + await _seed_aged(hass, [p["kept_plan"]]) assert (await _save(client, await _referenced_config(), 0))["success"] await _seed_aged(hass, list(p.values())) @@ -967,6 +984,9 @@ async def test_periodic_sweep_collects_too( await _setup(hass) client = await hass_ws_client(hass) _files, _plans, p = _paths(hass) + # the plan it names has to exist: config/set refuses a NEW broken + # reference (HP-1470-02). The orphans still arrive after the save. + await _seed_aged(hass, [p["kept_plan"]]) assert (await _save(client, await _referenced_config(), 0))["success"] await _seed_aged(hass, list(p.values())) @@ -994,6 +1014,9 @@ async def test_sweep_and_a_config_write_do_not_race( await _setup(hass) client = await hass_ws_client(hass) _files, plans, p = _paths(hass) + # the plan it names has to exist: config/set refuses a NEW broken + # reference (HP-1470-02). The orphans still arrive after the save. + await _seed_aged(hass, [p["kept_plan"]]) rev = (await _save(client, await _referenced_config(), 0))["result"]["rev"] # an aged, currently unreferenced plan that the commit below adopts @@ -1192,9 +1215,19 @@ async def test_uploads_are_bounded_by_a_store_quota( """HP-1470-01: nothing is deleted for being old, so growth stops at the door.""" from custom_components.houseplan import websocket_api as wsapi + from pathlib import Path + + from custom_components.houseplan.const import PLANS_DIR + from custom_components.houseplan.plans import dir_usage + await _setup(hass) client = await hass_ws_client(hass) - monkeypatch.setattr(wsapi, "MAX_PLANS_FILES", 2) + # every test in this module shares one config directory, so the plans folder + # is not empty here — budget two more from whatever is already stored + stored, _b = await hass.async_add_executor_job( + lambda: dir_usage(Path(hass.config.path(PLANS_DIR)))[::-1] + ) + monkeypatch.setattr(wsapi, "MAX_PLANS_FILES", stored + 2) await _upload(client, "q1", b"one") await _upload(client, "q1", b"two")