fix(files): review round 3, the server — a fetched locked re-read, a finish that follows its file, an eviction that counts again

- set_retention and delete_own re-read the folder under its lock and use that row (an unfetched ORM
  result never refreshes the object); a folder gone meanwhile is refused (409), and the route audits
  the retention actually replaced
- a finishing download is stamped only while it is still in the folder (and keep) it was read with:
  a move or a folder delete landing meanwhile sends the read round again, so no file is done with no
  expiry
- the eviction pass counts the disk before each candidate and skips one asked for since it began
- docs: source builds get their own volume-copy snippet
- race tests assert the lock wait; one built on a committing delete never raced

backend/app/downloads/folders.py

@@ -211,21 +211,30 @@ def move(db: Session, job: DownloadJob, folder: FileFolder) -> None:
     restamp(db, job)
 
 
-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(
+def _hold_for_change(db: Session, folder: FileFolder) -> None:
+    """Lock `folder` FOR UPDATE and read it afresh into the same object: a change waits out a file
+    being put in it or stamped from it right now (`own_folder`, `done_expiry`), and works from what
+    another change that landed first left, not from what the caller read before the wait. The row is
+    consumed on purpose: an ORM result refreshes the object only as its rows are fetched."""
+    fresh = db.execute(
         select(FileFolder)
         .where(FileFolder.id == folder.id)
         .with_for_update()
         .execution_options(populate_existing=True)
-    )
+    ).scalar_one_or_none()
+    if fresh is None:
+        raise FolderError("file_folder_gone")
+
+
+def set_retention(db: Session, folder: FileFolder, days: int | None) -> int | 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. Returns the
+    retention it replaced (equal to `days` when nothing changed)."""
+    _hold_for_change(db, folder)  # so the file arriving now is among the ones restamped below
     old = folder.retention_days
     if old == days:
-        return
+        return old
     folder.retention_days = days
     now = datetime.now(timezone.utc)
     floor = now + timedelta(days=sysconfig.get_int(db, "download_gc_grace_days"))
@@ -248,6 +257,7 @@ def set_retention(db: Session, folder: FileFolder, days: int | None) -> None:
         else:
             job.expires_at = max(job.expires_at + timedelta(days=days - old), floor)
         job.expiry_warned = False
+    return old
 
 
 def delete_own(db: Session, folder: FileFolder) -> int:
@@ -255,8 +265,7 @@ 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())
+    _hold_for_change(db, folder)  # so the file arriving now is taken home too
     # 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 = (

backend/app/downloads/gc.py

@@ -276,8 +276,7 @@ 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 = _on_disk(db)
-        if total > cap:
+        if _on_disk(db) > cap:
             kept_assets = select(DownloadJob.asset_id).where(
                 DownloadJob.kept.is_(True), DownloadJob.asset_id.is_not(None)
             )
@@ -296,21 +295,21 @@ def run_download_gc(db: Session) -> dict:
             )
             db.rollback()
             for asset_id in candidate_ids:
-                if total <= cap:
+                # Counted afresh before each: a file deleted since (this one, an upload, a candidate
+                # further down) has freed its space, and no other user's file goes for that space.
+                if _on_disk(db) <= cap:
                     break
-                # Locked and re-read like an expiry: a holder may have kept it, or every holder
-                # deleted it since the SELECT.
+                # Locked and re-read like an expiry: since the SELECT a holder may have kept it,
+                # every holder deleted it, or someone asked for it again (a fresh cache hit).
                 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):
+                if (
+                    asset is None
+                    or asset.status != "ready"
+                    or any(j.kept for j in holders)
+                    or (asset.last_access_at is not None and asset.last_access_at > now)
+                ):
                     db.commit()
                     continue
-                total -= asset.size_bytes or 0
                 for job in holders:
                     _notify(db, job, asset, "download_evicted")
                     db.delete(job)

backend/app/downloads/service.py

@@ -421,17 +421,7 @@ def finalize_if_running(db: Session, job_id: int, **fields) -> bool:
         return False
     db.flush()  # asset-then-job lock order: push pending asset writes before we lock the job row
     if fields.get("status") == JobStatus.DONE:
-        fields = {**fields, "ready_at": datetime.now(timezone.utc)}
-        # Done is when a file's retention starts: its folder's, unless the user kept it.
-        held = db.execute(
-            select(DownloadJob.folder_id, DownloadJob.kept).where(DownloadJob.id == job_id)
-        ).one_or_none()
-        if held is not None:
-            fields = {
-                **fields,
-                "expires_at": folders.done_expiry(db, held.folder_id, held.kept),
-                "expiry_warned": False,
-            }
+        return _finalize_done(db, job_id, {**fields, "ready_at": datetime.now(timezone.utc)})
     # A Core UPDATE with the guard as a WHERE — same atomic "only while still running" write as the
     # rest of the codebase's 2.0 statements, no hand-built SQL string. synchronize_session=False: this
     # is a terminal write, so we don't pay to sync the (about-to-be-discarded) ORM identity map. Bound
@@ -445,6 +435,41 @@ def finalize_if_running(db: Session, job_id: int, **fields) -> bool:
     return result.rowcount > 0
 
 
+def _finalize_done(db: Session, job_id: int, fields: dict) -> bool:
+    """`finalize_if_running` for a job that finished: done is when a file's retention starts, its
+    folder's unless the user kept it. The expiry is written only while the job is still where it was
+    read from — a move, a keep, or its folder's delete (which sends its files home) landing while the
+    folder's retention was read sends the read round again, never stamping a folder's retention on a
+    file now in another one."""
+    while True:
+        held = db.execute(
+            select(DownloadJob.folder_id, DownloadJob.kept, DownloadJob.status).where(
+                DownloadJob.id == job_id
+            )
+        ).one_or_none()
+        if held is None or held.status != JobStatus.RUNNING:
+            return False
+        stamped = {
+            **fields,
+            "expires_at": folders.done_expiry(db, held.folder_id, held.kept),
+            "expiry_warned": False,
+        }
+        hit = db.execute(
+            update(DownloadJob)
+            .where(
+                DownloadJob.id == job_id,
+                DownloadJob.status == JobStatus.RUNNING.value,
+                DownloadJob.folder_id.is_not_distinct_from(held.folder_id),
+                DownloadJob.kept == held.kept,
+            )
+            .values(**stamped)
+            .returning(DownloadJob.id)
+            .execution_options(synchronize_session=False)
+        ).scalar_one_or_none()
+        if hit is not None:
+            return True
+
+
 def release_asset(db: Session, job: DownloadJob) -> None:
     """Drop this job's hold on its asset when it leaves a holding state. Once NO job holds the
     asset anymore, delete the file + row immediately — a deleted download should free its disk,

