diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index e336d871..2289e647 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -37,9 +37,10 @@ from app.downloads.credentials import host_key_for_url # Worker-authored strings that this module owns because each is coupled to `_RULES`. # -# QUOTA_FULL_MESSAGE and CLIP_TOO_BIG_MESSAGE are raised by the worker and must classify back to -# `quota_exceeded`; their rules below match a fragment of each, and test_download_errors pins the pair, because a message and its -# rule drifting apart is exactly how a category silently becomes unreachable. +# QUOTA_FULL_MESSAGE, CLIP_TOO_BIG_MESSAGE and CONVERTED_TOO_BIG_MESSAGE are raised by the worker and +# must classify back to `quota_exceeded`; their rules below match a fragment of each, and +# test_download_errors pins each one, because a message and its rule drifting apart is exactly how a +# category silently becomes unreachable. # # NETWORK_REDACTED_MESSAGE REPLACES a message after classification (`user_error_text`). It is written # to the shared asset too, and a second job that inherits that failed asset re-diagnoses the stored @@ -53,11 +54,16 @@ from app.downloads.credentials import host_key_for_url # constants, imported above; each has a rule below that gives its category back, and a catalog entry # that gives the site's sentence. QUOTA_FULL_MESSAGE = "Your download storage is full — free up space, then try again." -# A cut clip held to the space left, which is not zero: "full" would be untrue. +# A cut clip, or an upload's converted file, held to the space left, which is not zero: "full" +# would be untrue. Both classify by the one phrase they share. CLIP_TOO_BIG_MESSAGE = ( "The clip is larger than the download storage you have left — " "free up space or cut a shorter clip, then try again." ) +CONVERTED_TOO_BIG_MESSAGE = ( + "The converted file is larger than the download storage you have left — " + "free up space, then try again." +) UPLOAD_MISSING_MESSAGE = "The uploaded file is missing." NETWORK_REDACTED_MESSAGE = "The server could not reach that address." diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 28baf133..d995e394 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -1160,7 +1160,7 @@ def _check_converted_fits(owner_id: int | None, old_size: int, new_size: int) -> return with SessionLocal() as db: if not quota.fits_after_conversion(db, owner_id, old_size, new_size): - raise StatedFailure(dlerrors.QUOTA_FULL_MESSAGE) + raise StatedFailure(dlerrors.CONVERTED_TOO_BIG_MESSAGE) def _unplace(placed: Path | None) -> None: diff --git a/backend/app/routes/public.py b/backend/app/routes/public.py index fcf0d575..42099a93 100644 --- a/backend/app/routes/public.py +++ b/backend/app/routes/public.py @@ -17,7 +17,7 @@ from app.db import get_db from app.downloads import links as linksmod from app.downloads import storage from app.downloads.status import JobStatus -from app.models import DownloadJob, DownloadLink, MediaAsset +from app.models import DownloadJob, DownloadLink, MediaAsset, User from app.ratelimit import RateLimiter, client_key from app.security import verify_password from app.refusal import refused @@ -44,6 +44,17 @@ def _link_or_404(db: Session, token: str) -> DownloadLink: return link +def _being_made(db: Session, job: DownloadJob) -> bool: + """The job is running, or queued for a worker that will take it: the worker leaves a suspended + owner's queued jobs where they are (`worker._claim_job`), as long as the suspension lasts.""" + if job.status == JobStatus.RUNNING: + return True + if job.status != JobStatus.QUEUED: + return False + owner = db.get(User, job.user_id) + return owner is not None and not owner.is_suspended + + def _ready_target(db: Session, link: DownloadLink) -> tuple[DownloadJob, MediaAsset]: job = db.get(DownloadJob, link.job_id) asset = db.get(MediaAsset, job.asset_id) if job and job.asset_id else None @@ -51,7 +62,7 @@ def _ready_target(db: Session, link: DownloadLink) -> tuple[DownloadJob, MediaAs job is not None and asset is not None and asset.status in ("pending", "downloading") - and job.status in (JobStatus.QUEUED, JobStatus.RUNNING) + and _being_made(db, job) ): # Being (re)made — a re-download, or a conversion — so the link works again once it is ready: # its own reason, so the page can say "try again shortly" rather than "this link is gone". diff --git a/backend/app/worker.py b/backend/app/worker.py index d185a01a..dbff8e35 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -167,7 +167,8 @@ def _claim_job() -> int | None: A suspended user's queued jobs are left where they are: suspension blocks their downloads (not only their sign-in), and lifting it lets them run in their old place. A job already running - when the user is suspended finishes.""" + when the user is suspended finishes on that worker; one a worker restart puts back in the queue + (`_requeue_if_running`, `_recover_orphans`) waits with the rest.""" with SessionLocal() as db: default_mc = sysconfig.get_int(db, "download_default_max_concurrent") rows = db.execute( diff --git a/backend/tests/test_download_errors.py b/backend/tests/test_download_errors.py index 6b6e0e5d..9451d3df 100644 --- a/backend/tests/test_download_errors.py +++ b/backend/tests/test_download_errors.py @@ -64,6 +64,7 @@ def test_our_own_worker_refusals_classify_as_themselves(): # from silently orphaning the category — the UI would then fall back to "Download failed". assert classify(dlerrors.QUOTA_FULL_MESSAGE) == "quota_exceeded" assert classify(dlerrors.CLIP_TOO_BIG_MESSAGE) == "quota_exceeded" + assert classify(dlerrors.CONVERTED_TOO_BIG_MESSAGE) == "quota_exceeded" def test_the_redacted_network_text_classifies_back_to_network(): diff --git a/backend/tests/test_upload_conversion.py b/backend/tests/test_upload_conversion.py index 2a33bea5..8a8407f0 100644 --- a/backend/tests/test_upload_conversion.py +++ b/backend/tests/test_upload_conversion.py @@ -15,6 +15,7 @@ import pytest from app import sysconfig from app.config import settings +from app.downloads import errors as dlerrors from app.downloads import gc as gcmod from app.downloads import pipeline, quota, storage from app.downloads import probe as probemod @@ -124,6 +125,8 @@ def test_a_conversion_that_outgrows_the_cap_is_refused_and_keeps_the_original( db.expire_all() job = db.get(DownloadJob, job.id) assert (job.status, job.error_code) == ("error", "quota_exceeded") + # 90 of the 100 are still free: the converted file is too big, the storage is not full. + assert job.error == dlerrors.CONVERTED_TOO_BIG_MESSAGE assert original.exists() assert [p.name for p in _uploads_dir(_root, u.id).iterdir()] == [original.name] @@ -558,6 +561,33 @@ def test_a_link_says_preparing_only_while_its_video_is_being_made( assert r.json()["detail"]["reason"] == reason +def test_a_link_to_a_suspended_owners_waiting_rebuild_says_it_is_gone( + db, as_user, make_user, _root, monkeypatch, upload_headers +): + # The worker leaves a suspended user's queued jobs where they are, maybe for good: like a paused + # rebuild, nothing is making the video, so "try again shortly" would never come true. + _probe(monkeypatch, vcodec="h264") + owner = make_user() + c = as_user(owner) + job = c.post( + "/api/downloads/upload", content=b"h264 bytes", headers=upload_headers("clip.mp4") + ).json() + link = c.post(f"/api/downloads/{job['id']}/links", json={"allow_download": False}).json() + db.get(MediaAsset, job["asset"]["id"]).status = "pending" + db.get(DownloadJob, job["id"]).status = "queued" + owner.is_suspended = True + db.commit() + r = c.get(f"/api/public/watch/{link['token']}") + assert r.status_code == 404 + assert r.json()["detail"]["reason"] == "video_gone" + # A rebuild already running when the owner was suspended still finishes. + db.get(DownloadJob, job["id"]).status = "running" + db.commit() + assert c.get(f"/api/public/watch/{link['token']}").json()["detail"]["reason"] == ( + "video_preparing" + ) + + def test_the_admin_storage_view_counts_a_waiting_upload(db, as_user, make_user, _root, monkeypatch): # The same disk the owner's footprint and the GC's cap count (`quota.ON_DISK`). _probe(monkeypatch)