mirror of
https://github.com/Matysh/houseplan-card
synced 2026-07-31 08:28:31 +00:00
fix: stop ageing files out entirely, except staging folders
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.
This commit is contained in:
@@ -23,14 +23,11 @@ MAX_SIGN_PATHS = 200
|
|||||||
# its configuration yet (review R3-1).
|
# its configuration yet (review R3-1).
|
||||||
PLAN_ORPHAN_TTL_S = 3600
|
PLAN_ORPHAN_TTL_S = 3600
|
||||||
|
|
||||||
# The scheduled sweep is a different judgement from a commit's. A commit knows
|
# Kept for compatibility with anything reading it; the collectors no longer use
|
||||||
# it replaced a file; the timer only knows nobody points at one right now — and
|
# a long grace at all. Every attempt to age files out ended badly — first by
|
||||||
# "nobody points at it" is a normal, reversible state. Detaching a plan (switch
|
# deleting detached plans, then by racing the save that was about to reference a
|
||||||
# a space to "draw") leaves the image on disk on purpose, and re-attaching it
|
# retried upload. What is left is deliberately simple: files go when the user's
|
||||||
# later is a thing people do. On 2026-07-28 the hourly rule applied to that case
|
# action says so, plus staging folders after PLAN_ORPHAN_TTL_S.
|
||||||
# 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.
|
|
||||||
SCHEDULED_GRACE_S = 30 * 24 * 3600
|
SCHEDULED_GRACE_S = 30 * 24 * 3600
|
||||||
FILES_DIR = "houseplan/files"
|
FILES_DIR = "houseplan/files"
|
||||||
CONF_ADMIN_ONLY = "admin_only"
|
CONF_ADMIN_ONLY = "admin_only"
|
||||||
|
|||||||
@@ -14,7 +14,7 @@ import time
|
|||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any
|
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
|
from .validation import MAX_FILENAME, PLAN_EXTENSIONS, sanitize_filename
|
||||||
|
|
||||||
_LOGGER = logging.getLogger(__name__)
|
_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
|
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
|
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
|
promises nothing. It goes. Everything else is kept, except a staging folder
|
||||||
transition and the files are kept, like a deleted space's plan. A staging
|
(`up_*`), which by construction only ever holds an upload from a dialog that
|
||||||
folder (`up_*` — only ever a dialog that was never saved) is collected after
|
was never saved: those go after PLAN_ORPHAN_TTL_S. Never raises: it runs
|
||||||
PLAN_ORPHAN_TTL_S, anything else after SCHEDULED_GRACE_S. Never raises: it
|
behind a durable write.
|
||||||
runs behind a 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)
|
||||||
@@ -141,7 +140,6 @@ def collect_attachments(
|
|||||||
# rule is exactly right there even on the timer.
|
# rule is exactly right there even on the timer.
|
||||||
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
|
|
||||||
removed = 0
|
removed = 0
|
||||||
try:
|
try:
|
||||||
folders = sorted(p for p in files_dir.iterdir() if p.is_dir()) if files_dir.is_dir() else []
|
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
|
# 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.
|
# saved — unambiguous, so an hour is right, and no device owns it.
|
||||||
staging = folder.name.startswith("up_")
|
staging = folder.name.startswith("up_")
|
||||||
limit = staging_cutoff if staging 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())
|
||||||
except OSError:
|
except OSError:
|
||||||
@@ -164,15 +161,14 @@ def collect_attachments(
|
|||||||
continue
|
continue
|
||||||
dropped = rel in old_refs and folder.name in live_markers
|
dropped = rel in old_refs and folder.name in live_markers
|
||||||
if not dropped:
|
if not dropped:
|
||||||
if not staging and folder.name not in live_markers:
|
if not staging:
|
||||||
# No device owns this folder any more. Whether the file was
|
# Same rule as for plans: not asked for, so kept. A file in
|
||||||
# attached once or is a leftover upload we cannot tell, and
|
# a device's folder that the device does not list is an
|
||||||
# by the standing rule that means we keep it. (A staging
|
# upload whose save was rejected — and ageing those out
|
||||||
# folder is exempt above: it has no device by construction.)
|
# raced the retry that was about to reference them.
|
||||||
continue
|
continue
|
||||||
# the device is there and never listed this file: a rejected upload
|
|
||||||
try:
|
try:
|
||||||
if item.stat().st_mtime >= limit:
|
if item.stat().st_mtime >= staging_cutoff:
|
||||||
continue
|
continue
|
||||||
except OSError:
|
except OSError:
|
||||||
continue
|
continue
|
||||||
@@ -275,8 +271,6 @@ def collect_plans(
|
|||||||
name for space, name in old_by_space.items()
|
name for space, name in old_by_space.items()
|
||||||
if new_by_space.get(space) and new_by_space[space] != name
|
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
|
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 []
|
||||||
@@ -290,25 +284,20 @@ def collect_plans(
|
|||||||
if not item.is_file() or item.name in new_refs or not is_plan_file(item.name):
|
if not item.is_file() or item.name in new_refs or not is_plan_file(item.name):
|
||||||
continue
|
continue
|
||||||
if item.name not in replaced:
|
if item.name not in replaced:
|
||||||
# Not a replacement. PRODUCT RULE (owner's decision, 2026-07-28):
|
# PRODUCT RULE (owner's decision, 2026-07-28): a plan file we were
|
||||||
# a file we were not told to delete is kept, however long it sits
|
# not told to delete is kept, however long it sits there. Detaching
|
||||||
# there. Detaching a plan is one click to undo and the editor says
|
# is one click to undo and the editor says the image stays; deleting
|
||||||
# the image stays; deleting a space is deliberate but the image is
|
# a space is deliberate but the image was imported and may be
|
||||||
# usually something the user imported and may not have elsewhere.
|
# nowhere else. The errors are not symmetrical — unnecessary
|
||||||
# The two errors are not symmetrical — a few unnecessary megabytes
|
# megabytes can be removed by hand, a deleted file cannot be
|
||||||
# can always be removed by hand, a file we should not have removed
|
# brought back.
|
||||||
# cannot be brought back.
|
|
||||||
#
|
#
|
||||||
# The single exception: a space that HAS a plan, and another file of
|
# There is deliberately no age rule here. An earlier version aged
|
||||||
# its own that has never been the plan. That can only be an upload
|
# out "rejected uploads" — a file of a space that has a plan, which
|
||||||
# whose save was rejected, and an hour is plenty for it.
|
# was never the plan — and that raced a save: the sweep deleted the
|
||||||
space = item.name.split(".")[0]
|
# upload from the failed attempt while a retry was committing a
|
||||||
if not new_by_space.get(space) or item.name in old_by_space.values():
|
# reference to it. A rule that can delete a file somebody is about
|
||||||
continue
|
# to point at is not worth the disk it reclaims.
|
||||||
try:
|
|
||||||
if item.stat().st_mtime >= reject_cutoff:
|
|
||||||
continue
|
|
||||||
except OSError:
|
|
||||||
continue
|
continue
|
||||||
try:
|
try:
|
||||||
item.unlink()
|
item.unlink()
|
||||||
|
|||||||
@@ -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 A → plan B | the user picked another image | removed immediately |
|
||||||
| Space in both, plan → none | detached; one click undoes it | **kept** |
|
| 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 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 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** |
|
| 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) |
|
| 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
|
**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
|
||||||
|
|||||||
@@ -14,6 +14,12 @@
|
|||||||
- **A plan whose space was deleted is kept**, rather than the thirty days
|
- **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
|
v1.46.5 promised — thirty days measured from the file's age is meaningless
|
||||||
anyway, since it was usually uploaded months earlier.
|
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)
|
## 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;
|
- **A detached plan is never deleted, at any age.** v1.46.4 gave it a month;
|
||||||
|
|||||||
@@ -20,6 +20,12 @@
|
|||||||
- **План удалённого пространства сохраняется**, а не тридцать дней, как обещала
|
- **План удалённого пространства сохраняется**, а не тридцать дней, как обещала
|
||||||
v1.46.5: тридцать дней по возрасту файла всё равно бессмысленны — обычно он
|
v1.46.5: тридцать дней по возрасту файла всё равно бессмысленны — обычно он
|
||||||
загружен месяцы назад.
|
загружен месяцы назад.
|
||||||
|
- **По возрасту больше не удаляется ничего**, кроме промежуточной папки диалога.
|
||||||
|
Правило, которое вычищало «отвергнутые загрузки», оказалось в гонке с
|
||||||
|
повторной попыткой: уборка удаляла файл неудавшегося сохранения ровно тогда,
|
||||||
|
когда следующая попытка коммитила ссылку на него. Правило, способное удалить
|
||||||
|
файл, на который кто-то вот-вот сошлётся, не стоит освобождаемого места.
|
||||||
|
Файлы уходят по действию, в остальных случаях остаются.
|
||||||
|
|
||||||
## v1.46.5 — 2026-07-28 (ревизия всех автоматических удалений)
|
## v1.46.5 — 2026-07-28 (ревизия всех автоматических удалений)
|
||||||
- **Отцеплённый план не удаляется никогда, ни в каком возрасте.** В v1.46.4 ему
|
- **Отцеплённый план не удаляется никогда, ни в каком возрасте.** В v1.46.4 ему
|
||||||
|
|||||||
@@ -570,16 +570,13 @@ def test_plan_collection_matrix(tmp_path):
|
|||||||
assert collect(d, cfg, cfg) == 0
|
assert collect(d, cfg, cfg) == 0
|
||||||
assert (d / "f2.png").is_file() and (d / "f3.png").is_file()
|
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")
|
seed("f4.current.png", "f4.reject.png")
|
||||||
live = {"spaces": [_sp("f4", "f4.current.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 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
|
# 6. the same file still referenced by another space is never touched
|
||||||
seed("shared.png")
|
seed("shared.png")
|
||||||
@@ -634,44 +631,38 @@ def test_attachment_collection_matrix(tmp_path):
|
|||||||
assert collect(d, cfg(), cfg()) == 1
|
assert collect(d, cfg(), cfg()) == 1
|
||||||
assert not (d / "up_x").exists()
|
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")
|
d = case("reject")
|
||||||
seed(d, "m3", "current.pdf")
|
seed(d, "m3", "current.pdf")
|
||||||
seed(d, "m3", "rejected.pdf", age=const.SCHEDULED_GRACE_S + 60)
|
seed(d, "m3", "rejected.pdf", age=const.SCHEDULED_GRACE_S + 60)
|
||||||
live = cfg(("m3", ["current.pdf"]))
|
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 (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):
|
def test_only_a_staging_folder_ages_out(tmp_path):
|
||||||
"""A staging folder is unambiguous; a marker folder is not.
|
"""The one age rule left. Everything else waits for the user to say so."""
|
||||||
|
|
||||||
`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)
|
||||||
|
ancient = time.time() - const.SCHEDULED_GRACE_S * 12
|
||||||
hour_ago = time.time() - const.PLAN_ORPHAN_TTL_S - 60
|
hour_ago = time.time() - const.PLAN_ORPHAN_TTL_S - 60
|
||||||
month_ago = time.time() - const.SCHEDULED_GRACE_S - 60
|
|
||||||
|
|
||||||
for path, when in (
|
for path, when in (
|
||||||
((files / "m1" / "recent.pdf"), hour_ago),
|
((files / "m1" / "ancient.pdf"), ancient),
|
||||||
((files / "m1" / "ancient.pdf"), month_ago),
|
|
||||||
((files / "up_abandoned" / "manual.pdf"), hour_ago),
|
((files / "up_abandoned" / "manual.pdf"), hour_ago),
|
||||||
):
|
):
|
||||||
path.write_bytes(b"x")
|
path.write_bytes(b"x")
|
||||||
os.utime(path, (when, when))
|
os.utime(path, (when, when))
|
||||||
|
|
||||||
cfg = {"markers": [{"id": "m1", "pdfs": []}]}
|
cfg = {"markers": [{"id": "m1", "pdfs": []}]}
|
||||||
removed = plans.collect_attachments(files, cfg, cfg)
|
assert plans.collect_attachments(files, cfg, cfg) == 1
|
||||||
assert (files / "m1" / "recent.pdf").is_file(), "an hour is not enough for a marker file"
|
assert (files / "m1" / "ancient.pdf").is_file(), "age alone is never a reason"
|
||||||
assert not (files / "m1" / "ancient.pdf").exists(), "a month is"
|
assert not (files / "up_abandoned").exists(), "a cancelled dialog goes after an hour"
|
||||||
assert not (files / "up_abandoned").exists(), "a cancelled dialog still goes after an hour"
|
|
||||||
assert removed == 2
|
|
||||||
|
|||||||
Reference in New Issue
Block a user