backend/app/routes/files.py

@@ -421,18 +421,22 @@ def update_folder(
             target_type="file_folder",
             target_id=folder.id,
         )
-    if "retention_days" in sent and body.retention_days != folder.retention_days:
-        before = folder.retention_days
-        folders.set_retention(db, folder, body.retention_days)
-        audit.record(
-            db,
-            user.id,
-            AuditAction.FILE_FOLDER_RETENTION,
-            target_type="file_folder",
-            target_id=folder.id,
-            before={"retention_days": before},
-            after={"retention_days": body.retention_days},
-        )
+    if "retention_days" in sent:
+        # The value replaced is the one read under the folder's lock, not the one read above it.
+        try:
+            before = folders.set_retention(db, folder, body.retention_days)
+        except folders.FolderError as e:
+            raise refused(409, e.reason) from e
+        if before != body.retention_days:
+            audit.record(
+                db,
+                user.id,
+                AuditAction.FILE_FOLDER_RETENTION,
+                target_type="file_folder",
+                target_id=folder.id,
+                before={"retention_days": before},
+                after={"retention_days": body.retention_days},
+            )
     db.commit()
     count, size = _folder_totals(db, folder.id)
     return _serialize_folder(folder, count, size)

backend/tests/test_file_folders.py

@@ -501,10 +501,11 @@ def test_a_retention_changed_while_a_file_goes_in_times_that_file_too(db, make_u
         lambda other: folders.set_retention(other, other.get(FileFolder, own_id), None)
     )
     t.join(timeout=1)
+    assert t.is_alive()  # the change waits for the file being put in
     folders.move(db, job, target)  # stamped with the 7 days it sees
     db.commit()
     t.join(timeout=10)
