fix(r5): address the /code-review high round — 10 findings

- 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
This commit is contained in:
2026-07-23 23:25:06 +02:00
parent 5130584d0a
commit 2a7f49c981
9 changed files with 307 additions and 79 deletions
+6 -5
View File
@@ -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: def _concat_quote(path: Path) -> str:
"""Escape a path for a single-quoted `file '…'` line of an ffmpeg concat list. """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 ffmpeg's quoting rule: inside single quotes EVERY character is literal except `'` itself, which
quote, reopen) and a `\\` must be doubled. On-disk names have been ASCII-slugged since 0.51.0, is written `'\\''` (close the quote, emit an escaped quote, reopen). Backslashes are literal
but that change is FORWARD-ONLY — a pre-0.51.0 file still on disk can be named `Don't_….mp4`, there — doubling them would corrupt a path that contains one. On-disk names have been
and an unescaped apostrophe there makes the whole list unparseable.""" ASCII-slugged since 0.51.0, but that change is FORWARD-ONLY: a pre-0.51.0 file still on disk can
return path.as_posix().replace("\\", "\\\\").replace("'", "'\\''") 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): def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: Path):
+5 -2
View File
@@ -1,3 +1,4 @@
import asyncio
import logging import logging
import sys import sys
from contextlib import asynccontextmanager from contextlib import asynccontextmanager
@@ -69,8 +70,10 @@ async def lifespan(app: FastAPI):
url, url,
) )
# No HLS session survives a restart (they live in this process), so anything still on disk is # 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. # a crashed run's segments — sweep them or they accumulate forever. Off the event loop: the
swept = plex_stream.sweep_orphans() # 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: if swept:
log.info("swept %d orphaned HLS segment directories", swept) log.info("swept %d orphaned HLS segment directories", swept)
start_scheduler() start_scheduler()
+15 -5
View File
@@ -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. CPU-only prod host; only P3's full transcode is CPU-heavy.
""" """
import logging import logging
import re
import shutil import shutil
import subprocess import subprocess
import threading 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" _HLS_ROOT = Path(settings.plex_hls_dir) if settings.plex_hls_dir else Path(settings.download_root) / ".plex-hls"
_SEG_SECONDS = 6 _SEG_SECONDS = 6
_SESSION_IDLE_S = 600 # reap a session with no access for this long _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() _lock = threading.Lock()
_sessions: dict[str, "HlsSession"] = {} _sessions: dict[str, "HlsSession"] = {}
@@ -255,12 +261,16 @@ def wait_for(path: Path, timeout: float = 20.0) -> bool:
def sweep_orphans() -> int: 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 Sessions live only in THIS process, so at startup every session directory under `_HLS_ROOT` is
leftover from a crashed or killed run — and nothing else ever reaps them (the download GC 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 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: with _lock:
live = {s.dir for s in _sessions.values()} live = {s.dir for s in _sessions.values()}
dropped = 0 dropped = 0
@@ -269,7 +279,7 @@ def sweep_orphans() -> int:
except OSError: # the root doesn't exist yet — nothing to sweep except OSError: # the root doesn't exist yet — nothing to sweep
return 0 return 0
for p in entries: 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) shutil.rmtree(p, ignore_errors=True)
dropped += 1 dropped += 1
return dropped return dropped
+47 -38
View File
@@ -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.""" ingested (with a stub channel) so the mirror is faithful."""
import hashlib import hashlib
import logging import logging
from collections.abc import Callable
from datetime import datetime, timezone from datetime import datetime, timezone
from sqlalchemy import delete, select 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 _MAX_CONSECUTIVE_FAILURES = 5
#: Returned by `_Breaker.run` when the write did not happen (refused or failed).
FAILED = object()
class _Breaker: 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: def __init__(self, limit: int = _MAX_CONSECUTIVE_FAILURES) -> None:
self.limit = limit self.limit = limit
@@ -296,6 +305,22 @@ class _Breaker:
self.limit, 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: 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 """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 created = 1
# Brand-new playlist: just append every video in order. # Brand-new playlist: just append every video in order.
for vid in desired: for vid in desired:
if not breaker.open(): if breaker.run(
failures.append(vid) lambda v=vid: yt.add_playlist_item(pl.yt_playlist_id, v),
continue lambda v=vid: failures.append(v),
try: ) is not FAILED:
yt.add_playlist_item(pl.yt_playlist_id, vid)
inserted += 1 inserted += 1
breaker.hit(True)
except YouTubeError:
failures.append(vid)
breaker.hit(False)
if not failures: if not failures:
pl.synced_fingerprint = fingerprint(pl.name, desired) pl.synced_fingerprint = fingerprint(pl.name, desired)
pl.dirty = False pl.dirty = False
@@ -354,30 +374,22 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict:
# Delete extras (local is authoritative). # Delete extras (local is authoritative).
for it in current: for it in current:
if it["video_id"] not in desired_set: if it["video_id"] not in desired_set:
if not breaker.open(): if breaker.run(
failures.append(it["video_id"]) lambda i=it: yt.delete_playlist_item(i["item_id"]),
continue lambda i=it: failures.append(i["video_id"]),
try: ) is not FAILED:
yt.delete_playlist_item(it["item_id"])
deleted += 1 deleted += 1
breaker.hit(True)
except YouTubeError:
failures.append(it["video_id"])
breaker.hit(False)
# Insert missing (append; the reorder pass below fixes positions). # Insert missing (append; the reorder pass below fixes positions).
cur_set = set(cur_vids) cur_set = set(cur_vids)
for vid in desired: for vid in desired:
if vid not in cur_set: if vid not in cur_set:
if not breaker.open(): item_id = breaker.run(
failures.append(vid) lambda v=vid: yt.add_playlist_item(pl.yt_playlist_id, v),
continue lambda v=vid: failures.append(v),
try: )
item_id_by_vid[vid] = yt.add_playlist_item(pl.yt_playlist_id, vid) if item_id is not FAILED:
item_id_by_vid[vid] = item_id
inserted += 1 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. # Reorder to match `desired` via insertion-sort moves over a local model.
model = [v for v in cur_vids if v in desired_set] + [ 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 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 continue # failed insert; skip
if i < len(model) and model[i] == want: if i < len(model) and model[i] == want:
continue continue
if not breaker.open(): if breaker.run(
failures.append(want) lambda w=want, pos=i: yt.move_playlist_item(
continue item_id_by_vid[w], pl.yt_playlist_id, w, pos
try: ),
yt.move_playlist_item(item_id_by_vid[want], pl.yt_playlist_id, want, i) lambda w=want: failures.append(w),
reordered += 1 ) is FAILED:
breaker.hit(True)
except YouTubeError:
failures.append(want)
breaker.hit(False)
continue continue
reordered += 1
j = model.index(want) j = model.index(want)
model.pop(j) model.pop(j)
model.insert(i, want) model.insert(i, want)
+15 -6
View File
@@ -73,6 +73,18 @@ def apply_channel_details(db: Session, items: list[dict]) -> None:
channel.details_synced_at = now 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: def import_subscriptions(db: Session, user: User) -> dict:
"""Pull all subscriptions for `user`, upsert channels + subscription links, prune """Pull all subscriptions for `user`, upsert channels + subscription links, prune
channels the user has unsubscribed from on YouTube, and fetch channel details for 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") sub_row.yt_subscription_id = sub.get("yt_subscription_id")
db.flush() db.flush()
# Prune subscriptions removed on YouTube — but NEVER on an empty fetch. The API can answer # Prune subscriptions removed on YouTube — but only when this fetch is trustworthy
# 200 with no items (a transient blip, a scope/token edge), and pruning against that wipes # (see `should_prune`).
# 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 removed = 0
existing = ( existing = (
db.execute(select(Subscription).where(Subscription.user_id == user.id)) db.execute(select(Subscription).where(Subscription.user_id == user.id))
.scalars() .scalars()
.all() .all()
) )
prune_skipped = bool(existing) and not fetched prune_skipped = not should_prune(len(fetched), len(existing))
if prune_skipped: if prune_skipped:
log.warning( log.warning(
"Subscription import (user %s): YouTube returned 0 subscriptions while %d exist " "Subscription import (user %s): YouTube returned 0 subscriptions while %d exist "
+31 -8
View File
@@ -16,6 +16,7 @@ Design:
* Crash-safe: on startup, orphaned `running` jobs are requeued and `downloading` assets reset. * Crash-safe: on startup, orphaned `running` jobs are requeued and `downloading` assets reset.
""" """
import logging import logging
import re
import shutil import shutil
import signal import signal
import threading import threading
@@ -351,6 +352,17 @@ def _staging_dir(asset_id: int) -> Path:
_THUMB_SUFFIXES = (".jpg", ".jpeg", ".png", ".webp") _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: 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()`, 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 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 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] req = (info.get("requested_downloads") or [{}])[0]
fp = req.get("filepath") or req.get("_filename") or info.get("filepath") or info.get("_filename") fp = req.get("filepath") or req.get("_filename") or info.get("filepath") or info.get("_filename")
if fp: if fp:
@@ -377,7 +390,9 @@ def _produced_file(info: dict, staging: Path) -> Path:
cands = [ cands = [
p p
for p in staging.iterdir() 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: if not cands:
raise RuntimeError("yt-dlp produced no output file") 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. """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 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] entries = [e for e in (info.get("entries") or []) if e]
if entries and not info.get("requested_downloads"): if not entries or info.get("requested_downloads"):
return {**info, **entries[0]} return info
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). # Errors worth retrying with a different player-client set (YouTube per-client flakiness).
@@ -729,9 +748,13 @@ def main() -> None:
return return
# The API may still be applying migrations when we boot. If they never arrive, exit instead of # 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(): 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() _recover_orphans()
n = max(1, settings.download_worker_concurrency) n = max(1, settings.download_worker_concurrency)
log.info("Download worker started (concurrency=%d, root=%s).", n, settings.download_root) log.info("Download worker started (concurrency=%d, root=%s).", n, settings.download_root)
+36 -2
View File
@@ -38,6 +38,22 @@ class TestProducedFile:
(tmp_path / "huge.mp4.part").write_bytes(b"x" * 999) (tmp_path / "huge.mp4.part").write_bytes(b"x" * 999)
assert _produced_file({}, staging) == tmp_path / "big.mp4" 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): def test_no_output_at_all_raises(self, tmp_path):
staging = _staging(tmp_path, "only.jpg") staging = _staging(tmp_path, "only.jpg")
with pytest.raises(RuntimeError): with pytest.raises(RuntimeError):
@@ -53,6 +69,22 @@ class TestMediaInfo:
info = {"id": "vid", "requested_downloads": [{"filepath": "/x.mp4"}]} info = {"id": "vid", "requested_downloads": [{"filepath": "/x.mp4"}]}
assert _media_info(info) is info 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): def test_wrapper_that_downloaded_itself_wins_over_its_entries(self):
info = {"id": "vid", "requested_downloads": [{"filepath": "/x.mp4"}], "entries": [{"id": "other"}]} info = {"id": "vid", "requested_downloads": [{"filepath": "/x.mp4"}], "entries": [{"id": "other"}]}
assert _media_info(info)["id"] == "vid" 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. # 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" assert _concat_quote(PurePosixPath("/m/Don't_Look.mp4")) == "/m/Don'\\''t_Look.mp4"
def test_backslash_is_doubled(self): def test_backslash_stays_literal(self):
assert _concat_quote(PurePosixPath("/m/a\\b.mp4")) == "/m/a\\\\b.mp4" # 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): def test_ordinary_path_is_unchanged(self):
assert _concat_quote(PurePosixPath("/m/clip.mp4")) == "/m/clip.mp4" assert _concat_quote(PurePosixPath("/m/clip.mp4")) == "/m/clip.mp4"
+94 -2
View File
@@ -1,5 +1,28 @@
"""Guards that stop one bad YouTube answer from cascading: the playlist-push circuit breaker.""" """Guards that stop one bad YouTube answer from cascading: the subscription prune guard, the
from app.sync.playlists import _Breaker 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: class TestBreaker:
@@ -26,3 +49,72 @@ class TestBreaker:
b.hit(False) b.hit(False)
b.hit(True) b.hit(True)
assert not b.open() 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"""<?xml version="1.0"?><feed xmlns="http://www.w3.org/2005/Atom"></feed>"""
self._patch(monkeypatch, status=200, body=feed)
assert rss.fetch_channel_feed("UC_quiet") == []
def test_entries_are_parsed(self, monkeypatch):
feed = b"""<?xml version="1.0"?>
<feed xmlns="http://www.w3.org/2005/Atom" xmlns:yt="http://www.youtube.com/xml/schemas/2015">
<entry><yt:videoId>abc123</yt:videoId><title>Hello</title>
<published>2026-07-23T10:00:00+00:00</published></entry>
</feed>"""
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"
+58 -11
View File
@@ -55,6 +55,21 @@ type LoopMode = "off" | "one" | "all";
const AUTO_MODES: AutoMode[] = ["off", "next", "prev", "random"]; const AUTO_MODES: AutoMode[] = ["off", "next", "prev", "random"];
const LOOP_MODES: LoopMode[] = ["off", "one", "all"]; 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 `<body>`: 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) --- // --- IFrame Player API loader (singleton) ---
let apiPromise: Promise<any> | null = null; let apiPromise: Promise<any> | null = null;
function loadYouTubeApi(): Promise<any> { function loadYouTubeApi(): Promise<any> {
@@ -168,9 +183,15 @@ export default function PlayerModal({
const [playerPrefs, patchPlayerPrefs] = useAccountPersistedObject(LS.ytPlayerPrefs, { const [playerPrefs, patchPlayerPrefs] = useAccountPersistedObject(LS.ytPlayerPrefs, {
volume: 100, volume: 100,
}); });
// The window/wheel listeners below are bound once, so they must read the LIVE prefs. // The live volume: updated on every notch, while the localStorage write behind it is debounced
const prefsRef = useRef(playerPrefs); // (a wheel spin fires ~16 events a second, and each patch is a synchronous setItem on the frame
prefsRef.current = playerPrefs; // 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<number | undefined>(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 focusModal = () => cardRef.current?.focus();
const togglePlay = () => { 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 /** 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). */ * 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 applyVolume = (level: number, flash = true) => {
const next = Math.max(0, Math.min(100, Math.round(level))); const next = Math.max(0, Math.min(100, Math.round(level)));
const p = playerRef.current; const p = playerRef.current;
@@ -201,15 +227,29 @@ export default function PlayerModal({
p.setVolume(next); p.setVolume(next);
if (next === 0) p.mute?.(); 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); if (flash) flashVolume(next);
}; };
const nudgeVolume = (delta: number) => { const nudgeVolume = (delta: number) => {
const p = playerRef.current; const p = playerRef.current;
if (!p || typeof p.getVolume !== "function") return; if (!p || typeof p.getVolume !== "function") return;
// Read the live player (the user may have used YouTube's own slider), falling back to the // Whose level do we step from? Our own ref, EXCEPT when the gesture starts fresh — then read
// stored level when the player can't answer yet. // the player, because the user may have moved YouTube's own slider in the meantime.
const cur = p.isMuted?.() ? 0 : (p.getVolume?.() ?? prefsRef.current.volume); // `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); applyVolume(cur + delta);
}; };
@@ -425,10 +465,14 @@ export default function PlayerModal({
togglePlay(); togglePlay();
} else if (e.key === "ArrowUp" || e.key === "ArrowDown") { } else if (e.key === "ArrowUp" || e.key === "ArrowDown") {
// Volume by keyboard (YouTube/Plex-style ±5). The only volume control that survives // Volume by keyboard (YouTube/Plex-style ±5). The only volume control that survives
// fullscreen, where the wheel goes to YouTube's own iframe. // fullscreen, where the wheel goes to YouTube's own iframe. But the modal card scrolls
if (typing) return; // (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(); e.preventDefault();
nudgeVolume(e.key === "ArrowUp" ? 5 : -5); nudgeVolume(up ? 5 : -5);
} else if (e.key === "ArrowLeft" || e.key === "ArrowRight") { } else if (e.key === "ArrowLeft" || e.key === "ArrowRight") {
// Plain arrows seek within the video (YouTube-style ±5s); Shift+arrow steps to the // 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). // 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(closeTimer.current);
window.clearTimeout(openTimer.current); window.clearTimeout(openTimer.current);
window.clearTimeout(volTimerRef.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]); }, [onClose]);
@@ -582,7 +629,7 @@ export default function PlayerModal({
// restore the remembered volume — a fresh player always starts at 100. // restore the remembered volume — a fresh player always starts at 100.
onReady: () => { onReady: () => {
focusModal(); focusModal();
applyVolume(prefsRef.current.volume, false); applyVolume(volumeRef.current, false);
}, },
onStateChange: (e: any) => { onStateChange: (e: any) => {
// 1 === playing → sync the navigated-to video's title/author for display. // 1 === playing → sync the navigated-to video's title/author for display.