v1.46.1: re-check of v1.46.0 — HP-1460-01, -02, -03

HP-1460-01: v1.46.0 stopped overwriting attachments, but picking a free name
and taking it were two steps. Two uploads racing between them agreed on the
same name, both answered 200, and one set of bytes replaced the other;
files/migrate had the same check-then-copy gap. reserve_filename now claims the
name with O_CREAT|O_EXCL as it picks it, and both paths use it. It also splits
the extension off the RAW name and budgets the stem against MAX_FILENAME
including the collision tag — a maximal name lost its '.pdf' and then grew past
the limit, so the view sanitised the request back to a different name and the
attachment 404'd for good.

HP-1460-02: cleanup lived in an 'except Exception', which CancelledError walks
past, only one tmp_path was tracked, promotion had no finally, and the
collector only walks marker folders — an aborted transfer stranded a .upload-*
that nothing would ever remove. An outer finally owns every temporary, a second
'file' part is refused, promotion failure cleans up, and sweep_upload_temps
runs at setup, daily, and inside the commit-scoped collector. Chunks are
batched to 1 MB per disk task instead of one per 64 KB.

HP-1460-03: the layout event reached the static card and not the full one, so
two full cards diverged until a reload. The full card subscribes now and
re-reads ONLY the layout, keyed on its revision. Two hazards handled: it
records revisions it produced itself, and the reaction is deferred ~200 ms
because the event can beat the reply to our own write over the same socket;
positions dragged but not yet sent are flushed and merged on top, so a fix for
a stale UI cannot become a lost drag.

Tests: smoke_layout_sync (fails on a v1.46.0 build), four pure tests for atomic
reservation incl. 20-thread concurrency and the length boundary, a backend test
walking every failing exit path of an upload, and — as the report asked — an
HA-harness test that a repair issue disappears with its space.
Docs: CHANGELOG.md + CHANGELOG.ru.md + ARCHITECTURE.md + TESTING.md + STATUS.md.

custom_components/houseplan/__init__.py

@@ -2,11 +2,13 @@
 from __future__ import annotations
 
 import logging
+from datetime import timedelta
 from pathlib import Path
 
 from homeassistant.components.frontend import add_extra_js_url
 from homeassistant.core import HomeAssistant
 from homeassistant.exceptions import ConfigEntryNotReady
+from homeassistant.helpers.event import async_track_time_interval
 
 from . import websocket_api as hp_ws
 from .const import (
@@ -18,6 +20,7 @@ from .const import (
     PLANS_URL,
     VERSION,
 )
+from .plans import sweep_upload_temps
 from .repairs import async_check_plan_files
 from .store import HouseplanConfigEntry, create_data
 
@@ -97,6 +100,25 @@ async def async_setup_entry(hass: HomeAssistant, entry: HouseplanConfigEntry) ->
         )
 
     await async_check_plan_files(hass, entry)
+
+    # Abandoned upload temporaries (HP-1460-02). A request cleans up after
+    # itself; a hard kill mid-upload does not, and the commit-scoped collector
+    # only walks marker folders — so without this a `.upload-*` from a crashed
+    # transfer would sit there for good, and on an instance nobody edits, so
+    # would a cancelled attachment. Once at startup, then daily.
+    async def _sweep(_now=None) -> None:
+        files_dir = Path(hass.config.path(FILES_DIR))
+        try:
+            n = await hass.async_add_executor_job(sweep_upload_temps, files_dir)
+            if n:
+                _LOGGER.info("House Plan: removed %s abandoned upload temporaries", n)
+        except Exception:  # noqa: BLE001 — housekeeping must never fail a setup
+            _LOGGER.exception("House Plan: sweeping upload temporaries failed")
+
+    await _sweep()
+    entry.async_on_unload(
+        async_track_time_interval(hass, _sweep, timedelta(hours=24))
+    )
     return True
 
 

custom_components/houseplan/const.py

@@ -24,7 +24,7 @@ MAX_SIGN_PATHS = 200
 PLAN_ORPHAN_TTL_S = 3600
 FILES_DIR = "houseplan/files"
 CONF_ADMIN_ONLY = "admin_only"