-    assert outcome == [None]
+    assert outcome == [7]  # the retention it replaced
     db.expire_all()
     assert db.get(DownloadJob, job.id).expires_at is None
 
@@ -527,13 +528,21 @@ def test_a_retention_changed_while_a_file_finishes_there_times_that_file_too(db,
         lambda other: folders.set_retention(other, other.get(FileFolder, own_id), None)
     )
     t.join(timeout=1)
+    assert t.is_alive()  # the change waits for the file being stamped
     db.commit()
     t.join(timeout=10)
-    assert outcome == [None]
+    assert outcome == [7]  # the retention it replaced
     db.expire_all()
     assert db.get(DownloadJob, job.id).expires_at is None
 
 
+def _deleted_uncommitted(db, job):
+    """`service.delete` up to its commit: the file is gone in this session, its row still locked."""
+    service.release_asset(db, job)
+    db.delete(job)
+    db.flush()
+
+
 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")
@@ -566,10 +575,10 @@ def test_a_file_deleted_while_its_folder_is_deleted_does_not_fail_the_delete(db,
     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
+    _deleted_uncommitted(db, db.get(DownloadJob, job.id))
     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 being deleted
     db.commit()
     t.join(timeout=10)
     assert outcome == [0]
@@ -577,6 +586,158 @@ def test_a_file_deleted_while_its_folder_is_deleted_does_not_fail_the_delete(db,
     assert db.get(FileFolder, f_id) is None
 
 
+def _own_with_a_file(db, uid, retention=30):
+    """A user's own folder keeping files `retention` days, holding one done file due in as many."""
+    own = FileFolder(
+        user_id=uid, kind=folders.FolderKind.OWN, name="Mine", retention_days=retention
+    )
+    db.add(own)
+    db.flush()
+    job = _job(db, uid, _asset(db))
+    job.folder_id = own.id
+    job.expires_at = NOW + timedelta(days=retention)
+    db.commit()
+    return own.id, job.id
+
+
+def _elsewhere(act):
+    other = SessionLocal()
+    try:
+        act(other)
+        other.commit()
+    finally:
+        other.close()
+
+
+def test_a_retention_change_works_from_what_a_change_before_it_left(db, make_user):
+    # The locked re-read was never fetched, so the change shifted the files from the value read before
+    # its wait: 30 -> 10 -> 60 left them due in 40 days under a folder saying 60.
+    own_id, job_id = _own_with_a_file(db, make_user().id)
+    stale = db.get(FileFolder, own_id)  # read before the other change lands
+    _elsewhere(lambda other: folders.set_retention(other, other.get(FileFolder, own_id), 10))
+    assert folders.set_retention(db, stale, 60) == 10
+    db.commit()
+    db.expire_all()
+    due = db.get(DownloadJob, job_id).expires_at - NOW
+    assert timedelta(days=59) < due < timedelta(days=61)
+
+
+def test_a_retention_change_to_a_folder_deleted_meanwhile_is_refused(db, make_user):
+    own_id, _ = _own_with_a_file(db, make_user().id)
+    stale = db.get(FileFolder, own_id)
+    _elsewhere(lambda other: folders.delete_own(other, other.get(FileFolder, own_id)))
+    with pytest.raises(folders.FolderError, match="file_folder_gone"):
+        folders.set_retention(db, stale, 60)
+
+
+def test_a_file_moved_out_while_its_folder_retention_changes_keeps_its_new_folders(db, make_user):
+    uid = make_user().id
+    own_id, job_id = _own_with_a_file(db, uid)
+    g = FileFolder(user_id=uid, kind=folders.FolderKind.OWN, name="G", retention_days=7)
+    db.add(g)
+    db.commit()
+    g_id = g.id
+    folders.move(db, db.get(DownloadJob, job_id), folders.own_folder(db, uid, g_id))
+    db.flush()  # the move is written, not yet committed
+    t, outcome = _in_thread(
+        lambda other: folders.set_retention(other, other.get(FileFolder, own_id), None)
+    )
+    t.join(timeout=1)
+    assert t.is_alive()  # the change waits for the file it would time
+    db.commit()
+    t.join(timeout=10)
+    assert outcome == [30]
+    db.expire_all()
+    job = db.get(DownloadJob, job_id)
+    assert job.folder_id == g_id
+    assert job.expires_at is not None  # G's 7 days, not the forever of the folder it left
+
+
+def test_a_file_deleted_while_its_folder_retention_changes_does_not_fail_the_change(
+    db, make_user, _root
+):
+    (_root / "a.mp4").write_bytes(b"x")
+    own_id, job_id = _own_with_a_file(db, make_user().id)
+    _deleted_uncommitted(db, db.get(DownloadJob, job_id))
+    t, outcome = _in_thread(
+        lambda other: folders.set_retention(other, other.get(FileFolder, own_id), None)
+    )
+    t.join(timeout=1)
+    assert t.is_alive()  # the change waits for the file it would time
+    db.commit()
+    t.join(timeout=10)
+    assert outcome == [30]
+
+
+def test_a_file_finishing_while_its_folder_is_deleted_gets_its_new_folders_retention(db, make_user):
+    # The finish read the folder unlocked, then waited out the delete and found no folder: the file
+    # was done with no expiry at all, in a source folder that keeps files 30 days.
+    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)
+    job_id = job.id
+    folders.move(db, job, own)
+    db.commit()
+    folders.delete_own(db, db.get(FileFolder, own_id))
+    db.flush()  # the delete, not yet committed
+    t, outcome = _in_thread(
+        lambda other: service.finalize_if_running(other, job_id, status=JobStatus.DONE)
+    )
+    t.join(timeout=1)
+    assert t.is_alive()  # the finish waits for the delete
+    db.commit()
+    t.join(timeout=10)
+    assert outcome == [True]
+    db.expire_all()
+    done = db.get(DownloadJob, job_id)
+    assert done.folder_id != own_id
+    assert done.expires_at is not None
+
+
+def _an_upload_over_the_cap_too(db, uid, _root):
+    (_root / "u.bin").write_bytes(b"x")
+    return _job(db, uid, _asset(db, "upload", "u1", rel="u.bin", size=10), kind="upload")
+
+
+def test_space_freed_elsewhere_while_the_cache_is_over_its_cap_evicts_no_more(
+    db, make_user, _root, monkeypatch
+):
+    # The pass counted the disk once: an upload deleted meanwhile still counted, so a second user's
+    # file went for space already free.
+    jold, jnew = _two_over_the_cap(db, make_user, _root)
+    up = _an_upload_over_the_cap_too(db, make_user().id, _root)
+
+    def upload_deleted(other, asset_id):
+        if asset_id == jold.asset_id:
+            service.delete(other, other.get(DownloadJob, up.id))
+
+    _meanwhile(monkeypatch, upload_deleted, step="_relock_evictable")
+    assert _gc(db)["evicted"] == 1
+    db.expire_all()
+    assert db.get(DownloadJob, jnew.id) is not None
+    assert (_root / "new.mp4").exists()
+
+
+def test_a_file_asked_for_again_while_the_cache_is_over_its_cap_stays(
+    db, make_user, _root, monkeypatch
+):
+    # A cache hit between the pass's SELECT and its lock: the file the user just asked for went.
+    jold, jnew = _two_over_the_cap(db, make_user, _root)
+
+    def asked_again(other, asset_id):
+        if asset_id == jold.asset_id:
+            other.get(MediaAsset, asset_id).last_access_at = datetime.now(timezone.utc)
+
+    _meanwhile(monkeypatch, asked_again, step="_relock_evictable")
+    _gc(db)
+    db.expire_all()
+    assert db.get(DownloadJob, jold.id) is not None
+    assert (_root / "old.mp4").exists()
+
+
 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).

docs/self-hosting.md

@@ -279,8 +279,18 @@ 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.
+  volume is `files` (it was `downloads`). Copy it over before you start the new version — with your
+  own compose file, not the self-host one above, whose volumes are other ones:
+
+  ```bash
+  docker compose down
+  docker volume ls --format '{{.Name}}' | grep '_downloads$'   # e.g. siftlode_downloads
+  OLD=siftlode_downloads                                         # the name it printed
+  NEW="${OLD%downloads}files"
+  docker volume create "$NEW"
+  docker run --rm -v "$OLD":/from -v "$NEW":/to alpine cp -a /from/. /to/
+  docker compose up -d --build
+  ```
 
 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