diff --git a/backend/app/downloads/folders.py b/backend/app/downloads/folders.py index 4a3769ef..6908f06c 100644 --- a/backend/app/downloads/folders.py +++ b/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 = ( diff --git a/backend/app/downloads/gc.py b/backend/app/downloads/gc.py index d0041c96..32d9a83a 100644 --- a/backend/app/downloads/gc.py +++ b/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) diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 7fa7f2b2..bbe5d82c 100644 --- a/backend/app/downloads/service.py +++ b/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, diff --git a/backend/app/routes/files.py b/backend/app/routes/files.py index ca773f43..25be682b 100644 --- a/backend/app/routes/files.py +++ b/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) diff --git a/backend/tests/test_file_folders.py b/backend/tests/test_file_folders.py index 9bd40d3a..a3b71fc1 100644 --- a/backend/tests/test_file_folders.py +++ b/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). diff --git a/docs/self-hosting.md b/docs/self-hosting.md index 83d71d9e..15cc439d 100644 --- a/docs/self-hosting.md +++ b/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