fix(files): review round 2, the server — locks the GC and folders read under, the mount check, the old mount name
- GC: the expiry warning marks each file conditionally on the expiry it saw, one commit each (a file deleted meanwhile no longer fails the run; one re-timed keeps its fresh expiry unwarned); eviction counts the disk again when a candidate was freed meanwhile, so it never evicts for space already free. - Folders: a retention change locks the folder and its files, and a finishing file reads the retention under a share lock, so neither misses the other; a folder delete locks the files it takes home (a file moved out meanwhile stays put, one deleted meanwhile no longer fails the delete). - Upload: the folder is checked before the probe and held only once the job names it. - Dedup enqueue locks the cached asset, so a release meanwhile yields a fresh one instead of a 500. - Mount check reads the kernel's mount table instead of comparing devices (the vfs storage driver gave a mounted volume the device of /), and only inside a container; the API also logs a fresh, empty volume in the library's place. - Compose files fall back to DOWNLOAD_HOST_PATH; the upgrade notes cover the from-source pull. - Folder routes call owned_or_404 directly.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
):
|
||||
|
||||
@@ -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):
|
||||
|
||||
Reference in New Issue
Block a user