From fad301e7a4771b56e2c816e89fc97d5690369b8f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 06:55:40 +0200 Subject: [PATCH 01/37] fix(youtube): a refused YouTube action answers a 422 that says what to do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two manual syncs (subscriptions import, playlist pull) had no YouTubeError handler, so a quota, token or transport failure was a raw 500 and the blocking "Server error" dialog (E9 F1). The other YouTube routes answered either the upstream's English or a playlist-scoped "youtube_failed". One helper, routes/_common.youtube_refusal: 422 {code: "youtube", reason} with the reason sorted by what the user can do — quota (wait for the reset), reconnect (the token step refused), unavailable (retry). Used by every route that surfaces a YouTube failure; localizeDetail translates it. "Reconnect" had no path: a grant Google revoked still read as granted, so Settings and the wizard offered no connect button while every call failed. An invalid_grant from the token endpoint now clears the stored grant. --- backend/app/routes/_common.py | 10 ++ backend/app/routes/channels.py | 10 +- backend/app/routes/feed.py | 4 +- backend/app/routes/playlists.py | 26 +++-- backend/app/routes/sync.py | 12 +- backend/app/youtube/client.py | 41 ++++++- backend/tests/test_log_pii.py | 4 +- backend/tests/test_youtube_refusals.py | 106 ++++++++++++++++++ .../usePlaylistActions.dom.test.tsx | 2 +- frontend/src/i18n/locales/en/errors.json | 5 + frontend/src/i18n/locales/en/playlists.json | 1 - frontend/src/i18n/locales/hu/errors.json | 5 + frontend/src/i18n/locales/hu/playlists.json | 1 - frontend/src/lib/api/core.test.ts | 17 +++ frontend/src/lib/api/core.ts | 4 + 15 files changed, 221 insertions(+), 27 deletions(-) create mode 100644 backend/tests/test_youtube_refusals.py diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py index 7223ce93..9154ae4c 100644 --- a/backend/app/routes/_common.py +++ b/backend/app/routes/_common.py @@ -13,6 +13,7 @@ from sqlalchemy.orm import Session from app.db import Base from app.models import User +from app.youtube.client import YouTubeError, refusal_reason M = TypeVar("M", bound=Base) @@ -51,3 +52,12 @@ def owned_or_404( if getattr(obj, owner_attr) != user.id: raise not_found(what) return obj + + +def youtube_refusal(exc: YouTubeError) -> HTTPException: + """The answer to an action YouTube refused: a 422 whose reason the UI translates + (`errors.youtube.`). One definition, because the routes had grown three shapes of it — + a bare English string quoting the upstream message, a playlist-scoped reason, and nothing at + all (a raw 500 from the manual syncs). The upstream failure is already logged where it + happened (`youtube.api.error`).""" + return HTTPException(status_code=422, detail={"code": "youtube", "reason": refusal_reason(exc)}) diff --git a/backend/app/routes/channels.py b/backend/app/routes/channels.py index 604bfaf2..10495618 100644 --- a/backend/app/routes/channels.py +++ b/backend/app/routes/channels.py @@ -25,7 +25,7 @@ from app.models import ( User, Video, ) -from app.routes._common import get_or_404, not_found, owned_or_404 +from app.routes._common import get_or_404, not_found, owned_or_404, youtube_refusal from app.sync.explore import explore_ingest_page from app.sync.runner import backfill_channel_now, run_recent_backfill from app.sync.subscriptions import apply_channel_details @@ -424,7 +424,7 @@ def explore_channel( channel = db.get(Channel, channel_id) result = explore_ingest_page(db, yt, channel, page_token) except YouTubeError as exc: - raise HTTPException(status_code=422, detail=f"YouTube couldn't load this channel: {exc}") + raise youtube_refusal(exc) return { "channel_id": channel_id, "ingested": result["ingested"], @@ -583,9 +583,7 @@ def subscribe( if "subscriptionDuplicate" in exc.reason.split(","): yt_sub_id = "" else: - raise HTTPException( - status_code=422, detail=f"YouTube couldn't subscribe you: {exc}" - ) + raise youtube_refusal(exc) # Enrich a stub channel (title/thumbnail/subscriber count) so it shows up properly right # away, on BOTH the normal and the desync path — a resurfaced stub still needs metadata to # render. Keep the client open (we're still inside the `with`); a metadata failure here @@ -649,7 +647,7 @@ def unsubscribe( except YouTubeError as exc: # 422 (a real, explainable error → speaking modal), not 502 which the client treats as # a transient "connection lost" gateway blip. - raise HTTPException(status_code=422, detail=f"YouTube couldn't unsubscribe you: {exc}") + raise youtube_refusal(exc) db.delete(sub) db.commit() return {"unsubscribed": channel_id} diff --git a/backend/app/routes/feed.py b/backend/app/routes/feed.py index 9c2dac8d..b38bbdb1 100644 --- a/backend/app/routes/feed.py +++ b/backend/app/routes/feed.py @@ -29,7 +29,7 @@ from app.models import ( Video, VideoState, ) -from app.routes._common import get_or_404 +from app.routes._common import get_or_404, youtube_refusal from app.sync.videos import parse_iso8601_duration from app.utils import iso from app.youtube.client import YouTubeClient, YouTubeError @@ -411,7 +411,7 @@ def get_video_detail( ): items = yt.get_videos([video_id]) except YouTubeError as exc: - raise HTTPException(status_code=422, detail=f"YouTube lookup failed: {exc}") + raise youtube_refusal(exc) if not items: raise HTTPException(status_code=404, detail="Unknown video") diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index c7617c50..718de2a9 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -23,7 +23,7 @@ from app.sync.playlists import ( sync_user_playlists, ) from app.ratelimit import RateLimiter -from app.routes._common import get_or_404, owned_or_404 +from app.routes._common import get_or_404, owned_or_404, youtube_refusal from app.youtube.client import YouTubeClient, YouTubeError router = APIRouter(prefix="/api/playlists", tags=["playlists"]) @@ -322,8 +322,12 @@ def sync_youtube( if not has_read_scope(user): raise _refused(403, "youtube_read_scope") _check_pull_cooldown(user) - with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PULL): - return sync_user_playlists(db, user) + try: + with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PULL): + return sync_user_playlists(db, user) + except YouTubeError as exc: + db.rollback() + raise youtube_refusal(exc) @router.post("/{playlist_id}/revert-youtube") @@ -344,9 +348,9 @@ def revert_youtube( try: with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PULL): return repull_playlist(db, user, pl) - except YouTubeError: + except YouTubeError as exc: db.rollback() - raise _refused(422, "youtube_failed") + raise youtube_refusal(exc) def _pushable(db: Session, user: User, playlist_id: int) -> Playlist: @@ -371,8 +375,8 @@ def push_plan( try: with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH): plan = plan_merge(db, user, pl) - except YouTubeError: - raise _refused(422, "youtube_failed") + except YouTubeError as exc: + raise youtube_refusal(exc) plan["remaining_today"] = quota.remaining_today(db) plan["affordable"] = plan["units_estimate"] <= plan["remaining_today"] return plan @@ -404,9 +408,9 @@ def push( if plan["units_estimate"] > quota.remaining_today(db): raise _refused(429, "quota_exceeded") return merge_playlist(db, user, pl, live=live) - except YouTubeError: + except YouTubeError as exc: db.rollback() - raise _refused(422, "youtube_failed") + raise youtube_refusal(exc) @router.get("/{playlist_id}") @@ -496,8 +500,8 @@ def delete_playlist( with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_DELETE): with YouTubeClient(db, user) as yt: yt.delete_playlist(pl.yt_playlist_id) - except YouTubeError: - raise _refused(422, "youtube_failed") + except YouTubeError as exc: + raise youtube_refusal(exc) db.delete(pl) # items cascade db.commit() return {"deleted": playlist_id} diff --git a/backend/app/routes/sync.py b/backend/app/routes/sync.py index 8b1e7462..a7a15d1e 100644 --- a/backend/app/routes/sync.py +++ b/backend/app/routes/sync.py @@ -7,10 +7,12 @@ from app.audit import AuditAction from app.auth import admin_user, current_user, has_read_scope, require_human from app.db import get_db from app.models import Channel, Subscription, User, Video +from app.routes._common import youtube_refusal from app.ratelimit import RateLimiter from app.sync.runner import estimate_deep_backfill from app.sync.subscriptions import import_subscriptions from app.scheduler import running_job_ids +from app.youtube.client import YouTubeError router = APIRouter(prefix="/api/sync", tags=["sync"]) @@ -32,8 +34,14 @@ def sync_subscriptions(user: User = Depends(require_human), db: Session = Depend ) if not _sync_limiter.allow(str(user.id)): raise HTTPException(status_code=429, detail={"code": "sync_cooldown"}) - with quota.measured(db, user.id, quota.QuotaAction.SUBSCRIPTIONS_PULL) as m: - result = import_subscriptions(db, user) + try: + with quota.measured(db, user.id, quota.QuotaAction.SUBSCRIPTIONS_PULL) as m: + result = import_subscriptions(db, user) + except YouTubeError as exc: + # The onboarding wizard's first call: an unguarded failure here was a raw 500 and the + # blocking "Server error" dialog, with nothing the user could act on (E9 F1). + db.rollback() + raise youtube_refusal(exc) result["quota_used_estimate"] = m.used result["quota_remaining_today"] = m.remaining return result diff --git a/backend/app/youtube/client.py b/backend/app/youtube/client.py index 381da7a9..0fa9679b 100644 --- a/backend/app/youtube/client.py +++ b/backend/app/youtube/client.py @@ -9,6 +9,7 @@ uses OAuth. import logging from collections.abc import Iterator from datetime import datetime, timedelta, timezone +from typing import Literal import httpx @@ -108,6 +109,22 @@ def is_quota_refusal(exc: YouTubeError) -> bool: return any(r in _QUOTA_REASONS for r in exc.reason.split(",")) +# What a refused YouTube action means to the person who asked for it — the three things they can +# act on differently: wait for tomorrow's budget, reconnect the account, or try again later. +YouTubeRefusal = Literal["quota", "reconnect", "unavailable"] + + +def refusal_reason(exc: YouTubeError) -> YouTubeRefusal: + """Sort a failure by what the user can do about it. The token step failing (Google refused the + refresh, or there is no usable token at all — raised before any endpoint answered, so with no + path and no transport reason) means the account must be reconnected; retrying cannot help.""" + if is_quota_refusal(exc): + return "quota" + if exc.path == "oauth2/token" or (not exc.path and exc.reason != "transport"): + return "reconnect" + return "unavailable" + + def _chunks(items: list, size: int) -> Iterator[list]: for i in range(0, len(items), size): yield items[i : i + size] @@ -181,7 +198,10 @@ class YouTubeClient: }, ) if resp.status_code != 200: - raise _upstream_failure("POST", "oauth2/token", resp) + exc = _upstream_failure("POST", "oauth2/token", resp) + if exc.reason == "invalid_grant": + self._drop_dead_grant(tok) + raise exc data = resp.json() access = data["access_token"] tok.access_token = encrypt(access) @@ -191,6 +211,25 @@ class YouTubeClient: log.info("Refreshed access token for user %s", self.user.id) return access + def _drop_dead_grant(self, tok) -> None: + """Google answered the refresh with `invalid_grant`: the grant was revoked or expired, and no + retry will revive it. Forget it, so the account reads as NOT connected — Settings and the + onboarding wizard then offer the connect button again (a fresh consent issues a new refresh + token). Kept as "granted", the UI showed a live connection with no way to renew it while + every call failed. Committed like the refresh above: the caller rolls back on the error.""" + tok.scopes = None + tok.access_token = None + tok.refresh_token_enc = None + tok.expiry = None + self.db.add(tok) + self.db.commit() + # INFO: the refusal itself is already the warning (`youtube.api.error`, invalid_grant). + log.info( + "cleared the dead YouTube grant of user %s", + self.user.id, + extra=logs.event("youtube.grant.cleared"), + ) + # --- core request --- def _get( self, diff --git a/backend/tests/test_log_pii.py b/backend/tests/test_log_pii.py index 92a52d73..fe523eb9 100644 --- a/backend/tests/test_log_pii.py +++ b/backend/tests/test_log_pii.py @@ -140,9 +140,9 @@ def test_a_refused_token_refresh_names_the_oauth_reason_and_not_its_prose(sink, request=httpx.Request("POST", "https://oauth2.googleapis.com/token"), ) client = yt.YouTubeClient.__new__(yt.YouTubeClient) # no HTTP client: `_send` is the stub - client.db = None + client.db = SimpleNamespace(add=lambda obj: None, commit=lambda: None) client.user = SimpleNamespace( - token=SimpleNamespace(access_token=None, expiry=None, refresh_token_enc=b"x") + id=1, token=SimpleNamespace(access_token=None, expiry=None, refresh_token_enc=b"x") ) monkeypatch.setattr(yt, "decrypt_optional", lambda value: None) monkeypatch.setattr(yt, "decrypt", lambda value: "a-refresh-token") diff --git a/backend/tests/test_youtube_refusals.py b/backend/tests/test_youtube_refusals.py new file mode 100644 index 00000000..724ef8e3 --- /dev/null +++ b/backend/tests/test_youtube_refusals.py @@ -0,0 +1,106 @@ +"""An action YouTube refuses answers a 422 the UI can translate — never a raw 500 — and says what the +user can do about it (`routes/_common.youtube_refusal`). E9 F1: the two manual syncs had no handler +at all, so a quota or transport failure opened the blocking "Server error" dialog.""" + +from types import SimpleNamespace + +import httpx +import pytest + +from app.auth import has_read_scope +from app.models import OAuthToken +from app.youtube import client as yt +from app.youtube.client import YouTubeError, refusal_reason + + +@pytest.mark.parametrize( + ("exc", "reason"), + [ + (YouTubeError("m", reason="quotaExceeded", path="playlists"), "quota"), + (YouTubeError("m", reason="backendError,dailyLimitExceeded", path="videos"), "quota"), + (YouTubeError("m", reason="invalid_grant", path="oauth2/token"), "reconnect"), + (YouTubeError("No refresh token; user must re-authenticate"), "reconnect"), + (YouTubeError("m", reason="transport"), "unavailable"), + (YouTubeError("m", reason="backendError", path="subscriptions"), "unavailable"), + ], +) +def test_a_failure_is_sorted_by_what_the_user_can_do(exc, reason): + assert refusal_reason(exc) == reason + + +@pytest.mark.parametrize( + ("route", "target"), + [ + ("/api/sync/subscriptions", "app.routes.sync.import_subscriptions"), + ("/api/playlists/sync-youtube", "app.routes.playlists.sync_user_playlists"), + ], +) +def test_a_manual_sync_youtube_refuses_is_a_422_not_a_500( + db, as_user, make_user, monkeypatch, route, target +): + user = make_user() + db.commit() + module, name = target.rsplit(".", 1) + + def refused(*a, **k): + raise YouTubeError("GET subscriptions -> 403: quotaExceeded", reason="quotaExceeded") + + monkeypatch.setattr(f"{module}.has_read_scope", lambda u: True) + monkeypatch.setattr(f"{module}.{name}", refused) + + r = as_user(user).post(route) + assert r.status_code == 422 + assert r.json()["detail"] == {"code": "youtube", "reason": "quota"} + + +def test_a_revoked_grant_is_forgotten_so_the_account_can_reconnect(db, make_user, monkeypatch): + """Google's `invalid_grant` is final. Kept, the grant read as live in Settings — with no connect + button — while every call failed; cleared, the connect button comes back.""" + user = make_user() + db.add( + OAuthToken( + user_id=user.id, + refresh_token_enc="x", + scopes="openid https://www.googleapis.com/auth/youtube.readonly", + ) + ) + db.commit() + db.refresh(user) + assert has_read_scope(user) + + resp = httpx.Response( + 400, + json={"error": "invalid_grant", "error_description": "Token has been expired or revoked."}, + request=httpx.Request("POST", yt.GOOGLE_TOKEN_URL), + ) + c = yt.YouTubeClient.__new__(yt.YouTubeClient) # no HTTP client: `_send` is the stub + c.db, c.user = db, user + monkeypatch.setattr(yt, "decrypt", lambda value: "a-refresh-token") + monkeypatch.setattr(yt.sysconfig, "get_str", lambda d, key: "") + monkeypatch.setattr(c, "_send", lambda *a, **k: resp) + + with pytest.raises(YouTubeError) as caught: + c._access_token() + assert refusal_reason(caught.value) == "reconnect" + + db.expire_all() + assert not has_read_scope(db.get(type(user), user.id)) + + +def test_any_other_token_refusal_keeps_the_grant(monkeypatch): + """Only `invalid_grant` is final: a 5xx from the token endpoint is Google having a bad minute.""" + resp = httpx.Response( + 503, json={"error": "backend_error"}, request=httpx.Request("POST", yt.GOOGLE_TOKEN_URL) + ) + tok = SimpleNamespace(access_token=None, expiry=None, refresh_token_enc="x", scopes="s") + c = yt.YouTubeClient.__new__(yt.YouTubeClient) + c.db = SimpleNamespace(add=lambda o: None, commit=lambda: None) + c.user = SimpleNamespace(id=1, token=tok) + monkeypatch.setattr(yt, "decrypt_optional", lambda value: None) + monkeypatch.setattr(yt, "decrypt", lambda value: "a-refresh-token") + monkeypatch.setattr(yt.sysconfig, "get_str", lambda d, key: "") + monkeypatch.setattr(c, "_send", lambda *a, **k: resp) + + with pytest.raises(YouTubeError): + c._access_token() + assert tok.scopes == "s" diff --git a/frontend/src/components/usePlaylistActions.dom.test.tsx b/frontend/src/components/usePlaylistActions.dom.test.tsx index 4b9569d6..7f8b5a50 100644 --- a/frontend/src/components/usePlaylistActions.dom.test.tsx +++ b/frontend/src/components/usePlaylistActions.dom.test.tsx @@ -49,7 +49,7 @@ const linked = { id: 4, name: "Mix", kind: "user", yt_playlist_id: "PL1" } as co describe("usePlaylistActions — deleting a list", () => { it("does not toast a failure the request layer already put on screen", async () => { vi.spyOn(globalThis, "fetch").mockResolvedValue( - new Response(JSON.stringify({ detail: { code: "playlist", reason: "youtube_failed" } }), { + new Response(JSON.stringify({ detail: { code: "youtube", reason: "unavailable" } }), { status: 422, headers: { "content-type": "application/json" }, }), diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index bf036a5a..6d4287f2 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -91,5 +91,10 @@ "reload": "Reload", "notifTitle": "Something broke", "notifMessage": "Unexpected error" + }, + "youtube": { + "quota": "Today's YouTube budget for this instance is spent. It resets at midnight Pacific time — try again then.", + "reconnect": "Google no longer accepts this account's YouTube connection. Sign out and sign in with Google again to reconnect it.", + "unavailable": "YouTube didn't accept that just now. Please try again." } } diff --git a/frontend/src/i18n/locales/en/playlists.json b/frontend/src/i18n/locales/en/playlists.json index 9a71c432..f1a6dc46 100644 --- a/frontend/src/i18n/locales/en/playlists.json +++ b/frontend/src/i18n/locales/en/playlists.json @@ -81,7 +81,6 @@ "name_required": "Give the playlist a name.", "youtube_read_scope": "Connect YouTube (read access) to sync your playlists.", "not_on_youtube": "This playlist isn't on YouTube.", - "youtube_failed": "YouTube didn't accept that just now. Please try again.", "watch_later_not_pushable": "The Watch later list can't be sent to YouTube.", "watch_later_undeletable": "The Watch later list can't be deleted.", "youtube_editing_off": "Enable YouTube editing in Settings first.", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index b9c15734..13be5034 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -91,5 +91,10 @@ "reload": "Újratöltés", "notifTitle": "Valami elromlott", "notifMessage": "Váratlan hiba" + }, + "youtube": { + "quota": "Ennek a példánynak elfogyott a mai YouTube-kerete. Csendes-óceáni idő szerint éjfélkor újraindul — akkor próbáld újra.", + "reconnect": "A Google már nem fogadja el ennek a fióknak a YouTube-kapcsolatát. Jelentkezz ki, majd lépj be újra Google-lel az összekötéshez.", + "unavailable": "A YouTube most nem fogadta el. Próbáld újra." } } diff --git a/frontend/src/i18n/locales/hu/playlists.json b/frontend/src/i18n/locales/hu/playlists.json index 95d0f529..8bd0725a 100644 --- a/frontend/src/i18n/locales/hu/playlists.json +++ b/frontend/src/i18n/locales/hu/playlists.json @@ -81,7 +81,6 @@ "name_required": "Adj nevet a lejátszási listának.", "youtube_read_scope": "A listák szinkronizálásához kapcsold össze a YouTube-ot (olvasási hozzáférés).", "not_on_youtube": "Ez a lejátszási lista nincs fent a YouTube-on.", - "youtube_failed": "A YouTube most nem fogadta el. Próbáld újra.", "watch_later_not_pushable": "A „Megnézem később” listát nem lehet a YouTube-ra küldeni.", "watch_later_undeletable": "A „Megnézem később” lista nem törölhető.", "youtube_editing_off": "Előbb engedélyezd a YouTube-szerkesztést a Beállításokban.", diff --git a/frontend/src/lib/api/core.test.ts b/frontend/src/lib/api/core.test.ts index 29239630..57d591f6 100644 --- a/frontend/src/lib/api/core.test.ts +++ b/frontend/src/lib/api/core.test.ts @@ -167,6 +167,23 @@ describe("req — a structured refusal reads in the user's language", () => { detail: localized, }); }); + + it.each(["quota", "reconnect", "unavailable"])( + "says what to do about an action YouTube refused (%s)", + async (reason) => { + // Waiting for tomorrow's budget, reconnecting and retrying are different advice; the manual + // syncs used to answer a raw 500 here, and the other routes the upstream's English. + vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify({ detail: { code: "youtube", reason } }), { + status: 422, + headers: { "content-type": "application/json" }, + }), + ); + await expect(req("/api/sync/subscriptions", { method: "POST" })).rejects.toMatchObject({ + detail: `errors.youtube.${reason}`, + }); + }, + ); }); describe("req — what it has already put in front of the user", () => { diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index 3593f182..cacbf517 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -129,6 +129,10 @@ function localizeDetail(d: unknown): string { return o.reason === "name_taken" ? t("plex.collection.nameTaken") : t("plex.collection.editFailed"); + // An action YouTube refused (routes/_common.youtube_refusal): what the reader can do about it — + // wait for the budget, reconnect, or retry. An unknown reason reads as the retryable one. + if (o?.code === "youtube") + return t(`errors.youtube.${o.reason}`, t("errors.youtube.unavailable")); // A playlist action the server refused (Watch later can't be deleted, YouTube said no, …). if (o?.code === "playlist") return t(`playlists.errors.${o.reason}`, t("errors.generic")); // An admin action on an account or an invite that the server refused: activating an address that From bacd6aec4f2c71d95a32d20aee623deefdd36610 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 06:57:20 +0200 Subject: [PATCH 02/37] fix(plex): one scrobble per finish, and no resume push lands after it Two near-end checkpoints arrive together routinely (the 10 s tick and the debounced seeked flush); both read "not watched" before writing and both scrobbled, bumping Plex's viewCount twice. The transition is now decided by the write: an insert that did not conflict, or an update conditional on the old status. The finish also popped the push-ordering entry that _resume_push_superseded reads, so an older resume push still in flight could land at Plex after the scrobble and revive a resume point on a watched item. It now stamps the entry instead; the existing eviction bounds the map. --- backend/app/routes/plex/playback.py | 72 +++++++++++++++++--------- backend/tests/test_plex_finish.py | 80 +++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 23 deletions(-) create mode 100644 backend/tests/test_plex_finish.py diff --git a/backend/app/routes/plex/playback.py b/backend/app/routes/plex/playback.py index d08b81ca..bd23e609 100644 --- a/backend/app/routes/plex/playback.py +++ b/backend/app/routes/plex/playback.py @@ -17,7 +17,8 @@ from fastapi import ( ) from fastapi.responses import FileResponse from pydantic import BaseModel, ConfigDict, Field -from sqlalchemy import select +from sqlalchemy import select, update +from sqlalchemy.dialects.postgresql import insert as pg_insert from sqlalchemy.orm import Session from app.auth import current_user, resolved_user_id @@ -295,6 +296,37 @@ def _push_watch( ) +def _mark_finished(db: Session, user_id: int, item_id: int, now: datetime) -> bool: + """Mark the item watched (position cleared) and say whether THIS call made it so. Two near-end + checkpoints arrive together routinely (the 10 s tick and the debounced `seeked` flush), and a + read-then-write let both see "not watched" and both scrobble. The transition is decided by the + write itself: an insert that did not conflict, or an update conditional on the old status — + under READ COMMITTED the second writer re-reads the committed row and matches nothing.""" + values = { + "status": "watched", + "watched_at": now, + "position_seconds": 0, + "progress_updated_at": now, + "synced_to_plex": False, + } + inserted = db.execute( + pg_insert(PlexState) + .values(user_id=user_id, item_id=item_id, **values) + .on_conflict_do_nothing(constraint="uq_plex_user_item") + .returning(PlexState.id) + ).first() + if inserted is not None: + return True + mine = (PlexState.user_id == user_id) & (PlexState.item_id == item_id) + if db.execute( + update(PlexState).where(mine, PlexState.status != "watched").values(values) + ).rowcount: + return True + # Already watched: a re-finish still restamps the row, as it always did — just no scrobble. + db.execute(update(PlexState).where(mine).values(values)) + return False + + class ItemProgressIn(BaseModel): # Finite only: JSON parsing here accepts `Infinity`/`NaN`, and the handler's `int()` turned # one into a 500. @@ -335,6 +367,22 @@ def item_progress( duration = int(body.duration_seconds) final = body.final now = datetime.now(timezone.utc) + near_end = duration > 0 and position > duration - _FINISH_MARGIN_S + if near_end: + flipped = _mark_finished(db, user.id, it.id, now) + # The finish supersedes every resume push still in flight for this item: stamping it as the + # latest push makes `_resume_push_superseded` drop them. Popping the entry instead (to keep + # the map small) removed the very guard, and an older resume push could then land at Plex + # AFTER the scrobble and revive a resume point on a watched item. Eviction bounds the map. + with _push_throttle_lock: + _last_push_at[(user.id, it.id)] = now + db.commit() + # Only the checkpoint that flipped the row scrobbles: a later checkpoint of the same finished + # item — or a second one racing this one — would otherwise bump Plex's viewCount again. + if flipped: + _push_watch(background, db, user.id, it.id, rating_key, "watched") + return {"status": "watched", "position_seconds": 0} + st = ( db.execute( select(PlexState).where(PlexState.user_id == user.id, PlexState.item_id == it.id) @@ -342,28 +390,6 @@ def item_progress( .scalars() .first() ) - was_watched = st is not None and st.status == "watched" - - near_end = duration > 0 and position > duration - _FINISH_MARGIN_S - if near_end: - if st is None: - st = PlexState(user_id=user.id, item_id=it.id) - db.add(st) - st.status = "watched" - st.watched_at = now - st.position_seconds = 0 - st.progress_updated_at = now - st.synced_to_plex = False - # Terminal state for resume write-back: drop the throttle/ordering entry so the map doesn't - # retain a dead key for every finished item. - with _push_throttle_lock: - _last_push_at.pop((user.id, it.id), None) - db.commit() - # Immediate on the watched transition; skip a repeat scrobble if it was already watched - # (a later checkpoint of the same finished item would otherwise bump Plex's viewCount again). - if not was_watched: - _push_watch(background, db, user.id, it.id, rating_key, "watched") - return {"status": "watched", "position_seconds": 0} if position < _PROGRESS_MIN_S: if st is not None and st.status == "new": diff --git a/backend/tests/test_plex_finish.py b/backend/tests/test_plex_finish.py new file mode 100644 index 00000000..3958eb1a --- /dev/null +++ b/backend/tests/test_plex_finish.py @@ -0,0 +1,80 @@ +"""The near-end checkpoint that finishes a Plex item. + +- Exactly one scrobble per finish, even when two near-end checkpoints race (the 10 s tick and the + debounced `seeked` flush arrive together): the transition is decided by a conditional write, not a + read taken before it. +- The finish supersedes every resume push still in flight, so none can land at Plex after the + scrobble and revive a resume point on a watched item. +""" + +import threading +from datetime import datetime, timedelta, timezone + +from sqlalchemy import select + +from app.db import SessionLocal +from app.models import PlexItem, PlexLibrary, PlexState +from app.routes.plex import playback + + +def _item(db) -> PlexItem: + lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) + db.add(lib) + db.flush() + item = PlexItem(rating_key="m1", library_id=lib.id, kind="movie", title="A Movie") + db.add(item) + db.commit() + return item + + +def test_two_racing_finishes_flip_the_row_once(db, make_user): + user = make_user() + item = _item(db) + db.add(PlexState(user_id=user.id, item_id=item.id, status="new", position_seconds=5000)) + db.commit() + now = datetime.now(timezone.utc) + + first, second = SessionLocal(), SessionLocal() + try: + assert playback._mark_finished(first, user.id, item.id, now) is True # holds the row lock + out: list[bool] = [] + racer = threading.Thread( + target=lambda: out.append(playback._mark_finished(second, user.id, item.id, now)) + ) + racer.start() + racer.join(0.5) + assert racer.is_alive(), "the second writer must wait on the first one's row lock" + first.commit() + racer.join(5) + second.commit() + finally: + first.close() + second.close() + assert out == [False], "the second checkpoint saw the committed finish: no second scrobble" + + +def test_a_first_ever_finish_inserts_and_counts_once(db, make_user): + user = make_user() + item = _item(db) + now = datetime.now(timezone.utc) + assert playback._mark_finished(db, user.id, item.id, now) is True + db.commit() + assert playback._mark_finished(db, user.id, item.id, now + timedelta(seconds=10)) is False + db.commit() + st = db.execute(select(PlexState).where(PlexState.item_id == item.id)).scalars().one() + assert (st.status, st.position_seconds) == ("watched", 0) + + +def test_the_finish_supersedes_a_resume_push_still_in_flight(db, as_user, make_user, monkeypatch): + user = make_user() + item = _item(db) + monkeypatch.setattr(playback.plex_stream, "touch_session", lambda key: None) + earlier = datetime.now(timezone.utc) - timedelta(seconds=5) + # A resume push was scheduled a moment ago and has not reached Plex yet. + assert playback._should_push_resume(user.id, item.id, earlier, final=True) + + r = as_user(user).post( + "/api/plex/item/m1/progress", json={"position_seconds": 2995, "duration_seconds": 3000} + ) + assert r.json() == {"status": "watched", "position_seconds": 0} + assert playback._resume_push_superseded(user.id, item.id, earlier) is True From 2df45b27d3ad258e5c29a718b1068803ed5f0979 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 06:58:11 +0200 Subject: [PATCH 03/37] fix(plex): a refused finish restores no resume point at the media's own end The rollback of a refused watched mark guarded against Plex's REPORTED duration. On a file shorter than its metadata the end sits outside the finish margin, so playing to the end and having the mark refused stored the very end as a resume point on an unwatched row: a ~99% bar that reopened in the last seconds. The media's own ended flag now decides. --- .../components/PlexPlayer.session.dom.test.tsx | 18 ++++++++++++++++++ frontend/src/lib/plexPlayerHooks.ts | 6 +++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/frontend/src/components/PlexPlayer.session.dom.test.tsx b/frontend/src/components/PlexPlayer.session.dom.test.tsx index 5593d358..d88d0a16 100644 --- a/frontend/src/components/PlexPlayer.session.dom.test.tsx +++ b/frontend/src/components/PlexPlayer.session.dom.test.tsx @@ -420,6 +420,24 @@ describe("leaving an item that was played through", () => { expect(qc.getQueryData(qk.plexItem("A", "en"))?.status).not.toBe("watched"); }); + it("restores nothing when the media ended short of the duration Plex reports", async () => { + // Plex's metadata can overrun the file. Ending at 2900 of a reported 3000 is outside the finish + // margin, so the duration-based guard alone wrote 2900 back as the resume point: an unwatched + // item with a ~99% bar that reopened in its last seconds. The media's own `ended` decides. + prefs({ upNext: false }); + const { progress } = mount({ + details: { A: item("A"), B: item("B") }, + sessions: { A: () => Promise.resolve(direct("A")), B: () => new Promise(() => {}) }, + queue: ["A", "B"], + }); + vi.spyOn(api, "plexSetState").mockRejectedValue(new Error("offline")); + await playingAt("/media/A", 2900); + endMedia(); + await screen.findByText("Title B"); + await tick(); + expect(progress.mock.calls.filter(([id, , , final]) => id === "A" && final)).toEqual([]); + }); + it("does restore the position when the refused finish came from the credits skip", async () => { // The other caller of the same helper captures a MID-episode position, and there the rollback // is the point: without it the viewer loses the stretch they watched. One helper, two callers, diff --git a/frontend/src/lib/plexPlayerHooks.ts b/frontend/src/lib/plexPlayerHooks.ts index 90020698..82f002f7 100644 --- a/frontend/src/lib/plexPlayerHooks.ts +++ b/frontend/src/lib/plexPlayerHooks.ts @@ -215,6 +215,10 @@ export function usePlexProgress(opts: { // Watched badge the server disagreed with (round 5). const pos = Math.floor(absRef.current); const dur = durationRef.current; + // The media's own end, not Plex's reported duration: a file shorter than its metadata (a + // configuration this file documents) ends outside the margin below, and restoring THAT + // position left an unwatched item with a ~99% bar that reopened in its last seconds. + const ended = video?.ended === true; void confirmed?.catch(() => { finishRefused = true; // Restore the resume point the finish replaced — but ONLY where there is one to restore. @@ -224,7 +228,7 @@ export function usePlexProgress(opts: { // performed, through a second route, exactly the write that had just been refused — while // the caller's error toast was already on screen saying it had failed (round 6). The // credits-skip caller captures a mid-episode position and is unaffected. - if (pos > 0 && dur > 0 && pos <= dur - FINISH_MARGIN_S) + if (!ended && pos > 0 && dur > 0 && pos <= dur - FINISH_MARGIN_S) api.plexProgress(id, pos, dur, true).catch(() => {}); // `settle` may already have patched the detail to watched / 0 on the way out. Dropping that // entry rather than invalidating it: an invalidated query with no observer keeps serving its From e74986cc9ad4e4d65faf61cc341fc17660b5b126 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 07:01:18 +0200 Subject: [PATCH 04/37] fix(playlists): a racing move is not a 500; ownership through owned_or_404 everywhere move_item checked "already in the target?" and then re-parented without a lock, so a second tab's add or the scheduler's YouTube merge committing the same video in between hit uq_playlist_video and answered a raw 500. The re-parent now runs in a savepoint and a conflict falls back to the already-present outcome. The target is touched only when the row landed in it (an untouched list was promoted in the rail's "recently changed" sort), and _touch stamps the database clock like the mixin's own onupdate. The canonical-patterns table names owned_or_404 against ad-hoc ownership helpers; six were left (_own_playlist, _own_playlist_or_404, _own_view, _own_tag, notifications._owned, downloads._own_job), two of them hand-rolled with a bare English 404. All call sites now use owned_or_404, and the download/link/notification 404s are structured like the rest. The contract test reads every "what is gone" the routes name, and the YouTube refusal reasons, and requires both languages to name each. --- backend/app/routes/downloads.py | 35 +++++----- backend/app/routes/notifications.py | 14 ++-- backend/app/routes/playlists.py | 64 +++++++++++-------- backend/app/routes/plex/playlists.py | 20 +++--- backend/app/routes/saved_views.py | 8 +-- backend/app/routes/tags.py | 11 ++-- backend/tests/test_playlist_move.py | 46 +++++++++++++ frontend/src/i18n/backendIds.contract.test.ts | 43 ++++++++++++- frontend/src/i18n/locales/en/errors.json | 5 +- frontend/src/i18n/locales/hu/errors.json | 5 +- 10 files changed, 166 insertions(+), 85 deletions(-) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 3077f192..ed479700 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -38,7 +38,7 @@ from app.models import ( MediaAsset, User, ) -from app.routes._common import get_or_404, not_found +from app.routes._common import get_or_404, not_found, owned_or_404 from app.security import hash_password from app.userscope import is_messageable_user, messageable_clauses from app.utils import iso @@ -166,15 +166,8 @@ def _serialize_job(db: Session, job: DownloadJob) -> dict: return _serialize(job, asset) -def _own_job(db: Session, user: User, job_id: int) -> DownloadJob: - job = db.get(DownloadJob, job_id) - if job is None or job.user_id != user.id: - raise HTTPException(status_code=404, detail="Unknown download") - return job - - def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob: - """`_own_job` for a transition that moves its asset's ref count (resume, cancel, delete, redownload) + """The ownership check (`owned_or_404`) for a transition that moves its asset's ref count (resume, cancel, delete, redownload) or writes the status those decide on (pause). The job is re-read under a row lock, so two overlapping requests for one job run one after the other and the second decides on the first's committed result — never both releasing the same hold, and never a pause turning a job a cancel @@ -183,7 +176,7 @@ def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob: The asset is locked BEFORE the job: the order a finishing worker child takes them in (`service.finalize_if_running`), so the two can never deadlock.""" while True: - asset_id = _own_job(db, user, job_id).asset_id + asset_id = owned_or_404(db, DownloadJob, job_id, user, what="download").asset_id if asset_id is not None: db.execute(select(MediaAsset.id).where(MediaAsset.id == asset_id).with_for_update()) job = db.execute( @@ -193,7 +186,7 @@ def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob: .execution_options(populate_existing=True) ).scalar_one_or_none() if job is None: - raise HTTPException(status_code=404, detail="Unknown download") + raise not_found("download") if job.asset_id == asset_id: return job db.rollback() # another request moved the job to another asset meanwhile: lock that one @@ -614,7 +607,7 @@ def update_download_meta( ) -> dict: """Update a download's display metadata: title, channel name + link, and extra reference URLs. All are display-only overrides — the on-disk file keeps its system-decided, id-bound name.""" - job = _own_job(db, user, job_id) + job = owned_or_404(db, DownloadJob, job_id, user, what="download") sent = body.model_fields_set if "display_name" in sent: job.display_name = ((body.display_name or "").strip())[:255] or None @@ -699,7 +692,7 @@ def delete_download( def _accessible_job(db: Session, user: User, job_id: int) -> DownloadJob: job = db.get(DownloadJob, job_id) if job is None: - raise HTTPException(status_code=404, detail="Unknown download") + raise not_found("download") if job.user_id == user.id: return job shared = db.execute( @@ -708,7 +701,7 @@ def _accessible_job(db: Session, user: User, job_id: int) -> DownloadJob: ) ).first() if shared is None: - raise HTTPException(status_code=404, detail="Unknown download") + raise not_found("download") return job @@ -820,7 +813,7 @@ def share_download( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - job = _own_job(db, user, job_id) + job = owned_or_404(db, DownloadJob, job_id, user, what="download") if job.status != "done" or job.asset_id is None: raise HTTPException(status_code=409, detail="Only a finished download can be shared.") target = body.email.strip().lower() @@ -863,7 +856,9 @@ def unshare_download( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - _own_job(db, user, job_id) # ownership guard (404s if not the caller's job) + owned_or_404( + db, DownloadJob, job_id, user, what="download" + ) # ownership guard (404s if not the caller's job) recipient = db.execute( select(User).where(func.lower(User.email) == email.strip().lower()) ).scalar_one_or_none() @@ -925,10 +920,10 @@ def share_recipients( def _own_link(db: Session, user: User, link_id: int) -> DownloadLink: link = db.get(DownloadLink, link_id) if link is None: - raise HTTPException(status_code=404, detail="Unknown link") + raise not_found("link") job = db.get(DownloadJob, link.job_id) if job is None or job.user_id != user.id: - raise HTTPException(status_code=404, detail="Unknown link") + raise not_found("link") return link @@ -944,7 +939,7 @@ def _link_expiry(days: int | None) -> datetime | None: def list_links( job_id: int, user: User = Depends(require_human), db: Session = Depends(get_db) ) -> list[dict]: - _own_job(db, user, job_id) + owned_or_404(db, DownloadJob, job_id, user, what="download") rows = ( db.execute( select(DownloadLink) @@ -975,7 +970,7 @@ def create_link( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - job = _own_job(db, user, job_id) + job = owned_or_404(db, DownloadJob, job_id, user, what="download") if job.status != "done" or job.asset_id is None: raise HTTPException(status_code=409, detail="Only a finished download can be shared.") pw = body.password.strip() diff --git a/backend/app/routes/notifications.py b/backend/app/routes/notifications.py index f5d43aca..33fb61d4 100644 --- a/backend/app/routes/notifications.py +++ b/backend/app/routes/notifications.py @@ -5,7 +5,7 @@ All endpoints use `current_user` (not `require_human`): notifications and their state are personal UI state with no quota or YouTube involvement, so the shared demo account manages its own inbox normally.""" -from fastapi import APIRouter, Depends, HTTPException, Query +from fastapi import APIRouter, Depends, Query from sqlalchemy import func, select, update from sqlalchemy.orm import Session @@ -14,6 +14,7 @@ from app.db import get_db from app.models import Notification, User from app.notifications import trim_read from app.utils import iso +from app.routes._common import owned_or_404 router = APIRouter(prefix="/api/me/notifications", tags=["notifications"]) @@ -72,20 +73,13 @@ def unread_count(user: User = Depends(current_user), db: Session = Depends(get_d return {"count": count or 0} -def _owned(db: Session, notification_id: int, user: User) -> Notification: - n = db.get(Notification, notification_id) - if n is None or n.user_id != user.id: - raise HTTPException(status_code=404, detail="Notification not found") - return n - - @router.post("/{notification_id}/read") def mark_read( notification_id: int, user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - n = _owned(db, notification_id, user) + n = owned_or_404(db, Notification, notification_id, user, what="notification") n.read = True db.commit() trim_read(db, user.id) @@ -110,7 +104,7 @@ def dismiss( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - n = _owned(db, notification_id, user) + n = owned_or_404(db, Notification, notification_id, user, what="notification") # Dismiss implies read (it no longer counts toward the unread badge). n.dismissed = True n.read = True diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 718de2a9..646dcf3d 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -3,11 +3,10 @@ CRUD (create / rename / delete, add / remove / reorder items) plus YouTube two-w mirror the user's own YouTube playlists in (`/sync-youtube`) and push/revert local edits back out (`/{id}/push-plan`, `/{id}/revert-youtube`), behind the optional write scope.""" -from datetime import datetime, timezone - from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel from sqlalchemy import func, select, and_ +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session, aliased from app import quota @@ -48,10 +47,6 @@ def _check_pull_cooldown(user: User) -> None: raise HTTPException(status_code=429, detail={"code": "sync_cooldown"}) -def _own_playlist(db: Session, user: User, playlist_id: int) -> Playlist: - return owned_or_404(db, Playlist, playlist_id, user, what="playlist") - - def _refused(status: int, reason: str) -> HTTPException: """A playlist action the server refuses, as a reason the UI translates (`playlists.errors.`) — a bare English detail reached the Hungarian UI verbatim.""" @@ -64,8 +59,11 @@ def _touch(pl: Playlist) -> None: `UpdatedAtMixin`'s `onupdate` fires on an UPDATE of the PLAYLIST row, and add / remove / reorder / move write the item rows instead — so without this the rail's "recently changed" sort would only ever see renames. The assignment is what makes the column mean "last time - this list changed", which is what the rail promises the reader.""" - pl.updated_at = datetime.now(timezone.utc) + this list changed", which is what the rail promises the reader. + + The DATABASE's clock, like the mixin's own `onupdate`: one sort key written from two clocks + (this process's and Postgres's) could order a rename and an item edit the wrong way round.""" + pl.updated_at = func.now() def _next_playlist_position(db: Session, user_id: int) -> int: @@ -339,7 +337,7 @@ def revert_youtube( """Reset one YouTube-linked playlist to its current YouTube state, discarding local edits (and clearing the dirty flag). The user-facing 'undo all my changes' for a list once the in-session undo history is gone.""" - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") if not pl.yt_playlist_id: raise _refused(400, "not_on_youtube") if not has_read_scope(user): @@ -356,7 +354,7 @@ def revert_youtube( def _pushable(db: Session, user: User, playlist_id: int) -> Playlist: """A local playlist the user may push to YouTube (create or update). The built-in Watch later and read-only YouTube mirrors are never pushable.""" - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") if pl.kind == "watch_later": raise _refused(400, "watch_later_not_pushable") return pl @@ -419,7 +417,7 @@ def get_playlist( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") state = aliased(VideoState) rows = db.execute( select(*feed_columns(user, state)) @@ -451,7 +449,7 @@ def rename_playlist( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") if "name" in body.model_fields_set: name = (body.name or "").strip() if not name: @@ -488,7 +486,7 @@ def delete_playlist( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") if pl.kind == "watch_later": raise _refused(400, "watch_later_undeletable") if on_youtube: @@ -514,7 +512,7 @@ def add_item( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") video_id = body.video_id get_or_404(db, Video, video_id, what="video") _add_item(db, pl, video_id, mark_dirty=True) @@ -528,7 +526,7 @@ def remove_item( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") _remove_item(db, pl, video_id, mark_dirty=True) return {"playlist_id": pl.id, "video_id": video_id} @@ -552,7 +550,7 @@ def move_item( exactly the kind of half-applied state this codebase treats as a bug rather than an edge case. Idempotent: if the video is already in the target, the source row is simply dropped (moving into a list that already has it is a removal, not a duplicate).""" - src = _own_playlist(db, user, playlist_id) + src = owned_or_404(db, Playlist, playlist_id, user, what="playlist") # Its own reason: "this playlist doesn't exist any more" about a TARGET that is gone was read as # the list on screen — which is still there. dst = owned_or_404(db, Playlist, body.target_playlist_id, user, what="move_target") @@ -575,19 +573,31 @@ def move_item( PlaylistItem.playlist_id == dst.id, PlaylistItem.video_id == video_id ) ) - if already is not None: - db.delete(item) # the uq_playlist_video constraint would reject a second row anyway - else: + dst_changed = False + if already is None: # Re-parent the existing row rather than delete+insert: `added_at` ("when I saved this") - # survives the move, and there is no window where the video belongs to neither list. - item.playlist_id = dst.id - item.position = _next_item_position(db, dst.id) - # Both sides changed, so both get the dirty recompute and the timestamp — the asymmetric - # version of this (touching only the source) is the recurring smell CLAUDE.md warns about. + # survives the move, and there is no window where the video belongs to neither list. The + # check above is not a lock: a second tab's add or the scheduler's YouTube merge can insert + # the same (playlist, video) in between, and uq_playlist_video then refuses the re-parent. + # That is the already-present outcome arriving late, not a failure — answer it the same way + # instead of a raw 500. + try: + with db.begin_nested(): + item.playlist_id = dst.id + item.position = _next_item_position(db, dst.id) + db.flush() + dst_changed = True + except IntegrityError: + pass # the savepoint put the row back in the source list; drop it below + if not dst_changed: + db.delete(item) + # The source always changed; the target only when the row landed in it. `_touch` on a target + # that already had the video promoted an untouched list in the rail's "recently changed" sort. recompute_dirty(db, src) - recompute_dirty(db, dst) _touch(src) - _touch(dst) + if dst_changed: + recompute_dirty(db, dst) + _touch(dst) db.commit() return {"ok": True, "from_playlist_id": src.id, "to_playlist_id": dst.id, "video_id": video_id} @@ -603,7 +613,7 @@ def reorder_items( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") order = body.video_ids items = { it.video_id: it diff --git a/backend/app/routes/plex/playlists.py b/backend/app/routes/plex/playlists.py index 022af3ae..9728b1d3 100644 --- a/backend/app/routes/plex/playlists.py +++ b/backend/app/routes/plex/playlists.py @@ -24,10 +24,6 @@ from app.routes._common import owned_or_404 router = APIRouter(prefix="/api/plex", tags=["plex"]) -def _own_playlist_or_404(db: Session, pid: int, user: User) -> PlexPlaylist: - return owned_or_404(db, PlexPlaylist, pid, user, what="playlist") - - def _playlist_card(pl: PlexPlaylist, count: int, thumb_rk: str | None) -> dict: return { "id": pl.id, @@ -140,7 +136,7 @@ def create_playlist( def playlist_detail( pid: int, user: User = Depends(current_user), db: Session = Depends(get_db) ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") rows = db.execute(_playlist_item_query(pid)).all() items = [it for (_pi, it) in rows] states = { @@ -183,7 +179,7 @@ def rename_playlist( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") title = body.title.strip() if not title: raise HTTPException(status_code=400, detail="title is required") @@ -203,7 +199,7 @@ def rename_playlist( def delete_playlist( pid: int, user: User = Depends(current_user), db: Session = Depends(get_db) ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") db.delete(pl) db.commit() return {"deleted": pid} @@ -220,7 +216,7 @@ def playlist_add_item( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") it = by_rating_key(db, PlexItem, body.item_rating_key) if it is None: raise HTTPException(status_code=404, detail="Unknown item") @@ -252,7 +248,7 @@ def playlist_remove_item( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") it = by_rating_key(db, PlexItem, item_rating_key) if it is not None: db.execute( @@ -297,7 +293,7 @@ def playlist_add_bulk( ) -> dict: """Append many items at once (a whole season/show). Already-present items are skipped; the input order is preserved for the newly-added ones. Returns how many were added + the new total.""" - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") items = _items_by_rks(db, body.item_rating_keys) existing = set( db.execute( @@ -340,7 +336,7 @@ def playlist_remove_bulk( ) -> dict: """Remove many items at once (a whole season/show, or a multi-select). Returns how many were removed + the new total.""" - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") ids = [it.id for it in _items_by_rks(db, body.item_rating_keys)] removed = 0 if ids: @@ -370,7 +366,7 @@ def reorder_playlist( db: Session = Depends(get_db), ) -> dict: """Set the playlist order from an explicit list of item rating_keys (0-based positions).""" - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") order = body.item_rating_keys id_by_rk = { it.rating_key: it.id diff --git a/backend/app/routes/saved_views.py b/backend/app/routes/saved_views.py index e2d6f68e..130b7aec 100644 --- a/backend/app/routes/saved_views.py +++ b/backend/app/routes/saved_views.py @@ -90,10 +90,6 @@ def create_view( return _serialize(view) -def _own_view(db: Session, user: User, view_id: int) -> SavedView: - return owned_or_404(db, SavedView, view_id, user, what="view") - - class UpdateViewIn(BaseModel): # PATCH: presence decides what's touched (read via model_fields_set). filters, when sent, # must be a real object — an explicit null is rejected (a view can't have null filters). @@ -109,7 +105,7 @@ def update_view( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - view = _own_view(db, user, view_id) + view = owned_or_404(db, SavedView, view_id, user, what="view") sent = body.model_fields_set if "name" in sent: name = (body.name or "").strip() @@ -148,7 +144,7 @@ def delete_view( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - view = _own_view(db, user, view_id) + view = owned_or_404(db, SavedView, view_id, user, what="view") db.delete(view) db.commit() return {"deleted": view_id} diff --git a/backend/app/routes/tags.py b/backend/app/routes/tags.py index d5b8c8d8..32165fd3 100644 --- a/backend/app/routes/tags.py +++ b/backend/app/routes/tags.py @@ -78,11 +78,6 @@ def create_tag( return {"id": tag.id, "name": tag.name, "color": tag.color, "category": tag.category} -def _own_tag(db: Session, user: User, tag_id: int) -> Tag: - # System tags (user_id NULL) are not user-editable → owned_or_404 404s them too. - return owned_or_404(db, Tag, tag_id, user, what="tag") - - class TagUpdateIn(BaseModel): # PATCH: presence matters, and `color: null` explicitly CLEARS it — so the endpoint reads # model_fields_set rather than testing for None (which couldn't tell absent from cleared). @@ -97,7 +92,8 @@ def update_tag( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - tag = _own_tag(db, user, tag_id) + # System tags (user_id NULL) are not user-editable: owned_or_404 404s them too. + tag = owned_or_404(db, Tag, tag_id, user, what="tag") sent = body.model_fields_set if "name" in sent: name = (body.name or "").strip() @@ -120,7 +116,8 @@ def delete_tag( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - tag = _own_tag(db, user, tag_id) + # System tags (user_id NULL) are not user-editable: owned_or_404 404s them too. + tag = owned_or_404(db, Tag, tag_id, user, what="tag") db.delete(tag) # ChannelTag rows cascade on tag delete. db.commit() return {"deleted": tag_id} diff --git a/backend/tests/test_playlist_move.py b/backend/tests/test_playlist_move.py index 9a936d32..8bf1de21 100644 --- a/backend/tests/test_playlist_move.py +++ b/backend/tests/test_playlist_move.py @@ -242,3 +242,49 @@ def test_the_list_endpoint_publishes_updated_at(db, make_user, as_user): rows = as_user(user).get("/api/playlists").json() assert len(rows) == 1 assert rows[0]["updated_at"] # the rail cannot sort by a field the API never sends + + +def test_a_target_that_already_had_it_is_not_recently_changed(db, make_user, as_user): + """Moving into a list that already holds the video only removes it from the source: the + target did not change, and bumping it promoted an untouched list in the rail's sort.""" + user = make_user() + _seed_videos(db, ["v1"]) + src = _playlist(db, user, "Source", ["v1"]) + dst = _playlist(db, user, "Target", ["v1"]) + old = datetime.now(timezone.utc) - timedelta(days=3) + src.updated_at = old + dst.updated_at = old + db.commit() + + assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200 + db.expire_all() + assert db.get(Playlist, src.id).updated_at > old + assert db.get(Playlist, dst.id).updated_at == old + + +def test_a_concurrent_add_to_the_target_lands_as_the_already_present_move( + db, make_user, as_user, monkeypatch +): + """The already-in-target check is a read, not a lock: another tab's add (or the scheduler's + YouTube merge) can commit the same video to the target in between, and the re-parent then + hits uq_playlist_video. That is the idempotent outcome arriving late — not a raw 500.""" + from app.db import SessionLocal + from app.routes import playlists as plroutes + + user = make_user() + _seed_videos(db, ["v1"]) + src = _playlist(db, user, "Source", ["v1"]) + dst = _playlist(db, user, "Target", []) + real = plroutes._next_item_position + + def add_first(session, playlist_id): + with SessionLocal() as other: # the other tab, committing between check and re-parent + other.add(PlaylistItem(playlist_id=dst.id, video_id="v1", position=0)) + other.commit() + return real(session, playlist_id) + + monkeypatch.setattr(plroutes, "_next_item_position", add_first) + assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200 + db.expire_all() + assert _ids(db, src.id) == [] + assert _ids(db, dst.id) == ["v1"] diff --git a/frontend/src/i18n/backendIds.contract.test.ts b/frontend/src/i18n/backendIds.contract.test.ts index 867c7658..d286a5d2 100644 --- a/frontend/src/i18n/backendIds.contract.test.ts +++ b/frontend/src/i18n/backendIds.contract.test.ts @@ -1,4 +1,4 @@ -import { readFileSync } from "node:fs"; +import { readdirSync, readFileSync } from "node:fs"; import { describe, expect, it } from "vitest"; // Every id the BACKEND owns and the UI names must have its name in both languages. The UI looks these @@ -86,6 +86,32 @@ const failDetails = [ ]; const DETAIL = "downloads.errors.fail.catalog."; +// What a structured 404 says is gone → `errors.notFound.`: every `not_found("x")` and every +// `owned_or_404(..., what="x")` / `get_or_404(..., what="x")` under routes/, read from the sources. +const routeSources = (dir = "routes"): string[] => + readdirSync(new URL(`../../../backend/app/${dir}/`, import.meta.url), { + withFileTypes: true, + }).flatMap((e) => + e.isDirectory() + ? routeSources(`${dir}/${e.name}`) + : e.name.endsWith(".py") + ? [backend(`${dir}/${e.name}`)] + : [], + ); +const goneWhats = [ + ...new Set( + routeSources().flatMap((src) => + [...src.matchAll(/not_found\("(\w+)"\)|what="(\w+)"/g)].map((m) => (m[1] ?? m[2])!), + ), + ), +]; +// A refused YouTube action → `errors.youtube.`, the reasons read from their Literal. +const youtubeReasons = [ + ...( + backend("youtube/client.py").match(/^YouTubeRefusal = Literal\[([^\]]*)\]/m)?.[1] ?? "" + ).matchAll(/"(\w+)"/g), +].map((m) => m[1]!); + /** The ids under `prefix` a locale names, `prefix.` or `prefix..`. */ const idsUnder = (keys: Set, prefix: string) => new Set( @@ -106,6 +132,8 @@ describe("backend ids have their names in both languages", () => { expect(quotaActions.length).toBeGreaterThan(0); expect(adminCodes).toEqual(expect.arrayContaining(["activate", "setpw"])); expect(failDetails).toEqual(expect.arrayContaining(["facebook.login_wall"])); + expect(goneWhats).toEqual(expect.arrayContaining(["playlist", "download", "job"])); + expect(youtubeReasons).toEqual(["quota", "reconnect", "unavailable"]); // Every code the frontend localizes is raised somewhere the scan can see. expect(adminCodes.filter((c) => !adminRefusals.some((m) => m[1] === c))).toEqual([]); }); @@ -159,6 +187,19 @@ describe("backend ids have their names in both languages", () => { expect([...named].filter((d) => !failDetails.includes(d))).toEqual([]); }); + it(`${lang}: every "that is gone" a 404 names, and every YouTube refusal`, () => { + expect(missing(goneWhats.map((w) => `errors.notFound.${w}`))).toEqual([]); + expect( + [...idsUnder(keys, "errors.notFound")].filter( + (w) => w !== "generic" && !goneWhats.includes(w), + ), + ).toEqual([]); + expect(missing(youtubeReasons.map((r) => `errors.youtube.${r}`))).toEqual([]); + expect( + [...idsUnder(keys, "errors.youtube")].filter((r) => !youtubeReasons.includes(r)), + ).toEqual([]); + }); + it(`${lang}: every admin refusal reason`, () => { expect(missing(adminReasons.map((r) => `errors.admin.${r}`))).toEqual([]); expect([...idsUnder(keys, "errors.admin")].filter((r) => !adminReasons.includes(r))).toEqual( diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index 6d4287f2..06f42c1b 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -18,7 +18,10 @@ "channel": "This channel isn't in the library any more.", "subscription": "You're not subscribed to this channel any more.", "recipient": "There's no user with that email address.", - "job": "This job doesn't exist any more." + "job": "This job doesn't exist any more.", + "download": "This download doesn't exist any more.", + "link": "This share link doesn't exist any more.", + "notification": "This notification doesn't exist any more." }, "admin": { "not_allowed": "Their address isn't approved any more. Add it to the whitelist first.", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index 13be5034..6a5c8887 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -18,7 +18,10 @@ "channel": "Ez a csatorna már nincs a könyvtárban.", "subscription": "Már nem vagy feliratkozva erre a csatornára.", "recipient": "Nincs felhasználó ezzel az e-mail címmel.", - "job": "Ez a feladat már nem létezik." + "job": "Ez a feladat már nem létezik.", + "download": "Ez a letöltés már nem létezik.", + "link": "Ez a megosztási link már nem létezik.", + "notification": "Ez az értesítés már nem létezik." }, "admin": { "not_allowed": "A címe már nincs jóváhagyva. Előbb vedd fel a fehérlistára.", From e6b76e156a1da53ab45cb017d91d26c946f9a4bd Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 07:03:35 +0200 Subject: [PATCH 05/37] refactor(discover): one shelf query per kind, one RAIL_LIMIT, a true rollup comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comfort shelf resolved each pinned channel with three queries (the row, its unwatched count, its newest video) and each playlist with two. channels_summary gets the count and the newest row per channel as window functions over one filtered set, playlists_summary reads every pinned list in one query, and the rows come in one IN() each — so the shelf's cost no longer grows with its length. A test counts the statements. RAIL_LIMIT had two definitions held in step by a comment; the Plex Discover route now imports the domain's. And rollup's "continue the latest-started episode" was false: it takes the last in-progress one in broadcast order, which is also what Home's next_episode picks. --- backend/app/discover/pins.py | 53 +++++++++++++++----- backend/app/discover/queries.py | 75 +++++++++++++++++------------ backend/app/plex/rollup.py | 4 +- backend/app/routes/plex/discover.py | 2 +- backend/tests/test_discover.py | 67 ++++++++++++++++++++++++++ 5 files changed, 157 insertions(+), 44 deletions(-) diff --git a/backend/app/discover/pins.py b/backend/app/discover/pins.py index 7a49ca47..98a6a0f9 100644 --- a/backend/app/discover/pins.py +++ b/backend/app/discover/pins.py @@ -8,14 +8,16 @@ second one. Every shelf card carries a resolved `next`: the thing that actually plays when you click the tile — a show's in-progress-or-next episode, a channel's newest unwatched video, a playlist's first -unwatched item. It costs a query per non-leaf pin, which is affordable precisely because this shelf is -short and hand-curated, and it buys the property the whole surface exists for: one click, no -intermediate page, no second decision. +unwatched item. That buys the property the whole surface exists for: one click, no intermediate page, +no second decision. Channels and playlists are resolved for the whole shelf at once (`_Batch`), so the +cost does not grow a query or two per pin. Domain module, so nothing here raises HTTP. `resolve_ref` returns None for "no such thing, or not yours" and the route turns that into its 404 — core never imports routes. """ +from dataclasses import dataclass, field + from sqlalchemy import func, select from sqlalchemy.orm import Session @@ -103,7 +105,34 @@ def next_position(db: Session, user: User) -> int: ) -def _shelf_card(db: Session, user: User, pin: ComfortPin) -> dict | None: +@dataclass +class _Batch: + """What the shelf resolves for every pinned channel and playlist in one go.""" + + channels: dict[str, Channel] = field(default_factory=dict) + channel_summary: dict[str, tuple[int, dict | None]] = field(default_factory=dict) + playlists: dict[int, Playlist] = field(default_factory=dict) + playlist_summary: dict[int, tuple[int, dict | None]] = field(default_factory=dict) + + @classmethod + def load(cls, db: Session, user: User, pins: list[ComfortPin]) -> "_Batch": + ch_ids = [p.channel_id for p in pins if p.channel_id is not None] + pl_ids = [p.playlist_id for p in pins if p.playlist_id is not None] + b = cls() + if ch_ids: + b.channels = { + c.id: c for c in db.scalars(select(Channel).where(Channel.id.in_(ch_ids))) + } + b.channel_summary = queries.channels_summary(db, user, ch_ids) + if pl_ids: + b.playlists = { + p.id: p for p in db.scalars(select(Playlist).where(Playlist.id.in_(pl_ids))) + } + b.playlist_summary = queries.playlists_summary(db, user, pl_ids) + return b + + +def _shelf_card(db: Session, user: User, pin: ComfortPin, batch: _Batch) -> dict | None: """One pin as a unified card with its resolved `next`. None when the pinned row has gone (the FK cascade removes the pin itself, so this is only the window between a delete and a refetch).""" card: dict | None = None @@ -117,11 +146,11 @@ def _shelf_card(db: Session, user: User, pin: ComfortPin) -> dict | None: nxt = dict(card) # a video plays itself elif pin.channel_id is not None: - ch = db.get(Channel, pin.channel_id) + ch = batch.channels.get(pin.channel_id) if ch is None: return None - card = cards.from_channel(ch, queries.channel_unwatched_count(db, user, ch.id)) - row = queries.channel_next_video(db, user, ch.id) + count, row = batch.channel_summary.get(ch.id, (0, None)) + card = cards.from_channel(ch, count) nxt = cards.from_video(row) if row else None elif pin.plex_item_id is not None: @@ -142,10 +171,10 @@ def _shelf_card(db: Session, user: User, pin: ComfortPin) -> dict | None: nxt = cards.from_plex(ep) if ep else None elif pin.playlist_id is not None: - pl = db.get(Playlist, pin.playlist_id) + pl = batch.playlists.get(pin.playlist_id) if pl is None: return None - count, row = queries.playlist_summary(db, user, pl.id) + count, row = batch.playlist_summary.get(pl.id, (0, None)) card = cards.from_playlist(pl, count, row.get("thumbnail_url") if row else None) nxt = cards.from_video(row) if row else None @@ -162,8 +191,10 @@ def _shelf_card(db: Session, user: User, pin: ComfortPin) -> dict | None: def shelf(db: Session, user: User) -> list[dict]: """The whole shelf, in the user's own order.""" out = [] - for pin in list_pins(db, user): - card = _shelf_card(db, user, pin) + pins = list_pins(db, user) + batch = _Batch.load(db, user, pins) + for pin in pins: + card = _shelf_card(db, user, pin, batch) if card is not None: out.append(card) return out diff --git a/backend/app/discover/queries.py b/backend/app/discover/queries.py index 403bd8c9..8b6e82f7 100644 --- a/backend/app/discover/queries.py +++ b/backend/app/discover/queries.py @@ -43,8 +43,9 @@ from app.plex import rollup from app.plex.cards import leaf_card, movie_card, show_card from app.plex.query import apply_show_state_filter, content_libs -# How many rows a single rail may carry. Matches `routes/plex/discover.py`'s RAIL_LIMIT on purpose — -# the two surfaces should feel like the same app, and a rail is a rail. +# How many rows a single rail may carry — Home's rails and the Plex module's Discover rails alike +# (`routes/plex/discover.py` imports it): the two surfaces should feel like the same app, and a rail +# is a rail. One definition, because a comment was all that held two copies in step. RAIL_LIMIT = 20 # The feed parameters a Home rail runs with. Spelled out rather than borrowed from the route's @@ -129,18 +130,6 @@ def yt_surprise(db: Session, user: User, budget: str, seed: int, limit: int) -> return [r for r in out if not r["position_seconds"]][:limit] -def channel_next_video(db: Session, user: User, channel_id: str) -> dict | None: - """The newest unwatched video of a pinned channel — what "put on that channel" resolves to.""" - # `library_source="all"`: a pinned channel means the whole channel, including videos that only - # ever arrived through a search. The feed's "organic" default is a feed-scoping choice, not a - # statement about what belongs to a channel. - params = dict(_FEED_BASE, channel_ids=[channel_id], scope="all", library_source="all") - query, rank_expr = filtered_query(db, user, **params) - keys = sort_keys("newest", 0, rank_expr) - row = db.execute(query.order_by(*order_by(keys)).limit(1)).first() - return serialize(row) if row is not None else None - - def yt_video_row(db: Session, user: User, video_id: str) -> dict | None: """One video as a feed row, with this user's watch state. @@ -158,31 +147,55 @@ def yt_video_row(db: Session, user: User, video_id: str) -> dict | None: return serialize(row) if row is not None else None -def channel_unwatched_count(db: Session, user: User, channel_id: str) -> int: - """How many unwatched videos a pinned channel is holding — the tile's only honest promise, since a - channel has no runtime of its own.""" - params = dict(_FEED_BASE, channel_ids=[channel_id], scope="all", library_source="all") - query, _ = filtered_query(db, user, **params) - return int(db.execute(select(func.count()).select_from(query.subquery())).scalar() or 0) +def channels_summary( + db: Session, user: User, channel_ids: list[str] +) -> dict[str, tuple[int, dict | None]]: + """For each pinned channel: (unwatched count, its newest unwatched video) — the tile's only honest + promise, since a channel has no runtime of its own, and what "put on that channel" resolves to. + + ONE query for the whole shelf, not two per channel: the count and the first row per channel are + window functions over the same filtered set. `library_source="all"`: a pinned channel means the + whole channel, including videos that only ever arrived through a search — the feed's "organic" + default is a feed-scoping choice, not a statement about what belongs to a channel.""" + if not channel_ids: + return {} + params = dict(_FEED_BASE, channel_ids=list(channel_ids), scope="all", library_source="all") + query, rank_expr = filtered_query(db, user, **params) + newest = order_by(sort_keys("newest", 0, rank_expr)) + per_channel = {"partition_by": Video.channel_id} + sub = query.add_columns( + func.row_number().over(**per_channel, order_by=newest).label("_rn"), + func.count().over(**per_channel).label("_total"), + ).subquery() + rows = db.execute(select(sub).where(sub.c._rn == 1)).all() + return {r.channel_id: (int(r._total), serialize(r)) for r in rows} -def playlist_summary(db: Session, user: User, playlist_id: int) -> tuple[int, dict | None]: - """(item count, the row to play next) for a pinned playlist — its first not-yet-watched item, or - its first item once the whole list is watched. The same row doubles as the tile's artwork, so a - shelf of playlists is scannable instead of a row of blank boxes.""" +def playlists_summary( + db: Session, user: User, playlist_ids: list[int] +) -> dict[int, tuple[int, dict | None]]: + """For each pinned playlist: (item count, the row to play next) — its first not-yet-watched item, + or its first item once the whole list is watched. The same row doubles as the tile's artwork, so + a shelf of playlists is scannable instead of a row of blank boxes. One query for the whole shelf.""" + if not playlist_ids: + return {} st = aliased(VideoState) rows = db.execute( - select(*feed_columns(user, st), PlaylistItem.position) + select(*feed_columns(user, st), PlaylistItem.playlist_id.label("_playlist")) .join(PlaylistItem, PlaylistItem.video_id == Video.id) .join(Channel, Channel.id == Video.channel_id) .outerjoin(st, and_(st.video_id == Video.id, st.user_id == user.id)) - .where(PlaylistItem.playlist_id == playlist_id) - .order_by(PlaylistItem.position.asc()) + .where(PlaylistItem.playlist_id.in_(playlist_ids)) + .order_by(PlaylistItem.playlist_id, PlaylistItem.position.asc()) ).all() - if not rows: - return 0, None - nxt = next((r for r in rows if (r.status or "new") not in ("watched", "hidden")), rows[0]) - return len(rows), serialize(nxt) + grouped: dict[int, list] = {} + for r in rows: + grouped.setdefault(r._playlist, []).append(r) + out: dict[int, tuple[int, dict | None]] = {} + for pid, items in grouped.items(): + nxt = next((r for r in items if (r.status or "new") not in ("watched", "hidden")), items[0]) + out[pid] = (len(items), serialize(nxt)) + return out # ── Plex ────────────────────────────────────────────────────────────────────────────────────────── diff --git a/backend/app/plex/rollup.py b/backend/app/plex/rollup.py index 53211250..7358338b 100644 --- a/backend/app/plex/rollup.py +++ b/backend/app/plex/rollup.py @@ -43,7 +43,9 @@ def rollup(cards: list[dict]) -> dict: ] resume = None if inprog: - resume = inprog[-1] # continue the latest-started episode + # The LAST in-progress episode in broadcast order — no timestamp is consulted. Home's + # `discover.queries.next_episode` picks the same one, so both Resume buttons agree. + resume = inprog[-1] else: resume = next((c for c in cards if c.get("status") != "watched"), None) return { diff --git a/backend/app/routes/plex/discover.py b/backend/app/routes/plex/discover.py index 3bf13054..6a6283a6 100644 --- a/backend/app/routes/plex/discover.py +++ b/backend/app/routes/plex/discover.py @@ -32,12 +32,12 @@ from app.plex.query import ( meta_filter_conds, top_art_keys, ) +from app.discover.queries import RAIL_LIMIT from app.plex.tmdb import resolve_recs from app.routes.plex._shared import _norm_lang router = APIRouter(prefix="/api/plex", tags=["plex"]) -RAIL_LIMIT = 20 GENRE_TILES = 12 SURPRISE_PER_KIND = 3 # Surprise-me: at most this many movies + this many shows (6 total) diff --git a/backend/tests/test_discover.py b/backend/tests/test_discover.py index 261001b7..a25beb5c 100644 --- a/backend/tests/test_discover.py +++ b/backend/tests/test_discover.py @@ -544,6 +544,73 @@ def test_shelf_resolves_a_playlist_to_its_first_unwatched_item(db): assert card["next"]["id"] == "vmed" +def _count_statements(db, fn) -> int: + """How many SQL statements `fn` sends through the session's connection.""" + from sqlalchemy import event + + seen: list[str] = [] + engine = db.get_bind() + listener = lambda *a, **k: seen.append(a[2]) # noqa: E731 — (conn, cursor, statement, …) + event.listen(engine, "before_cursor_execute", listener) + try: + fn() + finally: + event.remove(engine, "before_cursor_execute", listener) + return len(seen) + + +def test_the_shelf_resolves_each_channel_and_playlist_on_its_own_terms(db): + """Batched, the per-channel count and newest video come from one windowed query: each channel + must still get ITS count and ITS newest, not the shelf's overall first row.""" + user, ch = _seed_yt(db) + other = Channel(id="UCother", title="Other") + db.add(other) + db.flush() + db.add(Video(id="vother", channel_id=other.id, title="x", published_at=NOW - timedelta(days=9))) + pl = Playlist(user_id=user.id, name="Két elem") + empty = Playlist(user_id=user.id, name="Üres") + db.add_all([pl, empty]) + db.flush() + db.add(PlaylistItem(playlist_id=pl.id, video_id="vlong", position=0)) + for i, target in enumerate( + [ + {"channel_id": ch.id}, + {"channel_id": other.id}, + {"playlist_id": pl.id}, + {"playlist_id": empty.id}, + ] + ): + db.add(ComfortPin(user_id=user.id, position=i, **target)) + db.commit() + + got = [ + (c.get("unwatched_count", c.get("item_count")), (c["next"] or {}).get("id")) + for c in pins.shelf(db, user) + ] + assert got == [(3, "vshort"), (1, "vother"), (1, "vlong"), (0, None)] + + +def test_the_shelf_costs_the_same_queries_for_one_pin_or_many(db): + """A channel pin used to cost three queries and a playlist pin two, each; the shelf now + resolves every channel and every playlist together.""" + user, ch = _seed_yt(db) + extra = [Channel(id=f"UCx{i}", title=f"x{i}") for i in range(3)] + lists = [Playlist(user_id=user.id, name=f"p{i}") for i in range(4)] + db.add_all(extra + lists) + db.flush() + db.add(ComfortPin(user_id=user.id, channel_id=ch.id, position=0)) + db.add(ComfortPin(user_id=user.id, playlist_id=lists[0].id, position=1)) + db.commit() + few = _count_statements(db, lambda: pins.shelf(db, user)) + + for i, c in enumerate(extra): + db.add(ComfortPin(user_id=user.id, channel_id=c.id, position=10 + i)) + for i, p in enumerate(lists[1:]): + db.add(ComfortPin(user_id=user.id, playlist_id=p.id, position=20 + i)) + db.commit() + assert _count_statements(db, lambda: pins.shelf(db, user)) == few + + def test_a_pinned_video_survives_leaving_the_feed_scope(db): """The shelf is the fallback surface. An entry silently vanishing because the user unsubscribed from the channel is exactly the failure it must not have.""" From c60359bfedfa7d00e2a027a89fe5db8bc0717326 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 07:07:53 +0200 Subject: [PATCH 06/37] =?UTF-8?q?fix(plex):=20subtitles=20=E2=80=94=20a=20?= =?UTF-8?q?retried=20full=20pass=20resumes,=20a=20stalled=20track=20is=20p?= =?UTF-8?q?olled=20slowly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A full pass that ran out of time kept what it reached, but its retry an hour later started again at 0, so a file too big to read inside the extraction timeout never completed. The pass now starts where every embedded track's coverage from the head ends (less the window pre-roll, so a cue across the join is read whole). The seek needs -avoid_negative_ts on this read as on a window, measured on a real file: without it the cue running at the start shifts the whole output. A partial track whose full pass had failed never reached a settled state, so the player polled it every 2 s for the rest of the film. The status now says whether the track is still growing, and a stalled one is polled every 15 s — still inside the 60 s lookahead that reads the next window before an edge. --- backend/app/plex/subtitles.py | 38 +++++++++-- backend/tests/test_plex_subtitle_prepare.py | 67 +++++++++++++++++++ frontend/src/lib/api/plex.ts | 2 + .../lib/plexPlayerHooks.subtitle.dom.test.tsx | 16 +++++ frontend/src/lib/plexPlayerHooks.ts | 8 ++- 5 files changed, 126 insertions(+), 5 deletions(-) diff --git a/backend/app/plex/subtitles.py b/backend/app/plex/subtitles.py index c09e97ec..67f62cd2 100644 --- a/backend/app/plex/subtitles.py +++ b/backend/app/plex/subtitles.py @@ -642,7 +642,15 @@ def ensure(rating_key: str, sub_ord: int, at: float = 0.0) -> dict: probe = edge + 1.0 want_window = not _window_failed(key, probe) _schedule(rating_key, sub_ord, probe, want_window=want_window, want_full=not full_failed) - return {"state": "partial" if covered else "preparing", "code": None, "gen": gen} + # `growing`: whether the track will fill in on its own. False once the full pass has failed and + # is backed off — a partial track then stays partial until a window is asked for, so the player + # may poll slowly (only to read ahead of an edge) instead of every two seconds for the whole film. + return { + "state": "partial" if covered else "preparing", + "code": None, + "gen": gen, + "growing": not full_failed, + } def cached_track(rating_key: str, sub_ord: int) -> tuple[str | None, str | None]: @@ -822,10 +830,13 @@ def _extract_and_publish(rating_key: str, sub_ord: int, window_start: float | No for i, s in enumerate(subs) if s.get("index") is not None and not _is_bitmap(s) } - start = window_start or 0.0 + # A full pass starts where every track's leading coverage ends: a pass that ran out of time + # (`EXTRACT_TIMEOUT_S`) kept what it reached, and restarting from 0 each `FULL_RETRY_S` meant a file + # too big to read in one go never completed. Windows handle an offset start already; so does this. + start = window_start if window_start is not None else _full_pass_start(rating_key, targets) length = (WINDOW_LEN_S + WINDOW_PRE_S) if window_start is not None else None end = _FULL_END_S if length is None else start + length - texts, clean = _ffmpeg_extract(str(src), sorted(targets), window_start, length) + texts, clean = _ffmpeg_extract(str(src), sorted(targets), start or None, length) for emb_ord, vtt in texts.items(): key = cache_key(rating_key, targets[emb_ord]) # A pass that exited non-zero holds only what ffmpeg reached before it died. Claim no more @@ -846,6 +857,24 @@ def _extract_and_publish(rating_key: str, sub_ord: int, window_start: float | No _pass_failed(rating_key, [key], window_start, (covered_end, end)) +def _full_pass_start(rating_key: str, targets: dict[int, int]) -> float: + """Where the full pass may begin: the end of the span from 0 that EVERY target track already has + (one read feeds them all, so the least-covered track decides), backed off by `WINDOW_PRE_S` so a + cue running across the join is read whole — `merge_vtt` folds the overlap. 0 when any track has + nothing from the start.""" + ends = [] + for ord_ in targets.values(): + key = cache_key(rating_key, ord_) + ranges, _ = read_meta(key) if _exists(_vtt_path(key)) else ([], False) + lead = [b for a, b in ranges if a <= 0.0] + if not lead: + return 0.0 + ends.append(max(lead)) + if not ends: + return 0.0 + return max(0.0, float(f"{min(ends) - WINDOW_PRE_S:.3f}")) + + def _sidecar_vtt(db: Session, stream: dict) -> str: """An external subtitle file, fetched through Plex and converted to WebVTT.""" with PlexClient(db) as plex: @@ -884,9 +913,10 @@ def _ffmpeg_extract( cmd += ["-map", f"0:s:{emb_ord}"] if length is not None: cmd += ["-t", f"{length:.3f}"] + if start: # The input seek lands on the keyframe at or BEFORE `start`, so a cue already running # at `start` comes out at a negative time — and by default the muxer then shifts the - # WHOLE output so that cue sits at 0: every cue of the window late by the distance to + # WHOLE output so that cue sits at 0: every cue of the read late by the distance to # that keyframe (1–5 s measured on a real library file, a whole GOP on a synthetic # one), and doubled for good once the full pass merged in beside it. Kept negative, the # times are exact on the zero-based clock (`_vtt_ts_to_s` reads the sign). diff --git a/backend/tests/test_plex_subtitle_prepare.py b/backend/tests/test_plex_subtitle_prepare.py index fcaa4aa0..9a0dbcc8 100644 --- a/backend/tests/test_plex_subtitle_prepare.py +++ b/backend/tests/test_plex_subtitle_prepare.py @@ -412,6 +412,54 @@ def test_a_full_pass_that_died_is_not_cached_as_the_whole_track(monkeypatch): assert (after["state"], after["code"]) == ("unavailable", "extract_failed") +def test_a_retried_full_pass_resumes_where_every_track_already_reaches(monkeypatch): + """A pass that ran out of time keeps what it reached, but the retry an hour later started again + at 0 — so a file too big to read inside `EXTRACT_TIMEOUT_S` never completed. It now starts at + the end of the span every track has from the start (less `WINDOW_PRE_S`, so a cue across the + join is read whole), and finishing from there completes the track.""" + calls = _stub_item(monkeypatch, clean=False, cues=("39:58.000", "40:00.000")) + _manual_pool(monkeypatch) + subtitles._extract_and_publish("98614", 3, None) + assert calls[-1] == ([0, 2], None, None), "the first pass reads from the head" + calls = _stub_item(monkeypatch, clean=True, cues=("00:05.000", "00:07.000")) + subtitles._extract_and_publish("98614", 3, None) + assert calls == [([0, 2], 2370.0, None)] + for sub_ord in (1, 3): + assert subtitles.read_meta(subtitles.cache_key("98614", sub_ord))[1] is True + assert "00:39:35.000 --> 00:39:37.000" in ( + subtitles.cache_read(subtitles.cache_key("98614", 3)) or "" + ), "the resumed read's cues sit at absolute time (2370 + 5 s)" + + +def test_a_full_pass_starts_at_the_head_while_any_track_lacks_it(monkeypatch): + """One read feeds every embedded track, so the least-covered one decides: a window elsewhere in + the film, or one track with the head and another without, still means reading from 0.""" + calls = _stub_item(monkeypatch) + _manual_pool(monkeypatch) + subtitles._extract_and_publish("98614", 3, 3570.0) # a window mid-film, both tracks + subtitles.publish(subtitles.cache_key("98614", 1), _vtt(), (0.0, 600.0), False) + subtitles._extract_and_publish("98614", 3, None) + assert calls[-1] == ([0, 2], None, None) + + +def test_the_status_says_whether_the_track_is_still_growing(monkeypatch): + """`partial` alone kept the player polling every 2 s for the rest of the film once the full + pass had failed and backed off — a state that never reaches `ready` on its own.""" + _stub_item(monkeypatch, clean=True, cues=("00:05.000", "00:07.000")) + pool = _manual_pool(monkeypatch) + subtitles._extract_and_publish("98614", 3, 0.0) + assert subtitles.ensure("98614", 3, at=10.0)["growing"] is True + pool.jobs.clear() + _stub_item(monkeypatch, clean=False, cues=("39:58.000", "40:00.000")) + subtitles._extract_and_publish("98614", 3, None) + assert subtitles.ensure("98614", 3, at=100.0) == { + "state": "partial", + "code": None, + "gen": subtitles.generation(subtitles.cache_key("98614", 3)), + "growing": False, + } + + def test_polls_that_follow_the_playhead_join_one_window(monkeypatch): """The poll sends the LIVE position, so it moves a second per poll. A window keyed on that position was a new job every second — another read of the file per poll until the first landed.""" @@ -797,6 +845,25 @@ def test_a_window_lines_up_with_the_full_pass_on_a_real_file(tmp_path, monkeypat assert merged.count("-->") == 30, "a window cue survived beside its full-pass twin" +@needs_ffmpeg +def test_a_full_pass_resumed_mid_file_lines_up_on_a_real_file(tmp_path, monkeypatch): + """A retried full pass seeks like a window does but reads to the end (no `-t`): the cue already + running at its start must come out on time, not shifted to 0 with everything after it.""" + src = _make_mkv(tmp_path) + monkeypatch.setattr(subtitles, "CACHE_DIR", tmp_path / "cache") + head, clean = subtitles._ffmpeg_extract(str(src), [0], None, 30.0) + assert clean + rest, clean = subtitles._ffmpeg_extract(str(src), [0], 21.3, None) + assert clean + key = subtitles.cache_key("1", 0) + subtitles.publish(key, head[0], (0.0, 30.0), False) + subtitles.publish(key, subtitles.shift_vtt(rest[0], -21.3), (21.3, subtitles._FULL_END_S), True) + merged = subtitles.cache_read(key) or "" + assert "00:00:22.000 --> 00:00:23.500\nline 11" in merged, merged + assert "00:00:58.000 --> 00:00:59.500\nline 29" in merged, merged + assert merged.count("-->") == 30, "a cue across the join was kept twice or lost" + + # --- the third deep-review round ---------------------------------------------------------------- diff --git a/frontend/src/lib/api/plex.ts b/frontend/src/lib/api/plex.ts index 5042a861..f9d1be7c 100644 --- a/frontend/src/lib/api/plex.ts +++ b/frontend/src/lib/api/plex.ts @@ -678,6 +678,8 @@ export const plexApi = { state: "ready" | "partial" | "preparing" | "unavailable" | "failed"; code: string | null; gen: number; + /** On `partial`/`preparing`: whether the track still fills in on its own. */ + growing?: boolean; }> => // `quiet`: the player reports a failed poll inline, so no global error dialog on top of it. req( diff --git a/frontend/src/lib/plexPlayerHooks.subtitle.dom.test.tsx b/frontend/src/lib/plexPlayerHooks.subtitle.dom.test.tsx index a1ebf86c..a56c0cdf 100644 --- a/frontend/src/lib/plexPlayerHooks.subtitle.dom.test.tsx +++ b/frontend/src/lib/plexPlayerHooks.subtitle.dom.test.tsx @@ -208,6 +208,22 @@ describe("usePlexTracks — when the status poll asks again, and when it stops", expect(status).toHaveBeenCalledTimes(1); }); + it("slows down on a partial track that will not grow on its own, and keeps asking", async () => { + // The full pass failed and is backed off for an hour: `partial` never turns `ready` by itself, + // and the 2 s cadence ran for the rest of the film. It still carries the position — reading + // ahead of an edge depends on it — so it slows rather than stops. + const status = vi + .spyOn(api, "plexSubtitleStatus") + .mockResolvedValue({ state: "partial", code: null, gen: 4, growing: false }); + const { result } = mount(client()); + result.current.setSubOrd(0); + await waitFor(() => expect(status).toHaveBeenCalledTimes(1)); + await vi.advanceTimersByTimeAsync(10_000); + expect(status).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(6000); + expect(status).toHaveBeenCalledTimes(2); + }); + it("backs off after a failed request, and still asks again so a blip heals on its own", async () => { // A failed request leaves `data` empty; it used to read as "still preparing" and re-ask every // second for as long as the player stayed open. diff --git a/frontend/src/lib/plexPlayerHooks.ts b/frontend/src/lib/plexPlayerHooks.ts index 82f002f7..6ca2e3fd 100644 --- a/frontend/src/lib/plexPlayerHooks.ts +++ b/frontend/src/lib/plexPlayerHooks.ts @@ -484,6 +484,11 @@ const SUB_POLL_ERROR_MS = 10_000; // server keeps for a minute), so no need for the 1 s cadence — but the poll carries the position, and // moving into a stretch that IS cached should stop saying "couldn't be loaded" within a few seconds. const SUB_POLL_UNAVAILABLE_MS = 5_000; +// …and on a `partial` track that is no longer `growing` (its full pass failed and is backed off for +// an hour): it will not fill in on its own, so the 2 s cadence ran for the rest of the film. The poll +// still carries the position — it is what reads the next window ahead of an edge, which the server +// starts within LOOKAHEAD_S (60 s) of it — so it slows down rather than stops. +const SUB_POLL_STALLED_MS = 15_000; const SUB_TRACK_RETRIES = 2; const SUB_TRACK_RETRY_MS = 2_000; @@ -684,7 +689,8 @@ export function usePlexTracks(opts: { const st = q.state.data?.state; if (st === "ready" || st === "failed") return false; if (st === "unavailable") return SUB_POLL_UNAVAILABLE_MS; - return st === "partial" ? 2000 : 1000; + if (st === "partial") return q.state.data?.growing === false ? SUB_POLL_STALLED_MS : 2000; + return 1000; }, staleTime: 0, retry: false, From 6f9ba428d5ab4c5073f95f2be9220be71d820d01 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 07:10:43 +0200 Subject: [PATCH 07/37] fix(playlists): the move's conflict fallback says why it is silent The house guard requires a silent handler to carry a silent-ok note; the savepoint fallback added in e74986cc did not, and the gate's pytest lane caught it. --- backend/app/routes/playlists.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 646dcf3d..1256a6e1 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -587,8 +587,10 @@ def move_item( item.position = _next_item_position(db, dst.id) db.flush() dst_changed = True + # silent-ok: the conflict IS the answer — the target already holds the video, which is the + # idempotent outcome; the savepoint put the row back in the source list, dropped below. except IntegrityError: - pass # the savepoint put the row back in the source list; drop it below + pass if not dst_changed: db.delete(item) # The source always changed; the target only when the row landed in it. `_touch` on a target From dc4e8b9fcd0bab74b7b5e3aef6da70043533bf9f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 07:16:17 +0200 Subject: [PATCH 08/37] fix(plex): playback sessions per viewer, bounded in total; a fair share of subtitle passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HLS sessions were keyed by the item, so a second account opening the same film killed the first one's session; the displaced player reclaimed at its playhead and evicted the other in turn. The registry is now keyed by (viewer, rating_key), segment requests resolve by both, and the session directory carries the viewer (the orphan sweep still recognises the older shapes). Only transcodes counted toward a cap, so remuxes — each a whole segmented copy of the film on the scratch volume — were bounded by the library size. Three bounds now apply after every start, each dropping the least recently used: two sessions per account, the transcode cap, and a new admin knob, plex_max_sessions (default 8), over every session. A superseded session request returns at once, since it waits on the killed ffmpeg, so an account cannot park more request workers than it may hold sessions. The subtitle extractor's full-pass slots were global and minutes long each; one account opening subtitles on film after film could hold them all. An account may now hold three of the six, and still gets windows past that. --- backend/app/config.py | 4 + backend/app/plex/stream.py | 122 +++++++++++++------- backend/app/plex/subtitles.py | 37 +++++- backend/app/routes/plex/playback.py | 14 +-- backend/app/sysconfig.py | 1 + backend/tests/test_hls_reaper.py | 16 +-- backend/tests/test_plex_finish.py | 2 +- backend/tests/test_plex_hls_file_route.py | 50 +++++--- backend/tests/test_plex_playback_session.py | 2 +- backend/tests/test_plex_stream.py | 95 ++++++++++++--- backend/tests/test_plex_subtitle_prepare.py | 25 +++- frontend/src/i18n/locales/en/config.json | 6 +- frontend/src/i18n/locales/hu/config.json | 6 +- 13 files changed, 278 insertions(+), 102 deletions(-) diff --git a/backend/app/config.py b/backend/app/config.py index 2490b51a..8933fd62 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -315,6 +315,10 @@ class Settings(BaseSettings): # re-encode is CPU-heavy (the VAAPI hardware path is far cheaper and could sustain more); direct-serve # and remux (stream-copy) are not capped by this. plex_max_transcodes: int = 4 + # Max concurrent playback sessions of ANY kind — remux (stream-copy) included. Cheap on the CPU but + # each one is a whole segmented copy of the film on the scratch volume, written faster than + # realtime. One account holds at most two regardless (plex/stream.py `_MAX_SESSIONS_PER_USER`). + plex_max_sessions: int = 8 # --- Transcode (playable="transcode") re-encode tuning (E6 S1) --- # Hardware-encoder policy: "auto" = VAAPI (h264_vaapi) when the iGPU render node is present AND the # source codec is hardware-decodable, else libx264 software; "on" = force VAAPI wherever the render diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 9975e1be..656dabd9 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -60,8 +60,10 @@ _WARMUP_SEG_TIMEOUT_S = 8.0 # its index for that one. See `segment_name` for why this is a constant and not a comment. _VIDEO_VARIANT = "0" _SESSION_IDLE_S = 600 # reap a session with no access for this long -# A session directory is named `{rating_key}_{int(start_s)}_{tag}_{aoff:+.2f}` (see start_session); -# before multi-audio it was just `{rating_key}_{int(start_s)}`. `sweep_orphans` deletes ONLY names +# A session directory is named `{rating_key}_u{user_id}_{int(start_s)}_{tag}_{aoff:+.2f}` (see +# start_session); before per-viewer sessions it was `{rating_key}_{int(start_s)}_{tag}_{aoff:+.2f}`, +# and before multi-audio just `{rating_key}_{int(start_s)}` — the older shapes stay sweepable, since a +# restart leaves them behind like any other. `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. @@ -72,7 +74,9 @@ _SESSION_IDLE_S = 600 # reap a session with no access for this long # 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})?$") +_SESSION_DIR_RE = re.compile( + r"^\d+_\d+(_[^_]+_[+-]\d+\.\d{2})?$|^\d+_u\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". @@ -88,7 +92,16 @@ _PROCESS_START = time.time() _MTIME_GRACE_S = 2.0 _lock = threading.Lock() -_sessions: dict[str, "HlsSession"] = {} +# Keyed by VIEWER and item, (user_id, rating_key). Keyed by the item alone, a second account opening +# the same film `_kill`ed the first one's session: the displaced viewer got a 404, reclaimed at its +# playhead, and so evicted the other in turn — two people could not watch one film at once. (One +# account on two devices still shares a session; that case ends in the player's bounded reclaim.) +_sessions: dict[tuple[int, str], "HlsSession"] = {} +# How many live sessions one account may hold at once (two devices, or a next item starting while the +# last one winds down). A new one past it replaces that account's least recently used — whose waiting +# session request then returns at once, since `wait_for` watches the killed ffmpeg — so one account +# can neither pile up film-sized scratch copies nor park more than this many request workers. +_MAX_SESSIONS_PER_USER = 2 class HlsSession: @@ -102,16 +115,17 @@ class HlsSession: transcode: bool = False, hw: bool = False, tonemap: bool = False, + user_id: int = 0, ): - # Unguessable per-session id, carried in the URLs the client is handed. Sessions are keyed - # by rating_key alone, so a seek-restart or a SECOND viewer of the same item replaces the - # registered one — and the old viewer's next request for `seg_12.ts`, resolved by rating_key, - # was answered 200 with the NEW session's twelfth segment, produced from a different offset + # Unguessable per-session id, carried in the URLs the client is handed. A seek-restart + # replaces the viewer's registered session for the item — and the old player's next request + # for `seg_12.ts`, resolved by (viewer, rating_key) alone, was answered 200 with the NEW session's twelfth segment, produced from a different offset # under `-copyts`. Wrong bytes, not an error, so the client's reclaim never fired on it # (deep-review round 5). A request naming a session that is no longer registered is a 404, # which the player recovers from by restarting at the playhead. self.id = secrets.token_urlsafe(9) - self.key = key + self.key = key # the item's rating_key (what the logs name) + self.user_id = user_id # whose session it is: the registry key is (user_id, key) self.dir = directory self.proc = proc self.transcode = transcode # video re-encoded (non-h264 source), not stream-copied @@ -637,29 +651,47 @@ def _kill(s: HlsSession) -> None: shutil.rmtree(s.dir, ignore_errors=True) -def _enforce_cap(cap: int) -> None: - # Caller holds _lock. Only TRANSCODE sessions count toward plex_max_transcodes — remux - # (stream-copy) and direct-serve are cheap and deliberately uncapped (see config.py: - # plex_max_transcodes, and the user-facing config hint). Drop the least-recently-accessed - # transcode sessions over the cap, leaving any concurrent remux streams untouched. - tx = [(key, s) for key, s in _sessions.items() if s.transcode] - if len(tx) <= cap: - return - for key, s in sorted(tx, key=lambda kv: kv[1].last_access)[: len(tx) - cap]: - # Somebody's playback stops here. The route turns the missing session into a retryable - # 404 and the player restarts at the playhead, so from the server nothing failed — and - # "my film keeps rebuffering" has no other explanation anywhere. - log.info( - "a transcode session was dropped to stay under the cap", - extra=logs.event("plex.stream.session_culled", key=key, cap=cap, sessions=len(tx)), - ) - _kill(s) - _sessions.pop(key, None) +def _enforce_caps(user_id: int, max_transcodes: int, max_sessions: int) -> None: + """Caller holds _lock. Three bounds, applied after a session is registered, each dropping the + least recently accessed sessions over it: + + * this account's own sessions (`_MAX_SESSIONS_PER_USER`) — first, so a single account filling the + box evicts itself before anyone else; + * TRANSCODE sessions (plex_max_transcodes): the CPU/GPU-heavy ones; + * EVERY session (plex_max_sessions). A remux is cheap on the CPU but not on the disk: each one is a + whole segmented copy of the film under `_HLS_ROOT`, written faster than realtime. Counted only + by the transcode cap, N distinct items were N ffmpegs and N film-sized scratch directories. + + Somebody's playback stops at each drop. The route turns the missing session into a retryable 404 + and the player restarts at the playhead, so from the server nothing failed — and "my film keeps + rebuffering" has no other explanation anywhere, hence the line.""" + + def cull(pool: list[tuple[tuple[int, str], HlsSession]], cap: int, bound: str) -> None: + if len(pool) <= cap: + return + for reg, sess in sorted(pool, key=lambda kv: kv[1].last_access)[: len(pool) - cap]: + log.info( + "a session was dropped to stay under a cap", + extra=logs.event( + "plex.stream.session_culled", + key=sess.key, + bound=bound, + cap=cap, + sessions=len(pool), + ), + ) + _kill(sess) + _sessions.pop(reg, None) + + cull([kv for kv in _sessions.items() if kv[0][0] == user_id], _MAX_SESSIONS_PER_USER, "user") + cull([kv for kv in _sessions.items() if kv[1].transcode], max_transcodes, "transcode") + cull(list(_sessions.items()), max_sessions, "total") def start_session( db: DbSession, item: PlexItem, + user_id: int, start_s: float, audio_ord: int | None = None, aoff: float = 0.0, @@ -679,6 +711,7 @@ def start_session( # session respawn (the HW->SW fallback) must not pick up a mid-playback config change. loudnorm = sysconfig.get_bool(db, "plex_audio_loudnorm") key = item.rating_key + reg = (user_id, key) # the registry key: one session per viewer per item start_s = max(0.0, float(start_s)) audio_langs = _probe_audio_langs(src) if multi else [] is_multi = len(audio_langs) > 1 # only worth a master playlist when there really are ≥2 tracks @@ -691,7 +724,7 @@ def start_session( # the tag with a DASH — `_SESSION_DIR_RE` treats the tag as one underscore-free field, and that # regex is the only thing keeping the orphan sweep off a shared scratch path (see its comment). tag = ("multi" if is_multi else str(audio_ord)) + ("-ln" if loudnorm else "") - directory = _HLS_ROOT / f"{key}_{int(start_s)}_{tag}_{aoff:+.2f}" + directory = _HLS_ROOT / f"{key}_u{user_id}_{int(start_s)}_{tag}_{aoff:+.2f}" def _seg(i: int) -> Path: return directory / segment_name(i, _VIDEO_VARIANT if variants else None) @@ -717,9 +750,9 @@ def start_session( # platform without fork (the API always runs in a Linux container in prod) it's simply omitted. preexec = _sw_child_preexec if (transcode and not hw and os.name == "posix") else None with _lock: - if replace_only is not None and _sessions.get(key) is not replace_only: + if replace_only is not None and _sessions.get(reg) is not replace_only: return None # superseded/culled while we waited out of lock — don't clobber the newer one - old = _sessions.pop(key, None) + old = _sessions.pop(reg, None) if old is not None: _kill(old) shutil.rmtree(directory, ignore_errors=True) @@ -753,8 +786,9 @@ def start_session( transcode=transcode, hw=hw, tonemap=tonemap, + user_id=user_id, ) - _sessions[key] = s + _sessions[reg] = s # The one line that says which pipeline a session actually got. Everything downstream # — the cap, the HW→SW fallback, the subtitle offset — is about this choice, and until # now it was only inferable from the ffmpeg command nobody keeps. @@ -770,9 +804,13 @@ def start_session( start_s=round(start_s, 1), ), ) - # Admin-configurable concurrency cap (Configuration → Plex → Max concurrent transcodes); - # at least 1 so playback can't be capped to zero. - _enforce_cap(max(1, sysconfig.get_int(db, "plex_max_transcodes"))) + # Admin-configurable caps (Configuration → Plex); at least 1 each so playback can't be + # capped to zero. + _enforce_caps( + user_id, + max(1, sysconfig.get_int(db, "plex_max_transcodes")), + max(1, sysconfig.get_int(db, "plex_max_sessions")), + ) return s s = _spawn(*_select_encode(db, item, transcode, src)) @@ -798,7 +836,7 @@ def start_session( # turns a None return into a "Media file not found on disk" 404 — dishonest for a file that # exists. A (dead) session instead degrades to a retryable 404 ("No active playback session" / # "Not available yet") the client re-requests. - return current_session(key) or s + return current_session(user_id, key) or s s = respawned ready = wait_for(_seg(0), timeout=25.0, proc=s.proc) if ready: @@ -829,9 +867,9 @@ def start_session( return s -def current_session(key: str) -> HlsSession | None: +def current_session(user_id: int, key: str) -> HlsSession | None: with _lock: - s = _sessions.get(key) + s = _sessions.get((user_id, key)) if s is not None: s.last_access = time.time() return s @@ -1060,21 +1098,21 @@ def reap_idle(finished_idle_s: float = _FINISHED_IDLE_S) -> int: now = time.time() dropped = 0 with _lock: - for key, s in list(_sessions.items()): + for reg, s in list(_sessions.items()): done = s.proc.poll() is not None idle = now - s.last_access if idle > _SESSION_IDLE_S or (done and idle > finished_idle_s): _kill(s) - _sessions.pop(key, None) + _sessions.pop(reg, None) dropped += 1 return dropped -def touch_session(key: str) -> None: - """Say a viewer is still there for `key`, without fetching anything. `last_access` used to move +def touch_session(user_id: int, key: str) -> None: + """Say this viewer is still there for `key`, without fetching anything. `last_access` used to move only on an HLS file fetch, which a paused player does not do — so the only way to keep a paused viewer's session alive was to keep EVERY abandoned one just as long.""" with _lock: - s = _sessions.get(key) + s = _sessions.get((user_id, key)) if s is not None: s.last_access = time.time() diff --git a/backend/app/plex/subtitles.py b/backend/app/plex/subtitles.py index 67f62cd2..374a731e 100644 --- a/backend/app/plex/subtitles.py +++ b/backend/app/plex/subtitles.py @@ -461,11 +461,18 @@ _FULL_POOL = ThreadPoolExecutor(max_workers=2, thread_name_prefix="plex-subs-ful # which time something has finished. _MAX_WINDOWS_INFLIGHT = 8 _MAX_FULL_INFLIGHT = 6 +# …of which one account may hold this many. The full-pass slots are minutes each, and a hostile (or +# merely curious) account opening subtitles on film after film could otherwise hold all of them while +# everyone else's tracks never completed. A viewer over their share still gets windows — the part +# that makes a track watchable now — just not another whole-file read until one of theirs ends. +_MAX_FULL_PER_USER = 3 _MAX_TRACK_WINDOWS = 2 _LOCK = threading.Lock() # Queued or running jobs → the absolute span a window job reads (None for a full pass). _INFLIGHT: dict[tuple, tuple[float, float] | None] = {} +# rating_key → the account whose poll queued that item's full pass (see _MAX_FULL_PER_USER). +_FULL_OWNERS: dict[str, int] = {} # Track-level failures: a track that cannot be served at all (no item, a bitmap track, Plex down…). _FAILURES: dict[str, tuple[str, float]] = {} # rating_key → when its full pass last failed (see FULL_RETRY_S). @@ -515,6 +522,7 @@ def reset_state() -> None: global _STOPPING with _LOCK: _INFLIGHT.clear() + _FULL_OWNERS.clear() _FAILURES.clear() _FULL_FAILED.clear() _WIN_FAILED.clear() @@ -598,7 +606,7 @@ def _window_failed(key: str, at: float) -> bool: return any(a <= at <= b for a, b, _ in _live_window_failures(key)) -def ensure(rating_key: str, sub_ord: int, at: float = 0.0) -> dict: +def ensure(rating_key: str, sub_ord: int, at: float = 0.0, viewer: int = 0) -> dict: """State of one subtitle track, scheduling whatever extraction is still missing. NEVER blocks. ``ready`` = fully extracted · ``partial`` = usable, covers ``at``, still growing · ``preparing`` @@ -641,7 +649,14 @@ def ensure(rating_key: str, sub_ord: int, at: float = 0.0) -> dict: if edge - at <= LOOKAHEAD_S and edge < _FULL_END_S: probe = edge + 1.0 want_window = not _window_failed(key, probe) - _schedule(rating_key, sub_ord, probe, want_window=want_window, want_full=not full_failed) + _schedule( + rating_key, + sub_ord, + probe, + want_window=want_window, + want_full=not full_failed, + viewer=viewer, + ) # `growing`: whether the track will fill in on its own. False once the full pass has failed and # is backed off — a partial track then stays partial until a window is asked for, so the player # may poll slowly (only to read ahead of an edge) instead of every two seconds for the whole film. @@ -667,7 +682,12 @@ def cached_track(rating_key: str, sub_ord: int) -> tuple[str | None, str | None] def _schedule( - rating_key: str, sub_ord: int, at: float, want_window: bool, want_full: bool = True + rating_key: str, + sub_ord: int, + at: float, + want_window: bool, + want_full: bool = True, + viewer: int = 0, ) -> None: """Kick off the jobs this track still needs: the window that makes it watchable now, and the one full pass that makes it complete. A 1 s status poll (or a second viewer) joins a job already @@ -700,8 +720,15 @@ def _schedule( ident = ("win", rating_key, sub_ord, w_start) _INFLIGHT[ident] = (w_start, w_start + WINDOW_PRE_S + WINDOW_LEN_S) jobs.append((ident, w_start)) - if want_full and n_full < _MAX_FULL_INFLIGHT and ("full", rating_key) not in _INFLIGHT: + mine = sum(1 for owner in _FULL_OWNERS.values() if owner == viewer) + if ( + want_full + and n_full < _MAX_FULL_INFLIGHT + and ("full", rating_key) not in _INFLIGHT + and mine < _MAX_FULL_PER_USER + ): _INFLIGHT[("full", rating_key)] = None + _FULL_OWNERS[rating_key] = viewer jobs.append((("full", rating_key), None)) for ident, window_start in jobs: pool = _FULL_POOL if window_start is None else _WIN_POOL @@ -765,6 +792,8 @@ def _run( finally: with _LOCK: _INFLIGHT.pop(ident, None) + if ident[0] == "full": + _FULL_OWNERS.pop(rating_key, None) def _pass_failed( diff --git a/backend/app/routes/plex/playback.py b/backend/app/routes/plex/playback.py index bd23e609..fbda4a1d 100644 --- a/backend/app/routes/plex/playback.py +++ b/backend/app/routes/plex/playback.py @@ -121,7 +121,7 @@ def subtitle_status( `at` is the viewer's absolute position: the extractor windows around it, so what gets read first is the part actually about to be watched.""" - return subtitles.ensure(rating_key, sub_ord, at) + return subtitles.ensure(rating_key, sub_ord, at, viewer=user.id) @router.get("/subtitle/{rating_key}/{sub_ord}") @@ -362,7 +362,7 @@ def item_progress( # finish latch is up, or before the first position is known. Those are the states in which # reaping is the right answer anyway, and a viewer who scrubs back into a reaped session gets # the same 404 → reclaim the player already handles. - plex_stream.touch_session(str(rating_key)) + plex_stream.touch_session(user.id, str(rating_key)) position = max(0, int(body.position_seconds)) duration = int(body.duration_seconds) final = body.final @@ -607,7 +607,7 @@ def stream_session( } if plex_paths.local_media_path(db, it.file_path) is None: raise HTTPException(status_code=404, detail="Media file not found on disk") - s = plex_stream.start_session(db, it, start, audio, aoff, multi, transcode=transcode) + s = plex_stream.start_session(db, it, user.id, start, audio, aoff, multi, transcode=transcode) if s is None: raise HTTPException(status_code=404, detail="Media file not found on disk") # Report the REAL media start (the keyframe ffmpeg landed on, which hls.js zero-bases to), not the @@ -677,9 +677,9 @@ def stream_hls_file( references its segments/subtitles by relative name, all resolving back here. The session id in the path is what makes "back here" mean the same session. Sessions are keyed by - rating_key alone and a seek-restart or a second viewer of the same item replaces the registered - one, while the DIRECTORY is per (key, offset, tag) — so resolving by rating_key handed the old - viewer the new session's `seg_12.ts`, produced from a different offset under `-copyts`: wrong + (viewer, rating_key) and a seek-restart replaces the viewer's registered one, while the DIRECTORY + is per (key, viewer, offset, tag) — so resolving by the key alone handed the old player the new + session's `seg_12.ts`, produced from a different offset under `-copyts`: wrong bytes with a 200, which the player has no way to notice (deep-review round 5). A request for a session that is no longer the registered one is a 404, and the player reclaims at the playhead. @@ -695,7 +695,7 @@ def stream_hls_file( or ".." in filename ): raise HTTPException(status_code=404, detail="Not found") - s = plex_stream.current_session(str(rating_key)) + s = plex_stream.current_session(user.id, str(rating_key)) if s is None or s.id != session_id: raise HTTPException(status_code=404, detail="No active playback session") path = s.dir / filename diff --git a/backend/app/sysconfig.py b/backend/app/sysconfig.py index 41f1b7bd..4eeb0585 100644 --- a/backend/app/sysconfig.py +++ b/backend/app/sysconfig.py @@ -176,6 +176,7 @@ SPECS: tuple[ConfigSpec, ...] = ( ConfigSpec("plex_overlay_map", "str", "plex", "plex_overlay_map"), # (plex_sync_interval_min is tuned from the Scheduler dashboard, not here — it's a scheduler job.) ConfigSpec("plex_max_transcodes", "int", "plex", "plex_max_transcodes", min=0, max=16), + ConfigSpec("plex_max_sessions", "int", "plex", "plex_max_sessions", min=1, max=64), # Transcode re-encode tuning (E6 S1). The libx264 knobs apply only to the software fallback. # crf 0–51 (x264 range); max_height 0 = source. ConfigSpec("plex_transcode_hw", "choice", "plex", "plex_transcode_hw"), diff --git a/backend/tests/test_hls_reaper.py b/backend/tests/test_hls_reaper.py index 667474cc..b42b2ee4 100644 --- a/backend/tests/test_hls_reaper.py +++ b/backend/tests/test_hls_reaper.py @@ -116,10 +116,10 @@ def test_the_periodic_pass_still_reaps_what_nobody_has_touched_for_the_idle_limi def test_touching_a_session_keeps_it_through_the_short_rule(tmp_path, sessions): s = _Session(tmp_path, "paused", finished=True, idle_s=90) - sessions["12345"] = s - stream.touch_session("12345") + sessions[(1, "12345")] = s + stream.touch_session(1, "12345") assert stream.reap_idle() == 0 - assert "12345" in sessions and s.dir.exists() and not s.proc.killed + assert (1, "12345") in sessions and s.dir.exists() and not s.proc.killed # --- the two call sites, driven through the real routes (DB lane) ------------------------------- @@ -157,7 +157,7 @@ def test_a_paused_viewers_checkpoint_keeps_their_session_alive(db, monkeypatch, db.add(user) db.commit() s = _Session(tmp_path, "paused", finished=True, idle_s=90) - sessions["paused1"] = s + sessions[(user.id, "paused1")] = s try: resp = _client(db, user).post( f"/api/plex/item/{it.rating_key}/progress", @@ -167,7 +167,7 @@ def test_a_paused_viewers_checkpoint_keeps_their_session_alive(db, monkeypatch, app.dependency_overrides.clear() assert resp.status_code == 200, resp.text assert stream.reap_idle() == 0, "the checkpoint did not say the viewer is still there" - assert "paused1" in sessions and not s.proc.killed + assert (user.id, "paused1") in sessions and not s.proc.killed def test_starting_a_session_leaves_another_viewers_paused_one_alone( @@ -183,9 +183,9 @@ def test_starting_a_session_leaves_another_viewers_paused_one_alone( db.add(user) db.commit() paused = _Session(tmp_path, "paused", finished=True, idle_s=90) - sessions["someone-else"] = paused + sessions[(999, "someone-else")] = paused - def fake_start(db_, item, start, audio=None, aoff=0.0, multi=False, transcode=False): + def fake_start(db_, item, user_id, start, audio=None, aoff=0.0, multi=False, transcode=False): return SimpleNamespace( id="sid", media_start_s=start, @@ -201,4 +201,4 @@ def test_starting_a_session_leaves_another_viewers_paused_one_alone( finally: app.dependency_overrides.clear() assert resp.status_code == 200, resp.text - assert "someone-else" in sessions and not paused.proc.killed + assert (999, "someone-else") in sessions and not paused.proc.killed diff --git a/backend/tests/test_plex_finish.py b/backend/tests/test_plex_finish.py index 3958eb1a..464e695a 100644 --- a/backend/tests/test_plex_finish.py +++ b/backend/tests/test_plex_finish.py @@ -68,7 +68,7 @@ def test_a_first_ever_finish_inserts_and_counts_once(db, make_user): def test_the_finish_supersedes_a_resume_push_still_in_flight(db, as_user, make_user, monkeypatch): user = make_user() item = _item(db) - monkeypatch.setattr(playback.plex_stream, "touch_session", lambda key: None) + monkeypatch.setattr(playback.plex_stream, "touch_session", lambda uid, key: None) earlier = datetime.now(timezone.utc) - timedelta(seconds=5) # A resume push was scheduled a moment ago and has not reached Plex yet. assert playback._should_push_resume(user.id, item.id, earlier, final=True) diff --git a/backend/tests/test_plex_hls_file_route.py b/backend/tests/test_plex_hls_file_route.py index 81fb645c..a7a680c7 100644 --- a/backend/tests/test_plex_hls_file_route.py +++ b/backend/tests/test_plex_hls_file_route.py @@ -38,6 +38,9 @@ class _Proc: return 0 +VIEWER = 41 # the id of the account the test client is signed in as + + class _Session: def __init__(self, session_id: str, directory, running: bool = False, audio_langs=None): self.id = session_id @@ -61,7 +64,7 @@ def sessions(monkeypatch): @pytest.fixture def client(monkeypatch): monkeypatch.setattr(state, "_configured_cache", True) - user = User(email="hls@example.com") + user = User(id=VIEWER, email="hls@example.com") app.dependency_overrides[current_user] = lambda: user try: yield TestClient(app) @@ -74,12 +77,23 @@ def test_a_segment_is_served_from_the_session_the_url_names(client, tmp_path, se d.mkdir() (d / "seg_11.ts").write_bytes(b"") # its predecessor, so seg_12 is the next one (d / "seg_12.ts").write_bytes(b"mine") - sessions["m1"] = _Session("sid-a", d) + sessions[(VIEWER, "m1")] = _Session("sid-a", d) r = client.get("/api/plex/stream/m1/hls/sid-a/seg_12.ts") assert r.status_code == 200, r.text assert r.content == b"mine" +def test_another_viewers_session_of_the_same_item_is_not_this_viewers(client, tmp_path, sessions): + """Sessions are per viewer: a second account playing the same film has its own, and knowing its + id does not make it yours — resolution is by (viewer, item) before the id is compared.""" + d = tmp_path / "m1_u7_0_0_+0.00" + d.mkdir() + (d / "index.m3u8").write_bytes(b"#EXTM3U") + sessions[(7, "m1")] = _Session("sid-a", d) + r = client.get("/api/plex/stream/m1/hls/sid-a/index.m3u8") + assert r.status_code == 404, r.text + + def test_a_segment_of_a_replaced_session_is_a_404_not_the_new_sessions_bytes( client, tmp_path, sessions ): @@ -89,7 +103,7 @@ def test_a_segment_of_a_replaced_session_is_a_404_not_the_new_sessions_bytes( theirs.mkdir() (theirs / "seg_11.ts").write_bytes(b"") (theirs / "seg_12.ts").write_bytes(b"theirs") - sessions["m1"] = _Session("sid-b", theirs) + sessions[(VIEWER, "m1")] = _Session("sid-b", theirs) r = client.get("/api/plex/stream/m1/hls/sid-a/seg_12.ts") assert r.status_code == 404, r.text assert b"theirs" not in r.content @@ -105,7 +119,7 @@ def test_a_playlist_with_no_live_session_is_a_404_not_a_silent_restart_at_zero( monkeypatch.setattr( plex_stream, "start_session", - lambda *a, **k: started.append(a[2] if len(a) > 2 else 0.0), + lambda *a, **k: started.append(a[3] if len(a) > 3 else 0.0), ) r = client.get("/api/plex/stream/m1/hls/sid-gone/index.m3u8") assert r.status_code == 404, r.text @@ -122,7 +136,7 @@ def test_a_segment_that_will_never_arrive_fails_at_once(client, tmp_path, sessio d = tmp_path / "m1_0_0_+0.00" d.mkdir() (d / "seg_0.ts").write_bytes(b"x") # the session is producing; seg_99999 is simply not next - sessions["m1"] = _Session("sid-a", d, running=True) + sessions[(VIEWER, "m1")] = _Session("sid-a", d, running=True) started = time.monotonic() r = client.get("/api/plex/stream/m1/hls/sid-a/seg_99999.ts") elapsed = time.monotonic() - started @@ -150,7 +164,7 @@ def test_a_multi_audio_segment_beyond_the_region_fails_at_once_too( d.mkdir() (d / "seg_0_0.ts").write_bytes(b"x") (d / "seg_a0_0.ts").write_bytes(b"x") - sessions["m1"] = _Session("sid-a", d, running=True, audio_langs=["hu", "en"]) + sessions[(VIEWER, "m1")] = _Session("sid-a", d, running=True, audio_langs=["hu", "en"]) started = time.monotonic() r = client.get(f"/api/plex/stream/m1/hls/sid-a/{name}") elapsed = time.monotonic() - started @@ -166,7 +180,7 @@ def test_a_multi_audio_segment_whose_predecessor_exists_is_served(client, tmp_pa d.mkdir() (d / "seg_a0_0.ts").write_bytes(b"x") (d / "seg_a0_1.ts").write_bytes(b"audio") - sessions["m1"] = _Session("sid-a", d, running=True, audio_langs=["hu", "en"]) + sessions[(VIEWER, "m1")] = _Session("sid-a", d, running=True, audio_langs=["hu", "en"]) r = client.get("/api/plex/stream/m1/hls/sid-a/seg_a0_1.ts") assert r.status_code == 200, r.text assert r.content == b"audio" @@ -179,7 +193,7 @@ def test_a_name_the_session_cannot_produce_fails_at_once(client, tmp_path, sessi its playlists and its segments and nothing else, so any other name is answerable now.""" d = tmp_path / "m1_0_0_+0.00" d.mkdir() - sessions["m1"] = _Session("sid-a", d, running=True) + sessions[(VIEWER, "m1")] = _Session("sid-a", d, running=True) started = time.monotonic() r = client.get("/api/plex/stream/m1/hls/sid-a/nope.m3u8") elapsed = time.monotonic() - started @@ -194,7 +208,7 @@ def test_an_absurd_segment_index_is_a_404_not_a_500(client, tmp_path, sessions, subtitle routes were bounded for; the answer is the same 404 any other unproducible name gets.""" d = tmp_path / "m1_0_0_+0.00" d.mkdir() - sessions["m1"] = _Session("sid-a", d, running=True) + sessions[(VIEWER, "m1")] = _Session("sid-a", d, running=True) r = client.get(f"/api/plex/stream/m1/hls/sid-a/seg_{'9' * digits}.ts") assert r.status_code == 404, r.text @@ -209,7 +223,7 @@ def test_a_session_torn_down_during_the_read_is_a_404_not_a_500( d = tmp_path / "m1_0_0_+0.00" d.mkdir() (d / "seg_11.ts").write_bytes(b"") - sessions["m1"] = _Session("sid-a", d) + sessions[(VIEWER, "m1")] = _Session("sid-a", d) # The file existed when the wait checked, and is gone by the time the route reads it. monkeypatch.setattr(plex_stream, "wait_for", lambda *a, **k: True) r = client.get("/api/plex/stream/m1/hls/sid-a/seg_12.ts") @@ -224,7 +238,7 @@ def test_a_dead_producer_is_not_waited_out(client, tmp_path, sessions): d = tmp_path / "m1_0_0_+0.00" d.mkdir() (d / "seg_5.ts").write_bytes(b"x") - sessions["m1"] = _Session("sid-a", d, running=False) + sessions[(VIEWER, "m1")] = _Session("sid-a", d, running=False) started = time.monotonic() r = client.get("/api/plex/stream/m1/hls/sid-a/seg_6.ts") elapsed = time.monotonic() - started @@ -294,10 +308,10 @@ def test_a_name_only_the_other_argv_branch_writes_is_refused_at_once(tmp_path, c 30.0 s and `seg_a0_0.ts` 20.1 s to answer 404 (deep-review round 7).""" single = tmp_path / "m1_0_0_+0.00" single.mkdir() - sessions["m1"] = _Session("sid-a", single, running=True) + sessions[(VIEWER, "m1")] = _Session("sid-a", single, running=True) multi = tmp_path / "m2_0_multi_+0.00" multi.mkdir() - sessions["m2"] = _Session("sid-b", multi, running=True, audio_langs=["hu", "en"]) + sessions[(VIEWER, "m2")] = _Session("sid-b", multi, running=True, audio_langs=["hu", "en"]) # Each of these belongs to the other shape (or to no shape at all) and must be refused without # a wait — the assertion is the ELAPSED time as much as the status. @@ -315,8 +329,8 @@ def test_a_name_only_the_other_argv_branch_writes_is_refused_at_once(tmp_path, c ) # And the control: the names each session CAN write are still waited for, not refused. - assert plex_stream.artifact_may_arrive(sessions["m1"], "index.m3u8") - assert plex_stream.artifact_may_arrive(sessions["m1"], "seg_0.ts") - assert plex_stream.artifact_may_arrive(sessions["m2"], "master.m3u8") - assert plex_stream.artifact_may_arrive(sessions["m2"], "stream_a1.m3u8") - assert plex_stream.artifact_may_arrive(sessions["m2"], "seg_a1_0.ts") + assert plex_stream.artifact_may_arrive(sessions[(VIEWER, "m1")], "index.m3u8") + assert plex_stream.artifact_may_arrive(sessions[(VIEWER, "m1")], "seg_0.ts") + assert plex_stream.artifact_may_arrive(sessions[(VIEWER, "m2")], "master.m3u8") + assert plex_stream.artifact_may_arrive(sessions[(VIEWER, "m2")], "stream_a1.m3u8") + assert plex_stream.artifact_may_arrive(sessions[(VIEWER, "m2")], "seg_a1_0.ts") diff --git a/backend/tests/test_plex_playback_session.py b/backend/tests/test_plex_playback_session.py index ec5d7331..c868e329 100644 --- a/backend/tests/test_plex_playback_session.py +++ b/backend/tests/test_plex_playback_session.py @@ -55,7 +55,7 @@ def _post(db, user, monkeypatch, rating_key, *, captured): monkeypatch.setattr(plex_stream, "reap_idle", lambda: None) monkeypatch.setattr(plex_paths, "local_media_path", lambda db_, fp: "/local/a.mkv") - def fake_start(db_, it, start, audio, aoff, multi, transcode=False): + def fake_start(db_, it, user_id, start, audio, aoff, multi, transcode=False): captured["transcode"] = transcode return SimpleNamespace( id="sid", diff --git a/backend/tests/test_plex_stream.py b/backend/tests/test_plex_stream.py index 5c354d9b..143f3eb5 100644 --- a/backend/tests/test_plex_stream.py +++ b/backend/tests/test_plex_stream.py @@ -108,7 +108,7 @@ class TestSweepOrphans: 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) + stream._sessions[(1, "k")] = _FakeSession(d) assert stream.sweep_orphans(older_than=_cutoff(hls_root)) == 0 assert d.exists() @@ -179,10 +179,20 @@ class _CapturingProc: def __init__(self, *args, **kwargs): _CapturingProc.calls.append((args, kwargs)) + self.killed = False def poll(self): return None + def terminate(self): + self.killed = True + + def wait(self, timeout=None): + return 0 + + def kill(self): + self.killed = True + def _venc(cmd): """The video codec ffmpeg was told to use (the value after the first `-c:v`).""" @@ -495,7 +505,7 @@ class TestColdStartWarmup: return True monkeypatch.setattr(stream, "wait_for", spy) - s = stream.start_session(db=None, item=_FakeItem(), start_s=0.0) + s = stream.start_session(db=None, user_id=7, item=_FakeItem(), start_s=0.0) assert s is not None # seg_0 (unconditional now) + the warm-up run up to _WARMUP_SEGMENTS. assert waited == [f"seg_{i}.ts" for i in range(stream._WARMUP_SEGMENTS)] @@ -508,7 +518,7 @@ class TestColdStartWarmup: return path.name != "seg_1.ts" # seg_1 never appears → warm-up gives up there monkeypatch.setattr(stream, "wait_for", spy) - s = stream.start_session(db=None, item=_FakeItem(), start_s=0.0) + s = stream.start_session(db=None, user_id=7, item=_FakeItem(), start_s=0.0) assert s is not None assert waited == ["seg_0.ts", "seg_1.ts"] # never asked for seg_2 @@ -522,7 +532,7 @@ class TestColdStartWarmup: return False monkeypatch.setattr(stream, "wait_for", spy) - s = stream.start_session(db=None, item=_FakeItem(), start_s=0.0) + s = stream.start_session(db=None, user_id=7, item=_FakeItem(), start_s=0.0) assert s is not None assert waited == ["seg_0.ts"] @@ -548,6 +558,7 @@ class TestTranscodeStartSession: "plex_transcode_crf": 18, "plex_transcode_max_height": 480, "plex_max_transcodes": 4, + "plex_max_sessions": 8, }[k], ) monkeypatch.setattr( @@ -573,7 +584,7 @@ class TestTranscodeStartSession: stream, "_select_encode", lambda db, item, transcode, src: (False, False) ) s = stream.start_session( - db=None, item=_FakeItem(codec_video="mpeg4"), start_s=0.0, transcode=True + db=None, user_id=7, item=_FakeItem(codec_video="mpeg4"), start_s=0.0, transcode=True ) assert s is not None and s.transcode is True and s.hw is False (cmd, *_), kwargs = _CapturingProc.calls[0] @@ -590,7 +601,7 @@ class TestTranscodeStartSession: stream, "_select_encode", lambda db, item, transcode, src: (True, False) ) s = stream.start_session( - db=None, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True + db=None, user_id=7, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True ) assert s is not None and s.transcode is True and s.hw is True and s.tonemap is False assert len(_CapturingProc.calls) == 1 # first segment arrived — no HW→SW fallback respawn @@ -609,7 +620,7 @@ class TestTranscodeStartSession: # tonemap=True on the session, still on the GPU (hw=True) and never niced. monkeypatch.setattr(stream, "_select_encode", lambda db, item, transcode, src: (True, True)) s = stream.start_session( - db=None, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True + db=None, user_id=7, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True ) assert s is not None and s.transcode is True and s.hw is True and s.tonemap is True (cmd, *_), kwargs = _CapturingProc.calls[0] @@ -635,7 +646,7 @@ class TestTranscodeStartSession: monkeypatch.setattr(stream, "wait_for", flaky_wait) s = stream.start_session( - db=None, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True + db=None, user_id=7, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True ) assert ( s is not None and s.transcode is True and s.hw is False @@ -666,7 +677,7 @@ class TestTranscodeStartSession: monkeypatch.setattr(stream, "wait_for", flaky_wait) s = stream.start_session( - db=None, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True + db=None, user_id=7, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True ) assert s is not None and s.transcode is True and s.hw is False and s.tonemap is False assert len(_CapturingProc.calls) == 2 # the HW tonemap attempt, then the SW respawn @@ -699,17 +710,17 @@ class TestTranscodeStartSession: def wait_then_supersede(path, timeout=20.0, proc=None): # Simulate the concurrent replacement happening during our out-of-lock wait, then report the # (now-killed) HW proc produced nothing. - stream._sessions["12345"] = newer + stream._sessions[(7, "12345")] = newer return False monkeypatch.setattr(stream, "wait_for", wait_then_supersede) s = stream.start_session( - db=None, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True + db=None, user_id=7, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True ) # No SW respawn — only the initial HW _spawn ran; the newer session was left intact. assert len(_CapturingProc.calls) == 1 assert s is newer # yielded the currently-registered session, did not clobber it - assert stream._sessions["12345"] is newer + assert stream._sessions[(7, "12345")] is newer def test_a_cap_culled_hw_session_does_not_respawn_and_is_not_reported_missing( self, capturing_env, monkeypatch @@ -727,16 +738,16 @@ class TestTranscodeStartSession: def wait_then_cull(path, timeout=20.0, proc=None): captured["s"] = stream._sessions[ - "12345" + (7, "12345") ] # the HW session the initial _spawn just registered del stream._sessions[ - "12345" + (7, "12345") ] # simulate _enforce_cap dropping the LRU under cap pressure return False monkeypatch.setattr(stream, "wait_for", wait_then_cull) s = stream.start_session( - db=None, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True + db=None, user_id=7, item=_FakeItem(codec_video="hevc"), start_s=0.0, transcode=True ) assert ( len(_CapturingProc.calls) == 1 @@ -752,7 +763,9 @@ class TestTranscodeStartSession: monkeypatch.setattr( stream, "_select_encode", lambda db, item, transcode, src: (False, False) ) - s = stream.start_session(db=None, item=_FakeItem(), start_s=0.0) # transcode defaults False + s = stream.start_session( + db=None, user_id=7, item=_FakeItem(), start_s=0.0 + ) # transcode defaults False assert s is not None and s.transcode is False (cmd, *_), kwargs = _CapturingProc.calls[0] assert _venc(cmd) == "copy" @@ -896,7 +909,7 @@ class TestAudioLoudnorm: ) if multi: monkeypatch.setattr(stream, "_probe_audio_langs", lambda src: ["hun", "eng"]) - s = stream.start_session(db=None, item=_FakeItem(), start_s=0.0, multi=multi) + s = stream.start_session(db=None, user_id=7, item=_FakeItem(), start_s=0.0, multi=multi) assert s is not None (cmd, *_), _ = _CapturingProc.calls[0] return s, cmd @@ -945,3 +958,51 @@ class TestAudioLoudnorm: # sweep no longer recognises is one that never gets cleaned up. s, _ = self._spawn(monkeypatch, on=on) assert _SESSION_DIR_RE.match(s.dir.name) + + +class TestSessionsPerViewer: + """HLS sessions were keyed by the ITEM, so a second account opening the same film killed the + first one's session; each then reclaimed and evicted the other. And only transcodes counted + toward a cap, so remuxes — each a whole segmented copy of the film on disk — were unbounded.""" + + capturing_env = TestTranscodeStartSession.capturing_env + + @staticmethod + def _start(monkeypatch, user_id, rating_key="12345"): + monkeypatch.setattr( + stream, "_select_encode", lambda db, item, transcode, src: (False, False) + ) + return stream.start_session( + db=None, user_id=user_id, item=_FakeItem(rating_key=rating_key), start_s=0.0 + ) + + def test_two_viewers_of_one_film_each_keep_their_own_session(self, capturing_env, monkeypatch): + monkeypatch.setattr(stream, "_sessions", {}) + a = self._start(monkeypatch, user_id=1) + b = self._start(monkeypatch, user_id=2) + assert stream._sessions == {(1, "12345"): a, (2, "12345"): b} + assert not a.proc.killed + assert a.dir != b.dir and _SESSION_DIR_RE.match(a.dir.name) + + def test_one_account_holds_at_most_two_sessions(self, capturing_env, monkeypatch): + monkeypatch.setattr(stream, "_sessions", {}) + first = self._start(monkeypatch, user_id=1, rating_key="1") + first.last_access -= 60 # the least recently used of the three + self._start(monkeypatch, user_id=1, rating_key="2") + self._start(monkeypatch, user_id=1, rating_key="3") + assert sorted(stream._sessions) == [(1, "2"), (1, "3")] + assert first.proc.killed + + def test_remux_sessions_count_toward_the_overall_ceiling(self, capturing_env, monkeypatch): + monkeypatch.setattr(stream, "_sessions", {}) + monkeypatch.setattr( + stream.sysconfig, + "get_int", + lambda db, k: {"plex_max_transcodes": 4, "plex_max_sessions": 3}[k], + ) + oldest = self._start(monkeypatch, user_id=1) + oldest.last_access -= 60 + for uid in (2, 3, 4): + self._start(monkeypatch, user_id=uid) + assert len(stream._sessions) == 3 and (1, "12345") not in stream._sessions + assert oldest.proc.killed and not oldest.transcode diff --git a/backend/tests/test_plex_subtitle_prepare.py b/backend/tests/test_plex_subtitle_prepare.py index 9a0dbcc8..c6ccc90f 100644 --- a/backend/tests/test_plex_subtitle_prepare.py +++ b/backend/tests/test_plex_subtitle_prepare.py @@ -690,8 +690,8 @@ def test_a_scrubbing_viewer_cannot_queue_a_window_per_stop(monkeypatch): def test_nothing_more_is_queued_while_the_extractor_is_saturated(monkeypatch): pool = _manual_pool(monkeypatch) - for rk in range(20): - subtitles.ensure(str(rk), 0, at=0.0) + for rk in range(20): # twenty viewers, so no one account's share is what stops the passes + subtitles.ensure(str(rk), 0, at=0.0, viewer=rk) windows = [a for _, a in pool.jobs if a[3] is not None] assert len(windows) == subtitles._MAX_WINDOWS_INFLIGHT assert len(pool.jobs) - len(windows) == subtitles._MAX_FULL_INFLIGHT @@ -971,6 +971,27 @@ def test_a_window_is_not_refused_while_full_passes_hold_their_slots(monkeypatch) assert full.jobs == [], "past its bound no full pass is queued" +def test_one_account_cannot_hold_every_full_pass_slot(monkeypatch): + """The full-pass slots are global and minutes long each: one account opening subtitles on film + after film held them all, and nobody else's track ever completed. Past its share an account + still gets windows — what makes a track watchable now — and the next account still gets a pass.""" + win, full = _ManualPool(), _ManualPool() + monkeypatch.setattr(subtitles, "_WIN_POOL", win) + monkeypatch.setattr(subtitles, "_FULL_POOL", full) + monkeypatch.setattr(subtitles, "_extract_and_publish", lambda *a: None) + for rk in range(subtitles._MAX_FULL_PER_USER + 1): + subtitles.ensure(str(1000 + rk), 1, at=600.0, viewer=7) + assert len(full.jobs) == subtitles._MAX_FULL_PER_USER + assert len(win.jobs) == subtitles._MAX_FULL_PER_USER + 1, ( + "the account over its share still gets windows" + ) + subtitles.ensure("2000", 1, at=600.0, viewer=8) + assert len(full.jobs) == subtitles._MAX_FULL_PER_USER + 1, "another account is not shut out" + full.run_all() # the account's passes end: its share is free again + subtitles.ensure("3000", 1, at=600.0, viewer=7) + assert any(a[1] == "3000" and a[3] is None for _, a in full.jobs) + + def test_a_poll_after_shutdown_queues_nothing(monkeypatch): pool = _manual_pool(monkeypatch) subtitles.shutdown() diff --git a/frontend/src/i18n/locales/en/config.json b/frontend/src/i18n/locales/en/config.json index 586f0393..a342b443 100644 --- a/frontend/src/i18n/locales/en/config.json +++ b/frontend/src/i18n/locales/en/config.json @@ -193,7 +193,11 @@ }, "plex_max_transcodes": { "label": "Max concurrent transcodes", - "hint": "Cap on simultaneous transcode sessions for non-H.264 files. Direct-play and remux (stream-copy) files aren't limited." + "hint": "Cap on simultaneous transcode sessions for non-H.264 files. Remux (stream-copy) sessions count only toward the overall session limit; direct-play files aren't limited." + }, + "plex_max_sessions": { + "label": "Max concurrent playback sessions", + "hint": "Cap on all simultaneous playback sessions, remux (stream-copy) included — each one writes a full segmented copy of the film to the scratch volume. One account can hold at most two. Direct-play files aren't sessions and aren't limited." }, "plex_transcode_hw": { "label": "Hardware transcode", diff --git a/frontend/src/i18n/locales/hu/config.json b/frontend/src/i18n/locales/hu/config.json index 320d9f3c..6318540e 100644 --- a/frontend/src/i18n/locales/hu/config.json +++ b/frontend/src/i18n/locales/hu/config.json @@ -193,7 +193,11 @@ }, "plex_max_transcodes": { "label": "Max párhuzamos transcode", - "hint": "A nem-H.264 fájlok egyidejű transcode-munkameneteinek felső korlátja. A direktben játszható és a remux (stream-copy) fájlokra nincs korlát." + "hint": "A nem-H.264 fájlok egyidejű transcode-munkameneteinek felső korlátja. A remux (stream-copy) munkamenetek csak az összesített munkamenet-korlátba számítanak bele; a direktben játszható fájlokra nincs korlát." + }, + "plex_max_sessions": { + "label": "Max párhuzamos lejátszási munkamenet", + "hint": "Az összes egyidejű lejátszási munkamenet felső korlátja, a remuxot (stream-copy) is beleértve — mindegyik a film teljes, szegmentált másolatát írja a munkakötetre. Egy fiók legfeljebb kettőt tarthat. A direktben játszható fájlok nem munkamenetek, rájuk nincs korlát." }, "plex_transcode_hw": { "label": "Hardveres transcode", From 7ae5c57b6b8d7d124b6f9660b8fbd2274bb01f27 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 07:34:17 +0200 Subject: [PATCH 09/37] fix(i18n): every refusal the API answers is structured and said in the reader's language MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ~150 HTTPExceptions carried a hand-written English sentence as their detail, shown verbatim under the Hungarian UI — in the blocking error dialog and on every inline surface that printed HttpError.detail (sign-in, setup wizard, change password, the passphrase gate, live search). Four also quoted an upstream exception's text back to the user. - app/refusal.py: `refused(status, reason, **params)` → {code: "refused", reason, …params}; localizeDetail translates errors.refused. with the params interpolated. "Unknown X" 404s use the existing not_found(what). - sysconfig.set_value raises InvalidSetting (reason + bounds) instead of an English ValueError; the three writers answer it through setting_refused. - The Plex connection/write/image failures log what the upstream said and answer a reason; the search scraper's failure is logged and answered as youtube/unavailable (it crashed refusal_reason before). - HttpError.detail now only ever holds a translated reason: a string detail (only the framework's own can remain) is left out, so reading .detail is safe everywhere — including destructured, the hole the raw-detail source scan could not see. That scan and its five-file exception list are gone. - Enforced: test_http_details.py fails on any HTTPException with a sentence for a detail; the contract test requires every refused reason and every not_found "what" in both languages, and no key for one that is gone. - local/jsx-comment-text: a // inside JSX children renders as text; the rule is ten lines in eslint.config.js rather than eslint-plugin-react's ~1800 lockfile lines. --- backend/app/auth.py | 14 +-- backend/app/refusal.py | 37 ++++++ backend/app/routes/_common.py | 12 +- backend/app/routes/admin.py | 36 +++--- backend/app/routes/auth.py | 21 ++-- backend/app/routes/channels.py | 17 +-- backend/app/routes/config.py | 24 ++-- backend/app/routes/discover.py | 8 +- backend/app/routes/downloads.py | 51 ++++---- backend/app/routes/feed.py | 13 ++- backend/app/routes/me.py | 21 ++-- backend/app/routes/messages.py | 28 ++--- backend/app/routes/plex/_shared.py | 4 +- backend/app/routes/plex/admin.py | 22 ++-- backend/app/routes/plex/collections.py | 41 +++---- backend/app/routes/plex/detail.py | 5 +- backend/app/routes/plex/playback.py | 42 ++++--- backend/app/routes/plex/playlists.py | 11 +- backend/app/routes/public.py | 24 ++-- backend/app/routes/saved_views.py | 13 ++- backend/app/routes/scheduler.py | 15 ++- backend/app/routes/search.py | 31 +++-- backend/app/routes/setup.py | 26 ++--- backend/app/routes/sync.py | 6 +- backend/app/routes/tags.py | 11 +- backend/app/sysconfig.py | 27 ++++- backend/tests/test_feed_cursor.py | 2 +- backend/tests/test_http_details.py | 53 +++++++++ backend/tests/test_plex_collections_filter.py | 6 +- backend/tests/test_search_scrape_ingest.py | 2 +- frontend/eslint.config.js | 21 ++++ frontend/src/i18n/backendIds.contract.test.ts | 35 ++++++ frontend/src/i18n/locales/en/errors.json | 109 +++++++++++++++++- frontend/src/i18n/locales/hu/errors.json | 109 +++++++++++++++++- .../src/lib/actionErrors.contract.test.ts | 109 ------------------ frontend/src/lib/actionErrors.ts | 10 +- frontend/src/lib/api/core.test.ts | 28 +++++ frontend/src/lib/api/core.ts | 20 +++- .../src/lib/jsxCommentText.contract.test.ts | 30 +++++ 39 files changed, 716 insertions(+), 378 deletions(-) create mode 100644 backend/app/refusal.py create mode 100644 backend/tests/test_http_details.py delete mode 100644 frontend/src/lib/actionErrors.contract.test.ts create mode 100644 frontend/src/lib/jsxCommentText.contract.test.ts diff --git a/backend/app/auth.py b/backend/app/auth.py index b8044a9c..763954cf 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -14,6 +14,7 @@ from app.db import get_db from app.models import Invite, LogEvent, User from app.security import decrypt_optional from app.utils import canonical_email +from app.refusal import refused PASSWORD_MIN_LEN = 10 @@ -341,19 +342,19 @@ def current_user(request: Request, db: Session = Depends(get_db)) -> User: def _session_user(request: Request, db: Session, user_id: int | None, is_default: bool) -> User: if not user_id: - raise HTTPException(status_code=401, detail="Not authenticated") + raise refused(401, "not_authenticated") user = db.get(User, user_id) if user is None or not user.is_active or user.is_suspended: # Suspended/deactivated mid-session → end it so the block takes effect at once. _end_account_session(request, user_id, is_default) - raise HTTPException(status_code=401, detail="Not authenticated") + raise refused(401, "not_authenticated") # SA4: reject a cookie whose recorded epoch is behind the account's current one (revoked by a # password reset/change or "log out everywhere"). A cookie with no recorded epoch (pre-SA4, or an # account signed in before this shipped) is treated as epoch 0 — so the FIRST bump revokes it too. epochs = request.session.get("epochs") or {} if int(epochs.get(str(user_id), 0)) != (user.session_epoch or 0): _end_account_session(request, user_id, is_default) - raise HTTPException(status_code=401, detail="Not authenticated") + raise refused(401, "not_authenticated") # Always keep the session default in the switchable list — covers sessions created before # multi-session existed (their account_ids was never seeded), so the switcher isn't empty. default_id = request.session.get("user_id") @@ -418,10 +419,7 @@ def require_write_scope(user: User = Depends(require_human)) -> User: checks that had drifted into three different 403 messages across channels/playlists. Layers on `require_human` because writing to YouTube needs a real, non-demo identity.""" if not has_write_scope(user): - raise HTTPException( - status_code=403, - detail="Enable YouTube editing in Settings first.", - ) + raise refused(403, "youtube_editing_off") return user @@ -429,7 +427,7 @@ def admin_user(user: User = Depends(current_user)) -> User: """Dependency: require the signed-in user to be an admin. Single source of truth for the admin gate — every admin route/action depends on this rather than re-checking the role.""" if user.role != "admin": - raise HTTPException(status_code=403, detail="Admin only") + raise refused(403, "admin_only") return user diff --git a/backend/app/refusal.py b/backend/app/refusal.py new file mode 100644 index 00000000..fc70f7b6 --- /dev/null +++ b/backend/app/refusal.py @@ -0,0 +1,37 @@ +"""A request the app refuses, said in a way the UI can put in the reader's language. + +Every refusal the API answers carries a structured `detail` — `{code, reason, …params}` — and the client +translates it (`lib/api/core.ts` `localizeDetail`). A bare English string reached the Hungarian UI +verbatim: the blocking error dialog, and every inline surface that printed `HttpError.detail`, showed +whatever sentence the route happened to hand-write. `refused` is the generic family +(`errors.refused.`, the params interpolated into the sentence); the older specific codes +(`not_found`, `youtube`, `playlist`, the admin refusals, …) stay, since each is already translated. + +A test (`test_http_details.py`) reads every `HTTPException` in the app and fails on a string detail, +and the frontend's contract test requires every `refused(…, "")` to be named in both +languages — so the rule is enforced, not described. +""" + +from fastapi import HTTPException + + +def refused(status: int, reason: str, **params: object) -> HTTPException: + """The refusal to raise: `raise refused(409, "view_name_taken", name=name)`. `params` travel in + the detail for the sentence to interpolate (`{{name}}`); keep them to values the reader may see.""" + return HTTPException(status_code=status, detail={"code": "refused", "reason": reason, **params}) + + +# `sysconfig.InvalidSetting.reason` → the refusal it answers as. A dict of literals, so the frontend's +# contract test can read every reason this can produce. +_SETTING_REASONS = { + "not_a_number": "setting_not_a_number", + "too_low": "setting_too_low", + "too_high": "setting_too_high", + "not_a_choice": "setting_not_a_choice", + "no_encryption_key": "no_encryption_key", +} + + +def setting_refused(exc) -> HTTPException: + """A value the admin entered that `sysconfig.set_value` refused (`exc`: its `InvalidSetting`).""" + return refused(400, _SETTING_REASONS[exc.reason], key=exc.key, **exc.params) diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py index 9154ae4c..6689f6fa 100644 --- a/backend/app/routes/_common.py +++ b/backend/app/routes/_common.py @@ -46,18 +46,20 @@ def owned_or_404( ) -> M: """Fetch a row by primary key that `user` owns, or raise 404. A row owned by someone else 404s (NOT 403) on purpose — a 403 would confirm the id exists to a stranger. - Generalizes the near-identical per-route ownership helpers (e.g. plex's - `_own_playlist_or_404`, the saved-views owner check).""" + It replaced the near-identical per-route ownership helpers, and is called directly — a thin + `_own_x` wrapper around it is the ad-hoc shape CLAUDE.md's patterns table rules out.""" obj = get_or_404(db, model, id_, what=what) if getattr(obj, owner_attr) != user.id: raise not_found(what) return obj -def youtube_refusal(exc: YouTubeError) -> HTTPException: +def youtube_refusal(exc: Exception) -> HTTPException: """The answer to an action YouTube refused: a 422 whose reason the UI translates (`errors.youtube.`). One definition, because the routes had grown three shapes of it — a bare English string quoting the upstream message, a playlist-scoped reason, and nothing at all (a raw 500 from the manual syncs). The upstream failure is already logged where it - happened (`youtube.api.error`).""" - return HTTPException(status_code=422, detail={"code": "youtube", "reason": refusal_reason(exc)}) + happened (`youtube.api.error`). A failure that is not the Data API's (the search scraper's) is + YouTube being unavailable to us — the retryable answer.""" + reason = refusal_reason(exc) if isinstance(exc, YouTubeError) else "unavailable" + return HTTPException(status_code=422, detail={"code": "youtube", "reason": reason}) diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index 63e070da..c77f40b3 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -39,6 +39,7 @@ from app.routes.auth import ( send_verification_link, ) from app.utils import canonical_email, iso, valid_email +from app.refusal import refused router = APIRouter(prefix="/api/admin", tags=["admin"]) @@ -179,7 +180,7 @@ def _decide(db: Session, invite_id: int, admin: User, approved: bool) -> Invite: # sending a duplicate decision mail, even flipping an already-approved invite to "denied" and # mailing a rejection to an already-active account. if inv.status != "pending": - raise HTTPException(status_code=409, detail="Invite already decided") + raise refused(409, "invite_decided") inv.status = "approved" if approved else "denied" inv.decided_at = datetime.now(timezone.utc) inv.decided_by = admin.email @@ -242,7 +243,7 @@ def add_invite( """Manually whitelist an email (approved straight away). Convenience for the admin.""" email = body.email.strip().lower() if not valid_email(email): - raise HTTPException(status_code=400, detail="Enter a valid email address.") + raise refused(400, "invalid_email") # Reuse a canonically-equal row instead of adding a second one for the same mailbox. Prod carries # exactly that pair for one person from 2026-06-16 (hartmann.peter1986@ / hartmannpeter1986@), # which is how an admin ends up approving the same human twice and still seeing them bounce. @@ -366,10 +367,7 @@ def delete_invite( promoted to admin on its next Google sign-in). The admin has to hear that, not guess it.""" inv = get_or_404(db, Invite, invite_id, what="invite") if _accounts_for(db, [inv.email]): - raise HTTPException( - status_code=409, - detail="That address already has an account — delete the account instead.", - ) + raise refused(409, "address_has_account") still_allowed_by_env = _env_allowed(inv.email) audit.record( db, @@ -435,19 +433,19 @@ def set_user_role( account, and can't remove the last remaining admin. Emails the user when it actually changes.""" role = body.role if role not in ("user", "admin"): - raise HTTPException(status_code=400, detail="role must be 'user' or 'admin'") + raise refused(400, "invalid_role") target = get_or_404(db, User, user_id, what="user") if target.is_demo: - raise HTTPException(status_code=400, detail="The demo account's role can't be changed.") + raise refused(400, "demo_role") if target.id == admin.id: - raise HTTPException(status_code=400, detail="You can't change your own role.") + raise refused(400, "own_role") if ( target.role == "admin" and role == "user" and not target.is_suspended and count_admins(db, active_only=True) <= 1 ): - raise HTTPException(status_code=400, detail="Can't remove the last admin.") + raise refused(400, "last_admin") changed = target.role != role if changed: audit.record( @@ -493,16 +491,16 @@ def set_user_suspended( suspended = body.suspended target = get_or_404(db, User, user_id, what="user") if target.is_demo: - raise HTTPException(status_code=400, detail="The demo account can't be suspended.") + raise refused(400, "demo_suspend") if target.id == admin.id: - raise HTTPException(status_code=400, detail="You can't suspend your own account.") + raise refused(400, "own_suspend") if ( suspended and target.role == "admin" and not target.is_suspended and count_admins(db, active_only=True) <= 1 ): - raise HTTPException(status_code=400, detail="Can't suspend the last active admin.") + raise refused(400, "last_active_admin") was_suspended = target.is_suspended if was_suspended != suspended: audit.record( @@ -537,17 +535,15 @@ def admin_delete_user( not the demo account, not yourself (use Settings → Account), and not the last admin.""" target = get_or_404(db, User, user_id, what="user") if target.is_demo: - raise HTTPException(status_code=400, detail="The demo account can't be deleted.") + raise refused(400, "demo_delete") if target.id == admin.id: - raise HTTPException( - status_code=400, detail="Delete your own account from Settings → Account." - ) + raise refused(400, "own_delete") if ( target.role == "admin" and not target.is_suspended and count_admins(db, active_only=True) <= 1 ): - raise HTTPException(status_code=400, detail="Can't delete the last admin.") + raise refused(400, "last_admin") # Record BEFORE the purge — the target row (and its email) is gone afterwards. audit.record( db, @@ -691,7 +687,7 @@ def activate_user( `_activate_user_by_email`). What was missing was any way to know they had to.""" target = get_or_404(db, User, user_id, what="user") if target.is_demo: - raise HTTPException(status_code=400, detail="The demo account can't be activated.") + raise refused(400, "demo_activate") if target.is_active: return _serialize_user(target) # idempotent: a double click is not an error if not is_allowed(db, target.email): @@ -788,7 +784,7 @@ def add_demo_whitelist( sign-in). Idempotent — re-adding an existing email just returns it.""" email = body.email.strip().lower() if not valid_email(email): - raise HTTPException(status_code=400, detail="Enter a valid email address.") + raise refused(400, "invalid_email") row = db.execute(select(DemoWhitelist).where(DemoWhitelist.email == email)).scalar_one_or_none() if row is None: row = DemoWhitelist( diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 6d135751..0b614648 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -43,6 +43,7 @@ from app.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User from app.ratelimit import RateLimiter, client_key from app.security import encrypt, hash_password, hash_token, verify_password from app.utils import canonical_email, valid_email +from app.refusal import refused VERIFY_TTL = timedelta(hours=24) # A confirmation link that rides an APPROVAL or an invitation lives longer than one the person is @@ -378,7 +379,7 @@ async def callback( userinfo = token.get("userinfo") if not userinfo or not userinfo.get("sub"): - raise HTTPException(status_code=400, detail="No user info returned by Google") + raise refused(400, "google_no_userinfo") sub = userinfo["sub"] email = (userinfo.get("email") or "").lower() @@ -653,7 +654,7 @@ async def request_access( endpoint can't be probed for throttling either.""" email = (body.email or "").strip().lower() if not valid_email(email): - raise HTTPException(status_code=400, detail="Enter a valid email address.") + raise refused(400, "invalid_email") if not _request_access_limiter.allow(client_key(request)): return {"status": "pending"} if is_allowed(db, email): @@ -815,14 +816,11 @@ def register( password = body.password or "" # Input validation is safe to reveal (independent of whether the email exists). if not valid_email(email): - raise HTTPException(status_code=400, detail="Enter a valid email address.") + raise refused(400, "invalid_email") if len(password) < PASSWORD_MIN_LEN: - raise HTTPException( - status_code=400, - detail=f"Password must be at least {PASSWORD_MIN_LEN} characters.", - ) + raise refused(400, "weak_password", min=PASSWORD_MIN_LEN) if not sysconfig.get_bool(db, "allow_registration"): - raise HTTPException(status_code=403, detail="Registration is currently closed.") + raise refused(403, "registration_closed") if not _register_limiter.allow(client_key(request)): return {"status": "ok"} # silently throttle; uniform response @@ -1293,13 +1291,10 @@ def set_password( raise HTTPException(status_code=403, detail={"code": "demo"}) new_password = body.password or "" if len(new_password) < PASSWORD_MIN_LEN: - raise HTTPException( - status_code=400, - detail=f"Password must be at least {PASSWORD_MIN_LEN} characters.", - ) + raise refused(400, "weak_password", min=PASSWORD_MIN_LEN) if user.password_hash: if not verify_password(body.current_password or "", user.password_hash): - raise HTTPException(status_code=403, detail="Current password is incorrect.") + raise refused(403, "wrong_password") user.password_hash = hash_password(new_password) user.password_revoked_at = None # same as the reset path: a password exists again # SA4: a password change logs out every OTHER session; re-stamp this cookie at the new epoch so diff --git a/backend/app/routes/channels.py b/backend/app/routes/channels.py index 10495618..87e1623d 100644 --- a/backend/app/routes/channels.py +++ b/backend/app/routes/channels.py @@ -4,7 +4,7 @@ hidden, tags) and per-channel sync state. Local reads/writes plus YouTube-side w from datetime import datetime, timezone -from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException +from fastapi import APIRouter, BackgroundTasks, Depends from pydantic import BaseModel from sqlalchemy import func, select, update from sqlalchemy.orm import Session @@ -31,6 +31,7 @@ from app.sync.runner import backfill_channel_now, run_recent_backfill from app.sync.subscriptions import apply_channel_details from app.utils import iso from app.youtube.client import YouTubeClient, YouTubeError +from app.refusal import refused router = APIRouter(prefix="/api/channels", tags=["channels"]) @@ -383,12 +384,9 @@ def explore_channel( backfill quota reserve; blocked for the demo account (it spends shared quota).""" channel = get_or_404(db, Channel, channel_id, what="channel") if _is_blocked(db, user, channel_id): - raise HTTPException(status_code=403, detail="You've blocked this channel.") + raise refused(403, "channel_blocked") if quota.backfill_reserve_reached(): - raise HTTPException( - status_code=429, - detail="The shared daily quota reserve is reached — try exploring later.", - ) + raise refused(429, "quota_reserve") # Record/refresh the exploration (DB only) before ingesting, so the explorer can see the # videos as soon as they land. Flag the channel ephemeral unless it's organically followed. @@ -562,7 +560,7 @@ def subscribe( ) ).scalar_one_or_none() if existing is not None: - raise HTTPException(status_code=409, detail="Already subscribed to this channel.") + raise refused(409, "already_subscribed") with ( quota.attribute(user.id, quota.QuotaAction.CHANNELS_SUBSCRIBE), YouTubeClient(db, user) as yt, @@ -634,10 +632,7 @@ def unsubscribe( should Hide instead.""" sub = _user_subscription(db, user, channel_id) if not sub.yt_subscription_id: - raise HTTPException( - status_code=400, - detail="No YouTube subscription id on record — sync subscriptions first.", - ) + raise refused(400, "no_subscription_id") try: with ( quota.attribute(user.id, quota.QuotaAction.CHANNELS_UNSUBSCRIBE), diff --git a/backend/app/routes/config.py b/backend/app/routes/config.py index b850e6be..b5f68bf7 100644 --- a/backend/app/routes/config.py +++ b/backend/app/routes/config.py @@ -4,7 +4,7 @@ app.sysconfig. Secret values (e.g. SMTP password) are write-only — never retur import re from typing import Any -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends from pydantic import BaseModel from sqlalchemy.orm import Session @@ -17,6 +17,9 @@ from app.audit import AuditAction from app.auth import admin_user from app.db import get_db from app.models import User +from app.refusal import refused +from app.routes._common import not_found +from app.refusal import setting_refused router = APIRouter(prefix="/api/admin/config", tags=["admin"]) @@ -59,19 +62,16 @@ def set_config( ) -> dict: s = sysconfig.spec(key) if s is None: - raise HTTPException(status_code=404, detail="Unknown config key") + raise not_found("config_key") if "value" not in body.model_fields_set: - raise HTTPException(status_code=400, detail="Missing 'value'") + raise refused(400, "value_missing") if s.secret and not sysconfig.secrets_manageable(): - raise HTTPException( - status_code=400, - detail="Set TOKEN_ENCRYPTION_KEY to store secrets in the database.", - ) + raise refused(400, "no_encryption_key") old = None if s.secret else sysconfig.get(db, key) try: sysconfig.set_value(db, key, body.value) - except (TypeError, ValueError) as exc: - raise HTTPException(status_code=400, detail=str(exc) or "Invalid value") + except sysconfig.InvalidSetting as exc: + raise setting_refused(exc) from None # Never log a secret's value — only that it was changed, by whom. Non-secret values are logged # but with URL credentials redacted (a proxy/URL setting can embed user:pass). Skip the row when # a non-secret value didn't actually change (a no-op re-save shouldn't add trail noise); secrets @@ -101,7 +101,7 @@ def reset_config( ) -> dict: s = sysconfig.spec(key) if s is None: - raise HTTPException(status_code=404, detail="Unknown config key") + raise not_found("config_key") old = None if s.secret else sysconfig.get(db, key) sysconfig.reset(db, key) audit.record( @@ -126,8 +126,8 @@ def send_test_email( """Send a test email to the requesting admin using the current (DB or env) SMTP config — lets the admin verify the settings actually work.""" if not email_mod.email_enabled(): - raise HTTPException(status_code=400, detail="SMTP is not configured.") + raise refused(400, "smtp_off") sent = email_mod.send_test(user.email, mailtext.lang_for_user(user)) if not sent: - raise HTTPException(status_code=422, detail="SMTP send failed — check the settings.") + raise refused(422, "smtp_failed") return {"sent": True, "to": user.email} diff --git a/backend/app/routes/discover.py b/backend/app/routes/discover.py index e7447c09..1ac1bea1 100644 --- a/backend/app/routes/discover.py +++ b/backend/app/routes/discover.py @@ -11,7 +11,7 @@ thing it exists to remove. from typing import Literal -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends from pydantic import BaseModel from sqlalchemy import select, update from sqlalchemy.exc import IntegrityError @@ -23,7 +23,7 @@ from app.discover import hero as hero_mod from app.discover import pins, rails from app.discover.budget import Budget, DEFAULT_BUDGET from app.models import ComfortPin, User -from app.routes._common import owned_or_404 +from app.routes._common import owned_or_404, not_found router = APIRouter(prefix="/api/discover", tags=["discover"]) @@ -125,7 +125,7 @@ def add_pin( constraint is the mechanism, not the message.""" resolved = pins.resolve_ref(db, user, body.kind, body.ref) if resolved is None: - raise HTTPException(status_code=404, detail="Unknown item") + raise not_found("item") column, value = resolved existing = db.execute( @@ -157,7 +157,7 @@ def add_pin( # between `resolve_ref` confirming it and this INSERT (a Plex sync dropping a removed # item), which trips the FK. Semantically that is the same "no such thing" the pre-check # returns above — re-raising handed the user a 500 they could do nothing with. - raise HTTPException(status_code=404, detail="Unknown item") + raise not_found("item") return {"ok": True, "id": winner.id, "created": False} return {"ok": True, "id": pin.id, "created": True} diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index ed479700..20dc24ff 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -42,6 +42,7 @@ from app.routes._common import get_or_404, not_found, owned_or_404 from app.security import hash_password from app.userscope import is_messageable_user, messageable_clauses from app.utils import iso +from app.refusal import refused router = APIRouter(prefix="/api/downloads", tags=["downloads"]) admin_router = APIRouter(prefix="/api/admin/downloads", tags=["admin-downloads"]) @@ -63,7 +64,7 @@ def resolve_source(raw: str) -> tuple[str, str]: as a raw URL for yt-dlp's generic extractor.""" raw = (raw or "").strip() if not raw: - raise HTTPException(status_code=400, detail="A video link or id is required.") + raise refused(400, "link_required") if _VIDEO_ID.match(raw): return "youtube", raw if raw.startswith(("http://", "https://")): @@ -93,7 +94,7 @@ def resolve_source(raw: str) -> tuple[str, str]: if _VIDEO_ID.match(vid): return "youtube", vid return "url", raw - raise HTTPException(status_code=400, detail="That doesn't look like a valid video link.") + raise refused(400, "invalid_link") def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict: @@ -233,7 +234,7 @@ def create_profile( name = body.name.strip() spec = body.spec if not name: - raise HTTPException(status_code=400, detail="A profile name is required.") + raise refused(400, "profile_name_required") maxpos = db.execute( select(func.coalesce(func.max(DownloadProfile.sort_order), 100)).where( DownloadProfile.user_id == user.id @@ -261,16 +262,16 @@ def update_profile( ) -> dict: p = db.get(DownloadProfile, profile_id) if p is None or p.is_builtin or p.user_id != user.id: - raise HTTPException(status_code=404, detail="Unknown profile") + raise not_found("profile") sent = body.model_fields_set if "name" in sent: name = (body.name or "").strip() if not name: - raise HTTPException(status_code=400, detail="A profile name is required.") + raise refused(400, "profile_name_required") p.name = name[:80] if "spec" in sent: if body.spec is None: - raise HTTPException(status_code=400, detail="spec must be an object") + raise refused(400, "spec_not_object") p.spec = body.spec db.commit() return _serialize_profile(p) @@ -282,7 +283,7 @@ def delete_profile( ) -> dict: p = db.get(DownloadProfile, profile_id) if p is None or p.is_builtin or p.user_id != user.id: - raise HTTPException(status_code=404, detail="Unknown profile") + raise not_found("profile") db.delete(p) db.commit() return {"deleted": profile_id} @@ -312,7 +313,7 @@ def set_credential( """Store (encrypted) this user's cookies.txt for one supported host. Write-only: the value is never echoed back, logged, or audited — only the fact that it changed.""" if not dlcreds.is_supported_host(host_key): - raise HTTPException(status_code=404, detail="Unknown host") + raise not_found("host") try: cleaned = dlcreds.validate_cookies(body.cookies) except dlcreds.InvalidCookies as e: @@ -334,7 +335,7 @@ def delete_credential( host_key: str, user: User = Depends(require_human), db: Session = Depends(get_db) ) -> dict: if not dlcreds.is_supported_host(host_key): - raise HTTPException(status_code=404, detail="Unknown host") + raise not_found("host") if dlcreds.delete_credential(db, user.id, host_key): audit.record( db, @@ -355,7 +356,7 @@ def _resolve_spec( if profile_id is not None: p = db.get(DownloadProfile, profile_id) if p is None or (not p.is_builtin and p.user_id != user.id): - raise HTTPException(status_code=404, detail="Unknown profile") + raise not_found("profile") return p.spec, p.id if spec is not None: return spec, None @@ -366,7 +367,7 @@ def _resolve_spec( .limit(1) ).scalar_one_or_none() if fallback is None: - raise HTTPException(status_code=400, detail="No download profile available.") + raise refused(400, "no_profile") return fallback.spec, fallback.id @@ -477,7 +478,7 @@ def enqueue_edit( source file is only read, never modified, so editing a shared video is safe.""" source_job = _accessible_job(db, user, body.source_job_id) if source_job.status != "done" or source_job.asset_id is None: - raise HTTPException(status_code=409, detail="You can only edit a finished download.") + raise refused(409, "edit_unfinished") spec = body.edit_spec display_name = (body.display_name or "").strip() or None try: @@ -661,9 +662,9 @@ def redownload_download( if job.job_kind in ("edit", "upload"): # Neither has a re-fetchable source: an editor clip is derived locally, an upload IS the # source. Re-running yt-dlp against source_kind='upload'/'edit' would just fail. - raise HTTPException(status_code=409, detail="This item can't be re-downloaded.") + raise refused(409, "not_redownloadable") if job.status in ("queued", "running", "paused"): - raise HTTPException(status_code=409, detail="This download is still in progress.") + raise refused(409, "still_downloading") service.redownload(db, job) return _serialize_job(db, job) @@ -715,10 +716,10 @@ def download_file( job = _accessible_job(db, user, job_id) asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None if asset is None or asset.status != "ready" or not asset.rel_path: - raise HTTPException(status_code=409, detail="This download isn't ready.") + raise refused(409, "download_not_ready") path = storage.safe_abs_path(settings.download_root, asset.rel_path) if path is None: - raise HTTPException(status_code=404, detail="File is no longer available.") + raise not_found("file") filename = storage.download_filename( job.display_name or asset.title or asset.source_ref, asset.container, path @@ -757,7 +758,7 @@ def storyboard_meta( job = _accessible_job(db, user, job_id) asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None if asset is None or asset.status != "ready" or not asset.rel_path: - raise HTTPException(status_code=409, detail="This download isn't ready.") + raise refused(409, "download_not_ready") meta = editmod.ensure_storyboard(asset, settings.download_root) if meta is None: return {"available": False} @@ -778,10 +779,10 @@ def poster_image(job_id: int, user: User = Depends(require_human), db: Session = job = _accessible_job(db, user, job_id) asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None if asset is None or not asset.poster_path: - raise HTTPException(status_code=404, detail="No poster.") + raise not_found("poster") path = storage.safe_abs_path(settings.download_root, asset.poster_path) if path is None: - raise HTTPException(status_code=404, detail="No poster.") + raise not_found("poster") return FileResponse(path, media_type="image/jpeg") @@ -792,10 +793,10 @@ def storyboard_image( job = _accessible_job(db, user, job_id) asset = db.get(MediaAsset, job.asset_id) if job.asset_id else None if asset is None or not asset.storyboard_path: - raise HTTPException(status_code=404, detail="No filmstrip.") + raise not_found("filmstrip") path = storage.safe_abs_path(settings.download_root, asset.storyboard_path) if path is None: - raise HTTPException(status_code=404, detail="No filmstrip.") + raise not_found("filmstrip") return FileResponse(path, media_type="image/jpeg") @@ -815,10 +816,10 @@ def share_download( ) -> dict: job = owned_or_404(db, DownloadJob, job_id, user, what="download") if job.status != "done" or job.asset_id is None: - raise HTTPException(status_code=409, detail="Only a finished download can be shared.") + raise refused(409, "share_unfinished") target = body.email.strip().lower() if not target: - raise HTTPException(status_code=400, detail="A recipient email is required.") + raise refused(400, "recipient_required") recipient = db.execute( select(User).where(func.lower(User.email) == target) ).scalar_one_or_none() @@ -827,7 +828,7 @@ def share_download( if not is_messageable_user(recipient): raise not_found("recipient") if recipient.id == user.id: - raise HTTPException(status_code=400, detail="That's already your download.") + raise refused(400, "share_own") exists = db.execute( select(DownloadShare.id).where( DownloadShare.job_id == job_id, DownloadShare.to_user_id == recipient.id @@ -972,7 +973,7 @@ def create_link( ) -> dict: job = owned_or_404(db, DownloadJob, job_id, user, what="download") if job.status != "done" or job.asset_id is None: - raise HTTPException(status_code=409, detail="Only a finished download can be shared.") + raise refused(409, "share_unfinished") pw = body.password.strip() link = DownloadLink( job_id=job_id, diff --git a/backend/app/routes/feed.py b/backend/app/routes/feed.py index b38bbdb1..940199c8 100644 --- a/backend/app/routes/feed.py +++ b/backend/app/routes/feed.py @@ -1,6 +1,6 @@ from datetime import date, datetime, timezone -from fastapi import APIRouter, Depends, HTTPException, Query +from fastapi import APIRouter, Depends, Query from pydantic import BaseModel, ConfigDict, Field from sqlalchemy import and_, func, or_, select from sqlalchemy.dialects.postgresql import insert as pg_insert @@ -29,10 +29,11 @@ from app.models import ( Video, VideoState, ) -from app.routes._common import get_or_404, youtube_refusal +from app.routes._common import get_or_404, youtube_refusal, not_found from app.sync.videos import parse_iso8601_duration from app.utils import iso from app.youtube.client import YouTubeClient, YouTubeError +from app.refusal import refused router = APIRouter(prefix="/api", tags=["feed"]) @@ -120,7 +121,7 @@ def get_feed( vals, cursor_id = decode_cursor(keys, cursor) except ValueError: # `from None`: the ValueError is an expected bad-input signal, not an error to chain. - raise HTTPException(status_code=400, detail="Invalid feed cursor") from None + raise refused(400, "invalid_cursor") from None for k, v in zip(keys, vals): k["value"] = v query = query.where(keyset_predicate(keys, cursor_id)) @@ -235,7 +236,7 @@ def set_video_state( ) -> dict: status = body.status if status not in VALID_STATES: - raise HTTPException(status_code=400, detail=f"status must be one of {VALID_STATES}") + raise refused(400, "invalid_status") get_or_404(db, Video, video_id, what="video") now = datetime.now(timezone.utc) @@ -402,7 +403,7 @@ def get_video_detail( # The DB path above is free; the off-catalog YouTube lookup below spends a quota unit, so the # demo account can't reach it — it just sees "unknown" for a video we don't already store. if user.is_demo: - raise HTTPException(status_code=404, detail="Unknown video") + raise not_found("video") try: with ( @@ -413,7 +414,7 @@ def get_video_detail( except YouTubeError as exc: raise youtube_refusal(exc) if not items: - raise HTTPException(status_code=404, detail="Unknown video") + raise not_found("video") snippet = items[0].get("snippet", {}) stats = items[0].get("statistics", {}) diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index 74b15360..ce6fab67 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -24,6 +24,8 @@ from app import sysconfig from app.db import get_db from app.models import Invite, User from app.utils import iso +from app.refusal import refused +from app.routes._common import not_found router = APIRouter(prefix="/api/me", tags=["me"]) @@ -70,7 +72,7 @@ def switch_account( here) are allowed, and access is re-checked in case the invite was revoked since.""" target = body.user_id if target not in (request.session.get("account_ids") or []): - raise HTTPException(status_code=403, detail="Not an account you've signed into here.") + raise refused(403, "not_your_account") # An account `_session_user` would refuse — gone, deactivated, suspended, or its session revoked — is # refused here too, and dropped from the switch list the way `_session_user` drops it. Made the session # default instead, the next request's current_user would refuse it as the DEFAULT and clear the whole @@ -78,22 +80,20 @@ def switch_account( u = db.get(User, target) if u is None: drop_from_wallet(request.session, target) - raise HTTPException(status_code=404, detail="That account no longer exists.") + raise not_found("account") if not is_allowed(db, u.email): - raise HTTPException(status_code=403, detail="That account no longer has access.") + raise refused(403, "account_no_access") if not u.is_active: drop_from_wallet(request.session, target) - raise HTTPException(status_code=403, detail="That account no longer has access.") + raise refused(403, "account_no_access") if u.is_suspended: drop_from_wallet(request.session, target) - raise HTTPException(status_code=403, detail="That account is suspended.") + raise refused(403, "account_suspended") # Revoked: a password reset/change or "log out everywhere" bumped the account's epoch. epochs = request.session.get("epochs") or {} if int(epochs.get(str(target), 0)) != (u.session_epoch or 0): drop_from_wallet(request.session, target) - raise HTTPException( - status_code=403, detail="That account was signed out elsewhere — sign in again." - ) + raise refused(403, "account_signed_out") request.session["user_id"] = target return {"ok": True, "user_id": target} @@ -181,10 +181,7 @@ def delete_my_account( if user.is_demo: raise HTTPException(status_code=403, detail={"code": "demo", "reason": "delete_account"}) if user.role == "admin" and count_admins(db) <= 1: - raise HTTPException( - status_code=400, - detail="You're the only admin — promote another admin before deleting your account.", - ) + raise refused(400, "only_admin_delete") user_id = user.id # Full erasure (cascades) + access-request cleanup + Google-grant revocation; shared with the # admin delete path. Capture the id first — `user` is detached once purge_user deletes the row. diff --git a/backend/app/routes/messages.py b/backend/app/routes/messages.py index e9f705c6..64156870 100644 --- a/backend/app/routes/messages.py +++ b/backend/app/routes/messages.py @@ -16,7 +16,7 @@ routing and unread counts. from datetime import datetime, timezone -from fastapi import APIRouter, Depends, HTTPException, WebSocket, WebSocketDisconnect +from fastapi import APIRouter, Depends, WebSocket, WebSocketDisconnect from pydantic import BaseModel from sqlalchemy import and_, case, delete, func, or_, select from sqlalchemy.orm import Session @@ -29,6 +29,8 @@ from app.realtime import manager from app.security import verify_password from app.userscope import is_messageable_user, messageable_clauses from app.utils import iso +from app.refusal import refused +from app.routes._common import not_found router = APIRouter(prefix="/api/messages", tags=["messages"]) @@ -152,7 +154,7 @@ def setup_keys( """Store this user's public key + passphrase-wrapped private key. Set-once: changing the key would orphan every existing conversation key, so re-setup is rejected here.""" if db.get(MessageKey, user.id) is not None: - raise HTTPException(status_code=409, detail="Secure messaging is already set up.") + raise refused(409, "messaging_set_up") db.add( MessageKey( user_id=user.id, @@ -185,11 +187,11 @@ def reset_keys( less (Google-only) account has nothing to re-verify, so it relies on its live session plus the client-side danger confirmation.""" if not _reset_key_limiter.allow(str(user.id)): - raise HTTPException(status_code=429, detail="Too many attempts. Try again shortly.") + raise refused(429, "too_many_attempts") if user.password_hash and not verify_password( payload.current_password or "", user.password_hash ): - raise HTTPException(status_code=403, detail="Current password is incorrect.") + raise refused(403, "wrong_password") # Delete the now-dead ciphertext first (both directions), then the wrapped keypair itself so a # fresh setup is allowed. Order doesn't matter (no FK between them), but do it in one commit. db.execute( @@ -223,10 +225,10 @@ def public_key( p = db.get(User, user_id) has_history = db.scalar(select(Message.id).where(cond).limit(1)) if p is None or (not is_messageable_user(p) and not has_history): - raise HTTPException(status_code=404, detail="User not found") + raise not_found("user") k = db.get(MessageKey, user_id) if k is None: - raise HTTPException(status_code=404, detail="That member hasn't set up messaging yet.") + raise refused(404, "no_messaging") return {"user_id": user_id, "public_key": k.public_key} @@ -344,7 +346,7 @@ def get_thread( # still opens). Otherwise the same 404 as a non-existent id → no enumeration oracle. has_history = db.scalar(select(Message.id).where(cond).limit(1)) if p is None or (not is_messageable_user(p) and not has_history): - raise HTTPException(status_code=404, detail="User not found") + raise not_found("user") partner = _serialize_user(p) msgs = ( db.execute(select(Message).where(cond).order_by(Message.created_at.asc())).scalars().all() @@ -374,20 +376,20 @@ def send_message( """Send an end-to-end-encrypted message. The server stores only the ciphertext the client produced, then pushes it live to the recipient (and the sender's other devices).""" if not _send_limiter.allow(f"msg:{user.id}"): - raise HTTPException(status_code=429, detail="Too many messages — slow down a moment.") + raise refused(429, "message_rate") ct = (payload.ciphertext or "").strip() iv = (payload.iv or "").strip() if not ct or not iv: - raise HTTPException(status_code=400, detail="Message is empty.") + raise refused(400, "message_empty") if len(ct) > MAX_CIPHERTEXT: - raise HTTPException(status_code=400, detail="Message is too long.") + raise refused(400, "message_too_long") if payload.recipient_id == user.id: - raise HTTPException(status_code=400, detail="You can't message yourself.") + raise refused(400, "message_self") recipient = db.get(User, payload.recipient_id) if not is_messageable_user(recipient): - raise HTTPException(status_code=404, detail="Recipient not available.") + raise not_found("recipient") if db.get(MessageKey, recipient.id) is None: - raise HTTPException(status_code=404, detail="That member hasn't set up messaging yet.") + raise refused(404, "no_messaging") m = Message(kind="user", sender_id=user.id, recipient_id=recipient.id, ciphertext=ct, iv=iv) db.add(m) db.commit() diff --git a/backend/app/routes/plex/_shared.py b/backend/app/routes/plex/_shared.py index 02ec2fc3..78319f9c 100644 --- a/backend/app/routes/plex/_shared.py +++ b/backend/app/routes/plex/_shared.py @@ -1,11 +1,11 @@ """Shared helpers, serializers and constants for the plex route package.""" -from fastapi import HTTPException from sqlalchemy.orm import Session from app import logs from app.models import PlexItem from app.plex.lookups import by_rating_key +from app.routes._common import not_found log = logs.get_logger(__name__) @@ -13,7 +13,7 @@ log = logs.get_logger(__name__) def item_or_404(db: Session, rating_key: str) -> PlexItem: it = by_rating_key(db, PlexItem, rating_key) if it is None: - raise HTTPException(status_code=404, detail="Unknown Plex item") + raise not_found("plex_item") return it diff --git a/backend/app/routes/plex/admin.py b/backend/app/routes/plex/admin.py index 23600b0b..e9a2af74 100644 --- a/backend/app/routes/plex/admin.py +++ b/backend/app/routes/plex/admin.py @@ -2,12 +2,12 @@ from datetime import datetime, timezone -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends from pydantic import BaseModel from sqlalchemy import select from sqlalchemy.orm import Session -from app import sysconfig +from app import logs, sysconfig from app.auth import admin_user, current_user from app.db import get_db from app.models import ( @@ -20,6 +20,9 @@ from app.plex import sync as plex_sync from app.plex import watch_sync as plex_watch from app.plex.client import PlexClient, PlexError, PlexNotConfigured from app.utils import iso +from app.refusal import refused + +log = logs.get_logger(__name__) router = APIRouter(prefix="/api/plex", tags=["plex"]) @@ -35,10 +38,15 @@ def test_connection(_: User = Depends(admin_user), db: Session = Depends(get_db) info = plex.server_info() sections = plex.sections() media_check = _media_check(db, plex, sections) - except PlexNotConfigured as e: - raise HTTPException(status_code=400, detail=str(e)) + except PlexNotConfigured: + raise refused(400, "plex_not_configured") from None except PlexError as e: - raise HTTPException(status_code=422, detail=f"Plex connection failed: {e}") + # What the upstream said is for the log, not the toast: it can quote the server URL back. + log.warning( + "the Plex connection test failed", + extra=logs.event("plex.admin.connection_failed", reason=type(e).__name__, error=str(e)), + ) + raise refused(422, "plex_unreachable") from None return { "ok": True, "server_name": info.get("friendlyName") or info.get("title1") or "Plex", @@ -129,7 +137,7 @@ def watch_link_set( admin token, so no separate Plex login. Enabling it for the first time runs the one-time "Plex is master" import immediately so the owner's existing Plex history shows up in Siftlode.""" if not sysconfig.get_bool(db, "plex_enabled"): - raise HTTPException(status_code=409, detail="The Plex module is disabled.") + raise refused(409, "plex_disabled") enabled = body.enabled link = db.execute(select(PlexLink).where(PlexLink.user_id == user.id)).scalars().first() if link is None: @@ -150,6 +158,6 @@ def watch_import_now(user: User = Depends(admin_user), db: Session = Depends(get intersection; Siftlode-only states are preserved. Idempotent.""" link = db.execute(select(PlexLink).where(PlexLink.user_id == user.id)).scalars().first() if link is None or not link.sync_enabled: - raise HTTPException(status_code=409, detail="Enable Plex watch-sync first.") + raise refused(409, "watch_sync_off") result = plex_watch.import_owner_watch_state(db, link) return {**_link_dict(link), "import": result} diff --git a/backend/app/routes/plex/collections.py b/backend/app/routes/plex/collections.py index ada9eb92..f22c9288 100644 --- a/backend/app/routes/plex/collections.py +++ b/backend/app/routes/plex/collections.py @@ -9,6 +9,7 @@ from pydantic import BaseModel from sqlalchemy import func, select from sqlalchemy.orm import Session +from app import logs from app.auth import admin_user, current_user from app.db import get_db from app.models import ( @@ -26,6 +27,10 @@ from app.plex.client import PlexClient, PlexError, PlexNotConfigured from app.plex.lookups import by_rating_key from app.plex.query import content_libs, runtime_by_key, top_art_keys from app.utils import like_escape +from app.refusal import refused +from app.routes._common import not_found + +log = logs.get_logger(__name__) # The most-restrictive → least ordering used to pick a collection's effective content rating from its # members (Plex shows one cert on the collection; we don't store it, so derive it). @@ -234,7 +239,7 @@ def collection_detail( endpoints below.""" col = by_rating_key(db, PlexCollection, rating_key) if col is None: - raise HTTPException(status_code=404, detail="Unknown collection") + raise not_found("collection") movies = ( db.execute( select(PlexItem) @@ -325,9 +330,9 @@ def collection_detail( def _editable_collection_or_403(db: Session, rating_key: str) -> PlexCollection: col = by_rating_key(db, PlexCollection, rating_key) if col is None: - raise HTTPException(status_code=404, detail="Unknown collection") + raise not_found("collection") if not _collection_editable(col): - raise HTTPException(status_code=403, detail="This collection is read-only") + raise refused(403, "collection_read_only") return col @@ -362,14 +367,13 @@ def _write_or_pending_error(wrote: bool, e: Exception) -> HTTPException: Both use 422, NOT a 5xx: the frontend treats 502/503/504 as "gateway unreachable" and raises a sticky offline banner (see api/core.ts RETRIABLE_GATEWAY). A 422 surfaces the detail as an ordinary action-error toast, which is what both of these are — the app answered, it didn't go unreachable.""" - if wrote: - detail = ( - "Collection updated in Plex, but refreshing the local copy failed; " - f"it will reconcile on the next sync ({e})" - ) - else: - detail = f"Plex write failed: {e}" - return HTTPException(status_code=422, detail=detail) + log.warning( + "a Plex collection write did not settle", + extra=logs.event( + "plex.collections.write_failed", wrote=wrote, reason=type(e).__name__, error=str(e) + ), + ) + return refused(422, "collection_pending_sync") if wrote else refused(422, "plex_write_failed") class CreateCollectionIn(BaseModel): @@ -389,7 +393,7 @@ def create_collection( title = body.title.strip() seed = body.item_rating_key if not title or not seed: - raise HTTPException(status_code=400, detail="title and item_rating_key are required") + raise refused(400, "collection_fields_required") if library: lib = ( db.execute(select(PlexLibrary).where(PlexLibrary.plex_key == library)).scalars().first() @@ -399,7 +403,7 @@ def create_collection( seed_item = by_rating_key(db, PlexItem, seed) or by_rating_key(db, PlexShow, seed) lib = db.get(PlexLibrary, seed_item.library_id) if seed_item else None if lib is None: - raise HTTPException(status_code=404, detail="Unknown library") + raise not_found("library") _reject_duplicate_name(db, lib.id, title) wrote = False try: @@ -473,7 +477,7 @@ def collection_rename( col = _editable_collection_or_403(db, rating_key) title = body.title.strip() if not title: - raise HTTPException(status_code=400, detail="title is required") + raise refused(400, "title_required") _reject_duplicate_name(db, col.library_id, title, exclude_rk=rating_key) lib = db.get(PlexLibrary, col.library_id) wrote = False @@ -502,7 +506,7 @@ def collection_delete( db.commit() except (PlexError, PlexNotConfigured) as e: db.rollback() - raise HTTPException(status_code=422, detail=f"Plex write failed: {e}") + raise _write_or_pending_error(False, e) from None return {"deleted": rating_key} @@ -521,13 +525,10 @@ def collection_set_editable( (IMDb/TMDb/…) can never be made editable — they're regenerated outside Plex.""" col = by_rating_key(db, PlexCollection, rating_key) if col is None: - raise HTTPException(status_code=404, detail="Unknown collection") + raise not_found("collection") want = body.editable if want and (col.smart or _collection_source(col) != "collection"): - raise HTTPException( - status_code=400, - detail="Smart / auto-list collections can't be made editable", - ) + raise refused(400, "collection_smart") col.editable = want db.commit() return _collection_card(col) diff --git a/backend/app/routes/plex/detail.py b/backend/app/routes/plex/detail.py index 8a52df0e..f621ba6b 100644 --- a/backend/app/routes/plex/detail.py +++ b/backend/app/routes/plex/detail.py @@ -5,7 +5,7 @@ import time from concurrent.futures import ThreadPoolExecutor from datetime import datetime, timezone -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends from sqlalchemy import func, select from sqlalchemy.orm import Session @@ -30,6 +30,7 @@ from app.plex.client import PlexClient, PlexError, PlexNotConfigured from app.plex.lookups import by_rating_key from app.routes.plex._shared import _norm_lang, item_or_404 from app.routes.plex.collections import _collection_source +from app.routes._common import not_found log = logs.get_logger(__name__) @@ -109,7 +110,7 @@ def show_detail( hu = _norm_lang(lang) == "hu" sh = by_rating_key(db, PlexShow, rating_key) if sh is None: - raise HTTPException(status_code=404, detail="Unknown Plex show") + raise not_found("show") seasons = ( db.execute( select(PlexSeason) diff --git a/backend/app/routes/plex/playback.py b/backend/app/routes/plex/playback.py index fbda4a1d..4114b6e5 100644 --- a/backend/app/routes/plex/playback.py +++ b/backend/app/routes/plex/playback.py @@ -39,6 +39,8 @@ from app.plex import stream as plex_stream from app.plex import watch_sync as plex_watch from app.plex.client import PlexClient, PlexError, PlexNotConfigured from app.routes.plex._shared import item_or_404 +from app.refusal import refused +from app.routes._common import not_found router = APIRouter(prefix="/api/plex", tags=["plex"]) @@ -49,7 +51,7 @@ def _require_session(request: Request) -> int: low-sensitivity; a valid session is enough.""" uid, _ = resolved_user_id(request) if not uid: - raise HTTPException(status_code=401, detail="Not authenticated") + raise refused(401, "not_authenticated") return uid @@ -78,15 +80,17 @@ def image(rating_key: str, request: Request, variant: str = "thumb") -> Response with SessionLocal() as db: key = imgcache.key_for(db, str(rating_key), v) if not key: - raise HTTPException(status_code=404, detail="No image") + raise not_found("image") plex = PlexClient(db) # reads base URL + token now; the fetch below needs no DB - except PlexNotConfigured as e: - raise HTTPException(status_code=400, detail=str(e)) + except PlexNotConfigured: + raise refused(400, "plex_not_configured") from None try: with plex: data, ct = plex.image_bytes(key, w, h) - except PlexError as e: - raise HTTPException(status_code=502, detail=f"Plex image fetch failed: {e}") + # silent-ok: a byte proxy for an ; the browser shows the placeholder and no reader sees the + # detail — the 502 is the whole answer, as for the person-image proxy below. + except PlexError: + raise refused(502, "image_fetch_failed") from None imgcache.write(cache_key, data, ct) return imgcache.response(data, ct) @@ -214,7 +218,7 @@ def person_image(u: str, request: Request) -> Response: Signed-session auth (no DB) so it, like /image, never holds a pool connection during the fetch.""" _require_session(request) if not u.startswith(plex_people.PERSON_IMG_HOST): - raise HTTPException(status_code=400, detail="Unsupported image host") + raise refused(400, "image_host") cache_key = "person_" + hashlib.sha1(u.encode()).hexdigest() hit = imgcache.read(cache_key) if hit: @@ -224,7 +228,7 @@ def person_image(u: str, request: Request) -> Response: r = c.get(u) r.raise_for_status() except httpx.HTTPError: - raise HTTPException(status_code=502, detail="Image fetch failed") + raise refused(502, "image_fetch_failed") ct = r.headers.get("content-type", "image/jpeg") imgcache.write(cache_key, r.content, ct) return imgcache.response(r.content, ct) @@ -444,7 +448,7 @@ def item_state( it = item_or_404(db, rating_key) status = body.status if status not in ("new", "watched", "hidden"): - raise HTTPException(status_code=400, detail="Invalid status") + raise refused(400, "invalid_status") st = ( db.execute( select(PlexState).where(PlexState.user_id == user.id, PlexState.item_id == it.id) @@ -546,7 +550,7 @@ def show_state( """Mark a whole show watched/unwatched (all its episodes) for the current user + push to Plex.""" sh = by_rating_key(db, PlexShow, rating_key) if sh is None: - raise HTTPException(status_code=404, detail="Unknown Plex show") + raise not_found("show") watched = body.watched eps = ( db.execute(select(PlexItem).where(PlexItem.show_id == sh.id, PlexItem.kind == "episode")) @@ -568,7 +572,7 @@ def season_state( """Mark a whole season watched/unwatched (all its episodes) for the current user + push to Plex.""" se = by_rating_key(db, PlexSeason, rating_key) if se is None: - raise HTTPException(status_code=404, detail="Unknown Plex season") + raise not_found("season") watched = body.watched eps = ( db.execute(select(PlexItem).where(PlexItem.season_id == se.id, PlexItem.kind == "episode")) @@ -606,10 +610,10 @@ def stream_session( "start_s": 0.0, } if plex_paths.local_media_path(db, it.file_path) is None: - raise HTTPException(status_code=404, detail="Media file not found on disk") + raise refused(404, "media_missing") s = plex_stream.start_session(db, it, user.id, start, audio, aoff, multi, transcode=transcode) if s is None: - raise HTTPException(status_code=404, detail="Media file not found on disk") + raise refused(404, "media_missing") # Report the REAL media start (the keyframe ffmpeg landed on, which hls.js zero-bases to), not the # requested offset — the client's absolute clock and the subtitle cue-shift both key off this. # `transcode`/`hw`/`tonemap` let the player show an honest cold-start ("Preparing stream…") and @@ -640,7 +644,7 @@ def stream_file( it = item_or_404(db, rating_key) src = plex_paths.local_media_path(db, it.file_path) if src is None: - raise HTTPException(status_code=404, detail="Media file not found on disk") + raise refused(404, "media_missing") if download: # A clean, safe download name from the title + the real file extension. ext = src.suffix or (f".{it.container}" if it.container else "") @@ -694,10 +698,10 @@ def stream_hls_file( or "\\" in filename or ".." in filename ): - raise HTTPException(status_code=404, detail="Not found") + raise not_found("file") s = plex_stream.current_session(user.id, str(rating_key)) if s is None or s.id != session_id: - raise HTTPException(status_code=404, detail="No active playback session") + raise refused(404, "session_gone") path = s.dir / filename # Only a name THIS session can actually produce is worth waiting for: its own playlists, and a # segment of one of its own renditions whose predecessor is already on disk (ffmpeg writes them @@ -707,14 +711,14 @@ def stream_hls_file( # past the produced region — or a deliberate flood of hopeless names — from parking threadpool # workers and their DB connections (see `artifact_may_arrive`). if not plex_stream.artifact_may_arrive(s, filename): - raise HTTPException(status_code=404, detail="Not available yet") + raise refused(404, "not_ready") timeout = 30 if ext == ".m3u8" else 20 # `proc` so a session whose ffmpeg has already exited answers at once instead of polling out the # whole timeout: this is a SYNC route on the ~40-slot threadpool, holding a pooled DB connection # with it, so every such wait is a worker (and a connection) parked for 20 s — an ordinary seek # past the produced region was enough to do it (round 5). if not plex_stream.wait_for(path, timeout, proc=s.proc): - raise HTTPException(status_code=404, detail="Not available yet") + raise refused(404, "not_ready") try: content = path.read_bytes() except OSError: @@ -725,7 +729,7 @@ def stream_hls_file( # restarting at the playhead. Ordinary trigger: `loadSession` awaits the new session — which # kills and rmtrees the old one — before destroying the old hls instance, so the outgoing # instance's in-flight segment request races the rmtree on every seek-restart. - raise HTTPException(status_code=404, detail="No active playback session") from None + raise refused(404, "session_gone") from None return Response( content=content, media_type=_HLS_CT[ext], diff --git a/backend/app/routes/plex/playlists.py b/backend/app/routes/plex/playlists.py index 9728b1d3..9cd61fd5 100644 --- a/backend/app/routes/plex/playlists.py +++ b/backend/app/routes/plex/playlists.py @@ -2,7 +2,7 @@ from datetime import datetime, timezone -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends from pydantic import BaseModel from sqlalchemy import delete, func, select, update from sqlalchemy.orm import Session @@ -19,7 +19,8 @@ from app.models import ( ) from app.plex.cards import leaf_card from app.plex.lookups import by_rating_key -from app.routes._common import owned_or_404 +from app.routes._common import owned_or_404, not_found +from app.refusal import refused router = APIRouter(prefix="/api/plex", tags=["plex"]) @@ -116,7 +117,7 @@ def create_playlist( ) -> dict: title = body.title.strip() if not title: - raise HTTPException(status_code=400, detail="title is required") + raise refused(400, "title_required") pl = PlexPlaylist(user_id=user.id, title=title) db.add(pl) db.flush() @@ -182,7 +183,7 @@ def rename_playlist( pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") title = body.title.strip() if not title: - raise HTTPException(status_code=400, detail="title is required") + raise refused(400, "title_required") pl.title = title db.commit() count = ( @@ -219,7 +220,7 @@ def playlist_add_item( pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") it = by_rating_key(db, PlexItem, body.item_rating_key) if it is None: - raise HTTPException(status_code=404, detail="Unknown item") + raise not_found("item") exists = ( db.execute( select(PlexPlaylistItem).where( diff --git a/backend/app/routes/public.py b/backend/app/routes/public.py index e580a77c..7dafd1fb 100644 --- a/backend/app/routes/public.py +++ b/backend/app/routes/public.py @@ -6,7 +6,7 @@ the app or any account data. Password-protected links exchange the password once a short-lived signed grant that the media URL carries (a `