From 9868f1035f726cad80b6df61bca6f0f0607a7c2f Mon Sep 17 00:00:00 2001 From: Matysh Date: Tue, 28 Jul 2026 21:11:11 +0300 Subject: [PATCH 1/4] v1.46.6: the detach promise, actually kept this time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v1.46.4 and v1.46.5 documented that detaching a plan leaves the image on disk, added guards for it, and shipped tests. The guards were never reached: they sit behind 'not superseded', and a file that left the configuration was called superseded. From old_refs - new_refs alone, replacing a plan, detaching one and deleting its space are indistinguishable — so all three deleted the file, at the moment of the save, before any scheduled pass ever ran. Every test I wrote for this called collect_plans(d, cfg, cfg): old config equal to new, i.e. only the scheduled pass. The transition that mattered was never exercised. Codex reproduced it in four lines. Classification is by owner now: space in both, plan A -> plan B : the user picked another image -> removed space in both, plan -> none : detached -> kept space gone : kept (the image was imported; a thirty-day grace measured from file age is meaningless anyway, it was uploaded months ago) space has a plan, other file : rejected upload -> 1 h Attachments follow the same shape: dropped from a device that still exists -> removed (a trash button promises nothing); device gone -> kept; staging folder -> 1 h. Tests: a matrix per rule in the pure module, and — the part that was missing — test_detaching_a_plan_keeps_the_file, which goes through real config/set calls: attach, detach, assert the file is there, restart, assert again, re-attach, replace, assert the replaced one is gone, delete the space, assert the plan survives. Also strengthened the sweep/save race test to assert the save actually succeeded and the config points at the specific expected file, per the report. --- custom_components/houseplan/const.py | 2 +- .../houseplan/frontend/houseplan-card.js | 2 +- custom_components/houseplan/manifest.json | 2 +- custom_components/houseplan/plans.py | 82 +++++--- demo/srv/assets/houseplan-card.js | 2 +- dist/houseplan-card.js | 2 +- docs/ARCHITECTURE.md | 20 +- docs/CHANGELOG.md | 15 ++ docs/CHANGELOG.ru.md | 15 ++ docs/STATUS.md | 4 +- docs/TESTING.md | 13 +- package.json | 2 +- src/houseplan-card.ts | 2 +- tests_backend/test_ha_websocket.py | 59 +++++- tests_backend/test_validation.py | 189 +++++++++++------- 15 files changed, 288 insertions(+), 123 deletions(-) diff --git a/custom_components/houseplan/const.py b/custom_components/houseplan/const.py index ad402474..d56f3eb6 100755 --- a/custom_components/houseplan/const.py +++ b/custom_components/houseplan/const.py @@ -34,7 +34,7 @@ PLAN_ORPHAN_TTL_S = 3600 SCHEDULED_GRACE_S = 30 * 24 * 3600 FILES_DIR = "houseplan/files" CONF_ADMIN_ONLY = "admin_only" -VERSION = "1.46.5" +VERSION = "1.46.6" DEFAULT_CONFIG: dict = { "spaces": [], diff --git a/custom_components/houseplan/frontend/houseplan-card.js b/custom_components/houseplan/frontend/houseplan-card.js index 477ebbea..c84e4171 100755 --- a/custom_components/houseplan/frontend/houseplan-card.js +++ b/custom_components/houseplan/frontend/houseplan-card.js @@ -2561,4 +2561,4 @@ const t=globalThis,e=t.ShadowRoot&&(void 0===t.ShadyCSS||t.ShadyCSS.nativeShadow `} - `}}ps.properties={hass:{attribute:!1},_config:{state:!0},_space:{state:!0},_layout:{state:!0},_devices:{state:!0},_tip:{state:!0},_selId:{state:!0},_toast:{state:!0},_serverCfg:{state:!0},_mode:{state:!0},_tool:{state:!0},_path:{state:!0},_cursorPt:{state:!0},_mergeSel:{state:!0},_openingDialog:{state:!0},_openingInfo:{state:!0},_mergeDialog:{state:!0},_splitSel:{state:!0},_decorTool:{state:!0},_decorStyle:{state:!0},_decorDraft:{state:!0},_decorSel:{state:!0},_decorTextDialog:{state:!0},_kioskDialog:{state:!0},_kioskDots:{state:!0},_areaSel:{state:!0},_nameSel:{state:!0},_roomDialog:{state:!0},_roomEditId:{state:!0},_roomFill:{state:!0},_roomTempSrc:{state:!0},_roomHumSrc:{state:!0},_roomSrcOpen:{state:!0},_roomSrcFilter:{state:!0},_roomNameScale:{state:!0},_roomLabelScale:{state:!0},_spaceDialog:{state:!0},_infoCard:{state:!0},_rulesDialog:{state:!0},_settingsDialog:{state:!0},_importDialog:{state:!0},_markerDialog:{state:!0},_zoom:{state:!0},_view:{state:!0}},ps._touchSeen=!1,ps._noHoverMq="undefined"!=typeof window&&"function"==typeof window.matchMedia&&window.matchMedia("(hover: none)").matches,ps.styles=Ui,customElements.get("houseplan-card")||customElements.define("houseplan-card",ps),window.customCards=window.customCards||[],window.customCards.find(t=>"houseplan-card"===t.type)||window.customCards.push({type:"houseplan-card",name:"House Plan Card",description:"Interactive house plan: spaces, rooms and devices with live states and drag layout."}),console.info("%c HOUSEPLAN-CARD %c v1.46.5 ","background:#3ea6ff;color:#04121f;font-weight:700",""); + `}}ps.properties={hass:{attribute:!1},_config:{state:!0},_space:{state:!0},_layout:{state:!0},_devices:{state:!0},_tip:{state:!0},_selId:{state:!0},_toast:{state:!0},_serverCfg:{state:!0},_mode:{state:!0},_tool:{state:!0},_path:{state:!0},_cursorPt:{state:!0},_mergeSel:{state:!0},_openingDialog:{state:!0},_openingInfo:{state:!0},_mergeDialog:{state:!0},_splitSel:{state:!0},_decorTool:{state:!0},_decorStyle:{state:!0},_decorDraft:{state:!0},_decorSel:{state:!0},_decorTextDialog:{state:!0},_kioskDialog:{state:!0},_kioskDots:{state:!0},_areaSel:{state:!0},_nameSel:{state:!0},_roomDialog:{state:!0},_roomEditId:{state:!0},_roomFill:{state:!0},_roomTempSrc:{state:!0},_roomHumSrc:{state:!0},_roomSrcOpen:{state:!0},_roomSrcFilter:{state:!0},_roomNameScale:{state:!0},_roomLabelScale:{state:!0},_spaceDialog:{state:!0},_infoCard:{state:!0},_rulesDialog:{state:!0},_settingsDialog:{state:!0},_importDialog:{state:!0},_markerDialog:{state:!0},_zoom:{state:!0},_view:{state:!0}},ps._touchSeen=!1,ps._noHoverMq="undefined"!=typeof window&&"function"==typeof window.matchMedia&&window.matchMedia("(hover: none)").matches,ps.styles=Ui,customElements.get("houseplan-card")||customElements.define("houseplan-card",ps),window.customCards=window.customCards||[],window.customCards.find(t=>"houseplan-card"===t.type)||window.customCards.push({type:"houseplan-card",name:"House Plan Card",description:"Interactive house plan: spaces, rooms and devices with live states and drag layout."}),console.info("%c HOUSEPLAN-CARD %c v1.46.6 ","background:#3ea6ff;color:#04121f;font-weight:700",""); diff --git a/custom_components/houseplan/manifest.json b/custom_components/houseplan/manifest.json index 53b05b2f..87fdb855 100755 --- a/custom_components/houseplan/manifest.json +++ b/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.5" + "version": "1.46.6" } diff --git a/custom_components/houseplan/plans.py b/custom_components/houseplan/plans.py index a269a04e..58c4b8e6 100644 --- a/custom_components/houseplan/plans.py +++ b/custom_components/houseplan/plans.py @@ -121,14 +121,21 @@ def collect_attachments( ) -> int: """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 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. + A file the old revision referenced and the new one does not, whose marker + still exists, was removed on purpose — the dialog has a trash button and + promises nothing. It goes. If the marker itself is gone, that is a different + transition and the files are kept, like a deleted space's plan. A staging + folder (`up_*` — only ever a dialog that was never saved) is collected after + PLAN_ORPHAN_TTL_S, anything else after SCHEDULED_GRACE_S. Never raises: it + runs behind a durable write. """ new_refs = attachment_refs(new_cfg) old_refs = attachment_refs(old_cfg) + # Removing an attachment from a device that still exists is the user saying + # "drop this one" — a trash button, no promise that anything is kept. A + # device that is GONE is a different transition, and its files follow the + # same rule as a deleted space's plan: kept. + live_markers = {str(m.get("id")) for m in (new_cfg or {}).get("markers") or []} # 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. @@ -144,10 +151,9 @@ def collect_attachments( removed += sweep_upload_temps(files_dir, now) for folder in folders: # 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 + # saved — unambiguous, so an hour is right, and no device owns it. + staging = folder.name.startswith("up_") + limit = staging_cutoff if staging else cutoff try: items = sorted(p for p in folder.iterdir() if p.is_file()) except OSError: @@ -156,7 +162,15 @@ def collect_attachments( rel = f"{folder.name}/{item.name}" if rel in new_refs: continue - if rel not in old_refs: # not superseded: absence alone is weak evidence + dropped = rel in old_refs and folder.name in live_markers + if not dropped: + if not staging and folder.name not in live_markers: + # No device owns this folder any more. Whether the file was + # attached once or is a leftover upload we cannot tell, and + # by the standing rule that means we keep it. (A staging + # folder is exempt above: it has no device by construction.) + continue + # the device is there and never listed this file: a rejected upload try: if item.stat().st_mtime >= limit: continue @@ -196,6 +210,14 @@ def plan_refs(cfg: dict[str, Any] | None) -> set[str]: return out +def plan_by_space(cfg: dict[str, Any] | None) -> dict[str, str]: + """space id -> the plan file it references ('' when it has none).""" + return { + str(sp.get("id")): plan_basename(sp.get("plan_url")) + for sp in (cfg or {}).get("spaces") or [] + } + + def is_plan_file(name: str) -> bool: """Does this look like a plan we wrote: . or ..?""" parts = name.split(".") @@ -242,10 +264,19 @@ def collect_plans( # nothing) destroyed two detached plans on 2026-07-28. # The short rule fits exactly one case: a space that HAS a plan, where any # other file of its own can only be a superseded or rejected upload. - spaces = {str(sp.get("id")): sp.get("plan_url") for sp in (new_cfg or {}).get("spaces") or []} + old_by_space = plan_by_space(old_cfg) + new_by_space = plan_by_space(new_cfg) + # A file that left the configuration tells us nothing on its own: replacing a + # plan, detaching one and deleting a space all look identical from + # `old_refs - new_refs`. Only the first is a deletion the user asked for + # (HP-1465-01 — the guards below were written and then never reached, + # because the code decided "superseded" before asking why). + replaced = { + name for space, name in old_by_space.items() + if new_by_space.get(space) and new_by_space[space] != name + } now_s = time.time() if now is None else now reject_cutoff = now_s - PLAN_ORPHAN_TTL_S - cutoff = now_s - SCHEDULED_GRACE_S removed = 0 try: items = sorted(plans_dir.iterdir()) if plans_dir.is_dir() else [] @@ -258,21 +289,24 @@ def collect_plans( for item in items: 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 - if not superseded: + if item.name not in replaced: + # Not a replacement. PRODUCT RULE (owner's decision, 2026-07-28): + # a file we were not told to delete is kept, however long it sits + # there. Detaching a plan is one click to undo and the editor says + # the image stays; deleting a space is deliberate but the image is + # usually something the user imported and may not have elsewhere. + # The two errors are not symmetrical — a few unnecessary megabytes + # can always be removed by hand, a file we should not have removed + # cannot be brought back. + # + # The single exception: a space that HAS a plan, and another file of + # its own that has never been the plan. That can only be an upload + # whose save was rejected, and an hour is plenty for it. space = item.name.split(".")[0] - if space in spaces and not spaces[space]: - # PRODUCT RULE (owner's decision, 2026-07-28): a detached plan - # is never deleted, at any age. The space is there and currently - # has no plan — the image was detached, one click undoes that, - # and the editor says the file stays. The two errors are not - # symmetrical: a few megabytes we did not need can always be - # removed by hand, a file we should not have removed cannot be - # brought back. When in doubt, keep it. + if not new_by_space.get(space) or item.name in old_by_space.values(): continue - limit = reject_cutoff if space in spaces else cutoff try: - if item.stat().st_mtime >= limit: + if item.stat().st_mtime >= reject_cutoff: continue except OSError: continue diff --git a/demo/srv/assets/houseplan-card.js b/demo/srv/assets/houseplan-card.js index 477ebbea..c84e4171 100755 --- a/demo/srv/assets/houseplan-card.js +++ b/demo/srv/assets/houseplan-card.js @@ -2561,4 +2561,4 @@ const t=globalThis,e=t.ShadowRoot&&(void 0===t.ShadyCSS||t.ShadyCSS.nativeShadow `} - `}}ps.properties={hass:{attribute:!1},_config:{state:!0},_space:{state:!0},_layout:{state:!0},_devices:{state:!0},_tip:{state:!0},_selId:{state:!0},_toast:{state:!0},_serverCfg:{state:!0},_mode:{state:!0},_tool:{state:!0},_path:{state:!0},_cursorPt:{state:!0},_mergeSel:{state:!0},_openingDialog:{state:!0},_openingInfo:{state:!0},_mergeDialog:{state:!0},_splitSel:{state:!0},_decorTool:{state:!0},_decorStyle:{state:!0},_decorDraft:{state:!0},_decorSel:{state:!0},_decorTextDialog:{state:!0},_kioskDialog:{state:!0},_kioskDots:{state:!0},_areaSel:{state:!0},_nameSel:{state:!0},_roomDialog:{state:!0},_roomEditId:{state:!0},_roomFill:{state:!0},_roomTempSrc:{state:!0},_roomHumSrc:{state:!0},_roomSrcOpen:{state:!0},_roomSrcFilter:{state:!0},_roomNameScale:{state:!0},_roomLabelScale:{state:!0},_spaceDialog:{state:!0},_infoCard:{state:!0},_rulesDialog:{state:!0},_settingsDialog:{state:!0},_importDialog:{state:!0},_markerDialog:{state:!0},_zoom:{state:!0},_view:{state:!0}},ps._touchSeen=!1,ps._noHoverMq="undefined"!=typeof window&&"function"==typeof window.matchMedia&&window.matchMedia("(hover: none)").matches,ps.styles=Ui,customElements.get("houseplan-card")||customElements.define("houseplan-card",ps),window.customCards=window.customCards||[],window.customCards.find(t=>"houseplan-card"===t.type)||window.customCards.push({type:"houseplan-card",name:"House Plan Card",description:"Interactive house plan: spaces, rooms and devices with live states and drag layout."}),console.info("%c HOUSEPLAN-CARD %c v1.46.5 ","background:#3ea6ff;color:#04121f;font-weight:700",""); + `}}ps.properties={hass:{attribute:!1},_config:{state:!0},_space:{state:!0},_layout:{state:!0},_devices:{state:!0},_tip:{state:!0},_selId:{state:!0},_toast:{state:!0},_serverCfg:{state:!0},_mode:{state:!0},_tool:{state:!0},_path:{state:!0},_cursorPt:{state:!0},_mergeSel:{state:!0},_openingDialog:{state:!0},_openingInfo:{state:!0},_mergeDialog:{state:!0},_splitSel:{state:!0},_decorTool:{state:!0},_decorStyle:{state:!0},_decorDraft:{state:!0},_decorSel:{state:!0},_decorTextDialog:{state:!0},_kioskDialog:{state:!0},_kioskDots:{state:!0},_areaSel:{state:!0},_nameSel:{state:!0},_roomDialog:{state:!0},_roomEditId:{state:!0},_roomFill:{state:!0},_roomTempSrc:{state:!0},_roomHumSrc:{state:!0},_roomSrcOpen:{state:!0},_roomSrcFilter:{state:!0},_roomNameScale:{state:!0},_roomLabelScale:{state:!0},_spaceDialog:{state:!0},_infoCard:{state:!0},_rulesDialog:{state:!0},_settingsDialog:{state:!0},_importDialog:{state:!0},_markerDialog:{state:!0},_zoom:{state:!0},_view:{state:!0}},ps._touchSeen=!1,ps._noHoverMq="undefined"!=typeof window&&"function"==typeof window.matchMedia&&window.matchMedia("(hover: none)").matches,ps.styles=Ui,customElements.get("houseplan-card")||customElements.define("houseplan-card",ps),window.customCards=window.customCards||[],window.customCards.find(t=>"houseplan-card"===t.type)||window.customCards.push({type:"houseplan-card",name:"House Plan Card",description:"Interactive house plan: spaces, rooms and devices with live states and drag layout."}),console.info("%c HOUSEPLAN-CARD %c v1.46.6 ","background:#3ea6ff;color:#04121f;font-weight:700",""); diff --git a/dist/houseplan-card.js b/dist/houseplan-card.js index 477ebbea..c84e4171 100755 --- a/dist/houseplan-card.js +++ b/dist/houseplan-card.js @@ -2561,4 +2561,4 @@ const t=globalThis,e=t.ShadowRoot&&(void 0===t.ShadyCSS||t.ShadyCSS.nativeShadow `} - `}}ps.properties={hass:{attribute:!1},_config:{state:!0},_space:{state:!0},_layout:{state:!0},_devices:{state:!0},_tip:{state:!0},_selId:{state:!0},_toast:{state:!0},_serverCfg:{state:!0},_mode:{state:!0},_tool:{state:!0},_path:{state:!0},_cursorPt:{state:!0},_mergeSel:{state:!0},_openingDialog:{state:!0},_openingInfo:{state:!0},_mergeDialog:{state:!0},_splitSel:{state:!0},_decorTool:{state:!0},_decorStyle:{state:!0},_decorDraft:{state:!0},_decorSel:{state:!0},_decorTextDialog:{state:!0},_kioskDialog:{state:!0},_kioskDots:{state:!0},_areaSel:{state:!0},_nameSel:{state:!0},_roomDialog:{state:!0},_roomEditId:{state:!0},_roomFill:{state:!0},_roomTempSrc:{state:!0},_roomHumSrc:{state:!0},_roomSrcOpen:{state:!0},_roomSrcFilter:{state:!0},_roomNameScale:{state:!0},_roomLabelScale:{state:!0},_spaceDialog:{state:!0},_infoCard:{state:!0},_rulesDialog:{state:!0},_settingsDialog:{state:!0},_importDialog:{state:!0},_markerDialog:{state:!0},_zoom:{state:!0},_view:{state:!0}},ps._touchSeen=!1,ps._noHoverMq="undefined"!=typeof window&&"function"==typeof window.matchMedia&&window.matchMedia("(hover: none)").matches,ps.styles=Ui,customElements.get("houseplan-card")||customElements.define("houseplan-card",ps),window.customCards=window.customCards||[],window.customCards.find(t=>"houseplan-card"===t.type)||window.customCards.push({type:"houseplan-card",name:"House Plan Card",description:"Interactive house plan: spaces, rooms and devices with live states and drag layout."}),console.info("%c HOUSEPLAN-CARD %c v1.46.5 ","background:#3ea6ff;color:#04121f;font-weight:700",""); + `}}ps.properties={hass:{attribute:!1},_config:{state:!0},_space:{state:!0},_layout:{state:!0},_devices:{state:!0},_tip:{state:!0},_selId:{state:!0},_toast:{state:!0},_serverCfg:{state:!0},_mode:{state:!0},_tool:{state:!0},_path:{state:!0},_cursorPt:{state:!0},_mergeSel:{state:!0},_openingDialog:{state:!0},_openingInfo:{state:!0},_mergeDialog:{state:!0},_splitSel:{state:!0},_decorTool:{state:!0},_decorStyle:{state:!0},_decorDraft:{state:!0},_decorSel:{state:!0},_decorTextDialog:{state:!0},_kioskDialog:{state:!0},_kioskDots:{state:!0},_areaSel:{state:!0},_nameSel:{state:!0},_roomDialog:{state:!0},_roomEditId:{state:!0},_roomFill:{state:!0},_roomTempSrc:{state:!0},_roomHumSrc:{state:!0},_roomSrcOpen:{state:!0},_roomSrcFilter:{state:!0},_roomNameScale:{state:!0},_roomLabelScale:{state:!0},_spaceDialog:{state:!0},_infoCard:{state:!0},_rulesDialog:{state:!0},_settingsDialog:{state:!0},_importDialog:{state:!0},_markerDialog:{state:!0},_zoom:{state:!0},_view:{state:!0}},ps._touchSeen=!1,ps._noHoverMq="undefined"!=typeof window&&"function"==typeof window.matchMedia&&window.matchMedia("(hover: none)").matches,ps.styles=Ui,customElements.get("houseplan-card")||customElements.define("houseplan-card",ps),window.customCards=window.customCards||[],window.customCards.find(t=>"houseplan-card"===t.type)||window.customCards.push({type:"houseplan-card",name:"House Plan Card",description:"Interactive house plan: spaces, rooms and devices with live states and drag layout."}),console.info("%c HOUSEPLAN-CARD %c v1.46.6 ","background:#3ea6ff;color:#04121f;font-weight:700",""); diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9cf1294a..2cffbc99 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -214,14 +214,22 @@ we should not have removed cannot be brought back.** When the evidence is weak, keep the file. Owner's decision, 2026-07-28, after the one-hour rule applied to every unreferenced file destroyed two detached plans. +The classification is by **owner**, not by "is it referenced". A file leaving +the configuration looks identical whether the plan was replaced, detached, or +its space deleted — and only the first is a deletion the user asked for. Reading +`old_refs - new_refs` and calling it "superseded" deleted a plan the moment it +was detached, under documentation promising the opposite (HP-1465-01). + | Case | What it means | Rule | |---|---|---| -| In the old revision, not in the new | a save replaced it | removed immediately | -| Space exists, has **no** plan | detached, one click to undo | **never removed** | -| Space exists, has a plan | its own rejected upload | `PLAN_ORPHAN_TTL_S` (1 h) | -| Space no longer exists | deliberate deletion, but people misclick | `SCHEDULED_GRACE_S` (30 d) | -| Attachment in `up_*` | a dialog that was never saved | `PLAN_ORPHAN_TTL_S` (1 h) | -| Attachment in a marker folder | unreferenced, but may come back | `SCHEDULED_GRACE_S` (30 d) | +| Space in both, plan A → plan B | the user picked another image | removed immediately | +| Space in both, plan → none | detached; one click undoes it | **kept** | +| Space gone | deliberate, but the image was imported and may be nowhere else | **kept** | +| Space has a plan, plus another file of its own | an upload whose save was rejected | `PLAN_ORPHAN_TTL_S` (1 h) | +| Marker in both, attachment dropped from its list | a trash button, promising nothing | removed immediately | +| Marker gone | same call as a deleted space's plan | **kept** | +| Attachment in `up_*` | a dialog that was never saved; no device owns it | `PLAN_ORPHAN_TTL_S` (1 h) | +| Marker there, file it never listed | a rejected upload | `SCHEDULED_GRACE_S` (30 d) | **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 97f38c27..d352b019 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -1,5 +1,20 @@ # Changelog +## v1.46.6 — 2026-07-28 (the detach promise, actually kept this time) +- **Switching a space to "draw" no longer deletes its image.** v1.46.4 and + v1.46.5 said it did not, and the scheduled cleanup indeed left detached plans + alone — but the save itself deleted the file the moment the reference was + cleared, before any of those guards were reached. The cause: a file that left + the configuration was called "superseded", and from that difference alone + replacing a plan, detaching one and deleting its space are indistinguishable. + Only the first is a deletion anybody asked for. The transition is now + classified by the space that owned the file, and the same distinction applies + to attachments: dropping one from a device that still exists removes it, + deleting the device keeps its manuals. +- **A plan whose space was deleted is kept**, rather than the thirty days + v1.46.5 promised — thirty days measured from the file's age is meaningless + anyway, since it was usually uploaded months earlier. + ## v1.46.5 — 2026-07-28 (audit of every automatic deletion) - **A detached plan is never deleted, at any age.** v1.46.4 gave it a month; this makes it permanent and writes the reason down where the next change will diff --git a/docs/CHANGELOG.ru.md b/docs/CHANGELOG.ru.md index 898145a7..a21603b6 100755 --- a/docs/CHANGELOG.ru.md +++ b/docs/CHANGELOG.ru.md @@ -6,6 +6,21 @@ > **Правило проекта:** оба файла пополняются в одном коммите с самим > изменением — как и остальная документация (см. docs/STATUS.md). +## v1.46.6 — 2026-07-28 (обещание про отцепление, теперь выполненное) +- **Переключение пространства в «нарисовать» больше не удаляет его картинку.** + v1.46.4 и v1.46.5 утверждали, что не удаляет, и плановая уборка действительно + отцеплённые планы не трогала — но само сохранение удаляло файл в тот момент, + когда снималась ссылка, ещё до всех этих проверок. Причина: файл, покинувший + конфигурацию, считался «заменённым», а по одной этой разнице замену плана, + отцепление и удаление пространства различить невозможно. Удалением, о котором + просили, является только первое. Теперь переход классифицируется по + пространству-владельцу, и та же разница применяется к вложениям: убрали файл у + существующего устройства — он удаляется, удалили устройство — его инструкции + остаются. +- **План удалённого пространства сохраняется**, а не тридцать дней, как обещала + v1.46.5: тридцать дней по возрасту файла всё равно бессмысленны — обычно он + загружен месяцы назад. + ## v1.46.5 — 2026-07-28 (ревизия всех автоматических удалений) - **Отцеплённый план не удаляется никогда, ни в каком возрасте.** В v1.46.4 ему давался месяц; теперь это навсегда, и причина записана там, где её увидит diff --git a/docs/STATUS.md b/docs/STATUS.md index 8cc50314..5d1d0fdb 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -15,12 +15,12 @@ | Item | State | |---|---| -| Version | **v1.46.5** everywhere (manifest, const.py, package.json, CARD_VERSION); deployed to the home instance | +| Version | **v1.46.6** everywhere (manifest, const.py, package.json, CARD_VERSION); deployed to the home instance | | Workflow | Since 2026-07-22: minor changes go to branch **`dev`** (build + smokes → deploy home → commit → push, NO release); releases are batched on the owner's command (merge dev→main, one tag, one release with a summary changelog, CI checked on dev beforehand) | | GitHub | https://github.com/Matysh/houseplan-card — **`main` carries every published release, the latest tag is the current version above**; `dev` is where work lands and is merged into `main` at release time (so `dev` is normally equal to or ahead of `main`, never behind). Push via SSH key `ha_jb` (remote git@github.com:…); API releases via the fine-grained PAT in `~/.git-credentials` (Contents R/W, issued 2026-07-23) | | CI | validate.yml (hacs + hassfest + frontend + backend) green; release.yml attaches the bundle on release publish | | HACS | Custom repository works. **Inclusion PR: hacs/default#9004** — open, valid, labeled; ~864 older open PRs but merge rate ≈180/mo; realistic ETA 1–3 months (checked 2026-07-24) | -| Home instance | ha.jbstudio.pro (SSH port 323, key `ha_jb`), deployed **v1.46.5** via direct copy (HACS custom repo also installed) | +| Home instance | ha.jbstudio.pro (SSH port 323, key `ha_jb`), deployed **v1.46.6** via direct copy (HACS custom repo also installed) | | Localization | UI en/ru (src/i18n/*.json), everything user-visible localized incl. kiosk popover | | Tests | Four layers: frontend unit (`npm test`, node:test over `test-build/`), pure backend (`pytest tests_backend`, runs anywhere), HA-harness backend (same folder, CI only — needs py3.13 + pytest-homeassistant-custom-component), and browser smokes (`demo/smoke_*.mjs`, headless chromium). **Counts are not written down here** — they went stale within two releases while the version line beside them was kept current, which reads as less coverage than exists (review R5-2). Run `npm run inventory` for the current numbers, or read them off the last CI run | | Community | **Telegram chat: https://t.me/ha_houseplan** (created 2026-07-27) — the primary user-facing support channel; GitHub issues stay for bugs/features. Link it from any new release notes and posts | diff --git a/docs/TESTING.md b/docs/TESTING.md index 3484da85..411d8c5d 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -239,11 +239,14 @@ Run the *core flows* (marked ★ below) in each environment at least once per mi on the plan after a reload. Same for each tap action and each fill mode [auto: backend test_every_display_mode_the_editor_offers_is_accepted and neighbours, test_a_marker_showing_its_value_can_be_saved] -- [ ] Detaching a plan keeps the file (v1.46.4/v1.46.5): switch a space to - "draw", restart, wait — the image stays in `config/houseplan/plans/` - indefinitely and can be re-attached. Replacing a plan still removes the one - it replaced, immediately - [auto: unit: test_scheduled_collection_never_takes_a_detached_plan] +- [ ] Detaching a plan keeps the file (v1.46.6): switch a space to "draw" and + SAVE — the image is still in `config/houseplan/plans/` right afterwards, + and after a restart, and can be re-attached. Deleting the space keeps it + too. Replacing a plan still removes the one it replaced, immediately. + Check straight after the save: the earlier bug deleted the file at that + moment, while every scheduled-pass test passed + [auto: unit: test_plan_collection_matrix, test_attachment_collection_matrix, + backend test_detaching_a_plan_keeps_the_file] - [ ] Rebinding a device does not eat its manuals (v1.46.5): attach two files to a device, rebind it to another HA device — both are readable afterwards. If a copy failed, the file it failed on is still there rather than deleted diff --git a/package.json b/package.json index 1ab7c4f9..b61d0b02 100755 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "houseplan-card", - "version": "1.46.5", + "version": "1.46.6", "description": "Interactive house plan Lovelace card for Home Assistant", "license": "MIT", "type": "module", diff --git a/src/houseplan-card.ts b/src/houseplan-card.ts index 294a1ca1..6de53401 100755 --- a/src/houseplan-card.ts +++ b/src/houseplan-card.ts @@ -35,7 +35,7 @@ import './space-card'; import { cardStyles } from './styles'; import { langOf, t, type I18nKey } from './i18n'; -const CARD_VERSION = '1.46.5'; +const CARD_VERSION = '1.46.6'; const LS_KEY = 'houseplan_card_layout_v1'; const LS_CFG = 'houseplan_card_cfg_v1'; // cache of the server config+layout for instant rendering const LS_ZOOM = 'houseplan_card_zoom_v1'; diff --git a/tests_backend/test_ha_websocket.py b/tests_backend/test_ha_websocket.py index d364f181..d063cba5 100644 --- a/tests_backend/test_ha_websocket.py +++ b/tests_backend/test_ha_websocket.py @@ -889,7 +889,7 @@ def _paths(hass): "kept_file": os.path.join(files, "m5", "kept.pdf"), "kept_plan": os.path.join(plans, "s5.tok.png"), "orphan_file": os.path.join(files, "up_cancelled", "manual.pdf"), - "orphan_plan": os.path.join(plans, "deleted_space.orphan.png"), + "orphan_plan": os.path.join(plans, "s5.reject.png"), } @@ -992,18 +992,21 @@ async def test_sweep_and_a_config_write_do_not_race( cfg2["spaces"][0]["plan_url"] = "/api/houseplan/content/plans/_/s5.newcomer.png" entry = hass.config_entries.async_entries(DOMAIN)[0] - await asyncio.gather( + _reload, saved = await asyncio.gather( hass.config_entries.async_reload(entry.entry_id), # runs the sweep _save(client, cfg2, rev), ) await hass.async_block_till_done() + # assert the CONCRETE outcome: a save that came back `not_ready` would leave + # the old config pointing at the old file and satisfy a vaguer check + assert saved["success"], saved.get("error") await client.send_json_auto_id({"type": "houseplan/config/get"}) stored = (await client.receive_json())["result"]["config"] - referenced = stored["spaces"][0]["plan_url"].rsplit("/", 1)[-1] + assert stored["spaces"][0]["plan_url"].endswith("s5.newcomer.png") assert await hass.async_add_executor_job( - os.path.isfile, os.path.join(plans, referenced) - ), f"the accepted config points at {referenced}, which must exist" + os.path.isfile, os.path.join(plans, "s5.newcomer.png") + ), "the file the accepted config points at must exist" async def test_files_cleanup_keeps_referenced_files( @@ -1048,3 +1051,49 @@ async def test_files_cleanup_keeps_referenced_files( "a file the configuration still references survives a cleanup of its folder" ) assert not await hass.async_add_executor_job(os.path.isfile, os.path.join(folder, "orphan.pdf")) + + +async def test_detaching_a_plan_keeps_the_file( + hass: HomeAssistant, hass_ws_client: WebSocketGenerator +) -> None: + """HP-1465-01, through the real save — where the earlier tests never looked. + + Every check for this lived in the pure collector with old and new config + equal, i.e. the scheduled pass. The transition that matters is a save, and + there the file was deleted the moment the reference was cleared. + """ + import os + + from custom_components.houseplan.const import PLANS_DIR + + await _setup(hass) + client = await hass_ws_client(hass) + plans = hass.config.path(PLANS_DIR) + + url, name = await _upload(client, "d1", b"PLAN", ext="png") + rev = (await _save(client, await _cfg([{"id": "d1", "plan_url": url}]), 0))["result"]["rev"] + assert await hass.async_add_executor_job(os.path.isfile, os.path.join(plans, name)) + + # detach: the space stays, its plan does not + rev = (await _save(client, await _cfg([{"id": "d1", "plan_url": None}]), rev))["result"]["rev"] + assert await hass.async_add_executor_job(os.path.isfile, os.path.join(plans, name)), ( + "the editor says the image stays on disk — it has to actually stay" + ) + + # a restart does not change its mind either + entry = hass.config_entries.async_entries(DOMAIN)[0] + assert await hass.config_entries.async_reload(entry.entry_id) + await hass.async_block_till_done() + assert await hass.async_add_executor_job(os.path.isfile, os.path.join(plans, name)) + + # re-attach, then replace: THAT removes the one it replaced + rev = (await _save(client, await _cfg([{"id": "d1", "plan_url": url}]), rev))["result"]["rev"] + url2, name2 = await _upload(client, "d1", b"NEWPLAN", ext="png") + rev = (await _save(client, await _cfg([{"id": "d1", "plan_url": url2}]), rev))["result"]["rev"] + assert await hass.async_add_executor_job(os.path.isfile, os.path.join(plans, name2)) + assert not await hass.async_add_executor_job(os.path.isfile, os.path.join(plans, name)) + + # and deleting the space keeps its plan + await _save(client, await _cfg([]), rev) + await hass.async_block_till_done() + assert await hass.async_add_executor_job(os.path.isfile, os.path.join(plans, name2)) diff --git a/tests_backend/test_validation.py b/tests_backend/test_validation.py index bed59412..58db416d 100644 --- a/tests_backend/test_validation.py +++ b/tests_backend/test_validation.py @@ -265,17 +265,6 @@ def test_collect_plans_keeps_a_fresh_unreferenced_upload(tmp_path): assert (d / "f1.inflight.png").is_file() -def test_collect_plans_takes_an_aged_orphan(tmp_path): - collect_plans = plans.collect_plans - - d = _plans(tmp_path, ["f1.keep.png"]) - # past the long grace, and belonging to no space in the config - _plans(tmp_path, ["f1.abandoned.png"], age=const.SCHEDULED_GRACE_S + 60) - removed = collect_plans(d, _cfg("/p/f1.keep.png"), _cfg("/p/f1.keep.png")) - assert removed == 1 - assert (d / "f1.keep.png").is_file() and not (d / "f1.abandoned.png").exists() - - def test_collect_plans_never_touches_a_referenced_or_foreign_file(tmp_path): PLAN_ORPHAN_TTL_S = const.PLAN_ORPHAN_TTL_S collect_plans = plans.collect_plans @@ -477,31 +466,6 @@ def test_attachment_refs_reads_marker_urls(): assert plans.attachment_refs(cfg) == set() -def test_collect_attachments_supersedes_and_ages(tmp_path): - import os - import time - - collect_attachments = plans.collect_attachments - files = tmp_path / "files" - (files / "m1").mkdir(parents=True) - for n in ("old.pdf", "new.pdf", "cancelled.pdf"): - (files / "m1" / n).write_bytes(b"x") - - # the commit swapped old.pdf for new.pdf; cancelled.pdf is a fresh upload - # nobody saved — it may belong to a dialog that is still open - removed = collect_attachments(files, _acfg("m1/old.pdf"), _acfg("m1/new.pdf")) - assert removed == 1 - assert not (files / "m1" / "old.pdf").exists() - assert (files / "m1" / "new.pdf").is_file() - assert (files / "m1" / "cancelled.pdf").is_file() - - old = time.time() - const.SCHEDULED_GRACE_S - 60 - os.utime(files / "m1" / "cancelled.pdf", (old, old)) - assert collect_attachments(files, _acfg("m1/new.pdf"), _acfg("m1/new.pdf")) == 1 - assert not (files / "m1" / "cancelled.pdf").exists() - assert (files / "m1" / "new.pdf").is_file() - - def test_collect_attachments_removes_the_empty_folder_and_never_raises(tmp_path): import os import time @@ -554,53 +518,130 @@ def test_legacy_segments_are_dropped_by_the_server(): assert "segments" not in out -def test_scheduled_collection_never_takes_a_detached_plan(tmp_path): - """2026-07-28, on the author's own instance: two plans were deleted. - - Detaching a plan (switching a space to "draw") is reversible and the editor - says the file stays on disk. The timer only knows "nothing points at it", - applied the one-hour orphan rule, and removed images that had been detached - weeks earlier. A commit may still collect what it superseded — it knows it - replaced something. The timer may not. - """ +def _aged(path, seconds): import os import time + t = time.time() - seconds + os.utime(path, (t, t)) + + +def _sp(sid, url): + return {"id": sid, "plan_url": f"/api/houseplan/content/plans/_/{url}" if url else None} + + +def test_plan_collection_matrix(tmp_path): + """Which config transition means "the user asked for this file to go"? + + `old_refs - new_refs` cannot tell replace, detach and delete-space apart — + they look identical. v1.46.4/v1.46.5 added guards for detach and only ever + reached them on the scheduled pass, so the commit itself still deleted a + detached plan the moment it was detached (HP-1465-01). One case is a + deletion the user asked for; the rest are kept. + """ + collect = plans.collect_plans d = tmp_path / "plans" d.mkdir() - for n in ("f1.svg", "f2.tok.png", "gone.old.png"): - (d / n).write_bytes(b"x") - t = time.time() - const.SCHEDULED_GRACE_S - 60 - os.utime(d / n, (t, t)) - cfg = {"spaces": [{"id": "f1", "plan_url": None}, # detached, space alive - {"id": "f2", "plan_url": None}]} # same - assert plans.collect_plans(d, cfg, cfg) == 1 - assert (d / "f1.svg").is_file(), "a detached plan of a live space is kept, at any age" - assert (d / "f2.tok.png").is_file() - assert not (d / "gone.old.png").exists(), "a plan of a deleted space ages out after a month" + def seed(*names): + for n in names: + (d / n).write_bytes(b"x") + _aged(d / n, const.SCHEDULED_GRACE_S * 2) # old enough for any rule - # a space that HAS a plan: its other files can only be its own rejects - import os - import time + # 1. replace: the user picked a different image for the same space + seed("f1.old.png", "f1.new.png") + assert collect(d, {"spaces": [_sp("f1", "f1.old.png")]}, + {"spaces": [_sp("f1", "f1.new.png")]}) == 1 + assert not (d / "f1.old.png").exists() and (d / "f1.new.png").is_file() - (d / "f9.current.png").write_bytes(b"x") - (d / "f9.reject.png").write_bytes(b"x") - hour = time.time() - const.PLAN_ORPHAN_TTL_S - 60 - os.utime(d / "f9.reject.png", (hour, hour)) - live = {"spaces": [{"id": "f1", "plan_url": None}, {"id": "f2", "plan_url": None}, - {"id": "f9", "plan_url": "/p/f9.current.png"}]} - assert plans.collect_plans(d, live, live) == 1 - assert (d / "f9.current.png").is_file() - assert not (d / "f9.reject.png").exists() + # 2. detach: same space, switched to "draw" + seed("f2.png") + assert collect(d, {"spaces": [_sp("f2", "f2.png")]}, + {"spaces": [_sp("f2", None)]}) == 0 + assert (d / "f2.png").is_file(), "the editor says the file stays — it stays" - # a commit still removes what it SUPERSEDED — that it knows for certain - old = {"spaces": [{"id": "f1", "plan_url": "/p/f1.svg"}, {"id": "f2", "plan_url": None}]} - new = {"spaces": [{"id": "f1", "plan_url": "/p/f1.new.png"}, {"id": "f2", "plan_url": None}]} - (d / "f1.new.png").write_bytes(b"x") - assert plans.collect_plans(d, old, new) == 1 - assert not (d / "f1.svg").exists() - assert (d / "f2.tok.png").is_file(), "and still touches nothing else of a live space" + # 3. the space is deleted outright + seed("f3.png") + assert collect(d, {"spaces": [_sp("f3", "f3.png")]}, {"spaces": []}) == 0 + assert (d / "f3.png").is_file() + + # 4. the scheduled pass, later, still keeps both + cfg = {"spaces": [_sp("f2", None)]} + assert collect(d, cfg, cfg) == 0 + assert (d / "f2.png").is_file() and (d / "f3.png").is_file() + + # 5. a rejected upload: the space HAS a plan, this file never was one + seed("f4.current.png", "f4.reject.png") + live = {"spaces": [_sp("f4", "f4.current.png")]} + assert collect(d, live, live) == 1 + assert (d / "f4.current.png").is_file() and not (d / "f4.reject.png").exists() + + # …but only once it is old; a fresh one may be a transaction in flight + (d / "f4.fresh.png").write_bytes(b"x") + assert collect(d, live, live) == 0 + assert (d / "f4.fresh.png").is_file() + + # 6. the same file still referenced by another space is never touched + seed("shared.png") + assert collect(d, {"spaces": [_sp("a", "shared.png"), _sp("b", "shared.png")]}, + {"spaces": [_sp("a", None), _sp("b", "shared.png")]}) == 0 + assert (d / "shared.png").is_file() + + +def test_attachment_collection_matrix(tmp_path): + """Removing an attachment is a trash button; deleting the device is not.""" + collect = plans.collect_attachments + + def case(name): + d = tmp_path / name + d.mkdir() + return d + + def seed(root, folder, fname, age=None): + (root / folder).mkdir(parents=True, exist_ok=True) + p = root / folder / fname + p.write_bytes(b"x") + if age: + _aged(p, age) + return p + + def cfg(*markers): + return {"markers": [ + {"id": mid, "pdfs": [{"url": f"/api/houseplan/content/files/{mid}/{n}"} for n in names]} + for mid, names in markers + ]} + + # 1. the user removed one attachment from a device that still exists + d = case("dropped") + seed(d, "m1", "dropped.pdf") + seed(d, "m1", "kept.pdf") + assert collect(d, cfg(("m1", ["dropped.pdf", "kept.pdf"])), cfg(("m1", ["kept.pdf"]))) == 1 + assert not (d / "m1" / "dropped.pdf").exists() + assert (d / "m1" / "kept.pdf").is_file() + + # 2. the device itself is gone: its manuals are not ours to throw away + d = case("device_gone") + seed(d, "m2", "manual.pdf", age=const.SCHEDULED_GRACE_S * 2) + assert collect(d, cfg(("m2", ["manual.pdf"])), cfg()) == 0 + assert (d / "m2" / "manual.pdf").is_file() + # and the scheduled pass, later, agrees + assert collect(d, cfg(), cfg()) == 0 + assert (d / "m2" / "manual.pdf").is_file() + + # 3. a dialog that was never saved, in its own staging folder + d = case("staging") + seed(d, "up_x", "manual.pdf", age=const.PLAN_ORPHAN_TTL_S + 60) + assert collect(d, cfg(), cfg()) == 1 + assert not (d / "up_x").exists() + + # 4. an upload into a live device's folder whose save was rejected + d = case("reject") + seed(d, "m3", "current.pdf") + seed(d, "m3", "rejected.pdf", age=const.SCHEDULED_GRACE_S + 60) + live = cfg(("m3", ["current.pdf"])) + assert collect(d, live, live) == 1 + assert (d / "m3" / "current.pdf").is_file() + assert not (d / "m3" / "rejected.pdf").exists() def test_attachment_grace_is_a_month_outside_a_staging_folder(tmp_path): From 8e07e3c958734421c9379f554122d01992b67ad4 Mon Sep 17 00:00:00 2001 From: Matysh Date: Tue, 28 Jul 2026 21:13:45 +0300 Subject: [PATCH 2/4] test: race the sweep against a save, not a reload against a save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reload has an unload window where any WS call answers not_ready, so the save failed at random — and the vaguer assertion this test used to carry was exactly what hid that. Driving data.sweep() directly is the concurrency the write lock actually guards. --- tests_backend/test_ha_websocket.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests_backend/test_ha_websocket.py b/tests_backend/test_ha_websocket.py index d063cba5..3cb6fabf 100644 --- a/tests_backend/test_ha_websocket.py +++ b/tests_backend/test_ha_websocket.py @@ -991,11 +991,14 @@ async def test_sweep_and_a_config_write_do_not_race( cfg2 = await _referenced_config() cfg2["spaces"][0]["plan_url"] = "/api/houseplan/content/plans/_/s5.newcomer.png" - entry = hass.config_entries.async_entries(DOMAIN)[0] - _reload, saved = await asyncio.gather( - hass.config_entries.async_reload(entry.entry_id), # runs the sweep - _save(client, cfg2, rev), - ) + # Drive the sweep directly rather than through a reload: an entry reload has + # an unload window in which any WS call legitimately answers `not_ready`, so + # a save racing THAT proves nothing about the lock and fails at random. + from custom_components.houseplan.store import get_data + + data = get_data(hass) + assert data is not None and data.sweep is not None + _swept, saved = await asyncio.gather(data.sweep(), _save(client, cfg2, rev)) await hass.async_block_till_done() # assert the CONCRETE outcome: a save that came back `not_ready` would leave From f4af2fe5087727e4d559217acc4f8dab2015dbaf Mon Sep 17 00:00:00 2001 From: Matysh Date: Tue, 28 Jul 2026 21:18:53 +0300 Subject: [PATCH 3/4] fix: stop ageing files out entirely, except staging folders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The strengthened race test earned its keep on the first run: the sweep deleted an aged 'rejected upload' while a save was committing a reference to it, and the accepted config came out pointing at nothing. The write lock serializes the two but cannot help when the sweep goes first. So the age rule is gone for plans and for marker folders. What remains is one sentence: a file goes when an action says so — a plan replaced, an attachment dropped from a device that still exists — plus a per-dialog staging folder after an hour, which by construction can only hold an upload nobody saved. Cost: an upload whose save failed sits there until someone removes it by hand. That is the side of the trade the owner picked, and it is the side that cannot lose data. --- custom_components/houseplan/const.py | 13 +++--- custom_components/houseplan/plans.py | 61 ++++++++++++---------------- docs/ARCHITECTURE.md | 4 +- docs/CHANGELOG.md | 6 +++ docs/CHANGELOG.ru.md | 6 +++ tests_backend/test_validation.py | 43 ++++++++------------ 6 files changed, 61 insertions(+), 72 deletions(-) diff --git a/custom_components/houseplan/const.py b/custom_components/houseplan/const.py index d56f3eb6..9d9f054b 100755 --- a/custom_components/houseplan/const.py +++ b/custom_components/houseplan/const.py @@ -23,14 +23,11 @@ MAX_SIGN_PATHS = 200 # its configuration yet (review R3-1). PLAN_ORPHAN_TTL_S = 3600 -# The scheduled sweep is a different judgement from a commit's. A commit knows -# it replaced a file; the timer only knows nobody points at one right now — and -# "nobody points at it" is a normal, reversible state. Detaching a plan (switch -# a space to "draw") leaves the image on disk on purpose, and re-attaching it -# later is a thing people do. On 2026-07-28 the hourly rule applied to that case -# and removed two plans the owner had detached weeks earlier; they were not -# recoverable. So the timer waits a month, and never touches a plan or an -# attachment that still belongs to something in the configuration. +# Kept for compatibility with anything reading it; the collectors no longer use +# a long grace at all. Every attempt to age files out ended badly — first by +# deleting detached plans, then by racing the save that was about to reference a +# retried upload. What is left is deliberately simple: files go when the user's +# action says so, plus staging folders after PLAN_ORPHAN_TTL_S. SCHEDULED_GRACE_S = 30 * 24 * 3600 FILES_DIR = "houseplan/files" CONF_ADMIN_ONLY = "admin_only" diff --git a/custom_components/houseplan/plans.py b/custom_components/houseplan/plans.py index 58c4b8e6..b67c6ca2 100644 --- a/custom_components/houseplan/plans.py +++ b/custom_components/houseplan/plans.py @@ -14,7 +14,7 @@ import time from pathlib import Path from typing import Any -from .const import PLAN_ORPHAN_TTL_S, SCHEDULED_GRACE_S +from .const import PLAN_ORPHAN_TTL_S from .validation import MAX_FILENAME, PLAN_EXTENSIONS, sanitize_filename _LOGGER = logging.getLogger(__name__) @@ -123,11 +123,10 @@ def collect_attachments( A file the old revision referenced and the new one does not, whose marker still exists, was removed on purpose — the dialog has a trash button and - promises nothing. It goes. If the marker itself is gone, that is a different - transition and the files are kept, like a deleted space's plan. A staging - folder (`up_*` — only ever a dialog that was never saved) is collected after - PLAN_ORPHAN_TTL_S, anything else after SCHEDULED_GRACE_S. Never raises: it - runs behind a durable write. + promises nothing. It goes. Everything else is kept, except a staging folder + (`up_*`), which by construction only ever holds an upload from a dialog that + was never saved: those go after PLAN_ORPHAN_TTL_S. Never raises: it runs + behind a durable write. """ new_refs = attachment_refs(new_cfg) old_refs = attachment_refs(old_cfg) @@ -141,7 +140,6 @@ def collect_attachments( # rule is exactly right there even on the timer. now_s = time.time() if now is None else now staging_cutoff = now_s - PLAN_ORPHAN_TTL_S - cutoff = now_s - SCHEDULED_GRACE_S removed = 0 try: folders = sorted(p for p in files_dir.iterdir() if p.is_dir()) if files_dir.is_dir() else [] @@ -153,7 +151,6 @@ def collect_attachments( # A staging folder only ever holds an upload from a dialog that was never # saved — unambiguous, so an hour is right, and no device owns it. staging = folder.name.startswith("up_") - limit = staging_cutoff if staging else cutoff try: items = sorted(p for p in folder.iterdir() if p.is_file()) except OSError: @@ -164,15 +161,14 @@ def collect_attachments( continue dropped = rel in old_refs and folder.name in live_markers if not dropped: - if not staging and folder.name not in live_markers: - # No device owns this folder any more. Whether the file was - # attached once or is a leftover upload we cannot tell, and - # by the standing rule that means we keep it. (A staging - # folder is exempt above: it has no device by construction.) + if not staging: + # Same rule as for plans: not asked for, so kept. A file in + # a device's folder that the device does not list is an + # upload whose save was rejected — and ageing those out + # raced the retry that was about to reference them. continue - # the device is there and never listed this file: a rejected upload try: - if item.stat().st_mtime >= limit: + if item.stat().st_mtime >= staging_cutoff: continue except OSError: continue @@ -275,8 +271,6 @@ def collect_plans( name for space, name in old_by_space.items() if new_by_space.get(space) and new_by_space[space] != name } - now_s = time.time() if now is None else now - reject_cutoff = now_s - PLAN_ORPHAN_TTL_S removed = 0 try: items = sorted(plans_dir.iterdir()) if plans_dir.is_dir() else [] @@ -290,26 +284,21 @@ def collect_plans( if not item.is_file() or item.name in new_refs or not is_plan_file(item.name): continue if item.name not in replaced: - # Not a replacement. PRODUCT RULE (owner's decision, 2026-07-28): - # a file we were not told to delete is kept, however long it sits - # there. Detaching a plan is one click to undo and the editor says - # the image stays; deleting a space is deliberate but the image is - # usually something the user imported and may not have elsewhere. - # The two errors are not symmetrical — a few unnecessary megabytes - # can always be removed by hand, a file we should not have removed - # cannot be brought back. + # PRODUCT RULE (owner's decision, 2026-07-28): a plan file we were + # not told to delete is kept, however long it sits there. Detaching + # is one click to undo and the editor says the image stays; deleting + # a space is deliberate but the image was imported and may be + # nowhere else. The errors are not symmetrical — unnecessary + # megabytes can be removed by hand, a deleted file cannot be + # brought back. # - # The single exception: a space that HAS a plan, and another file of - # its own that has never been the plan. That can only be an upload - # whose save was rejected, and an hour is plenty for it. - space = item.name.split(".")[0] - if not new_by_space.get(space) or item.name in old_by_space.values(): - continue - try: - if item.stat().st_mtime >= reject_cutoff: - continue - except OSError: - continue + # There is deliberately no age rule here. An earlier version aged + # out "rejected uploads" — a file of a space that has a plan, which + # was never the plan — and that raced a save: the sweep deleted the + # upload from the failed attempt while a retry was committing a + # reference to it. A rule that can delete a file somebody is about + # to point at is not worth the disk it reclaims. + continue try: item.unlink() removed += 1 diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2cffbc99..741dea87 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -225,11 +225,11 @@ was detached, under documentation promising the opposite (HP-1465-01). | Space in both, plan A → plan B | the user picked another image | removed immediately | | Space in both, plan → none | detached; one click undoes it | **kept** | | Space gone | deliberate, but the image was imported and may be nowhere else | **kept** | -| Space has a plan, plus another file of its own | an upload whose save was rejected | `PLAN_ORPHAN_TTL_S` (1 h) | +| Space has a plan, plus another file of its own | an upload whose save was rejected | **kept** — ageing these out raced the retry that referenced them | | Marker in both, attachment dropped from its list | a trash button, promising nothing | removed immediately | | Marker gone | same call as a deleted space's plan | **kept** | | Attachment in `up_*` | a dialog that was never saved; no device owns it | `PLAN_ORPHAN_TTL_S` (1 h) | -| Marker there, file it never listed | a rejected upload | `SCHEDULED_GRACE_S` (30 d) | +| Marker there, file it never listed | a rejected upload | **kept**, same reason | **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 d352b019..71e6abf6 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -14,6 +14,12 @@ - **A plan whose space was deleted is kept**, rather than the thirty days v1.46.5 promised — thirty days measured from the file's age is meaningless anyway, since it was usually uploaded months earlier. +- **Nothing is deleted for being old any more**, except a per-dialog staging + folder. The rule that aged out "rejected uploads" turned out to race a retry: + the cleanup removed the file from a failed save while the next attempt was + committing a reference to it. A rule that can delete a file somebody is about + to point at is not worth the disk it reclaims. Files therefore go when an + action says so, and otherwise stay. ## v1.46.5 — 2026-07-28 (audit of every automatic deletion) - **A detached plan is never deleted, at any age.** v1.46.4 gave it a month; diff --git a/docs/CHANGELOG.ru.md b/docs/CHANGELOG.ru.md index a21603b6..3ef92266 100755 --- a/docs/CHANGELOG.ru.md +++ b/docs/CHANGELOG.ru.md @@ -20,6 +20,12 @@ - **План удалённого пространства сохраняется**, а не тридцать дней, как обещала v1.46.5: тридцать дней по возрасту файла всё равно бессмысленны — обычно он загружен месяцы назад. +- **По возрасту больше не удаляется ничего**, кроме промежуточной папки диалога. + Правило, которое вычищало «отвергнутые загрузки», оказалось в гонке с + повторной попыткой: уборка удаляла файл неудавшегося сохранения ровно тогда, + когда следующая попытка коммитила ссылку на него. Правило, способное удалить + файл, на который кто-то вот-вот сошлётся, не стоит освобождаемого места. + Файлы уходят по действию, в остальных случаях остаются. ## v1.46.5 — 2026-07-28 (ревизия всех автоматических удалений) - **Отцеплённый план не удаляется никогда, ни в каком возрасте.** В v1.46.4 ему diff --git a/tests_backend/test_validation.py b/tests_backend/test_validation.py index 58db416d..d193c030 100644 --- a/tests_backend/test_validation.py +++ b/tests_backend/test_validation.py @@ -570,16 +570,13 @@ def test_plan_collection_matrix(tmp_path): assert collect(d, cfg, cfg) == 0 assert (d / "f2.png").is_file() and (d / "f3.png").is_file() - # 5. a rejected upload: the space HAS a plan, this file never was one + # 5. an upload whose save was rejected is kept too, at any age. Ageing those + # out raced the retry: the sweep deleted a file the save was committing a + # reference to (caught by test_sweep_and_a_config_write_do_not_race). seed("f4.current.png", "f4.reject.png") live = {"spaces": [_sp("f4", "f4.current.png")]} - assert collect(d, live, live) == 1 - assert (d / "f4.current.png").is_file() and not (d / "f4.reject.png").exists() - - # …but only once it is old; a fresh one may be a transaction in flight - (d / "f4.fresh.png").write_bytes(b"x") assert collect(d, live, live) == 0 - assert (d / "f4.fresh.png").is_file() + assert (d / "f4.current.png").is_file() and (d / "f4.reject.png").is_file() # 6. the same file still referenced by another space is never touched seed("shared.png") @@ -634,44 +631,38 @@ def test_attachment_collection_matrix(tmp_path): assert collect(d, cfg(), cfg()) == 1 assert not (d / "up_x").exists() - # 4. an upload into a live device's folder whose save was rejected + # 4. an upload into a live device's folder whose save was rejected: kept, + # for the same reason as a plan's — a retry may be about to reference it d = case("reject") seed(d, "m3", "current.pdf") seed(d, "m3", "rejected.pdf", age=const.SCHEDULED_GRACE_S + 60) live = cfg(("m3", ["current.pdf"])) - assert collect(d, live, live) == 1 + assert collect(d, live, live) == 0 assert (d / "m3" / "current.pdf").is_file() - assert not (d / "m3" / "rejected.pdf").exists() + assert (d / "m3" / "rejected.pdf").is_file() -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). - """ +def test_only_a_staging_folder_ages_out(tmp_path): + """The one age rule left. Everything else waits for the user to say so.""" import os import time files = tmp_path / "files" (files / "m1").mkdir(parents=True) (files / "up_abandoned").mkdir(parents=True) + ancient = time.time() - const.SCHEDULED_GRACE_S * 12 hour_ago = time.time() - const.PLAN_ORPHAN_TTL_S - 60 - 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 / "m1" / "ancient.pdf"), ancient), ((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" / "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 + assert plans.collect_attachments(files, cfg, cfg) == 1 + assert (files / "m1" / "ancient.pdf").is_file(), "age alone is never a reason" + assert not (files / "up_abandoned").exists(), "a cancelled dialog goes after an hour" + + From a66272c6f45a45fe865cb55b152ce3585fcfbdcc Mon Sep 17 00:00:00 2001 From: Matysh Date: Tue, 28 Jul 2026 21:22:33 +0300 Subject: [PATCH 4/4] test: two HA-harness tests still asserted the old age rule One demanded an aged upload be collected; the shared sweep fixture expected an aged plan file to disappear. Both now assert the opposite, which is the rule. --- tests_backend/test_ha_websocket.py | 50 ++++++++++++++++++------------ 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/tests_backend/test_ha_websocket.py b/tests_backend/test_ha_websocket.py index 3cb6fabf..e8bbd0b1 100644 --- a/tests_backend/test_ha_websocket.py +++ b/tests_backend/test_ha_websocket.py @@ -342,35 +342,41 @@ async def test_commit_does_not_collect_another_client_s_uncommitted_upload( assert not (plans / pa).exists() -async def test_abandoned_uploads_are_collected_once_old( +async def test_a_rejected_upload_is_kept_not_aged_out( hass: HomeAssistant, hass_ws_client: WebSocketGenerator ) -> None: - """A rejected upload must not accumulate forever — but only age may free it.""" + """v1.46.6: age is never a reason to delete a plan file. + + It used to be, for "a file of a space that has a plan and never was one" — + an upload whose save had failed. That raced the retry: the sweep removed the + file while the next save was committing a reference to it. Keeping it costs + a few megabytes nobody can lose. + """ import os import time - from pathlib import Path - from custom_components.houseplan.const import PLANS_DIR, PLAN_ORPHAN_TTL_S + from custom_components.houseplan.const import PLANS_DIR, SCHEDULED_GRACE_S + from custom_components.houseplan.store import get_data await _setup(hass) client = await hass_ws_client(hass) - plans = Path(hass.config.path(PLANS_DIR)) - plans.mkdir(parents=True, exist_ok=True) - for stale in plans.glob("r3.*"): - stale.unlink() + plans = hass.config.path(PLANS_DIR) url0, p0 = await _upload(client, "r3", b"zero") rev = (await _save(client, await _cfg([{"id": "r3", "plan_url": url0}]), 0))["result"]["rev"] - _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 + _url, orphan = await _upload(client, "r3", b"never saved") + old = time.time() - SCHEDULED_GRACE_S * 12 + os.utime(os.path.join(plans, orphan), (old, old)) - ok = await _save(client, await _cfg([{"id": "r3", "plan_url": url0}]), rev) - assert ok["success"] - assert not (plans / orphan).exists(), "an aged, unreferenced upload is collected" - assert (plans / p0).is_file(), "the referenced plan is never touched" + # a commit, and the scheduled pass, and any amount of age: it stays + assert (await _save(client, await _cfg([{"id": "r3", "plan_url": url0}]), rev))["success"] + data = get_data(hass) + await data.sweep() + await hass.async_block_till_done() + + assert await hass.async_add_executor_job(os.path.isfile, os.path.join(plans, orphan)) + assert await hass.async_add_executor_job(os.path.isfile, os.path.join(plans, p0)) async def test_collection_ignores_files_that_are_not_plans( @@ -889,7 +895,9 @@ def _paths(hass): "kept_file": os.path.join(files, "m5", "kept.pdf"), "kept_plan": os.path.join(plans, "s5.tok.png"), "orphan_file": os.path.join(files, "up_cancelled", "manual.pdf"), - "orphan_plan": os.path.join(plans, "s5.reject.png"), + # a plan file is never collected by age any more; keep one around and + # assert exactly that + "kept_reject": os.path.join(plans, "s5.reject.png"), } @@ -907,8 +915,12 @@ async def _assert_swept(hass, p) -> None: assert await hass.async_add_executor_job(os.path.isfile, p["kept_file"]), "referenced file kept" assert await hass.async_add_executor_job(os.path.isfile, p["kept_plan"]), "referenced plan kept" - assert not await hass.async_add_executor_job(os.path.isfile, p["orphan_file"]) - assert not await hass.async_add_executor_job(os.path.isfile, p["orphan_plan"]) + assert not await hass.async_add_executor_job(os.path.isfile, p["orphan_file"]), ( + "a staging folder from a dialog nobody saved is the one thing age collects" + ) + assert await hass.async_add_executor_job(os.path.isfile, p["kept_reject"]), ( + "a plan file is never removed for being old" + ) async def test_startup_sweep_collects_what_no_commit_will(