diff --git a/VERSION b/VERSION index 09b1cdc..806cc5f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.53.1 \ No newline at end of file +0.54.0 \ No newline at end of file diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 0d4944d..99235ae 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -24,7 +24,10 @@ import hashlib import json import logging import math +import queue import subprocess +import threading +import time from pathlib import Path log = logging.getLogger("siftlode.edit") @@ -178,6 +181,17 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] return cmd +def _concat_quote(path: Path) -> str: + """Escape a path for a single-quoted `file '…'` line of an ffmpeg concat list. + + ffmpeg's quoting rule: inside single quotes EVERY character is literal except `'` itself, which + is written `'\\''` (close the quote, emit an escaped quote, reopen). Backslashes are literal + there — doubling them would corrupt a path that contains one. On-disk names have been + ASCII-slugged since 0.51.0, but that change is FORWARD-ONLY: a pre-0.51.0 file still on disk can + be named `Don't_….mp4`, and an unescaped apostrophe makes the whole list unparseable.""" + return path.as_posix().replace("'", "'\\''") + + def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: Path): """Plan a multi-segment JOIN (cut-list → one file). Returns (cmd, prep_files) where prep_files maps a path → text content the worker must write before running (a concat list, if any). @@ -211,7 +225,7 @@ def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: listp = staging / "concat.txt" lines = [] for s in segs: - lines.append(f"file '{src.as_posix()}'") + lines.append(f"file '{_concat_quote(src)}'") lines.append(f"inpoint {s['start_s']}") lines.append(f"outpoint {s['end_s']}") prep[listp] = "\n".join(lines) + "\n" @@ -238,34 +252,114 @@ def _parse_out_time(line: str) -> float | None: return None -def run_ffmpeg(cmd, total_s, on_progress, should_cancel) -> None: +# No output on `-progress pipe:1` for this long means ffmpeg is wedged (a stuck network source, a +# hung filter) — a download job may legitimately be slow, but it is never SILENT for minutes. +_FFMPEG_STALL_TIMEOUT_S = 600.0 +# Hard cap after we ask the process to go away; past it we stop waiting on it altogether. +_FFMPEG_EXIT_GRACE_S = 10.0 + + +def _pump(stream, sink: "queue.Queue[str | None]") -> None: + """Read a text pipe to EOF into a queue (None = EOF). Runs in its own thread so the main loop + can time out and poll cancel instead of blocking forever on a silent ffmpeg.""" + try: + for line in stream: + sink.put(line) + except (ValueError, OSError): # pipe closed under us by the kill path + pass + finally: + sink.put(None) + + +def _drain_tail(stream, sink: list[str]) -> None: + """Consume a pipe to EOF, keeping only its tail. + + ffmpeg's stderr MUST be read *while it runs*: the OS pipe buffer is ~64KB, and once it fills, + ffmpeg blocks forever on its next write — the process never exits, so `proc.wait()` never + returns and the worker thread is gone for good. Reading it only after the exit (as this used + to) is exactly the deadlock.""" + try: + for line in stream: + sink.append(line) + if len(sink) > 200: + del sink[:100] + except (ValueError, OSError): + pass + + +def _stop(proc: subprocess.Popen) -> None: + """Terminate, then kill — never wait unbounded on a process we've given up on. Safe to call on + a process that already exited (the single exit path below may reach it after a clean wait).""" + if proc.poll() is not None: + return + proc.terminate() + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + log.error("ffmpeg pid %s survived SIGKILL", proc.pid) + + +def run_ffmpeg(cmd, total_s, on_progress, should_cancel, stall_timeout_s=_FFMPEG_STALL_TIMEOUT_S) -> None: """Run an ffmpeg edit, reporting 0–100 progress and honouring cooperative cancel. - Raises EditAborted if should_cancel() turns True mid-run, RuntimeError on a nonzero exit.""" + Both pipes are drained by threads (see `_drain_tail`) and the main loop polls on a 1s tick, so + cancel is honoured within a second even while ffmpeg is quiet, and a wedged process is killed + by the stall watchdog instead of pinning a worker thread forever. + + Raises EditAborted if should_cancel() turns True mid-run, RuntimeError on a nonzero exit or a + stall.""" proc = subprocess.Popen( cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, bufsize=1 ) + err_tail: list[str] = [] + lines: queue.Queue[str | None] = queue.Queue() + readers = [ + threading.Thread(target=_pump, args=(proc.stdout, lines), daemon=True), + threading.Thread(target=_drain_tail, args=(proc.stderr, err_tail), daemon=True), + ] + for t in readers: + t.start() try: - assert proc.stdout is not None - for line in proc.stdout: - line = line.strip() + last_output = time.monotonic() + while True: + try: + line = lines.get(timeout=1.0) + except queue.Empty: + line = "" + if line is None: # stdout hit EOF — ffmpeg is done (or dying) + break + if line: + last_output = time.monotonic() + secs = _parse_out_time(line.strip()) + if secs is not None and total_s: + on_progress(max(0.0, min(99.0, secs * 100.0 / total_s))) if should_cancel(): - proc.terminate() - try: - proc.wait(timeout=5) - except subprocess.TimeoutExpired: - proc.kill() raise EditAborted - secs = _parse_out_time(line) - if secs is not None and total_s: - on_progress(max(0.0, min(99.0, secs * 100.0 / total_s))) + if time.monotonic() - last_output > stall_timeout_s: + raise RuntimeError(f"ffmpeg stalled: no progress for {int(stall_timeout_s)}s") + try: + ret = proc.wait(timeout=_FFMPEG_EXIT_GRACE_S) + except subprocess.TimeoutExpired: # pipes closed but the process lingers + raise RuntimeError("ffmpeg did not exit after closing its output") + except BaseException: + # ONE exit path stops the process, so the invariant is unconditional: nothing leaves this + # function with ffmpeg still running. Not just our own EditAborted/stall — `on_progress` + # writes to the database, and an exception from THERE used to unwind past a live process, + # orphaning it (still encoding, no one left to reap it) after a 10s join on its pipes. + _stop(proc) + raise finally: - if proc.stdout: - proc.stdout.close() - ret = proc.wait() + for t in readers: + t.join(timeout=_FFMPEG_EXIT_GRACE_S) + for pipe in (proc.stdout, proc.stderr): + if pipe: + pipe.close() if ret != 0: - err = (proc.stderr.read() if proc.stderr else "") or "" - raise RuntimeError(f"ffmpeg failed ({ret}): {err[:300]}") + raise RuntimeError(f"ffmpeg failed ({ret}): {''.join(err_tail)[-300:]}") # --- ffmpeg: universal browser playability -------------------------------------------------- @@ -278,29 +372,41 @@ _BROWSER_SAFE_VCODEC = ("h264",) _BROWSER_SAFE_ACODEC = ("aac", "mp3") +def parse_probe_streams(payload: str) -> tuple[str | None, str | None]: + """(video_codec, audio_codec) out of ffprobe's JSON stream list, lowercased. + + JSON, not the flat `default=nw=1` output: there the fields arrive in ffprobe's own struct + order, which puts `codec_name` BEFORE `codec_type`, so a line-by-line "remember the last + codec_type" parser attributes every name to the PREVIOUS stream. Measured on real downloads: + an ordinary H.264+AAC mp4 probed as `v=aac a=None` — the browser-playability check then + believed the video stream was AAC and re-encoded a perfectly fine file on every download.""" + try: + streams = json.loads(payload or "{}").get("streams") or [] + except ValueError: + return None, None + vcodec = acodec = None + for s in streams: + name = (s.get("codec_name") or "").lower() or None + ctype = (s.get("codec_type") or "").lower() + if ctype == "video" and vcodec is None: + vcodec = name + elif ctype == "audio" and acodec is None: + acodec = name + return vcodec, acodec + + def probe_codecs(path: Path) -> tuple[str | None, str | None]: """Return (video_codec, audio_codec) lowercased via one ffprobe call; None for a missing stream or if ffprobe fails (a failed probe is treated as 'leave it alone' upstream).""" try: out = subprocess.run( ["ffprobe", "-v", "error", "-show_entries", "stream=codec_type,codec_name", - "-of", "default=nw=1", str(path)], + "-of", "json", str(path)], capture_output=True, text=True, timeout=30, check=True, - ).stdout.lower() + ).stdout except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError): return None, None - vcodec = acodec = None - ctype = None - for line in out.splitlines(): - if line.startswith("codec_type="): - ctype = line.split("=", 1)[1].strip() - elif line.startswith("codec_name="): - name = line.split("=", 1)[1].strip() or None - if ctype == "video" and vcodec is None: - vcodec = name - elif ctype == "audio" and acodec is None: - acodec = name - return vcodec, acodec + return parse_probe_streams(out) def probe_duration(path: Path) -> int: diff --git a/backend/app/main.py b/backend/app/main.py index 41f8a19..85a6725 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1,6 +1,7 @@ +import asyncio import logging import sys -from contextlib import asynccontextmanager +from contextlib import asynccontextmanager, suppress from pathlib import Path from fastapi import FastAPI, HTTPException @@ -27,6 +28,7 @@ from starlette.middleware.sessions import SessionMiddleware from app import auth, state from app.config import settings from app.db import SessionLocal +from app.plex import stream as plex_stream from app.routes import ( admin, audit as audit_routes, @@ -67,14 +69,43 @@ async def lifespan(app: FastAPI): " Open the install wizard (internal access only):\n %s", url, ) + # No HLS session survives a restart (they live in this process), so anything still on disk is + # a crashed run's segments — sweep them or they accumulate forever. FIRE AND FORGET, not + # awaited: uvicorn does not serve a single request until lifespan startup returns, so awaiting + # the sweep would hold the port shut for as long as the rmtree runs (gigabytes of `.ts` after a + # long uptime) — the 502 + failed post-deploy health probe this is meant to avoid. A thread + # alone doesn't buy that; not waiting does. The sweep only touches directories no live session + # owns, so racing it against early playback is safe. + sweep = asyncio.create_task(_sweep_hls_orphans()) start_scheduler() try: yield finally: log.info("Siftlode shutting down") + # Awaited, not just cancelled: a bare `.cancel()` lets the loop close with the task still + # pending ("Task was destroyed but it is pending!" — noise that reads like a bug during + # triage). Cancelling does NOT interrupt the rmtree thread itself; this only makes our own + # bookkeeping honest. + sweep.cancel() + with suppress(asyncio.CancelledError): + await sweep shutdown_scheduler() +async def _sweep_hls_orphans() -> None: + """Background half of the startup HLS sweep (see lifespan). Owns its own errors: a failure here + must never take the app down, and a cancelled shutdown is not one.""" + try: + swept = await asyncio.to_thread(plex_stream.sweep_orphans) + except asyncio.CancelledError: + raise + except Exception: + log.exception("HLS orphan sweep failed") + return + if swept: + log.info("swept %d orphaned HLS segment directories", swept) + + app = FastAPI(title=settings.app_name, lifespan=lifespan) app.add_middleware( diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 5d1f5c2..dbaea40 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -15,6 +15,7 @@ segments. The remux is CPU-light (video copy + a tiny audio transcode), so it ru CPU-only prod host; only P3's full transcode is CPU-heavy. """ import logging +import re import shutil import subprocess import threading @@ -33,6 +34,32 @@ log = logging.getLogger("siftlode.plex") _HLS_ROOT = Path(settings.plex_hls_dir) if settings.plex_hls_dir else Path(settings.download_root) / ".plex-hls" _SEG_SECONDS = 6 _SESSION_IDLE_S = 600 # reap a session with no access for this long +# A session directory is named `{rating_key}_{int(start_s)}_{tag}_{aoff:+.2f}` (see start_session); +# before multi-audio it was just `{rating_key}_{int(start_s)}`. `sweep_orphans` deletes ONLY names +# matching one of those two shapes: `_HLS_ROOT` is admin-configurable (PLEX_HLS_DIR), so it may +# legitimately point at a shared scratch path we do not own, and a blanket "delete every directory +# under the root" would take the neighbours with it. +# +# The KEY is anchored to digits (a Plex rating_key is a number) — that is what keeps foreign names +# out. A `.+` there let `snapshot_2024_backup_+1.00` through, and pairing `.+` with an optional tail +# degenerated all the way to "anything ending in _" (`backup_2024`, `release_10`). A +# non-numeric rating_key would merely leave its directory unswept, which is the safe direction to +# fail. Covered by test_plex_stream.py — this regex is the only thing standing between the sweep +# and someone else's data. +_SESSION_DIR_RE = re.compile(r"^\d+_\d+(_[^_]+_[+-]\d+\.\d{2})?$") +# When this process started. `sweep_orphans` only deletes directories older than this: everything +# newer belongs to a session THIS run created, and since `start_session` reuses a directory path +# for the same (rating_key, offset, tag), "stale name" is not the same as "stale directory". +_PROCESS_START = time.time() +# Slack on that comparison, because the two sides come from DIFFERENT clocks. A file's mtime is +# stamped from the kernel's COARSE clock (updated once per timer tick), while `time.time()` reads +# the fine-grained one — measured in this very container, a directory created 2ms AFTER +# `_PROCESS_START` got an mtime 2ms BEFORE it. Without slack the sweep would classify a session +# started in the first instants after boot as a leftover and delete it while ffmpeg writes into it +# — and "the first instants after boot" is exactly when the sweep runs. Erring on the keep side +# costs nothing: a genuine leftover from the last seconds before the restart simply survives until +# the next one. +_MTIME_GRACE_S = 2.0 _lock = threading.Lock() _sessions: dict[str, "HlsSession"] = {} @@ -254,6 +281,52 @@ def wait_for(path: Path, timeout: float = 20.0) -> bool: return path.exists() +def sweep_orphans(older_than: float | None = None) -> int: + """Delete OUR segment directories that no live session owns. + + Sessions live only in THIS process, so at startup every session directory under `_HLS_ROOT` is + a leftover from a crashed or killed run — and nothing else ever reaps them (the download GC + deliberately skips `.plex-hls` as a system tree), so a few restarts mid-playback leave + gigabytes of `.ts` segments behind forever. + + Only entries whose name matches `_SESSION_DIR_RE` are touched: the root is admin-configurable, + so anything else under it belongs to someone else and is left alone. Blocking (rmtree) — call + it off the event loop. + + TWO guards against deleting a LIVE session, because this now runs CONCURRENTLY with request + handling (the caller stopped awaiting it so startup isn't held up): + * `older_than` (default: this process's start, minus `_MTIME_GRACE_S` of cross-clock slack) — + a directory touched since we booted was made by THIS run, so it is by definition not a + leftover. This is the guard that matters, because `start_session` REUSES the same path for + the same (key, offset, tag): a stale directory can become a live one at any moment. + * the live-session check and the rmtree happen together under `_lock`, so a session can't be + registered in the gap between them. The lock is held for one directory at a time — long + enough to be atomic, short enough that a boot-time sweep of a big backlog doesn't stall + playback.""" + cutoff = (_PROCESS_START - _MTIME_GRACE_S) if older_than is None else older_than + dropped = 0 + try: + entries = list(_HLS_ROOT.iterdir()) + except OSError: # the root doesn't exist yet — nothing to sweep + return 0 + for p in entries: + if not _SESSION_DIR_RE.match(p.name): + continue + if not p.is_dir(): + continue + try: + if p.stat().st_mtime >= cutoff: + continue # this run made (or reused) it — never ours to delete + except OSError: # vanished under us (a concurrent restart of that session) + continue + with _lock: + if any(s.dir == p for s in _sessions.values()): + continue + shutil.rmtree(p, ignore_errors=True) + dropped += 1 + return dropped + + def reap_idle() -> int: now = time.time() dropped = 0 diff --git a/backend/app/routes/sync.py b/backend/app/routes/sync.py index 9cfa522..41e0fef 100644 --- a/backend/app/routes/sync.py +++ b/backend/app/routes/sync.py @@ -52,8 +52,11 @@ def sync_subscriptions( def sync_rss( user: User = Depends(require_human), db: Session = Depends(get_db) ) -> dict: - new = run_rss_poll(db, _user_channels(db, user)) - return {"new_videos": new} + # A partial failure comes back in `failed_feeds` so a manual poll reports it instead of an + # innocent-looking "0 new"; a TOTAL outage raises out of run_rss_poll and surfaces as a 500, + # which is the honest answer for a poll that reached nothing. + result = run_rss_poll(db, _user_channels(db, user)) + return {"new_videos": result["new"], "failed_feeds": result["failed"]} @router.post("/backfill") diff --git a/backend/app/sync/playlists.py b/backend/app/sync/playlists.py index 59dbdc0..81ecb2c 100644 --- a/backend/app/sync/playlists.py +++ b/backend/app/sync/playlists.py @@ -7,7 +7,9 @@ never synced. Videos in a playlist that aren't in our shared catalog yet are fet ingested (with a stub channel) so the mirror is faithful.""" import hashlib import logging +from collections.abc import Callable from datetime import datetime, timezone +from typing import TypeVar from sqlalchemy import delete, select from sqlalchemy.orm import Session @@ -263,6 +265,71 @@ def plan_push(db: Session, user: User, pl: Playlist, *, live=None) -> dict: } +# After this many consecutive write failures we stop calling YouTube for the rest of the push. +# The failure modes that matter here are all account-wide, not per-video: quota exhausted, the +# write scope revoked, the token dead. Every remaining write then fails identically — but each +# one still SPENDS 50 units, so a long playlist burns through the whole (already dead) budget, +# and tomorrow's too, to produce a failure list we knew after the fifth item. +_MAX_CONSECUTIVE_FAILURES = 5 + + +class _Failed: + """Type of the `FAILED` sentinel, so `run()` can stay generic in the op's return type: a call + site gets back `str | _Failed` (not a bare `object`) and keeps its item id typed.""" + + +#: Returned by `_Breaker.run` when the write did not happen (refused or failed). +FAILED = _Failed() + +T = TypeVar("T") + + +class _Breaker: + """Consecutive-failure circuit breaker for a single push run. Any success re-arms it. + + Drive it with `run()` — it owns the whole open-check / call / record-outcome sequence, so a + write site can't accidentally implement two thirds of the protocol (forgetting to record a + failure would silently disable the breaker for that operation).""" + + def __init__(self, limit: int = _MAX_CONSECUTIVE_FAILURES) -> None: + self.limit = limit + self.streak = 0 + self.tripped = False + + def open(self) -> bool: + """True while writes may still be attempted.""" + return not self.tripped + + def hit(self, ok: bool) -> None: + if ok: + self.streak = 0 + return + self.streak += 1 + if self.streak >= self.limit and not self.tripped: + self.tripped = True + log.warning( + "playlist push: %d consecutive YouTube write failures — stopping this run " + "(quota, scope or token); the remaining items are reported as failures", + self.limit, + ) + + def run(self, op: Callable[[], T], on_fail: Callable[[], None]) -> T | _Failed: + """Attempt one YouTube write. Returns the op's result, or `FAILED` when the breaker is + already tripped (op not called) or the op raised YouTubeError. `on_fail` records the item + in the caller's failure list either way.""" + if self.tripped: + on_fail() + return FAILED + try: + out = op() + except YouTubeError: + on_fail() + self.hit(False) + return FAILED + self.hit(True) + return out + + def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: """Push a local playlist to YouTube: create it if needed, then reconcile membership and order so YouTube matches local (local wins). Marks the playlist clean on success. @@ -271,6 +338,7 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: desired = _local_video_ids(db, pl) inserted = deleted = reordered = created = 0 failures: list[str] = [] + breaker = _Breaker() with YouTubeClient(db, user) as yt: if not pl.yt_playlist_id: pl.yt_playlist_id = yt.create_playlist(pl.name) @@ -278,11 +346,12 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: created = 1 # Brand-new playlist: just append every video in order. for vid in desired: - try: - yt.add_playlist_item(pl.yt_playlist_id, vid) + added = breaker.run( + lambda v=vid: yt.add_playlist_item(pl.yt_playlist_id, v), + lambda v=vid: failures.append(v), + ) + if not isinstance(added, _Failed): inserted += 1 - except YouTubeError: - failures.append(vid) if not failures: pl.synced_fingerprint = fingerprint(pl.name, desired) pl.dirty = False @@ -293,6 +362,7 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: "deleted": deleted, "reordered": reordered, "failures": failures, + "aborted": breaker.tripped, "yt_playlist_id": pl.yt_playlist_id, } @@ -313,20 +383,23 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: # Delete extras (local is authoritative). for it in current: if it["video_id"] not in desired_set: - try: - yt.delete_playlist_item(it["item_id"]) + dropped = breaker.run( + lambda i=it: yt.delete_playlist_item(i["item_id"]), + lambda i=it: failures.append(i["video_id"]), + ) + if not isinstance(dropped, _Failed): deleted += 1 - except YouTubeError: - failures.append(it["video_id"]) # Insert missing (append; the reorder pass below fixes positions). cur_set = set(cur_vids) for vid in desired: if vid not in cur_set: - try: - item_id_by_vid[vid] = yt.add_playlist_item(pl.yt_playlist_id, vid) + item_id = breaker.run( + lambda v=vid: yt.add_playlist_item(pl.yt_playlist_id, v), + lambda v=vid: failures.append(v), + ) + if not isinstance(item_id, _Failed): + item_id_by_vid[vid] = item_id inserted += 1 - except YouTubeError: - failures.append(vid) # Reorder to match `desired` via insertion-sort moves over a local model. model = [v for v in cur_vids if v in desired_set] + [ v for v in desired if v not in cur_set and v in item_id_by_vid @@ -336,12 +409,15 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: continue # failed insert; skip if i < len(model) and model[i] == want: continue - try: - yt.move_playlist_item(item_id_by_vid[want], pl.yt_playlist_id, want, i) - reordered += 1 - except YouTubeError: - failures.append(want) + moved = breaker.run( + lambda w=want, pos=i: yt.move_playlist_item( + item_id_by_vid[w], pl.yt_playlist_id, w, pos + ), + lambda w=want: failures.append(w), + ) + if isinstance(moved, _Failed): continue + reordered += 1 j = model.index(want) model.pop(j) model.insert(i, want) @@ -355,6 +431,7 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: "deleted": deleted, "reordered": reordered, "failures": failures, + "aborted": breaker.tripped, "yt_playlist_id": pl.yt_playlist_id, } diff --git a/backend/app/sync/runner.py b/backend/app/sync/runner.py index e325a50..dca3564 100644 --- a/backend/app/sync/runner.py +++ b/backend/app/sync/runner.py @@ -25,7 +25,7 @@ from app.sync.videos import ( run_shorts_classification, ) from app.youtube.client import YouTubeClient -from app.youtube.rss import fetch_channel_feed +from app.youtube.rss import RssError, fetch_channel_feed # RSS feeds are free and network-bound, so fetch many at once; DB writes stay single-threaded. RSS_POLL_WORKERS = 16 @@ -48,12 +48,31 @@ def get_service_user(db: Session) -> User | None: return users[0] if users else None -def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> int: +def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str, int]: + """Poll every channel's free RSS feed. Returns `{"new": inserted, "failed": channels_that_did + _not_complete}` — the failure count is part of the RESULT, not just a log line, so the scheduler + card and the audit summary can tell "no new uploads" apart from "nothing worked" (an egress + outage, or a database that can't accept writes, otherwise renders identically to a quiet hour). + + `failed` counts a channel whose feed could not be READ *or* whose rows could not be APPLIED — + both leave the channel un-polled, and counting only the first would move the blind spot rather + than remove it. + + Raises RuntimeError when NOTHING could be polled (`failed == total`, total > 0): a run that + achieved literally nothing is a failed run, and only an exception turns the scheduler card red + — a returned count leaves it green with the outage buried in the summary line. The message + carries the FIRST underlying error, because on a small instance "all channels failed" is also + what a single dead feed id looks like.""" if channels is None: channels = db.execute(select(Channel)).scalars().all() total = len(channels) new = 0 done = 0 + failed = 0 + # Kept for the total-outage message: "failed for all N channels" alone can't tell an egress + # outage from one permanently dead feed id — which is the whole difference on an instance with + # a single subscription, where BOTH render as 100% failed. + first_error: str | None = None # Fetch feeds concurrently (network-bound, no DB), then apply inserts on this thread as # each fetch returns — a SQLAlchemy session isn't thread-safe. Pass the channel id (a # plain str) to the workers; the ORM instance is touched only here on the main thread. @@ -65,17 +84,44 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> int: channel = futures[fut] try: entries = fut.result() - except Exception: + # A failed poll is NOT an empty feed: skip the apply so `last_rss_at` stays where it + # was (stamping it made a permanently-broken feed look freshly polled). The shared + # counter/progress tail lives after the statement, so the two outcomes can stay two + # clauses: an RssError is EXPECTED (404, timeout) and gets one warning line; anything + # else is a bug on our side and earns a traceback. + except RssError as exc: + log.warning("RSS poll skipped for channel %s: %s", channel.id, exc) + failed += 1 + first_error = first_error or f"{channel.id}: {exc}" + except Exception as exc: log.exception("RSS fetch failed for channel %s", channel.id) - entries = [] - try: - new += apply_rss_feed(db, channel, entries) - except Exception: - db.rollback() - log.exception("RSS apply failed for channel %s", channel.id) + failed += 1 + first_error = first_error or f"{channel.id}: {exc}" + else: + try: + new += apply_rss_feed(db, channel, entries) + except Exception as exc: + # A write failure (read-only DB, failover) leaves the channel just as un-polled + # as an unreachable feed — count it, or a wholly broken run reports "failed=0". + db.rollback() + failed += 1 + first_error = first_error or f"{channel.id} (apply): {exc}" + log.exception("RSS apply failed for channel %s", channel.id) + # One place that advances the counter, whatever happened above. done += 1 progress.report(done, total, "rss_poll") - return new + if failed: + log.warning("RSS poll: %d/%d channels could not be polled", failed, total) + if total and failed == total: + # Nothing was polled at all — an egress outage (the tinyproxy-after-reboot incident), a + # dead DNS, a read-only database. Returning normally would record the run as `ok` with the + # failure hidden inside the result string, i.e. a green pill for a total outage. + # The first error TRAVELS WITH the message on purpose: on an instance with one or two + # subscriptions "all channels failed" is also what ONE dead feed id looks like, and a red + # card the user can't act on is its own kind of dishonest. `channel: HTTP 404` says + # "fix this subscription"; `Connection refused` says "the egress is down". + raise RuntimeError(f"RSS poll failed for all {total} channels — first: {first_error}") + return {"new": new, "failed": failed} def run_enrich(db: Session, max_batches: int = 200, floor: int = 200) -> int: diff --git a/backend/app/sync/subscriptions.py b/backend/app/sync/subscriptions.py index e8b98cc..019445a 100644 --- a/backend/app/sync/subscriptions.py +++ b/backend/app/sync/subscriptions.py @@ -73,6 +73,18 @@ def apply_channel_details(db: Session, items: list[dict]) -> None: channel.details_synced_at = now +def should_prune(fetched_count: int, existing_count: int) -> bool: + """Whether an import may delete the local subscriptions missing from this fetch. + + NEVER on an empty fetch that contradicts local state: the API can answer 200 with no items (a + transient blip, a scope/token edge), and pruning against that wipes every subscription the user + has — taking each row's `deep_requested` flag with it — in a single run. Someone who genuinely + unsubscribed from everything simply keeps their rows until a sync that returns something. + + Pure so it can be tested without a database; `import_subscriptions` is the only caller.""" + return fetched_count > 0 or existing_count == 0 + + def import_subscriptions(db: Session, user: User) -> dict: """Pull all subscriptions for `user`, upsert channels + subscription links, prune channels the user has unsubscribed from on YouTube, and fetch channel details for @@ -108,17 +120,26 @@ def import_subscriptions(db: Session, user: User) -> dict: sub_row.yt_subscription_id = sub.get("yt_subscription_id") db.flush() - # Prune subscriptions removed on YouTube. + # Prune subscriptions removed on YouTube — but only when this fetch is trustworthy + # (see `should_prune`). removed = 0 existing = ( db.execute(select(Subscription).where(Subscription.user_id == user.id)) .scalars() .all() ) - for sub_row in existing: - if sub_row.channel_id not in fetched: - db.delete(sub_row) - removed += 1 + prune_skipped = not should_prune(len(fetched), len(existing)) + if prune_skipped: + log.warning( + "Subscription import (user %s): YouTube returned 0 subscriptions while %d exist " + "locally — skipping the prune (treating it as a transient empty response)", + user.id, len(existing), + ) + else: + for sub_row in existing: + if sub_row.channel_id not in fetched: + db.delete(sub_row) + removed += 1 db.commit() # Fetch details for channels that don't have them yet. @@ -144,6 +165,7 @@ def import_subscriptions(db: Session, user: User) -> dict: "channels_new": new_channels, "channels_detailed": detailed, "removed_stale": removed, + "prune_skipped": prune_skipped, } log.info("Subscription import (user %s): %s", user.id, result) return result diff --git a/backend/app/sync/videos.py b/backend/app/sync/videos.py index 1b3178e..2a8cba9 100644 --- a/backend/app/sync/videos.py +++ b/backend/app/sync/videos.py @@ -14,7 +14,6 @@ from app import progress, sysconfig from app.models import LIVE_OR_UPCOMING, Channel, Video from app.titles import normalize_title from app.youtube.client import YouTubeClient, best_thumbnail -from app.youtube.rss import fetch_channel_feed from app.youtube.shorts import make_client, probe_is_short _DURATION_RE = re.compile( @@ -84,10 +83,6 @@ def apply_rss_feed(db: Session, channel: Channel, entries: list[dict]) -> int: return inserted -def poll_rss_channel(db: Session, channel: Channel) -> int: - return apply_rss_feed(db, channel, fetch_channel_feed(channel.id)) - - # --- Backfill (uploads playlist; costs quota) --- def _stub_from_playlist_item(item: dict, channel_id: str) -> dict | None: content = item.get("contentDetails", {}) diff --git a/backend/app/worker.py b/backend/app/worker.py index 3774c27..97819ec 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -16,6 +16,7 @@ Design: * Crash-safe: on startup, orphaned `running` jobs are requeued and `downloading` assets reset. """ import logging +import re import shutil import signal import threading @@ -93,24 +94,33 @@ def _parse_upload_date(raw) -> date | None: # --- claim + recovery ----------------------------------------------------------------------- -def _wait_for_schema(timeout: float = 300.0) -> None: - """Block until the download tables exist before doing any DB work. +def _wait_for_schema(timeout: float = 300.0) -> bool: + """Block until the download tables exist before doing any DB work. Returns False if they never + appeared (or we were asked to stop while waiting). The API applies migrations on its OWN startup, and the worker is a separate container that may boot first (they start in parallel). Without this, the worker would hit a missing `download_jobs` - table and crash-loop until the API's migrations land. Poll patiently instead.""" + table and crash-loop until the API's migrations land. Poll patiently instead — and if the schema + is still absent when the timeout runs out, say so and let the caller exit, because "continuing" + only moved the same crash one call further down (into `_recover_orphans`), where it read as an + unrelated failure.""" deadline = time.monotonic() + timeout while not _stop.is_set(): try: with SessionLocal() as db: db.execute(text("SELECT 1 FROM download_jobs LIMIT 1")) - return + return True except Exception as exc: # noqa: BLE001 — table not migrated yet / DB not up yet if time.monotonic() > deadline: - log.warning("download schema still absent after %ss; continuing: %s", timeout, str(exc)[:120]) - return + log.error( + "download schema still absent after %ss — the API's migrations have not landed; " + "exiting so the container restarts: %s", + timeout, str(exc)[:120], + ) + return False log.info("waiting for the database schema (API migrations)…") _stop.wait(3.0) + return False def _recover_orphans() -> None: @@ -263,15 +273,14 @@ def _fill_asset_meta_early(asset_id: int, info: dict) -> None: def _make_progress_hook(job_id: int, asset_id: int): - state = {"last_db": 0.0, "last_check": 0.0, "meta_done": False} + state = {"meta_done": False} + cancelled = _cancel_poll(job_id) # same policy as the ffmpeg path + allow_write = _rate_limiter(_DB_WRITE_INTERVAL_S) def hook(d: dict) -> None: - now = time.monotonic() # Cooperative pause/cancel: if the job is no longer 'running', abort the download. - if now - state["last_check"] >= 2.0: - state["last_check"] = now - if _job_status(job_id) not in ("running", None): - raise _Aborted + if cancelled(): + raise _Aborted if not state["meta_done"] and d.get("info_dict"): state["meta_done"] = True try: @@ -280,9 +289,8 @@ def _make_progress_hook(job_id: int, asset_id: int): pass if d.get("status") != "downloading": return - if now - state["last_db"] < 1.0: + if not allow_write(): return - state["last_db"] = now # yt-dlp downloads the video and audio streams SEPARATELY (each 0→100%), then merges. # Label the phase so the user understands the bar resetting between streams instead of # seeing a mysterious 95%→5% jump. @@ -309,6 +317,68 @@ def _make_progress_hook(job_id: int, asset_id: int): return hook +# Both progress sources call back FAR faster than the database should be touched: yt-dlp fires its +# hook per chunk, and ffmpeg's `-progress pipe:1` emits a block of ~12 lines every ~0.5s while +# run_ffmpeg calls its callbacks once per LINE. Every callback here is a round-trip (`_job_status` +# opens a session and SELECTs; `_set_job` opens one and UPDATE+COMMITs), so unthrottled they cost +# ~10^5 queries on a long job. ONE interval, ONE implementation, used by both paths — three +# hand-rolled copies is how the ffmpeg side ended up unthrottled while the download side was fine. +_DB_WRITE_INTERVAL_S = 1.0 +_CANCEL_POLL_INTERVAL_S = 1.0 + + +def _rate_limiter(interval_s: float, *, now=time.monotonic): + """`should_run()` → True at most once per `interval_s` (the first call always passes). + `now` is injectable so tests drive a clock instead of patching the global one.""" + # `None`, not 0.0: "never ran" has to be a VALUE, not a falsy timestamp. A truthiness test here + # reads a legitimate `now() == 0.0` as "never ran" and lets a second call straight through — + # unreachable with real `time.monotonic()`, but `now` exists to be injected, and a test clock + # starting at 0 is the obvious choice. That would let a throttle test encode the wrong + # behaviour and still pass. + state: dict[str, float | None] = {"at": None} + + def should_run() -> bool: + t = now() + last = state["at"] + if last is not None and t - last < interval_s: + return False + state["at"] = t + return True + + return should_run + + +def _cancel_poll(job_id: int, interval_s: float = _CANCEL_POLL_INTERVAL_S, *, now=time.monotonic): + """Cooperative "has this job been paused/cancelled?" poll, rate-limited and sticky: once it + says yes it never queries again. Shared by the yt-dlp hook and run_ffmpeg so cancel latency is + one policy, not one per call site.""" + allow = _rate_limiter(interval_s, now=now) + state = {"cancelled": False} + + def cancelled() -> bool: + if not state["cancelled"] and allow(): + state["cancelled"] = _job_status(job_id) not in ("running", None) + return state["cancelled"] + + return cancelled + + +def _ffmpeg_progress_writer(job_id: int, phase: str, *, now=time.monotonic): + """`on_progress` for run_ffmpeg: persist at most one row write per interval, and only when the + whole-percent value actually changed (the bar shows integers).""" + allow = _rate_limiter(_DB_WRITE_INTERVAL_S, now=now) + state = {"pct": -1} + + def write(pct: float) -> None: + whole = int(pct) + if whole == state["pct"] or not allow(): + return + state["pct"] = whole + _set_job(job_id, progress=whole, phase=phase) + + return write + + def _pp_phase(pp: str) -> str: """Map a yt-dlp postprocessor class name to a user-facing phase label. ffmpeg post-steps aren't byte-progress operations (no %), so the UI shows the step name + an indeterminate @@ -341,13 +411,69 @@ def _staging_dir(asset_id: int) -> Path: return Path(settings.download_root) / ".staging" / f"asset-{asset_id}" +_THUMB_SUFFIXES = (".jpg", ".jpeg", ".png", ".webp") +# 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 +# stream as a ready asset. +_YTDLP_TEMP_RE = re.compile( + r"\.part(-Frag\d+)?$" # in-flight download / fragment + r"|\.ytdl$" # resume state + r"|\.temp\.[^.]+$" # merge/postprocess temp + r"|\.f\d+\.[^.]+$", # per-format stream kept before the merge + re.IGNORECASE, +) + + def _find_thumbnail(staging: Path, media: Path) -> Path | None: for p in staging.iterdir(): - if p != media and p.suffix.lower() in (".jpg", ".jpeg", ".png", ".webp"): + if p != media and p.suffix.lower() in _THUMB_SUFFIXES: return p return None +def _produced_file(info: dict, staging: Path) -> Path: + """The media file yt-dlp actually wrote. + + `requested_downloads[0].filepath` is the authoritative answer on YouTube, but an arbitrary-URL + extractor may not fill it in at all — and `Path("")` is `Path(".")`, which *passes* `.exists()`, + so a missing value used to sail past the fallback and blow up two steps later (`with_name` on an + empty name: the BBC-news failure). Anything that isn't a real file counts as absent; then fall + back to the largest file left in staging that is neither a thumbnail nor one of yt-dlp's own + working files (see `_YTDLP_TEMP_RE`).""" + req = (info.get("requested_downloads") or [{}])[0] + fp = req.get("filepath") or req.get("_filename") or info.get("filepath") or info.get("_filename") + if fp: + produced = Path(str(fp)) + if produced.is_file(): + return produced + cands = [ + p + for p in staging.iterdir() + if p.is_file() + and p.suffix.lower() not in _THUMB_SUFFIXES + and not _YTDLP_TEMP_RE.search(p.name) + ] + if not cands: + raise RuntimeError("yt-dlp produced no output file") + return max(cands, key=lambda p: p.stat().st_size) + + +def _media_info(info: dict) -> dict: + """Unwrap a playlist/multi_video extraction to the entry that carries the download. + + A single non-YouTube page (a news article's video, a multi-rendition embed) can extract as a + wrapper whose own dict has no `requested_downloads` — the media lives on an entry. Pick the + FIRST entry that actually downloaded something: on a page with a skipped teaser in front of the + real video, entries[0] carries no file, and taking it blindly would label the produced file + with the wrong id/title/duration.""" + entries = [e for e in (info.get("entries") or []) if e] + if not entries or info.get("requested_downloads"): + return info + chosen = next((e for e in entries if e.get("requested_downloads")), entries[0]) + return {**info, **chosen} + + # Errors worth retrying with a different player-client set (YouTube per-client flakiness). _RETRIABLE_MARKERS = ( "not a bot", @@ -426,8 +552,8 @@ def _ensure_browser_playable(job_id: int, produced: Path, spec: dict, info: dict editmod.run_ffmpeg( editmod.build_browser_safe_cmd(produced, dest, reenc_v, reenc_a), total_s=total_s, - on_progress=lambda pct: _set_job(job_id, progress=int(pct), phase="processing"), - should_cancel=lambda: _job_status(job_id) not in ("running", None), + on_progress=_ffmpeg_progress_writer(job_id, "processing"), + should_cancel=_cancel_poll(job_id), ) produced.unlink(missing_ok=True) # Keep the stored asset metadata truthful about what's now on disk. @@ -447,17 +573,9 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe _set_job(job_id, phase="downloading", progress=0) url = service.source_url(source_kind, source_ref) - info = _extract_with_fallback(job_id, asset_id, url, spec, staging) - - req = (info.get("requested_downloads") or [{}])[0] - produced = Path(req.get("filepath") or "") - if not produced.exists(): - # Fallback: the single media file left in staging (ignore the thumbnail). - cands = [p for p in staging.iterdir() if p.suffix.lower() not in (".jpg", ".jpeg", ".png", ".webp")] - if not cands: - raise RuntimeError("yt-dlp produced no output file") - produced = cands[0] + info = _media_info(_extract_with_fallback(job_id, asset_id, url, spec, staging)) + produced = _produced_file(info, staging) produced = _ensure_browser_playable(job_id, produced, spec, info) ext = produced.suffix.lstrip(".").lower() meta = storage.MediaMeta( @@ -613,8 +731,8 @@ def _process_edit(job_id: int, asset_id: int) -> None: editmod.run_ffmpeg( cmd, total_s, - on_progress=lambda pct: _set_job(job_id, progress=int(pct), phase="editing"), - should_cancel=lambda: _job_status(job_id) not in ("running", None), + on_progress=_ffmpeg_progress_writer(job_id, "editing"), + should_cancel=_cancel_poll(job_id), ) if not dest_staging.exists(): raise RuntimeError("ffmpeg produced no output file") @@ -689,9 +807,14 @@ def main() -> None: log.info("WORKER_ENABLED is off; worker exiting (nothing to do).") return - _wait_for_schema() # the API may still be applying migrations when we boot - if _stop.is_set(): - return + # The API may still be applying migrations when we boot. If they never arrive, exit instead of + # pressing on into a guaranteed crash — with a FAILURE status, so the container comes back under + # any restart policy (`on-failure` too, not just the `unless-stopped` our composes ship) and the + # exit code doesn't claim success for a worker that never started working. + if not _wait_for_schema(): + if _stop.is_set(): + return # asked to shut down while waiting — that IS a clean exit + raise SystemExit(1) _recover_orphans() n = max(1, settings.download_worker_concurrency) log.info("Download worker started (concurrency=%d, root=%s).", n, settings.download_root) diff --git a/backend/app/youtube/rss.py b/backend/app/youtube/rss.py index 99defa0..73cb3ee 100644 --- a/backend/app/youtube/rss.py +++ b/backend/app/youtube/rss.py @@ -8,14 +8,24 @@ import httpx RSS_URL = "https://www.youtube.com/feeds/videos.xml?channel_id={channel_id}" +class RssError(Exception): + """The feed could not be read (transport failure or a non-200 answer). + + Deliberately NOT the same as an empty feed: a failed poll used to return `[]`, which the + caller stamped as a successful `last_rss_at` — so a channel whose feed 404s permanently + (deleted, renamed id) looked like it was being polled fine forever, and nothing surfaced it.""" + + def fetch_channel_feed(channel_id: str) -> list[dict]: + """The channel's recent uploads as stubs. `[]` means the feed really is empty; a failure + raises RssError so the caller can leave the channel un-stamped and log it.""" url = RSS_URL.format(channel_id=channel_id) try: resp = httpx.get(url, timeout=20.0, headers={"User-Agent": "Siftlode/1.0"}) - except httpx.HTTPError: - return [] + except httpx.HTTPError as exc: + raise RssError(f"RSS fetch failed for {channel_id}: {exc}") from exc if resp.status_code != 200: - return [] + raise RssError(f"RSS fetch for {channel_id} returned HTTP {resp.status_code}") parsed = feedparser.parse(resp.content) out: list[dict] = [] diff --git a/backend/tests/test_download_output.py b/backend/tests/test_download_output.py new file mode 100644 index 0000000..3f9e51c --- /dev/null +++ b/backend/tests/test_download_output.py @@ -0,0 +1,139 @@ +"""Picking the file yt-dlp actually produced, for sources that aren't YouTube. + +The regression these cover: `Path(info["filepath"] or "")` is `Path(".")`, which PASSES `.exists()`, +so a missing filepath (an arbitrary-URL extractor) skipped the staging fallback and crashed later +with "PosixPath('.') has an empty name" instead of downloading.""" +import pytest + +from app.downloads.edit import _concat_quote, parse_probe_streams +from app.worker import _media_info, _produced_file +from pathlib import Path, PurePosixPath + + +def _staging(tmp_path: Path, *names: str) -> Path: + for n in names: + p = tmp_path / n + p.write_bytes(b"x" * (10 * (names.index(n) + 1))) + return tmp_path + + +class TestProducedFile: + def test_uses_the_reported_filepath(self, tmp_path): + staging = _staging(tmp_path, "v.mp4", "v.jpg") + info = {"requested_downloads": [{"filepath": str(tmp_path / "v.mp4")}]} + assert _produced_file(info, staging) == tmp_path / "v.mp4" + + @pytest.mark.parametrize("info", [{}, {"requested_downloads": [{}]}, {"requested_downloads": [{"filepath": ""}]}]) + def test_missing_filepath_falls_back_to_staging(self, tmp_path, info): + staging = _staging(tmp_path, "thumb.jpg", "clip.mp4") + assert _produced_file(info, staging) == tmp_path / "clip.mp4" + + def test_stale_filepath_falls_back_to_staging(self, tmp_path): + staging = _staging(tmp_path, "clip.mp4") + info = {"requested_downloads": [{"filepath": str(tmp_path / "gone.mp4")}]} + assert _produced_file(info, staging) == tmp_path / "clip.mp4" + + def test_picks_the_largest_media_file_ignoring_thumbs_and_partials(self, tmp_path): + staging = _staging(tmp_path, "small.mp4", "big.mp4", "cover.webp") + (tmp_path / "huge.mp4.part").write_bytes(b"x" * 999) + assert _produced_file({}, staging) == tmp_path / "big.mp4" + + @pytest.mark.parametrize( + "leftover", + ["vid.f137.mp4", "vid.mp4.part-Frag12", "vid.ytdl", "vid.temp.mp4", "vid.mp4.part"], + ) + def test_ytdlp_working_files_never_win(self, tmp_path, leftover): + # Each of these can legitimately be the BIGGEST file in staging (a video-only per-format + # stream especially) — storing one would serve a silent or truncated "download". + (tmp_path / leftover).write_bytes(b"x" * 5000) + (tmp_path / "vid.mp4").write_bytes(b"x" * 10) + assert _produced_file({}, tmp_path) == tmp_path / "vid.mp4" + + def test_only_leftovers_is_treated_as_no_output(self, tmp_path): + (tmp_path / "vid.f251.webm").write_bytes(b"x" * 5000) + with pytest.raises(RuntimeError): + _produced_file({}, tmp_path) + + def test_no_output_at_all_raises(self, tmp_path): + staging = _staging(tmp_path, "only.jpg") + with pytest.raises(RuntimeError): + _produced_file({}, staging) + + +class TestMediaInfo: + def test_playlist_wrapper_unwraps_to_the_entry_that_downloaded(self): + info = {"id": "page", "title": "Article", "entries": [{"id": "vid", "requested_downloads": [{"filepath": "/x.mp4"}]}]} + assert _media_info(info)["id"] == "vid" + + def test_plain_result_is_untouched(self): + info = {"id": "vid", "requested_downloads": [{"filepath": "/x.mp4"}]} + assert _media_info(info) is info + + def test_picks_the_entry_that_actually_downloaded(self): + # A page whose first entry was skipped: taking entries[0] would label the produced file + # with the teaser's id/title. + info = { + "id": "page", + "entries": [ + {"id": "teaser", "title": "Teaser"}, + {"id": "main", "title": "Main", "requested_downloads": [{"filepath": "/x.mp4"}]}, + ], + } + assert _media_info(info)["id"] == "main" + + def test_falls_back_to_the_first_entry_when_none_downloaded(self): + info = {"id": "page", "entries": [{"id": "a"}, {"id": "b"}]} + assert _media_info(info)["id"] == "a" + + def test_wrapper_that_downloaded_itself_wins_over_its_entries(self): + info = {"id": "vid", "requested_downloads": [{"filepath": "/x.mp4"}], "entries": [{"id": "other"}]} + assert _media_info(info)["id"] == "vid" + + def test_empty_entries_are_ignored(self): + info = {"id": "vid", "entries": [None]} + assert _media_info(info)["id"] == "vid" + + +class TestParseProbeStreams: + # ffprobe emits codec_name BEFORE codec_type, so the old line parser shifted every name onto + # the previous stream and an H.264+AAC mp4 came back as (aac, None) → a needless re-encode. + def test_reads_each_stream_by_its_own_type(self): + payload = '{"streams":[{"codec_name":"h264","codec_type":"video"},{"codec_name":"aac","codec_type":"audio"}]}' + assert parse_probe_streams(payload) == ("h264", "aac") + + def test_audio_first_ordering_is_handled(self): + payload = '{"streams":[{"codec_name":"AAC","codec_type":"audio"},{"codec_name":"H264","codec_type":"video"}]}' + assert parse_probe_streams(payload) == ("h264", "aac") + + def test_extra_streams_do_not_win_over_the_first_of_their_kind(self): + payload = ( + '{"streams":[{"codec_name":"h264","codec_type":"video"},' + '{"codec_name":"vp9","codec_type":"video"},' + '{"codec_name":"mov_text","codec_type":"subtitle"},' + '{"codec_name":"opus","codec_type":"audio"}]}' + ) + assert parse_probe_streams(payload) == ("h264", "opus") + + def test_audio_only_and_video_only(self): + assert parse_probe_streams('{"streams":[{"codec_name":"opus","codec_type":"audio"}]}') == (None, "opus") + assert parse_probe_streams('{"streams":[{"codec_name":"av1","codec_type":"video"}]}') == ("av1", None) + + def test_garbage_is_not_a_crash(self): + assert parse_probe_streams("") == (None, None) + assert parse_probe_streams("not json") == (None, None) + + +class TestConcatQuote: + # PurePosixPath so the escaping is asserted identically on a Windows dev box and in the + # Linux test container (a WindowsPath would eat the backslash as a separator). + def test_apostrophe_is_closed_escaped_and_reopened(self): + # A pre-0.51.0 file can still be named with an apostrophe; unescaped it breaks the list. + assert _concat_quote(PurePosixPath("/m/Don't_Look.mp4")) == "/m/Don'\\''t_Look.mp4" + + def test_backslash_stays_literal(self): + # Inside single quotes ffmpeg treats `\` literally — doubling it would point at a + # different (non-existent) path. + assert _concat_quote(PurePosixPath("/m/a\\b.mp4")) == "/m/a\\b.mp4" + + def test_ordinary_path_is_unchanged(self): + assert _concat_quote(PurePosixPath("/m/clip.mp4")) == "/m/clip.mp4" diff --git a/backend/tests/test_ffmpeg_callbacks.py b/backend/tests/test_ffmpeg_callbacks.py new file mode 100644 index 0000000..d4173b5 --- /dev/null +++ b/backend/tests/test_ffmpeg_callbacks.py @@ -0,0 +1,79 @@ +"""The rate limit in front of run_ffmpeg's callbacks. + +run_ffmpeg calls `on_progress`/`should_cancel` once per ffmpeg output LINE (`-progress pipe:1` +emits a block of ~12 every half second), and both callbacks are database round-trips: `_set_job` +opens a session and UPDATE+COMMITs, `_job_status` opens one and SELECTs. Unthrottled, a long +re-encode is ~10^5 of each.""" +from app import worker + + +class TestProgressWriter: + def test_writes_the_first_value_immediately(self, monkeypatch): + writes = [] + monkeypatch.setattr(worker, "_set_job", lambda job_id, **f: writes.append(f)) + worker._ffmpeg_progress_writer(7, "processing")(12.4) + assert writes == [{"progress": 12, "phase": "processing"}] + + def test_a_burst_of_lines_costs_one_write(self, monkeypatch): + writes = [] + monkeypatch.setattr(worker, "_set_job", lambda job_id, **f: writes.append(f)) + write = worker._ffmpeg_progress_writer(7, "processing") + for pct in range(0, 40): # what one second of ffmpeg output looks like + write(pct) + assert len(writes) == 1 + + def test_the_next_second_writes_again(self, monkeypatch): + writes = [] + clock = {"t": 1000.0} + monkeypatch.setattr(worker, "_set_job", lambda job_id, **f: writes.append(f)) + write = worker._ffmpeg_progress_writer(7, "processing", now=lambda: clock["t"]) + write(10) + write(11) # same second → dropped + clock["t"] += 1.5 + write(12) + assert [w["progress"] for w in writes] == [10, 12] + + def test_an_unchanged_whole_percent_is_never_rewritten(self, monkeypatch): + writes = [] + clock = {"t": 1000.0} + monkeypatch.setattr(worker, "_set_job", lambda job_id, **f: writes.append(f)) + write = worker._ffmpeg_progress_writer(7, "processing", now=lambda: clock["t"]) + write(10.1) + clock["t"] += 5 + write(10.9) # still 10% — the bar shows integers + assert [w["progress"] for w in writes] == [10] + + +class TestCancelPoll: + def test_asks_once_then_reuses_the_answer(self, monkeypatch): + calls = [] + monkeypatch.setattr(worker, "_job_status", lambda job_id: calls.append(job_id) or "running") + cancelled = worker._cancel_poll(7) + for _ in range(40): + assert cancelled() is False + assert len(calls) == 1 + + def test_a_cancelled_job_is_reported_and_never_queried_again(self, monkeypatch): + calls = [] + clock = {"t": 1000.0} + + def status(job_id): + calls.append(job_id) + return "cancelled" + + monkeypatch.setattr(worker, "_job_status", status) + cancelled = worker._cancel_poll(7, now=lambda: clock["t"]) + assert cancelled() is True + clock["t"] += 60 # cancellation is sticky: no re-query, and no un-cancelling + assert cancelled() is True + assert len(calls) == 1 + + def test_a_cancel_arriving_later_is_seen_on_the_next_tick(self, monkeypatch): + clock = {"t": 1000.0} + state = {"status": "running"} + monkeypatch.setattr(worker, "_job_status", lambda job_id: state["status"]) + cancelled = worker._cancel_poll(7, now=lambda: clock["t"]) + assert cancelled() is False + state["status"] = "paused" + clock["t"] += 1.0 # the throttle must not delay a cancel by more than its interval + assert cancelled() is True diff --git a/backend/tests/test_plex_stream.py b/backend/tests/test_plex_stream.py new file mode 100644 index 0000000..aab0654 --- /dev/null +++ b/backend/tests/test_plex_stream.py @@ -0,0 +1,155 @@ +"""The guards in front of a recursive delete. + +`sweep_orphans` rmtree's directories under `_HLS_ROOT`, and that root is admin-configurable +(PLEX_HLS_DIR) — it can legitimately be a shared scratch path. `_SESSION_DIR_RE` is the ONLY thing +that keeps the sweep off a neighbour's data, so the names it accepts are pinned here. + +Since the sweep stopped being awaited at startup it also runs CONCURRENTLY with playback, so the +second half of this file pins what keeps it off a LIVE session's segments.""" +import os + +import pytest + +from app.plex import stream +from app.plex.stream import _SESSION_DIR_RE + + +@pytest.mark.parametrize( + "name", + [ + "12345_0", # legacy shape (pre multi-audio): {rating_key}_{start} + "12345_842", + "12345_0_None_+0.00", # current shape, single audio track (audio_ord None) + "12345_0_2_+0.00", # current shape, explicit audio_ord + "12345_842_multi_-1.25", # multi-audio, negative offset + "12345_842_multi_+12.50", + ], +) +def test_our_session_directories_are_swept(name): + assert _SESSION_DIR_RE.match(name) + + +@pytest.mark.parametrize( + "name", + [ + "backup_2024", # the class an "optional tail" pattern wrongly matched + "release_10", + "important_data_2024", + "node_modules", + "logs", + "plex-hls", + "12345", # no start offset — not a session dir + "12345_0_multi", # truncated: missing the offset + "12345_0_multi_1.25", # offset without its sign + "snapshot_2024_backup_+1.00", # session-ish tail, but the key isn't a rating_key + "backup_2024_multi_+0.00", + ".config", + ], +) +def test_foreign_directories_are_left_alone(name): + assert not _SESSION_DIR_RE.match(name) + + +class _FakeSession: + def __init__(self, directory): + self.dir = directory + + +@pytest.fixture +def hls_root(tmp_path, monkeypatch): + """Point the sweep at a temp root with no sessions registered.""" + monkeypatch.setattr(stream, "_HLS_ROOT", tmp_path) + monkeypatch.setattr(stream, "_sessions", {}) + return tmp_path + + +def _session_dir(root, name, *, age_s=0.0): + """A session directory, optionally backdated by `age_s` to look like a previous run's.""" + d = root / name + d.mkdir() + (d / "seg_0.ts").write_bytes(b"x") + if age_s: + t = d.stat().st_mtime - age_s + os.utime(d, (t, t)) + return d + + +def _cutoff(root): + """A "process start" read from the FILESYSTEM, not from a clock. + + The guard compares a directory's mtime against a timestamp, and the two do not have to come + from the same clock: on a bind-mounted dev volume the file times are stamped by the host while + `time.time()` runs in the container VM, and the drift between them is real (it made an earlier + version of these tests flaky). Anchoring the cutoff to a file this test just created removes + the clock from the assertion; that the DEFAULT cutoff is `_PROCESS_START` is pinned separately + below.""" + probe = root / ".cutoff-probe" + probe.write_bytes(b"") + at = probe.stat().st_mtime + probe.unlink() + return at + + +class TestSweepOrphans: + def test_a_previous_runs_leftovers_are_deleted(self, hls_root): + d = _session_dir(hls_root, "12345_0_None_+0.00", age_s=3600) + assert stream.sweep_orphans(older_than=_cutoff(hls_root)) == 1 + assert not d.exists() + + def test_foreign_directories_survive(self, hls_root): + keep = hls_root / "backup_2024" + keep.mkdir() + (keep / "important.tar").write_bytes(b"x") + assert stream.sweep_orphans(older_than=_cutoff(hls_root)) == 0 + assert (keep / "important.tar").exists() + + def test_a_live_sessions_directory_is_never_deleted(self, hls_root): + # Registered session, but with an OLD directory — only the live check can save it. + d = _session_dir(hls_root, "12345_0_multi_+0.00", age_s=3600) + stream._sessions["k"] = _FakeSession(d) + assert stream.sweep_orphans(older_than=_cutoff(hls_root)) == 0 + assert d.exists() + + def test_a_directory_this_run_created_is_never_deleted(self, hls_root): + # THE RACE: the sweep is no longer awaited before the app serves, and `start_session` + # REUSES the path of a same-key session — so between our listing and our rmtree, a stale + # name can become a live directory. A fresh mtime is what tells them apart, and it must + # hold even while `_sessions` is still empty (the session registers a beat later). + cutoff = _cutoff(hls_root) + fresh = _session_dir(hls_root, "12345_0_None_+0.00") # created "after boot" + old = _session_dir(hls_root, "999_0_None_+0.00", age_s=3600) + assert stream.sweep_orphans(older_than=cutoff) == 1 # only the leftover + assert fresh.exists() + assert not old.exists() + + def test_the_default_cutoff_is_this_processs_start(self, hls_root, monkeypatch): + # The parameter exists for the tests; production calls it with no argument, so pin what + # that means — otherwise the guard above could be right and unused. + d = _session_dir(hls_root, "12345_0_None_+0.00") + monkeypatch.setattr(stream, "_MTIME_GRACE_S", 0.0) + monkeypatch.setattr(stream, "_PROCESS_START", d.stat().st_mtime + 1) + assert stream.sweep_orphans() == 1 # "older than boot" → swept + assert not d.exists() + + def test_a_directory_stamped_just_before_boot_survives(self, hls_root, monkeypatch): + # MEASURED in the container, not theory: a file's mtime comes from the kernel's COARSE + # clock and `time.time()` from the fine one, so a directory created 2ms AFTER + # `_PROCESS_START` was stamped 2ms BEFORE it. Without the grace the sweep deletes a session + # that started in the first instants after boot — which is exactly when the sweep runs. + d = _session_dir(hls_root, "12345_0_None_+0.00") + monkeypatch.setattr(stream, "_PROCESS_START", d.stat().st_mtime + 0.05) # the skew + assert stream.sweep_orphans() == 0 + assert d.exists() + + def test_a_missing_root_is_not_an_error(self, tmp_path, monkeypatch): + monkeypatch.setattr(stream, "_HLS_ROOT", tmp_path / "nope") + monkeypatch.setattr(stream, "_sessions", {}) + assert stream.sweep_orphans() == 0 + + def test_a_stray_file_with_a_session_name_is_left_alone(self, hls_root): + # Only directories are ours; a same-named FILE belongs to someone else. + f = hls_root / "12345_0" + f.write_bytes(b"x") + os.utime(f, (f.stat().st_mtime - 3600,) * 2) # old enough that only is_dir() saves it + assert stream.sweep_orphans(older_than=_cutoff(hls_root)) == 0 + assert f.exists() diff --git a/backend/tests/test_run_ffmpeg.py b/backend/tests/test_run_ffmpeg.py new file mode 100644 index 0000000..d72bde2 --- /dev/null +++ b/backend/tests/test_run_ffmpeg.py @@ -0,0 +1,182 @@ +"""run_ffmpeg's process plumbing — the part that can hang a worker thread forever. + +No ffmpeg here: a fake Popen stands in, so the reader threads, the 1s poll loop, the stall watchdog +and the bounded exit wait are all exercised on any machine. These are regression tests for a real +deadlock: stderr was opened and never read, so a chatty ffmpeg filled the ~64KB pipe, blocked on its +next write and never exited, and `proc.wait()` never returned.""" +import subprocess +import time + +import pytest + +from app.downloads import edit + + +class _FakeStream: + """A pipe. `block=True` = the process is alive but silent (a wedged ffmpeg): iteration parks + until the stream is closed, which is what terminate()/kill() does to a real pipe.""" + + def __init__(self, lines=(), block=False): + self._lines = list(lines) + self._block = block + self.closed = False + self.consumed = 0 + + def __iter__(self): + return self + + def __next__(self): + if self._lines: + self.consumed += 1 + return self._lines.pop(0) + while self._block and not self.closed: + time.sleep(0.02) + raise StopIteration + + def close(self): + self.closed = True + + +class _FakeProc: + pid = 4242 + + def __init__(self, stdout, stderr, returncode=0, wait_timeouts=0): + self.stdout = stdout + self.stderr = stderr + self._rc = returncode + # How many of the first `wait()` calls raise TimeoutExpired — a process ignoring SIGTERM, + # or one that lingers after closing its pipes. + self._wait_timeouts = wait_timeouts + self.terminated = False + self.killed = False + self.waits = 0 + + def poll(self): + # Popen.poll(): None while the process is alive. Our fake stays alive until it is stopped — + # `_stop` uses this to skip a process that has already gone. + return self._rc if self.terminated else None + + def terminate(self): + self.terminated = True + # A terminated process's pipes hit EOF — that is how the reader threads finish. + self.stdout.close() + self.stderr.close() + + def kill(self): + self.killed = True + self.terminate() + + def wait(self, timeout=None): + self.waits += 1 + if self._wait_timeouts > 0: + self._wait_timeouts -= 1 + raise subprocess.TimeoutExpired("ffmpeg", timeout or 0) + return self._rc + + +@pytest.fixture +def fake_popen(monkeypatch): + """Install a fake Popen and hand the test the process it will produce.""" + + def install(proc): + monkeypatch.setattr(edit.subprocess, "Popen", lambda *a, **k: proc) + return proc + + return install + + +def test_progress_is_reported_and_a_clean_exit_returns(fake_popen): + proc = fake_popen( + _FakeProc(_FakeStream(["out_time_us=5000000\n", "progress=continue\n"]), _FakeStream()) + ) + seen = [] + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=seen.append, should_cancel=lambda: False) + assert seen == [50.0] # 5s of 10s + assert not proc.terminated + + +def test_progress_is_capped_below_100(fake_popen): + fake_popen(_FakeProc(_FakeStream(["out_time_us=99000000\n"]), _FakeStream())) + seen = [] + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=seen.append, should_cancel=lambda: False) + assert seen == [99.0] # never reports done before the process actually exits + + +def test_a_nonzero_exit_raises_with_the_stderr_tail(fake_popen): + # The tail must come from the drained stderr — reading the pipe only AFTER the exit was the + # original deadlock. + proc = fake_popen( + _FakeProc(_FakeStream([]), _FakeStream(["Invalid data found\n"]), returncode=2) + ) + with pytest.raises(RuntimeError, match="Invalid data found"): + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=lambda _p: None, should_cancel=lambda: False) + assert not proc.terminated # it exited on its own; nothing to kill + + +def test_cancel_is_honoured_while_the_process_is_silent(fake_popen): + # The pre-fix loop only checked cancel when a progress line arrived, so a quiet ffmpeg was + # uncancellable. The 1s poll tick is what makes this terminate. + proc = fake_popen(_FakeProc(_FakeStream(block=True), _FakeStream(block=True))) + started = time.monotonic() + with pytest.raises(edit.EditAborted): + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=lambda _p: None, should_cancel=lambda: True) + assert proc.terminated + assert time.monotonic() - started < 5 # not "whenever ffmpeg next speaks" + + +def test_a_wedged_process_is_killed_by_the_stall_watchdog(fake_popen): + proc = fake_popen(_FakeProc(_FakeStream(block=True), _FakeStream(block=True))) + with pytest.raises(RuntimeError, match="stalled"): + edit.run_ffmpeg( + ["ffmpeg"], + total_s=10, + on_progress=lambda _p: None, + should_cancel=lambda: False, + stall_timeout_s=0.5, + ) + assert proc.terminated # the worker thread is freed, not parked forever + + +def test_a_callback_exception_still_stops_the_process(fake_popen): + # `on_progress` is a database write in production. A failure THERE used to unwind past a live + # ffmpeg — orphaning it, still encoding, after a 10s join on its pipes. + proc = fake_popen(_FakeProc(_FakeStream(["out_time_us=1000000\n"]), _FakeStream(block=True))) + + def boom(_pct): + raise RuntimeError("database is down") + + with pytest.raises(RuntimeError, match="database is down"): + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=boom, should_cancel=lambda: False) + assert proc.terminated + + +def test_a_process_that_ignores_sigterm_is_killed(fake_popen): + # `_stop`'s escalation: terminate, wait 5s, then SIGKILL. Without it a cancel could park the + # worker thread on a process that simply refuses to go. + proc = fake_popen( + _FakeProc(_FakeStream(block=True), _FakeStream(block=True), wait_timeouts=1) + ) + with pytest.raises(edit.EditAborted): + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=lambda _p: None, should_cancel=lambda: True) + assert proc.terminated and proc.killed + + +def test_a_process_lingering_after_eof_is_stopped_and_reported(fake_popen): + # Pipes closed but the process still around: the bounded wait must give up rather than block + # the worker thread on `proc.wait()` forever (the old code waited with no timeout at all). + proc = fake_popen(_FakeProc(_FakeStream([]), _FakeStream(), wait_timeouts=1)) + with pytest.raises(RuntimeError, match="did not exit"): + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=lambda _p: None, should_cancel=lambda: False) + assert proc.terminated + # Two waits: the bounded grace wait that timed out, then _stop's own — never an unbounded one. + assert proc.waits == 2 + + +def test_stderr_is_drained_while_the_process_runs(fake_popen): + # The deadlock case: a chatty ffmpeg that exits 0. EVERY stderr line must be consumed — with a + # real pipe, an unread one fills at ~64KB and blocks ffmpeg's next write forever. + err = _FakeStream([f"warning {i}\n" for i in range(500)]) + fake_popen(_FakeProc(_FakeStream(["out_time_us=1000000\n"]), err)) + edit.run_ffmpeg(["ffmpeg"], total_s=10, on_progress=lambda _p: None, should_cancel=lambda: False) + assert err.consumed == 500 + assert err.closed diff --git a/backend/tests/test_sync_guards.py b/backend/tests/test_sync_guards.py new file mode 100644 index 0000000..754bf73 --- /dev/null +++ b/backend/tests/test_sync_guards.py @@ -0,0 +1,225 @@ +"""Guards that stop one bad YouTube answer from cascading: the subscription prune guard, the +playlist-push circuit breaker, and RSS telling a failure apart from an empty feed.""" +import httpx +import pytest + +from app.sync import runner +from app.sync.playlists import FAILED, _Breaker +from app.sync.subscriptions import should_prune +from app.youtube import rss +from app.youtube.client import YouTubeError + + +class TestShouldPrune: + def test_a_normal_fetch_prunes(self): + assert should_prune(fetched_count=12, existing_count=15) is True + + def test_an_empty_fetch_against_existing_rows_never_prunes(self): + # The data-loss case: one anomalous empty-but-200 response must not wipe 15 rows. + assert should_prune(fetched_count=0, existing_count=15) is False + + def test_an_empty_fetch_with_nothing_local_is_fine(self): + # A brand-new account: nothing to lose, so the (no-op) prune may run. + assert should_prune(fetched_count=0, existing_count=0) is True + + def test_a_single_remaining_subscription_still_prunes(self): + assert should_prune(fetched_count=1, existing_count=200) is True + + +class TestBreaker: + def test_open_until_the_limit_is_reached(self): + b = _Breaker(limit=3) + for _ in range(2): + b.hit(False) + assert b.open() + b.hit(False) + assert not b.open() and b.tripped + + def test_a_success_re_arms_the_streak(self): + b = _Breaker(limit=3) + b.hit(False) + b.hit(False) + b.hit(True) # one write went through — this isn't an account-wide failure + b.hit(False) + b.hit(False) + assert b.open() + + def test_stays_tripped_once_tripped(self): + b = _Breaker(limit=2) + b.hit(False) + b.hit(False) + b.hit(True) + assert not b.open() + + +def _quota_dead(): + """A write op that fails the way a quota-exhausted / scope-revoked account fails.""" + raise YouTubeError("quota exceeded") + + +class TestBreakerRun: + def test_returns_the_ops_result_and_leaves_failures_alone(self): + b, failed = _Breaker(), [] + assert b.run(lambda: "item-1", lambda: failed.append("v1")) == "item-1" + assert failed == [] + + def test_a_youtube_error_records_the_failure_and_counts_towards_the_trip(self): + b, failed = _Breaker(limit=2), [] + assert b.run(_quota_dead, lambda: failed.append("v1")) is FAILED + assert failed == ["v1"] and b.open() + b.run(_quota_dead, lambda: failed.append("v2")) + assert not b.open() + + def test_a_tripped_breaker_never_calls_the_op(self): + b, failed, calls = _Breaker(limit=1), [], [] + b.run(_quota_dead, lambda: failed.append("v1")) + assert b.run(lambda: calls.append("called"), lambda: failed.append("v2")) is FAILED + assert calls == [] # the whole point: no more quota spent + assert failed == ["v1", "v2"] # …but the item is still reported + + def test_other_exceptions_are_not_swallowed(self): + # Only YouTubeError is a "write failed" signal; a bug must still surface. + def boom(): + raise ValueError("bug") + + with pytest.raises(ValueError): + _Breaker().run(boom, lambda: None) + + +class _StubDb: + """Just enough Session for run_rss_poll when the caller supplies the channel list.""" + + def __init__(self): + self.rollbacks = 0 + + def rollback(self): + self.rollbacks += 1 + + +class _StubChannel: + def __init__(self, cid): + self.id = cid + + +class TestRssPollResult: + """`failed` must count every channel that did not get polled — feed-side AND apply-side. + Counting only the fetch failures would move the "a broken run looks like a quiet hour" blind + spot rather than remove it.""" + + def _run(self, monkeypatch, *, fetch, apply_, channels=("UC1", "UC2")): + monkeypatch.setattr(runner, "fetch_channel_feed", fetch) + monkeypatch.setattr(runner, "apply_rss_feed", apply_) + return runner.run_rss_poll(_StubDb(), [_StubChannel(c) for c in channels]) + + def test_a_healthy_run_reports_no_failures(self, monkeypatch): + out = self._run(monkeypatch, fetch=lambda cid: [{"id": "v"}], apply_=lambda db, ch, e: 1) + assert out == {"new": 2, "failed": 0} + + def test_unreadable_feeds_are_counted(self, monkeypatch): + # One dead channel among healthy ones: counted, but the run itself is still a run. + def fetch(cid): + if cid == "UC_dead": + raise rss.RssError("404") + return [{"id": "v"}] + + out = self._run( + monkeypatch, fetch=fetch, apply_=lambda db, ch, e: 1, channels=("UC1", "UC_dead") + ) + assert out == {"new": 1, "failed": 1} + + def test_apply_failures_are_counted_too(self, monkeypatch): + # A per-row write failure (a constraint clash on one channel) while the rest store fine. + def apply_(db, ch, entries): + if ch.id == "UC_bad": + raise RuntimeError("constraint") + return 1 + + out = self._run( + monkeypatch, + fetch=lambda cid: [{"id": "v"}], + apply_=apply_, + channels=("UC1", "UC_bad"), + ) + assert out == {"new": 1, "failed": 1}, "an unstored channel must not read as ok" + + def test_a_quiet_hour_is_distinguishable_from_an_outage(self, monkeypatch): + quiet = self._run(monkeypatch, fetch=lambda cid: [], apply_=lambda db, ch, e: 0) + assert quiet == {"new": 0, "failed": 0} # same "new", different "failed" — that's the point + + def test_a_total_outage_raises_instead_of_returning_ok(self, monkeypatch): + # Every feed unreachable (the tinyproxy-lost-the-boot-race incident). Returning a result + # here recorded the scheduler run as "ok", with the outage only visible inside the summary + # text — a green pill for a run that did nothing at all. + def boom(cid): + raise rss.RssError("Connection refused") + + with pytest.raises(RuntimeError, match="all 2 channels"): + self._run(monkeypatch, fetch=boom, apply_=lambda db, ch, e: 1) + + def test_the_outage_message_carries_the_underlying_error(self, monkeypatch): + # "all N channels failed" is ALSO what one dead feed id looks like on a small instance. + # The card has to say which: `HTTP 404` = fix the subscription, `Connection refused` = + # the egress is down. + def dead(cid): + raise rss.RssError(f"RSS fetch for {cid} returned HTTP 404") + + with pytest.raises(RuntimeError, match="HTTP 404"): + self._run(monkeypatch, fetch=dead, apply_=lambda db, ch, e: 1, channels=("UC_dead",)) + + def test_a_read_only_database_is_a_total_outage_too(self, monkeypatch): + # Every feed reads fine, nothing can be stored: just as un-polled, just as red. + def boom(db, ch, entries): + raise RuntimeError("read-only transaction") + + with pytest.raises(RuntimeError, match="read-only transaction"): + self._run(monkeypatch, fetch=lambda cid: [{"id": "v"}], apply_=boom) + + def test_no_channels_at_all_is_not_an_outage(self, monkeypatch): + # A fresh instance with nothing subscribed must not report an error every poll. + out = self._run( + monkeypatch, fetch=lambda cid: [], apply_=lambda db, ch, e: 0, channels=() + ) + assert out == {"new": 0, "failed": 0} + + +class TestRssErrorVsEmpty: + """A failed poll must RAISE (so the caller leaves `last_rss_at` alone); only a genuinely + empty feed may return [].""" + + def _patch(self, monkeypatch, **kwargs): + def fake_get(url, **_): + if "exc" in kwargs: + raise kwargs["exc"] + return httpx.Response( + kwargs["status"], + content=kwargs.get("body", b""), + request=httpx.Request("GET", url), + ) + + monkeypatch.setattr(rss.httpx, "get", fake_get) + + def test_non_200_raises(self, monkeypatch): + self._patch(monkeypatch, status=404) + with pytest.raises(rss.RssError): + rss.fetch_channel_feed("UC_dead") + + def test_transport_error_raises(self, monkeypatch): + self._patch(monkeypatch, exc=httpx.ConnectError("no route")) + with pytest.raises(rss.RssError): + rss.fetch_channel_feed("UC_offline") + + def test_an_empty_but_valid_feed_returns_empty(self, monkeypatch): + feed = b"""""" + self._patch(monkeypatch, status=200, body=feed) + assert rss.fetch_channel_feed("UC_quiet") == [] + + def test_entries_are_parsed(self, monkeypatch): + feed = b""" + + abc123Hello + 2026-07-23T10:00:00+00:00 + """ + self._patch(monkeypatch, status=200, body=feed) + out = rss.fetch_channel_feed("UC_live") + assert [e["id"] for e in out] == ["abc123"] + assert out[0]["channel_id"] == "UC_live" diff --git a/frontend/src/components/Feed.tsx b/frontend/src/components/Feed.tsx index 0da24e0..ad1a3b5 100644 --- a/frontend/src/components/Feed.tsx +++ b/frontend/src/components/Feed.tsx @@ -137,6 +137,11 @@ export default function Feed({ (video: Video, startAt: number | null = null) => setActiveVideo({ video, startAt }), [], ); + // Stable identity, like Playlists' `closePlayer`: PlayerModal's keydown/scroll-lock effect + // depends on `onClose`, so a fresh closure every Feed render re-subscribed those listeners, + // re-ran focusModal() under the open player, dropped its in-flight timers and flushed the + // debounced volume write on each pass. + const closePlayer = useCallback(() => setActiveVideo(null), []); const ytActive = !!ytSearch; // How many live-search results to gather (the count selector replaces a manual "load more"; @@ -484,7 +489,7 @@ export default function Feed({ video={activeVideo.video} startAt={activeVideo.startAt} queue={ytPlayerQueue} - onClose={() => setActiveVideo(null)} + onClose={closePlayer} onState={onState} /> @@ -739,7 +744,7 @@ export default function Feed({ video={activeVideo.video} startAt={activeVideo.startAt} queue={playerQueue} - onClose={() => setActiveVideo(null)} + onClose={closePlayer} onState={onState} /> diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 1fc3d1d..a720f13 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -11,6 +11,9 @@ import { ChevronLeft, ChevronRight, ExternalLink, + Maximize, + Minimize, + Minimize2, Repeat, Repeat1, Settings, @@ -34,7 +37,15 @@ import { relativeTime, } from "../lib/format"; import { renderDescription } from "../lib/descriptionLinks"; -import { useBackToClose } from "../lib/history"; +import { + exitFullscreen, + fullscreenElement, + onFullscreenChange, + requestFullscreen, +} from "../lib/fullscreen"; +import { useBackToClose, useBackToExit } from "../lib/history"; +import { DEFAULT_VOLUME, normalizeVolume, stepVolume } from "../lib/playerVolume"; +import { LS, readAccountMerged, writeAccount } from "../lib/storage"; import { useScrollFade } from "../lib/useScrollFade"; // Experiment (branch experiment/inline-player): play the video in-app via the @@ -54,6 +65,21 @@ type LoopMode = "off" | "one" | "all"; const AUTO_MODES: AutoMode[] = ["off", "next", "prev", "random"]; const LOOP_MODES: LoopMode[] = ["off", "one", "all"]; +/** Can `el` (or an ancestor inside the modal) still scroll vertically in `dir` (-1 up, 1 down)? + * Used to leave ArrowUp/ArrowDown alone while there is content to scroll — the volume shortcut + * must not eat the only keyboard way to read a long description. Stops at ``: the page + * behind the modal is `overflow:hidden` anyway. */ +function canScrollBy(el: HTMLElement | null, dir: -1 | 1): boolean { + for (let node = el; node && node !== document.body; node = node.parentElement) { + const style = getComputedStyle(node); + if (!/(auto|scroll|overlay)/.test(style.overflowY)) continue; + const room = + dir < 0 ? node.scrollTop > 0 : node.scrollTop + node.clientHeight < node.scrollHeight - 1; // -1: sub-pixel heights + if (room) return true; + } + return false; +} + // --- IFrame Player API loader (singleton) --- let apiPromise: Promise | null = null; function loadYouTubeApi(): Promise { @@ -155,6 +181,42 @@ export default function PlayerModal({ // navigable (previously the overlay ate clicks below the top items). Moving the pointer off the // video, or the window regaining focus, re-arms our click/scroll/keyboard shortcuts. const [nativeControls, setNativeControls] = useState(false); + // True while OUR stage element is the fullscreen element. In fullscreen the interaction overlay + // yields entirely (see the overlay's pointer-events below): YouTube's control bar only appears + // while the iframe itself sees mouse movement, so an armed overlay kept the bar — and with it + // the native fullscreen button — permanently hidden after pressing F. + const [isFullscreen, setIsFullscreen] = useState(false); + // Our own "fills the viewport" mode (the F key). Not the Fullscreen API — see toggleFullscreen. + const [maximized, setMaximized] = useState(false); + const maximizedRef = useRef(false); + maximizedRef.current = maximized; + // Mirrored into a ref because the keydown handler is bound once (deps: [onClose]) and would + // otherwise read `false` forever — the house pattern here (see navRef/volumeRef). + const isFullscreenRef = useRef(false); + // Nested Back: peel fullscreen → maximise → close, one browser/mouse Back at a time (each is a + // history layer only while active). Mirrors the Esc ladder in the keydown handler below. + useBackToExit(isFullscreen, exitFullscreen); + useBackToExit(maximized, () => setMaximized(false)); + // Volume survives the modal, the next video and a reload: a YT.Player is created per video and + // always starts at full volume. Per-account, like the Plex player's prefs. Level 0 IS the muted + // state (that's how YouTube's own control behaves), so there's no separate `muted` flag to + // contradict it. + // + // Ref + explicit read/write rather than `useAccountPersistedObject`: nothing RENDERS the level + // (the on-player flash has its own `volumeUi` state), the once-bound wheel/key listeners need a + // ref anyway, and the hook's write lives inside a `setState` updater — which is both a second + // write on every settle and not something React must run while the component is unmounting. + // Sanitised HERE, at the boundary where the untrusted value enters (`normalizeVolume`, see + // lib/playerVolume): a stored `null` / `"80"` / missing key becomes the default once, instead of + // travelling into the arithmetic. Read via a lazy `useState` initializer so it happens once, not + // on every render (the house pattern — see the frozen queue above). + const [storedVolume] = useState(() => + normalizeVolume(readAccountMerged(LS.ytPlayerPrefs, { volume: DEFAULT_VOLUME }).volume), + ); + const volumeRef = useRef(storedVolume); + const volSaveTimerRef = useRef(undefined); + // When the last volume gesture happened — see nudgeVolume (a spin must not re-read the player). + const lastNudgeAtRef = useRef(0); const focusModal = () => cardRef.current?.focus(); const togglePlay = () => { @@ -164,26 +226,83 @@ export default function PlayerModal({ p.pauseVideo?.(); // 1 === playing else p.playVideo?.(); }; - const toggleFullscreen = () => { - const el = stageRef.current; - if (!el) return; - if (document.fullscreenElement) document.exitFullscreen?.(); - else el.requestFullscreen?.(); + /** F = maximise, deliberately WITHOUT the Fullscreen API. + * + * The measured reason (Chrome/Brave/Firefox alike, so it is spec behaviour, not a browser bug): + * a cross-origin embed reads its fullscreen state from ITS OWN document. While WE hold the + * browser's fullscreen lock — on our wrapper or even on the iframe element — the YouTube player + * never learns it is fullscreen, so its own fullscreen button stays in the "enter" state and a + * click on it changes nothing visible. That is the reported bug: every other YouTube control + * (gear, CC, volume) works, only its fullscreen button looks dead. + * + * So we don't take the lock at all: `maximized` is a CSS layer that fills the viewport. The page + * stays non-fullscreen, which means the embed's own fullscreen button works exactly as it does + * anywhere else — and OUR shortcuts and overlays (wheel volume, the level flash, the queue bar) + * keep working, because our DOM is what's on screen. The one thing we give up is hiding the + * browser's own chrome; YouTube's button provides that, correctly, when the user wants it. + */ + const toggleFullscreen = () => setMaximized((m) => !m); + // True browser fullscreen that WE own (the stage element). Unlike YouTube's own fullscreen it keeps + // OUR overlay + control bar on screen, so wheel/arrow volume and our controls work inside it too. + // We never surface YouTube's own fullscreen button, so the "a cross-origin embed's own button looks + // dead while we hold the lock" problem that made F avoid the Fullscreen API does not arise. + const toggleTrueFullscreen = () => { + if (fullscreenElement()) exitFullscreen(); + else if (stageRef.current) requestFullscreen(stageRef.current); + }; + // Leave the big view back to the modal card: drop fullscreen first if held, then un-maximise. + const exitBigView = () => { + if (fullscreenElement()) exitFullscreen(); + setMaximized(false); }; const flashVolume = (level: number) => { setVolumeUi(level); window.clearTimeout(volTimerRef.current); volTimerRef.current = window.setTimeout(() => setVolumeUi(null), 1200); }; - const nudgeVolume = (delta: number) => { + /** Write the current level out now, cancelling any pending debounce. */ + const persistVolume = () => { + window.clearTimeout(volSaveTimerRef.current); + volSaveTimerRef.current = undefined; + writeAccount(LS.ytPlayerPrefs, { volume: volumeRef.current }); + }; + /** Push a level (0–100) to the player and remember it; 0 also mutes (a player left at zero + * volume but unmuted shows YouTube's speaker icon as ON, which reads as broken). */ + const applyVolume = (level: number, flash = true) => { + // Callers hand over an already-normalised level (stepVolume / the stored value), but the + // player can still answer NaN, so re-normalise here against the LAST GOOD level — never the + // default: jumping a quiet session to full blast is the surprise this feature exists to avoid. + const next = normalizeVolume(level, volumeRef.current); const p = playerRef.current; - if (!p || typeof p.getVolume !== "function") return; - const cur = p.isMuted?.() ? 0 : (p.getVolume?.() ?? 50); - const next = Math.max(0, Math.min(100, Math.round(cur + delta))); - if (next > 0) p.unMute?.(); - p.setVolume?.(next); - if (next === 0) p.mute?.(); - flashVolume(next); + if (p && typeof p.setVolume === "function") { + if (next > 0) p.unMute?.(); + p.setVolume(next); + if (next === 0) p.mute?.(); + } + volumeRef.current = next; + // Debounced: one write when the spin settles, not one per notch. The unmount cleanup flushes + // a pending write, so the last change is never lost. + window.clearTimeout(volSaveTimerRef.current); + volSaveTimerRef.current = window.setTimeout(persistVolume, 400); + if (flash) flashVolume(next); + }; + const nudgeVolume = (delta: number) => { + // No early return when the player isn't ready yet: the level lives in `volumeRef` and onReady + // applies it, so a wheel spin in the first second after opening (the natural reaction to a + // video that starts loud) now lands instead of being silently dropped. + const p = playerRef.current; + // Whose level do we step from? Our own ref, EXCEPT when the gesture starts fresh — then read + // the player, because the user may have moved YouTube's own slider in the meantime. + // `getVolume()` is answered across the iframe boundary and lags our `setVolume` by a beat, so + // re-reading it mid-spin resurrects a stale level and silently drops notches (measured: five + // wheel notches moved 100 → 90 instead of 75). + const now = Date.now(); + const continuing = now - lastNudgeAtRef.current < 1000; + lastNudgeAtRef.current = now; + const live = + p && typeof p.getVolume === "function" ? (p.isMuted?.() ? 0 : p.getVolume()) : null; + const cur = continuing || live === null ? volumeRef.current : live; + applyVolume(stepVolume(cur, { delta, fallback: volumeRef.current })); }; // The player can navigate to other videos (YouTube links in a description). The @@ -375,7 +494,13 @@ export default function PlayerModal({ const onKey = (e: KeyboardEvent) => { if (e.key === "Escape") { // In fullscreen, let the browser's Esc exit fullscreen only — don't also close the modal. - if (document.fullscreenElement) return; + if (fullscreenElement()) return; + // Same for our own maximise: Esc steps back out of it before it closes the player. + if (maximizedRef.current) { + e.preventDefault(); + setMaximized(false); + return; + } // Defer to any Modal opened above the player (e.g. the download dialog): its Escape closes // it, ours would otherwise also fire and close the player underneath it (U-C). if (modalCount() > 0) return; @@ -396,6 +521,21 @@ export default function PlayerModal({ if (typing || tag === "button" || tag === "a") return; e.preventDefault(); togglePlay(); + } else if (e.key === "ArrowUp" || e.key === "ArrowDown") { + // Volume by keyboard (YouTube/Plex-style ±5). The only volume control that survives + // fullscreen, where the wheel goes to YouTube's own iframe. But the modal card scrolls + // (a long description) and Up/Down is how you scroll it from the keyboard — so defer + // whenever the focused element can still scroll that way, exactly like Space defers to a + // focused button. NOT while the video fills the screen though — in fullscreen OR in our own + // maximise — because the card is behind/off-screen there: deferring would scroll something + // invisible and leave the viewer with no volume control at all. Both flags, not just + // fullscreen: since F stopped taking the browser's lock, maximise is the common case and + // checking only `isFullscreen` made the keys dead exactly where they matter most. + const up = e.key === "ArrowUp"; + const covered = isFullscreenRef.current || maximizedRef.current; + if (typing || (!covered && canScrollBy(el, up ? -1 : 1))) return; + e.preventDefault(); + nudgeVolume(up ? 5 : -5); } else if (e.key === "ArrowLeft" || e.key === "ArrowRight") { // Plain arrows seek within the video (YouTube-style ±5s); Shift+arrow steps to the // previous/next video in the queue (the feed's order or a playlist). @@ -420,6 +560,9 @@ export default function PlayerModal({ window.clearTimeout(closeTimer.current); window.clearTimeout(openTimer.current); window.clearTimeout(volTimerRef.current); + // Flush a debounced volume write — closing the player right after a wheel spin (the normal + // way to leave) must not drop the level the user just set. + if (volSaveTimerRef.current != null) persistVolume(); }; }, [onClose]); @@ -431,6 +574,9 @@ export default function PlayerModal({ const el = dialogRef.current; if (!el) return; const onWheel = (e: WheelEvent) => { + // Ctrl/⌘+wheel is the browser's zoom gesture — never swallow it. Volume is the PLAIN wheel + // (and ↑/↓); taking the modifier too left zoom unreachable while the player was open. + if (e.ctrlKey || e.metaKey) return; e.preventDefault(); focusModal(); // re-grab focus if it had slipped into the native controls nudgeVolume(e.deltaY < 0 ? 5 : -5); @@ -439,6 +585,27 @@ export default function PlayerModal({ return () => el.removeEventListener("wheel", onWheel); }, []); + // Track the browser's fullscreen state. WE never request it any more (F maximises via CSS), so + // this is YouTube's own fullscreen — entered from its control bar, left by its button or Esc. + useEffect(() => { + const onChange = () => { + const fsEl = fullscreenElement(); + // Refs only — this listener is bound once, so it must not close over render values. + const p = playerRef.current; + const frame = p && typeof p.getIframe === "function" ? p.getIframe() : null; + const ours = !!fsEl && (fsEl === stageRef.current || fsEl === frame); + isFullscreenRef.current = ours; // read by the once-bound keydown handler + setIsFullscreen(ours); // drives the overlay's pointer-events + // LEAVING fullscreen is where the user got stranded: to reach YouTube's fullscreen button + // they had to click INTO the player, which moves keyboard focus into the cross-origin iframe + // — and it stays there afterwards. Our window-level shortcuts then never fire (F went to + // YouTube instead, giving a second, control-less fullscreen), so the maximised view had no + // way out. Taking focus back here restores F/Space/arrows the moment fullscreen ends. + if (!fsEl) focusModal(); + }; + return onFullscreenChange(onChange); // both spellings — see lib/fullscreen + }, []); + // Yield the interaction overlay to YouTube's native UI. Clicking a native control (gear / seek // bar / CC) moves focus into the player iframe — the only cross-origin signal we get. While the // iframe holds focus we drop the overlay's pointer-events so the (arbitrarily tall) settings @@ -532,8 +699,12 @@ export default function PlayerModal({ vq: "hd1080", }, events: { - // Keep keyboard focus on the modal, not the iframe, so shortcuts work right away. - onReady: () => focusModal(), + // Keep keyboard focus on the modal, not the iframe, so shortcuts work right away, and + // restore the remembered volume — a fresh player always starts at 100. + onReady: () => { + focusModal(); + applyVolume(volumeRef.current, false); + }, onStateChange: (e: any) => { // 1 === playing → sync the navigated-to video's title/author for display. if (e?.data === 1) { @@ -653,19 +824,34 @@ export default function PlayerModal({ >
{/* Hide the iframe entirely on error so YouTube's own error screen can't bleed through our overlay. */} + {/* In the big views the video area is inset by the control bar's height (`bottom-14`) so + the bar below never overlaps it. The inset lives on THIS wrapper, not on `mountRef`: + YT.Player REPLACES the mount element with its iframe (width/height 100% of the parent), + which would drop any class on mountRef itself — the iframe then sizes to this wrapper. */}
- {/* Interaction layer over the CENTRE of the video: catches click (play/pause) and stops - the iframe stealing keyboard focus. It deliberately leaves the top AND bottom edges - uncovered so YouTube's native controls — the top-right cluster (volume / CC / settings) - and the bottom bar (seek / More videos / fullscreen) — stay clickable. Hidden on error - so the "Open on YouTube" CTA is clickable. */} + className={ + maximized || isFullscreen ? "absolute inset-x-0 top-0 bottom-14" : "w-full h-full" + } + > +
+
+ {/* Interaction layer over the CENTRE of the video: catches click (play/pause) + wheel + volume (the wheel event bubbles from here to the modal's listener — over the bare + cross-origin iframe it would be swallowed) and stops the iframe stealing keyboard focus. + It leaves the top AND bottom edges uncovered so YouTube's edge controls stay reachable. + Armed in the big views too (that is what keeps wheel volume working after F, the reported + gap) — it only yields to `nativeControls`, i.e. once the user opens YouTube's own + settings menu (focus moves into the iframe), so that menu is navigable end to end. + Hidden on error so the "Open on YouTube" CTA is clickable. */} {playerError == null && (
{ @@ -679,6 +865,143 @@ export default function PlayerModal({ }`} /> )} + {/* Our own control bar for the big views (maximise / our fullscreen), where the card's + controls are off-screen. It's IN the stage (so it survives true fullscreen, where a + portal would not) and DOCKED in its own strip: the video area above is inset by + this bar's height (see mountRef's `bottom-14`), so the bar NEVER overlaps YouTube's seek + bar — that stays reachable by hovering the video's bottom edge, just above ours. Always + visible (no auto-hide) so there's no guessing whether it will appear and no fight with the + seek bar over the same pixels. stopPropagation so a click here doesn't toggle playback; + the exit button also re-arms our shortcuts. Add-to-playlist / download are hidden in true + fullscreen — their popovers portal to , outside the fullscreen element. */} + {(maximized || isFullscreen) && ( +
e.stopPropagation()} + className="absolute inset-x-0 bottom-0 z-menu flex h-14 items-center justify-start gap-x-3 overflow-x-auto border-t border-white/10 bg-neutral-950/95 px-3 text-sm text-white" + > + {hasQueue && ( + <> + + + {index + 1} / {queue!.length} + + + + + + + + )} + {!navigated && !isFullscreen && ( + + )} + {!navigated && !isFullscreen && ( + + )} + {!navigated && ( + + )} + + + +
+ )} {/* Discreet badge while YouTube's own controls have taken over (overlay yielded). */} {playerError == null && nativeControls && (
diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index f445d0d..8a373ee 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -35,6 +35,12 @@ import { } from "lucide-react"; import { api, type PlexMarker } from "../lib/api"; import { formatDuration } from "../lib/format"; +import { + exitFullscreen, + fullscreenElement, + onFullscreenChange, + requestFullscreen, +} from "../lib/fullscreen"; import { LS, useAccountPersistedObject } from "../lib/storage"; import { useDismiss } from "../lib/useDismiss"; import { useScrollFade } from "../lib/useScrollFade"; @@ -579,17 +585,17 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { [nudgeVolume], ); + // Via lib/fullscreen, not the raw API: WebKit (Safari/iPadOS) ships only the prefixed spelling, + // so reading `document.fullscreenElement` alone made this player believe it is never fullscreen + // — the button did nothing, `fs` never flipped, and Escape (below) read "not fullscreen" as + // permission to close the whole player instead of just leaving fullscreen. const toggleFs = useCallback(() => { const el = wrapRef.current; if (!el) return; - if (document.fullscreenElement) document.exitFullscreen().catch(() => {}); - else el.requestFullscreen().catch(() => {}); - }, []); - useEffect(() => { - const onFs = () => setFs(!!document.fullscreenElement); - document.addEventListener("fullscreenchange", onFs); - return () => document.removeEventListener("fullscreenchange", onFs); + if (fullscreenElement()) exitFullscreen(); + else requestFullscreen(el); }, []); + useEffect(() => onFullscreenChange(() => setFs(!!fullscreenElement())), []); const go = useCallback((otherId: string | null | undefined) => { if (otherId) { @@ -876,7 +882,7 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { setTracksOpen(false); } else if (helpOpenRef.current) setHelpOpen(false); else if (infoOpenRef.current) setInfoOpen(false); - else if (!document.fullscreenElement) onClose(); + else if (!fullscreenElement()) onClose(); break; } }; diff --git a/frontend/src/i18n/locales/en/player.json b/frontend/src/i18n/locales/en/player.json index 4f19dd2..9321044 100644 --- a/frontend/src/i18n/locales/en/player.json +++ b/frontend/src/i18n/locales/en/player.json @@ -35,6 +35,9 @@ "unavailableBody": "This video isn't available — it may be private, removed, or region-restricted.", "embedDisabledBody": "The owner disabled playback on other sites (common for auto-generated “Topic” music tracks). It still plays on YouTube.", "openOnYoutube": "Open on YouTube", - "shortcutsHint": "Click: play/pause · Scroll: volume · F: fullscreen", - "nativeControls": "YouTube controls active" + "shortcutsHint": "Click: play/pause · Scroll or ↑ ↓: volume · F: fill the window", + "exitMaximize": "Exit full window (Esc)", + "nativeControls": "YouTube controls active", + "fullscreen": "Fullscreen", + "exitFullscreen": "Exit fullscreen" } diff --git a/frontend/src/i18n/locales/hu/player.json b/frontend/src/i18n/locales/hu/player.json index 398fcbc..1c36416 100644 --- a/frontend/src/i18n/locales/hu/player.json +++ b/frontend/src/i18n/locales/hu/player.json @@ -35,6 +35,9 @@ "unavailableBody": "Ez a videó nem elérhető — lehet privát, eltávolított vagy régiókorlátozott.", "embedDisabledBody": "A tulajdonos letiltotta a lejátszást más oldalakon (gyakori az auto-generált „Topic” zenei sávoknál). A YouTube-on viszont lejátszható.", "openOnYoutube": "Megnyitás YouTube-on", - "shortcutsHint": "Kattintás: lejátszás/szünet · Görgő: hangerő · F: teljes képernyő", - "nativeControls": "YouTube vezérlők aktívak" + "shortcutsHint": "Kattintás: lejátszás/szünet · Görgő vagy ↑ ↓: hangerő · F: teljes ablak", + "exitMaximize": "Kilépés a teljes ablakból (Esc)", + "nativeControls": "YouTube vezérlők aktívak", + "fullscreen": "Teljes képernyő", + "exitFullscreen": "Kilépés a teljes képernyőből" } diff --git a/frontend/src/index.css b/frontend/src/index.css index 75f3062..f274e8d 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -579,13 +579,39 @@ html[data-scheme="matrix"][data-theme="light"] { } } -/* The in-app player stage, when fullscreened via the F key / native button, fills the screen - instead of keeping its 16:9 letterbox (and drops the rounded top corners). */ +/* The in-app player stage filling the screen: either because something fullscreened it (YouTube's + own button hands the lock to the iframe, not to us — but keep this for any browser that does), + or via our own F-key maximise. `--max` deliberately does NOT use the Fullscreen API: while we + hold the browser's lock, the cross-origin YouTube embed never learns it is fullscreen and its own + fullscreen button stops working (see PlayerModal.toggleFullscreen). */ +.player-stage:fullscreen, +.player-stage--max { + aspect-ratio: auto; + border-radius: 0; +} + +/* A fullscreened element is already sized by the browser; the viewport units are only for the + `:fullscreen` path, where the stage is not positioned. */ .player-stage:fullscreen { width: 100vw; height: 100vh; - aspect-ratio: auto; - border-radius: 0; +} + +/* Sizing, precisely: the element keeps Tailwind's `w-full` from its base className, so the WIDTH is + `100%` of the containing block — which, for a fixed element, is the viewport minus any classic + scrollbar. The HEIGHT comes from `inset: 0` (top+bottom, once `aspect-ratio` is released above). + `right: 0` is therefore ignored (over-constrained: left + width win) — harmless, but don't read + this rule as inset-sized. What had to go are the viewport units it used to inherit: `100vw` + INCLUDES the scrollbar (horizontal overflow) and `100vh` includes a mobile browser's retracting + chrome, which pushes the bottom edge — YouTube's own control bar — off screen. */ +.player-stage--max { + position: fixed; + inset: 0; + /* rail-level (40): this stage only has to beat the modal chrome it is nested inside (whose own + children top out at z-menu/20). The modal root is a z-overlay stacking context, so nothing + here escapes it — 9999 is the glass tuner's reserved level and claimed an authority this + layer neither has nor needs. */ + z-index: 40; } /* Thin, theme-aware scrollbars */ diff --git a/frontend/src/lib/fullscreen.ts b/frontend/src/lib/fullscreen.ts new file mode 100644 index 0000000..9dae2cf --- /dev/null +++ b/frontend/src/lib/fullscreen.ts @@ -0,0 +1,49 @@ +// The browser's Fullscreen API, WebKit included. +// +// Safari and iPadOS still ship only the prefixed spelling: no `fullscreenchange`, no +// `fullscreenElement`, no `requestFullscreen` on a plain element. Reading only the unprefixed names +// there means the page believes it is NEVER fullscreen — which silently breaks whatever the +// fullscreen state drives (an overlay that must yield, a focus reclaim, an Esc branch that would +// otherwise close the whole player instead of just leaving fullscreen). +// +// Both video surfaces (PlayerModal's YouTube embed, PlexPlayer's