diff --git a/tests/test_make_room_for.py b/tests/test_make_room_for.py index ba34efe..41922d3 100644 --- a/tests/test_make_room_for.py +++ b/tests/test_make_room_for.py @@ -135,7 +135,7 @@ def test_make_room_evicts_until_under_then_true(env, monkeypatch): _patch_quota(monkeypatch, used_bytes=2 * (1 << 30)) monkeypatch.setattr( "web.services.retention._delete_clip_files", - lambda row, recordings: 1 << 30, + lambda row, recordings: (1 << 30, True), ) ok = ret.make_room_for( db, str(rec), size=0, before_ts=300, @@ -190,7 +190,7 @@ def test_make_room_for_skips_locked_clip_protect_ro_false(env, monkeypatch): _patch_quota(monkeypatch, used_bytes=2 * (1 << 30)) monkeypatch.setattr( "web.services.retention._delete_clip_files", - lambda row, recordings: 1 << 30, + lambda row, recordings: (1 << 30, True), ) ok = ret.make_room_for( diff --git a/tests/test_queue_delete.py b/tests/test_queue_delete.py index de12a6d..3cf91f9 100644 --- a/tests/test_queue_delete.py +++ b/tests/test_queue_delete.py @@ -1,6 +1,7 @@ """queue.delete_clips: local file + index delete, mark download_queue skipped.""" from __future__ import annotations +import os import time from pathlib import Path @@ -49,7 +50,8 @@ def test_delete_removes_files_index_and_marks_skipped(tmp_path): rec, db = _env(tmp_path) cid, path = _make_clip(rec, db, basename="A.MP4") res = delete_clips(db, ["A.MP4"], str(rec)) - assert res == {"deleted": 1, "skipped": 1, "protected": 0} + assert res == {"deleted": 1, "skipped": 1, "protected": 0, + "protected_names": [], "failed": 0} assert not path.exists() assert not (path.parent / "A.MP4.gpx").exists() with db.conn() as c: @@ -76,7 +78,8 @@ def test_delete_nonexistent_file_reports_zero(tmp_path): from web.services.queue import delete_clips rec, db = _env(tmp_path) res = delete_clips(db, ["GHOST.MP4"], str(rec)) - assert res == {"deleted": 0, "skipped": 0, "protected": 0} + assert res == {"deleted": 0, "skipped": 0, "protected": 0, + "protected_names": [], "failed": 0} def test_delete_skips_locked_clip(tmp_path): @@ -97,7 +100,8 @@ def test_delete_undownloaded_marks_skipped_only(tmp_path): with db.write() as c: _insert_queue(c, "PEND.MP4", "pending") res = delete_clips(db, ["PEND.MP4"], str(rec)) - assert res == {"deleted": 0, "skipped": 1, "protected": 0} + assert res == {"deleted": 0, "skipped": 1, "protected": 0, + "protected_names": [], "failed": 0} with db.conn() as c: assert c.execute( "SELECT state FROM download_queue WHERE filename='PEND.MP4'" @@ -116,7 +120,9 @@ def test_delete_missing_file_does_not_raise(tmp_path): def test_delete_empty_is_noop(tmp_path): from web.services.queue import delete_clips rec, db = _env(tmp_path) - assert delete_clips(db, [], str(rec)) == {"deleted": 0, "skipped": 0, "protected": 0} + assert delete_clips(db, [], str(rec)) == { + "deleted": 0, "skipped": 0, "protected": 0, + "protected_names": [], "failed": 0} def test_delete_logs_info_audit_line(tmp_path, caplog): @@ -141,6 +147,60 @@ def test_delete_noop_does_not_log(tmp_path, caplog): assert not [r for r in caplog.records if r.name == "viofosync.queue"] +def test_delete_failed_unlink_keeps_rows_and_reports_failed(tmp_path, monkeypatch): + # A file the container can't unlink (classic Unraid PUID/permissions + # mismatch) must NOT count as deleted: the index row stays, the queue row + # stays untouched, and the response says 'failed'. Dropping the row while + # the .mp4 survives would free nothing and resurrect the clip on rescan. + from web.services.queue import delete_clips + rec, db = _env(tmp_path) + _, path = _make_clip(rec, db, basename="STUCK.MP4") + real_remove = os.remove + + def deny_mp4(p, *a, **kw): + if str(p) == str(path): + raise PermissionError(1, "Operation not permitted", str(p)) + return real_remove(p, *a, **kw) + + monkeypatch.setattr("web.services.retention.os.remove", deny_mp4) + res = delete_clips(db, ["STUCK.MP4"], str(rec)) + assert res["deleted"] == 0 and res["failed"] == 1 + assert path.exists() + with db.conn() as c: + assert c.execute("SELECT COUNT(*) AS n FROM clip_index").fetchone()["n"] == 1 + row = c.execute( + "SELECT state FROM download_queue WHERE filename='STUCK.MP4'" + ).fetchone() + assert row["state"] == "done" # not marked skipped — nothing went away + + +def test_delete_force_removes_protected(tmp_path): + # force=True is the confirm-through path: the UI has already shown a + # second "delete anyway?" dialog, so RO and user-locked clips go too. + from web.services.queue import delete_clips + rec, db = _env(tmp_path) + _, ro_path = _make_clip(rec, db, basename="RO.MP4", event_type="ro") + _, lk_path = _make_clip(rec, db, basename="LOCKED.MP4") + with db.write() as c: + c.execute("UPDATE clip_index SET locked=1 WHERE basename='LOCKED.MP4'") + res = delete_clips(db, ["RO.MP4", "LOCKED.MP4"], str(rec), force=True) + assert res["deleted"] == 2 and res["protected"] == 0 and res["failed"] == 0 + assert not ro_path.exists() and not lk_path.exists() + with db.conn() as c: + assert c.execute("SELECT COUNT(*) AS n FROM clip_index").fetchone()["n"] == 0 + + +def test_delete_reports_protected_names(tmp_path): + # The UI force-deletes exactly these names after its second confirm. + from web.services.queue import delete_clips + rec, db = _env(tmp_path) + _make_clip(rec, db, basename="RO.MP4", event_type="ro") + _, ok_path = _make_clip(rec, db, basename="OK.MP4") + res = delete_clips(db, ["RO.MP4", "OK.MP4"], str(rec)) + assert res["deleted"] == 1 and not ok_path.exists() + assert res["protected"] == 1 and res["protected_names"] == ["RO.MP4"] + + @pytest.fixture def authed_client(tmp_config_dir: Path, tmp_recordings_dir: Path, monkeypatch): from web import app as app_mod @@ -192,3 +252,31 @@ def test_delete_endpoint_empty_body(authed_client): r = authed_client.post("/api/queue/delete", json={}) assert r.status_code == 200 assert r.json()["deleted"] == 0 and r.json()["skipped"] == 0 + + +def test_delete_endpoint_force_confirm_through(authed_client, tmp_recordings_dir: Path): + # First call refuses the RO clip and names it; the forced follow-up + # (the UI's "delete anyway?" path) removes it. + db = authed_client.app.state.db + folder = tmp_recordings_dir / "2026-06-26" + folder.mkdir(exist_ok=True) + path = folder / "RO.MP4" + path.write_bytes(b"x" * 128) + with db.write() as c: + c.execute( + "INSERT INTO clip_index " + "(path, basename, group_name, timestamp, camera, sequence, " + " event_type, size_bytes, has_gpx, scanned_at) " + "VALUES (?, 'RO.MP4', '2026-06-26', 1, 'F', 1, 'ro', 128, 0, 1)", + (str(path),), + ) + r = authed_client.post("/api/queue/delete", json={"filenames": ["RO.MP4"]}) + body = r.json() + assert body["deleted"] == 0 and body["protected"] == 1 + assert body["protected_names"] == ["RO.MP4"] + assert path.exists() + r = authed_client.post( + "/api/queue/delete", json={"filenames": ["RO.MP4"], "force": True}) + body = r.json() + assert body["deleted"] == 1 and body["protected"] == 0 + assert not path.exists() diff --git a/tests/test_queue_lock.py b/tests/test_queue_lock.py index 3829a8f..306bb85 100644 --- a/tests/test_queue_lock.py +++ b/tests/test_queue_lock.py @@ -125,3 +125,26 @@ def test_lock_endpoint(authed_client): assert c.execute( "SELECT locked FROM download_queue WHERE filename='B.MP4'" ).fetchone()["locked"] == 1 + + +def test_unlock_endpoint(authed_client): + # /queue/unlock is the reverse of /queue/lock — before it existed a + # mark-read-only was permanent short of sqlite3 surgery on the DB. + db = authed_client.app.state.db + with db.write() as c: + _insert_queue(c, "U.MP4", "done") + _insert_index(c, "U.MP4") + + authed_client.post("/api/queue/lock", json={"filenames": ["U.MP4"]}) + r = authed_client.post("/api/queue/unlock", json={"filenames": ["U.MP4"]}) + assert r.status_code == 200 + body = r.json() + assert body["ok"] is True and body["updated"] == 1 + + with db.conn() as c: + assert c.execute( + "SELECT locked FROM clip_index WHERE basename='U.MP4'" + ).fetchone()["locked"] == 0 + assert c.execute( + "SELECT locked FROM download_queue WHERE filename='U.MP4'" + ).fetchone()["locked"] == 0 diff --git a/tests/test_retention.py b/tests/test_retention.py index 2ec310f..84e63bf 100644 --- a/tests/test_retention.py +++ b/tests/test_retention.py @@ -238,7 +238,7 @@ def _patch_quota_scanner(monkeypatch, half_gib: int) -> None: orig_del = ret._delete_clip_files def del_returning(*a, **kw): orig_del(*a, **kw) - return half_gib + return (half_gib, True) monkeypatch.setattr(ret, "_delete_clip_files", del_returning) diff --git a/tests/test_retention_export_guard.py b/tests/test_retention_export_guard.py index 8659647..0f8f4b0 100644 --- a/tests/test_retention_export_guard.py +++ b/tests/test_retention_export_guard.py @@ -158,7 +158,7 @@ def test_make_room_for_skips_protected_clips(env, monkeypatch): def _del(*a, **kw): orig_del(*a, **kw) - return int(0.6 * gib) + return (int(0.6 * gib), True) monkeypatch.setattr(ret, "_delete_clip_files", _del) ok = ret.make_room_for( diff --git a/tests/test_retention_failed_unlink.py b/tests/test_retention_failed_unlink.py new file mode 100644 index 0000000..b89ba79 --- /dev/null +++ b/tests/test_retention_failed_unlink.py @@ -0,0 +1,127 @@ +"""Failed unlinks must not drop index rows, credit freed bytes, or wedge +the retention loops. + +Regression tests for the Unraid permissions failure mode: the container +user can't unlink in the recordings share, so os.remove raises +PermissionError. Before the fix, delete_clip removed the clip_index row +anyway (the clip vanished from the UI, no space freed, and it returned on +the next rescan), and the disk-pressure pass credited the file's size as +freed while looping over the same un-unlinkable candidates. +""" +from __future__ import annotations + +import collections +import os +from pathlib import Path + +import web.services.retention as ret + + +def _env(tmp_path: Path): + from web.db import Database + rec = tmp_path / "rec" + rec.mkdir() + db = Database(str(rec / "v.db")) + return rec, db + + +def _make_clip(rec: Path, db, *, basename: str, ts: int): + folder = rec / "2026-06-26" + folder.mkdir(exist_ok=True) + path = folder / basename + path.write_bytes(b"x" * 1024) + with db.write() as c: + cur = c.execute( + "INSERT INTO clip_index " + "(path, basename, group_name, timestamp, camera, sequence, " + " event_type, size_bytes, has_gpx, scanned_at) " + "VALUES (?, ?, '2026-06-26', ?, 'F', 1, 'normal', 1024, 0, 1)", + (str(path), basename, ts), + ) + return cur.lastrowid, path + + +def _deny_mp4(monkeypatch): + """Patch retention's os.remove so every .MP4 unlink raises the classic + share-permissions error; sidecars/caches still remove normally.""" + real_remove = os.remove + + def deny(p, *a, **kw): + if str(p).endswith(".MP4"): + raise PermissionError(1, "Operation not permitted", str(p)) + return real_remove(p, *a, **kw) + + monkeypatch.setattr("web.services.retention.os.remove", deny) + + +def _index_count(db) -> int: + with db.conn() as c: + return c.execute("SELECT COUNT(*) AS n FROM clip_index").fetchone()["n"] + + +def test_delete_clip_keeps_index_row_on_failed_unlink(tmp_path, monkeypatch): + rec, db = _env(tmp_path) + cid, path = _make_clip(rec, db, basename="A.MP4", ts=100) + _deny_mp4(monkeypatch) + freed, ok = ret.delete_clip(db, {"id": cid, "path": str(path)}, str(rec)) + assert ok is False + assert freed == 0 # nothing was actually reclaimed + assert path.exists() + assert _index_count(db) == 1 # row survives → UI stays truthful + + +def test_sweep_time_rule_failed_unlink_keeps_row(tmp_path, monkeypatch): + rec, db = _env(tmp_path) + _, path = _make_clip(rec, db, basename="OLD.MP4", ts=0) + _deny_mp4(monkeypatch) + summary = ret.sweep( + db, str(rec), max_days=1, disk_pct=0, protect_ro=True, + _now=86400 * 30, + ) + assert summary["deleted_time"] == 0 + assert summary["failed"] == 1 + assert summary["bytes_freed"] == 0 + assert path.exists() + assert _index_count(db) == 1 + + +def test_disk_pressure_pass_skips_failed_and_terminates(tmp_path, monkeypatch): + # Disk usage is pinned over threshold, and every unlink fails — the + # pass must still terminate (failed ids leave the candidate pool) + # with nothing deleted and nothing credited as freed. + rec, db = _env(tmp_path) + _make_clip(rec, db, basename="A.MP4", ts=100) + _make_clip(rec, db, basename="B.MP4", ts=200) + + DU = collections.namedtuple("DU", "total used free") + monkeypatch.setattr( + "web.services.retention.shutil.disk_usage", + lambda p: DU(total=100, used=95, free=5), + ) + _deny_mp4(monkeypatch) + deleted, bytes_freed, protected, failed = ret._disk_pressure_pass( + db, str(rec), disk_pct=80, quota_gb=0, protect_ro=True, sink=None, + ) + assert deleted == 0 + assert bytes_freed == 0 + assert failed == 2 + assert _index_count(db) == 2 + + +def test_make_room_for_failed_unlink_terminates_false(tmp_path, monkeypatch): + # Same wedge-guard for the import path: with every candidate + # un-unlinkable and the quota permanently breached, make_room_for + # must exhaust the pool and report False rather than spin. + rec, db = _env(tmp_path) + _make_clip(rec, db, basename="A.MP4", ts=100) + monkeypatch.setattr( + ret, "_scan_dir_bytes", + lambda p, exclude=frozenset(): 2 * (1 << 30), + ) + _deny_mp4(monkeypatch) + ok = ret.make_room_for( + db, str(rec), size=0, before_ts=300, + disk_pct=0, quota_gb=1, protect_ro=True, + ) + assert ok is False + assert _index_count(db) == 1 diff --git a/web/app.py b/web/app.py index 1331143..230ebca 100644 --- a/web/app.py +++ b/web/app.py @@ -18,6 +18,7 @@ from contextlib import asynccontextmanager, suppress from fastapi import FastAPI +from fastapi.middleware.gzip import GZipMiddleware from fastapi.responses import FileResponse, Response from fastapi.staticfiles import StaticFiles @@ -406,6 +407,10 @@ def create_app() -> FastAPI: app.state.log_handler = log_handler app.add_middleware(SetupModeMiddleware) + # Compress the SPA (app.js alone is ~200 KB) and the JSON day payloads + # (~80 KB for a 240-clip day, re-fetched on every re-render). ~5-8x + # smaller on the wire; minimum_size skips tiny responses and streams. + app.add_middleware(GZipMiddleware, minimum_size=1024) app.include_router(auth_router.router) app.include_router(archive_router.router) diff --git a/web/routers/archive.py b/web/routers/archive.py index 3fdba15..be6f6f4 100644 --- a/web/routers/archive.py +++ b/web/routers/archive.py @@ -1027,6 +1027,11 @@ async def geocode( def _fetch_clip(request: Request, clip_id: int) -> dict: + # Blocking: sqlite connect + an isfile() that can stall for hundreds + # of ms on a spun-down array. Async handlers must call this via + # asyncio.to_thread or they stall the event loop (and with it every + # other request and the progress WebSocket) — a day view issues ~240 + # of these back-to-back. with _db(request).conn() as c: row = c.execute( "SELECT id, path, basename, size_bytes, duration_s " @@ -1042,7 +1047,7 @@ def _fetch_clip(request: Request, clip_id: int) -> dict: @router.get("/clip/{clip_id}/thumb") async def clip_thumb(request: Request, clip_id: int): - clip = _fetch_clip(request, clip_id) + clip = await asyncio.to_thread(_fetch_clip, request, clip_id) s = _settings(request) path = await thumbs.ensure_thumb( s.recordings, clip_id, clip["path"] @@ -1064,7 +1069,7 @@ async def clip_filmstrip(request: Request, clip_id: int): """Slicing metadata for the clip's filmstrip sprite (generates it on demand). 204 when ffmpeg is unavailable so the UI shows placeholder tiles.""" - clip = _fetch_clip(request, clip_id) + clip = await asyncio.to_thread(_fetch_clip, request, clip_id) s = _settings(request) meta = await filmstrip.ensure_filmstrip( s.recordings, clip_id, clip["path"], clip.get("duration_s") @@ -1083,7 +1088,7 @@ async def clip_filmstrip(request: Request, clip_id: int): @router.get("/clip/{clip_id}/filmstrip.jpg") async def clip_filmstrip_jpg(request: Request, clip_id: int): - clip = _fetch_clip(request, clip_id) + clip = await asyncio.to_thread(_fetch_clip, request, clip_id) s = _settings(request) meta = await filmstrip.ensure_filmstrip( s.recordings, clip_id, clip["path"], clip.get("duration_s") diff --git a/web/routers/queue.py b/web/routers/queue.py index 66f8620..5ab7e3c 100644 --- a/web/routers/queue.py +++ b/web/routers/queue.py @@ -162,6 +162,9 @@ class Lock(BaseModel): class DeleteClips(BaseModel): filenames: List[str] = Field(default_factory=list) + # Confirm-through: the UI sends force=True only after a second dialog + # that explicitly names the protected (read-only / locked) clips. + force: bool = False class DeleteFromCamera(BaseModel): @@ -182,7 +185,9 @@ def delete_from_camera_route(body: DeleteFromCamera, request: Request) -> dict: @router.post("/queue/delete", dependencies=[Depends(require_csrf)]) def delete_clips(body: DeleteClips, request: Request) -> dict: recordings = request.app.state.settings_provider.get().recordings - res = q.delete_clips(request.app.state.db, body.filenames, recordings) + res = q.delete_clips( + request.app.state.db, body.filenames, recordings, force=body.force, + ) q.emit_queue_changed(request.app.state.db, request.app.state.hub) return {"ok": True, **res} @@ -205,6 +210,16 @@ def lock(body: Lock, request: Request) -> dict: return {"ok": True, "updated": n} +@router.post("/queue/unlock", dependencies=[Depends(require_csrf)]) +def unlock(body: Lock, request: Request) -> dict: + """Clear the user 'retain indefinitely' flag — the reverse of /queue/lock. + Dashcam-locked clips (event_type='ro') keep that provenance; deleting those + goes through the delete route's force flag instead.""" + n = q.set_locked(request.app.state.db, body.filenames, False) + q.emit_queue_changed(request.app.state.db, request.app.state.hub) + return {"ok": True, "updated": n} + + @router.get("/sync/status") def sync_status(request: Request) -> dict: worker = getattr(request.app.state, "sync_worker", None) diff --git a/web/services/filmstrip.py b/web/services/filmstrip.py index e6b795c..3c5200b 100644 --- a/web/services/filmstrip.py +++ b/web/services/filmstrip.py @@ -44,9 +44,16 @@ class FilmstripMeta: duration_s: float +# Same makedirs memo as thumbs.py — sprite_path/meta_path are called per +# request and from retention; don't pay a NAS metadata round-trip each time. +_ensured_dirs: set[str] = set() + + def _cache_dir(recordings: str) -> str: d = os.path.join(recordings, ".filmstrips") - os.makedirs(d, exist_ok=True) + if d not in _ensured_dirs: + os.makedirs(d, exist_ok=True) + _ensured_dirs.add(d) return d diff --git a/web/services/queue.py b/web/services/queue.py index c3dfcb1..0d50a76 100644 --- a/web/services/queue.py +++ b/web/services/queue.py @@ -21,6 +21,7 @@ from __future__ import annotations import logging +import os import time from dataclasses import dataclass from typing import Iterable, List, Optional, Sequence @@ -847,30 +848,42 @@ def skip(db: Database, filenames: List[str]) -> int: return cur.rowcount -def delete_clips(db: Database, filenames: List[str], recordings: str) -> dict: +def delete_clips( + db: Database, filenames: List[str], recordings: str, *, force: bool = False, +) -> dict: """User-initiated delete: remove downloaded files + clip_index rows and mark the queue rows skipped. Clips the user has pinned read-only (clip_index or - download_queue locked=1) or dashcam-locked (event_type='ro') are skipped and - reported as 'protected'. Returns {deleted, skipped, protected}.""" + download_queue locked=1) or dashcam-locked (event_type='ro') are refused and + reported as 'protected' — unless ``force`` is True, the confirm-through + path: the UI has already shown a second "delete anyway?" dialog covering + exactly those clips. A clip whose file could not actually be unlinked + (share permissions, immutable bit) keeps BOTH its index and queue rows and + is reported as 'failed', never 'deleted' — otherwise the tile disappears, + no space is freed, and the clip resurfaces on the next rescan. + Returns {deleted, skipped, protected, failed}.""" if not filenames: - return {"deleted": 0, "skipped": 0, "protected": 0} + return {"deleted": 0, "skipped": 0, "protected": 0, + "protected_names": [], "failed": 0} from . import retention as _retention - ph = ",".join("?" * len(filenames)) - with db.conn() as c: - protected = { - r["name"] for r in c.execute( - f"SELECT basename AS name FROM clip_index " - f"WHERE basename IN ({ph}) " - f"AND (COALESCE(locked,0)=1 OR COALESCE(event_type,'')='ro') " - f"UNION " - f"SELECT filename AS name FROM download_queue " - f"WHERE filename IN ({ph}) AND COALESCE(locked,0)=1", - [*filenames, *filenames], - ).fetchall() - } + protected: set = set() + if not force: + ph = ",".join("?" * len(filenames)) + with db.conn() as c: + protected = { + r["name"] for r in c.execute( + f"SELECT basename AS name FROM clip_index " + f"WHERE basename IN ({ph}) " + f"AND (COALESCE(locked,0)=1 OR COALESCE(event_type,'')='ro') " + f"UNION " + f"SELECT filename AS name FROM download_queue " + f"WHERE filename IN ({ph}) AND COALESCE(locked,0)=1", + [*filenames, *filenames], + ).fetchall() + } targets = [f for f in filenames if f not in protected] deleted = 0 skipped = 0 + failed: set = set() if targets: tph = ",".join("?" * len(targets)) with db.conn() as c: @@ -878,19 +891,34 @@ def delete_clips(db: Database, filenames: List[str], recordings: str) -> dict: f"SELECT id, path, basename, event_type FROM clip_index " f"WHERE basename IN ({tph})", targets, ).fetchall() + pruned = set() for r in rows: - _retention.delete_clip(db, dict(r), recordings) - deleted += 1 - with db.write() as c: - cur = c.execute( - f"UPDATE download_queue SET state='skipped', skip_reason='user' " - f"WHERE filename IN ({tph})", targets, - ) - skipped = cur.rowcount # rows actually marked, not len(targets) - if deleted or skipped or protected: - log.info("archive delete: removed %d clip(s), %d protected — %s", - deleted, len(protected), _names(filenames)) - return {"deleted": deleted, "skipped": skipped, "protected": len(protected)} + _, ok = _retention.delete_clip(db, dict(r), recordings, prune=False) + if ok: + deleted += 1 + pruned.add(os.path.dirname(r["path"])) + else: + failed.add(r["basename"]) + _retention.prune_empty_dirs(pruned) + # Queue rows only for clips that actually went away (or were never + # downloaded) — a failed unlink keeps its row so state stays truthful. + mark = [f for f in targets if f not in failed] + if mark: + mph = ",".join("?" * len(mark)) + with db.write() as c: + cur = c.execute( + f"UPDATE download_queue SET state='skipped', skip_reason='user' " + f"WHERE filename IN ({mph})", mark, + ) + skipped = cur.rowcount # rows actually marked, not len(mark) + if deleted or skipped or protected or failed: + log.info("archive delete: removed %d clip(s), %d protected, %d failed — %s", + deleted, len(protected), len(failed), _names(filenames)) + # protected_names lets the UI force-delete exactly the refused clips + # after its second "delete anyway?" confirmation. + return {"deleted": deleted, "skipped": skipped, + "protected": len(protected), "protected_names": sorted(protected), + "failed": len(failed)} def unskip(db: Database, filenames: List[str]) -> int: diff --git a/web/services/retention.py b/web/services/retention.py index db02c6b..8c95986 100644 --- a/web/services/retention.py +++ b/web/services/retention.py @@ -52,16 +52,26 @@ def _eligible_by_time( return False, "kept" -def _delete_clip_files(rec: dict, recordings: str) -> int: - """Delete the .mp4, .gpx sidecar, and cached thumb for one - clip. Returns the number of bytes freed (best-effort; 0 on - failure).""" +def _delete_clip_files(rec: dict, recordings: str) -> tuple[int, bool]: + """Delete the .mp4, .gpx sidecar, and cached thumb/sprite/meta for + one clip. Returns ``(bytes_freed, ok)``. ``ok`` is False when the + primary recording itself could not be unlinked (typically share + permissions) — sidecar/cache failures don't clear it. On failure + ``bytes_freed`` reports 0 so quota bookkeeping never credits space + that is still occupied. Callers must keep the index row when ``ok`` + is False, or the clip silently reappears on the next rescan while + its file keeps holding disk. + + Day-folder pruning is NOT done here — callers collect + ``os.path.dirname(rec['path'])`` and call :func:`prune_empty_dirs` + once per batch, so a NAS doesn't pay a failing rmdir per clip.""" freed = 0 path = rec["path"] try: freed = os.path.getsize(path) except OSError: freed = 0 + ok = True for p in ( path, path + ".gpx", @@ -73,15 +83,22 @@ def _delete_clip_files(rec: dict, recordings: str) -> int: os.remove(p) except FileNotFoundError: pass - except OSError as e: # pragma: no cover — best-effort + except OSError as e: log.warning("retention: could not remove %s: %s", p, e) - # Best-effort prune of an empty group folder. - parent = os.path.dirname(path) - try: - os.rmdir(parent) - except OSError: - pass - return freed + if p == path: + ok = False + return (freed if ok else 0, ok) + + +def prune_empty_dirs(dirs) -> None: + """Best-effort rmdir of day/group folders that may now be empty. + Almost always fails with ENOTEMPTY, which is fine — batching means + that syscall is paid once per folder, not once per deleted clip.""" + for d in set(dirs): + try: + os.rmdir(d) + except OSError: + pass def _delete_index_row(db: Database, clip_id: int) -> None: @@ -90,13 +107,22 @@ def _delete_index_row(db: Database, clip_id: int) -> None: c.execute("DELETE FROM clip_index WHERE id = ?", (clip_id,)) -def delete_clip(db: Database, rec: dict, recordings: str) -> int: +def delete_clip( + db: Database, rec: dict, recordings: str, *, prune: bool = True, +) -> tuple[int, bool]: """Delete one downloaded clip's files (mp4 + .gpx + thumb/sprite/meta) and its - clip_index row. Returns bytes freed. Public wrapper over the same plumbing the - retention sweep uses, for the user-initiated archive delete.""" - freed = _delete_clip_files(rec, recordings) - _delete_index_row(db, rec["id"]) - return freed + clip_index row. Returns ``(bytes_freed, ok)``. When the primary file could + not be unlinked the index row is KEPT and ``ok`` is False — dropping the row + while the .mp4 survives would report success, free nothing, and resurrect + the clip on the next rescan. Public wrapper over the same plumbing the + retention sweep uses, for the user-initiated archive delete; batch callers + pass ``prune=False`` and call :func:`prune_empty_dirs` once themselves.""" + freed, ok = _delete_clip_files(rec, recordings) + if ok: + _delete_index_row(db, rec["id"]) + if prune: + prune_empty_dirs((os.path.dirname(rec["path"]),)) + return freed, ok def _broadcast(sink, filename: str, reason: str) -> None: @@ -130,6 +156,7 @@ def sweep( now = _now if _now is not None else int(_time.time()) deleted_time = 0 protected = 0 + failed = 0 bytes_freed = 0 # Phase 1: time-based. ``protect_ids`` (clips referenced by @@ -154,6 +181,7 @@ def sweep( "— examining", len(rows), max_days, ) + pruned: set[str] = set() for row in rows: ok, reason = _eligible_by_time( row, now=now, max_days=max_days, protect_ro=protect_ro, @@ -162,8 +190,13 @@ def sweep( if reason == "ro_protected": protected += 1 continue - bytes_freed += _delete_clip_files(row, recordings) + freed, removed = _delete_clip_files(row, recordings) + if not removed: + failed += 1 + continue + bytes_freed += freed _delete_index_row(db, row["id"]) + pruned.add(os.path.dirname(row["path"])) deleted_time += 1 _broadcast(sink, row["basename"], "time") if deleted_time % 10 == 0: @@ -172,12 +205,13 @@ def sweep( "(%.1f MB freed so far)", deleted_time, len(rows), bytes_freed / (1 << 20), ) + prune_empty_dirs(pruned) # Phase 2: disk-pressure. Two independent triggers, either can # be set on its own; both is fine and uses OR semantics. deleted_disk = 0 if disk_pct > 0 or quota_gb > 0: - deleted_disk, freed_2, protected_2 = _disk_pressure_pass( + deleted_disk, freed_2, protected_2, failed_2 = _disk_pressure_pass( db, recordings, disk_pct=disk_pct, quota_gb=quota_gb, @@ -188,18 +222,20 @@ def sweep( ) bytes_freed += freed_2 protected += protected_2 + failed += failed_2 summary = { "deleted_time": deleted_time, "deleted_disk": deleted_disk, "protected": protected, + "failed": failed, "bytes_freed": bytes_freed, } - if deleted_time or deleted_disk or protected: + if deleted_time or deleted_disk or protected or failed: log.info( "retention sweep: %d by time, %d by disk, %d protected, " - "%.1f MB freed", - deleted_time, deleted_disk, protected, + "%d failed, %.1f MB freed", + deleted_time, deleted_disk, protected, failed, bytes_freed / (1 << 20), ) return summary @@ -399,10 +435,12 @@ def _disk_pressure_pass( If we exit still over-threshold AND ``protect_ro`` is on, counts the surviving RO clips and reports them as ``protected`` so an operator can see why usage didn't drop. Returns ``(deleted, - bytes_freed, protected)``. + bytes_freed, protected, failed)``. """ deleted = 0 bytes_freed = 0 + failed_ids: set[int] = set() + pruned: set[str] = set() while _over_threshold( recordings, disk_pct=disk_pct, quota_gb=quota_gb, refresh=True, exclude=exclude, @@ -411,6 +449,13 @@ def _disk_pressure_pass( params: list = [] if protect_ro: conds.append("COALESCE(event_type, '') != 'ro'") + if failed_ids: + # Clips whose unlink already failed this pass: keep them out + # of the next batch or the loop would re-select them forever + # while never freeing a byte. + fph = ",".join("?" * len(failed_ids)) + conds.append(f"id NOT IN ({fph})") + params.extend(failed_ids) guard_sql, guard_params = _protect_clause(protect_ids) if guard_sql: conds.append(guard_sql.removeprefix(" AND ")) @@ -427,18 +472,24 @@ def _disk_pressure_pass( if not rows: break for row in rows: - freed = _delete_clip_files(row, recordings) + freed, removed = _delete_clip_files(row, recordings) + if not removed: + failed_ids.add(row["id"]) + continue _cache_subtract(recordings, freed, exclude=exclude) bytes_freed += freed _delete_index_row(db, row["id"]) + pruned.add(os.path.dirname(row["path"])) deleted += 1 _broadcast(sink, row["basename"], "disk") if not _over_threshold( recordings, disk_pct=disk_pct, quota_gb=quota_gb, exclude=exclude, ): - return deleted, bytes_freed, 0 + prune_empty_dirs(pruned) + return deleted, bytes_freed, 0, len(failed_ids) + prune_empty_dirs(pruned) protected = 0 if protect_ro and _over_threshold( recordings, disk_pct=disk_pct, quota_gb=quota_gb, refresh=True, @@ -449,7 +500,7 @@ def _disk_pressure_pass( "SELECT COUNT(*) AS n FROM clip_index " "WHERE COALESCE(event_type, '') = 'ro'" ).fetchone()["n"] - return deleted, bytes_freed, protected + return deleted, bytes_freed, protected, len(failed_ids) def make_room_for( @@ -492,26 +543,43 @@ def _over() -> bool: return False where = "WHERE timestamp < ? AND COALESCE(locked, 0) = 0" - params: list = [before_ts] + base_params: list = [before_ts] if protect_ro: where += " AND COALESCE(event_type, '') != 'ro'" guard_sql, guard_params = _protect_clause(protect_ids) where += guard_sql - params.extend(guard_params) + base_params.extend(guard_params) - while _over(): - with db.conn() as c: - row = c.execute( - f"SELECT id, path, basename FROM clip_index {where} " - f"ORDER BY timestamp ASC LIMIT 1", - params, - ).fetchone() - if row is None: - return False - freed = _delete_clip_files(dict(row), recordings) - _delete_index_row(db, row["id"]) - used = max(0, used - freed) - return True + failed_ids: set[int] = set() + pruned: set[str] = set() + try: + while _over(): + cond = where + params = list(base_params) + if failed_ids: + # Un-unlinkable clips must leave the candidate pool, or + # the oldest-first LIMIT 1 re-selects them forever. + fph = ",".join("?" * len(failed_ids)) + cond += f" AND id NOT IN ({fph})" + params.extend(failed_ids) + with db.conn() as c: + row = c.execute( + f"SELECT id, path, basename FROM clip_index {cond} " + f"ORDER BY timestamp ASC LIMIT 1", + params, + ).fetchone() + if row is None: + return False + freed, removed = _delete_clip_files(dict(row), recordings) + if not removed: + failed_ids.add(row["id"]) + continue + _delete_index_row(db, row["id"]) + pruned.add(os.path.dirname(row["path"])) + used = max(0, used - freed) + return True + finally: + prune_empty_dirs(pruned) def import_exclude_set(recordings: str, import_path: str = "") -> frozenset[str]: diff --git a/web/services/thumbs.py b/web/services/thumbs.py index 1e8a0b3..ea90f29 100644 --- a/web/services/thumbs.py +++ b/web/services/thumbs.py @@ -38,9 +38,18 @@ def _discard(path: str) -> None: os.remove(path) +# Dirs already ensured this process. The path helpers below run on every +# thumb request and inside retention loops; without the memo each call pays +# a makedirs — a metadata round-trip on a NAS mount — for a dir that +# almost always exists. +_ensured_dirs: set[str] = set() + + def _cache_dir(recordings: str) -> str: d = os.path.join(recordings, ".thumbs") - os.makedirs(d, exist_ok=True) + if d not in _ensured_dirs: + os.makedirs(d, exist_ok=True) + _ensured_dirs.add(d) return d diff --git a/web/static/app.js b/web/static/app.js index 73e43fb..646606b 100644 --- a/web/static/app.js +++ b/web/static/app.js @@ -94,7 +94,7 @@ function escHtml(s) { // ---------- API helpers ---------- -async function api(path, opts = {}) { +async function api(path, opts = {}, _retriedCsrf = false) { const headers = { "content-type": "application/json", ...(opts.headers || {}) }; if (state.csrf && opts.method && opts.method !== "GET") { headers["x-csrf-token"] = state.csrf; @@ -104,12 +104,15 @@ async function api(path, opts = {}) { showLogin(); throw new Error("unauthorised"); } - if (r.status === 403 && state.csrf) { - // refresh CSRF once and retry + if (r.status === 403 && state.csrf && !_retriedCsrf) { + // Refresh CSRF once and retry; a second 403 falls through to the + // throw below. Without the guard a persistently-403ing POST (proxy + // caching the csrf GET, session reissued between calls) recurses + // forever with no error surfaced — the button just looks dead. const cr = await fetch("/api/auth/csrf", { credentials: "same-origin" }); if (cr.ok) { state.csrf = (await cr.json()).csrf; - return api(path, opts); + return api(path, opts, true); } } if (!r.ok) throw new Error(`${r.status} ${r.statusText}`); @@ -458,8 +461,17 @@ function refreshOpenArchiveDays() { if (body && !body.hidden) renderDayBody(body, dayEl.dataset.day); }); } +const OPEN_DAY_REFRESH_MS = 300; +const OPEN_DAY_REFRESH_SYNC_MS = 2000; function scheduleOpenArchiveRefresh() { - if (_openDayRefreshTimer) clearTimeout(_openDayRefreshTimer); + // Coalescing throttle, not a resetting debounce: queue_changed fires on + // every item transition during a download session, and re-arming the + // timer each time both starved the refresh under a steady stream and, + // once idle, re-rendered every open day per transition. While a sync is + // active, widen the window — one refresh per ~2 s is plenty for status + // icons, and renderDayBody skips unchanged days anyway. + if (_openDayRefreshTimer) return; + const delay = state.syncRunning ? OPEN_DAY_REFRESH_SYNC_MS : OPEN_DAY_REFRESH_MS; _openDayRefreshTimer = setTimeout(() => { _openDayRefreshTimer = null; if (document.getElementById("view-archive").hidden) { @@ -467,7 +479,7 @@ function scheduleOpenArchiveRefresh() { return; } refreshOpenArchiveDays(); - }, 300); + }, delay); } // ---------- Archive ---------- @@ -746,9 +758,20 @@ async function renderDayBody(body, date) { } catch (e) { destroyJourneyMaps(body); body.innerHTML = `
Failed to load: ${e}
`; + body._renderKey = null; // error is on screen — force a real rebuild next time return; } + // Skip-unchanged diff: key the render on the payload plus everything else + // that shapes it. A sync-time refresh hits every open day, but usually only + // the day being downloaded into actually changed — the rest bail here, + // keeping their DOM (and any open Leaflet maps) untouched instead of a + // wipe-and-rebuild of ~hundreds of tiles per queue_changed. + const renderKey = JSON.stringify( + { q: q.toString(), maps: state.showMaps, loc: state.filters.location, data, route }); + if (body._renderKey === renderKey) return; + body._renderKey = renderKey; + // FLIP: snapshot keyed positions before the swap, animate after (below). const flipPrev = flipCapture(body); @@ -1473,7 +1496,8 @@ function renderClipPair(pair) { const overlay = thumbEl.querySelector(".film-scrub"); if (!overlay) return; wireLazyFilmstripScrub( - overlay, thumbEl, () => api(`/api/archive/clip/${id}/filmstrip`)); + overlay, thumbEl, + (signal) => api(`/api/archive/clip/${id}/filmstrip`, { signal })); }); // Selection checkbox → export set. Preserve selected state @@ -1821,19 +1845,52 @@ async function runQueueAction(action, filenames) { } toast(`${action.replaceAll("-", " ")}: ${res.updated} updated`); clearSelection(); - refreshOpenArchiveDays(); + // Skip removes tiles from the archive, so the day cards' totals + // changed too — anything else only flips per-clip status icons. + if (action === "skip") loadDays(); + else refreshOpenArchiveDays(); } catch (e) { toast(`Action failed: ${e.message || e}`, { type: "error" }); } } +function deleteResultToast(res) { + const bits = [`Deleted ${res.deleted}`]; + if (res.skipped) bits.push(`${res.skipped} won't re-download`); + if (res.protected) bits.push(`${res.protected} read-only/locked kept`); + if (res.failed) bits.push(`${res.failed} failed — file could not be removed, see Logs`); + toast(bits.join(", "), { type: res.failed ? "error" : "success" }); +} + async function runQueueDelete(filenames) { try { await animateOutSelectedPairs(); const res = await api("/api/queue/delete", { method: "POST", body: JSON.stringify({ filenames }) }); - toast(`Deleted ${res.deleted}, skipped ${res.skipped}`); + // Confirm-through for protected clips: the first call already deleted + // everything deletable, so ask once more and force-delete exactly the + // refused names. Declining keeps them — the reload below restores + // their faded-out tiles. + if (res.protected > 0 && (res.protected_names || []).length) { + const ok = confirm( + `${res.protected} of the selected clip(s) are read-only or locked ` + + `(dashcam event recordings, or clips marked read-only). Delete them anyway?`, + ); + if (ok) { + const forced = await api("/api/queue/delete", { + method: "POST", + body: JSON.stringify({ filenames: res.protected_names, force: true }), + }); + res.deleted += forced.deleted; + res.skipped += forced.skipped; + res.failed += forced.failed; + res.protected = 0; + } + } + deleteResultToast(res); clearSelection(); - refreshOpenArchiveDays(); + // Full reload, not just open bodies: the day cards' clip counts and + // GB totals changed too. + loadDays(); } catch (e) { toast(`Delete failed: ${e.message || e}`, { type: "error" }); } @@ -1866,11 +1923,13 @@ async function applyClipAction() { toast("Select some clips first.", { type: "error" }); return; } - if (action === "mark-ro") { + if (action === "mark-ro" || action === "clear-ro") { + const lock = action === "mark-ro"; try { - const res = await api("/api/queue/lock", + const res = await api(lock ? "/api/queue/lock" : "/api/queue/unlock", { method: "POST", body: JSON.stringify({ filenames }) }); - toast(`Marked ${res.updated} read-only`); + toast(lock ? `Marked ${res.updated} read-only` + : `Cleared read-only on ${res.updated}`); clearSelection(); refreshOpenArchiveDays(); } catch (e) { @@ -2071,25 +2130,41 @@ function applyFilmstripScrub(el, spriteUrl, frames) { return true; } -// Lazily load a filmstrip the first time `hoverEl` is hovered, then wire `el` -// to scrub. `load` returns a promise of the filmstrip metadata -// ({ sprite_url, frames }). Loading on first hover (rather than for every -// visible tile) avoids spawning ffmpeg across a whole day just by opening it. -// A thrown error (network/5xx) re-arms so a later hover retries; a clip that -// can't be rendered (204, no sprite_url) is left as a permanent no-op. +// Lazily load a filmstrip once `hoverEl` has been hovered for a beat, then +// wire `el` to scrub. `load(signal)` returns a promise of the filmstrip +// metadata ({ sprite_url, frames }). The dwell requirement matters: each cold +// request spawns ffmpeg on the server, and firing on raw mouseenter meant a +// fast sweep across a day grid queued one sprite job per tile crossed — +// saturating the browser's ~6 connections per origin and blocking every other +// API call behind sprite generation. Leaving the tile cancels the timer and +// aborts any in-flight request. A thrown error (network/5xx/abort) re-arms so +// a later hover retries; a clip that can't be rendered (204, no sprite_url) +// is left as a permanent no-op. +const FILMSTRIP_HOVER_DWELL_MS = 350; function wireLazyFilmstripScrub(el, hoverEl, load) { let started = false; + let dwellTimer = null; + let ctrl = null; hoverEl.addEventListener("mouseenter", () => { - if (started) return; - started = true; - Promise.resolve() - .then(load) - .then((meta) => { - if (meta && meta.sprite_url) { - applyFilmstripScrub(el, meta.sprite_url, meta.frames); - } - }) - .catch(() => { started = false; }); + if (started || dwellTimer) return; + dwellTimer = setTimeout(() => { + dwellTimer = null; + started = true; + ctrl = new AbortController(); + Promise.resolve(ctrl.signal) + .then(load) + .then((meta) => { + if (meta && meta.sprite_url) { + applyFilmstripScrub(el, meta.sprite_url, meta.frames); + } + }) + .catch(() => { started = false; }) + .finally(() => { ctrl = null; }); + }, FILMSTRIP_HOVER_DWELL_MS); + }); + hoverEl.addEventListener("mouseleave", () => { + if (dwellTimer) { clearTimeout(dwellTimer); dwellTimer = null; } + if (ctrl) ctrl.abort(); }); } diff --git a/web/static/index.html b/web/static/index.html index 8d37ce7..661dc75 100644 --- a/web/static/index.html +++ b/web/static/index.html @@ -116,6 +116,7 @@