v1.45.1: follow-up review of v1.45.0 — R3-1, R3-2

R3-1 (high): v1.45.0 made the upload safe but left deletion to the client —
after a successful save the card asked the backend to remove everything but the
file it had just committed. Two open editors cannot be ordered: a delayed
request from one deleted the plan the other had just saved, leaving the
accepted configuration pointing at nothing, the exact damage copy-on-write was
introduced to prevent.

houseplan/plan/cleanup is removed. config/set collects inside its own write
lock from the two configurations that bracket the commit (plans.collect_plans):
a file the old revision referenced and the new one does not is superseded and
goes; any other unreferenced upload waits out PLAN_ORPHAN_TTL_S, because a
fresh one may belong to a transaction that has not committed yet. The collector
lives in a pure module so it can be reasoned about and unit-tested without the
HA harness.

R3-2: houseplan-space-card signed its plan url and threw the result away —
getCardSize() mutated a throwaway model while render() rebuilt its own from the
config, so the <image> requested the protected path and got 401 on every
render. Both cards now share ContentSigner (src/signing.ts), which also gives
the static card batching, expiry handling and periodic re-signing.  is
released in finally: one failed request no longer wedges a url for the life of
the page.

Tests: five backend interleaving cases from the report, six unit tests for the
pure collector, smoke_space_card_bg (verified to fail against a v1.45.0 build:
the raw url reaches the DOM and no retry happens). 57 smokes, 124 unit, 22
backend-pure.
Docs: CHANGELOG.md + CHANGELOG.ru.md + ARCHITECTURE.md + TESTING.md + STATUS.md.

custom_components/houseplan/const.py

@@ -16,9 +16,15 @@ CONTENT_URL = "/api/houseplan/content"
 # the same number; a client that sends more used to get a partial answer with no
 # way to tell which paths were dropped (review R2-2).
 MAX_SIGN_PATHS = 200
+
+# An uploaded plan that no accepted configuration references is collected only
+# once it is this old. Age is a race guard, not a policy: a plan uploaded
+# seconds ago may belong to another client's transaction that has not written
+# its configuration yet (review R3-1).
+PLAN_ORPHAN_TTL_S = 3600
 FILES_DIR = "houseplan/files"
 CONF_ADMIN_ONLY = "admin_only"
-VERSION = "1.45.0"
+VERSION = "1.45.1"
 
 DEFAULT_CONFIG: dict = {
     "spaces": [],

custom_components/houseplan/manifest.json

@@ -16,5 +16,5 @@
   "issue_tracker": "https://github.com/Matysh/houseplan-card/issues",
   "requirements": [],
   "single_config_entry": true,
-  "version": "1.45.0"
+  "version": "1.45.1"
 }

custom_components/houseplan/plans.py

