diff --git a/custom_components/houseplan/const.py b/custom_components/houseplan/const.py index d56f3eb..9d9f054 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 58c4b8e..b67c6ca 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 2cffbc9..741dea8 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 d352b01..71e6abf 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 a21603b..3ef9226 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 58db416..d193c03 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" + +