-VERSION = "1.46.0"
+VERSION = "1.46.1"
 
 DEFAULT_CONFIG: dict = {
     "spaces": [],

custom_components/houseplan/http_api.py

@@ -22,7 +22,7 @@ from homeassistant.core import HomeAssistant
 
 from .const import CONF_ADMIN_ONLY, CONTENT_URL, FILES_DIR, FILES_URL, PLANS_DIR
 from .auth import may_write
-from .plans import unique_filename
+from .plans import TMP_PREFIX, reserve_filename
 from .validation import (
     FILE_EXTENSIONS,
     MAX_FILE_BYTES,
@@ -34,6 +34,8 @@ from .validation import (
 _LOGGER = logging.getLogger(__name__)
 
 _CHUNK = 64 * 1024
+# batch disk writes: one executor job per megabyte instead of per chunk
+_FLUSH_AT = 1024 * 1024
 
 _MIME = {
     ".pdf": "application/pdf",
@@ -119,92 +121,122 @@ class HouseplanUploadView(HomeAssistantView):
         if not may_write(hass, request.get("hass_user")):
             return web.json_response({"error": "unauthorized"}, status=403)
 
+        files_root = Path(hass.config.path(FILES_DIR))
         marker_id = "misc"
         filename: str | None = None
-        tmp_path: Path | None = None
-        too_large = False
-        bad_ext = False
-        files_root = Path(hass.config.path(FILES_DIR))
+        # Every temporary file this request creates, promoted or not. The outer
+        # `finally` removes whatever is left: a dropped connection, a second
+        # `file` part or a failure while promoting used to leave a `.upload-*`
+        # behind for good, and the collector only ever walks marker folders, so
+        # nothing would have picked it up (HP-1460-02).
+        temps: list[Path] = []
+        error: tuple[dict, int] | None = None
 
-        def _open_tmp() -> Path:
+        def _new_tmp() -> Path:
             files_root.mkdir(parents=True, exist_ok=True)
-            fd, name = tempfile.mkstemp(prefix=".upload-", dir=str(files_root))
+            fd, name = tempfile.mkstemp(prefix=TMP_PREFIX, dir=str(files_root))
             os.close(fd)
             return Path(name)
 
-        def _append(target: Path, data: bytes) -> None:
+        def _flush(target: Path, blocks: list[bytes]) -> None:
             with open(target, "ab") as fh:
-                fh.write(data)
+                for block in blocks:
+                    fh.write(block)
 
-        def _discard(target: Path) -> None:
-            try:
-                target.unlink()
-            except OSError:
-                pass
+        def _cleanup(paths: list[Path]) -> None:
+            for path in paths:
+                try:
+                    path.unlink()
+                except OSError:
+                    pass
 
         try:
-            reader = await request.multipart()
-            async for part in reader:
-                if part.name == "marker_id":
-                    marker_id = sanitize_marker_id(await part.text())
-                elif part.name == "file":
-                    filename = part.filename or "file"
-                    if file_ext(filename) not in FILE_EXTENSIONS:
-                        bad_ext = True
-                        break
-                    # Stream to a temporary file instead of collecting the whole
-                    # upload in memory and copying it again into one buffer: a
-                    # 50 MB manual used to cost ~100 MB of RSS mid-request, and
-                    # a few of those at once is real pressure on a small Home
-                    # Assistant host (HP-1454-06).
-                    tmp_path = await hass.async_add_executor_job(_open_tmp)
-                    size = 0
-                    while chunk := await part.read_chunk(_CHUNK):
-                        size += len(chunk)
-                        if size > MAX_FILE_BYTES:
-                            too_large = True
+            try:
+                reader = await request.multipart()
+                async for part in reader:
+                    if part.name == "marker_id":
+                        marker_id = sanitize_marker_id(await part.text())
+                    elif part.name == "file":
+                        if filename is not None:
+                            # one upload per request: a second part would strand
+                            # the first temporary file and make the response
+                            # ambiguous about which url was returned
+                            error = ({"error": "one_file_only"}, 400)
                             break
-                        await hass.async_add_executor_job(_append, tmp_path, chunk)
-                    if too_large:
-                        break
-        except Exception as err:  # noqa: BLE001
-            _LOGGER.warning("House Plan upload: multipart read error: %s", err)
-            if tmp_path is not None:
-                await hass.async_add_executor_job(_discard, tmp_path)
-            return web.json_response({"error": "bad_request"}, status=400)
+                        filename = part.filename or "file"
+                        if file_ext(filename) not in FILE_EXTENSIONS:
+                            error = ({"error": "bad_ext", "allowed": sorted(FILE_EXTENSIONS)}, 400)
+                            break
+                        # Stream to a temporary file instead of collecting the
+                        # whole upload in memory and copying it again into one
+                        # buffer: a 50 MB manual used to cost ~100 MB of RSS
+                        # mid-request (HP-1454-06). Blocks are batched so this
+                        # is one executor job per megabyte, not per 64 KB.
+                        tmp = await hass.async_add_executor_job(_new_tmp)
+                        temps.append(tmp)
+                        size = 0
+                        pending: list[bytes] = []
+                        buffered = 0
+                        while chunk := await part.read_chunk(_CHUNK):
+                            size += len(chunk)
+                            if size > MAX_FILE_BYTES:
+                                error = (
+                                    {"error": "too_large", "max_mb": MAX_FILE_BYTES // 1024 // 1024},
+                                    413,
+                                )
+                                break
+                            pending.append(chunk)
+                            buffered += len(chunk)
+                            if buffered >= _FLUSH_AT:
+                                await hass.async_add_executor_job(_flush, tmp, pending)
+                                pending, buffered = [], 0
+                        if error:
+                            break
+                        if pending:
+                            await hass.async_add_executor_job(_flush, tmp, pending)
+            except Exception as err:  # noqa: BLE001
+                _LOGGER.warning("House Plan upload: multipart read error: %s", err)
+                error = ({"error": "bad_request"}, 400)
 
-        if bad_ext:
-            if tmp_path is not None:
-                await hass.async_add_executor_job(_discard, tmp_path)
+            if error:
+                return web.json_response(error[0], status=error[1])
+            if not temps or not filename:
+                return web.json_response({"error": "no_file"}, status=400)
+
+            tmp_path = temps[0]
+            target_dir = files_root / marker_id
+            safe_name = filename
+
+            def _promote() -> str:
+                """Claim a free name, then move the finished upload onto it.
+
+                Never overwrite an existing attachment: its bytes may be
+                referenced by the stored configuration, and this upload is not
+                part of that transaction — a cancelled dialog or a rejected save
+                would leave the old url serving the new content (HP-1454-02).
+                The name is reserved atomically, so two uploads racing on the
+                same filename cannot agree on it (HP-1460-01).
+                """
+                name = reserve_filename(target_dir, safe_name)
+                try:
+                    os.replace(tmp_path, target_dir / name)
+                except OSError:
+                    (target_dir / name).unlink(missing_ok=True)
+                    raise
+                return name
+
+            try:
+                name = await hass.async_add_executor_job(_promote)
+            except OSError as err:
+                _LOGGER.warning("House Plan upload: could not store the file: %s", err)
+                return web.json_response({"error": "io_error"}, status=500)
+            temps.remove(tmp_path)  # it is the attachment now, not a temporary
             return web.json_response(
-                {"error": "bad_ext", "allowed": sorted(FILE_EXTENSIONS)}, status=400
+                {"ok": True, "url": f"{CONTENT_URL}/files/{marker_id}/{name}", "name": filename}
             )
-        if too_large:
-            await hass.async_add_executor_job(_discard, tmp_path)
-            return web.json_response(
-                {"error": "too_large", "max_mb": MAX_FILE_BYTES // 1024 // 1024}, status=413
-            )
-        if tmp_path is None or not filename:
-            return web.json_response({"error": "no_file"}, status=400)
-
-        safe_name = sanitize_filename(filename)
-        target_dir = files_root / marker_id
-
-        def _promote() -> str:
-            """Move the finished upload in under a name that is free.
-
-            Never overwrite: the previous bytes may be referenced by the stored
-            configuration, and this upload is not part of that transaction — a
-            cancelled dialog or a rejected save would leave the old url pointing
-            at the new content (HP-1454-02). An unreferenced upload is collected
-            later by config/set, once it is old enough.
-            """
-            target_dir.mkdir(parents=True, exist_ok=True)
-            name = unique_filename(target_dir, safe_name)
-            os.replace(tmp_path, target_dir / name)
-            return name
-
-        name = await hass.async_add_executor_job(_promote)
-        return web.json_response(
-            {"ok": True, "url": f"{CONTENT_URL}/files/{marker_id}/{name}", "name": filename}
-        )
+        finally:
+            # BaseException too: cancelling the request task raises
+            # asyncio.CancelledError, which an `except Exception` never saw —
+            # an aborted large upload leaked its temporary file every time
+            if temps:
+                await hass.async_add_executor_job(_cleanup, list(temps))

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.46.0"
+  "version": "1.46.1"
 }

custom_components/houseplan/plans.py

@@ -9,40 +9,66 @@ and tested.
 from __future__ import annotations
 
 import logging
+import os
 import time
 from pathlib import Path
 from typing import Any
 
 from .const import PLAN_ORPHAN_TTL_S
-from .validation import PLAN_EXTENSIONS, sanitize_filename
+from .validation import MAX_FILENAME, PLAN_EXTENSIONS, sanitize_filename
 
 _LOGGER = logging.getLogger(__name__)
 
+# Streaming uploads land here first. The prefix is a dot so the name can never
+# collide with an attachment (sanitize_filename strips leading dots) and is easy
+# to sweep.
+TMP_PREFIX = ".upload-"
 
-def unique_filename(directory: Path, name: str) -> str:
-    """A name inside `directory` that is not taken, deriving from `name`.
 
-    Uploads never overwrite. The bytes already under a name may be referenced by
-    the stored configuration, and an upload is not part of that transaction: a
-    cancelled dialog or a rejected save would otherwise leave a live url serving
-    someone else's file (HP-1454-02).
+def reserve_filename(directory: Path, name: str) -> str:
+    """Atomically claim a free name inside `directory` and return it.
+
+    Creates the file, empty, with `O_CREAT | O_EXCL`, so the name is *taken* the
+    moment it is chosen. The previous version asked `exists()` and returned a
+    string; two uploads racing between the check and the write agreed on the
+    same name and one silently overwrote the other, both reporting success
+    (HP-1460-01). The caller writes the real bytes over the placeholder — it
+    owns the name by then — and must remove it if it never gets that far.
+
+    The result is guaranteed to satisfy `sanitize_filename(result) == result`:
+    the content view sanitises the name in the request too, so a name it would
+    shorten or rewrite is a file that is written and then never served.
     """
-    safe = sanitize_filename(name)
-    if not (directory / safe).exists():
-        return safe
-    stem, dot, suffix = safe.rpartition(".")
+    directory.mkdir(parents=True, exist_ok=True)
+    # Split the extension off the RAW name: sanitize_filename() truncates to
+    # MAX_FILENAME, so sanitising first would cut ".pdf" off a long name and the
+    # attachment would be stored — and served — without its type.
+    base = name.rsplit("/", 1)[-1].rsplit("\\", 1)[-1]
+    stem, dot, suffix = base.rpartition(".")
     if not dot:
-        stem, suffix = safe, ""
-    i = 2
+        stem, suffix = base, ""
+    stem = sanitize_filename(stem)
+    ext = f".{sanitize_filename(suffix)[:16]}" if suffix else ""
+    i = 1
     while True:
-        # "-2", not " (2)": the content view sanitizes the name in the REQUEST
-        # too, and a space or a bracket there turns into "_", so a file called
-        # "manual (2).pdf" was written and then never served. Only characters
-        # that survive sanitize_filename may be used to build a name.
-        candidate = f"{stem}-{i}{'.' + suffix if suffix else ''}"
-        if not (directory / candidate).exists():
-            return candidate
-        i += 1
+        tag = "" if i == 1 else f"-{i}"
+        # budget the stem so the WHOLE name fits, including the collision tag —
+        # appending "-2" to an already maximal name produced a url the view
+        # truncated back to something else, i.e. a permanent 404
+        room = MAX_FILENAME - len(ext) - len(tag)
+        candidate = (stem[:room] if room > 0 else "f") + tag + ext
+        candidate = sanitize_filename(candidate)
+        if candidate.startswith("."):  # a name that is only an extension
+            candidate = "file" + candidate
+        try:
+            fd = os.open(directory / candidate, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o644)
+        except FileExistsError:
+            i += 1
+            if i > 10000:  # pathological directory; do not spin forever
+                raise
+            continue
+        os.close(fd)
+        return candidate
 
 
 def attachment_refs(cfg: dict[str, Any] | None) -> set[str]:
@@ -59,6 +85,34 @@ def attachment_refs(cfg: dict[str, Any] | None) -> set[str]:
     return out
 
 
+def sweep_upload_temps(files_dir: Path, now: float | None = None) -> int:
+    """Remove abandoned streaming temporaries (HP-1460-02).
+
+    The request itself deletes its own, but a hard kill — a restart mid-upload,
+    an OOM — leaves one behind, and the attachment collector only walks marker
+    folders, so it would never be seen. Age-gated for the same reason as the
+    rest: a fresh one belongs to a request still in flight.
+    """
+    cutoff = (time.time() if now is None else now) - PLAN_ORPHAN_TTL_S
+    removed = 0
+    try:
+        items = [p for p in files_dir.iterdir() if p.is_file()] if files_dir.is_dir() else []
+    except OSError as err:
+        _LOGGER.warning("House Plan: could not list %s: %s", files_dir, err)
+        return 0
+    for item in items:
+        if not item.name.startswith(TMP_PREFIX):
+            continue
+        try:
+            if item.stat().st_mtime >= cutoff:
+                continue
+            item.unlink()
+            removed += 1
+        except OSError:
+            continue
+    return removed
+
+
 def collect_attachments(
     files_dir: Path,
     old_cfg: dict[str, Any] | None,
@@ -82,6 +136,7 @@ def collect_attachments(
     except OSError as err:
         _LOGGER.warning("House Plan: could not list %s: %s", files_dir, err)
         return 0
+    removed += sweep_upload_temps(files_dir, now)
     for folder in folders:
         try:
             items = sorted(p for p in folder.iterdir() if p.is_file())

custom_components/houseplan/validation.py

@@ -16,6 +16,9 @@ MAX_FILE_BYTES = 50 * 1024 * 1024
 
 SPACE_ID_RE = re.compile(r"^[a-z0-9_-]{1,64}$")
 _SAFE_NAME_RE = re.compile(r"[^A-Za-z0-9._-]+")
+# The name length the content view will accept back in a request. Anything a
+# generated name must fit inside, collision tag included (HP-1460-01).
+MAX_FILENAME = 120
 
 # ---------- sanitizers ----------
 
@@ -33,7 +36,7 @@ def sanitize_marker_id(value: str) -> str:
 def sanitize_filename(value: str) -> str:
     """Drop the path and leading dots, keep a safe file name."""
     raw = value.rsplit("/", 1)[-1].rsplit("\\", 1)[-1]
-    return _SAFE_NAME_RE.sub("_", raw).lstrip(".")[:120] or "file"
+    return _SAFE_NAME_RE.sub("_", raw).lstrip(".")[:MAX_FILENAME] or "file"
 
 
 def file_ext(filename: str) -> str:

custom_components/houseplan/websocket_api.py

@@ -20,7 +20,7 @@ from .const import (
     CONTENT_URL, FILES_DIR, MAX_SIGN_PATHS, PLANS_DIR, PLANS_URL,
 )
 from .auth import may_write
-from .plans import collect_attachments, collect_plans, unique_filename
+from .plans import collect_attachments, collect_plans, reserve_filename
 from .store import HouseplanData, get_data, get_entry
 from .validation import (
     CONFIG_SCHEMA, LAYOUT_SCHEMA, MAX_CONFIG_BYTES, MAX_PLAN_BYTES,
@@ -194,11 +194,17 @@ async def ws_files_migrate(hass: HomeAssistant, connection, msg: dict[str, Any])
             if not f.is_file():
                 continue
             # a different file may already own this name — do NOT silently point
-            # the url at it; the shared helper picks a free one, using only
-            # characters the content view will accept back in a request
-            target = dst / unique_filename(dst, f.name)
-            shutil.copy2(str(f), str(target))
-            mapping[f.name] = target.name
+            # the url at it. The shared helper CLAIMS a free one atomically, so
+            # a concurrent migrate or upload cannot pick the same one, and the
+            # name it returns is one the content view accepts back in a request.
+            name = reserve_filename(dst, f.name)
+            target = dst / name
+            try:
+                shutil.copy2(str(f), str(target))
+            except OSError:
+                target.unlink(missing_ok=True)  # never leave an empty placeholder
+                raise
+            mapping[f.name] = name
         return mapping
 
     try: