fix(files): review round 1, the server — the tray's window, the GC's locks, a file's origin, the upgrade path

- The queue keeps a done job by when it became done (ready_at), not by its last write: renaming, keeping or moving an old file, or the GC's expiry warning, no longer brings it back as just finished. - GC expiry and eviction re-read each file under the asset→job locks the user routes take, and commit one file at a time: a delete, a re-download or a keep made while the GC runs is honoured, never a second release that frees a file another user still holds. - A file records the origin it was placed under (download_jobs.source_key, migration 0105): an edit whose parent expired still goes back to its folder, not to Uploads. - A folder a file is being put in is held (FOR SHARE) until it lands; a delete of it waits and takes that file home too, instead of failing on the foreign key. - The hash route reads at most two files at once (429 past that) and holds no connection meanwhile. - 0104's backfill dates old files by their request date only: no notice prod wrote names its job. - The old DOWNLOAD_ROOT still finds the files; a files root that is not a mounted volume stops the worker and is logged by the API; the volume-copy recipe looks the project-prefixed names up. - Stale comments about the /downloads mount and the Downloads page.
2026-10-13 00:14:09 +02:00
parent 96509fe356
commit 815083bcc4
29 changed files with 593 additions and 92 deletions
@@ -68,9 +68,11 @@ def site_key(url: str) -> str:
 def source_key_for(db: Session, job: DownloadJob) -> str:
    """The origin a job's file came from — the source folder it belongs in.

-    youtube → YouTube; a clip → the clip's source (YouTube or Plex); an upload → Uploads; a URL →
-    its site; an edit → wherever its parent's file came from (its parent job, or, once that is
-    deleted, the parent's asset)."""
+    The one recorded when it was placed, else: youtube → YouTube; a clip → the clip's source
+    (YouTube or Plex); an upload → Uploads; a URL → its site; an edit → wherever its parent's file
+    came from (its parent job, or, once that is deleted, the parent's asset)."""
+    if job.source_key:
+        return job.source_key
    kind = job.source_kind
    if kind == "youtube":
        return SourceKey.YOUTUBE
@@ -122,9 +124,11 @@ def source_folder(db: Session, user_id: int, key: str) -> FileFolder:


 def place(db: Session, job: DownloadJob) -> None:
-    """Put a new job's file in its source folder. Called wherever a job is created; the job must
-    carry its `user_id`, `source_kind` and source refs already."""
-    job.folder_id = source_folder(db, job.user_id, source_key_for(db, job)).id
+    """Put a new job's file in its source folder, and record that origin on it. Called wherever a
+    job is created; the job must carry its `user_id`, `source_kind` and source refs already."""
+    key = source_key_for(db, job)
+    job.source_key = key
+    job.folder_id = source_folder(db, job.user_id, key).id


 def done_expiry(db: Session, folder_id: int | None, kept: bool) -> datetime | None:
@@ -151,12 +155,25 @@ class FolderError(Exception):
        self.reason = reason


-def upload_target(db: Session, user_id: int, folder_id: int) -> FileFolder:
-    """The folder an upload asked to land in, if it may: one of the user's own folders, or their
-    Uploads folder. A file put in a site's folder would claim an origin it does not have."""
-    folder = db.get(FileFolder, folder_id)
+def own_folder(db: Session, user_id: int, folder_id: int, *, hold: bool = True) -> FileFolder:
+    """The user's folder `folder_id` that a file is about to be put in — held (FOR SHARE) until the
+    caller's transaction ends, so a delete of the folder waits for the file and then takes it home
+    with the rest (`delete_own` locks it FOR UPDATE), never failing on its foreign key. `hold=False`
+    only looks (a check ahead of a long wait, which must not hold a lock through it). Another user's
+    folder answers like a missing one."""
+    stmt = select(FileFolder).where(FileFolder.id == folder_id)
+    if hold:
+        stmt = stmt.with_for_update(read=True)
+    folder = db.execute(stmt.execution_options(populate_existing=True)).scalar_one_or_none()
    if folder is None or folder.user_id != user_id:
        raise FolderError("file_folder_gone")
+    return folder
+
+
+def upload_target(db: Session, user_id: int, folder_id: int, *, hold: bool = True) -> FileFolder:
+    """The folder an upload asked to land in, if it may: one of the user's own folders, or their
+    Uploads folder. A file put in a site's folder would claim an origin it does not have."""
+    folder = own_folder(db, user_id, folder_id, hold=hold)
    if folder.kind == FolderKind.SOURCE and folder.source_key != SourceKey.UPLOAD:
        raise FolderError("file_folder_not_its_source")
    return folder
@@ -219,6 +236,8 @@ def delete_own(db: Session, folder: FileFolder) -> int:
    folder it came from, its clock starting there now. Returns how many moved."""
    if folder.kind != FolderKind.OWN:
        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()
    for job in jobs:
        job.folder_id = source_folder(db, job.user_id, source_key_for(db, job)).id
@@ -56,6 +56,32 @@ def _notify(db: Session, job: DownloadJob, asset: MediaAsset | None, ntype: str)
    )


+def _relock_expired(
+    db: Session, job_id: int, asset_id: int | None, now: datetime
+) -> DownloadJob | None:
+    """Re-read an expired job under the lock order the user routes take (`own_job_for_update`: the
+    asset, then the job) and say whether it is still to go. Between the pass's SELECT and here a
+    user may have deleted or re-downloaded it (dropping it again would release its asset a second
+    time, freeing a file another job still holds) or kept or moved it (it is no longer expired)."""
+    if asset_id is not None:
+        db.execute(select(MediaAsset.id).where(MediaAsset.id == asset_id).with_for_update())
+    job = db.execute(
+        select(DownloadJob)
+        .where(DownloadJob.id == job_id)
+        .with_for_update()
+        .execution_options(populate_existing=True)
+    ).scalar_one_or_none()
+    still = (
+        job is not None
+        and job.asset_id == asset_id
+        and job.status == JobStatus.DONE
+        and not job.kept
+        and job.expires_at is not None
+        and job.expires_at <= now
+    )
+    return job if still else None
+
+
 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,22 +202,22 @@ def run_download_gc(db: Session) -> dict:
        db.commit()

    # 2. Expiry — a file past its folder's retention leaves the library (warned in pass 1).
-    dead = (
-        db.execute(
-            select(DownloadJob).where(
-                DownloadJob.status == JobStatus.DONE,
-                DownloadJob.kept.is_(False),
-                DownloadJob.expires_at.is_not(None),
-                DownloadJob.expires_at <= now,
-            )
+    #    Each file is re-checked under its locks and committed on its own: the release unlinks the
+    #    file at once, so it must never act on what the SELECT saw a while ago.
+    dead = db.execute(
+        select(DownloadJob.id, DownloadJob.asset_id).where(
+            DownloadJob.status == JobStatus.DONE,
+            DownloadJob.kept.is_(False),
+            DownloadJob.expires_at.is_not(None),
+            DownloadJob.expires_at <= now,
        )
-        .scalars()
-        .all()
-    )
-    for job in dead:
-        _drop_job(db, job, "download_expired")
-        expired += 1
-    if dead:
+    ).all()
+    db.rollback()  # the pass-2 snapshot holds no locks; each file takes its own below
+    for job_id, asset_id in dead:
+        job = _relock_expired(db, job_id, asset_id, now)
+        if job is not None:
+            _drop_job(db, job, "download_expired")
+            expired += 1
        db.commit()

    # 3. LRU eviction over the total cache cap: the disk's safety net. Never an upload (no source to
@@ -221,15 +247,33 @@ def run_download_gc(db: Session) -> dict:
                .scalars()
                .all()
            )
-            for asset in candidates:
+            candidate_ids = [a.id for a in candidates]
+            db.rollback()
+            for asset_id in candidate_ids:
                if total <= cap:
                    break
-                total -= asset.size_bytes or 0
+                # 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))
+                    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):
+                    db.commit()
+                    continue
+                total -= asset.size_bytes or 0
                for job in holders:
                    _notify(db, job, asset, "download_evicted")
                    db.delete(job)
@@ -237,7 +281,7 @@ def run_download_gc(db: Session) -> dict:
                    storage.delete_asset_files(settings.files_root, asset.rel_path)
                db.delete(asset)
                evicted += 1
-            db.commit()
+                db.commit()

    # 4. Reap orphaned errored rows (no file, no holder). get_or_create_asset resets a still-
    #    referenced errored row to pending on re-enqueue, so these only accumulate once every
@@ -49,7 +49,7 @@ def source_url(source_kind: str, source_ref: str) -> str:


 def reference_url(job, asset) -> str | None:
-    """The clean "downloaded from" URL for a job — shared by the private Downloads page and the public
+    """The clean "downloaded from" URL for a job — shared by the Files library and the public
    watch page so both surfaces always agree. None for edit derivatives (a clip's source is the user's
    own earlier download, not a web page) and for a Plex clip (the library is not a public page); a
    YouTube clip links its video at the clip's start; else the canonical page URL yt-dlp recorded; else
@@ -11,6 +11,7 @@ a Plex library using a metadata agent picks up rich info. Channel = "show", vide
 date-based — the community-standard mapping for YouTube-in-Plex.
 """

+import os
 import re
 import shutil
 import unicodedata
@@ -67,6 +68,25 @@ _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."""
+    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 not os.access(path, os.W_OK):
+        return f"{root} is not writable"
+    return None
+
+
 def _ascii_fold(text: str) -> str:
    """Transliterate to pure ASCII: apply the explicit map, NFKD-split accented letters and drop
    the combining marks (á→a, ő→o), then strip any leftover non-ASCII (non-Latin scripts that have