refactor(e5): deferred review-tail (backend) — enrich off request path, reuse, tests

Address the deep-review's deferred backend items so E5 is review-complete:

- person-enrich OFF the request path (ABUSE finding): person_detail no longer makes
  the TMDB call inline (even bounded to 8s it sat on the serve thread, un-throttled,
  demo-reachable — it violated the module's "no live TMDB call at serve time"
  invariant). It now schedules a FastAPI BackgroundTask (people._enrich_person_bg,
  fresh session, best-effort) after the response; the first open serves the row as-is
  and the bio lands by the next open. The inline try/rollback recovery is gone with it.

- reuse: a shared tmdb.resolve_library_guids owns the collision-safe guid→library
  bucketing (movie vs tv ids are independent sequences) that resolve_recs and
  people._resolve_known_for each hand-duplicated; a new rollup.item_state_map dedups
  the "PlexState-by-item_id" fetch across the person + discover paths.

- test gaps: enrich_batch transient TmdbError leaves the watermark NULL to retry;
  guid_to_tmdb_id resolves a tvdb source + tv media_type via /find's tv_results;
  _continue_watching includes a NULL-status started movie and excludes a hidden one;
  /api/search/youtube returns 422 (not 5xx) when ingest fails (route-level, B-core-1).

Backend lane 403 → 413, ruff clean. No migration; serve contracts unchanged.
This commit is contained in:
2026-08-21 06:09:17 +02:00
parent 5316f48af2
commit 13fb2b4e61
8 changed files with 263 additions and 120 deletions
+18
View File
@@ -76,3 +76,21 @@ def state_map(db: Session, user_id: int, show_ids: list[int]) -> dict[int, str]:
agg_subquery(db, user_id).where(PlexItem.show_id.in_(show_ids))
).all()
return {r.show_id: show_status(r.total, r.watched, r.inprog) for r in rows}
def item_state_map(
db: Session, user_id: int, item_ids: list[int]
) -> dict[int, PlexState]:
"""A user's PlexState rows for a set of leaf items (movies or episodes), keyed by item_id — the
leaf analogue of `state_map` (which rolls a show's episodes up to a status string). One batched
fetch, so the caller serializes cards without a per-card query. Empty in → empty dict (no `IN ()`)."""
if not item_ids:
return {}
return {
s.item_id: s
for s in db.execute(
select(PlexState).where(
PlexState.user_id == user_id, PlexState.item_id.in_(item_ids)
)
).scalars()
}
+50 -29
View File
@@ -344,40 +344,61 @@ def run_enrich(db: Session) -> dict:
return {"enriched": n}
def resolve_recs(db: Session, recs: list[dict] | None) -> list[dict]:
"""Tag each recommendation with whether it's in OUR library (matched by guid `tmdb:<id>`) and, if so,
its rating_key — so the client can deep-link the ones we have and merely display the rest.
def resolve_library_guids(
db: Session,
entries: list[dict] | None,
*,
movie_lib_ids: list[int] | None = None,
show_lib_ids: list[int] | None = None,
) -> tuple[dict[str, PlexItem], dict[str, PlexShow]]:
"""Match a list of TMDB credits/recs (each `{tmdb_id, media_type, …}`) to our library rows by the
canonical guid `tmdb:<id>`, returning `(movies_by_guid, shows_by_guid)` of the ORM rows.
TMDB movie ids and tv ids are independent sequences (id 603 can be BOTH a movie and a show), so the
match is bucketed by each rec's own media_type: a movie rec only matches a library movie, a tv rec
only a library show — otherwise a numeric-id collision would deep-link a rec to an unrelated title."""
if not recs:
return []
⭐ TMDB movie ids and tv ids are INDEPENDENT sequences (id 603 can be BOTH a movie and a show), so
each entry matches ONLY its own media_type's table — a movie credit against a library movie, a tv
credit against a library show; a blind numeric match would resolve to an unrelated same-id title.
Pass `movie_lib_ids`/`show_lib_ids` to scope to the enabled libraries (the person page); leave them
None to match across all libraries (the detail-page recs). An explicit empty list means "that side
has no enabled library" → its bucket is skipped entirely."""
if not entries:
return {}, {}
movie_guids = [
f"tmdb:{r['tmdb_id']}" for r in recs if r.get("tmdb_id") and r.get("media_type") != "tv"
f"tmdb:{e['tmdb_id']}" for e in entries if e.get("tmdb_id") and e.get("media_type") != "tv"
]
tv_guids = [
f"tmdb:{r['tmdb_id']}" for r in recs if r.get("tmdb_id") and r.get("media_type") == "tv"
f"tmdb:{e['tmdb_id']}" for e in entries if e.get("tmdb_id") and e.get("media_type") == "tv"
]
movie_lib: dict[str, str] = {}
show_lib: dict[str, str] = {}
if movie_guids:
for guid, rk in db.execute(
select(PlexItem.guid, PlexItem.rating_key).where(
PlexItem.kind == "movie", PlexItem.guid.in_(movie_guids)
)
):
movie_lib.setdefault(guid, rk)
if tv_guids:
for guid, rk in db.execute(
select(PlexShow.guid, PlexShow.rating_key).where(PlexShow.guid.in_(tv_guids))
):
show_lib.setdefault(guid, rk)
movies: dict[str, PlexItem] = {}
if movie_guids and movie_lib_ids != []:
conds = [PlexItem.kind == "movie", PlexItem.guid.in_(movie_guids)]
if movie_lib_ids is not None:
conds.append(PlexItem.library_id.in_(movie_lib_ids))
for m in db.execute(select(PlexItem).where(*conds)).scalars():
movies.setdefault(m.guid, m)
shows: dict[str, PlexShow] = {}
if tv_guids and show_lib_ids != []:
conds = [PlexShow.guid.in_(tv_guids)]
if show_lib_ids is not None:
conds.append(PlexShow.library_id.in_(show_lib_ids))
for s in db.execute(select(PlexShow).where(*conds)).scalars():
shows.setdefault(s.guid, s)
return movies, shows
def resolve_recs(db: Session, recs: list[dict] | None) -> list[dict]:
"""Tag each recommendation with whether it's in OUR library (matched by guid `tmdb:<id>`, bucketed
by media_type — see resolve_library_guids) and, if so, its rating_key — so the client can deep-link
the ones we have and merely display the rest."""
if not recs:
return []
movies, shows = resolve_library_guids(db, recs)
out: list[dict] = []
for r in recs:
g = f"tmdb:{r.get('tmdb_id')}"
rk = show_lib.get(g) if r.get("media_type") == "tv" else movie_lib.get(g)
out.append({**r, "in_library": rk is not None, "rating_key": rk})
row = shows.get(g) if r.get("media_type") == "tv" else movies.get(g)
out.append(
{**r, "in_library": row is not None, "rating_key": row.rating_key if row else None}
)
return out
@@ -453,9 +474,9 @@ def enrich_person(db: Session, person: PlexPerson) -> bool:
if not person.tmdb_person_id or not sysconfig.get_str(db, "tmdb_api_key"):
return False
try:
# This runs on the request thread (first person-page open), so cap the outbound TMDB call with
# a short timeout — a slow/hanging TMDB must not tie up a worker for the default 30s. (The fuller
# fix — moving person enrichment off the request path — is logged in ROADMAP under E5.)
# Called from a background task (off the request path — see people._enrich_person_bg), so it no
# longer blocks a serve; still cap the outbound TMDB call with a short timeout so a hanging TMDB
# doesn't tie up a threadpool worker for the default 30s.
with TmdbClient(db, timeout=8.0) as tmdb:
payload = tmdb.fetch_person(person.tmdb_person_id)
except TmdbError:
+1 -12
View File
@@ -37,19 +37,8 @@ def _scope_libs(db: Session, scope: str) -> tuple[list[int], list[int]]:
)
def _movie_states(db: Session, uid: int, items: list[PlexItem]) -> dict[int, PlexState]:
"""One batched fetch of the user's watch-state for a set of movies (avoids a per-card query)."""
ids = [it.id for it in items]
if not ids:
return {}
rows = db.execute(
select(PlexState).where(PlexState.user_id == uid, PlexState.item_id.in_(ids))
).scalars().all()
return {r.item_id: r for r in rows}
def _movie_cards(db: Session, uid: int, items: list[PlexItem]) -> list[dict]:
sts = _movie_states(db, uid, items)
sts = rollup.item_state_map(db, uid, [it.id for it in items])
return [movie_card(it, sts.get(it.id)) for it in items]
+33 -60
View File
@@ -2,17 +2,16 @@
import logging
from fastapi import APIRouter, Depends
from fastapi import APIRouter, BackgroundTasks, Depends
from sqlalchemy import func, or_, select
from sqlalchemy.orm import Session
from app.auth import current_user
from app.db import get_db
from app.db import SessionLocal, get_db
from app.models import (
PlexItem,
PlexPerson,
PlexShow,
PlexState,
User,
)
from app.plex import rollup
@@ -36,44 +35,13 @@ def _resolve_known_for(
popularity order. Matched by the canonical guid `tmdb:<id>`; out-of-library credits are dropped."""
if not known_for:
return []
# Bucket by media_type: TMDB movie ids and tv ids collide numerically, so a movie credit must only
# match a library movie and a tv credit only a library show (mirrors tmdb.resolve_recs). Matching
# blind to media_type would render a tv credit as an unrelated movie whose tmdb id happens to match.
movie_guids = [
f"tmdb:{k['tmdb_id']}" for k in known_for if k.get("tmdb_id") and k.get("media_type") != "tv"
]
tv_guids = [
f"tmdb:{k['tmdb_id']}" for k in known_for if k.get("tmdb_id") and k.get("media_type") == "tv"
]
if not movie_guids and not tv_guids:
return []
movies: dict[str, PlexItem] = {}
if movie_lib_ids and movie_guids:
for m in db.execute(
select(PlexItem).where(
PlexItem.kind == "movie",
PlexItem.library_id.in_(movie_lib_ids),
PlexItem.guid.in_(movie_guids),
)
).scalars():
movies.setdefault(m.guid, m)
shows: dict[str, PlexShow] = {}
if show_lib_ids and tv_guids:
for s in db.execute(
select(PlexShow).where(
PlexShow.library_id.in_(show_lib_ids), PlexShow.guid.in_(tv_guids)
)
).scalars():
shows.setdefault(s.guid, s)
m_states = {
s.item_id: s
for s in db.execute(
select(PlexState).where(
PlexState.user_id == uid,
PlexState.item_id.in_([m.id for m in movies.values()] or [0]),
)
).scalars()
}
# Match each credit to a library row by guid, bucketed by media_type (movie ids and tv ids collide
# numerically) and scoped to the enabled libraries — the shared helper owns that collision-safe
# logic (the detail-page recs use the same one).
movies, shows = plex_tmdb.resolve_library_guids(
db, known_for, movie_lib_ids=movie_lib_ids, show_lib_ids=show_lib_ids
)
m_states = rollup.item_state_map(db, uid, [m.id for m in movies.values()])
show_statuses = rollup.state_map(db, uid, [s.id for s in shows.values()])
out: list[dict] = []
for k in known_for:
@@ -144,9 +112,28 @@ def people_cards(
_PERSON_SECTION_MAX = 200
def _enrich_person_bg(person_id: int) -> None:
"""Fetch + persist one person's TMDB bio OFF the request path — a FastAPI background task that runs
AFTER the response is sent, so the outbound TMDB call never sits on the serve latency (the module's
"no live TMDB call at serve time" invariant). The request's session is already closed by then, so
open a fresh one. Best-effort: a TMDB failure just leaves `tmdb_enriched_at` NULL to retry on the
next open, and the bio lands from the row by then."""
db = SessionLocal()
try:
person = db.get(PlexPerson, person_id)
if person is not None:
plex_tmdb.enrich_person(db, person)
except Exception: # noqa: BLE001 — background best-effort; never surfaces to a user
db.rollback()
log.warning("background person bio enrich failed for id=%s", person_id, exc_info=True)
finally:
db.close()
@router.get("/person/{name}")
def person_detail(
name: str,
background_tasks: BackgroundTasks,
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
@@ -158,18 +145,13 @@ def person_detail(
E5 S6: the first time a page is opened, the person's TMDB bio + "Known for" credits are lazily
fetched (from the S2-linked `tmdb_person_id`) and persisted, then served from the row on every open.
The fetch runs OFF the request path (a background task); the first open serves whatever's in the row
(bio possibly still null) and the bio lands from the row by the next open.
"""
person = db.execute(select(PlexPerson).where(PlexPerson.name == name)).scalars().first()
# Lazy one-time bio enrich — best-effort, never fail the page if TMDB is down / the id is stale.
# Lazy one-time bio enrich, scheduled AFTER the response so the TMDB call is never on the serve path.
if person is not None and person.tmdb_person_id and person.tmdb_enriched_at is None:
try:
plex_tmdb.enrich_person(db, person)
except Exception: # noqa: BLE001 — enrichment is optional; the filmography still renders
# A failed enrich_person commit leaves the session in a needs-rollback state; without this
# the following SELECTs would raise PendingRollbackError and 500 the page (SQLAlchemy 2.x
# deactivates the transaction on a commit-time flush failure and does NOT auto-reset it).
db.rollback()
log.warning("person bio enrich failed for %s", name, exc_info=True)
background_tasks.add_task(_enrich_person_bg, person.id)
movie_lib_ids, show_lib_ids = _content_libs(db)
movies: list[dict] = []
@@ -187,16 +169,7 @@ def person_detail(
.order_by(PlexItem.year.desc().nullslast(), func.lower(PlexItem.title))
.limit(_PERSON_SECTION_MAX)
).scalars().all()
item_ids = [it.id for it in items]
states = {
s.item_id: s
for s in db.execute(
select(PlexState).where(
PlexState.user_id == user.id,
PlexState.item_id.in_(item_ids or [0]),
)
).scalars()
}
states = rollup.item_state_map(db, user.id, [it.id for it in items])
movies = [movie_card(it, states.get(it.id)) for it in items]
shows: list[dict] = []
+27
View File
@@ -113,6 +113,33 @@ def test_continue_watching_movies_and_shows(db, monkeypatch):
assert "m1" not in ids
def test_continue_watching_null_status_in_hidden_out(db, monkeypatch):
# A started movie whose PlexState.status is NULL is still "in progress": `NULL NOT IN (...)` is NULL
# (not TRUE), so a bare notin_ would wrongly drop it — the or_(is_(None), notin_) branch keeps it.
# A hidden started movie must be excluded.
lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True)
db.add(lib)
db.flush()
m_null = PlexItem(rating_key="mn", library_id=lib.id, kind="movie", title="Null Status", guid="tmdb:1")
m_hidden = PlexItem(rating_key="mh", library_id=lib.id, kind="movie", title="Hidden", guid="tmdb:2")
db.add_all([m_null, m_hidden])
db.flush()
user = User(email="cw-null@example.com")
db.add(user)
db.flush()
db.add_all([
PlexState(user_id=user.id, item_id=m_null.id, status=None, position_seconds=500,
progress_updated_at=NOW),
PlexState(user_id=user.id, item_id=m_hidden.id, status="hidden", position_seconds=500,
progress_updated_at=NOW - timedelta(hours=1)),
])
db.commit()
ids = _ids(_get(db, user, monkeypatch, "/api/plex/discover?scope=movie").json()["continue_watching"])
assert "mn" in ids # NULL-status started movie IS in progress
assert "mh" not in ids # hidden is excluded
def test_because_you_watched_resolves_in_library_recs(db, monkeypatch):
user = _seed(db)
d = _get(db, user, monkeypatch, "/api/plex/discover?scope=both").json()
+36 -19
View File
@@ -100,34 +100,51 @@ def _get(db, user, monkeypatch, path):
app.dependency_overrides.clear()
def test_person_detail_survives_poisoned_enrich(db, monkeypatch):
# If the lazy enrich_person leaves the session in a failed-transaction state (e.g. a commit-time
# flush error), person_detail's except must roll it back so the following SELECTs still run — the
# page renders despite the enrich failure ("never fail the page"). Without the db.rollback() fix the
# poisoned session would raise PendingRollbackError on the next query and 500.
from sqlalchemy import text
def test_person_detail_schedules_enrich_off_request_path(db, monkeypatch):
# The bio fetch runs OFF the request path (a background task after the response), so the serve never
# makes a live TMDB call. An un-enriched person (tmdb_person_id set + tmdb_enriched_at None) schedules
# the enrich and the page still renders immediately with whatever's in the row (bio still null).
from app.routes.plex import people
lib = PlexLibrary(plex_key="9", title="Movies", kind="movie", enabled=True)
db.add(lib)
db.flush()
# Un-enriched person (tmdb_person_id set + tmdb_enriched_at None) → triggers the lazy enrich path.
db.add(PlexPerson(name="Poison P", tmdb_person_id=777))
user = User(email="poison@example.com")
db.add(PlexPerson(name="Lazy P", tmdb_person_id=777))
user = User(email="lazybg@example.com")
db.add(user)
db.commit()
def _poison_then_raise(db_, person_):
try:
db_.execute(text("SELECT 1 / 0")) # aborts the transaction (like a failed enrich commit)
except Exception: # noqa: BLE001 — swallow so the tx stays poisoned, then raise like the real path
pass
raise RuntimeError("enrich commit blew up")
scheduled: list[int] = []
monkeypatch.setattr(people, "_enrich_person_bg", lambda pid: scheduled.append(pid))
monkeypatch.setattr(plex_tmdb, "enrich_person", _poison_then_raise)
resp = _get(db, user, monkeypatch, "/api/plex/person/Lazy P")
assert resp.status_code == 200, resp.text
assert resp.json()["biography"] is None # not enriched yet — the bio lands by the next open
assert len(scheduled) == 1 # the enrich was scheduled off the request path
resp = _get(db, user, monkeypatch, "/api/plex/person/Poison P")
assert resp.status_code == 200, resp.text # page still renders (session was rolled back)
assert resp.json()["biography"] is None # enrich failed, so no bio
def test_person_detail_no_enrich_when_already_enriched(db, monkeypatch):
# An already-enriched person (tmdb_enriched_at set) must NOT schedule another background fetch.
from datetime import datetime, timezone
from app.routes.plex import people
lib = PlexLibrary(plex_key="8", title="Movies", kind="movie", enabled=True)
db.add(lib)
db.flush()
db.add(PlexPerson(name="Done P", tmdb_person_id=888,
tmdb_enriched_at=datetime(2024, 1, 1, tzinfo=timezone.utc), biography="Bio."))
user = User(email="donebg@example.com")
db.add(user)
db.commit()
scheduled: list[int] = []
monkeypatch.setattr(people, "_enrich_person_bg", lambda pid: scheduled.append(pid))
resp = _get(db, user, monkeypatch, "/api/plex/person/Done P")
assert resp.status_code == 200, resp.text
assert resp.json()["biography"] == "Bio."
assert scheduled == [] # already enriched → no background work
def test_person_detail_serves_bio_and_resolves_known_for(db, monkeypatch):
+38
View File
@@ -142,6 +142,19 @@ def test_guid_to_tmdb_id_direct_find_and_bad():
assert tmdb.guid_to_tmdb_id("local://abc", "movie", fake) is None
def test_guid_to_tmdb_id_tvdb_source_and_tv_media_type():
# A tvdb guid for a TV item goes through /find and must pick the tv_results list (not movie_results),
# and a tvdb source maps to the tvdb_id external_source. The media_type branch is the whole point.
fake = FakeTmdb(find_map={"81189": {"movie_results": [], "tv_results": [{"id": 1396}]}})
assert tmdb.guid_to_tmdb_id("tvdb:81189", "tv", fake) == 1396
# The SAME /find result, asked for a movie, reads the (empty) movie_results → None.
assert tmdb.guid_to_tmdb_id("tvdb:81189", "movie", fake) is None
# A direct tmdb guid for a tv item is just the int (no /find).
assert tmdb.guid_to_tmdb_id("tmdb:1396", "tv", fake) == 1396
# An unknown provider (only imdb/tvdb are /find sources) is unresolvable.
assert tmdb.guid_to_tmdb_id("plex:abc", "tv", fake) is None
# ---- DB-lane: batch enrich + serve --------------------------------------------------------------
def _lib(db, key="1", kind="movie"):
@@ -245,6 +258,31 @@ def test_run_enrich_noops_without_key(db):
assert out["enriched"] == 0 and "skipped" in out
class _RaisingTmdb:
"""A client whose fetch raises a transient TmdbError (e.g. a 429), to exercise the retry path."""
def find(self, external_id, source):
return None
def fetch(self, tmdb_id, media_type):
raise tmdb.TmdbError("429 rate limited")
def test_enrich_batch_transient_error_leaves_watermark_null(db):
# A transient TmdbError mid-batch must leave the row's tmdb_enriched_at NULL so the next run retries
# it (unlike a definitive no-match, which stamps the watermark). The row is NOT counted as processed.
lib = _lib(db)
mv = PlexItem(rating_key="m1", library_id=lib.id, kind="movie", title="Godfather", guid="tmdb:238")
db.add(mv)
db.commit()
n = tmdb.enrich_batch(db, _RaisingTmdb(), 50)
assert n == 0 # the transient failure is skipped, not processed
db.refresh(mv)
assert mv.tmdb_enriched_at is None # left NULL → retried next run
assert mv.tmdb_id is None
def test_enrich_batch_skips_shows_without_guid(db):
lib = _lib(db, key="2", kind="show")
sh = PlexShow(rating_key="s1", library_id=lib.id, title="No Guid Show") # guid None → not selected
@@ -93,3 +93,63 @@ def test_scrape_ingest_enrich_miss_keeps_nonnull_placeholder(db):
c = db.get(Video, "vid_ccccccc")
assert c.published_at is not None # placeholder wrote a real value → no crash
assert c.enriched_at is None
def test_search_youtube_route_returns_422_when_ingest_fails(db, monkeypatch):
# Route-level guard (B-core-1): an ingest failure with nothing yet collected must surface as a 422
# + message on this JSON endpoint (the FE hides 5xx behind a generic "connection lost" + auto-retry,
# burning a second quota-costed search), and the session is rolled back. Here the page fetch succeeds
# (one candidate) and _ingest_candidates raises → the endpoint returns 422.
from fastapi.testclient import TestClient
import app.state as state
from app.auth import require_human
from app.db import get_db
from app.main import app
from app.routes import search as search_mod
user = User(email="route422@example.com")
db.add(user)
db.commit()
monkeypatch.setattr(state, "_configured_cache", True)
monkeypatch.setattr(
search_mod.sysconfig, "get_str", lambda d, k: "scrape" if k == "search_source" else ""
)
monkeypatch.setattr(search_mod.sysconfig, "get_int", lambda d, k: 100) # daily cap not reached
monkeypatch.setattr(search_mod.quota, "actions_today", lambda d, uid, action: 0)
class _FakeYTCM:
def __init__(self, *a, **k):
pass
def __enter__(self):
return self
def __exit__(self, *a):
return False
monkeypatch.setattr(search_mod, "YouTubeClient", _FakeYTCM)
monkeypatch.setattr(
search_mod.search_scrape,
"search",
lambda term, continuation=None: {
"items": [_candidate("vid_route12", "UC_route_a")],
"next_page_token": None,
},
)
def _boom(db_, yt, user_, candidates):
raise RuntimeError("ingest exploded")
monkeypatch.setattr(search_mod, "_ingest_candidates", _boom)
app.dependency_overrides[get_db] = lambda: db
app.dependency_overrides[require_human] = lambda: user
try:
resp = TestClient(app).get("/api/search/youtube?q=test+term")
finally:
app.dependency_overrides.clear()
assert resp.status_code == 422, resp.text
assert "YouTube search failed" in resp.json()["detail"]