From 68cfc7e25dd3eeee0130562f5bd48b69042515a2 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 6 Oct 2026 03:19:32 +0200 Subject: [PATCH] fix(downloads): an upload's conversion is guarded, counted, named and cleaned up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Every ffmpeg/ffprobe over a library file names the demuxers it may use (edit.INPUT_GUARD; a downloaded thumbnail edit.THUMB_GUARD): a playlist-type demuxer would read local paths named inside a user's upload. - A re-encode ends on an even frame (edit.EVEN_FRAME): an odd 4:4:4/RGB source failed its conversion; a clip stores the frame its file has. - An upload counts against its owner's cap from the moment it lands (queued, or kept after a failed conversion) — the footprint, the GC's cap and the admin view read one ON_DISK predicate — and a conversion that outgrows the cap is refused, keeping the original. - The converted name keeps _a{id} (a dotted bare name once lost it and two uploads shared a file); a converted file placed but not committed is removed, and the orphan sweep enters .uploads; the LRU cap never evicts an upload. - Ingest judges the extension the file is stored under; a failed conversion is convert_failed / upload_missing, reports the "converting" phase, and notifies as an upload. Job statuses in routes/downloads use JobStatus. --- backend/app/downloads/edit.py | 62 ++- backend/app/downloads/errors.py | 12 + backend/app/downloads/gc.py | 50 +-- backend/app/downloads/pipeline.py | 62 ++- backend/app/downloads/playable.py | 6 +- backend/app/downloads/quota.py | 43 ++- backend/app/downloads/storage.py | 16 +- backend/app/downloads/upload.py | 15 +- backend/app/models.py | 3 +- backend/app/routes/downloads.py | 26 +- backend/tests/test_ffmpeg_input_guard.py | 163 ++++++++ backend/tests/test_odd_frame.py | 73 ++++ backend/tests/test_upload_conversion.py | 355 ++++++++++++++++++ backend/tests/test_worker_dispatch.py | 74 ++++ .../src/components/{ => ui}/CrashBoundary.tsx | 0 15 files changed, 892 insertions(+), 68 deletions(-) create mode 100644 backend/tests/test_ffmpeg_input_guard.py create mode 100644 backend/tests/test_odd_frame.py create mode 100644 backend/tests/test_upload_conversion.py create mode 100644 backend/tests/test_worker_dispatch.py rename frontend/src/components/{ => ui}/CrashBoundary.tsx (100%) diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 38c90f5d..f340ff04 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -194,6 +194,44 @@ def clip_duration(spec: dict, source_duration: int | None) -> int | None: # only patchily (the one safe H.264 is 8-bit 4:2:0). X264_ARGS = ["-c:v", "libx264", "-preset", "veryfast", "-crf", "20", "-pix_fmt", "yuv420p"] AAC_ARGS = ["-c:a", "aac", "-b:a", "192k"] +# The last step of every video filter chain a libx264 re-encode runs: 4:2:0 halves the chroma, so +# libx264 refuses an odd width or height ("width not divisible by 2") — measured on an odd source +# that is not 4:2:0 already (a 4:4:4 or RGB screen capture, 1365x767). A crop of a 4:2:0 source is +# rounded to its chroma grid by ffmpeg itself; this holds every other path to the encoder to the same +# even frame. Rounding down to even costs at most one pixel row/column. +EVEN_FRAME = "scale=trunc(iw/2)*2:trunc(ih/2)*2" + +# The demuxers ffmpeg/ffprobe may use to open a library file — single-file containers only. Without +# it ffmpeg picks the demuxer from the file's CONTENT and NAME, and an upload's name is the user's: +# a playlist-type demuxer (HLS, DASH, concat) opens whatever local paths its entries name, so a +# crafted upload could copy another user's clip or a Plex file into its own converted asset. A file +# no demuxer here can open fails the probe, and an unprobeable upload is a download-only 'file'. +MEDIA_DEMUXERS = ( + "mov", # mp4, m4a, mov, 3gp + "matroska", # mkv, webm + "mpegts", + "mpeg", + "avi", + "asf", + "flv", + "ogg", + "mp3", + "aac", + "ac3", + "eac3", + "flac", + "wav", + "w64", + "aiff", + "caf", + "amr", +) +INPUT_GUARD = ["-format_whitelist", ",".join(MEDIA_DEMUXERS)] +# A downloaded thumbnail is the remote site's bytes under the suffix yt-dlp gave it (`.png`, `.webp`, +# `.jpg` — `pipeline._THUMB_SUFFIXES`): only the single-image demuxers those open, so a "thumbnail" +# that is really a playlist cannot pull a local file into the poster. Anything else fails, and the +# poster falls back to a cut frame (`ensure_poster`). +THUMB_GUARD = ["-format_whitelist", "png_pipe,jpeg_pipe,webp_pipe"] def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str]: @@ -221,7 +259,7 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] ] if start: cmd += ["-ss", f"{start:.3f}"] - cmd += ["-i", str(src)] + cmd += [*INPUT_GUARD, "-i", str(src)] if end is not None: dur = end - start if dur > 0: @@ -229,10 +267,9 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] if reencode: # Input -ss + re-encode is frame-accurate (decodes from the prior keyframe, discards up to - # the exact cut). A crop adds the filter; an accurate trim re-encodes with no filter. - if crop: - cmd += ["-vf", f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}"] - cmd += [*X264_ARGS, *AAC_ARGS] + # the exact cut). A crop adds the filter; an accurate trim only evens the frame. + cropf = f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}," if crop else "" + cmd += ["-vf", cropf + EVEN_FRAME, *X264_ARGS, *AAC_ARGS] else: cmd += ["-c", "copy", "-avoid_negative_ts", "make_zero"] @@ -280,11 +317,14 @@ def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: parts, labels = [], [] for i, s in enumerate(segs): st, en = s["start_s"], s["end_s"] - parts.append(f"[0:v]trim=start={st}:end={en},setpts=PTS-STARTPTS{cropf}[v{i}]") + parts.append( + f"[0:v]trim=start={st}:end={en},setpts=PTS-STARTPTS{cropf},{EVEN_FRAME}[v{i}]" + ) parts.append(f"[0:a]atrim=start={st}:end={en},asetpts=PTS-STARTPTS[a{i}]") labels.append(f"[v{i}][a{i}]") parts.append(f"{''.join(labels)}concat=n={len(segs)}:v=1:a=1[v][a]") cmd = head + [ + *INPUT_GUARD, "-i", str(src), "-filter_complex", @@ -304,7 +344,9 @@ def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: lines.append(f"inpoint {s['start_s']}") lines.append(f"outpoint {s['end_s']}") prep[listp] = "\n".join(lines) + "\n" - cmd = head + ["-f", "concat", "-safe", "0", "-i", str(listp), "-c", "copy"] + # The list is ours; the whitelist reaches the file it names too (ffmpeg hands it down). + guard = ["-format_whitelist", "concat," + INPUT_GUARD[1]] + cmd = head + ["-f", "concat", "-safe", "0", *guard, "-i", str(listp), "-c", "copy"] if out_ext in ("mp4", "m4a", "mov"): cmd += ["-movflags", "+faststart"] @@ -477,7 +519,7 @@ def run_ffprobe_json( package: the shared media probe (`probe.probe_json`: the stored facts and the phone-safe rule) goes through it, so the subprocess flags/timeout/error handling live in one place instead of drifting copies.""" - args = ["ffprobe", "-v", "error"] + args = ["ffprobe", "-v", "error", *INPUT_GUARD] if show_streams: args.append("-show_streams") if show_format: @@ -507,6 +549,7 @@ def probe_duration(path: Path) -> int: "ffprobe", "-v", "error", + *INPUT_GUARD, "-show_entries", "format=duration", "-of", @@ -564,6 +607,7 @@ def build_storyboard_cmd(src: Path, dest: Path, duration_s: int | None, geom: di "-loglevel", "error", "-nostdin", + *INPUT_GUARD, "-i", str(src), "-frames:v", @@ -587,6 +631,7 @@ def build_poster_cmd(src: Path, dest: Path, at_s: float) -> list[str]: "-nostdin", "-ss", f"{max(0.0, at_s):.3f}", + *INPUT_GUARD, "-i", str(src), "-frames:v", @@ -608,6 +653,7 @@ def build_thumbnail_jpg_cmd(src: Path, dest: Path) -> list[str]: "-loglevel", "error", "-nostdin", + *THUMB_GUARD, "-i", str(src), "-frames:v", diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index 3475d5f2..e390d355 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -53,6 +53,7 @@ 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." +UPLOAD_MISSING_MESSAGE = "The uploaded file is missing." NETWORK_REDACTED_MESSAGE = "The server could not reach that address." # (marker, category) — first match wins, so order most-specific → most-general. Matching is @@ -98,6 +99,8 @@ _RULES: tuple[tuple[str, str], ...] = ( # An edit's source download went away — our own wording, and MORE specific than the generic # "no longer available" below, so it must be matched first. ("source download is no longer available", "source_missing"), + # An upload's original went away before its conversion ran — our wording (UPLOAD_MISSING_MESSAGE). + (UPLOAD_MISSING_MESSAGE.lower().rstrip("."), "upload_missing"), ("source file is missing", "source_missing"), # Private / deleted / removed / terminated. ("private video", "unavailable"), @@ -263,6 +266,15 @@ def classify(msg: str | None) -> str: return "unknown" +def classify_conversion(msg: str | None) -> str: + """The category of an upload's failed conversion. The generic ffmpeg categories speak of a + download ("the video downloaded but…"), which an upload never was: a failed encode or an + unrecognised failure of one is `convert_failed`; a category of its own (a full quota, a missing + original) stays as it is.""" + code = classify(msg) + return "convert_failed" if code in ("postprocess", "unknown") else code + + def diagnose( msg: str | None, *, url: str | None = None, cookie_attached: bool = False ) -> Diagnosis: diff --git a/backend/app/downloads/gc.py b/backend/app/downloads/gc.py index 8218cf40..7a9c2afd 100644 --- a/backend/app/downloads/gc.py +++ b/backend/app/downloads/gc.py @@ -6,12 +6,13 @@ Five passes each run: 2. Expiry deletion — delete assets past their TTL (files + row). The FK SET NULL nulls the holders' jobs' asset_id, so those jobs surface as "expired" (re-downloadable) in the UI. 3. LRU eviction — if a total cache cap is set and exceeded, delete least-recently-accessed - ready assets until back under it. + ready assets (never an upload) until back under it. 4. Orphan reap — delete errored, fileless, unreferenced asset rows (they carry no TTL, so passes 1-3 skip them; benign row-count hygiene). 5. Orphan FILE sweep — delete on-disk files/dirs no surviving asset owns (cruft left when a - re-download renames an asset, e.g. old accented channel dirs), skipping system trees and - anything newer than an hour so an in-flight download is never touched. + re-download renames an asset, e.g. old accented channel dirs, or an upload's stranded file). + It skips the dot-prefixed system trees (all but `.uploads`) and anything newer than an hour, + so an in-flight download is never touched. Runs in the API process (which mounts DOWNLOAD_ROOT). Pure disk/DB work — no YouTube quota. """ @@ -20,19 +21,19 @@ import time from datetime import datetime, timedelta, timezone from pathlib import Path -from sqlalchemy import func, select +from sqlalchemy import func, or_, select from sqlalchemy.orm import Session from app import logs, sysconfig from app.config import settings -from app.downloads import storage +from app.downloads import quota, storage from app.models import DownloadJob, MediaAsset from app.notifications import create_notification log = logs.get_logger(__name__) -# Dot-prefixed top-level trees that are NOT part of the Plex/flat asset layout — the orphan sweep -# never touches them (user clips, Plex HLS/img caches, in-progress staging, editor storyboards). -_SYSTEM_TREE = {".edits", ".plex-hls", ".plex-img-cache", ".staging", ".storyboards"} +# The one dot-prefixed top-level tree the orphan sweep enters. The others are not part of the asset +# layout (user clips, Plex HLS/img caches, in-progress staging, editor storyboards) and are skipped. +_UPLOAD_TREE = ".uploads" _ORPHAN_MIN_AGE_S = 3600 # only sweep files older than this, so an in-flight download is never hit @@ -67,16 +68,20 @@ def _delete_asset(db: Session, asset: MediaAsset, ntype: str) -> None: def _sweep_orphan_files(db: Session, download_root: str) -> int: """Delete media/sidecar files (and now-empty dirs) under DOWNLOAD_ROOT that no `ready` asset owns — cruft left when a re-download changes an asset's on-disk name (e.g. an old accented - channel dir, or a poster from a superseded naming scheme). Skips the dot-prefixed system trees - and only removes files older than `_ORPHAN_MIN_AGE_S`, so a just-completed or in-flight download - is never deleted, and only files NOT owned by a surviving asset.""" + channel dir, or a poster from a superseded naming scheme) — or, under `.uploads`, that no upload + row names at all. Skips the other dot-prefixed system trees and only removes files older than + `_ORPHAN_MIN_AGE_S`, so a just-completed or in-flight download is never deleted, and only files + NOT owned by a surviving asset.""" root = Path(download_root) if not root.is_dir(): return 0 keep: set[Path] = set() + # An upload's file is on disk from ingest on, whatever its state (a queued conversion's original, + # a failed one kept for Retry), so every upload row keeps its file — not only a ready one. for rel in db.execute( select(MediaAsset.rel_path).where( - MediaAsset.status == "ready", MediaAsset.rel_path.is_not(None) + or_(MediaAsset.status == "ready", MediaAsset.source_kind == "upload"), + MediaAsset.rel_path.is_not(None), ) ).scalars(): media = (root / rel).resolve() @@ -99,7 +104,10 @@ def _sweep_orphan_files(db: Session, download_root: str) -> int: unreclaimed = 0 for entry in root.iterdir(): - if entry.name.startswith("."): # .edits / .plex-* / .staging / .storyboards + any dotfile + # .edits / .plex-* / .staging / .storyboards + any dotfile — but `.uploads` is swept: a + # conversion stopped between placing the new file and the commit that points at it (or the + # unlink of the original after it) leaves a file no row names, and nothing else finds it. + if entry.name.startswith(".") and entry.name != _UPLOAD_TREE: continue files = [entry] if entry.is_file() else [p for p in entry.rglob("*") if p.is_file()] for f in files: @@ -188,17 +196,17 @@ def run_download_gc(db: Session) -> dict: if cap > 0: total = ( db.execute( - select(func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where( - MediaAsset.status == "ready" - ) + select(func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where(quota.ON_DISK) ).scalar() or 0 ) if total > cap: + # An upload counts toward the total (it is disk like any other) but is never evicted: + # it has no source to fetch again, and it lives until its owner deletes it. candidates = ( db.execute( select(MediaAsset) - .where(MediaAsset.status == "ready") + .where(MediaAsset.status == "ready", MediaAsset.source_kind != "upload") .order_by(MediaAsset.last_access_at.asc().nulls_first()) ) .scalars() @@ -232,10 +240,10 @@ def run_download_gc(db: Session) -> dict: if orphans: db.commit() - # 4b. Reap unheld uploads. An upload carries no expires_at and is never re-enqueued, so the passes - # above never free one that lost its last holder outside `release_asset` — a conversion that - # finished after its job was cancelled, or one cancelled mid-run (the asset was `downloading` - # then). Its file, ready or still the unconverted original, sits at `rel_path`. + # 4b. Reap unheld uploads. An upload carries no expires_at, is never evicted and never re-enqueued, + # so the passes above never free one that lost its last holder outside `release_asset` — a + # conversion that finished after its job was cancelled, or one cancelled mid-run (the asset + # was `downloading` then). Its file, ready or still the unconverted original, is at `rel_path`. unheld = ( db.execute( select(MediaAsset).where( diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 3d3510ea..853a3b5d 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -59,6 +59,10 @@ _DB_WRITE_INTERVAL_S = 1.0 _CANCEL_POLL_INTERVAL_S = 1.0 _THUMB_SUFFIXES = (".jpg", ".jpeg", ".png", ".webp") +# The job phase while a file is brought into the phone-safe shape (`_ensure_phone_safe`) for a +# download or an upload. Its own word, not yt-dlp's "processing": it reports ffmpeg's real progress, +# so the queue row shows a % for it, where yt-dlp's post-steps only get an indeterminate pulse. +CONVERTING = "converting" # yt-dlp's working files in the staging dir. None of them is ever the finished media, but several # CAN be the biggest file there (a video-only per-format stream, an abandoned fragment run), so the # staging fallback must exclude them by name — picking one would store e.g. a silent video-only @@ -76,7 +80,7 @@ def _notify_download_done(db, user_id: int | None, title: str | None, job_id, *, """Post a best-effort 'ready' notice to the owner's inbox — downloads/edits used to finish silently while scheduler jobs and playlist-push already notified (feedback symmetry). A notification failure must never mask the completed job, so it is isolated + rolled back (mirrors - scheduler._notify_done). `kind` ("download"/"edit") lets the inbox pick accurate copy; the + scheduler._notify_done). `kind` ("download"/"edit"/"upload") lets the inbox pick accurate copy; the English title here is only a fallback for an unknown type. `user_id`/`title` are captured by the caller BEFORE its commit, so reading them here needs no post-commit refresh of an expired row.""" if user_id is None: @@ -529,7 +533,7 @@ def _download( job_id, produced, staging, - "processing", + CONVERTING, int(info["duration"]) if info.get("duration") else None, ) ext = produced.suffix.lstrip(".").lower() @@ -751,7 +755,9 @@ def _fail_edit( job_id, status=JobStatus.ERROR, error=msg, - error_code=dlerrors.classify(msg), + error_code=dlerrors.classify_conversion(msg) + if what == "upload" + else dlerrors.classify(msg), phase=None, ) db.commit() @@ -833,6 +839,8 @@ def _process_edit(job_id: int, asset_id: int) -> None: storage.place_file(dest_staging, settings.download_root, rel) final = storage.abs_path(settings.download_root, rel) size = final.stat().st_size if final.exists() else None + # The clip's real frame: a re-encode evens an odd crop or source (`editmod.EVEN_FRAME`). + _, _, out_w, out_h, _ = probe.probe_media(final) _, retention_days = _download_settings() expires = datetime.now(timezone.utc) + timedelta(days=retention_days) @@ -844,7 +852,9 @@ def _process_edit(job_id: int, asset_id: int) -> None: asset.size_bytes = size asset.container = out_ext asset.duration_s = total_s or asset.duration_s - if crop: + if out_w and out_h: + asset.width, asset.height = out_w, out_h + elif crop: asset.width = edit_spec["crop"]["w"] asset.height = edit_spec["crop"]["h"] else: @@ -886,29 +896,57 @@ def _process_edit(job_id: int, asset_id: int) -> None: shutil.rmtree(staging, ignore_errors=True) +def _check_converted_fits(owner_id: int | None, old_size: int, new_size: int) -> None: + """Refuse a converted upload that would push its owner over the storage cap: the upload was + charged at its original size, and a re-encode can come out larger. The original stays, counted, + for the owner to delete or keep.""" + if owner_id is None: + return + with SessionLocal() as db: + if not quota.fits_after_conversion(db, owner_id, old_size, new_size): + raise StatedFailure(dlerrors.QUOTA_FULL_MESSAGE) + + +def _unplace(placed: Path | None) -> None: + """Remove a converted upload that was placed at its new rel but never committed: the row still + names the original, so nothing else would ever find this file.""" + if placed is not None: + placed.unlink(missing_ok=True) + + def _convert_upload(job_id: int, asset_id: int) -> None: """Convert a queued upload into the phone-safe shape (`playable`) and make it ready. The original sits at the asset's `rel_path` (see `upload`); the converted file replaces it there under the new extension, and only then is the original removed — so a failed or stopped - conversion leaves the original in place for Retry, and a delete still finds it by `rel_path`.""" + conversion leaves the original in place for Retry, and a delete still finds it by `rel_path`. + A converted file placed but never committed is removed again on the way out; a stop in the + instant between the commit and the original's unlink is left to the GC's `.uploads` sweep.""" with SessionLocal() as db: asset = db.get(MediaAsset, asset_id) old_rel = asset.rel_path if asset else None + old_ext = asset.container if asset else None + old_size = (asset.size_bytes or 0) if asset else 0 duration = asset.duration_s if asset else None + job = db.get(DownloadJob, job_id) + owner_id = job.user_id if job else None src = storage.safe_abs_path(settings.download_root, old_rel) if old_rel else None if old_rel is None or src is None or not src.exists(): - _fail_edit(job_id, asset_id, "The uploaded file is missing.", what="upload") + _fail_edit(job_id, asset_id, dlerrors.UPLOAD_MISSING_MESSAGE, what="upload") return staging = _staging_dir(asset_id) + placed: Path | None = None # the converted file while it is at its new rel, uncommitted try: shutil.rmtree(staging, ignore_errors=True) staging.mkdir(parents=True, exist_ok=True) - safe = _ensure_phone_safe(job_id, src, staging, "processing", duration, keep_source=True) + safe = _ensure_phone_safe(job_id, src, staging, CONVERTING, duration, keep_source=True) ext = safe.suffix.lstrip(".").lower() - new_rel = Path(old_rel).with_suffix(f".{ext}").as_posix() + new_rel = storage.with_ext(old_rel, old_ext, ext) if safe != src: + _check_converted_fits(owner_id, old_size, safe.stat().st_size) storage.place_file(safe, settings.download_root, new_rel) + if new_rel != old_rel: + placed = storage.abs_path(settings.download_root, new_rel) final = storage.abs_path(settings.download_root, new_rel) vcodec, acodec, width, height, dur = probe.probe_media(final) with SessionLocal() as db: @@ -923,15 +961,14 @@ def _convert_upload(job_id: int, asset_id: int) -> None: asset.duration_s = dur or asset.duration_s asset.error = None asset.last_access_at = datetime.now(timezone.utc) - job = db.get(DownloadJob, job_id) - owner_id = job.user_id if job else None title = asset.title if asset else None finalized = service.finalize_if_running( db, job_id, status=JobStatus.DONE, progress=100, phase=None ) db.commit() + placed = None if finalized: - _notify_download_done(db, owner_id, title, job_id, kind="download") + _notify_download_done(db, owner_id, title, job_id, kind="upload") if new_rel != old_rel: src.unlink(missing_ok=True) if finalized: @@ -943,6 +980,7 @@ def _convert_upload(job_id: int, asset_id: int) -> None: ) except editmod.EditAborted: # Paused/canceled mid-encode: the original is untouched; free the asset for a later retry. + _unplace(placed) _reset_asset_pending(asset_id) log.info( "upload job %s conversion aborted (pause/cancel)", @@ -951,6 +989,8 @@ def _convert_upload(job_id: int, asset_id: int) -> None: ) except Exception as exc: # noqa: BLE001 — surface the failure, keep the worker alive # silent-ok: not silent — `_fail_edit` is the report, and it takes `exc` for its stack. + _unplace(placed) _fail_edit(job_id, asset_id, str(exc)[:500], exc, what="upload") + finally: shutil.rmtree(staging, ignore_errors=True) diff --git a/backend/app/downloads/playable.py b/backend/app/downloads/playable.py index 8de66bcb..ec0381c4 100644 --- a/backend/app/downloads/playable.py +++ b/backend/app/downloads/playable.py @@ -80,6 +80,7 @@ def build_cmd(src: Path, dest: Path, p: Plan, threads: int | None = None) -> lis "-progress", "pipe:1", "-nostats", + *editmod.INPUT_GUARD, "-i", str(src), ] @@ -87,7 +88,10 @@ def build_cmd(src: Path, dest: Path, p: Plan, threads: int | None = None) -> lis # else rides along — a subtitle or data track the .mp4 muxer cannot take would fail the run. if p.has_video: cmd += ["-map", "0:V:0", "-map", "0:a:0?"] - cmd += editmod.X264_ARGS if p.reencode_video else ["-c:v", "copy"] + if p.reencode_video: + cmd += ["-vf", editmod.EVEN_FRAME, *editmod.X264_ARGS] + else: + cmd += ["-c:v", "copy"] else: cmd += ["-map", "0:a:0"] cmd += editmod.AAC_ARGS if p.reencode_audio else ["-c:a", "copy"] diff --git a/backend/app/downloads/quota.py b/backend/app/downloads/quota.py index 2fcd174e..66c92ef8 100644 --- a/backend/app/downloads/quota.py +++ b/backend/app/downloads/quota.py @@ -1,7 +1,9 @@ """Per-user download limits + footprint accounting. A user's *footprint* is the total size of the ready assets their library references (cache hits -count — it's "how much disk you're responsible for", not "how much you personally downloaded"). +count — it's "how much disk you're responsible for", not "how much you personally downloaded"). An +upload counts from the moment it lands, ready or not: its original is on disk while its conversion +waits, and stays there when the conversion fails (for Retry) — bytes a ready-only sum never saw. Limits come from a per-user DownloadQuota row if the admin set one, else the sysconfig defaults. `unlimited` (e.g. the admin's own account) bypasses the byte cap. @@ -18,16 +20,24 @@ is unknown until it finishes. Edit jobs do not consult it at all (tracked as SEC from dataclasses import dataclass -from sqlalchemy import func, select +from sqlalchemy import and_, func, or_, select from sqlalchemy.orm import Session from app import sysconfig +from app.downloads.status import JobStatus from app.models import DownloadJob, DownloadQuota, MediaAsset # Jobs a user is considered to be "holding" (occupy a job slot / footprint). Terminal failures # (error, canceled) don't count. -_HOLDING = ("queued", "running", "paused", "done") -_RUNNING = ("queued", "running") +_HOLDING = (JobStatus.QUEUED, JobStatus.RUNNING, JobStatus.PAUSED, JobStatus.DONE) +_RUNNING = (JobStatus.QUEUED, JobStatus.RUNNING) +# An asset whose file is on disk: a ready one, or an upload in any state (its original lands at +# ingest, see the module docstring). Every byte sum — a user's footprint, the GC's total-cap pass, +# the admin's storage view — reads it, so they all count the same disk. +ON_DISK = or_( + MediaAsset.status == "ready", + and_(MediaAsset.source_kind == "upload", MediaAsset.rel_path.is_not(None)), +) @dataclass @@ -67,12 +77,17 @@ def resolve(db: Session, user_id: int) -> QuotaLimits: def footprint(db: Session, user_id: int) -> int: - """Total bytes of the distinct ready assets this user's active jobs reference.""" + """Total bytes of the distinct assets this user's jobs hold on disk: the ready ones their active + jobs reference, and every upload of theirs — including one whose conversion failed, which keeps + its original (see the module docstring).""" held_assets = ( select(DownloadJob.asset_id) .where( DownloadJob.user_id == user_id, - DownloadJob.status.in_(_HOLDING), + or_( + DownloadJob.status.in_(_HOLDING), + and_(DownloadJob.job_kind == "upload", DownloadJob.status == JobStatus.ERROR), + ), DownloadJob.asset_id.is_not(None), ) .distinct() @@ -80,7 +95,7 @@ def footprint(db: Session, user_id: int) -> int: return int( db.execute( select(func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where( - MediaAsset.id.in_(held_assets), MediaAsset.status == "ready" + MediaAsset.id.in_(held_assets), ON_DISK ) ).scalar() or 0 @@ -111,7 +126,7 @@ def at_concurrency_limit(db: Session, user_id: int) -> bool: lim = resolve(db, user_id) if lim.unlimited: return False - running = _count(db, user_id, ("running",)) + running = _count(db, user_id, (JobStatus.RUNNING,)) return running >= lim.max_concurrent @@ -160,6 +175,18 @@ def check_upload(db: Session, user_id: int, add_bytes: int) -> None: raise QuotaExceeded("max_bytes", lim.max_bytes, projected) +def fits_after_conversion(db: Session, user_id: int, old_bytes: int, new_bytes: int) -> bool: + """Whether an upload converted from `old_bytes` (what it is charged at now) to `new_bytes` keeps + its owner within the cap. A re-encode can come out larger than the original, and nothing else + re-checks it once the upload was let in.""" + if new_bytes <= old_bytes: # a smaller file never breaks a cap the larger one was let in under + return True + lim = resolve(db, user_id) + if lim.unlimited: + return True + return footprint(db, user_id) - old_bytes + new_bytes <= lim.max_bytes + + def usage(db: Session, user_id: int) -> dict: """Compact usage snapshot for the UI (footprint + counts vs limits).""" lim = resolve(db, user_id) diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index 0084938e..4a3ea17e 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -168,13 +168,25 @@ def upload_rel_path(user_id: int, asset_id: int, filename: str) -> str: episode sidecars). The name is system-decided and bound to the asset id (`_a{id}` keeps two same-named uploads apart); the original filename is display metadata only. The extension is preserved (shell-safe, bounded) so the served file + its Content-Disposition carry a correct - suffix; an extensionless upload stays extensionless.""" - stem = sanitize(Path(filename).stem, 80) + suffix; an extensionless upload stays extensionless. + + The stem holds no dot, so the last dot of the name is always the extension's: every sibling + derived by swapping it (`with_suffix` — the poster, the converted file) keeps `_a{id}`. A dotted + stem with no extension ('Holiday.2024.ä') once lost it, and two such uploads shared one file.""" + stem = sanitize(Path(filename).stem, 80).replace(".", "_") ext = _clean_upload_ext(filename) suffix = f".{ext}" if ext else "" return f".uploads/{user_id}/{stem}_a{asset_id}{suffix}" +def with_ext(rel: str, old_ext: str | None, ext: str) -> str: + """`rel` with its extension — `old_ext`, the stored `container`, None for a bare name — swapped + for `ext`. Not `Path.with_suffix`: on a bare name whose stem holds a dot (an upload placed before + the stems lost their dots) that would take the stem's tail for the extension, and `_a{id}` with it.""" + base = rel[: -len(old_ext) - 1] if old_ext and rel.endswith(f".{old_ext}") else rel + return f"{base}.{ext}" + + def upload_ext(filename: str) -> str | None: """The stored `container` for an uploaded file: its shell-safe lowercased extension, or None when the upload has no extension (a bare name). Shares `_clean_upload_ext` with `upload_rel_path` so the diff --git a/backend/app/downloads/upload.py b/backend/app/downloads/upload.py index e7ebd443..7a491ea7 100644 --- a/backend/app/downloads/upload.py +++ b/backend/app/downloads/upload.py @@ -10,9 +10,11 @@ now" shortcut `service.enqueue` takes on a cache hit. Media that is NOT phone-safe (an iPhone's HEVC .mov, a VP9 .webm, an .mkv) is placed as it came and queued: the asset stays `pending` and the job `queued`, and the download worker converts it (`pipeline._convert_upload`) — so a share link never hands a friend a file their phone cannot play. -Only the converted file is kept (the owner's choice: one file per upload; a 10-bit HDR original -becomes 8-bit SDR). Until then the original sits at the asset's `rel_path`, which is what a failed -conversion's Retry starts from again, and what a delete removes (`service.release_asset`). +Only the converted file is kept (the owner's choice: one file per upload). A 10-bit HDR original +becomes 8-bit H.264 with its colour tags kept — still tagged HDR (HLG/PQ, BT.2020), not tone-mapped +to SDR (the owner's choice too). Until then the original sits at the asset's `rel_path`, which is +what a failed conversion's Retry starts from again, and what a delete removes +(`service.release_asset`). Kinds (the invariant lives in `mediakind`): a probe with a real video stream -> video, audio-only -> audio, neither -> a generic 'file' (pdf/zip/docx/…) that is download-only and never rendered @@ -63,7 +65,10 @@ def ingest_upload( data = probe.probe_json(src_path) vcodec, acodec, width, height, duration = probe.facts(data) kind = mediakind.derive(vcodec, acodec) - safe = playable.plan(data, Path(filename).suffix) if kind != mediakind.FILE else None + # Judged by the extension the file is PLACED under (and the worker re-judges), not the raw one: + # 'clip.mp4~' is stored as .mp4, and judging 'mp4~' queued a conversion with nothing to convert. + ext = storage.upload_ext(filename) or "" + safe = playable.plan(data, ext) if kind != mediakind.FILE else None convert = safe is not None and safe.needs_change # An upload never dedups (each file is its own asset): a random ref makes the cache key unique, @@ -88,7 +93,7 @@ def ingest_upload( asset.status = "pending" if convert else "ready" asset.rel_path = rel asset.size_bytes = actual_size - asset.container = storage.upload_ext(filename) + asset.container = ext or None asset.vcodec = vcodec asset.acodec = acodec asset.width = width diff --git a/backend/app/models.py b/backend/app/models.py index 608bc46d..4c799a33 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -912,7 +912,8 @@ class DownloadJob(Base, TimestampMixin, UpdatedAtMixin): ) source_kind: Mapped[str] = mapped_column(String(16)) # "youtube" | "url" source_ref: Mapped[str] = mapped_column(String(512)) - # "download" (default) or "edit" (a per-user trim/crop derivative of another job). + # "download" (default), "edit" (a per-user trim/crop derivative of another job) or "upload" (a + # user's own file, converted by the worker when it is not phone-safe — see `downloads.upload`). job_kind: Mapped[str] = mapped_column(String(16), default="download", server_default="download") # For edit jobs: the parent download this clip is cut from (SET NULL if the parent is deleted). source_job_id: Mapped[int | None] = mapped_column( diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 49e656dc..d9a81c19 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -32,6 +32,7 @@ from app.downloads import links as linksmod from app.downloads import quota, service, storage from app.downloads import upload as uploadmod from app.downloads import urlguard +from app.downloads.status import JobStatus from app.models import ( DownloadJob, DownloadLink, @@ -131,8 +132,8 @@ def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict: "job_kind": job.job_kind, "source_job_id": job.source_job_id, "edit_spec": job.edit_spec, - "can_download": ready and job.status == "done", - "expired": job.asset_id is None and job.status == "done", + "can_download": ready and job.status == JobStatus.DONE, + "expired": job.asset_id is None and job.status == JobStatus.DONE, "asset": None if asset is None else { @@ -540,7 +541,7 @@ def enqueue_edit( clip is a NEW per-user derivative in the editor's own library (counts against their quota); the source file is only read, never modified, so editing a shared video is safe.""" source_job = _accessible_job(db, user, body.source_job_id) - if source_job.status != "done" or source_job.asset_id is None: + if source_job.status != JobStatus.DONE or source_job.asset_id is None: raise refused(409, "edit_unfinished") spec = body.edit_spec display_name = display_text(body.display_name) @@ -594,7 +595,12 @@ def my_usage(user: User = Depends(current_user), db: Session = Depends(get_db)) # Priority when a source has several jobs (e.g. a done one plus a fresh queued one). -_INDEX_RANK = {"done": 4, "running": 3, "paused": 2, "queued": 1} +_INDEX_RANK: dict[str, int] = { + JobStatus.DONE: 4, + JobStatus.RUNNING: 3, + JobStatus.PAUSED: 2, + JobStatus.QUEUED: 1, +} @router.get("/index") @@ -604,7 +610,7 @@ def my_download_index(user: User = Depends(current_user), db: Session = Depends( rows = db.execute( select(DownloadJob.source_ref, DownloadJob.status).where( DownloadJob.user_id == user.id, - DownloadJob.status.in_(("queued", "running", "paused", "done")), + DownloadJob.status.in_(tuple(_INDEX_RANK)), ) ).all() out: dict[str, str] = {} @@ -735,7 +741,7 @@ def redownload_download( # Neither has a re-fetchable source: an editor clip is derived locally, an upload IS the # source. Re-running yt-dlp against source_kind='upload'/'edit' would just fail. raise refused(409, "not_redownloadable") - if job.status in ("queued", "running", "paused"): + if job.status in (JobStatus.QUEUED, JobStatus.RUNNING, JobStatus.PAUSED): raise refused(409, "still_downloading") service.redownload(db, job) return _serialize_job(db, job) @@ -887,7 +893,7 @@ def share_download( db: Session = Depends(get_db), ) -> dict: job = owned_or_404(db, DownloadJob, job_id, user, what="download") - if job.status != "done" or job.asset_id is None: + if job.status != JobStatus.DONE or job.asset_id is None: raise refused(409, "share_unfinished") target = body.email.strip().lower() if not target: @@ -1051,7 +1057,7 @@ def create_link( db: Session = Depends(get_db), ) -> dict: job = owned_or_404(db, DownloadJob, job_id, user, what="download") - if job.status != "done" or job.asset_id is None: + if job.status != JobStatus.DONE or job.asset_id is None: raise refused(409, "share_unfinished") pw = body.password.strip() link = DownloadLink( @@ -1139,9 +1145,7 @@ def admin_list_downloads( @admin_router.get("/storage") def admin_storage(admin: User = Depends(admin_user), db: Session = Depends(get_db)) -> dict: ready = db.execute( - select(func.count(), func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where( - MediaAsset.status == "ready" - ) + select(func.count(), func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where(quota.ON_DISK) ).one() total_assets = db.execute(select(func.count()).select_from(MediaAsset)).scalar() per_user = db.execute( diff --git a/backend/tests/test_ffmpeg_input_guard.py b/backend/tests/test_ffmpeg_input_guard.py new file mode 100644 index 00000000..d87cbf2e --- /dev/null +++ b/backend/tests/test_ffmpeg_input_guard.py @@ -0,0 +1,163 @@ +"""Every ffmpeg/ffprobe the downloads package runs over a library file names the demuxers it may use +(`edit.INPUT_GUARD`, a downloaded thumbnail's `edit.THUMB_GUARD`). + +Without the list, ffmpeg picks a demuxer from the file's content and name, and both are the user's +for an upload (the remote site's for a thumbnail): a playlist-type demuxer opens whatever local paths +its entries name. These tests read the argv each builder hands to ffmpeg — the guard is a flag, so +the flag is what is held — and scan the package for an ffmpeg/ffprobe call written without one. + +Not in scope: `app/plex`, whose ffmpeg reads the admin's own Plex library, not user-supplied bytes. +""" + +import ast +import shutil +import subprocess +from pathlib import Path + +import pytest + +from app.downloads import edit, playable + +DOWNLOADS = Path(__file__).resolve().parents[1] / "app" / "downloads" +# The demuxers that read paths named inside a file. `concat` is allowed only where the list is ours. +PLAYLIST_DEMUXERS = {"hls", "dash", "concat", "image2", "ffconcat", "tee"} +MEDIA = set(edit.MEDIA_DEMUXERS) + + +def _whitelists(cmd: list[str]) -> list[set[str] | None]: + """For each `-i` in `cmd`, the demuxers the `-format_whitelist` given since the previous input + allows — None when that input has none.""" + out, current = [], None + for i, tok in enumerate(cmd): + if tok == "-format_whitelist": + current = set(cmd[i + 1].split(",")) + elif tok == "-i": + out.append(current) + current = None + return out + + +def _assert_media_guarded(cmd: list[str], extra: set[str] = frozenset()) -> None: + lists = _whitelists(cmd) + assert lists, cmd # a builder that opens no input is not what this test is about + for allowed in lists: + assert allowed is not None, f"an input with no -format_whitelist: {cmd}" + assert allowed <= MEDIA | extra, allowed - MEDIA - extra + assert not (allowed - extra) & PLAYLIST_DEMUXERS + + +def test_the_media_list_names_no_demuxer_that_reads_other_paths(): + assert not MEDIA & PLAYLIST_DEMUXERS + assert edit.INPUT_GUARD == ["-format_whitelist", ",".join(edit.MEDIA_DEMUXERS)] + + +def test_the_phone_safe_conversion_is_guarded(): + p = playable.Plan("mp4", True, True, True, True) + _assert_media_guarded(playable.build_cmd(Path("in.mov"), Path("out.mp4"), p)) + + +@pytest.mark.parametrize( + "spec", + [ + {"trim": {"start_s": 1, "end_s": 3}, "accurate": False}, + {"trim": {"start_s": 1, "end_s": 3}, "accurate": True}, + {"crop": {"x": 0, "y": 0, "w": 101, "h": 51}}, + ], + ids=["fast trim", "accurate trim", "crop"], +) +def test_a_clip_is_guarded(spec): + spec = edit.normalize_edit_spec(spec) + _assert_media_guarded(edit.build_edit_cmd(Path("in.mp4"), Path("out.mp4"), spec, "mp4")) + + +@pytest.mark.parametrize("accurate", [True, False], ids=["accurate join", "fast join"]) +def test_a_join_is_guarded(accurate, tmp_path): + segs = [{"start_s": 0, "end_s": 1}, {"start_s": 2, "end_s": 3}] + spec = edit.normalize_edit_spec({"segments": segs, "accurate": accurate}) + cmd, _ = edit.build_concat_plan(Path("in.mp4"), Path("out.mp4"), spec, "mp4", tmp_path) + # The fast join reads a concat list WE write; the whitelist rides down to the file it names. + _assert_media_guarded(cmd, extra=set() if accurate else {"concat"}) + + +def test_the_poster_and_the_filmstrip_are_guarded(): + _assert_media_guarded(edit.build_poster_cmd(Path("in.mp4"), Path("p.jpg"), 2.0)) + geom = edit.storyboard_geometry(60) + _assert_media_guarded(edit.build_storyboard_cmd(Path("in.mp4"), Path("s.jpg"), 60, geom)) + + +def test_a_downloaded_thumbnail_opens_as_a_single_image_only(): + (allowed,) = _whitelists(edit.build_thumbnail_jpg_cmd(Path("t.webp"), Path("t.jpg"))) + assert allowed == {"png_pipe", "jpeg_pipe", "webp_pipe"} + + +def test_every_ffprobe_is_guarded(monkeypatch): + seen = [] + + class _Done: + stdout = "{}" + + def _run(args, **_k): + seen.append(args) + return _Done() + + monkeypatch.setattr(edit.subprocess, "run", _run) + edit.run_ffprobe_json(Path("x.mp4"), show_streams=True) + edit.probe_duration(Path("x.mp4")) + assert len(seen) == 2 + for args in seen: + i = args.index("-format_whitelist") + assert set(args[i + 1].split(",")) == MEDIA + + +def _unguarded_calls(source: str) -> list[int]: + """Lines of a list literal that starts an ffmpeg/ffprobe command or hands one an input (`-i`) + and carries no `*…GUARD` / `*guard` splat and no "-format_whitelist" of its own. A builder that + adds its input in a later `+=` must guard that list — that is where the input is.""" + bad = [] + for node in ast.walk(ast.parse(source)): + if not isinstance(node, ast.List): + continue + consts = {e.value for e in node.elts if isinstance(e, ast.Constant)} + if not consts & {"-i", "ffprobe"}: + continue + guarded = "-format_whitelist" in consts or any( + isinstance(e, ast.Starred) + and isinstance(v := e.value, ast.Name | ast.Attribute) + and (v.id if isinstance(v, ast.Name) else v.attr).upper().endswith("GUARD") + for e in node.elts + ) + if not guarded: + bad.append(node.lineno) + return bad + + +def test_no_ffmpeg_call_in_the_package_opens_an_input_unguarded(): + found = { + f"{p.name}:{line}" + for p in sorted(DOWNLOADS.rglob("*.py")) + for line in _unguarded_calls(p.read_text(encoding="utf-8")) + } + assert found == set() + + +def test_the_scan_sees_an_unguarded_call(): + # The scan above is green over nothing if it cannot see one: both shapes it must catch. + assert _unguarded_calls('cmd = ["ffmpeg", "-y", "-i", src]') == [1] + assert _unguarded_calls('a = ["ffprobe", "-v", "error", str(p)]') == [1] + assert _unguarded_calls('cmd = [*INPUT_GUARD, "-i", src]') == [] + + +@pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") +@pytest.mark.parametrize("ext", ["png", "webp", "jpg"]) +def test_the_thumbnail_guard_still_opens_every_thumbnail_a_download_keeps(ext, tmp_path): + # `pipeline._THUMB_SUFFIXES`: what the guard must still let through (a .jpg that is really a + # PNG goes the ffmpeg route too — `thumbnail_to_jpg` copies only true JPEG bytes). + src = tmp_path / f"t.{ext}" + fmt = ["-f", "image2", "-c:v", "png"] if ext == "jpg" else [] + subprocess.run( + ["ffmpeg", "-y", "-loglevel", "error", "-f", "lavfi", "-i", "testsrc2=size=64x48", + "-frames:v", "1", *fmt, str(src)], + check=True, + ) # fmt: skip + out = edit.thumbnail_to_jpg(src, tmp_path / "poster.jpg") + assert out is not None and edit._is_jpeg(out) diff --git a/backend/tests/test_odd_frame.py b/backend/tests/test_odd_frame.py new file mode 100644 index 00000000..13f13561 --- /dev/null +++ b/backend/tests/test_odd_frame.py @@ -0,0 +1,73 @@ +"""A libx264 re-encode of an odd-sized frame: 4:2:0 halves the chroma, so libx264 refuses an odd +width or height ("width not divisible by 2"). Measured: a source that is odd AND not 4:2:0 already +(a 4:4:4 or RGB screen capture, 1365x767) failed its conversion outright; a 4:2:0 source never +reaches the encoder odd (ffmpeg's crop rounds to its chroma grid), but the clip then stored the +size the user dragged, not the one the file has. Every re-encode now ends its filter chain on +`edit.EVEN_FRAME` (at most one pixel row or column), and a clip stores its probed frame. + +Real ffmpeg on both paths — the refusal is the encoder's, so only the encoder can show it is gone. +""" + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from app.config import settings +from app.downloads import pipeline, playable, probe, service, storage +from app.downloads import upload as uploadmod +from app.models import DownloadJob, MediaAsset + +pytestmark = pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") + + +def _ffmpeg(*args: str) -> None: + subprocess.run(["ffmpeg", "-y", "-loglevel", "error", *args], check=True) + + +def _frame(path: Path) -> tuple[int | None, int | None]: + _, _, w, h, _ = probe.probe_media(path) + return w, h + + +def test_an_odd_sized_4_4_4_video_converts_to_the_even_frame_below_it(tmp_path): + src = tmp_path / "in.mkv" + _ffmpeg( + "-f", "lavfi", "-i", "testsrc=size=161x121:rate=10:duration=1", + "-c:v", "libx264", "-pix_fmt", "yuv444p", str(src), + ) # fmt: skip + assert _frame(src) == (161, 121) # odd for real (`testsrc2` rounds its own size) + p = playable.plan(probe.probe_json(src), "mkv") + assert p is not None and p.reencode_video + dest = tmp_path / "out.mp4" + subprocess.run(playable.build_cmd(src, dest, p), check=True, capture_output=True) + assert _frame(dest) == (160, 120) + + +def test_an_odd_crop_makes_a_clip_and_stores_the_frame_it_has(db, make_user, tmp_path, monkeypatch): + monkeypatch.setattr(settings, "download_root", str(tmp_path)) + src = tmp_path / "in.mp4" + _ffmpeg( + "-f", "lavfi", "-i", "testsrc2=size=160x120:rate=10:duration=2", + "-f", "lavfi", "-i", "sine=duration=2", + "-c:v", "libx264", "-pix_fmt", "yuv420p", "-c:a", "aac", str(src), + ) # fmt: skip + u = make_user() + source = uploadmod.ingest_upload( + db, u.id, filename="clip.mp4", src_path=src, size=src.stat().st_size + ) + assert source.status == "done" # already phone-safe: the clip's source as it came + job = service.enqueue_edit(db, u.id, source, {"crop": {"x": 3, "y": 5, "w": 101, "h": 51}}) + asset = db.get(MediaAsset, job.asset_id) + job.status, asset.status = "running", "downloading" # the worker's claim + db.commit() + + pipeline._process_edit(job.id, asset.id) + + db.expire_all() + job, asset = db.get(DownloadJob, job.id), db.get(MediaAsset, job.asset_id) + assert (job.status, job.error, asset.status) == ("done", None, "ready") + final = storage.abs_path(str(tmp_path), asset.rel_path) + assert _frame(final) == (100, 50) + assert (asset.width, asset.height) == (100, 50) # what the file is, not what was asked diff --git a/backend/tests/test_upload_conversion.py b/backend/tests/test_upload_conversion.py new file mode 100644 index 00000000..e7452881 --- /dev/null +++ b/backend/tests/test_upload_conversion.py @@ -0,0 +1,355 @@ +"""An upload after ingest: what it costs its owner from the moment it lands, how the worker's +conversion names, checks and cleans up the file it makes, and what the owner and a share link see. + +DB-backed. The conversion's ffmpeg step (`pipeline._ensure_phone_safe`) is stubbed to write a file of +a chosen size — these tests are about what happens around the encode; `test_upload.py` runs the real +one, and `test_playable.py` judges its bytes. +""" + +import os +import time +from datetime import datetime, timedelta, timezone +from pathlib import Path + +import pytest + +from app import sysconfig +from app.config import settings +from app.downloads import gc as gcmod +from app.downloads import pipeline, quota, storage +from app.downloads import probe as probemod +from app.downloads import upload as uploadmod +from app.downloads.stated import StatedFailure +from app.models import DownloadJob, DownloadQuota, MediaAsset, Notification +from sqlalchemy import select + + +@pytest.fixture +def _root(tmp_path, monkeypatch): + monkeypatch.setattr(settings, "download_root", str(tmp_path)) + return tmp_path + + +def _probe(monkeypatch, vcodec="hevc"): + """ffprobe's view of every file here: `vcodec` + AAC, 8-bit 4:2:0 — HEVC is queued for the + worker, H.264 lands as it came.""" + data = { + "streams": [ + {"codec_type": "video", "codec_name": vcodec, "pix_fmt": "yuv420p", + "width": 640, "height": 360}, + {"codec_type": "audio", "codec_name": "aac"}, + ], + "format": {"format_name": "mov,mp4", "duration": "6"}, + } # fmt: skip + monkeypatch.setattr(probemod, "probe_json", lambda _p: data) + + +def _ingest(db, root, user_id, name="clip.mov", size=10): + src = root / "incoming.bin" + src.write_bytes(b"o" * size) + job = uploadmod.ingest_upload(db, user_id, filename=name, src_path=src, size=size) + return job, db.get(MediaAsset, job.asset_id) + + +def _claim(db, job, asset): + """What the worker's claim does before it converts.""" + job.status, asset.status = "running", "downloading" + db.commit() + + +def _converts_to(monkeypatch, size, phases=None): + """The encode writes a `size`-byte .mp4 into its staging dir (and notes the phase it reports).""" + + def _encode(job_id, produced, out_dir, phase, total_s=None, **_k): + if phases is not None: + phases.append(phase) + out = Path(out_dir) / "converted.mp4" + out.write_bytes(b"c" * size) + return out + + monkeypatch.setattr(pipeline, "_ensure_phone_safe", _encode) + + +def _cap(db, user_id, max_bytes): + db.add( + DownloadQuota( + user_id=user_id, max_bytes=max_bytes, max_concurrent=2, max_jobs=50, unlimited=False + ) + ) + db.commit() + + +def _uploads_dir(root, user_id) -> Path: + return root / ".uploads" / str(user_id) + + +# --- what it costs --------------------------------------------------------------------------- + + +def test_a_queued_upload_counts_against_its_owner_from_the_moment_it_lands( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + u = make_user() + job, asset = _ingest(db, _root, u.id, size=10) + assert (job.status, asset.status) == ("queued", "pending") + assert quota.footprint(db, u.id) == 10 # its original is on disk already + + +def test_a_failed_conversion_still_counts_its_kept_original(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + u = make_user() + job, asset = _ingest(db, _root, u.id, size=10) + job.status, asset.status = "error", "error" # kept for Retry + db.commit() + assert storage.abs_path(str(_root), asset.rel_path).exists() + assert quota.footprint(db, u.id) == 10 + + +def test_a_conversion_that_outgrows_the_cap_is_refused_and_keeps_the_original( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + u = make_user() + _cap(db, u.id, max_bytes=100) + job, asset = _ingest(db, _root, u.id, size=10) + original = storage.abs_path(str(_root), asset.rel_path) + _claim(db, job, asset) + _converts_to(monkeypatch, size=500) + + pipeline._convert_upload(job.id, asset.id) + + db.expire_all() + job = db.get(DownloadJob, job.id) + assert (job.status, job.error_code) == ("error", "quota_exceeded") + assert original.exists() + assert [p.name for p in _uploads_dir(_root, u.id).iterdir()] == [original.name] + + +def test_a_conversion_that_shrinks_passes_an_owner_at_the_cap(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + u = make_user() + _cap(db, u.id, max_bytes=10) + job, asset = _ingest(db, _root, u.id, size=10) + _claim(db, job, asset) + _converts_to(monkeypatch, size=5) + + pipeline._convert_upload(job.id, asset.id) + + db.expire_all() + assert db.get(DownloadJob, job.id).status == "done" + assert db.get(MediaAsset, asset.id).size_bytes == 5 + + +# --- the converted file's name and its leftovers ----------------------------------------------- + + +def test_two_dotted_bare_names_keep_a_file_each(db, make_user, _root, monkeypatch): + # 'Holiday.2024.ä': its suffix is no extension (nothing shell-safe is left of it), so the stem's + # own dot once read as one — and the converted name lost `_a{id}`, the one thing keeping two apart. + _probe(monkeypatch) + u = make_user() + _converts_to(monkeypatch, size=5) + finals = [] + for _ in range(2): + job, asset = _ingest(db, _root, u.id, name="Holiday.2024.ä") + _claim(db, job, asset) + pipeline._convert_upload(job.id, asset.id) + db.expire_all() + asset = db.get(MediaAsset, asset.id) + assert asset.status == "ready" + assert asset.rel_path.endswith(f"_a{asset.id}.mp4"), asset.rel_path + finals.append(asset.rel_path) + assert finals[0] != finals[1] + assert all(storage.abs_path(str(_root), r).exists() for r in finals) + + +def test_a_conversion_failing_after_it_placed_its_file_removes_it( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + u = make_user() + job, asset = _ingest(db, _root, u.id) + original = storage.abs_path(str(_root), asset.rel_path) + _claim(db, job, asset) + _converts_to(monkeypatch, size=5) + + def _probe_fails(_path): + raise RuntimeError("probe crashed") + + monkeypatch.setattr(pipeline.probe, "probe_media", _probe_fails) + pipeline._convert_upload(job.id, asset.id) + + db.expire_all() + assert db.get(DownloadJob, job.id).status == "error" + assert original.exists() # Retry starts from it + # The row still names the original: a converted file left beside it is one nothing would find. + assert [p.name for p in _uploads_dir(_root, u.id).iterdir()] == [original.name] + + +# --- what the owner is told ------------------------------------------------------------------ + + +def test_a_failed_encode_is_a_failed_conversion_not_a_download_step( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + job, asset = _ingest(db, _root, make_user().id) + _claim(db, job, asset) + + def _ffmpeg_fails(*_a, **_k): + raise StatedFailure("ffmpeg failed (1): unsupported") + + monkeypatch.setattr(pipeline.editmod, "run_ffmpeg", _ffmpeg_fails) + pipeline._convert_upload(job.id, asset.id) + db.expire_all() + assert db.get(DownloadJob, job.id).error_code == "convert_failed" + + +def test_a_missing_original_says_so(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + job, asset = _ingest(db, _root, make_user().id) + _claim(db, job, asset) + storage.abs_path(str(_root), asset.rel_path).unlink() + + pipeline._convert_upload(job.id, asset.id) + db.expire_all() + assert db.get(DownloadJob, job.id).error_code == "upload_missing" + + +def test_the_conversion_reports_its_own_phase(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + job, asset = _ingest(db, _root, make_user().id) + _claim(db, job, asset) + phases = [] + _converts_to(monkeypatch, size=5, phases=phases) + pipeline._convert_upload(job.id, asset.id) + assert phases == ["converting"] # not yt-dlp's "processing": the queue row shows ffmpeg's % + + +def test_the_owner_is_told_their_upload_is_ready(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + u = make_user() + job, asset = _ingest(db, _root, u.id) + _claim(db, job, asset) + _converts_to(monkeypatch, size=5) + pipeline._convert_upload(job.id, asset.id) + notes = db.execute(select(Notification).where(Notification.user_id == u.id)).scalars().all() + assert [n.data.get("kind") for n in notes] == ["upload"] + + +def test_an_extension_is_judged_as_it_is_stored(db, make_user, _root, monkeypatch): + # 'clip.mp4~' is stored as .mp4; judged by the raw 'mp4~' it queued a conversion of a file that + # was phone-safe already. + _probe(monkeypatch, vcodec="h264") + job, asset = _ingest(db, _root, make_user().id, name="clip.mp4~") + assert (job.status, asset.status, asset.container) == ("done", "ready", "mp4") + + +# --- the garbage collector --------------------------------------------------------------------- + + +def _aged(path: Path, hours: float = 5) -> None: + old = time.time() - hours * 3600 + os.utime(path, (old, old)) + + +def test_the_sweep_takes_a_stranded_upload_file_and_keeps_a_waiting_original( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + u = make_user() + _job, asset = _ingest(db, _root, u.id) + original = storage.abs_path(str(_root), asset.rel_path) + stranded = original.with_name("left_by_a_stopped_conversion_a999.mp4") + stranded.write_bytes(b"x") + _aged(original) + _aged(stranded) + + gcmod._sweep_orphan_files(db, str(_root)) + + assert original.exists() # queued for conversion: its row names it, ready or not + assert not stranded.exists() # no row names it + + +def test_the_cache_cap_never_evicts_an_upload(db, make_user, _root, monkeypatch): + _probe(monkeypatch, vcodec="h264") + u = make_user() + _job, upload = _ingest(db, _root, u.id, name="mine.mp4", size=10) + db.add( + MediaAsset( + source_kind="youtube", source_ref="vid1", format_sig="best", status="ready", + rel_path="Chan/clip.mp4", size_bytes=10, title="downloaded", + last_access_at=datetime.now(timezone.utc), + ) + ) # fmt: skip + # The upload is the least recently used: an LRU pass that may take it takes it first. + upload.last_access_at = datetime.now(timezone.utc) - timedelta(days=30) + db.commit() + (_root / "Chan").mkdir() + (_root / "Chan" / "clip.mp4").write_bytes(b"d" * 10) + sysconfig.set_value(db, "download_total_max_bytes", 15) # 20 on disk + db.commit() + + gcmod.run_download_gc(db) + + db.expire_all() + assert db.get(MediaAsset, upload.id).status == "ready" + assert storage.abs_path(str(_root), upload.rel_path).exists() + + +# --- a share link ---------------------------------------------------------------------------- + + +@pytest.mark.parametrize("state", ["pending", "downloading"]) +def test_a_link_whose_video_is_being_made_again_says_preparing_not_gone( + state, db, as_user, make_user, _root, monkeypatch, upload_headers +): + # A link is made on a finished job; its asset can be (re)made under it later — a shared cache row + # re-fetched, a conversion retried. Until it is ready the page says "try again shortly". + _probe(monkeypatch, vcodec="h264") + c = as_user(make_user()) + 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() + assert c.get(f"/api/public/watch/{link['token']}").status_code == 200 + db.get(MediaAsset, job["asset"]["id"]).status = state + db.commit() + r = c.get(f"/api/public/watch/{link['token']}") + assert r.status_code == 404 + assert r.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) + u = make_user() + _ingest(db, _root, u.id, size=10) + r = as_user(make_user(role="admin")).get("/api/admin/downloads/storage").json() + assert (r["ready_files"], r["total_bytes"]) == (1, 10) + assert {p["user_id"]: p["footprint_bytes"] for p in r["per_user"]} == {u.id: 10} + + +@pytest.mark.parametrize( + ("job_status", "asset_status", "ref_count"), + [ + ("done", "ready", 1), # a finished upload its owner keeps + ("queued", "pending", 1), # one waiting for the worker + ("canceled", "downloading", 0), # cancelled mid-conversion: the worker still reads it + ], +) +def test_the_upload_reap_leaves_one_still_held_or_still_read( + job_status, asset_status, ref_count, db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + job, asset = _ingest(db, _root, make_user().id) + path = storage.abs_path(str(_root), asset.rel_path) + job.status, asset.status, asset.ref_count = job_status, asset_status, ref_count + db.commit() + + gcmod.run_download_gc(db) + + db.expire_all() + assert db.get(MediaAsset, asset.id) is not None + assert path.exists() diff --git a/backend/tests/test_worker_dispatch.py b/backend/tests/test_worker_dispatch.py new file mode 100644 index 00000000..8dc74b96 --- /dev/null +++ b/backend/tests/test_worker_dispatch.py @@ -0,0 +1,74 @@ +"""The worker child's dispatch (`runner._run`) and the edit path's last step before it stores a clip. + +`runner._run` picks the pipeline entry by the job's kind; the tests elsewhere call those entries +directly, so a kind routed to the wrong one (an upload sent to yt-dlp) would pass all of them. +""" + +from pathlib import Path + +import pytest + +from app.config import settings +from app.downloads import pipeline, runner, service, storage +from app.downloads import probe as probemod +from app.downloads import upload as uploadmod +from app.models import DownloadJob, MediaAsset + + +@pytest.mark.parametrize( + ("kind", "entry"), + [("upload", "_convert_upload"), ("edit", "_process_edit"), ("download", "_download")], +) +def test_each_job_kind_reaches_its_own_pipeline_entry(kind, entry, monkeypatch): + called = [] + for name in ("_convert_upload", "_process_edit", "_download"): + monkeypatch.setattr(pipeline, name, lambda *a, _n=name: called.append(_n)) + runner._run(5, 7, kind, "url", "https://example.org/v", {}, 1) + assert called == [entry] + + +def test_a_clip_the_phone_safe_step_changed_is_stored_under_its_new_extension( + db, make_user, tmp_path, monkeypatch +): + # A fast trim stream-copies its source's container; when the phone-safe step remuxes it (an + # audio clip out of an .mp4 becomes an .m4a), the stored path and container follow the new file. + monkeypatch.setattr(settings, "download_root", str(tmp_path)) + data = { + "streams": [{"codec_type": "audio", "codec_name": "aac"}], + "format": {"format_name": "mov,mp4", "duration": "10"}, + } + monkeypatch.setattr(probemod, "probe_json", lambda _p: data) + src = tmp_path / "in.bin" + src.write_bytes(b"a" * 10) + u = make_user() + source = uploadmod.ingest_upload(db, u.id, filename="song.m4a", src_path=src, size=10) + # Stored as an .mp4, the way an audio file was kept before every ready file met the rule. + kept = db.get(MediaAsset, source.asset_id) + old = storage.abs_path(str(tmp_path), kept.rel_path) + kept.rel_path, kept.container = kept.rel_path[: -len(".m4a")] + ".mp4", "mp4" + old.rename(storage.abs_path(str(tmp_path), kept.rel_path)) + db.commit() + job = service.enqueue_edit( + db, u.id, source, {"trim": {"start_s": 1, "end_s": 3}, "accurate": False} + ) + asset = db.get(MediaAsset, job.asset_id) + job.status, asset.status = "running", "downloading" + db.commit() + + def _trim(cmd, *_a, **_k): + Path(cmd[-1]).write_bytes(b"t" * 4) # the stream-copied clip, at the path ffmpeg was given + + def _remux(job_id, produced, out_dir, phase, total_s=None, **_k): + out = Path(out_dir) / "remuxed.m4a" + out.write_bytes(b"r" * 3) + return out + + monkeypatch.setattr(pipeline.editmod, "run_ffmpeg", _trim) + monkeypatch.setattr(pipeline, "_ensure_phone_safe", _remux) + pipeline._process_edit(job.id, asset.id) + + db.expire_all() + asset = db.get(MediaAsset, asset.id) + assert (db.get(DownloadJob, job.id).status, asset.container) == ("done", "m4a") + assert asset.rel_path.endswith(".m4a") + assert storage.abs_path(str(tmp_path), asset.rel_path).read_bytes() == b"rrr" diff --git a/frontend/src/components/CrashBoundary.tsx b/frontend/src/components/ui/CrashBoundary.tsx similarity index 100% rename from frontend/src/components/CrashBoundary.tsx rename to frontend/src/components/ui/CrashBoundary.tsx