fix(downloads): a converted upload too big says so, and a suspended owner's waiting rebuild is not "preparing"
- A converted upload over the space left (not zero) fails with CONVERTED_TOO_BIG_MESSAGE, not "storage is full"; it classifies as quota_exceeded by the phrase it shares with the clip's. - The public watch page says "preparing" only for a job a worker will take: a suspended owner's queued job waits for as long as the suspension lasts, so the link says the video is gone, as for a paused rebuild. - The worker's claim docstring names the restart case: a running job put back in the queue waits.
This commit is contained in:
@@ -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`.
|
# 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_FULL_MESSAGE, CLIP_TOO_BIG_MESSAGE and CONVERTED_TOO_BIG_MESSAGE are raised by the worker and
|
||||||
# `quota_exceeded`; their rules below match a fragment of each, and test_download_errors pins the pair, because a message and its
|
# must classify back to `quota_exceeded`; their rules below match a fragment of each, and
|
||||||
# rule drifting apart is exactly how a category silently becomes unreachable.
|
# 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
|
# 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
|
# 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
|
# constants, imported above; each has a rule below that gives its category back, and a catalog entry
|
||||||
# that gives the site's sentence.
|
# that gives the site's sentence.
|
||||||
QUOTA_FULL_MESSAGE = "Your download storage is full — free up space, then try again."
|
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 = (
|
CLIP_TOO_BIG_MESSAGE = (
|
||||||
"The clip is larger than the download storage you have left — "
|
"The clip is larger than the download storage you have left — "
|
||||||
"free up space or cut a shorter clip, then try again."
|
"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."
|
UPLOAD_MISSING_MESSAGE = "The uploaded file is missing."
|
||||||
NETWORK_REDACTED_MESSAGE = "The server could not reach that address."
|
NETWORK_REDACTED_MESSAGE = "The server could not reach that address."
|
||||||
|
|
||||||
|
|||||||
@@ -1160,7 +1160,7 @@ def _check_converted_fits(owner_id: int | None, old_size: int, new_size: int) ->
|
|||||||
return
|
return
|
||||||
with SessionLocal() as db:
|
with SessionLocal() as db:
|
||||||
if not quota.fits_after_conversion(db, owner_id, old_size, new_size):
|
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:
|
def _unplace(placed: Path | None) -> None:
|
||||||
|
|||||||
@@ -17,7 +17,7 @@ from app.db import get_db
|
|||||||
from app.downloads import links as linksmod
|
from app.downloads import links as linksmod
|
||||||
from app.downloads import storage
|
from app.downloads import storage
|
||||||
from app.downloads.status import JobStatus
|
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.ratelimit import RateLimiter, client_key
|
||||||
from app.security import verify_password
|
from app.security import verify_password
|
||||||
from app.refusal import refused
|
from app.refusal import refused
|
||||||
@@ -44,6 +44,17 @@ def _link_or_404(db: Session, token: str) -> DownloadLink:
|
|||||||
return link
|
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]:
|
def _ready_target(db: Session, link: DownloadLink) -> tuple[DownloadJob, MediaAsset]:
|
||||||
job = db.get(DownloadJob, link.job_id)
|
job = db.get(DownloadJob, link.job_id)
|
||||||
asset = db.get(MediaAsset, job.asset_id) if job and job.asset_id else None
|
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
|
job is not None
|
||||||
and asset is not None
|
and asset is not None
|
||||||
and asset.status in ("pending", "downloading")
|
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:
|
# 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".
|
# its own reason, so the page can say "try again shortly" rather than "this link is gone".
|
||||||
|
|||||||
@@ -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
|
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
|
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:
|
with SessionLocal() as db:
|
||||||
default_mc = sysconfig.get_int(db, "download_default_max_concurrent")
|
default_mc = sysconfig.get_int(db, "download_default_max_concurrent")
|
||||||
rows = db.execute(
|
rows = db.execute(
|
||||||
|
|||||||
@@ -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".
|
# 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.QUOTA_FULL_MESSAGE) == "quota_exceeded"
|
||||||
assert classify(dlerrors.CLIP_TOO_BIG_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():
|
def test_the_redacted_network_text_classifies_back_to_network():
|
||||||
|
|||||||
@@ -15,6 +15,7 @@ import pytest
|
|||||||
|
|
||||||
from app import sysconfig
|
from app import sysconfig
|
||||||
from app.config import settings
|
from app.config import settings
|
||||||
|
from app.downloads import errors as dlerrors
|
||||||
from app.downloads import gc as gcmod
|
from app.downloads import gc as gcmod
|
||||||
from app.downloads import pipeline, quota, storage
|
from app.downloads import pipeline, quota, storage
|
||||||
from app.downloads import probe as probemod
|
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()
|
db.expire_all()
|
||||||
job = db.get(DownloadJob, job.id)
|
job = db.get(DownloadJob, job.id)
|
||||||
assert (job.status, job.error_code) == ("error", "quota_exceeded")
|
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 original.exists()
|
||||||
assert [p.name for p in _uploads_dir(_root, u.id).iterdir()] == [original.name]
|
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
|
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):
|
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`).
|
# The same disk the owner's footprint and the GC's cap count (`quota.ON_DISK`).
|
||||||
_probe(monkeypatch)
|
_probe(monkeypatch)
|
||||||
|
|||||||
Reference in New Issue
Block a user