mirror of
https://github.com/Matysh/houseplan-card
synced 2026-07-31 16:38:31 +00:00
fix: the guard has to be per-case, not blanket
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.
This commit is contained in:
@@ -122,17 +122,16 @@ def collect_attachments(
|
|||||||
"""The same commit-scoped rule as `collect_plans`, for marker 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
|
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
|
by this commit and goes. Otherwise a staging folder (`up_*` — only ever a
|
||||||
left alone, a staging folder (`up_*`, only ever a dialog that was never
|
dialog that was never saved) is collected after PLAN_ORPHAN_TTL_S, and
|
||||||
saved) is collected after PLAN_ORPHAN_TTL_S, and anything else waits out
|
anything else waits out SCHEDULED_GRACE_S. Never raises: it runs behind a
|
||||||
SCHEDULED_GRACE_S. Never raises: it runs behind a durable write.
|
durable write.
|
||||||
"""
|
"""
|
||||||
new_refs = attachment_refs(new_cfg)
|
new_refs = attachment_refs(new_cfg)
|
||||||
old_refs = attachment_refs(old_cfg)
|
old_refs = attachment_refs(old_cfg)
|
||||||
# Same distinction as for plans. A staging folder (`up_*`) is different: it
|
# 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
|
# only ever holds an upload from a dialog that was never saved, so the short
|
||||||
# rule is exactly right there even on the timer.
|
# 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
|
now_s = time.time() if now is None else now
|
||||||
staging_cutoff = now_s - PLAN_ORPHAN_TTL_S
|
staging_cutoff = now_s - PLAN_ORPHAN_TTL_S
|
||||||
cutoff = now_s - SCHEDULED_GRACE_S
|
cutoff = now_s - SCHEDULED_GRACE_S
|
||||||
@@ -144,10 +143,10 @@ def collect_attachments(
|
|||||||
return 0
|
return 0
|
||||||
removed += sweep_upload_temps(files_dir, now)
|
removed += sweep_upload_temps(files_dir, now)
|
||||||
for folder in folders:
|
for folder in folders:
|
||||||
if folder.name in live_markers:
|
# A staging folder only ever holds an upload from a dialog that was never
|
||||||
continue # the marker is still there; its files are its own business
|
# saved — unambiguous, so an hour is right. A marker's own folder is not:
|
||||||
# a staging folder only ever holds an upload from a dialog that was
|
# removing an attachment is deliberate, but so is re-adding one, and the
|
||||||
# never saved — unambiguous, so the short rule is right there
|
# file may have been detached rather than abandoned. Give it a month.
|
||||||
limit = staging_cutoff if folder.name.startswith("up_") else cutoff
|
limit = staging_cutoff if folder.name.startswith("up_") else cutoff
|
||||||
try:
|
try:
|
||||||
items = sorted(p for p in folder.iterdir() if p.is_file())
|
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
|
# 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
|
# file stays on disk. So the scheduled pass keeps anything belonging to a
|
||||||
# space that still exists, and waits a month for the rest.
|
# 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 []}
|
# A space with NO plan_url has had its image detached — reversible, and the
|
||||||
cutoff = (time.time() if now is None else now) - SCHEDULED_GRACE_S
|
# 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
|
removed = 0
|
||||||
try:
|
try:
|
||||||
items = sorted(plans_dir.iterdir()) if plans_dir.is_dir() else []
|
items = sorted(plans_dir.iterdir()) if plans_dir.is_dir() else []
|
||||||
@@ -252,8 +259,8 @@ def collect_plans(
|
|||||||
continue
|
continue
|
||||||
superseded = item.name in old_refs
|
superseded = item.name in old_refs
|
||||||
if not superseded:
|
if not superseded:
|
||||||
if item.name.split(".")[0] in live_spaces:
|
if item.name.split(".")[0] in detached:
|
||||||
continue # detached, not abandoned — the space is still there
|
continue # detached, not abandoned — the space is waiting for it
|
||||||
try:
|
try:
|
||||||
if item.stat().st_mtime >= cutoff:
|
if item.stat().st_mtime >= cutoff:
|
||||||
continue
|
continue
|
||||||
|
|||||||
+11
-9
@@ -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
|
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
|
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.
|
write is accepted — the same copy → save → cleanup order as a rebind.
|
||||||
`config/set` collects what its commit superseded — and only that, because
|
`config/set` collects what its commit superseded — that much a commit knows for
|
||||||
supersession is the one thing a commit knows for certain. *Unreferenced* is a
|
certain. *Unreferenced* is a far weaker signal, and how weak depends on the
|
||||||
much weaker signal: detaching a plan is reversible and the editor promises the
|
case. A space with `plan_url = null` had its image **detached**, which is
|
||||||
file stays. So a file belonging to a space or marker that still exists is never
|
reversible and which the editor promises leaves the file alone: those are never
|
||||||
collected, and anything else waits `SCHEDULED_GRACE_S` (30 days). The exception
|
collected. A space that does have a plan can only be holding its own rejected
|
||||||
is a per-dialog staging folder, which by construction only holds an upload from
|
uploads, so `PLAN_ORPHAN_TTL_S` (1 hour) still applies to them. For attachments,
|
||||||
a dialog that was never saved — `PLAN_ORPHAN_TTL_S` (1 hour) is right there.
|
a per-dialog staging folder holds nothing but an upload from a dialog that was
|
||||||
This is not theoretical caution: the one-hour rule applied to unreferenced files
|
never saved — one hour again — while a marker's own folder waits
|
||||||
destroyed two detached plans on 2026-07-28.
|
`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
|
**Config writes are serialized** (HP-1454-03). `_writeConfig()` chains onto a
|
||||||
single promise: one `config/set` in flight, each carrying the revision the
|
single promise: one `config/set` in flight, each carrying the revision the
|
||||||
|
|||||||
+7
-5
@@ -12,11 +12,13 @@
|
|||||||
`config/houseplan/plans/` before updating anything else — and please report it
|
`config/houseplan/plans/` before updating anything else — and please report it
|
||||||
in the Telegram chat if a file is missing.
|
in the Telegram chat if a file is missing.
|
||||||
The rule now: **a commit still removes exactly what it replaced**, because
|
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
|
that it knows for certain. Beyond that the question is whether "unreferenced"
|
||||||
reversible state — a file belonging to a space or a device that still exists
|
means "abandoned", and the answer depends on the case. A space with no plan at
|
||||||
is never collected, and anything else waits thirty days instead of an hour.
|
all has had one detached and may want it back — its files are never collected.
|
||||||
The one unambiguous case keeps the short rule: a per-dialog staging folder
|
A space that does have a plan can only be holding rejected uploads of its own,
|
||||||
only ever holds an upload from a dialog that was never saved.
|
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)
|
## 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
|
- **The cleanup at startup now actually cleans up.** It looked its own runtime
|
||||||
|
|||||||
@@ -17,11 +17,14 @@
|
|||||||
план после v1.46.0 и инстанс перезапускался или проработал сутки — загляните в
|
план после v1.46.0 и инстанс перезапускался или проработал сутки — загляните в
|
||||||
`config/houseplan/plans/` и напишите в Telegram-чат, если файла нет.
|
`config/houseplan/plans/` и напишите в Telegram-чат, если файла нет.
|
||||||
Правило теперь такое: **коммит по-прежнему удаляет ровно то, что заменил**, —
|
Правило теперь такое: **коммит по-прежнему удаляет ровно то, что заменил**, —
|
||||||
это он знает наверняка. Всё остальное лишь «непривязано», а это обратимое
|
это он знает наверняка. Дальше вопрос в том, означает ли «непривязан»
|
||||||
состояние: файл, принадлежащий существующему пространству или устройству, не
|
«брошен», и ответ зависит от случая. У пространства, у которого плана нет
|
||||||
удаляется никогда, а всё прочее ждёт тридцать дней вместо часа. Один
|
вовсе, его отцепили — и, возможно, вернут: его файлы не удаляются никогда. У
|
||||||
однозначный случай сохраняет короткое правило: промежуточная папка диалога
|
пространства, у которого план есть, лишние файлы могут быть только его же
|
||||||
содержит только загрузку из диалога, который так и не сохранили.
|
отвергнутыми загрузками — они по-прежнему уходят через час. Вложения вне
|
||||||
|
промежуточной папки диалога ждут месяц; сама промежуточная папка, где по
|
||||||
|
построению лежит только загрузка из несохранённого диалога, сохраняет часовое
|
||||||
|
правило.
|
||||||
|
|
||||||
## v1.46.3 — 2026-07-28 (перепроверка v1.46.2: HP-1462-01)
|
## v1.46.3 — 2026-07-28 (перепроверка v1.46.2: HP-1462-01)
|
||||||
- **Уборка при старте действительно убирает.** Она искала свои же runtime-данные
|
- **Уборка при старте действительно убирает.** Она искала свои же runtime-данные
|
||||||
|
|||||||
@@ -363,6 +363,7 @@ async def test_abandoned_uploads_are_collected_once_old(
|
|||||||
_url, orphan = await _upload(client, "r3", b"abandoned")
|
_url, orphan = await _upload(client, "r3", b"abandoned")
|
||||||
old = time.time() - PLAN_ORPHAN_TTL_S - 60
|
old = time.time() - PLAN_ORPHAN_TTL_S - 60
|
||||||
os.utime(plans / orphan, (old, old))
|
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)
|
ok = await _save(client, await _cfg([{"id": "r3", "plan_url": url0}]), rev)
|
||||||
assert ok["success"]
|
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"
|
assert not bad["success"] and bad["error"]["code"] == "conflict"
|
||||||
|
|
||||||
await hass.async_block_till_done()
|
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(
|
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
|
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:
|
def _do() -> None:
|
||||||
for path in names:
|
for path in names:
|
||||||
|
|||||||
@@ -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"
|
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 os
|
||||||
import time
|
import time
|
||||||
|
|
||||||
files = tmp_path / "files"
|
files = tmp_path / "files"
|
||||||
(files / "m1").mkdir(parents=True)
|
(files / "m1").mkdir(parents=True)
|
||||||
(files / "up_abandoned").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
|
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": []}]}
|
cfg = {"markers": [{"id": "m1", "pdfs": []}]}
|
||||||
removed = plans.collect_attachments(files, cfg, cfg)
|
removed = plans.collect_attachments(files, cfg, cfg)
|
||||||
assert (files / "m1" / "unlinked.pdf").is_file(), "the marker exists; leave its files alone"
|
assert (files / "m1" / "recent.pdf").is_file(), "an hour is not enough for a marker file"
|
||||||
assert not staging.exists(), "a cancelled dialog is still collected after an hour"
|
assert not (files / "m1" / "ancient.pdf").exists(), "a month is"
|
||||||
assert not (files / "dead" / "x.pdf").exists(), "a marker that is gone ages out"
|
assert not (files / "up_abandoned").exists(), "a cancelled dialog still goes after an hour"
|
||||||
assert removed == 2
|
assert removed == 2
|
||||||
|
|||||||
Reference in New Issue
Block a user