@@ -0,0 +1,88 @@
+"""Plan-file collection — pure, so it is unit-testable without Home Assistant.
+
+The file system is not part of the configuration store's transaction, so who
+may delete a plan file, and when, is a correctness question rather than a
+housekeeping one. It lives here, apart from the WebSocket plumbing, precisely
+because it is the part that has to be reasoned about and tested.
+"""
+from __future__ import annotations
+
+import logging
+import time
+from pathlib import Path
+from typing import Any
+
+from .const import PLAN_ORPHAN_TTL_S
+from .validation import PLAN_EXTENSIONS
+
+_LOGGER = logging.getLogger(__name__)
+
+
+def plan_basename(url: Any) -> str:
+    """File name a stored plan_url points at ('' when there is none)."""
+    if not isinstance(url, str) or not url:
+        return ""
+    return url.split("?", 1)[0].rsplit("/", 1)[-1]
+
+
+def plan_refs(cfg: dict[str, Any] | None) -> set[str]:
+    """Plan file names a configuration references."""
+    out: set[str] = set()
+    for sp in (cfg or {}).get("spaces") or []:
+        name = plan_basename(sp.get("plan_url"))
+        if name:
+            out.add(name)
+    return out
+
+
+def is_plan_file(name: str) -> bool:
+    """Does this look like a plan we wrote: <space>.<ext> or <space>.<token>.<ext>?"""
+    parts = name.split(".")
+    return len(parts) in (2, 3) and parts[-1].lower() in PLAN_EXTENSIONS
+
+
+def collect_plans(
+    plans_dir: Path,
+    old_cfg: dict[str, Any] | None,
+    new_cfg: dict[str, Any],
+    now: float | None = None,
+) -> int:
+    """Drop plan files the accepted configuration made obsolete (review R3-1).
+
+    Called inside the config write lock, right after the new revision is
+    stored, so it decides from the two configurations that actually bracket the
+    commit instead of trusting a client to say what may be deleted. The earlier
+    design — a `plan/cleanup` command carrying `keep` — could not be ordered
+    against another client's commit: a delayed call removed the file that
+    client had just saved, leaving the accepted configuration pointing at
+    nothing, which is the damage copy-on-write was introduced to prevent.
+
+    Two rules, both conservative:
+      * a file the OLD configuration referenced and the new one does not was
+        authoritative and has been superseded — remove it;
+      * any other unreferenced plan file is a rejected or abandoned upload, and
+        is removed only once PLAN_ORPHAN_TTL_S has passed: a fresh one may
+        belong to a transaction that has not committed yet.
+    """
+    if not plans_dir.is_dir():
+        return 0
+    new_refs = plan_refs(new_cfg)
+    old_refs = plan_refs(old_cfg)
+    cutoff = (time.time() if now is None else now) - PLAN_ORPHAN_TTL_S
+    removed = 0
+    for item in sorted(plans_dir.iterdir()):
+        if not item.is_file() or item.name in new_refs or not is_plan_file(item.name):
+            continue
+        superseded = item.name in old_refs
+        try:
+            stale = item.stat().st_mtime < cutoff
+        except OSError:
+            stale = False
+        if not superseded and not stale:
+            continue
+        try:
+            item.unlink()
+            removed += 1
+        except OSError as err:
+            _LOGGER.warning("House Plan: could not remove the old plan %s: %s", item, err)
+    return removed

custom_components/houseplan/websocket_api.py

@@ -19,10 +19,11 @@ from .const import (
     CONTENT_URL, MAX_SIGN_PATHS, PLANS_DIR, PLANS_URL,
 )
 from .auth import may_write
+from .plans import collect_plans
 from .store import HouseplanData, get_data, get_entry
 from .validation import (
     CONFIG_SCHEMA, LAYOUT_SCHEMA, MAX_PLAN_BYTES,
-    PLAN_EXTENSIONS, POS_SCHEMA, sanitize_filename, valid_space_id,
+    PLAN_EXTENSIONS, POS_SCHEMA, valid_space_id,
 )
 
 
@@ -39,7 +40,6 @@ def async_register(hass: HomeAssistant) -> None:
     websocket_api.async_register_command(hass, ws_config_get)
     websocket_api.async_register_command(hass, ws_config_set)
     websocket_api.async_register_command(hass, ws_plan_set)
-    websocket_api.async_register_command(hass, ws_plan_cleanup)
     websocket_api.async_register_command(hass, ws_files_migrate)
     websocket_api.async_register_command(hass, ws_files_cleanup)
     websocket_api.async_register_command(hass, ws_content_sign)
@@ -321,6 +321,7 @@ async def ws_config_get(hass: HomeAssistant, connection, msg: dict[str, Any]) ->
     connection.send_result(msg["id"], {"config": config, "rev": data.get("rev", 0)})
 
 
