From 2f46947dcfc1c7cd60e51160f765dbbdfd1c3e83 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 23 Jul 2026 22:32:25 +0200 Subject: [PATCH 01/19] =?UTF-8?q?fix(downloads):=20R5=20S1=20=E2=80=94=20d?= =?UTF-8?q?ownloads=20robustness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pick the produced file safely: Path("") is Path("."), which passes .exists(), so a missing yt-dlp filepath skipped the staging fallback and crashed in with_name ("PosixPath('.') has an empty name") on non-YouTube sources - unwrap a playlist/multi_video extraction to the entry that carries the media - run_ffmpeg: drain stderr while it runs (a full 64KB pipe blocked ffmpeg forever and hung the worker thread), 1s cancel polling, stall watchdog and a bounded wait on exit - escape apostrophes/backslashes in the ffmpeg concat list (pre-0.51.0 names) - sweep orphaned HLS segment dirs at startup (nothing else ever reaped them) --- backend/app/downloads/edit.py | 124 ++++++++++++++++++++++---- backend/app/main.py | 6 ++ backend/app/plex/stream.py | 21 +++++ backend/app/worker.py | 52 ++++++++--- backend/tests/test_download_output.py | 76 ++++++++++++++++ 5 files changed, 249 insertions(+), 30 deletions(-) create mode 100644 backend/tests/test_download_output.py diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 0d4944d..c57342c 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,16 @@ 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. + + The demuxer's own rule: inside single quotes a literal `'` is written `'\\''` (close, escaped + quote, reopen) and a `\\` must be doubled. 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 there makes the whole list unparseable.""" + return path.as_posix().replace("\\", "\\\\").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 +224,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 +251,107 @@ 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.""" + 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() + _stop(proc) 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: + _stop(proc) + 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 + _stop(proc) + raise RuntimeError("ffmpeg did not exit after closing its output") 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 -------------------------------------------------- diff --git a/backend/app/main.py b/backend/app/main.py index 41f8a19..45643c4 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -27,6 +27,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,6 +68,11 @@ 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. + swept = plex_stream.sweep_orphans() + if swept: + log.info("swept %d orphaned HLS segment directories", swept) start_scheduler() try: yield diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 5d1f5c2..4401db1 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -254,6 +254,27 @@ def wait_for(path: Path, timeout: float = 20.0) -> bool: return path.exists() +def sweep_orphans() -> int: + """Delete segment directories no live session owns. + + Sessions live only in THIS process, so at startup every 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.""" + with _lock: + live = {s.dir for s in _sessions.values()} + 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 p.is_dir() and p not in live: + shutil.rmtree(p, ignore_errors=True) + dropped += 1 + return dropped + + def reap_idle() -> int: now = time.time() dropped = 0 diff --git a/backend/app/worker.py b/backend/app/worker.py index 3774c27..2437d72 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -341,13 +341,51 @@ def _staging_dir(asset_id: int) -> Path: return Path(settings.download_root) / ".staging" / f"asset-{asset_id}" +_THUMB_SUFFIXES = (".jpg", ".jpeg", ".png", ".webp") + + 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 non-thumbnail file left in staging.""" + 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 p.name.endswith(".part") + ] + 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 the first entry.""" + entries = [e for e in (info.get("entries") or []) if e] + if entries and not info.get("requested_downloads"): + return {**info, **entries[0]} + return info + + # Errors worth retrying with a different player-client set (YouTube per-client flakiness). _RETRIABLE_MARKERS = ( "not a bot", @@ -447,17 +485,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( diff --git a/backend/tests/test_download_output.py b/backend/tests/test_download_output.py new file mode 100644 index 0000000..c71ad80 --- /dev/null +++ b/backend/tests/test_download_output.py @@ -0,0 +1,76 @@ +"""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 +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" + + 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_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 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_is_doubled(self): + 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" From 4209f6bb245a4cbe883853010a89c1c81d529029 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 23 Jul 2026 22:36:33 +0200 Subject: [PATCH 02/19] =?UTF-8?q?fix(sync):=20R5=20S2=20=E2=80=94=20sync?= =?UTF-8?q?=20guards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - never prune subscriptions on an empty fetch: one anomalous empty-but-200 response wiped every subscription (and its deep_requested flag) in one run - push_playlist: stop after 5 consecutive write failures — a quota-dead or scope-revoked account otherwise burns 50 units per remaining item - RSS: raise RssError instead of returning [] on a transport/HTTP failure, so a failed poll no longer stamps last_rss_at and a 404 feed stops looking healthy - worker: exit when the schema never appears instead of "continuing" into a crash one call later --- backend/app/sync/playlists.py | 57 +++++++++++++++++++++++++++++++ backend/app/sync/runner.py | 18 ++++++++-- backend/app/sync/subscriptions.py | 23 ++++++++++--- backend/app/worker.py | 26 +++++++++----- backend/app/youtube/rss.py | 16 +++++++-- backend/tests/test_sync_guards.py | 28 +++++++++++++++ 6 files changed, 150 insertions(+), 18 deletions(-) create mode 100644 backend/tests/test_sync_guards.py diff --git a/backend/app/sync/playlists.py b/backend/app/sync/playlists.py index 59dbdc0..82926c7 100644 --- a/backend/app/sync/playlists.py +++ b/backend/app/sync/playlists.py @@ -263,6 +263,40 @@ 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 _Breaker: + """Consecutive-failure circuit breaker for a single push run. Any success re-arms it.""" + + 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 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 +305,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 +313,16 @@ 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: + if not breaker.open(): + failures.append(vid) + continue try: yt.add_playlist_item(pl.yt_playlist_id, vid) inserted += 1 + breaker.hit(True) except YouTubeError: failures.append(vid) + breaker.hit(False) if not failures: pl.synced_fingerprint = fingerprint(pl.name, desired) pl.dirty = False @@ -293,6 +333,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 +354,30 @@ 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: + if not breaker.open(): + failures.append(it["video_id"]) + continue try: yt.delete_playlist_item(it["item_id"]) deleted += 1 + breaker.hit(True) except YouTubeError: failures.append(it["video_id"]) + breaker.hit(False) # Insert missing (append; the reorder pass below fixes positions). cur_set = set(cur_vids) for vid in desired: if vid not in cur_set: + if not breaker.open(): + failures.append(vid) + continue try: item_id_by_vid[vid] = yt.add_playlist_item(pl.yt_playlist_id, vid) inserted += 1 + breaker.hit(True) except YouTubeError: failures.append(vid) + breaker.hit(False) # 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,11 +387,16 @@ 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 + if not breaker.open(): + failures.append(want) + continue try: yt.move_playlist_item(item_id_by_vid[want], pl.yt_playlist_id, want, i) reordered += 1 + breaker.hit(True) except YouTubeError: failures.append(want) + breaker.hit(False) continue j = model.index(want) model.pop(j) @@ -355,6 +411,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..d2de856 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 @@ -54,6 +54,7 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> int: total = len(channels) new = 0 done = 0 + failed = 0 # 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,9 +66,20 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> int: channel = futures[fut] try: entries = fut.result() + except RssError as exc: + # A failed poll is NOT an empty feed: skip the apply so `last_rss_at` stays where + # it was. Stamping it here made a permanently-broken feed look freshly polled. + log.warning("RSS poll skipped for channel %s: %s", channel.id, exc) + failed += 1 + done += 1 + progress.report(done, total, "rss_poll") + continue except Exception: log.exception("RSS fetch failed for channel %s", channel.id) - entries = [] + failed += 1 + done += 1 + progress.report(done, total, "rss_poll") + continue try: new += apply_rss_feed(db, channel, entries) except Exception: @@ -75,6 +87,8 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> int: log.exception("RSS apply failed for channel %s", channel.id) done += 1 progress.report(done, total, "rss_poll") + if failed: + log.warning("RSS poll: %d/%d channel feeds could not be read", failed, total) return new diff --git a/backend/app/sync/subscriptions.py b/backend/app/sync/subscriptions.py index e8b98cc..1d95ec7 100644 --- a/backend/app/sync/subscriptions.py +++ b/backend/app/sync/subscriptions.py @@ -108,17 +108,29 @@ 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 NEVER on an empty fetch. 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 one + # run. Someone who genuinely unsubscribed from everything simply keeps their rows until a + # sync that returns something. 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 = bool(existing) and not fetched + 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 +156,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/worker.py b/backend/app/worker.py index 2437d72..00c1787 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -93,24 +93,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: @@ -719,8 +728,9 @@ 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(): + # The API may still be applying migrations when we boot. If they never arrive, exit instead of + # pressing on into a guaranteed crash — the container's restart policy retries the whole wait. + if not _wait_for_schema(): return _recover_orphans() n = max(1, settings.download_worker_concurrency) 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_sync_guards.py b/backend/tests/test_sync_guards.py new file mode 100644 index 0000000..c887b5d --- /dev/null +++ b/backend/tests/test_sync_guards.py @@ -0,0 +1,28 @@ +"""Guards that stop one bad YouTube answer from cascading: the playlist-push circuit breaker.""" +from app.sync.playlists import _Breaker + + +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() From 63a36db1165801a43c4c51aa6f01278fe35f6879 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 23 Jul 2026 22:56:42 +0200 Subject: [PATCH 03/19] fix(downloads): probe codecs from ffprobe JSON, not its flat output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ffprobe prints codec_name BEFORE codec_type, so the line parser attributed every name to the previous stream: a plain H.264+AAC mp4 probed as (aac, None) and the browser-playability check re-encoded a perfectly good file on EVERY download. Verified on localdev — the same source now stores without a re-encode pass. --- backend/app/downloads/edit.py | 40 +++++++++++++++++---------- backend/tests/test_download_output.py | 31 ++++++++++++++++++++- 2 files changed, 56 insertions(+), 15 deletions(-) diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index c57342c..395721d 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -364,29 +364,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/tests/test_download_output.py b/backend/tests/test_download_output.py index c71ad80..d48bcad 100644 --- a/backend/tests/test_download_output.py +++ b/backend/tests/test_download_output.py @@ -5,7 +5,7 @@ so a missing filepath (an arbitrary-URL extractor) skipped the staging fallback with "PosixPath('.') has an empty name" instead of downloading.""" import pytest -from app.downloads.edit import _concat_quote +from app.downloads.edit import _concat_quote, parse_probe_streams from app.worker import _media_info, _produced_file from pathlib import Path, PurePosixPath @@ -62,6 +62,35 @@ class TestMediaInfo: 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). From 5130584d0a28272942f8d6f446b0e62d93663de2 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 23 Jul 2026 22:56:42 +0200 Subject: [PATCH 04/19] =?UTF-8?q?fix(player):=20R5=20S3=20=E2=80=94=20full?= =?UTF-8?q?screen=20controls,=20volume=20memory,=20Ctrl+wheel=20zoom?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - the interaction overlay yields fully in fullscreen: YouTube only reveals its control bar while the iframe itself sees the mouse, so after pressing F the native fullscreen button was unreachable - remember the volume per account (a YT.Player is created per video and always starts at 100), plus ArrowUp/ArrowDown volume keys that work in fullscreen - Ctrl/Cmd+wheel falls through to the browser's zoom instead of the volume --- frontend/src/components/PlayerModal.tsx | 71 ++++++++++++++++++++---- frontend/src/i18n/locales/en/player.json | 2 +- frontend/src/i18n/locales/hu/player.json | 2 +- frontend/src/lib/storage.ts | 3 + 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 1fc3d1d..96930c1 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -35,6 +35,7 @@ import { } from "../lib/format"; import { renderDescription } from "../lib/descriptionLinks"; import { useBackToClose } from "../lib/history"; +import { LS, useAccountPersistedObject } from "../lib/storage"; import { useScrollFade } from "../lib/useScrollFade"; // Experiment (branch experiment/inline-player): play the video in-app via the @@ -155,6 +156,21 @@ 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); + // 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. + const [playerPrefs, patchPlayerPrefs] = useAccountPersistedObject(LS.ytPlayerPrefs, { + volume: 100, + }); + // The window/wheel listeners below are bound once, so they must read the LIVE prefs. + const prefsRef = useRef(playerPrefs); + prefsRef.current = playerPrefs; const focusModal = () => cardRef.current?.focus(); const togglePlay = () => { @@ -175,15 +191,26 @@ export default function PlayerModal({ window.clearTimeout(volTimerRef.current); volTimerRef.current = window.setTimeout(() => setVolumeUi(null), 1200); }; + /** 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) => { + const next = Math.max(0, Math.min(100, Math.round(level))); + const p = playerRef.current; + if (p && typeof p.setVolume === "function") { + if (next > 0) p.unMute?.(); + p.setVolume(next); + if (next === 0) p.mute?.(); + } + patchPlayerPrefs({ volume: next }); + if (flash) flashVolume(next); + }; const nudgeVolume = (delta: number) => { 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); + // Read the live player (the user may have used YouTube's own slider), falling back to the + // stored level when the player can't answer yet. + const cur = p.isMuted?.() ? 0 : (p.getVolume?.() ?? prefsRef.current.volume); + applyVolume(cur + delta); }; // The player can navigate to other videos (YouTube links in a description). The @@ -396,6 +423,12 @@ 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. + if (typing) return; + e.preventDefault(); + nudgeVolume(e.key === "ArrowUp" ? 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). @@ -431,6 +464,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 +475,16 @@ export default function PlayerModal({ return () => el.removeEventListener("wheel", onWheel); }, []); + // Track whether we're the fullscreen element (F, our button, or the browser's own Esc exit). + useEffect(() => { + const onChange = () => + setIsFullscreen( + !!document.fullscreenElement && document.fullscreenElement === stageRef.current, + ); + document.addEventListener("fullscreenchange", onChange); + return () => document.removeEventListener("fullscreenchange", onChange); + }, []); + // 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 +578,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(prefsRef.current.volume, false); + }, onStateChange: (e: any) => { // 1 === playing → sync the navigated-to video's title/author for display. if (e?.data === 1) { @@ -665,7 +715,8 @@ export default function PlayerModal({ 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. */} + so the "Open on YouTube" CTA is clickable, and fully yielded in fullscreen (YouTube + reveals its control bar only while the iframe itself sees the mouse). */} {playerError == null && (
{ @@ -675,7 +726,7 @@ export default function PlayerModal({ title={t("player.shortcutsHint")} aria-label={t("player.shortcutsHint")} className={`absolute inset-x-0 top-[12%] bottom-[22%] z-base cursor-pointer ${ - nativeControls ? "pointer-events-none" : "" + nativeControls || isFullscreen ? "pointer-events-none" : "" }`} /> )} diff --git a/frontend/src/i18n/locales/en/player.json b/frontend/src/i18n/locales/en/player.json index 4f19dd2..3023af9 100644 --- a/frontend/src/i18n/locales/en/player.json +++ b/frontend/src/i18n/locales/en/player.json @@ -35,6 +35,6 @@ "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", + "shortcutsHint": "Click: play/pause · Scroll or ↑ ↓: volume · F: fullscreen", "nativeControls": "YouTube controls active" } diff --git a/frontend/src/i18n/locales/hu/player.json b/frontend/src/i18n/locales/hu/player.json index 398fcbc..c2b4b36 100644 --- a/frontend/src/i18n/locales/hu/player.json +++ b/frontend/src/i18n/locales/hu/player.json @@ -35,6 +35,6 @@ "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ő", + "shortcutsHint": "Kattintás: lejátszás/szünet · Görgő vagy ↑ ↓: hangerő · F: teljes képernyő", "nativeControls": "YouTube vezérlők aktívak" } diff --git a/frontend/src/lib/storage.ts b/frontend/src/lib/storage.ts index 40e900b..fa98bb5 100644 --- a/frontend/src/lib/storage.ts +++ b/frontend/src/lib/storage.ts @@ -50,6 +50,9 @@ export const LS = { plexQ: "siftlode.plexQ", plexPlaylistLayout: "siftlode.plexPlaylistLayout", plexPlayerPrefs: "siftlode.plexPlayerPrefs", + // The YouTube modal player's volume/mute. A YT.Player instance is created per video and starts + // at full volume, so without this every next video is loud again (same idea as plexPlayerPrefs). + ytPlayerPrefs: "siftlode.ytPlayerPrefs", notifHistory: "siftlode.notifications", notifSettings: "siftlode.notifSettings", onboardingDismissed: "siftlode.onboarding.dismissed", From 2a7f49c981ff7c6137931fbb5aed27476516ad41 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 23 Jul 2026 23:25:06 +0200 Subject: [PATCH 05/19] =?UTF-8?q?fix(r5):=20address=20the=20/code-review?= =?UTF-8?q?=20high=20round=20=E2=80=94=2010=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - HLS sweep only deletes directories matching the session-name shape; the root is admin-configurable and may be a shared path - playlist unwrap picks the first entry that actually downloaded, not entries[0] - concat quoting keeps backslashes literal (ffmpeg treats them so inside '') - ArrowUp/Down defer while the focused element can still scroll that way - the staging fallback skips yt-dlp working files (.part-Frag/.ytdl/.temp/.fNNN) - the boot HLS sweep runs off the event loop - volume persistence is debounced (was a setItem per wheel notch) and a spin no longer loses notches to the iframe's lagging getVolume - _Breaker.run owns the whole open/call/record protocol; four call sites shrink - should_prune extracted and unit-tested, plus RSS error-vs-empty tests - the worker exits non-zero when the schema never lands --- backend/app/downloads/edit.py | 11 +-- backend/app/main.py | 7 +- backend/app/plex/stream.py | 20 ++++-- backend/app/sync/playlists.py | 85 ++++++++++++---------- backend/app/sync/subscriptions.py | 21 ++++-- backend/app/worker.py | 39 +++++++--- backend/tests/test_download_output.py | 38 +++++++++- backend/tests/test_sync_guards.py | 96 ++++++++++++++++++++++++- frontend/src/components/PlayerModal.tsx | 69 +++++++++++++++--- 9 files changed, 307 insertions(+), 79 deletions(-) diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 395721d..97f102b 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -184,11 +184,12 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] def _concat_quote(path: Path) -> str: """Escape a path for a single-quoted `file '…'` line of an ffmpeg concat list. - The demuxer's own rule: inside single quotes a literal `'` is written `'\\''` (close, escaped - quote, reopen) and a `\\` must be doubled. 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 there makes the whole list unparseable.""" - return path.as_posix().replace("\\", "\\\\").replace("'", "'\\''") + 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): diff --git a/backend/app/main.py b/backend/app/main.py index 45643c4..e419b3a 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1,3 +1,4 @@ +import asyncio import logging import sys from contextlib import asynccontextmanager @@ -69,8 +70,10 @@ async def lifespan(app: FastAPI): 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. - swept = plex_stream.sweep_orphans() + # a crashed run's segments — sweep them or they accumulate forever. Off the event loop: the + # first sweep after a long uptime can be gigabytes of `.ts` files, and blocking here delays + # serving (a 502 + a failed post-deploy health probe). + swept = await asyncio.to_thread(plex_stream.sweep_orphans) if swept: log.info("swept %d orphaned HLS segment directories", swept) start_scheduler() diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 4401db1..198704f 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,11 @@ 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). +# `sweep_orphans` deletes ONLY names matching this shape: `_HLS_ROOT` is admin-configurable +# (PLEX_HLS_DIR), so it may legitimately point at a shared scratch path we do not own — a blanket +# "delete every directory under the root" would take the neighbours with it. +_SESSION_DIR_RE = re.compile(r"^.+_\d+_[^_]+_[+-]\d+\.\d{2}$") _lock = threading.Lock() _sessions: dict[str, "HlsSession"] = {} @@ -255,12 +261,16 @@ def wait_for(path: Path, timeout: float = 20.0) -> bool: def sweep_orphans() -> int: - """Delete segment directories no live session owns. + """Delete OUR segment directories that no live session owns. - Sessions live only in THIS process, so at startup every directory under `_HLS_ROOT` is a - leftover from a crashed or killed run — and nothing else ever reaps them (the download GC + 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.""" + 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.""" with _lock: live = {s.dir for s in _sessions.values()} dropped = 0 @@ -269,7 +279,7 @@ def sweep_orphans() -> int: except OSError: # the root doesn't exist yet — nothing to sweep return 0 for p in entries: - if p.is_dir() and p not in live: + if p.is_dir() and p not in live and _SESSION_DIR_RE.match(p.name): shutil.rmtree(p, ignore_errors=True) dropped += 1 return dropped diff --git a/backend/app/sync/playlists.py b/backend/app/sync/playlists.py index 82926c7..27e4049 100644 --- a/backend/app/sync/playlists.py +++ b/backend/app/sync/playlists.py @@ -7,6 +7,7 @@ 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 sqlalchemy import delete, select @@ -271,8 +272,16 @@ def plan_push(db: Session, user: User, pl: Playlist, *, live=None) -> dict: _MAX_CONSECUTIVE_FAILURES = 5 +#: Returned by `_Breaker.run` when the write did not happen (refused or failed). +FAILED = object() + + class _Breaker: - """Consecutive-failure circuit breaker for a single push run. Any success re-arms it.""" + """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 @@ -296,6 +305,22 @@ class _Breaker: self.limit, ) + def run(self, op: Callable[[], object], on_fail: Callable[[], None]) -> object: + """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 @@ -313,16 +338,11 @@ 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: - if not breaker.open(): - failures.append(vid) - continue - try: - yt.add_playlist_item(pl.yt_playlist_id, vid) + if breaker.run( + lambda v=vid: yt.add_playlist_item(pl.yt_playlist_id, v), + lambda v=vid: failures.append(v), + ) is not FAILED: inserted += 1 - breaker.hit(True) - except YouTubeError: - failures.append(vid) - breaker.hit(False) if not failures: pl.synced_fingerprint = fingerprint(pl.name, desired) pl.dirty = False @@ -354,30 +374,22 @@ 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: - if not breaker.open(): - failures.append(it["video_id"]) - continue - try: - yt.delete_playlist_item(it["item_id"]) + if breaker.run( + lambda i=it: yt.delete_playlist_item(i["item_id"]), + lambda i=it: failures.append(i["video_id"]), + ) is not FAILED: deleted += 1 - breaker.hit(True) - except YouTubeError: - failures.append(it["video_id"]) - breaker.hit(False) # Insert missing (append; the reorder pass below fixes positions). cur_set = set(cur_vids) for vid in desired: if vid not in cur_set: - if not breaker.open(): - failures.append(vid) - continue - 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 item_id is not FAILED: + item_id_by_vid[vid] = item_id inserted += 1 - breaker.hit(True) - except YouTubeError: - failures.append(vid) - breaker.hit(False) # 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 @@ -387,17 +399,14 @@ 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 - if not breaker.open(): - failures.append(want) - continue - try: - yt.move_playlist_item(item_id_by_vid[want], pl.yt_playlist_id, want, i) - reordered += 1 - breaker.hit(True) - except YouTubeError: - failures.append(want) - breaker.hit(False) + if 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), + ) is FAILED: continue + reordered += 1 j = model.index(want) model.pop(j) model.insert(i, want) diff --git a/backend/app/sync/subscriptions.py b/backend/app/sync/subscriptions.py index 1d95ec7..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,18 +120,15 @@ 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 — but NEVER on an empty fetch. 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 one - # run. Someone who genuinely unsubscribed from everything simply keeps their rows until a - # sync that returns something. + # 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() ) - prune_skipped = bool(existing) and not fetched + 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 " diff --git a/backend/app/worker.py b/backend/app/worker.py index 00c1787..e319a55 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 @@ -351,6 +352,17 @@ def _staging_dir(asset_id: int) -> Path: _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: @@ -367,7 +379,8 @@ def _produced_file(info: dict, staging: Path) -> Path: 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 non-thumbnail file left in staging.""" + 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: @@ -377,7 +390,9 @@ def _produced_file(info: dict, staging: Path) -> Path: cands = [ p for p in staging.iterdir() - if p.is_file() and p.suffix.lower() not in _THUMB_SUFFIXES and not p.name.endswith(".part") + 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") @@ -388,11 +403,15 @@ 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 the first entry.""" + 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 entries and not info.get("requested_downloads"): - return {**info, **entries[0]} - return info + 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). @@ -729,9 +748,13 @@ def main() -> None: return # The API may still be applying migrations when we boot. If they never arrive, exit instead of - # pressing on into a guaranteed crash — the container's restart policy retries the whole wait. + # 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(): - return + 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/tests/test_download_output.py b/backend/tests/test_download_output.py index d48bcad..3f9e51c 100644 --- a/backend/tests/test_download_output.py +++ b/backend/tests/test_download_output.py @@ -38,6 +38,22 @@ class TestProducedFile: (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): @@ -53,6 +69,22 @@ class TestMediaInfo: 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" @@ -98,8 +130,10 @@ class TestConcatQuote: # 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_is_doubled(self): - assert _concat_quote(PurePosixPath("/m/a\\b.mp4")) == "/m/a\\\\b.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_sync_guards.py b/backend/tests/test_sync_guards.py index c887b5d..484f906 100644 --- a/backend/tests/test_sync_guards.py +++ b/backend/tests/test_sync_guards.py @@ -1,5 +1,28 @@ -"""Guards that stop one bad YouTube answer from cascading: the playlist-push circuit breaker.""" -from app.sync.playlists import _Breaker +"""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.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: @@ -26,3 +49,72 @@ class TestBreaker: b.hit(False) b.hit(True) assert not b.open() + + +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(lambda: (_ for _ in ()).throw(YouTubeError("quota")), lambda: failed.append("v1")) is FAILED + assert failed == ["v1"] and b.open() + b.run(lambda: (_ for _ in ()).throw(YouTubeError("quota")), 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(lambda: (_ for _ in ()).throw(YouTubeError("quota")), 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. + with pytest.raises(ValueError): + _Breaker().run(lambda: (_ for _ in ()).throw(ValueError("bug")), lambda: None) + + +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/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 96930c1..8e7d268 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -55,6 +55,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 { @@ -168,9 +183,15 @@ export default function PlayerModal({ const [playerPrefs, patchPlayerPrefs] = useAccountPersistedObject(LS.ytPlayerPrefs, { volume: 100, }); - // The window/wheel listeners below are bound once, so they must read the LIVE prefs. - const prefsRef = useRef(playerPrefs); - prefsRef.current = playerPrefs; + // The live volume: updated on every notch, while the localStorage write behind it is debounced + // (a wheel spin fires ~16 events a second, and each patch is a synchronous setItem on the frame + // the player is decoding on). Everything that READS the level reads this ref, so it never sees + // the stale persisted value mid-spin. The window/wheel listeners are bound once, so a ref — not + // state — is what they can safely close over. + const volumeRef = useRef(playerPrefs.volume); + 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 = () => { @@ -193,6 +214,11 @@ export default function PlayerModal({ }; /** 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 persistVolume = () => { + window.clearTimeout(volSaveTimerRef.current); + volSaveTimerRef.current = undefined; + patchPlayerPrefs({ volume: volumeRef.current }); + }; const applyVolume = (level: number, flash = true) => { const next = Math.max(0, Math.min(100, Math.round(level))); const p = playerRef.current; @@ -201,15 +227,29 @@ export default function PlayerModal({ p.setVolume(next); if (next === 0) p.mute?.(); } - patchPlayerPrefs({ volume: next }); + 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) => { const p = playerRef.current; if (!p || typeof p.getVolume !== "function") return; - // Read the live player (the user may have used YouTube's own slider), falling back to the - // stored level when the player can't answer yet. - const cur = p.isMuted?.() ? 0 : (p.getVolume?.() ?? prefsRef.current.volume); + // 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 cur = continuing + ? volumeRef.current + : p.isMuted?.() + ? 0 + : (p.getVolume?.() ?? volumeRef.current); applyVolume(cur + delta); }; @@ -425,10 +465,14 @@ export default function PlayerModal({ 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. - if (typing) return; + // 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. + const up = e.key === "ArrowUp"; + if (typing || canScrollBy(el, up ? -1 : 1)) return; e.preventDefault(); - nudgeVolume(e.key === "ArrowUp" ? 5 : -5); + 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). @@ -453,6 +497,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]); @@ -582,7 +629,7 @@ export default function PlayerModal({ // restore the remembered volume — a fresh player always starts at 100. onReady: () => { focusModal(); - applyVolume(prefsRef.current.volume, false); + applyVolume(volumeRef.current, false); }, onStateChange: (e: any) => { // 1 === playing → sync the navigated-to video's title/author for display. From 53ff61653daf41a423596fa07ad53f76108bc2a1 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 23 Jul 2026 23:59:58 +0200 Subject: [PATCH 06/19] =?UTF-8?q?fix(r5):=20address=20the=20second=20/code?= =?UTF-8?q?-review=20high=20round=20=E2=80=94=207=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - the volume keys no longer defer to a scrollable card while in fullscreen (mirrored into a ref: the keydown handler is bound once and state would be stale) - the debounced volume flush writes straight to storage; persisting inside a setState updater is not guaranteed to run for an unmounting component - the HLS sweep also matches the original {key}_{start} directory naming, which is what the oldest leftovers on disk actually use - run_rss_poll returns {new, failed} so an egress outage stops rendering as "ok — 0" in the scheduler card and the audit log; /api/sync/rss reports it too - drop the unused poll_rss_channel (it silently gained a new exception) - _Breaker.run is generic, so a call site keeps its item-id type - test helper instead of the generator-throw idiom --- backend/app/plex/stream.py | 8 +++++--- backend/app/routes/sync.py | 6 ++++-- backend/app/sync/playlists.py | 14 +++++++++++--- backend/app/sync/runner.py | 8 ++++++-- backend/app/sync/videos.py | 5 ----- backend/tests/test_sync_guards.py | 16 ++++++++++++---- frontend/src/components/PlayerModal.tsx | 24 +++++++++++++++++------- 7 files changed, 55 insertions(+), 26 deletions(-) diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 198704f..704cf8b 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -34,11 +34,13 @@ 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). -# `sweep_orphans` deletes ONLY names matching this shape: `_HLS_ROOT` is admin-configurable +# 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)}`, so the tail is OPTIONAL here — the +# oldest leftovers on disk carry the old shape and are exactly what the sweep exists to remove. +# `sweep_orphans` deletes ONLY names matching this: `_HLS_ROOT` is admin-configurable # (PLEX_HLS_DIR), so it may legitimately point at a shared scratch path we do not own — a blanket # "delete every directory under the root" would take the neighbours with it. -_SESSION_DIR_RE = re.compile(r"^.+_\d+_[^_]+_[+-]\d+\.\d{2}$") +_SESSION_DIR_RE = re.compile(r"^.+_\d+(_[^_]+_[+-]\d+\.\d{2})?$") _lock = threading.Lock() _sessions: dict[str, "HlsSession"] = {} diff --git a/backend/app/routes/sync.py b/backend/app/routes/sync.py index 9cfa522..7f196a1 100644 --- a/backend/app/routes/sync.py +++ b/backend/app/routes/sync.py @@ -52,8 +52,10 @@ 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} + result = run_rss_poll(db, _user_channels(db, user)) + # `failed` = feeds that could not be read at all (see run_rss_poll); surfaced so a manual poll + # reports an outage instead of an innocent-looking "0 new". + 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 27e4049..3606e28 100644 --- a/backend/app/sync/playlists.py +++ b/backend/app/sync/playlists.py @@ -9,6 +9,7 @@ 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 @@ -272,8 +273,15 @@ def plan_push(db: Session, user: User, pl: Playlist, *, live=None) -> dict: _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 = object() +FAILED = _Failed() + +T = TypeVar("T") class _Breaker: @@ -305,7 +313,7 @@ class _Breaker: self.limit, ) - def run(self, op: Callable[[], object], on_fail: Callable[[], None]) -> object: + 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.""" @@ -387,7 +395,7 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: lambda v=vid: yt.add_playlist_item(pl.yt_playlist_id, v), lambda v=vid: failures.append(v), ) - if item_id is not FAILED: + if not isinstance(item_id, _Failed): item_id_by_vid[vid] = item_id inserted += 1 # Reorder to match `desired` via insertion-sort moves over a local model. diff --git a/backend/app/sync/runner.py b/backend/app/sync/runner.py index d2de856..c2e310e 100644 --- a/backend/app/sync/runner.py +++ b/backend/app/sync/runner.py @@ -48,7 +48,11 @@ 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: + """Poll every channel's free RSS feed. Returns `{"new": inserted, "failed": unreadable_feeds}` + — 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 "every feed was unreachable" (an egress + outage otherwise renders identically to a quiet hour).""" if channels is None: channels = db.execute(select(Channel)).scalars().all() total = len(channels) @@ -89,7 +93,7 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> int: progress.report(done, total, "rss_poll") if failed: log.warning("RSS poll: %d/%d channel feeds could not be read", failed, total) - return new + return {"new": new, "failed": failed} def run_enrich(db: Session, max_batches: int = 200, floor: int = 200) -> int: 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/tests/test_sync_guards.py b/backend/tests/test_sync_guards.py index 484f906..e88aeba 100644 --- a/backend/tests/test_sync_guards.py +++ b/backend/tests/test_sync_guards.py @@ -51,6 +51,11 @@ class TestBreaker: 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(), [] @@ -59,22 +64,25 @@ class TestBreakerRun: def test_a_youtube_error_records_the_failure_and_counts_towards_the_trip(self): b, failed = _Breaker(limit=2), [] - assert b.run(lambda: (_ for _ in ()).throw(YouTubeError("quota")), lambda: failed.append("v1")) is FAILED + assert b.run(_quota_dead, lambda: failed.append("v1")) is FAILED assert failed == ["v1"] and b.open() - b.run(lambda: (_ for _ in ()).throw(YouTubeError("quota")), lambda: failed.append("v2")) + 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(lambda: (_ for _ in ()).throw(YouTubeError("quota")), lambda: failed.append("v1")) + 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(lambda: (_ for _ in ()).throw(ValueError("bug")), lambda: None) + _Breaker().run(boom, lambda: None) class TestRssErrorVsEmpty: diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 8e7d268..75682c3 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -35,7 +35,7 @@ import { } from "../lib/format"; import { renderDescription } from "../lib/descriptionLinks"; import { useBackToClose } from "../lib/history"; -import { LS, useAccountPersistedObject } from "../lib/storage"; +import { LS, useAccountPersistedObject, writeAccount } from "../lib/storage"; import { useScrollFade } from "../lib/useScrollFade"; // Experiment (branch experiment/inline-player): play the video in-app via the @@ -176,6 +176,9 @@ export default function PlayerModal({ // 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); + // 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); // 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 @@ -217,6 +220,11 @@ export default function PlayerModal({ const persistVolume = () => { window.clearTimeout(volSaveTimerRef.current); volSaveTimerRef.current = undefined; + // Write STRAIGHT to storage, not through patchPlayerPrefs: that helper persists inside a + // `setState` updater, and React is under no obligation to run an updater for a component that + // is unmounting — which is exactly when the flush below fires (close the player right after a + // wheel spin). The state copy is only needed while mounted, so patch that separately. + writeAccount(LS.ytPlayerPrefs, { volume: volumeRef.current }); patchPlayerPrefs({ volume: volumeRef.current }); }; const applyVolume = (level: number, flash = true) => { @@ -468,9 +476,10 @@ export default function PlayerModal({ // 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. + // focused button. NOT in fullscreen though: the card is off-screen there, so deferring + // would scroll something invisible and leave fullscreen with no volume control at all. const up = e.key === "ArrowUp"; - if (typing || canScrollBy(el, up ? -1 : 1)) return; + if (typing || (!isFullscreenRef.current && canScrollBy(el, up ? -1 : 1))) return; e.preventDefault(); nudgeVolume(up ? 5 : -5); } else if (e.key === "ArrowLeft" || e.key === "ArrowRight") { @@ -524,10 +533,11 @@ export default function PlayerModal({ // Track whether we're the fullscreen element (F, our button, or the browser's own Esc exit). useEffect(() => { - const onChange = () => - setIsFullscreen( - !!document.fullscreenElement && document.fullscreenElement === stageRef.current, - ); + const onChange = () => { + const ours = !!document.fullscreenElement && document.fullscreenElement === stageRef.current; + isFullscreenRef.current = ours; // read by the once-bound keydown handler + setIsFullscreen(ours); // drives the overlay's pointer-events + }; document.addEventListener("fullscreenchange", onChange); return () => document.removeEventListener("fullscreenchange", onChange); }, []); From c0db80d33365f50466670883904b7cab5201d756 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 24 Jul 2026 00:17:09 +0200 Subject: [PATCH 07/19] =?UTF-8?q?fix(r5):=20address=20the=20third=20/code-?= =?UTF-8?q?review=20high=20round=20=E2=80=94=207=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - the sweep guard anchors the rating_key to digits: the round-2 widening had degenerated to "anything ending in _" (backup_2024, release_10) - RSS counts apply-side failures too, so a read-only database no longer reports a wholly failed poll as "ok — new=0, failed=0" - the player reads/writes its volume directly (readAccount/writeAccount): the persisted-object hook rendered nothing and cost a second write per settle - new tests: the sweep guard's accept/reject table, and run_ffmpeg's threading (drain, cancel while silent, stall watchdog, stderr tail on a nonzero exit) - the breaker sentinel is checked one way (isinstance) at all four sites - run_rss_poll is typed dict[str, int] All three new test groups were mutation-checked: reverting each fix turns them red (or, for the cancel/stall pair, hangs — which is the bug itself). --- backend/app/plex/stream.py | 18 ++-- backend/app/sync/playlists.py | 15 +-- backend/app/sync/runner.py | 19 ++-- backend/tests/test_plex_stream.py | 44 ++++++++ backend/tests/test_run_ffmpeg.py | 133 ++++++++++++++++++++++++ backend/tests/test_sync_guards.py | 50 +++++++++ frontend/src/components/PlayerModal.tsx | 27 ++--- 7 files changed, 271 insertions(+), 35 deletions(-) create mode 100644 backend/tests/test_plex_stream.py create mode 100644 backend/tests/test_run_ffmpeg.py diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 704cf8b..d55e243 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -35,12 +35,18 @@ _HLS_ROOT = Path(settings.plex_hls_dir) if settings.plex_hls_dir else Path(setti _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)}`, so the tail is OPTIONAL here — the -# oldest leftovers on disk carry the old shape and are exactly what the sweep exists to remove. -# `sweep_orphans` deletes ONLY names matching this: `_HLS_ROOT` is admin-configurable -# (PLEX_HLS_DIR), so it may legitimately point at a shared scratch path we do not own — a blanket -# "delete every directory under the root" would take the neighbours with it. -_SESSION_DIR_RE = re.compile(r"^.+_\d+(_[^_]+_[+-]\d+\.\d{2})?$") +# 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})?$") _lock = threading.Lock() _sessions: dict[str, "HlsSession"] = {} diff --git a/backend/app/sync/playlists.py b/backend/app/sync/playlists.py index 3606e28..81ecb2c 100644 --- a/backend/app/sync/playlists.py +++ b/backend/app/sync/playlists.py @@ -346,10 +346,11 @@ 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: - if breaker.run( + added = breaker.run( lambda v=vid: yt.add_playlist_item(pl.yt_playlist_id, v), lambda v=vid: failures.append(v), - ) is not FAILED: + ) + if not isinstance(added, _Failed): inserted += 1 if not failures: pl.synced_fingerprint = fingerprint(pl.name, desired) @@ -382,10 +383,11 @@ 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: - if breaker.run( + dropped = breaker.run( lambda i=it: yt.delete_playlist_item(i["item_id"]), lambda i=it: failures.append(i["video_id"]), - ) is not FAILED: + ) + if not isinstance(dropped, _Failed): deleted += 1 # Insert missing (append; the reorder pass below fixes positions). cur_set = set(cur_vids) @@ -407,12 +409,13 @@ 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 - if breaker.run( + 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), - ) is FAILED: + ) + if isinstance(moved, _Failed): continue reordered += 1 j = model.index(want) diff --git a/backend/app/sync/runner.py b/backend/app/sync/runner.py index c2e310e..a75b196 100644 --- a/backend/app/sync/runner.py +++ b/backend/app/sync/runner.py @@ -48,11 +48,15 @@ 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) -> dict: - """Poll every channel's free RSS feed. Returns `{"new": inserted, "failed": unreadable_feeds}` - — 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 "every feed was unreachable" (an egress - outage otherwise renders identically to a quiet hour).""" +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.""" if channels is None: channels = db.execute(select(Channel)).scalars().all() total = len(channels) @@ -87,12 +91,15 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict: try: new += apply_rss_feed(db, channel, entries) except Exception: + # 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 still reports "failed=0". db.rollback() + failed += 1 log.exception("RSS apply failed for channel %s", channel.id) done += 1 progress.report(done, total, "rss_poll") if failed: - log.warning("RSS poll: %d/%d channel feeds could not be read", failed, total) + log.warning("RSS poll: %d/%d channels could not be polled", failed, total) return {"new": new, "failed": failed} diff --git a/backend/tests/test_plex_stream.py b/backend/tests/test_plex_stream.py new file mode 100644 index 0000000..fcf12d8 --- /dev/null +++ b/backend/tests/test_plex_stream.py @@ -0,0 +1,44 @@ +"""The guard 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.""" +import pytest + +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) diff --git a/backend/tests/test_run_ffmpeg.py b/backend/tests/test_run_ffmpeg.py new file mode 100644 index 0000000..2ea5b21 --- /dev/null +++ b/backend/tests/test_run_ffmpeg.py @@ -0,0 +1,133 @@ +"""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 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): + self.stdout = stdout + self.stderr = stderr + self._rc = returncode + self.terminated = False + self.killed = False + + 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): + 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 proc._rc == 2 + + +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_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 index e88aeba..a658ee4 100644 --- a/backend/tests/test_sync_guards.py +++ b/backend/tests/test_sync_guards.py @@ -3,6 +3,7 @@ playlist-push circuit breaker, and RSS telling a failure apart from an empty fee 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 @@ -85,6 +86,55 @@ class TestBreakerRun: _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_): + monkeypatch.setattr(runner, "fetch_channel_feed", fetch) + monkeypatch.setattr(runner, "apply_rss_feed", apply_) + return runner.run_rss_poll(_StubDb(), [_StubChannel("UC1"), _StubChannel("UC2")]) + + 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): + def boom(cid): + raise rss.RssError("404") + + out = self._run(monkeypatch, fetch=boom, apply_=lambda db, ch, e: 1) + assert out == {"new": 0, "failed": 2} + + def test_apply_failures_are_counted_too(self, monkeypatch): + # A read-only database: every feed reads fine, nothing can be stored. + def boom(db, ch, entries): + raise RuntimeError("read-only transaction") + + out = self._run(monkeypatch, fetch=lambda cid: [{"id": "v"}], apply_=boom) + assert out == {"new": 0, "failed": 2}, "a run where nothing was stored 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 + + class TestRssErrorVsEmpty: """A failed poll must RAISE (so the caller leaves `last_rss_at` alone); only a genuinely empty feed may return [].""" diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 75682c3..3368479 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -35,7 +35,7 @@ import { } from "../lib/format"; import { renderDescription } from "../lib/descriptionLinks"; import { useBackToClose } from "../lib/history"; -import { LS, useAccountPersistedObject, writeAccount } from "../lib/storage"; +import { LS, readAccount, writeAccount } from "../lib/storage"; import { useScrollFade } from "../lib/useScrollFade"; // Experiment (branch experiment/inline-player): play the video in-app via the @@ -183,15 +183,12 @@ export default function PlayerModal({ // 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. - const [playerPrefs, patchPlayerPrefs] = useAccountPersistedObject(LS.ytPlayerPrefs, { - volume: 100, - }); - // The live volume: updated on every notch, while the localStorage write behind it is debounced - // (a wheel spin fires ~16 events a second, and each patch is a synchronous setItem on the frame - // the player is decoding on). Everything that READS the level reads this ref, so it never sees - // the stale persisted value mid-spin. The window/wheel listeners are bound once, so a ref — not - // state — is what they can safely close over. - const volumeRef = useRef(playerPrefs.volume); + // + // 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. + const volumeRef = useRef(readAccount(LS.ytPlayerPrefs, { volume: 100 }).volume); const volSaveTimerRef = useRef(undefined); // When the last volume gesture happened — see nudgeVolume (a spin must not re-read the player). const lastNudgeAtRef = useRef(0); @@ -215,18 +212,14 @@ export default function PlayerModal({ window.clearTimeout(volTimerRef.current); volTimerRef.current = window.setTimeout(() => setVolumeUi(null), 1200); }; - /** 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). */ + /** Write the current level out now, cancelling any pending debounce. */ const persistVolume = () => { window.clearTimeout(volSaveTimerRef.current); volSaveTimerRef.current = undefined; - // Write STRAIGHT to storage, not through patchPlayerPrefs: that helper persists inside a - // `setState` updater, and React is under no obligation to run an updater for a component that - // is unmounting — which is exactly when the flush below fires (close the player right after a - // wheel spin). The state copy is only needed while mounted, so patch that separately. writeAccount(LS.ytPlayerPrefs, { volume: volumeRef.current }); - patchPlayerPrefs({ 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) => { const next = Math.max(0, Math.min(100, Math.round(level))); const p = playerRef.current; From fc82c8b47afe07341b72183e25c0bf44429c5e38 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 24 Jul 2026 00:45:42 +0200 Subject: [PATCH 08/19] =?UTF-8?q?fix(r5):=20address=20the=20fourth=20/code?= =?UTF-8?q?-review=20high=20round=20=E2=80=94=206=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Feed memoizes onClose (like Playlists already did): the inline closure re-ran PlayerModal's keydown effect on every Feed render, which since the debounce landed also flushed the volume write and re-stole focus - the player reads its volume with readAccountMerged again — a partial stored object made the level undefined -> NaN -> a persisted null; applyVolume also refuses a non-finite level outright - that read moved into a lazy useState initializer (was running per render) - the two RSS except branches share one tail, so a counter can't drift again - run_ffmpeg tests cover the SIGTERM->SIGKILL escalation and the lingering- process path; dropped an assertion that only restated the fixture - storage.ts documents the actual { volume } shape (0 IS muted) --- backend/app/sync/runner.py | 36 ++++++++++++------------- backend/tests/test_run_ffmpeg.py | 33 +++++++++++++++++++++-- frontend/src/components/Feed.tsx | 9 +++++-- frontend/src/components/PlayerModal.tsx | 15 ++++++++--- frontend/src/lib/storage.ts | 5 ++-- 5 files changed, 70 insertions(+), 28 deletions(-) diff --git a/backend/app/sync/runner.py b/backend/app/sync/runner.py index a75b196..bb41b0e 100644 --- a/backend/app/sync/runner.py +++ b/backend/app/sync/runner.py @@ -74,28 +74,26 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str channel = futures[fut] try: entries = fut.result() - except RssError as exc: + except Exception as exc: # A failed poll is NOT an empty feed: skip the apply so `last_rss_at` stays where # it was. Stamping it here made a permanently-broken feed look freshly polled. - log.warning("RSS poll skipped for channel %s: %s", channel.id, exc) + # An RssError is an EXPECTED outcome (404, timeout) → one warning line; anything + # else is a bug on our side and earns a traceback. + if isinstance(exc, RssError): + log.warning("RSS poll skipped for channel %s: %s", channel.id, exc) + else: + log.exception("RSS fetch failed for channel %s", channel.id) failed += 1 - done += 1 - progress.report(done, total, "rss_poll") - continue - except Exception: - log.exception("RSS fetch failed for channel %s", channel.id) - failed += 1 - done += 1 - progress.report(done, total, "rss_poll") - continue - try: - new += apply_rss_feed(db, channel, entries) - except Exception: - # 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 still reports "failed=0". - db.rollback() - failed += 1 - log.exception("RSS apply failed for channel %s", channel.id) + else: + try: + new += apply_rss_feed(db, channel, entries) + except Exception: + # 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 + 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") if failed: diff --git a/backend/tests/test_run_ffmpeg.py b/backend/tests/test_run_ffmpeg.py index 2ea5b21..5dec4dc 100644 --- a/backend/tests/test_run_ffmpeg.py +++ b/backend/tests/test_run_ffmpeg.py @@ -4,6 +4,7 @@ No ffmpeg here: a fake Popen stands in, so the reader threads, the 1s poll loop, 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 @@ -39,12 +40,16 @@ class _FakeStream: class _FakeProc: pid = 4242 - def __init__(self, stdout, stderr, returncode=0): + 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 terminate(self): self.terminated = True @@ -57,6 +62,10 @@ class _FakeProc: 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 @@ -96,7 +105,7 @@ def test_a_nonzero_exit_raises_with_the_stderr_tail(fake_popen): ) 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 proc._rc == 2 + assert not proc.terminated # it exited on its own; nothing to kill def test_cancel_is_honoured_while_the_process_is_silent(fake_popen): @@ -123,6 +132,26 @@ def test_a_wedged_process_is_killed_by_the_stall_watchdog(fake_popen): assert proc.terminated # the worker thread is freed, not parked forever +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 + + 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. 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 3368479..34f175b 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -35,7 +35,7 @@ import { } from "../lib/format"; import { renderDescription } from "../lib/descriptionLinks"; import { useBackToClose } from "../lib/history"; -import { LS, readAccount, writeAccount } from "../lib/storage"; +import { LS, readAccountMerged, writeAccount } from "../lib/storage"; import { useScrollFade } from "../lib/useScrollFade"; // Experiment (branch experiment/inline-player): play the video in-app via the @@ -188,7 +188,14 @@ export default function PlayerModal({ // (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. - const volumeRef = useRef(readAccount(LS.ytPlayerPrefs, { volume: 100 }).volume); + // `readAccountMerged` (not readAccount) so a partial/older stored object still gets the default + // — a missing `volume` would otherwise travel as undefined into the arithmetic below. 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( + () => readAccountMerged(LS.ytPlayerPrefs, { volume: 100 }).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); @@ -221,7 +228,9 @@ export default function PlayerModal({ /** 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) => { - const next = Math.max(0, Math.min(100, Math.round(level))); + // A non-finite level (a corrupt stored value, a player that answered NaN) must not become a + // clamped NaN: that would mute the player AND persist `null`, poisoning every later session. + const next = Number.isFinite(level) ? Math.max(0, Math.min(100, Math.round(level))) : 100; const p = playerRef.current; if (p && typeof p.setVolume === "function") { if (next > 0) p.unMute?.(); diff --git a/frontend/src/lib/storage.ts b/frontend/src/lib/storage.ts index fa98bb5..9ae2496 100644 --- a/frontend/src/lib/storage.ts +++ b/frontend/src/lib/storage.ts @@ -50,8 +50,9 @@ export const LS = { plexQ: "siftlode.plexQ", plexPlaylistLayout: "siftlode.plexPlaylistLayout", plexPlayerPrefs: "siftlode.plexPlayerPrefs", - // The YouTube modal player's volume/mute. A YT.Player instance is created per video and starts - // at full volume, so without this every next video is loud again (same idea as plexPlayerPrefs). + // The YouTube modal player's volume (`{ volume: 0–100 }`; 0 IS muted — there is no separate + // flag). A YT.Player instance is created per video and starts at full volume, so without this + // every next video is loud again (same idea as plexPlayerPrefs). ytPlayerPrefs: "siftlode.ytPlayerPrefs", notifHistory: "siftlode.notifications", notifSettings: "siftlode.notifSettings", From c4942eb5901da7e51989578a0ba233d6ffc9190e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 24 Jul 2026 01:50:50 +0200 Subject: [PATCH 09/19] =?UTF-8?q?fix(r5):=20address=20the=20fifth=20/code-?= =?UTF-8?q?review=20high=20round=20=E2=80=94=206=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - run_ffmpeg stops the process from ONE exit path (`except BaseException`), so an exception out of on_progress (a DB write) can no longer orphan a live ffmpeg after a 10s join on its pipes; _stop is now a no-op on a dead process - both run_ffmpeg callbacks are throttled to ~1s (they were a DB round-trip per output line: ~10^5 queries on a long re-encode), mirroring the download hook - volume arithmetic extracted to lib/playerVolume with unit tests; a corrupt level now falls back to the LAST GOOD one instead of full blast, and the sanitising happens where the stored value enters - RSS is back to two except clauses (the shared tail already sits after the try) - new tests: callback-exception cleanup, the two throttles; the fake Popen grew poll() and its wait count is now asserted Mutation-checked: each new group goes red when its fix is reverted. --- backend/app/downloads/edit.py | 15 +++-- backend/app/sync/runner.py | 19 +++--- backend/app/worker.py | 48 ++++++++++++-- backend/tests/test_ffmpeg_callbacks.py | 83 +++++++++++++++++++++++++ backend/tests/test_run_ffmpeg.py | 20 ++++++ frontend/src/components/PlayerModal.tsx | 29 ++++----- frontend/src/lib/playerVolume.test.ts | 57 +++++++++++++++++ frontend/src/lib/playerVolume.ts | 30 +++++++++ 8 files changed, 268 insertions(+), 33 deletions(-) create mode 100644 backend/tests/test_ffmpeg_callbacks.py create mode 100644 frontend/src/lib/playerVolume.test.ts create mode 100644 frontend/src/lib/playerVolume.ts diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 97f102b..99235ae 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -288,7 +288,10 @@ def _drain_tail(stream, sink: list[str]) -> None: def _stop(proc: subprocess.Popen) -> None: - """Terminate, then kill — never wait unbounded on a process we've given up on.""" + """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) @@ -335,16 +338,20 @@ def run_ffmpeg(cmd, total_s, on_progress, should_cancel, stall_timeout_s=_FFMPEG if secs is not None and total_s: on_progress(max(0.0, min(99.0, secs * 100.0 / total_s))) if should_cancel(): - _stop(proc) raise EditAborted if time.monotonic() - last_output > stall_timeout_s: - _stop(proc) 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 - _stop(proc) 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: for t in readers: t.join(timeout=_FFMPEG_EXIT_GRACE_S) diff --git a/backend/app/sync/runner.py b/backend/app/sync/runner.py index bb41b0e..f4f1109 100644 --- a/backend/app/sync/runner.py +++ b/backend/app/sync/runner.py @@ -74,15 +74,16 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str channel = futures[fut] try: entries = fut.result() - except Exception as exc: - # A failed poll is NOT an empty feed: skip the apply so `last_rss_at` stays where - # it was. Stamping it here made a permanently-broken feed look freshly polled. - # An RssError is an EXPECTED outcome (404, timeout) → one warning line; anything - # else is a bug on our side and earns a traceback. - if isinstance(exc, RssError): - log.warning("RSS poll skipped for channel %s: %s", channel.id, exc) - else: - log.exception("RSS fetch failed for channel %s", channel.id) + # 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 + except Exception: + log.exception("RSS fetch failed for channel %s", channel.id) failed += 1 else: try: diff --git a/backend/app/worker.py b/backend/app/worker.py index e319a55..faf5086 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -319,6 +319,46 @@ def _make_progress_hook(job_id: int, asset_id: int): return hook +# ffmpeg's `-progress pipe:1` emits a block of ~12 lines every ~0.5s, and run_ffmpeg calls both of +# its callbacks once per LINE. Each of these is a database round-trip (`_job_status` opens a session +# and SELECTs; `_set_job` opens one and UPDATE+COMMITs), so an unthrottled pair costs ~10^5 queries +# on a long re-encode. Both are therefore rate-limited to ~1s — the same 1s DB throttle the +# download progress hook already applies (see `_make_progress_hook`). +_FFMPEG_CALLBACK_INTERVAL_S = 1.0 + + +def _ffmpeg_progress_writer(job_id: int, phase: str): + """`on_progress` for run_ffmpeg: persist at most one row write per second, and only when the + whole-percent value actually changed (the bar shows integers).""" + state = {"at": 0.0, "pct": -1} + + def write(pct: float) -> None: + whole = int(pct) + now = time.monotonic() + if whole == state["pct"] or now - state["at"] < _FFMPEG_CALLBACK_INTERVAL_S: + return + state["at"] = now + state["pct"] = whole + _set_job(job_id, progress=whole, phase=phase) + + return write + + +def _ffmpeg_cancel_poll(job_id: int): + """`should_cancel` for run_ffmpeg: ask the DB at most once a second and reuse the answer in + between. Cancellation is sticky, so once it says yes it never queries again.""" + state = {"at": 0.0, "cancelled": False} + + def cancelled() -> bool: + now = time.monotonic() + if not state["cancelled"] and now - state["at"] >= _FFMPEG_CALLBACK_INTERVAL_S: + state["at"] = now + state["cancelled"] = _job_status(job_id) not in ("running", None) + return state["cancelled"] + + return cancelled + + 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 @@ -492,8 +532,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=_ffmpeg_cancel_poll(job_id), ) produced.unlink(missing_ok=True) # Keep the stored asset metadata truthful about what's now on disk. @@ -671,8 +711,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=_ffmpeg_cancel_poll(job_id), ) if not dest_staging.exists(): raise RuntimeError("ffmpeg produced no output file") diff --git a/backend/tests/test_ffmpeg_callbacks.py b/backend/tests/test_ffmpeg_callbacks.py new file mode 100644 index 0000000..c2644f5 --- /dev/null +++ b/backend/tests/test_ffmpeg_callbacks.py @@ -0,0 +1,83 @@ +"""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)) + monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"]) + write = worker._ffmpeg_progress_writer(7, "processing") + 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)) + monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"]) + write = worker._ffmpeg_progress_writer(7, "processing") + 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._ffmpeg_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) + monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"]) + cancelled = worker._ffmpeg_cancel_poll(7) + 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"]) + monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"]) + cancelled = worker._ffmpeg_cancel_poll(7) + 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_run_ffmpeg.py b/backend/tests/test_run_ffmpeg.py index 5dec4dc..d72bde2 100644 --- a/backend/tests/test_run_ffmpeg.py +++ b/backend/tests/test_run_ffmpeg.py @@ -51,6 +51,11 @@ class _FakeProc: 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. @@ -132,6 +137,19 @@ def test_a_wedged_process_is_killed_by_the_stall_watchdog(fake_popen): 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. @@ -150,6 +168,8 @@ def test_a_process_lingering_after_eof_is_stopped_and_reported(fake_popen): 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): diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 34f175b..ab340e1 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -35,6 +35,7 @@ import { } from "../lib/format"; import { renderDescription } from "../lib/descriptionLinks"; import { useBackToClose } from "../lib/history"; +import { DEFAULT_VOLUME, normalizeVolume, stepVolume } from "../lib/playerVolume"; import { LS, readAccountMerged, writeAccount } from "../lib/storage"; import { useScrollFade } from "../lib/useScrollFade"; @@ -188,12 +189,12 @@ export default function PlayerModal({ // (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. - // `readAccountMerged` (not readAccount) so a partial/older stored object still gets the default - // — a missing `volume` would otherwise travel as undefined into the arithmetic below. 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( - () => readAccountMerged(LS.ytPlayerPrefs, { volume: 100 }).volume, + // 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); @@ -227,10 +228,10 @@ export default function PlayerModal({ }; /** 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) => { - // A non-finite level (a corrupt stored value, a player that answered NaN) must not become a - // clamped NaN: that would mute the player AND persist `null`, poisoning every later session. - const next = Number.isFinite(level) ? Math.max(0, Math.min(100, Math.round(level))) : 100; + const applyVolume = (level: unknown, flash = true) => { + // An unusable level (a player that answered NaN) falls back to the LAST GOOD one, never to 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.setVolume === "function") { if (next > 0) p.unMute?.(); @@ -255,12 +256,8 @@ export default function PlayerModal({ const now = Date.now(); const continuing = now - lastNudgeAtRef.current < 1000; lastNudgeAtRef.current = now; - const cur = continuing - ? volumeRef.current - : p.isMuted?.() - ? 0 - : (p.getVolume?.() ?? volumeRef.current); - applyVolume(cur + delta); + const cur = continuing ? volumeRef.current : p.isMuted?.() ? 0 : p.getVolume?.(); + applyVolume(stepVolume(cur, delta, volumeRef.current), true); }; // The player can navigate to other videos (YouTube links in a description). The diff --git a/frontend/src/lib/playerVolume.test.ts b/frontend/src/lib/playerVolume.test.ts new file mode 100644 index 0000000..0915eb9 --- /dev/null +++ b/frontend/src/lib/playerVolume.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from "vitest"; + +import { DEFAULT_VOLUME, normalizeVolume, stepVolume } from "./playerVolume"; + +describe("normalizeVolume", () => { + it("keeps a valid level", () => { + expect(normalizeVolume(0)).toBe(0); + expect(normalizeVolume(37)).toBe(37); + expect(normalizeVolume(100)).toBe(100); + }); + + it("clamps out-of-range levels and rounds to whole percents", () => { + expect(normalizeVolume(-20)).toBe(0); + expect(normalizeVolume(150)).toBe(100); + expect(normalizeVolume(42.6)).toBe(43); + }); + + it.each([ + ["a missing key", undefined], + ["an explicit null", null], + ["a stringified number", "80"], + ["NaN from a broken player", NaN], + ["a whole object", { volume: 50 }], + ])("falls back on %s", (_label, raw) => { + // The regression: a partial stored object made the level undefined, which Math.round turned + // into NaN — muting playback and persisting `null` for every later session. + expect(normalizeVolume(raw, 15)).toBe(15); + }); + + it("prefers the last known level over the default, so a quiet session stays quiet", () => { + expect(normalizeVolume(NaN, 15)).toBe(15); + expect(normalizeVolume(NaN, 15)).not.toBe(DEFAULT_VOLUME); + }); + + it("uses the default when the fallback is unusable too", () => { + expect(normalizeVolume(NaN, NaN)).toBe(DEFAULT_VOLUME); + expect(normalizeVolume(null, undefined as unknown as number)).toBe(DEFAULT_VOLUME); + }); +}); + +describe("stepVolume", () => { + it("steps up and down from the current level", () => { + expect(stepVolume(60, 5, 100)).toBe(65); + expect(stepVolume(60, -5, 100)).toBe(55); + }); + + it("clamps at both ends instead of wrapping", () => { + expect(stepVolume(98, 5, 100)).toBe(100); + expect(stepVolume(3, -5, 100)).toBe(0); + }); + + it("steps from the fallback when the player answers garbage", () => { + // A spin must not lose notches to a stale/unusable reading — it continues from what we last set. + expect(stepVolume(undefined, -5, 90)).toBe(85); + expect(stepVolume(NaN, -5, 90)).toBe(85); + }); +}); diff --git a/frontend/src/lib/playerVolume.ts b/frontend/src/lib/playerVolume.ts new file mode 100644 index 0000000..552bc2a --- /dev/null +++ b/frontend/src/lib/playerVolume.ts @@ -0,0 +1,30 @@ +// The YouTube player's volume arithmetic, kept out of PlayerModal so it can be unit-tested. Every +// rule here was established by a bug: a wheel spin that lost notches to the iframe's lagging +// getVolume, a partial stored object that became NaN and persisted as `null`, and a "corrupt → +// default" fallback that jumped an already-quiet session to full blast. + +/** Level used when nothing trustworthy is available. */ +export const DEFAULT_VOLUME = 100; + +const clamp = (n: number) => Math.max(0, Math.min(100, Math.round(n))); + +/** + * Coerce anything that claims to be a volume into a usable 0–100 level. + * + * Only a real, finite `number` is accepted: the stored blob is JSON, so a level is a number or it + * is corrupt — `null`, `undefined`, `"80"` and `NaN` all mean "we don't know", and `Number(null)` + * being `0` (silently muting the player) is exactly the coercion to avoid. `fallback` is the last + * level known to work; it is itself validated, so a corrupt value can never travel through it. + */ +export function normalizeVolume(raw: unknown, fallback: number = DEFAULT_VOLUME): number { + if (typeof raw === "number" && Number.isFinite(raw)) return clamp(raw); + if (typeof fallback === "number" && Number.isFinite(fallback)) return clamp(fallback); + return DEFAULT_VOLUME; +} + +/** One volume step (wheel notch / arrow key) from `current`, falling back to `fallback` when the + * player answered something unusable. */ +export function stepVolume(current: unknown, delta: number, fallback: number): number { + const base = normalizeVolume(current, fallback); + return clamp(base + delta); +} From d8d82e47d0dcc0d02ec0a4a4c679ebc914eac3c2 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 24 Jul 2026 02:16:58 +0200 Subject: [PATCH 10/19] fix(player): give YouTube's own fullscreen a working path + round-6 findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reported bug: after F, the embed's own fullscreen button is dead. Root cause measured in Chrome — the browser's fullscreen lock sits on OUR wrapper, so YouTube's button would need a nested request from a cross-origin frame. Handing the lock to the iframe fixes it, but `iframe.requestFullscreen()` only resolves from a CLICK (from a key press the promise stays pending forever, and awaiting it spends the activation so a fallback is denied too). So: F keeps fullscreening our wrapper (unchanged, our overlays stay visible) and a new toolbar button hands the lock to the iframe — verified in real Chrome: document.fullscreenElement is the IFRAME at 1920x1024, i.e. a plain YouTube fullscreen where its own controls work. Round-6 review findings, all fixed: - one _rate_limiter + one _cancel_poll shared by the yt-dlp hook and run_ffmpeg (was three hand-rolled throttles; cancel latency differed 2s vs 1s) - stepVolume takes {delta, fallback} — two adjacent number args were silently transposable - volume gestures before the player is ready are honoured instead of dropped - normalizeVolume's fallback is typed unknown (its guard was unreachable) and applyVolume takes a number again - the throttle tests inject a clock instead of patching global time.monotonic --- backend/app/worker.py | 86 ++++++++++++++---------- backend/tests/test_ffmpeg_callbacks.py | 14 ++-- frontend/src/components/PlayerModal.tsx | 84 ++++++++++++++++++++--- frontend/src/i18n/locales/en/player.json | 2 + frontend/src/i18n/locales/hu/player.json | 2 + frontend/src/lib/playerVolume.test.ts | 15 +++-- frontend/src/lib/playerVolume.ts | 15 +++-- 7 files changed, 149 insertions(+), 69 deletions(-) diff --git a/backend/app/worker.py b/backend/app/worker.py index faf5086..850c764 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -273,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: @@ -290,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. @@ -319,46 +317,62 @@ def _make_progress_hook(job_id: int, asset_id: int): return hook -# ffmpeg's `-progress pipe:1` emits a block of ~12 lines every ~0.5s, and run_ffmpeg calls both of -# its callbacks once per LINE. Each of these is a database round-trip (`_job_status` opens a session -# and SELECTs; `_set_job` opens one and UPDATE+COMMITs), so an unthrottled pair costs ~10^5 queries -# on a long re-encode. Both are therefore rate-limited to ~1s — the same 1s DB throttle the -# download progress hook already applies (see `_make_progress_hook`). -_FFMPEG_CALLBACK_INTERVAL_S = 1.0 +# 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 _ffmpeg_progress_writer(job_id: int, phase: str): - """`on_progress` for run_ffmpeg: persist at most one row write per second, and only when the - whole-percent value actually changed (the bar shows integers).""" - state = {"at": 0.0, "pct": -1} +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.""" + state = {"at": 0.0} - def write(pct: float) -> None: - whole = int(pct) - now = time.monotonic() - if whole == state["pct"] or now - state["at"] < _FFMPEG_CALLBACK_INTERVAL_S: - return - state["at"] = now - state["pct"] = whole - _set_job(job_id, progress=whole, phase=phase) + def should_run() -> bool: + t = now() + if state["at"] and t - state["at"] < interval_s: + return False + state["at"] = t + return True - return write + return should_run -def _ffmpeg_cancel_poll(job_id: int): - """`should_cancel` for run_ffmpeg: ask the DB at most once a second and reuse the answer in - between. Cancellation is sticky, so once it says yes it never queries again.""" - state = {"at": 0.0, "cancelled": False} +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: - now = time.monotonic() - if not state["cancelled"] and now - state["at"] >= _FFMPEG_CALLBACK_INTERVAL_S: - state["at"] = now + 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 @@ -533,7 +547,7 @@ def _ensure_browser_playable(job_id: int, produced: Path, spec: dict, info: dict editmod.build_browser_safe_cmd(produced, dest, reenc_v, reenc_a), total_s=total_s, on_progress=_ffmpeg_progress_writer(job_id, "processing"), - should_cancel=_ffmpeg_cancel_poll(job_id), + should_cancel=_cancel_poll(job_id), ) produced.unlink(missing_ok=True) # Keep the stored asset metadata truthful about what's now on disk. @@ -712,7 +726,7 @@ def _process_edit(job_id: int, asset_id: int) -> None: cmd, total_s, on_progress=_ffmpeg_progress_writer(job_id, "editing"), - should_cancel=_ffmpeg_cancel_poll(job_id), + should_cancel=_cancel_poll(job_id), ) if not dest_staging.exists(): raise RuntimeError("ffmpeg produced no output file") diff --git a/backend/tests/test_ffmpeg_callbacks.py b/backend/tests/test_ffmpeg_callbacks.py index c2644f5..d4173b5 100644 --- a/backend/tests/test_ffmpeg_callbacks.py +++ b/backend/tests/test_ffmpeg_callbacks.py @@ -26,8 +26,7 @@ class TestProgressWriter: writes = [] clock = {"t": 1000.0} monkeypatch.setattr(worker, "_set_job", lambda job_id, **f: writes.append(f)) - monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"]) - write = worker._ffmpeg_progress_writer(7, "processing") + write = worker._ffmpeg_progress_writer(7, "processing", now=lambda: clock["t"]) write(10) write(11) # same second → dropped clock["t"] += 1.5 @@ -38,8 +37,7 @@ class TestProgressWriter: writes = [] clock = {"t": 1000.0} monkeypatch.setattr(worker, "_set_job", lambda job_id, **f: writes.append(f)) - monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"]) - write = worker._ffmpeg_progress_writer(7, "processing") + 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 @@ -50,7 +48,7 @@ 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._ffmpeg_cancel_poll(7) + cancelled = worker._cancel_poll(7) for _ in range(40): assert cancelled() is False assert len(calls) == 1 @@ -64,8 +62,7 @@ class TestCancelPoll: return "cancelled" monkeypatch.setattr(worker, "_job_status", status) - monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"]) - cancelled = worker._ffmpeg_cancel_poll(7) + 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 @@ -75,8 +72,7 @@ class TestCancelPoll: clock = {"t": 1000.0} state = {"status": "running"} monkeypatch.setattr(worker, "_job_status", lambda job_id: state["status"]) - monkeypatch.setattr(worker.time, "monotonic", lambda: clock["t"]) - cancelled = worker._ffmpeg_cancel_poll(7) + 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 diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index ab340e1..42f964b 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -11,6 +11,7 @@ import { ChevronLeft, ChevronRight, ExternalLink, + Maximize, Repeat, Repeat1, Settings, @@ -209,11 +210,49 @@ export default function PlayerModal({ p.pauseVideo?.(); // 1 === playing else p.playVideo?.(); }; + /** The element F (and our own button) fullscreens: the PLAYER IFRAME, not our stage wrapper. + * + * Fullscreening the wrapper looked equivalent — the video filled the screen either way — but it + * left the browser's fullscreen lock on an element YouTube knows nothing about, so YouTube's own + * fullscreen button had to make a NESTED request from inside a cross-origin frame while we held + * that lock. That is the user-reported bug: after F, the embed's own fullscreen control is dead. + * Targeting the iframe makes our shortcut and YouTube's button the same operation on the same + * element — i.e. exactly a plain YouTube embed, where the button demonstrably works. + * + * Trade-off, deliberate: our on-player volume flash and the "YouTube controls active" badge sit + * OUTSIDE the iframe, so they aren't painted in fullscreen — YouTube's own volume UI covers that + * case. The stage stays as the fallback for the window between mount and the player being ready. + */ + const playerIframe = (): HTMLElement | null => { + const p = playerRef.current; + const frame = p && typeof p.getIframe === "function" ? p.getIframe() : null; + return (frame as HTMLElement | null) ?? null; + }; + /** F / our stage-fullscreen: puts OUR wrapper on screen, so the volume flash and the queue + * chrome stay visible. YouTube's own fullscreen button is inert while we hold that lock — use + * `enterNativeFullscreen` (the toolbar button) when you want YouTube's own controls. */ const toggleFullscreen = () => { - const el = stageRef.current; - if (!el) return; if (document.fullscreenElement) document.exitFullscreen?.(); - else el.requestFullscreen?.(); + else stageRef.current?.requestFullscreen?.(); + }; + /** True YouTube fullscreen: hand the browser's fullscreen lock to the PLAYER IFRAME, so the + * embed behaves exactly like any YouTube embed — its own control bar, its own fullscreen + * button, its quality menu, all live. + * + * Why a button and not the F key: measured in Chrome (2026-07-24), `iframe.requestFullscreen()` + * resolves from a CLICK but stays pending forever when it comes from a key press — the request + * must be delegated to the out-of-process frame, and a keyboard gesture in the parent document + * doesn't carry that delegation. Awaiting it and falling back is not an option either: the await + * spends the transient activation, so the second request is denied and the key does nothing. + * A click is the gesture that provably works, so this is a click-only affordance. */ + const enterNativeFullscreen = () => { + const frame = playerIframe(); + if (!frame?.requestFullscreen) return; + if (document.fullscreenElement) document.exitFullscreen?.(); + frame.requestFullscreen?.().catch(() => { + // Denied (an old browser, a policy) — keep the old behaviour rather than nothing at all. + stageRef.current?.requestFullscreen?.(); + }); }; const flashVolume = (level: number) => { setVolumeUi(level); @@ -228,8 +267,9 @@ export default function PlayerModal({ }; /** 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: unknown, flash = true) => { - // An unusable level (a player that answered NaN) falls back to the LAST GOOD one, never to the + 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; @@ -246,8 +286,10 @@ export default function PlayerModal({ 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; - if (!p || typeof p.getVolume !== "function") return; // 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 @@ -256,8 +298,10 @@ export default function PlayerModal({ const now = Date.now(); const continuing = now - lastNudgeAtRef.current < 1000; lastNudgeAtRef.current = now; - const cur = continuing ? volumeRef.current : p.isMuted?.() ? 0 : p.getVolume?.(); - applyVolume(stepVolume(cur, delta, volumeRef.current), true); + 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 @@ -530,10 +574,16 @@ export default function PlayerModal({ return () => el.removeEventListener("wheel", onWheel); }, []); - // Track whether we're the fullscreen element (F, our button, or the browser's own Esc exit). + // Track whether the fullscreen element is ours — the player iframe (see fullscreenTarget) or, + // before the player exists, the stage. Covers F, our button, YouTube's own button and the + // browser's Esc exit alike, since all of them land on one of those two elements. useEffect(() => { const onChange = () => { - const ours = !!document.fullscreenElement && document.fullscreenElement === stageRef.current; + const fsEl = document.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 }; @@ -1067,10 +1117,22 @@ export default function PlayerModal({
)} + {/* True YouTube fullscreen — see enterNativeFullscreen. Deliberately a CLICK-only + affordance (the iframe fullscreen request is only reliable from a click), and the + only way to reach YouTube's own control bar + fullscreen button; F fullscreens our + wrapper instead, which keeps our overlays but leaves YouTube's controls inert. */} + {!navigated && ( )} {!navigated && ( diff --git a/frontend/src/i18n/locales/en/player.json b/frontend/src/i18n/locales/en/player.json index 3023af9..e24adc6 100644 --- a/frontend/src/i18n/locales/en/player.json +++ b/frontend/src/i18n/locales/en/player.json @@ -36,5 +36,7 @@ "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 or ↑ ↓: volume · F: fullscreen", + "nativeFullscreen": "YouTube fullscreen", + "nativeFullscreenHint": "YouTube fullscreen — the player's own controls (quality, subtitles, its fullscreen button). F fills the screen with our player instead.", "nativeControls": "YouTube controls active" } diff --git a/frontend/src/i18n/locales/hu/player.json b/frontend/src/i18n/locales/hu/player.json index c2b4b36..11e32e1 100644 --- a/frontend/src/i18n/locales/hu/player.json +++ b/frontend/src/i18n/locales/hu/player.json @@ -36,5 +36,7 @@ "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ő vagy ↑ ↓: hangerő · F: teljes képernyő", + "nativeFullscreen": "YouTube teljes képernyő", + "nativeFullscreenHint": "YouTube teljes képernyő — a lejátszó saját vezérlőivel (minőség, felirat, saját teljes képernyő gomb). Az F a mi lejátszónkkal tölti ki a képernyőt.", "nativeControls": "YouTube vezérlők aktívak" } diff --git a/frontend/src/lib/playerVolume.test.ts b/frontend/src/lib/playerVolume.test.ts index 0915eb9..606fa73 100644 --- a/frontend/src/lib/playerVolume.test.ts +++ b/frontend/src/lib/playerVolume.test.ts @@ -34,24 +34,25 @@ describe("normalizeVolume", () => { it("uses the default when the fallback is unusable too", () => { expect(normalizeVolume(NaN, NaN)).toBe(DEFAULT_VOLUME); - expect(normalizeVolume(null, undefined as unknown as number)).toBe(DEFAULT_VOLUME); + expect(normalizeVolume(null, undefined)).toBe(DEFAULT_VOLUME); + expect(normalizeVolume(null, "80")).toBe(DEFAULT_VOLUME); }); }); describe("stepVolume", () => { it("steps up and down from the current level", () => { - expect(stepVolume(60, 5, 100)).toBe(65); - expect(stepVolume(60, -5, 100)).toBe(55); + expect(stepVolume(60, { delta: 5, fallback: 100 })).toBe(65); + expect(stepVolume(60, { delta: -5, fallback: 100 })).toBe(55); }); it("clamps at both ends instead of wrapping", () => { - expect(stepVolume(98, 5, 100)).toBe(100); - expect(stepVolume(3, -5, 100)).toBe(0); + expect(stepVolume(98, { delta: 5, fallback: 100 })).toBe(100); + expect(stepVolume(3, { delta: -5, fallback: 100 })).toBe(0); }); it("steps from the fallback when the player answers garbage", () => { // A spin must not lose notches to a stale/unusable reading — it continues from what we last set. - expect(stepVolume(undefined, -5, 90)).toBe(85); - expect(stepVolume(NaN, -5, 90)).toBe(85); + expect(stepVolume(undefined, { delta: -5, fallback: 90 })).toBe(85); + expect(stepVolume(NaN, { delta: -5, fallback: 90 })).toBe(85); }); }); diff --git a/frontend/src/lib/playerVolume.ts b/frontend/src/lib/playerVolume.ts index 552bc2a..d9f7498 100644 --- a/frontend/src/lib/playerVolume.ts +++ b/frontend/src/lib/playerVolume.ts @@ -16,15 +16,18 @@ const clamp = (n: number) => Math.max(0, Math.min(100, Math.round(n))); * being `0` (silently muting the player) is exactly the coercion to avoid. `fallback` is the last * level known to work; it is itself validated, so a corrupt value can never travel through it. */ -export function normalizeVolume(raw: unknown, fallback: number = DEFAULT_VOLUME): number { +export function normalizeVolume(raw: unknown, fallback: unknown = DEFAULT_VOLUME): number { if (typeof raw === "number" && Number.isFinite(raw)) return clamp(raw); if (typeof fallback === "number" && Number.isFinite(fallback)) return clamp(fallback); return DEFAULT_VOLUME; } -/** One volume step (wheel notch / arrow key) from `current`, falling back to `fallback` when the - * player answered something unusable. */ -export function stepVolume(current: unknown, delta: number, fallback: number): number { - const base = normalizeVolume(current, fallback); - return clamp(base + delta); +/** One volume step (wheel notch / arrow key) from `current`. + * + * `delta` and `fallback` ride in an options object on purpose: as two adjacent `number` + * parameters they were transposable without a type error, and swapping them silently steps the + * volume BY the current level. */ +export function stepVolume(current: unknown, opts: { delta: number; fallback: unknown }): number { + const base = normalizeVolume(current, opts.fallback); + return clamp(base + opts.delta); } From 45654fc7ecb603caf9d9d3a72a6fb1576e0269ac Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 24 Jul 2026 02:46:42 +0200 Subject: [PATCH 11/19] fix(player): F maximises without the Fullscreen API, so YouTube's own controls work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause, measured in Chrome/Brave/Firefox alike (so: 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 on the iframe element — the YouTube player never learns it is fullscreen, so its own fullscreen button stays in the "enter" state and clicking it changes nothing visible. Every other YouTube control works, which is exactly what the user saw. So we stop taking the lock: F now toggles a CSS layer (`player-stage--max`) that fills the viewport. The page stays non-fullscreen, so the embed's own fullscreen button behaves natively, and our own overlays and shortcuts keep working because our DOM is what's on screen. Escape steps out of maximise before closing. Verified end to end in real Chrome: - F -> stage 1920x889, document.fullscreenElement null (no lock taken) - YouTube's own fullscreen button -> real fullscreen (innerHeight 889 -> 1024, fullscreenElement becomes the IFRAME) and its icon flips to "exit" - clicking it again -> back to the maximised view, our class still applied - wheel volume while maximised: 100 -> 85, with the level flash visible Drops the "YouTube fullscreen" toolbar button added earlier today: with the lock gone, YouTube's own button does that job correctly. --- frontend/src/components/PlayerModal.tsx | 85 ++++++++---------------- frontend/src/i18n/locales/en/player.json | 2 - frontend/src/i18n/locales/hu/player.json | 2 - frontend/src/index.css | 16 ++++- 4 files changed, 41 insertions(+), 64 deletions(-) diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 42f964b..a3d4025 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -11,7 +11,6 @@ import { ChevronLeft, ChevronRight, ExternalLink, - Maximize, Repeat, Repeat1, Settings, @@ -178,6 +177,10 @@ export default function PlayerModal({ // 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); @@ -210,50 +213,22 @@ export default function PlayerModal({ p.pauseVideo?.(); // 1 === playing else p.playVideo?.(); }; - /** The element F (and our own button) fullscreens: the PLAYER IFRAME, not our stage wrapper. + /** F = maximise, deliberately WITHOUT the Fullscreen API. * - * Fullscreening the wrapper looked equivalent — the video filled the screen either way — but it - * left the browser's fullscreen lock on an element YouTube knows nothing about, so YouTube's own - * fullscreen button had to make a NESTED request from inside a cross-origin frame while we held - * that lock. That is the user-reported bug: after F, the embed's own fullscreen control is dead. - * Targeting the iframe makes our shortcut and YouTube's button the same operation on the same - * element — i.e. exactly a plain YouTube embed, where the button demonstrably works. + * 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. * - * Trade-off, deliberate: our on-player volume flash and the "YouTube controls active" badge sit - * OUTSIDE the iframe, so they aren't painted in fullscreen — YouTube's own volume UI covers that - * case. The stage stays as the fallback for the window between mount and the player being ready. + * 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 playerIframe = (): HTMLElement | null => { - const p = playerRef.current; - const frame = p && typeof p.getIframe === "function" ? p.getIframe() : null; - return (frame as HTMLElement | null) ?? null; - }; - /** F / our stage-fullscreen: puts OUR wrapper on screen, so the volume flash and the queue - * chrome stay visible. YouTube's own fullscreen button is inert while we hold that lock — use - * `enterNativeFullscreen` (the toolbar button) when you want YouTube's own controls. */ - const toggleFullscreen = () => { - if (document.fullscreenElement) document.exitFullscreen?.(); - else stageRef.current?.requestFullscreen?.(); - }; - /** True YouTube fullscreen: hand the browser's fullscreen lock to the PLAYER IFRAME, so the - * embed behaves exactly like any YouTube embed — its own control bar, its own fullscreen - * button, its quality menu, all live. - * - * Why a button and not the F key: measured in Chrome (2026-07-24), `iframe.requestFullscreen()` - * resolves from a CLICK but stays pending forever when it comes from a key press — the request - * must be delegated to the out-of-process frame, and a keyboard gesture in the parent document - * doesn't carry that delegation. Awaiting it and falling back is not an option either: the await - * spends the transient activation, so the second request is denied and the key does nothing. - * A click is the gesture that provably works, so this is a click-only affordance. */ - const enterNativeFullscreen = () => { - const frame = playerIframe(); - if (!frame?.requestFullscreen) return; - if (document.fullscreenElement) document.exitFullscreen?.(); - frame.requestFullscreen?.().catch(() => { - // Denied (an old browser, a policy) — keep the old behaviour rather than nothing at all. - stageRef.current?.requestFullscreen?.(); - }); - }; + const toggleFullscreen = () => setMaximized((m) => !m); const flashVolume = (level: number) => { setVolumeUi(level); window.clearTimeout(volTimerRef.current); @@ -494,6 +469,12 @@ export default function PlayerModal({ if (e.key === "Escape") { // In fullscreen, let the browser's Esc exit fullscreen only — don't also close the modal. if (document.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; @@ -809,7 +790,9 @@ export default function PlayerModal({ >
{/* Hide the iframe entirely on error so YouTube's own error screen can't bleed through our overlay. */} @@ -832,7 +815,7 @@ export default function PlayerModal({ title={t("player.shortcutsHint")} aria-label={t("player.shortcutsHint")} className={`absolute inset-x-0 top-[12%] bottom-[22%] z-base cursor-pointer ${ - nativeControls || isFullscreen ? "pointer-events-none" : "" + nativeControls || isFullscreen || maximized ? "pointer-events-none" : "" }`} /> )} @@ -1117,22 +1100,10 @@ export default function PlayerModal({
)} - {/* True YouTube fullscreen — see enterNativeFullscreen. Deliberately a CLICK-only - affordance (the iframe fullscreen request is only reliable from a click), and the - only way to reach YouTube's own control bar + fullscreen button; F fullscreens our - wrapper instead, which keeps our overlays but leaves YouTube's controls inert. */} - {!navigated && ( )} {!navigated && ( diff --git a/frontend/src/i18n/locales/en/player.json b/frontend/src/i18n/locales/en/player.json index e24adc6..3023af9 100644 --- a/frontend/src/i18n/locales/en/player.json +++ b/frontend/src/i18n/locales/en/player.json @@ -36,7 +36,5 @@ "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 or ↑ ↓: volume · F: fullscreen", - "nativeFullscreen": "YouTube fullscreen", - "nativeFullscreenHint": "YouTube fullscreen — the player's own controls (quality, subtitles, its fullscreen button). F fills the screen with our player instead.", "nativeControls": "YouTube controls active" } diff --git a/frontend/src/i18n/locales/hu/player.json b/frontend/src/i18n/locales/hu/player.json index 11e32e1..c2b4b36 100644 --- a/frontend/src/i18n/locales/hu/player.json +++ b/frontend/src/i18n/locales/hu/player.json @@ -36,7 +36,5 @@ "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ő vagy ↑ ↓: hangerő · F: teljes képernyő", - "nativeFullscreen": "YouTube teljes képernyő", - "nativeFullscreenHint": "YouTube teljes képernyő — a lejátszó saját vezérlőivel (minőség, felirat, saját teljes képernyő gomb). Az F a mi lejátszónkkal tölti ki a képernyőt.", "nativeControls": "YouTube vezérlők aktívak" } diff --git a/frontend/src/index.css b/frontend/src/index.css index 75f3062..5cf0599 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -579,15 +579,25 @@ 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). */ -.player-stage:fullscreen { +/* 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 { width: 100vw; height: 100vh; aspect-ratio: auto; border-radius: 0; } +.player-stage--max { + position: fixed; + inset: 0; + z-index: 9999; /* the tuner level — above the modal chrome it is nested in */ +} + /* Thin, theme-aware scrollbars */ * { scrollbar-color: var(--border) transparent; From aae37591cb51605ac0dbe4090e5b398fa31a31a6 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 24 Jul 2026 03:07:46 +0200 Subject: [PATCH 12/19] fix(player): reclaim keyboard focus when fullscreen ends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UAT found a dead end: F -> YouTube's fullscreen button -> Esc left the player maximised with no way back, and a further F produced a second, control-less fullscreen with the wheel driving YouTube's own volume box. Measured cause: reaching YouTube's control bar means clicking INTO the player, which moves keyboard focus into the cross-origin iframe — and it stays there. Our window-level shortcuts then never fire (document.activeElement === IFRAME), so F went to YouTube instead of toggling our maximise. Fix: on fullscreenchange, when fullscreen ends, focus the modal card again. Verified in real Chrome, step by step: - F -> maximised, no browser lock, focus on our card - YouTube's own FS button -> real fullscreen (innerHeight 889 -> 1024) - leaving fullscreen -> focus back on our card (was: still the iframe) - F -> back to the normal modal view (was: a second fullscreen with no controls) --- frontend/src/components/PlayerModal.tsx | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index a3d4025..91ca719 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -555,9 +555,8 @@ export default function PlayerModal({ return () => el.removeEventListener("wheel", onWheel); }, []); - // Track whether the fullscreen element is ours — the player iframe (see fullscreenTarget) or, - // before the player exists, the stage. Covers F, our button, YouTube's own button and the - // browser's Esc exit alike, since all of them land on one of those two elements. + // 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 = document.fullscreenElement; @@ -567,6 +566,12 @@ export default function PlayerModal({ 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(); }; document.addEventListener("fullscreenchange", onChange); return () => document.removeEventListener("fullscreenchange", onChange); From a5336d20e002edf3fd979cd9958273f3381c8a09 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 24 Jul 2026 03:41:29 +0200 Subject: [PATCH 13/19] =?UTF-8?q?fix(r5):=20address=20the=20seventh=20/cod?= =?UTF-8?q?e-review=20high=20round=20=E2=80=94=207=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five of them sit where round 6 turned F from a Fullscreen API call into a CSS maximise: the guards around it still asked `isFullscreen`, which that pivot made permanently false. - Arrow-key volume was dead in the maximised view: the scroll-deference checked only `isFullscreenRef`, so the (now hidden but still scrollable) card kept swallowing Up/Down. Both flags now count as "the video fills the screen". Measured: 87px of scroll room on the focused card, ArrowDown moves 80 -> 75 and scrolls nothing. - The maximised view had no way out once the pointer touched the video. Reaching YouTube's controls means clicking into the cross-origin iframe, which takes keyboard focus with it, and the re-arm paths (stage mouseleave, window focus) need the pointer to leave the browser entirely while the stage fills it. So the exit now lives on screen, in our DOM: a button that un-maximises AND pulls focus back, re-arming every shortcut. - WebKit fires only `webkitfullscreenchange`/`webkitFullscreenElement`, so on Safari/iPadOS YouTube's own fullscreen never registered — no overlay yield, no focus reclaim. One `fullscreenEl()` reader, both event spellings. - The stage claimed `z-index: 9999`, the glass tuner's reserved level, for a job that only needs to beat the modal chrome it is nested in (its siblings top out at z-menu). Dropped to rail-level; nothing escapes the modal's stacking context either way. - `inset: 0` already sizes a fixed layer — the inherited `100vw`/`100vh` over-constrained it, which is how a classic scrollbar gets a horizontal overflow and a mobile URL bar hides the bottom edge (YouTube's control bar). - The shortcut hint still promised "F: fullscreen" in both locales. Backend, both "the fix stopped one layer short": - The startup HLS sweep was awaited inside lifespan, so uvicorn still served nothing until it finished — a thread doesn't help when nothing is listening yet. Fire-and-forget task instead, cancelled on shutdown. Measured with 200 planted segments: "Application startup complete" now precedes "swept 1". - An RSS poll where every channel failed returned normally, so the scheduler recorded `ok` with the outage buried in the summary string. `failed == total` now raises: a run that reached nothing is a failed run. Suites: backend 147 -> 150. --- backend/app/main.py | 28 ++++++++--- backend/app/routes/sync.py | 5 +- backend/app/sync/runner.py | 12 ++++- backend/tests/test_sync_guards.py | 59 +++++++++++++++++++----- frontend/src/components/PlayerModal.tsx | 57 ++++++++++++++++++++--- frontend/src/i18n/locales/en/player.json | 3 +- frontend/src/i18n/locales/hu/player.json | 3 +- frontend/src/index.css | 19 ++++++-- 8 files changed, 155 insertions(+), 31 deletions(-) diff --git a/backend/app/main.py b/backend/app/main.py index e419b3a..184f06e 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -70,20 +70,36 @@ async def lifespan(app: FastAPI): 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. Off the event loop: the - # first sweep after a long uptime can be gigabytes of `.ts` files, and blocking here delays - # serving (a 502 + a failed post-deploy health probe). - swept = await asyncio.to_thread(plex_stream.sweep_orphans) - if swept: - log.info("swept %d orphaned HLS segment directories", swept) + # 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") + sweep.cancel() 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/routes/sync.py b/backend/app/routes/sync.py index 7f196a1..41e0fef 100644 --- a/backend/app/routes/sync.py +++ b/backend/app/routes/sync.py @@ -52,9 +52,10 @@ def sync_subscriptions( def sync_rss( user: User = Depends(require_human), db: Session = Depends(get_db) ) -> dict: + # 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)) - # `failed` = feeds that could not be read at all (see run_rss_poll); surfaced so a manual poll - # reports an outage instead of an innocent-looking "0 new". return {"new_videos": result["new"], "failed_feeds": result["failed"]} diff --git a/backend/app/sync/runner.py b/backend/app/sync/runner.py index f4f1109..cf37c9a 100644 --- a/backend/app/sync/runner.py +++ b/backend/app/sync/runner.py @@ -56,7 +56,11 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str `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.""" + 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.""" if channels is None: channels = db.execute(select(Channel)).scalars().all() total = len(channels) @@ -99,6 +103,12 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str progress.report(done, total, "rss_poll") 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 count + # in the message keeps the summary just as informative. + raise RuntimeError(f"RSS poll failed for all {total} channels") return {"new": new, "failed": failed} diff --git a/backend/tests/test_sync_guards.py b/backend/tests/test_sync_guards.py index a658ee4..8f3a78b 100644 --- a/backend/tests/test_sync_guards.py +++ b/backend/tests/test_sync_guards.py @@ -106,34 +106,71 @@ class TestRssPollResult: 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_): + 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("UC1"), _StubChannel("UC2")]) + 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): - def boom(cid): - raise rss.RssError("404") + # 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=boom, apply_=lambda db, ch, e: 1) - assert out == {"new": 0, "failed": 2} + 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 read-only database: every feed reads fine, nothing can be stored. - def boom(db, ch, entries): - raise RuntimeError("read-only transaction") + # 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_=boom) - assert out == {"new": 0, "failed": 2}, "a run where nothing was stored must not read as ok" + 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_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="all 2 channels"): + 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 diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 91ca719..d2a67a7 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -11,6 +11,7 @@ import { ChevronLeft, ChevronRight, ExternalLink, + Minimize2, Repeat, Repeat1, Settings, @@ -56,6 +57,18 @@ type LoopMode = "off" | "one" | "all"; const AUTO_MODES: AutoMode[] = ["off", "next", "prev", "random"]; const LOOP_MODES: LoopMode[] = ["off", "one", "all"]; +/** The element currently holding the browser's fullscreen lock, WebKit included. + * + * Safari/iPadOS still ship only the prefixed API: no `fullscreenchange`, no `fullscreenElement`. + * We never request fullscreen ourselves any more (F maximises via CSS), but YouTube's own button + * inside the embed does — and on WebKit that arrives as `webkitfullscreenchange`, so an unprefixed + * -only reader thinks the page never entered fullscreen and skips both the overlay yield and the + * focus reclaim. */ +function fullscreenEl(): Element | null { + const d = document as Document & { webkitFullscreenElement?: Element | null }; + return d.fullscreenElement ?? d.webkitFullscreenElement ?? null; +} + /** 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 @@ -468,7 +481,7 @@ 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 (fullscreenEl()) return; // Same for our own maximise: Esc steps back out of it before it closes the player. if (maximizedRef.current) { e.preventDefault(); @@ -500,10 +513,14 @@ export default function PlayerModal({ // 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 in fullscreen though: the card is off-screen there, so deferring - // would scroll something invisible and leave fullscreen with no volume control at all. + // 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"; - if (typing || (!isFullscreenRef.current && canScrollBy(el, up ? -1 : 1))) return; + 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") { @@ -559,7 +576,7 @@ export default function PlayerModal({ // this is YouTube's own fullscreen — entered from its control bar, left by its button or Esc. useEffect(() => { const onChange = () => { - const fsEl = document.fullscreenElement; + const fsEl = fullscreenEl(); // 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; @@ -573,8 +590,14 @@ export default function PlayerModal({ // way out. Taking focus back here restores F/Space/arrows the moment fullscreen ends. if (!fsEl) focusModal(); }; + // Both spellings: WebKit (Safari/iPadOS) fires only the prefixed one, and a browser that + // supports both never fires both for the same transition. document.addEventListener("fullscreenchange", onChange); - return () => document.removeEventListener("fullscreenchange", onChange); + document.addEventListener("webkitfullscreenchange", onChange); + return () => { + document.removeEventListener("fullscreenchange", onChange); + document.removeEventListener("webkitfullscreenchange", onChange); + }; }, []); // Yield the interaction overlay to YouTube's native UI. Clicking a native control (gear / seek @@ -824,6 +847,28 @@ export default function PlayerModal({ }`} /> )} + {/* The only way OUT of the maximised view that survives a click on the video. Reaching + YouTube's controls means clicking into the cross-origin iframe, which takes keyboard + focus with it — our window-level F/Esc then never fire, and while the stage fills the + viewport the re-arm paths (stage mouseleave, window focus) need the pointer to leave + the browser entirely. So the exit lives on screen, in OUR DOM: clicking it both + un-maximises and pulls focus back, which re-arms every shortcut. */} + {maximized && ( + + )} {/* Discreet badge while YouTube's own controls have taken over (overlay yielded). */} {playerError == null && nativeControls && (
diff --git a/frontend/src/i18n/locales/en/player.json b/frontend/src/i18n/locales/en/player.json index 3023af9..dd35c01 100644 --- a/frontend/src/i18n/locales/en/player.json +++ b/frontend/src/i18n/locales/en/player.json @@ -35,6 +35,7 @@ "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 or ↑ ↓: volume · F: fullscreen", + "shortcutsHint": "Click: play/pause · Scroll or ↑ ↓: volume · F: fill the window", + "exitMaximize": "Exit full window (Esc)", "nativeControls": "YouTube controls active" } diff --git a/frontend/src/i18n/locales/hu/player.json b/frontend/src/i18n/locales/hu/player.json index c2b4b36..1b5b0c5 100644 --- a/frontend/src/i18n/locales/hu/player.json +++ b/frontend/src/i18n/locales/hu/player.json @@ -35,6 +35,7 @@ "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ő vagy ↑ ↓: hangerő · F: teljes képernyő", + "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" } diff --git a/frontend/src/index.css b/frontend/src/index.css index 5cf0599..3811408 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -586,16 +586,29 @@ html[data-scheme="matrix"][data-theme="light"] { fullscreen button stops working (see PlayerModal.toggleFullscreen). */ .player-stage:fullscreen, .player-stage--max { - width: 100vw; - height: 100vh; 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; +} + +/* `inset: 0` alone does the sizing here — adding width/height would over-constrain the box (left + + width win, right/bottom are ignored), which is how `100vw` sneaks a horizontal overflow in next + to a classic scrollbar and `100vh` pushes the bottom edge — YouTube's own control bar — under a + mobile browser's chrome. */ .player-stage--max { position: fixed; inset: 0; - z-index: 9999; /* the tuner level — above the modal chrome it is nested in */ + /* 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 */ From bdf7c03b59095d7fb0936cfbad8bc886d3253c55 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 24 Jul 2026 04:44:20 +0200 Subject: [PATCH 14/19] =?UTF-8?q?fix(r5):=20address=20the=20eighth=20/code?= =?UTF-8?q?-review=20high=20round=20=E2=80=94=207=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worst one was, again, the previous round's own fix. - Round 7 stopped awaiting the HLS sweep so startup could serve immediately — which also made it run CONCURRENTLY with playback, and `sweep_orphans` snapshotted the live-session set BEFORE listing the root and rmtree'd outside the lock. Since `start_session` reuses a directory path for the same (key, offset, tag), a stale name can become a live session at any moment. Two guards now: nothing whose mtime is newer than this process's start is ever deleted, and the live check + rmtree happen together under the lock, one directory at a time. - The sweep task is awaited after cancel, so shutdown can't leave it pending. - `_rate_limiter` used a truthiness test on its timestamp, so a `now()` of exactly 0.0 read as "never ran" — unreachable in production, but `now` exists to be injected and a test clock starting at 0 is the obvious choice. - The RSS total-outage error now carries the first underlying error: "all N channels failed" is also what ONE dead feed id looks like on a small instance, and a red card the user can't act on is its own kind of dishonest. - The WebKit fullscreen handling moved into `lib/fullscreen` and PlexPlayer uses it too — it had the same unprefixed-only reads in three places, including an Escape branch that would have closed the whole player instead of leaving fullscreen. - The `--max` CSS comment claimed `inset: 0` did the sizing; Tailwind's `w-full` is still on the element and wins. Comment now states the real mechanism. Verified in real Chrome (the pane can't composite or do native fullscreen): - F fills the 1920x889 viewport; the exit pill renders top-right on the letterbox band and overlaps NO YouTube control — the collision this round suspected does not reproduce in this embed (its own buttons sit bottom-left / bottom-right). - Four real ArrowDown presses while maximised: persisted volume 100 -> 80, with the level flash visible on screen. - A real click on the exit pill un-maximises and returns focus to the card. - lib/fullscreen driven from a real click: enter (innerHeight 889 -> 1024), the change listener fires exactly once per transition, exit returns to 889. Backend suite 150 -> 158; the new sweep tests are mutation-checked, and they read their cutoff from the filesystem rather than a clock (the container VM and the bind-mounted host disagree by enough to make a clock-based assertion flaky). --- backend/app/main.py | 8 +- backend/app/plex/stream.py | 37 +++++++-- backend/app/sync/runner.py | 24 ++++-- backend/app/worker.py | 10 ++- backend/tests/test_plex_stream.py | 104 +++++++++++++++++++++++- backend/tests/test_sync_guards.py | 12 ++- frontend/src/components/PlayerModal.tsx | 26 +----- frontend/src/components/PlexPlayer.tsx | 22 +++-- frontend/src/index.css | 11 ++- frontend/src/lib/fullscreen.ts | 49 +++++++++++ 10 files changed, 251 insertions(+), 52 deletions(-) create mode 100644 frontend/src/lib/fullscreen.ts diff --git a/backend/app/main.py b/backend/app/main.py index 184f06e..85a6725 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1,7 +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 @@ -82,7 +82,13 @@ async def lifespan(app: FastAPI): 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() diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index d55e243..98c0757 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -47,6 +47,10 @@ _SESSION_IDLE_S = 600 # reap a session with no access for this long # 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() _lock = threading.Lock() _sessions: dict[str, "HlsSession"] = {} @@ -268,7 +272,7 @@ def wait_for(path: Path, timeout: float = 20.0) -> bool: return path.exists() -def sweep_orphans() -> int: +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 @@ -278,18 +282,39 @@ def sweep_orphans() -> int: 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.""" - with _lock: - live = {s.dir for s in _sessions.values()} + 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) — 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 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 p.is_dir() and p not in live and _SESSION_DIR_RE.match(p.name): + 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 + dropped += 1 return dropped diff --git a/backend/app/sync/runner.py b/backend/app/sync/runner.py index cf37c9a..dca3564 100644 --- a/backend/app/sync/runner.py +++ b/backend/app/sync/runner.py @@ -60,13 +60,19 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str 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.""" + — 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. @@ -86,17 +92,20 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str except RssError as exc: log.warning("RSS poll skipped for channel %s: %s", channel.id, exc) failed += 1 - except Exception: + first_error = first_error or f"{channel.id}: {exc}" + except Exception as exc: log.exception("RSS fetch 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: + 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 @@ -106,9 +115,12 @@ def run_rss_poll(db: Session, channels: list[Channel] | None = None) -> dict[str 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 count - # in the message keeps the summary just as informative. - raise RuntimeError(f"RSS poll failed for all {total} channels") + # 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} diff --git a/backend/app/worker.py b/backend/app/worker.py index 850c764..97819ec 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -330,11 +330,17 @@ _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.""" - state = {"at": 0.0} + # `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() - if state["at"] and t - state["at"] < interval_s: + last = state["at"] + if last is not None and t - last < interval_s: return False state["at"] = t return True diff --git a/backend/tests/test_plex_stream.py b/backend/tests/test_plex_stream.py index fcf12d8..cb10b95 100644 --- a/backend/tests/test_plex_stream.py +++ b/backend/tests/test_plex_stream.py @@ -1,10 +1,16 @@ -"""The guard in front of a recursive delete. +"""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.""" +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 @@ -42,3 +48,97 @@ def test_our_session_directories_are_swept(name): ) 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, "_PROCESS_START", d.stat().st_mtime + 1) + assert stream.sweep_orphans() == 1 # "older than boot" → swept + assert not 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_sync_guards.py b/backend/tests/test_sync_guards.py index 8f3a78b..754bf73 100644 --- a/backend/tests/test_sync_guards.py +++ b/backend/tests/test_sync_guards.py @@ -156,12 +156,22 @@ class TestRssPollResult: 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="all 2 channels"): + 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): diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index d2a67a7..0edb592 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -35,6 +35,7 @@ import { relativeTime, } from "../lib/format"; import { renderDescription } from "../lib/descriptionLinks"; +import { fullscreenElement, onFullscreenChange } from "../lib/fullscreen"; import { useBackToClose } from "../lib/history"; import { DEFAULT_VOLUME, normalizeVolume, stepVolume } from "../lib/playerVolume"; import { LS, readAccountMerged, writeAccount } from "../lib/storage"; @@ -57,18 +58,6 @@ type LoopMode = "off" | "one" | "all"; const AUTO_MODES: AutoMode[] = ["off", "next", "prev", "random"]; const LOOP_MODES: LoopMode[] = ["off", "one", "all"]; -/** The element currently holding the browser's fullscreen lock, WebKit included. - * - * Safari/iPadOS still ship only the prefixed API: no `fullscreenchange`, no `fullscreenElement`. - * We never request fullscreen ourselves any more (F maximises via CSS), but YouTube's own button - * inside the embed does — and on WebKit that arrives as `webkitfullscreenchange`, so an unprefixed - * -only reader thinks the page never entered fullscreen and skips both the overlay yield and the - * focus reclaim. */ -function fullscreenEl(): Element | null { - const d = document as Document & { webkitFullscreenElement?: Element | null }; - return d.fullscreenElement ?? d.webkitFullscreenElement ?? null; -} - /** 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 @@ -481,7 +470,7 @@ 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 (fullscreenEl()) 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(); @@ -576,7 +565,7 @@ export default function PlayerModal({ // this is YouTube's own fullscreen — entered from its control bar, left by its button or Esc. useEffect(() => { const onChange = () => { - const fsEl = fullscreenEl(); + 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; @@ -590,14 +579,7 @@ export default function PlayerModal({ // way out. Taking focus back here restores F/Space/arrows the moment fullscreen ends. if (!fsEl) focusModal(); }; - // Both spellings: WebKit (Safari/iPadOS) fires only the prefixed one, and a browser that - // supports both never fires both for the same transition. - document.addEventListener("fullscreenchange", onChange); - document.addEventListener("webkitfullscreenchange", onChange); - return () => { - document.removeEventListener("fullscreenchange", onChange); - document.removeEventListener("webkitfullscreenchange", onChange); - }; + return onFullscreenChange(onChange); // both spellings — see lib/fullscreen }, []); // Yield the interaction overlay to YouTube's native UI. Clicking a native control (gear / seek 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/index.css b/frontend/src/index.css index 3811408..f274e8d 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -597,10 +597,13 @@ html[data-scheme="matrix"][data-theme="light"] { height: 100vh; } -/* `inset: 0` alone does the sizing here — adding width/height would over-constrain the box (left + - width win, right/bottom are ignored), which is how `100vw` sneaks a horizontal overflow in next - to a classic scrollbar and `100vh` pushes the bottom edge — YouTube's own control bar — under a - mobile browser's chrome. */ +/* 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; 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