From f953a3c286f1943bf659317a25b2d88226f8157b Mon Sep 17 00:00:00 2001 From: Matysh Date: Tue, 28 Jul 2026 19:59:32 +0300 Subject: [PATCH] fix: the guard has to be per-case, not blanket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Protecting every file of a live space also protected the ones a commit had just superseded, and gave rejected uploads immortality. The distinction that matters is narrower: a space with NO plan_url has had its image detached and may want it back; a space that has one can only be holding its own rejects. Attachments: staging folders keep the hour, marker folders get the month. Also: the layout event test asserted the order of separately fired bus events, which nothing promises — it came back [2,1,3] in CI. --- custom_components/houseplan/plans.py | 33 +++++++++++++++++----------- docs/ARCHITECTURE.md | 20 +++++++++-------- docs/CHANGELOG.md | 12 +++++----- docs/CHANGELOG.ru.md | 13 ++++++----- tests_backend/test_ha_websocket.py | 6 +++-- tests_backend/test_validation.py | 32 +++++++++++++++++---------- 6 files changed, 70 insertions(+), 46 deletions(-) diff --git a/custom_components/houseplan/plans.py b/custom_components/houseplan/plans.py index 96d03ec..962cd6f 100644 --- a/custom_components/houseplan/plans.py +++ b/custom_components/houseplan/plans.py @@ -122,17 +122,16 @@ def collect_attachments( """The same commit-scoped rule as `collect_plans`, for marker attachments. A file the old revision referenced and the new one does not was superseded - by this commit and goes. Otherwise: files of a marker that still exists are - left alone, a staging folder (`up_*`, only ever a dialog that was never - saved) is collected after PLAN_ORPHAN_TTL_S, and anything else waits out - SCHEDULED_GRACE_S. Never raises: it runs behind a durable write. + by this commit and goes. Otherwise a staging folder (`up_*` — only ever a + dialog that was never saved) is collected after PLAN_ORPHAN_TTL_S, and + anything else waits out SCHEDULED_GRACE_S. Never raises: it runs behind a + durable write. """ new_refs = attachment_refs(new_cfg) old_refs = attachment_refs(old_cfg) # Same distinction as for plans. A staging folder (`up_*`) is different: it # only ever holds an upload from a dialog that was never saved, so the short # rule is exactly right there even on the timer. - live_markers = {str(m.get("id")) for m in (new_cfg or {}).get("markers") or []} now_s = time.time() if now is None else now staging_cutoff = now_s - PLAN_ORPHAN_TTL_S cutoff = now_s - SCHEDULED_GRACE_S @@ -144,10 +143,10 @@ def collect_attachments( return 0 removed += sweep_upload_temps(files_dir, now) for folder in folders: - if folder.name in live_markers: - continue # the marker is still there; its files are its own business - # a staging folder only ever holds an upload from a dialog that was - # never saved — unambiguous, so the short rule is right there + # A staging folder only ever holds an upload from a dialog that was never + # saved — unambiguous, so an hour is right. A marker's own folder is not: + # removing an attachment is deliberate, but so is re-adding one, and the + # file may have been detached rather than abandoned. Give it a month. limit = staging_cutoff if folder.name.startswith("up_") else cutoff try: items = sorted(p for p in folder.iterdir() if p.is_file()) @@ -236,8 +235,16 @@ def collect_plans( # editor detaches the image when a space switches to "draw" and says the # file stays on disk. So the scheduled pass keeps anything belonging to a # space that still exists, and waits a month for the rest. - live_spaces = {str(sp.get("id")) for sp in (new_cfg or {}).get("spaces") or []} - cutoff = (time.time() if now is None else now) - SCHEDULED_GRACE_S + # A space with NO plan_url has had its image detached — reversible, and the + # editor promises the file stays. A space that HAS one is different: any + # other file of its own is a superseded or rejected upload, so the short + # rule is right for those. Getting this distinction wrong (protecting + # nothing) destroyed two detached plans on 2026-07-28. + detached = { + str(sp.get("id")) for sp in (new_cfg or {}).get("spaces") or [] + if not sp.get("plan_url") + } + cutoff = (time.time() if now is None else now) - PLAN_ORPHAN_TTL_S removed = 0 try: items = sorted(plans_dir.iterdir()) if plans_dir.is_dir() else [] @@ -252,8 +259,8 @@ def collect_plans( continue superseded = item.name in old_refs if not superseded: - if item.name.split(".")[0] in live_spaces: - continue # detached, not abandoned — the space is still there + if item.name.split(".")[0] in detached: + continue # detached, not abandoned — the space is waiting for it try: if item.stat().st_mtime >= cutoff: continue diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 41e1181..72a3d3d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -207,15 +207,17 @@ happen when somebody saves, and a file uploaded into a dialog that was then cancelled would wait for a write that may never come. A new icon has no id yet, so its files go to a per-dialog staging folder and move to the real id once the config write is accepted — the same copy → save → cleanup order as a rebind. -`config/set` collects what its commit superseded — and only that, because -supersession is the one thing a commit knows for certain. *Unreferenced* is a -much weaker signal: detaching a plan is reversible and the editor promises the -file stays. So a file belonging to a space or marker that still exists is never -collected, and anything else waits `SCHEDULED_GRACE_S` (30 days). The exception -is a per-dialog staging folder, which by construction only holds an upload from -a dialog that was never saved — `PLAN_ORPHAN_TTL_S` (1 hour) is right there. -This is not theoretical caution: the one-hour rule applied to unreferenced files -destroyed two detached plans on 2026-07-28. +`config/set` collects what its commit superseded — that much a commit knows for +certain. *Unreferenced* is a far weaker signal, and how weak depends on the +case. A space with `plan_url = null` had its image **detached**, which is +reversible and which the editor promises leaves the file alone: those are never +collected. A space that does have a plan can only be holding its own rejected +uploads, so `PLAN_ORPHAN_TTL_S` (1 hour) still applies to them. For attachments, +a per-dialog staging folder holds nothing but an upload from a dialog that was +never saved — one hour again — while a marker's own folder waits +`SCHEDULED_GRACE_S` (30 days). This is not theoretical caution: applying the +one-hour rule to every unreferenced file destroyed two detached plans on +2026-07-28. **Config writes are serialized** (HP-1454-03). `_writeConfig()` chains onto a single promise: one `config/set` in flight, each carrying the revision the diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index e92a84c..48d7622 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -12,11 +12,13 @@ `config/houseplan/plans/` before updating anything else — and please report it in the Telegram chat if a file is missing. The rule now: **a commit still removes exactly what it replaced**, because - that it knows for certain. Everything else is only *unreferenced*, which is a - reversible state — a file belonging to a space or a device that still exists - is never collected, and anything else waits thirty days instead of an hour. - The one unambiguous case keeps the short rule: a per-dialog staging folder - only ever holds an upload from a dialog that was never saved. + that it knows for certain. Beyond that the question is whether "unreferenced" + means "abandoned", and the answer depends on the case. A space with no plan at + all has had one detached and may want it back — its files are never collected. + A space that does have a plan can only be holding rejected uploads of its own, + so those still go after an hour. Attachments outside a per-dialog staging + folder wait a month; a staging folder, which by construction only ever holds + an upload from a dialog that was never saved, keeps the one-hour rule. ## v1.46.3 — 2026-07-28 (re-check of v1.46.2: HP-1462-01) - **The cleanup at startup now actually cleans up.** It looked its own runtime diff --git a/docs/CHANGELOG.ru.md b/docs/CHANGELOG.ru.md index 48cdcae..66b38bb 100755 --- a/docs/CHANGELOG.ru.md +++ b/docs/CHANGELOG.ru.md @@ -17,11 +17,14 @@ план после v1.46.0 и инстанс перезапускался или проработал сутки — загляните в `config/houseplan/plans/` и напишите в Telegram-чат, если файла нет. Правило теперь такое: **коммит по-прежнему удаляет ровно то, что заменил**, — - это он знает наверняка. Всё остальное лишь «непривязано», а это обратимое - состояние: файл, принадлежащий существующему пространству или устройству, не - удаляется никогда, а всё прочее ждёт тридцать дней вместо часа. Один - однозначный случай сохраняет короткое правило: промежуточная папка диалога - содержит только загрузку из диалога, который так и не сохранили. + это он знает наверняка. Дальше вопрос в том, означает ли «непривязан» + «брошен», и ответ зависит от случая. У пространства, у которого плана нет + вовсе, его отцепили — и, возможно, вернут: его файлы не удаляются никогда. У + пространства, у которого план есть, лишние файлы могут быть только его же + отвергнутыми загрузками — они по-прежнему уходят через час. Вложения вне + промежуточной папки диалога ждут месяц; сама промежуточная папка, где по + построению лежит только загрузка из несохранённого диалога, сохраняет часовое + правило. ## v1.46.3 — 2026-07-28 (перепроверка v1.46.2: HP-1462-01) - **Уборка при старте действительно убирает.** Она искала свои же runtime-данные diff --git a/tests_backend/test_ha_websocket.py b/tests_backend/test_ha_websocket.py index 930b4e1..32f097c 100644 --- a/tests_backend/test_ha_websocket.py +++ b/tests_backend/test_ha_websocket.py @@ -363,6 +363,7 @@ async def test_abandoned_uploads_are_collected_once_old( _url, orphan = await _upload(client, "r3", b"abandoned") old = time.time() - PLAN_ORPHAN_TTL_S - 60 os.utime(plans / orphan, (old, old)) + # r3 still HAS a plan, so this really is a rejected upload — collectable ok = await _save(client, await _cfg([{"id": "r3", "plan_url": url0}]), rev) assert ok["success"] @@ -609,7 +610,8 @@ async def test_layout_keeps_its_revision_and_announces_changes( assert not bad["success"] and bad["error"]["code"] == "conflict" await hass.async_block_till_done() - assert [e["rev"] for e in events] == [1, 2, 3] + # the bus does not promise ordering between separately fired events + assert sorted(e["rev"] for e in events) == [1, 2, 3] async def test_uploaded_svg_is_sandboxed_and_a_pdf_is_not( @@ -862,7 +864,7 @@ async def _seed_aged(hass, names) -> None: from custom_components.houseplan.const import SCHEDULED_GRACE_S - old = time.time() - SCHEDULED_GRACE_S - 60 + old = time.time() - SCHEDULED_GRACE_S - 60 # past every grace def _do() -> None: for path in names: diff --git a/tests_backend/test_validation.py b/tests_backend/test_validation.py index 139598f..21d0fdd 100644 --- a/tests_backend/test_validation.py +++ b/tests_backend/test_validation.py @@ -589,26 +589,34 @@ def test_scheduled_collection_never_takes_a_detached_plan(tmp_path): assert (d / "f2.tok.png").is_file(), "and still touches nothing else of a live space" -def test_scheduled_collection_keeps_a_live_marker_s_files(tmp_path): +def test_attachment_grace_is_a_month_outside_a_staging_folder(tmp_path): + """A staging folder is unambiguous; a marker folder is not. + + `up_*` only ever holds an upload from a dialog that was never saved, so an + hour is right there. A marker's own folder may hold a file that is merely + unreferenced at the moment, and one hour of that turned out to be a way to + lose data (2026-07-28). + """ import os import time files = tmp_path / "files" (files / "m1").mkdir(parents=True) (files / "up_abandoned").mkdir(parents=True) - (files / "dead").mkdir(parents=True) - old = time.time() - const.SCHEDULED_GRACE_S - 60 - for p in ((files / "m1" / "unlinked.pdf"), (files / "dead" / "x.pdf")): - p.write_bytes(b"x") - os.utime(p, (old, old)) - staging = files / "up_abandoned" / "manual.pdf" - staging.write_bytes(b"x") hour_ago = time.time() - const.PLAN_ORPHAN_TTL_S - 60 - os.utime(staging, (hour_ago, hour_ago)) + month_ago = time.time() - const.SCHEDULED_GRACE_S - 60 + + for path, when in ( + ((files / "m1" / "recent.pdf"), hour_ago), + ((files / "m1" / "ancient.pdf"), month_ago), + ((files / "up_abandoned" / "manual.pdf"), hour_ago), + ): + path.write_bytes(b"x") + os.utime(path, (when, when)) cfg = {"markers": [{"id": "m1", "pdfs": []}]} removed = plans.collect_attachments(files, cfg, cfg) - assert (files / "m1" / "unlinked.pdf").is_file(), "the marker exists; leave its files alone" - assert not staging.exists(), "a cancelled dialog is still collected after an hour" - assert not (files / "dead" / "x.pdf").exists(), "a marker that is gone ages out" + assert (files / "m1" / "recent.pdf").is_file(), "an hour is not enough for a marker file" + assert not (files / "m1" / "ancient.pdf").exists(), "a month is" + assert not (files / "up_abandoned").exists(), "a cancelled dialog still goes after an hour" assert removed == 2