+
 @websocket_api.websocket_command(
     {
         vol.Required("type"): "houseplan/config/set",
@@ -362,6 +363,11 @@ async def ws_config_set(hass: HomeAssistant, connection, msg: dict[str, Any]) ->
             return
         new_rev = current_rev + 1
         await rt.config_store.async_save({"config": msg["config"], "rev": new_rev})
+        # still holding the lock: the file system is not part of the store's
+        # transaction, so collection has to be pinned to this commit (R3-1)
+        await hass.async_add_executor_job(
+            collect_plans, Path(hass.config.path(PLANS_DIR)), data.get("config"), msg["config"]
+        )
     hass.bus.async_fire("houseplan_config_updated", {"rev": new_rev})
     # refresh repair issues (broken plan references) without waiting for a restart
     entry = get_entry(hass)
@@ -406,8 +412,10 @@ async def ws_plan_set(hass: HomeAssistant, connection, msg: dict[str, Any]) -> N
     # deleted here (review R2-1). The old name stays readable, so a config write
     # that is later rejected — revision conflict, validation, lost connection —
     # leaves the stored plan exactly as it was. The card calls
-    # `houseplan/plan/cleanup` only after its config CAS succeeds; a crash in
-    # between leaves an orphan file that the next successful save removes.
+    # nothing here; the superseded file is collected by `config/set` itself,
+    # inside the write lock, once a revision that no longer references it has
+    # been accepted (review R3-1). A crash in between leaves an orphan, which
+    # the same collector removes on a later commit once it is old enough.
     #
     # `.` separates the id from the token because a space id cannot contain one
     # (SPACE_ID_RE), so "<space>.<token>.<ext>" can never be confused with the
@@ -422,61 +430,3 @@ async def ws_plan_set(hass: HomeAssistant, connection, msg: dict[str, Any]) -> N
 
     await hass.async_add_executor_job(_write)
     connection.send_result(msg["id"], {"ok": True, "url": f"{CONTENT_URL}/plans/_/{name}"})
-
-
-def _plan_files(plans_dir: Path, space_id: str) -> list[Path]:
-    """Every plan file belonging to a space: the legacy flat name and versioned ones."""
-    out: list[Path] = []
-    if not plans_dir.is_dir():
-        return out
-    for item in plans_dir.iterdir():
-        if not item.is_file():
-            continue
-        parts = item.name.split(".")
-        # "<space>.<ext>" (legacy) or "<space>.<token>.<ext>"
-        if len(parts) in (2, 3) and parts[0] == space_id and parts[-1].lower() in PLAN_EXTENSIONS:
-            out.append(item)
-    return out
-
-
-@websocket_api.websocket_command(
-    {
-        vol.Required("type"): "houseplan/plan/cleanup",
-        vol.Required("space_id"): str,
-        vol.Required("keep"): str,
-    }
-)
-@websocket_api.async_response
-async def ws_plan_cleanup(hass: HomeAssistant, connection, msg: dict[str, Any]) -> None:
-    """Drop superseded plan files for a space (review R2-1).
-
-    Called by the card ONLY after the config write that references `keep` has
-    been accepted. Until then every previous file is still on disk, which is
-    what makes a rejected save harmless. Deleting nothing is always a safe
-    outcome here — the orphans are bounded by one per rejected upload and are
-    collected by the next successful one.
-    """
-    if not _check_write(hass, connection):
-        connection.send_error(msg["id"], "unauthorized", "Only administrators may manage plans")
-        return
-    space_id = msg["space_id"]
-    if not valid_space_id(space_id):
-        connection.send_error(msg["id"], "invalid_space_id", "space_id: only [a-z0-9_-], up to 64 characters")
-        return
-    keep = sanitize_filename(msg["keep"])
-    plans_dir = Path(hass.config.path(PLANS_DIR))
-
-    def _clean() -> int:
-        removed = 0
-        for item in _plan_files(plans_dir, space_id):
-            if item.name == keep:
-                continue
-            try:
-                item.unlink()
-                removed += 1
-            except OSError as err:  # noqa: PERF203 — a stuck file must not fail the save
-                _LOGGER.warning("House Plan: could not remove the old plan %s: %s", item, err)
-        return removed
-
-    removed = await hass.async_add_executor_job(_clean)
-    connection.send_result(msg["id"], {"ok": True, "removed": removed})