From 6ac9ffd45e420dfd67655d50806ad57e6269fca7 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 1 Oct 2026 08:31:03 +0200 Subject: [PATCH] =?UTF-8?q?fix(plex,audit):=20E30=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20only=20a=20missing=20file=20is=20a=20dead=20headsho?= =?UTF-8?q?t,=20one=20spelling=20per=20portrait?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - warm_portraits: only 404/410 mark a headshot dead (measured: Plex answers a dead CDN file 404); a refused token or a 5xx is Plex failing, ends the run after three in a row and marks nothing. 'failed' counts every failure, not only the current streak. - is_portrait_url: plain path segments only — no dot or empty segment an HTTP client folds onto one file; person_thumb_url uses the same predicate (27 852 stored headshots all pass). - Tests: Plex refusing (401/403/429/5xx), failures between successes (portraits and bios), the kept original served from cache, the audit_clear / demo_reset / discovery_purge rows' counts as data, and the 0094 backfill on real rows. --- backend/app/plex/people.py | 35 ++++++++------- backend/tests/test_audit_count_rows.py | 36 ++++++++++++++++ backend/tests/test_plex_overlay.py | 39 +++++++++++++++++ backend/tests/test_plex_people_enrich.py | 54 ++++++++++++++++++++---- 4 files changed, 141 insertions(+), 23 deletions(-) create mode 100644 backend/tests/test_audit_count_rows.py diff --git a/backend/app/plex/people.py b/backend/app/plex/people.py index 0e1e6d61..afac6525 100644 --- a/backend/app/plex/people.py +++ b/backend/app/plex/people.py @@ -29,9 +29,7 @@ PERSON_IMG_HOST = "https://metadata-static.plex.tv/" def person_thumb_url(thumb: str | None) -> str | None: """Proxied /person-image URL for a Plex CDN headshot, or None when it's missing / off-CDN.""" t = str(thumb or "") - return ( - f"/api/plex/person-image?u={quote(t, safe='')}" if t.startswith(PERSON_IMG_HOST) else None - ) + return f"/api/plex/person-image?u={quote(t, safe='')}" if is_portrait_url(t) else None def cast_from_meta(meta: dict, limit: int = 24) -> list[dict]: @@ -169,13 +167,15 @@ def people_by_credits( # a new box re-caches instead of serving a stale size. Only a resized image is ever written under it — # an original fetched without Plex keeps its own key, and goes once its small twin lands. PORTRAIT_BOX = 320 -# A headshot Plex answered with an error for (a dead CDN file) is not asked for again this long, so a +# A headshot Plex answered "not found" for (a dead CDN file) is not asked for again this long, so a # few dead faces never hold up the rest of the prefetch. PORTRAIT_RETRY = timedelta(days=7) -# The one shape a Plex CDN headshot has (…/7/people/.jpg): no query, no fragment. The -# person-image proxy takes nothing else, so it cannot be made to cache endless variants of one file -# (the image cache has no eviction). -_PORTRAIT_URL = re.compile(re.escape(PERSON_IMG_HOST) + r"[A-Za-z0-9_./-]+") +# The one shape a Plex CDN headshot has (…/7/people/.jpg): plain path segments, no query, no +# fragment, no dot segment or empty segment (an HTTP client folds `a/../b` and the CDN `a//b` onto one +# file). The person-image proxy takes nothing else, so it cannot be made to cache endless spellings of +# one file (the image cache has no eviction). Used where a stored headshot is made, served and warmed. +_SEGMENT = r"[A-Za-z0-9_-]+(?:\.[A-Za-z0-9_-]+)*" # a name, dots only between its parts +_PORTRAIT_URL = re.compile(re.escape(PERSON_IMG_HOST) + _SEGMENT + rf"(?:/{_SEGMENT})*") def is_portrait_url(url: str) -> bool: @@ -219,15 +219,17 @@ def cdn_url(photo_thumb: str) -> str | None: return url if is_portrait_url(url) else None -# Plex not answering this many headshots in a row ends a run: it is Plex that is failing, not the files. +# Plex failing this many headshots in a row ends a run: it is Plex that is failing, not the files. _PLEX_DOWN_AFTER = 3 +# The statuses that mean the one file is gone. +_DEAD = frozenset({404, 410}) def warm_portraits(db: Session, limit: int) -> dict: """Cache up to `limit` not-yet-cached headshots, the most-credited people first (E30 S6), so a person's face comes from local disk. Needs Plex (the resize) and the Plex module on; without them nothing is prefetched — full-size originals for every person would cost gigabytes. A headshot Plex - answers with an error for is marked and left for PORTRAIT_RETRY; Plex not answering ends the run.""" + answers "not found" for is marked and left for PORTRAIT_RETRY; Plex failing in a row ends the run.""" if not sysconfig.get_bool(db, "plex_enabled"): return {"cached": 0, "skipped": "disabled"} try: @@ -242,7 +244,7 @@ def warm_portraits(db: Session, limit: int) -> dict: ).tuples() if thumb } - done = dead = down = 0 + done = dead = plex_failed = down = 0 # `down` counts in a row, `plex_failed` in all with plex: for pid in people_by_credits(db, "portrait", None): if done + dead >= limit or down >= _PLEX_DOWN_AFTER: @@ -257,12 +259,15 @@ def warm_portraits(db: Session, limit: int) -> dict: cache_portrait(plex, url) done, down = done + 1, 0 except PlexError as exc: - # An error status is Plex's answer about this one file; no answer is Plex itself. - if isinstance(exc.__cause__, httpx.HTTPStatusError): + # "Not found" is Plex's answer about this one file (a dead CDN headshot: measured 404, + # the CDN itself says 403). Anything else — no answer, a refused token, a 5xx — is + # Plex failing, which must neither mark the file nor keep the run going. + cause = exc.__cause__ + if isinstance(cause, httpx.HTTPStatusError) and cause.response.status_code in _DEAD: dead += 1 imgcache.mark_miss(key) else: - down += 1 + plex_failed, down = plex_failed + 1, down + 1 log.warning( "a portrait could not be cached", extra=logs.event( @@ -271,5 +276,5 @@ def warm_portraits(db: Session, limit: int) -> dict: reason=type(exc.__cause__).__name__, ), ) - failed = dead + down + failed = dead + plex_failed return {"cached": done, **({"failed": failed} if failed else {})} diff --git a/backend/tests/test_audit_count_rows.py b/backend/tests/test_audit_count_rows.py new file mode 100644 index 00000000..1cf7f433 --- /dev/null +++ b/backend/tests/test_audit_count_rows.py @@ -0,0 +1,36 @@ +"""E30: an admin action that counts something keeps the count as data in its audit row's `after`, under +the key the audit log names in the reader's language (`scheduler.result.`, frontend +auditColumns.tsx) — not only in the English `summary`. A renamed key would fall back to the raw word. +DB lane (Postgres). +""" + +from sqlalchemy import select + +from app.models import AuditLog + + +def _after(db, action: str) -> dict: + db.expire_all() + return db.execute(select(AuditLog.after).where(AuditLog.action == action)).scalar_one() + + +def test_clearing_the_audit_log_keeps_the_count_as_data(as_user, db, make_user): + admin = make_user(email="boss@example.com", role="admin") + db.add_all([AuditLog(action="sync_pause"), AuditLog(action="sync_resume")]) + db.commit() + assert as_user(admin).delete("/api/admin/audit").json()["cleared"] == 2 + assert _after(db, "audit_clear") == {"cleared": 2} + + +def test_a_demo_reset_keeps_the_count_as_data(as_user, db, make_user): + admin = make_user(email="boss@example.com", role="admin") + seeded = as_user(admin).post("/api/admin/demo/reset").json()["playlists_seeded"] + assert _after(db, "demo_reset") == {"playlists_seeded": seeded} + + +def test_a_discovery_purge_keeps_its_counts_as_data(as_user, db, make_user): + admin = make_user(email="boss@example.com", role="admin") + body = as_user(admin).post("/api/admin/purge-discovery").json() + body.pop("ok") + assert _after(db, "discovery_purge") == body + assert {"videos_deleted", "channels_deleted", "search_videos_deleted"} <= set(body) diff --git a/backend/tests/test_plex_overlay.py b/backend/tests/test_plex_overlay.py index 4d1706e8..bf70ca34 100644 --- a/backend/tests/test_plex_overlay.py +++ b/backend/tests/test_plex_overlay.py @@ -6,10 +6,14 @@ Uses the DB lane (`db` fixture) — skipped when no Postgres is reachable. Runs (migrations 0063/0064) so the JSONB + STORED generated columns are exercised. """ +from alembic import command +from alembic.config import Config from sqlalchemy import func, select +from sqlalchemy.orm import Session import app.plex.sync as syncmod from app import sysconfig +from app.db import engine from app.models import PlexItem, PlexLibrary, PlexShow from app.plex.genres import hu_genre, hu_genres from app.plex.sync import _apply_item, _canonical_guid, _sync_overlay @@ -378,3 +382,38 @@ def test_the_genre_map_is_idempotent_and_keeps_an_unknown_word(): assert once == ["Dráma", "Akció"] assert hu_genres(once) == once assert hu_genres(None) is None and hu_genres([]) is None + + +def test_0094_gives_every_row_without_a_hungarian_list_one(db): + cfg = Config("alembic.ini") + command.downgrade(cfg, "0093_plex_person_known_for_votes") + upgraded = False + try: + with Session(engine) as s: + lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) + s.add(lib) + s.flush() + rows = { + "bare": PlexItem(genres=["Action", "Biography"]), + "overlaid": PlexItem( + genres=["Action"], genres_hu=["Kaland"] + ), # its twin's own list + "genreless": PlexItem(genres=[]), + } + for key, row in rows.items(): + row.rating_key, row.library_id, row.kind, row.title = key, lib.id, "movie", key + show = PlexShow(rating_key="s", library_id=lib.id, title="s", genres=["Comedy"]) + s.add_all([*rows.values(), show]) + s.commit() + ids = {k: r.id for k, r in rows.items()} + show_id = show.id + command.upgrade(cfg, "head") + upgraded = True + with Session(engine) as s: + got = {k: s.get(PlexItem, i).genres_hu for k, i in ids.items()} # type: ignore[union-attr] + show_hu = s.get(PlexShow, show_id).genres_hu # type: ignore[union-attr] + assert got == {"bare": ["Akció", "Életrajzi"], "overlaid": ["Kaland"], "genreless": None} + assert show_hu == ["Vígjáték"] + finally: + if not upgraded: + command.upgrade(cfg, "head") diff --git a/backend/tests/test_plex_people_enrich.py b/backend/tests/test_plex_people_enrich.py index c59cfeb9..4666d6b8 100644 --- a/backend/tests/test_plex_people_enrich.py +++ b/backend/tests/test_plex_people_enrich.py @@ -88,6 +88,14 @@ def test_failures_in_a_row_end_the_batch(db): assert tmdb.asked == [1, 2, 3] +def test_failures_between_successes_do_not_end_the_batch(db): + db.add_all([PlexPerson(name=f"P{i}", tmdb_person_id=i) for i in range(1, 9)]) + db.commit() + tmdb = _Tmdb(fail_on={1, 3, 5, 7}) # fail, ok, fail, ok, …: never three in a row + assert plex_tmdb.enrich_people_batch(db, tmdb, 10) == {"enriched": 4, "failed": 4} # type: ignore[arg-type] + assert tmdb.asked == list(range(1, 9)) + + def test_the_batch_stores_the_hungarian_biography(db): db.add(PlexPerson(name="A", tmdb_person_id=1)) db.commit() @@ -108,12 +116,14 @@ def test_the_batch_stores_the_hungarian_biography(db): class _Plex: - """A PlexClient stand-in. `down`: Plex does not answer; `dead`: urls Plex answers with an error.""" + """A PlexClient stand-in. `down`: urls Plex does not answer for (True: none); `dead`: urls Plex + answers "not found" for (a dead CDN file); `status`: an error status Plex answers every url with.""" - def __init__(self, down=False, dead=()): + def __init__(self, down=False, dead=(), status=None): self.asked: list[tuple[str, int | None]] = [] self.down = down self.dead = set(dead) + self.status = status def __enter__(self): return self @@ -123,14 +133,15 @@ class _Plex: def image_bytes(self, url, width=None, height=None): self.asked.append((url, width)) - if self.down: + if self.down is True or (self.down and url in self.down): raise plex_people.PlexError("down") from httpx.ConnectError("refused") - if url in self.dead: + code = 404 if url in self.dead else self.status + if code: req = httpx.Request("GET", url) status = httpx.HTTPStatusError( - "404", request=req, response=httpx.Response(404, request=req) + str(code), request=req, response=httpx.Response(code, request=req) ) - raise plex_people.PlexError("404") from status + raise plex_people.PlexError(str(code)) from status return b"small", "image/jpeg" @@ -210,6 +221,21 @@ def test_plex_not_answering_ends_the_portrait_run(db, cache_dir, plex_on): assert not list(cache_dir.glob("*.miss")) # Plex was down, the files are not to blame +@pytest.mark.parametrize("code", [401, 403, 429, 500, 503]) +def test_plex_refusing_is_plex_failing_not_a_dead_file(db, cache_dir, plex_on, code): + _faces(db, 8) + plex_on["plex"] = _Plex(status=code) # a rotated token, a busy or broken transcoder + assert plex_people.warm_portraits(db, 100) == {"cached": 0, "failed": 3} + assert len(plex_on["plex"].asked) == 3 + assert not list(cache_dir.glob("*.miss")) # the files are not to blame: asked again next run + + +def test_plex_failing_between_successes_does_not_end_the_run(db, cache_dir, plex_on): + _faces(db, 8) + plex_on["plex"] = _Plex(down={CDN + f"{i}.jpg" for i in (0, 2, 4, 6)}) # down, ok, down, ok, … + assert plex_people.warm_portraits(db, 100) == {"cached": 4, "failed": 4} + + def test_dead_headshots_are_left_alone_and_the_rest_still_land(db, cache_dir, plex_on): _faces(db, 8) dead = {CDN + f"{i}.jpg" for i in range(6)} # more dead files than any failure limit @@ -237,9 +263,19 @@ def test_the_person_image_route(db, cache_dir, monkeypatch): def get(u): return client.get("/api/plex/person-image", params={"u": u}) - # Only a headshot-shaped url on the CDN host: no query variants of one file, no other host. - for bad in (url + "?n=1", url + "#x", "https://example.com/people/a.jpg"): + # Only a headshot-shaped url on the CDN host: no query variants of one file, no spellings an HTTP + # client folds onto it, no other host. + for bad in ( + url + "?n=1", + url + "#x", + CDN + "x/../face.jpg", + CDN + "./face.jpg", + plex_people.PERSON_IMG_HOST + "people//face.jpg", + CDN + ".face.jpg", + "https://example.com/people/a.jpg", + ): assert get(bad).status_code == 400, bad + assert plex_people.person_thumb_url(bad) is None, bad # never stored either # No Plex: the original from the CDN, kept under its own key (the job resizes it later). def not_configured(d): @@ -268,6 +304,8 @@ def test_the_person_image_route(db, cache_dir, monkeypatch): resp = get(url) assert (resp.status_code, resp.content) == (200, b"original") assert imgcache.has(original) and not imgcache.has(small) + monkeypatch.setattr(plex_people.httpx, "Client", None) # the CDN is not asked again + assert get(url).content == b"original" # the kept original answers # With Plex: resized, and it replaces the original. monkeypatch.setattr(playback, "PlexClient", lambda d: _Plex())