diff --git a/backend/app/downloads/folders.py b/backend/app/downloads/folders.py index 570d9598..4a3769ef 100644 --- a/backend/app/downloads/folders.py +++ b/backend/app/downloads/folders.py @@ -136,7 +136,14 @@ def done_expiry(db: Session, folder_id: int | None, kept: bool) -> datetime | No folder that keeps files).""" if kept or folder_id is None: return None - days = db.execute(select(FileFolder.retention_days).where(FileFolder.id == folder_id)).scalar() + # Read under a share lock, held to the caller's commit: a retention change (`set_retention` locks + # the folder) waits for this file and restamps it too, or this read waits for the change and + # takes the new value — never the old value stamped on a file the change did not see. + days = db.execute( + select(FileFolder.retention_days) + .where(FileFolder.id == folder_id) + .with_for_update(read=True) + ).scalar() return None if days is None else datetime.now(timezone.utc) + timedelta(days=days) @@ -208,18 +215,30 @@ def set_retention(db: Session, folder: FileFolder, days: int | None) -> None: """Change how long `folder` keeps a file (None = until deleted). The done files already in it move by the difference — each keeps the age it has — but none ends up closer than the GC's warning window, so a shortened retention still warns before it takes anything.""" + # Wait out a file being put in it or stamped from it right now (`own_folder`, `done_expiry`), so + # that file is among the ones restamped below. Re-read: another change may have landed first. + db.execute( + select(FileFolder) + .where(FileFolder.id == folder.id) + .with_for_update() + .execution_options(populate_existing=True) + ) old = folder.retention_days if old == days: return folder.retention_days = days now = datetime.now(timezone.utc) floor = now + timedelta(days=sysconfig.get_int(db, "download_gc_grace_days")) + # Locked as they are read: one moved out or deleted meanwhile is no longer this folder's to time. jobs = db.execute( - select(DownloadJob).where( + select(DownloadJob) + .where( DownloadJob.folder_id == folder.id, DownloadJob.status == JobStatus.DONE, DownloadJob.kept.is_(False), ) + .with_for_update() + .execution_options(populate_existing=True) ).scalars() for job in jobs: if days is None: @@ -238,7 +257,18 @@ def delete_own(db: Session, folder: FileFolder) -> int: raise FolderError("file_folder_not_own") # Wait out a file being put in it right now (`own_folder`), then take that one home too. db.execute(select(FileFolder.id).where(FileFolder.id == folder.id).with_for_update()) - jobs = db.execute(select(DownloadJob).where(DownloadJob.folder_id == folder.id)).scalars().all() + # Locked as they are read: a file moved out meanwhile stays where it went, and one deleted + # meanwhile is not written back (its gone row would fail the delete). + jobs = ( + db.execute( + select(DownloadJob) + .where(DownloadJob.folder_id == folder.id) + .with_for_update() + .execution_options(populate_existing=True) + ) + .scalars() + .all() + ) for job in jobs: job.folder_id = source_folder(db, job.user_id, source_key_for(db, job)).id restamp(db, job) diff --git a/backend/app/downloads/gc.py b/backend/app/downloads/gc.py index 981c4b85..d0041c96 100644 --- a/backend/app/downloads/gc.py +++ b/backend/app/downloads/gc.py @@ -22,7 +22,7 @@ import time from datetime import datetime, timedelta, timezone from pathlib import Path -from sqlalchemy import func, or_, select +from sqlalchemy import func, or_, select, update from sqlalchemy.orm import Session from app import logs, sysconfig @@ -82,6 +82,59 @@ def _relock_expired( return job if still else None +def _mark_warned(db: Session, job_id: int, expires_at: datetime) -> DownloadJob | None: + """Mark one file warned if it still goes when the pass saw it go — not deleted, kept, moved or + re-timed since (each of those sets a fresh expiry, and its own warning to come) — and return it. + The UPDATE's row lock holds it so until the commit.""" + hit = db.execute( + update(DownloadJob) + .where( + DownloadJob.id == job_id, + DownloadJob.status == JobStatus.DONE, + DownloadJob.kept.is_(False), + DownloadJob.expiry_warned.is_(False), + DownloadJob.expires_at == expires_at, + ) + .values(expiry_warned=True) + .returning(DownloadJob.id) + .execution_options(synchronize_session=False) + ).scalar_one_or_none() + return db.get(DownloadJob, job_id) if hit is not None else None + + +def _on_disk(db: Session) -> int: + """The bytes the cache holds now (every asset whose file is on disk).""" + return ( + db.execute( + select(func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where(quota.ON_DISK) + ).scalar() + or 0 + ) + + +def _relock_evictable(db: Session, asset_id: int) -> tuple[MediaAsset | None, list[DownloadJob]]: + """Re-read an eviction candidate and its holders under their locks (the asset, then its jobs — + the order the user routes take): between the pass's SELECT and here a holder may have kept it, + or every holder deleted it.""" + asset = db.execute( + select(MediaAsset) + .where(MediaAsset.id == asset_id) + .with_for_update() + .execution_options(populate_existing=True) + ).scalar_one_or_none() + holders = ( + db.execute( + select(DownloadJob) + .where(DownloadJob.asset_id == asset_id) + .with_for_update() + .execution_options(populate_existing=True) + ) + .scalars() + .all() + ) + return asset, list(holders) + + def _drop_job(db: Session, job: DownloadJob, ntype: str) -> None: """Take a file out of its user's library: tell them, release the job's hold on the asset (the file goes with the last holder), delete the row (its shares and links cascade).""" @@ -176,29 +229,28 @@ def run_download_gc(db: Session) -> dict: grace_days = sysconfig.get_int(db, "download_gc_grace_days") warned = expired = evicted = reaped = 0 - # 1. Pre-expiry warnings — one per file, the first run inside the grace window. + # 1. Pre-expiry warnings — one per file, the first run inside the grace window. Each mark is + # conditional on the expiry the SELECT saw and committed on its own: a file deleted since is + # passed over (a write to its gone row would fail the whole run), and one kept, moved or re-timed + # since keeps its fresh expiry unwarned. warn_before = now + timedelta(days=grace_days) - soon = ( - db.execute( - select(DownloadJob).where( - DownloadJob.status == JobStatus.DONE, - DownloadJob.kept.is_(False), - DownloadJob.expiry_warned.is_(False), - DownloadJob.expires_at.is_not(None), - DownloadJob.expires_at > now, - DownloadJob.expires_at <= warn_before, - ) + soon = db.execute( + select(DownloadJob.id, DownloadJob.expires_at).where( + DownloadJob.status == JobStatus.DONE, + DownloadJob.kept.is_(False), + DownloadJob.expiry_warned.is_(False), + DownloadJob.expires_at.is_not(None), + DownloadJob.expires_at > now, + DownloadJob.expires_at <= warn_before, ) - .scalars() - .all() - ) - for job in soon: - _notify( - db, job, db.get(MediaAsset, job.asset_id) if job.asset_id else None, "download_expiring" - ) - job.expiry_warned = True - warned += 1 - if soon: + ).all() + db.rollback() # the pass-1 snapshot holds no locks; each mark takes its own below + for job_id, expires_at in soon: + job = _mark_warned(db, job_id, expires_at) + if job is not None: + asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None + _notify(db, job, asset, "download_expiring") + warned += 1 db.commit() # 2. Expiry — a file past its folder's retention leaves the library (warned in pass 1). @@ -224,19 +276,14 @@ def run_download_gc(db: Session) -> dict: # fetch again) and never a file anyone kept; an evicted file leaves every holder's library. cap = sysconfig.get_int(db, "download_total_max_bytes") if cap > 0: - total = ( - db.execute( - select(func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where(quota.ON_DISK) - ).scalar() - or 0 - ) + total = _on_disk(db) if total > cap: kept_assets = select(DownloadJob.asset_id).where( DownloadJob.kept.is_(True), DownloadJob.asset_id.is_not(None) ) - candidates = ( + candidate_ids = ( db.execute( - select(MediaAsset) + select(MediaAsset.id) .where( MediaAsset.status == "ready", MediaAsset.source_kind != "upload", @@ -247,30 +294,20 @@ def run_download_gc(db: Session) -> dict: .scalars() .all() ) - candidate_ids = [a.id for a in candidates] db.rollback() for asset_id in candidate_ids: if total <= cap: break # Locked and re-read like an expiry: a holder may have kept it, or every holder - # deleted it (the file is then already gone and counted out) since the SELECT. - asset = db.execute( - select(MediaAsset) - .where(MediaAsset.id == asset_id) - .with_for_update() - .execution_options(populate_existing=True) - ).scalar_one_or_none() - holders = ( - db.execute( - select(DownloadJob) - .where(DownloadJob.asset_id == asset_id) - .with_for_update() - .execution_options(populate_existing=True) - ) - .scalars() - .all() - ) - if asset is None or asset.status != "ready" or any(j.kept for j in holders): + # deleted it since the SELECT. + asset, holders = _relock_evictable(db, asset_id) + if asset is None or asset.status != "ready": + # Its file went (or is being fetched afresh) since the sum: the disk is counted + # again, or the pass would evict another user's file for space already free. + db.commit() + total = _on_disk(db) + continue + if any(j.kept for j in holders): db.commit() continue total -= asset.size_bytes or 0 diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 3da8fd6b..7fa7f2b2 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -65,6 +65,23 @@ def reference_url(job, asset) -> str | None: return None +def library_left_behind(db: Session, files_root: str) -> bool: + """True when the library names files and none of the newest of them is under `files_root` — a + fresh, empty volume mounted in the library's place (a compose file changed under an install, its + old volume left behind). The mount check cannot see that: the new volume is a real mount.""" + rels = ( + db.execute( + select(MediaAsset.rel_path) + .where(MediaAsset.status == "ready", MediaAsset.rel_path.is_not(None)) + .order_by(MediaAsset.id.desc()) + .limit(20) + ) + .scalars() + .all() + ) + return bool(rels) and not any(storage.abs_path(files_root, r).exists() for r in rels if r) + + def get_or_create_asset( db: Session, source_kind: str, source_ref: str, format_sig: str ) -> MediaAsset: @@ -77,7 +94,12 @@ def get_or_create_asset( MediaAsset.source_ref == source_ref, MediaAsset.format_sig == format_sig, ) - asset = db.execute(stmt).scalar_one_or_none() + # Locked to the caller's commit: a release or the GC freeing this asset meanwhile (they lock it + # too) either waits for the new hold, or has deleted the row by the time this read returns — and + # the caller then gets a fresh one, never a job pointing at a row that is gone. + asset = db.execute( + stmt.with_for_update().execution_options(populate_existing=True) + ).scalar_one_or_none() if asset is not None: # A previously-FAILED shared asset must be re-attempted for the new job about to hold it — # reset it to pending (+clear the error) so the worker actually re-downloads instead of diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index 8a003117..c3de9bf8 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -68,20 +68,52 @@ _FOLD_MAP = str.maketrans( ) -def files_root_problem(root: str, system_root: str = "/") -> str | None: - """Why `root` cannot be the files library's volume, or None when it can. A root that sits on the - container's own filesystem is the image's empty directory, not a mounted volume: the library - would answer 404 for every file, and what a download writes would go when the container does — - the state an install is in when its compose file still mounts the library somewhere else.""" +def _mount_points(mountinfo: str) -> list[str]: + """The mount points a /proc//mountinfo table lists (its fifth field, octal escapes undone).""" + points = [] + for line in mountinfo.splitlines(): + fields = line.split(" ") + if len(fields) > 4: + points.append(re.sub(r"\\([0-7]{3})", lambda m: chr(int(m.group(1), 8)), fields[4])) + return points + + +def _in_container() -> bool: + return Path("/.dockerenv").exists() or Path("/run/.containerenv").exists() + + +def _read_mountinfo() -> str | None: + try: + return Path("/proc/self/mountinfo").read_text() + # silent-ok: no mount table to read (not Linux) — the caller then cannot tell, and refuses nothing. + except OSError: + return None + + +def files_root_problem( + root: str, mountinfo: str | None = None, in_container: bool | None = None +) -> str | None: + """Why `root` cannot be the files library's volume, or None when it can. In a container, a root + that is no mount of its own (nor inside one) is the image's empty directory: the library would + answer 404 for every file, and what a download writes would go when the container does — the + state an install is in when its compose file still mounts the library somewhere else. + + Asked of the kernel's mount table, not by comparing devices: a storage driver that keeps the + container's root on the host's own disk (vfs, the fallback under Docker-in-LXC on older ZFS) gives + a mounted volume the same device as `/`. Outside a container a plain directory is a fine root.""" path = Path(root) if not path.is_dir(): return f"{root} does not exist" - try: - if path.stat().st_dev == Path(system_root).stat().st_dev: - return f"{root} is not a mounted volume (it is the container's own empty directory)" - # silent-ok: the failure IS the answer — returned as the reason, which the caller logs. - except OSError as e: - return f"{root} cannot be read ({e})" + if in_container if in_container is not None else _in_container(): + table = mountinfo if mountinfo is not None else _read_mountinfo() + if table is not None: + real = os.path.realpath(root) + mounted = any( + m != "/" and (real == m or real.startswith(m.rstrip("/") + "/")) + for m in _mount_points(table) + ) + if not mounted: + return f"{root} is not a mounted volume (it is the container's own empty directory)" if not os.access(path, os.W_OK): return f"{root} is not writable" return None diff --git a/backend/app/downloads/upload.py b/backend/app/downloads/upload.py index 00668379..7e382a19 100644 --- a/backend/app/downloads/upload.py +++ b/backend/app/downloads/upload.py @@ -70,7 +70,9 @@ def ingest_upload( rejected upload leaves nothing behind and the caller just cleans up its temp file.""" quota.check_upload(db, user_id, size) # the file's size is known → charge it up front if folder_id is not None: - folders.upload_target(db, user_id, folder_id) + # Refused before any work; held only once the job is about to name it (below), not through + # the probe and the poster, which a rename or delete of the folder would wait out. + folders.upload_target(db, user_id, folder_id, hold=False) data = probe.probe_json(src_path) vcodec, acodec, width, height, duration = probe.facts(data) @@ -138,7 +140,7 @@ def ingest_upload( db.add(job) folders.place(db, job) if folder_id is not None: - job.folder_id = folder_id + job.folder_id = folders.upload_target(db, user_id, folder_id).id if not convert: folders.stamp_done(db, job) service.incr_ref_count(db, asset.id) diff --git a/backend/app/main.py b/backend/app/main.py index cd7cbba2..795456b7 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -28,6 +28,7 @@ log = logs.get_logger(__name__) from app import bodylimit, collation, logsink, state, sysconfig # noqa: E402 — logging is configured first from app.config import settings # noqa: E402 — logging is configured first from app.db import SessionLocal, engine # noqa: E402 — logging is configured first +from app.downloads import service as download_service # noqa: E402 — logging is configured first from app.downloads import storage # noqa: E402 — logging is configured first from app.plex import stream as plex_stream # noqa: E402 — logging is configured first from app.plex import subtitles as plex_subtitles # noqa: E402 — logging is configured first @@ -84,6 +85,18 @@ async def lifespan(app: FastAPI): # With the traceback: this catch spans a connection, a query and a coercion at boot, # and `%s` of the exception does not say which of them raised. log.exception("could not read the stored log level; staying at %s", current_level()) + if not problem: + try: + left = download_service.library_left_behind(db, settings.files_root) + except Exception: # a database not reachable yet: the check is a hint, never a gate + log.exception("could not check the files library against its database") + else: + if left: + log.critical( + "The files library is empty but the database names files: the volume " + "mounted at %s is not the one that holds them. See docs/self-hosting.md.", + settings.files_root, + ) if not state.is_configured(db): token = state.rotate_setup_token(db) url = f"{settings.app_base}/setup?token={token}" diff --git a/backend/app/routes/files.py b/backend/app/routes/files.py index 3c67f709..ca773f43 100644 --- a/backend/app/routes/files.py +++ b/backend/app/routes/files.py @@ -339,10 +339,6 @@ def _serialize_folder(folder: FileFolder, count: int, size: int) -> dict: } -def _own_folder_or_404(db: Session, user: User, folder_id: int) -> FileFolder: - return owned_or_404(db, FileFolder, folder_id, user, what="folder") - - @router.get("/folders") def list_folders(user: User = Depends(current_user), db: Session = Depends(get_db)) -> list[dict]: """The caller's folders with how many files each holds and their size. A source folder its @@ -407,7 +403,7 @@ def update_folder( ) -> dict: """Rename one of the caller's own folders (a source folder's name is its origin's), and/or set how long any of their folders keeps a file (`folders.set_retention`).""" - folder = _own_folder_or_404(db, user, folder_id) + folder = owned_or_404(db, FileFolder, folder_id, user, what="folder") sent = body.model_fields_set if "name" in sent: if folder.kind != folders.FolderKind.OWN: @@ -457,7 +453,7 @@ def delete_folder( folder_id: int, user: User = Depends(current_user), db: Session = Depends(get_db) ) -> dict: """Delete one of the caller's own folders; its files go back to their source folders.""" - folder = _own_folder_or_404(db, user, folder_id) + folder = owned_or_404(db, FileFolder, folder_id, user, what="folder") try: moved = folders.delete_own(db, folder) except folders.FolderError as e: diff --git a/backend/tests/test_file_folders.py b/backend/tests/test_file_folders.py index 4c1b068f..9bd40d3a 100644 --- a/backend/tests/test_file_folders.py +++ b/backend/tests/test_file_folders.py @@ -308,21 +308,22 @@ def test_one_users_expiry_leaves_another_users_copy(db, make_user, _root): assert (_root / "a.mp4").exists() -def _meanwhile(monkeypatch, act): - """Run `act` in another session (another request) between the GC's expiry SELECT and its - handling of each file it found — the window a user's click can land in.""" - real = gcmod._relock_expired +def _meanwhile(monkeypatch, act, step="_relock_expired"): + """Run `act` in another session (another request) between a GC pass's SELECT and its handling of + each row it found — the window a user's click can land in. `step` is the pass's per-row re-read + (expiry, warning, eviction); `act` gets that row's id.""" + real = getattr(gcmod, step) - def relock(db, job_id, asset_id, now): + def wrapped(db, key, *rest): other = SessionLocal() try: - act(other, job_id) + act(other, key) other.commit() finally: other.close() - return real(db, job_id, asset_id, now) + return real(db, key, *rest) - monkeypatch.setattr(gcmod, "_relock_expired", relock) + monkeypatch.setattr(gcmod, step, wrapped) def test_a_file_kept_while_the_gc_runs_stays(db, make_user, _root, monkeypatch): @@ -376,6 +377,236 @@ def test_the_cache_cap_never_evicts_a_kept_file_and_an_evicted_one_leaves_every_ assert (_root / "old.mp4").exists() and not (_root / "new.mp4").exists() +def test_a_file_deleted_while_the_gc_warns_does_not_stop_the_run(db, make_user, _root, monkeypatch): + # Written back from a stale read, the warning of a deleted file failed the whole run: nothing + # expired or was evicted that cycle, nor the next while the race lasted. + (_root / "a.mp4").write_bytes(b"x") + (_root / "b.mp4").write_bytes(b"x") + uid = make_user().id + _job(db, uid, _asset(db), expires_at=NOW + timedelta(hours=1)) + gone = _job( + db, uid, _asset(db, ref="bbbbbbbbbbb", rel="b.mp4"), expires_at=NOW - timedelta(minutes=1) + ) + + def user_deletes(other, jid): + service.delete(other, other.get(DownloadJob, jid)) + + _meanwhile(monkeypatch, user_deletes, step="_mark_warned") + result = _gc(db) + assert (result["warned"], result["expired"]) == (0, 1) + db.expire_all() + assert db.get(DownloadJob, gone.id) is None + assert _notices(db, uid, "download_expiring") == 0 + + +def test_a_file_re_timed_while_the_gc_warns_keeps_its_new_expiry_unwarned( + db, make_user, _root, monkeypatch +): + # Moved, un-kept or its folder's retention changed: a fresh expiry, warned when it comes — not + # marked warned now for a date it no longer has. + uid = make_user().id + job = _job(db, uid, _asset(db), expires_at=NOW + timedelta(hours=1)) + later = NOW + timedelta(days=30) + + def re_timed(other, jid): + moved = other.get(DownloadJob, jid) + moved.expires_at = later + moved.expiry_warned = False + + _meanwhile(monkeypatch, re_timed, step="_mark_warned") + assert _gc(db)["warned"] == 0 + db.expire_all() + fresh = db.get(DownloadJob, job.id) + assert (fresh.expires_at, fresh.expiry_warned) == (later, False) + assert _notices(db, uid, "download_expiring") == 0 + + +def _two_over_the_cap(db, make_user, _root): + sysconfig.set_value(db, "download_total_max_bytes", 15) + db.commit() + a, b = make_user().id, make_user().id + for name in ("old.mp4", "new.mp4"): + (_root / name).write_bytes(b"x") + old = _asset(db, ref="oldoldoldol", rel="old.mp4", size=10) + new = _asset(db, ref="newnewnewne", rel="new.mp4", size=10) + old.last_access_at = NOW - timedelta(days=9) + new.last_access_at = NOW - timedelta(days=1) + return _job(db, a, old), _job(db, b, new) + + +def test_a_file_deleted_while_the_cache_is_over_its_cap_frees_that_space_and_no_more( + db, make_user, _root, monkeypatch +): + # Its bytes still counted, the pass went on to evict another user's file for space already free. + jold, jnew = _two_over_the_cap(db, make_user, _root) + + def holder_deletes(other, asset_id): + if asset_id == jold.asset_id: + service.delete(other, other.get(DownloadJob, jold.id)) + + _meanwhile(monkeypatch, holder_deletes, step="_relock_evictable") + assert _gc(db)["evicted"] == 0 + db.expire_all() + assert db.get(DownloadJob, jnew.id) is not None + assert (_root / "new.mp4").exists() + + +def test_a_file_kept_while_the_cache_is_over_its_cap_stays(db, make_user, _root, monkeypatch): + jold, jnew = _two_over_the_cap(db, make_user, _root) + + def holder_keeps(other, asset_id): + if asset_id == jold.asset_id: + other.get(DownloadJob, jold.id).kept = True + + _meanwhile(monkeypatch, holder_keeps, step="_relock_evictable") + assert _gc(db)["evicted"] == 1 # the newer one goes in its place + db.expire_all() + assert db.get(DownloadJob, jold.id) is not None + assert (_root / "old.mp4").exists() + assert db.get(DownloadJob, jnew.id) is None + + +def _in_thread(work): + """Run `work(other_session)` in another thread and session; returns (thread, outcome list).""" + import threading + + outcome: list[object] = [] + + def run(): + other = SessionLocal() + try: + outcome.append(work(other)) + other.commit() + except Exception as e: # noqa: BLE001 — the assertion below names what went wrong + outcome.append(e) + finally: + other.close() + + t = threading.Thread(target=run) + t.start() + return t, outcome + + +def test_a_retention_changed_while_a_file_goes_in_times_that_file_too(db, make_user): + # The change used to list the folder's files before the arriving one was in, so that one kept the + # old retention — deleted later from a folder now set to keep files forever. + uid = make_user().id + own = FileFolder(user_id=uid, kind=folders.FolderKind.OWN, name="Mine", retention_days=7) + db.add(own) + db.commit() + own_id = own.id + job = _job(db, uid, _asset(db)) + target = folders.own_folder(db, uid, own_id) # held from the check until this commits + t, outcome = _in_thread( + lambda other: folders.set_retention(other, other.get(FileFolder, own_id), None) + ) + t.join(timeout=1) + folders.move(db, job, target) # stamped with the 7 days it sees + db.commit() + t.join(timeout=10) + assert outcome == [None] + db.expire_all() + assert db.get(DownloadJob, job.id).expires_at is None + + +def test_a_retention_changed_while_a_file_finishes_there_times_that_file_too(db, make_user): + # The worker's finish read the folder's old retention unlocked, while the change listed the done + # files before this one was among them. + uid = make_user().id + own = FileFolder(user_id=uid, kind=folders.FolderKind.OWN, name="Mine", retention_days=7) + db.add(own) + db.commit() + own_id = own.id + job = _job(db, uid, _asset(db), status=JobStatus.RUNNING) + folders.move(db, job, own) + db.commit() + job.status = JobStatus.DONE + folders.stamp_done(db, job) # the finish: stamped with the 7 days it reads + db.flush() + t, outcome = _in_thread( + lambda other: folders.set_retention(other, other.get(FileFolder, own_id), None) + ) + t.join(timeout=1) + db.commit() + t.join(timeout=10) + assert outcome == [None] + db.expire_all() + assert db.get(DownloadJob, job.id).expires_at is None + + +def test_a_file_moved_out_while_its_folder_is_deleted_stays_where_it_went(db, make_user): + uid = make_user().id + f = FileFolder(user_id=uid, kind=folders.FolderKind.OWN, name="F") + g = FileFolder(user_id=uid, kind=folders.FolderKind.OWN, name="G") + db.add_all([f, g]) + db.commit() + f_id, g_id = f.id, g.id + job = _job(db, uid, _asset(db)) + folders.move(db, job, f) + db.commit() + folders.move(db, job, folders.own_folder(db, uid, g_id)) + db.flush() # the move is written, not yet committed + t, outcome = _in_thread(lambda other: folders.delete_own(other, other.get(FileFolder, f_id))) + t.join(timeout=1) + assert t.is_alive() # the delete waits for the file it would take home + db.commit() + t.join(timeout=10) + assert outcome == [0] + db.expire_all() + assert db.get(DownloadJob, job.id).folder_id == g_id + + +def test_a_file_deleted_while_its_folder_is_deleted_does_not_fail_the_delete(db, make_user, _root): + (_root / "a.mp4").write_bytes(b"x") + uid = make_user().id + f = FileFolder(user_id=uid, kind=folders.FolderKind.OWN, name="F") + db.add(f) + db.commit() + f_id = f.id + job = _job(db, uid, _asset(db)) + folders.move(db, job, f) + db.commit() + service.delete(db, db.get(DownloadJob, job.id)) + db.flush() # deleted, not yet committed + t, outcome = _in_thread(lambda other: folders.delete_own(other, other.get(FileFolder, f_id))) + t.join(timeout=1) + db.commit() + t.join(timeout=10) + assert outcome == [0] + db.expire_all() + assert db.get(FileFolder, f_id) is None + + +def test_a_file_asked_for_while_its_cached_copy_is_freed_gets_a_fresh_one(db, make_user): + # The enqueue read the cached copy unlocked: freed meanwhile, its job pointed at a row that was gone + # (a foreign-key failure, a 500). + asset = _asset(db) + asset_id, sig = asset.id, asset.format_sig + db.commit() + db.execute(select(MediaAsset).where(MediaAsset.id == asset_id).with_for_update()) + db.delete(db.get(MediaAsset, asset_id)) + db.flush() # the release, not yet committed + t, outcome = _in_thread( + lambda other: service.get_or_create_asset(other, "youtube", "abcdefghijk", sig).id + ) + t.join(timeout=1) + assert t.is_alive() # it waits for the release + db.commit() + t.join(timeout=10) + [fresh] = outcome + assert isinstance(fresh, int) and fresh != asset_id + + +def test_an_empty_volume_in_the_librarys_place_is_noticed(db, make_user, _root): + # A fresh volume is a real, writable mount: only the files the library names tell it is the wrong one. + assert service.library_left_behind(db, str(_root)) is False # nothing named yet + _asset(db, rel="a.mp4") + db.commit() + assert service.library_left_behind(db, str(_root)) is True + (_root / "a.mp4").write_bytes(b"x") + assert service.library_left_behind(db, str(_root)) is False + + def test_deleting_a_user_takes_their_folders_and_files_with_them(db, make_user): # Both tables cascade from users; the files' folder FK refuses deleting a folder that still holds # files, which must not stop the account's own deletion. diff --git a/backend/tests/test_files_api.py b/backend/tests/test_files_api.py index 5a3ddf3e..1f4f4395 100644 --- a/backend/tests/test_files_api.py +++ b/backend/tests/test_files_api.py @@ -498,6 +498,50 @@ def test_an_upload_to_a_folder_it_may_not_use_is_refused_before_its_body_is_read assert db.execute(select(DownloadJob.id)).all() == before +def test_an_upload_holds_its_folder_only_once_its_file_is_about_to_be_in_it( + db, make_user, as_user, _root, upload_headers, monkeypatch +): + # Held from the start, the folder waited out the probe and the poster (up to a minute) before a + # rename or a delete of it could go on; not held at all, a delete could take it from under the file. + alice = make_user() + own = _own(db, alice.id) + steps: list[object] = [] + real = folders.own_folder + + def own_folder(db_, user_id, folder_id, *, hold=True): + steps.append(("folder", hold)) + return real(db_, user_id, folder_id, hold=hold) + + def probe(_p): + steps.append("probe") + + monkeypatch.setattr(folders, "own_folder", own_folder) + monkeypatch.setattr(uploadmod.probe, "probe_json", probe) + r = as_user(alice).post( + "/api/files/upload", content=b"x", headers=upload_headers("a.txt", folder=own.id) + ) + assert r.status_code == 200, r.text + # The route's own look before the body, the ingest's look, the probe, then the hold. + assert steps[-3:] == [("folder", False), "probe", ("folder", True)] + + +def test_a_file_moved_holds_the_folder_it_goes_to(db, make_user, as_user, monkeypatch): + alice = make_user() + job = _file(db, alice.id) + own = _own(db, alice.id) + holds: list[bool] = [] + real = folders.own_folder + + def own_folder(db_, user_id, folder_id, *, hold=True): + holds.append(hold) + return real(db_, user_id, folder_id, hold=hold) + + monkeypatch.setattr(folders, "own_folder", own_folder) + r = as_user(alice).patch(f"/api/files/{job.id}", json={"folder_id": own.id}) + assert r.status_code == 200, r.text + assert holds == [True] + + def test_a_folder_deleted_while_its_upload_streams_refuses_the_upload( db, make_user, as_user, _root, _no_probe, upload_headers, monkeypatch ): diff --git a/backend/tests/test_storage.py b/backend/tests/test_storage.py index 7525e773..53d92b27 100644 --- a/backend/tests/test_storage.py +++ b/backend/tests/test_storage.py @@ -177,15 +177,29 @@ def test_sidecars_keep_the_full_basename_when_the_title_has_a_dot(tmp_path: Path assert "Chan_-_2026-08-12_-_Sonka_Hazilag.nfo" not in names # the old truncated orphan +def _mounts(*points): + """A /proc/self/mountinfo table with these mount points (the fields around them are filler).""" + return "\n".join(f"{i} 1 0:{i} / {p} rw - ext4 /dev/x rw" for i, p in enumerate(points, 22)) + + def test_a_files_root_on_the_containers_own_disk_is_refused(tmp_path): # The image's empty /files, left unmounted by an old compose file: every file would 404 and # every download would go with the container. - assert "not a mounted volume" in files_root_problem( - str(tmp_path), system_root=str(tmp_path.parent) - ) + root = str(tmp_path) + plain = _mounts("/", "/proc", "/etc/hosts") + assert "not a mounted volume" in files_root_problem(root, plain, in_container=True) assert "does not exist" in files_root_problem(str(tmp_path / "gone")) - # On another filesystem than the container's own, and writable: the volume. - assert files_root_problem(str(tmp_path), system_root="/proc") is None + # Its own mount, or inside one: the volume — whatever device it reports. On a storage driver that + # keeps the container's root on the host's disk (vfs) a volume has the same device as /. + assert files_root_problem(root, _mounts("/", root), in_container=True) is None + assert files_root_problem(root, _mounts("/", str(tmp_path.parent)), in_container=True) is None + # A mount point with a space in it, as the kernel escapes it. + spaced = tmp_path / "my files" + spaced.mkdir() + escaped = str(spaced).replace(" ", "\\040") + assert files_root_problem(str(spaced), _mounts("/", escaped), in_container=True) is None + # Outside a container a plain directory is a fine root. + assert files_root_problem(root, plain, in_container=False) is None def test_the_old_name_of_the_files_root_still_finds_the_files(monkeypatch): diff --git a/docker-compose.home.yml.example b/docker-compose.home.yml.example index ea55314f..7ac42b60 100644 --- a/docker-compose.home.yml.example +++ b/docker-compose.home.yml.example @@ -86,7 +86,7 @@ services: volumes: # Downloaded media. Defaults to a Docker-managed named volume; set FILES_HOST_PATH in .env # to a host directory (e.g. one your Plex server reads) to keep the Plex-style tree there. - - ${FILES_HOST_PATH:-siftlode_files}:/files + - ${FILES_HOST_PATH:-${DOWNLOAD_HOST_PATH:-siftlode_files}}:/files # Plex media, read-only. Only used when the optional Plex module is enabled: set # PLEX_MEDIA_HOST_PATH in .env to the host directory holding the Plex library (the local side # of plex_path_map, default /plex-media). Left unset → an empty named volume (harmless; the @@ -137,7 +137,7 @@ services: condition: service_started networks: [internal] volumes: - - ${FILES_HOST_PATH:-siftlode_files}:/files + - ${FILES_HOST_PATH:-${DOWNLOAD_HOST_PATH:-siftlode_files}}:/files # Plex media (read-only) — same mount as the api; used by Plex clip jobs (a later phase). - ${PLEX_MEDIA_HOST_PATH:-siftlode_plex_media}:/plex-media:ro mem_limit: 2g diff --git a/docker-compose.selfhost.yml b/docker-compose.selfhost.yml index fecf70a1..596266b9 100644 --- a/docker-compose.selfhost.yml +++ b/docker-compose.selfhost.yml @@ -62,7 +62,7 @@ services: volumes: # Downloaded media. Defaults to a Docker-managed named volume; set FILES_HOST_PATH in .env # to a host directory (e.g. one your Plex server reads) to keep the Plex-style tree there. - - ${FILES_HOST_PATH:-siftlode_files}:/files + - ${FILES_HOST_PATH:-${DOWNLOAD_HOST_PATH:-siftlode_files}}:/files # Plex library, read-only. The Plex module streams files itself, so it needs to SEE them: set # PLEX_MEDIA_HOST_PATH in .env to the host directory holding the library (the local side of the # admin "path map", whose default is /data=/plex-media). Left unset it falls back to an empty @@ -111,7 +111,7 @@ services: condition: service_started networks: [internal] volumes: - - ${FILES_HOST_PATH:-siftlode_files}:/files + - ${FILES_HOST_PATH:-${DOWNLOAD_HOST_PATH:-siftlode_files}}:/files # Same read-only Plex library as the API — the worker resolves library paths too. - ${PLEX_MEDIA_HOST_PATH:-siftlode_plex_media}:/plex-media:ro security_opt: diff --git a/docker-compose.yml b/docker-compose.yml index 673dfed1..af7a1e59 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -47,7 +47,7 @@ services: volumes: # Downloaded media. Defaults to a Docker-managed named volume; set FILES_HOST_PATH in .env # to a host directory (e.g. one your Plex server reads) to keep the Plex-style tree there. - - ${FILES_HOST_PATH:-files}:/files + - ${FILES_HOST_PATH:-${DOWNLOAD_HOST_PATH:-files}}:/files # Plex library, read-only. The Plex module streams files itself, so it needs to SEE them: set # PLEX_MEDIA_HOST_PATH in .env to the host directory holding the library (the local side of the # admin "path map", whose default is /data=/plex-media). Left unset it falls back to an empty @@ -91,7 +91,7 @@ services: bgutil-pot: condition: service_started volumes: - - ${FILES_HOST_PATH:-files}:/files + - ${FILES_HOST_PATH:-${DOWNLOAD_HOST_PATH:-files}}:/files # Same read-only Plex library as the API — the worker resolves library paths too. - ${PLEX_MEDIA_HOST_PATH:-plex_media}:/plex-media:ro logging: *logging diff --git a/docs/self-hosting.md b/docs/self-hosting.md index 3180bcbc..83d71d9e 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -260,7 +260,8 @@ compose file from before still mounts `/downloads` and sets `DOWNLOAD_ROOT`, and that name. To move to the new layout, download the new [`docker-compose.selfhost.yml`](../docker-compose.selfhost.yml) over your old one, then: -- **If you set a host directory**, rename `DOWNLOAD_HOST_PATH` to `FILES_HOST_PATH` in `.env`. +- **If you set a host directory**, nothing to do: the new compose file still reads + `DOWNLOAD_HOST_PATH` when `FILES_HOST_PATH` is not set. Rename it in `.env` when it suits you. - **If you used the default volume**, copy it over once before you start the new version (Docker cannot rename a volume). Compose names a volume after the project, which is your folder's name unless you set one, so look the old one up rather than guessing it: @@ -277,10 +278,15 @@ that name. To move to the new layout, download the new Open the Files library and check your files play; only then remove the old volume (`docker volume rm "$OLD"`). +- **If you build from source**, a `git pull` brings the new `docker-compose.yml`, whose default + volume is `files` (it was `downloads`): copy it over the same way, with `downloads` and `files` in + place of `siftlode_downloads` and `siftlode_files`, before you start the new version. If the files library is not mounted where the app looks, the API says so in its log at start (`The files library is not reachable`), and the download worker refuses to start rather than write -files into a container that throws them away. +files into a container that throws them away. A fresh, empty volume in the library's place (the old +one not copied over) is a real mount, so the worker runs; the API's log says that too, at start +(`The files library is empty but the database names files`). Copy the old volume over, as above. **The registry keeps only the newest few releases, not every version the changelog lists.** A pin is "decide when you take the next hop", not "stay here forever": once you are several releases behind, diff --git a/frontend/src/lib/compose.contract.test.ts b/frontend/src/lib/compose.contract.test.ts new file mode 100644 index 00000000..a2ac0415 --- /dev/null +++ b/frontend/src/lib/compose.contract.test.ts @@ -0,0 +1,23 @@ +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; + +// The compose files an install runs from. The library's host directory was renamed (DOWNLOAD_HOST_PATH +// → FILES_HOST_PATH); a `.env` that still sets the old name must keep mounting that directory, not a +// fresh empty volume the app would take for the library. Held here because this lane can read the +// whole repository; the backend's test lane sees only backend/. +const REPO = fileURLToPath(new URL("../../../", import.meta.url)); +const COMPOSE = [ + "docker-compose.yml", + "docker-compose.selfhost.yml", + "docker-compose.home.yml.example", +]; + +describe("compose files · the library mount", () => { + it.each(COMPOSE)("%s falls back to the old host-path name on every service", (file) => { + const text = readFileSync(REPO + file, "utf8"); + const mounts = text.match(/^\s*- .*:\/files\s*$/gm) ?? []; + expect(mounts.length).toBe(2); // the API and the worker + for (const m of mounts) expect(m).toContain("${FILES_HOST_PATH:-${DOWNLOAD_HOST_PATH:-"); + }); +});