From 79704b0001f9eee7db3bd7c7b3886445353a1be7 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 4 Oct 2026 04:16:52 +0200 Subject: [PATCH] fix(youtube): "this playlist no longer exists" only where the user picked that list playlistNotFound was mapped to "gone" everywhere: exploring a channel without uploads (its uploads list answers not found) and a list deleted mid bulk pull both said a playlist no longer exists. The reason is now "gone" only on single-playlist routes; explore reads it as an empty back-catalogue, and the bulk pull skips that list and syncs the rest. --- backend/app/routes/_common.py | 9 ++++-- backend/app/routes/playlists.py | 8 ++--- backend/app/sync/explore.py | 12 ++++++-- backend/app/sync/playlists.py | 26 +++++++++++++--- backend/app/youtube/client.py | 19 +++++++++--- backend/tests/test_empty_listing_guards.py | 27 +++++++++++++++++ backend/tests/test_youtube_refusals.py | 35 ++++++++++++++++++++-- 7 files changed, 117 insertions(+), 19 deletions(-) diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py index f4105db3..0fe3ff64 100644 --- a/backend/app/routes/_common.py +++ b/backend/app/routes/_common.py @@ -60,14 +60,17 @@ def owned_or_404( return obj -def youtube_refusal(exc: Exception) -> HTTPException: +def youtube_refusal(exc: Exception, *, playlist: bool = False) -> 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`). 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" + YouTube being unavailable to us — the retryable answer. `playlist`: the route acts on one + playlist the user picked, so a missing playlist is THAT one, gone (`refusal_reason`).""" + reason = ( + refusal_reason(exc, playlist=playlist) if isinstance(exc, YouTubeError) else "unavailable" + ) return HTTPException(status_code=422, detail={"code": "youtube", "reason": reason}) diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 123ab40b..bc2f79cb 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -372,7 +372,7 @@ def revert_youtube( raise not_found("playlist") from None except YouTubeError as exc: db.rollback() - raise youtube_refusal(exc) from exc + raise youtube_refusal(exc, playlist=True) from exc def _pushable(db: Session, user: User, playlist_id: int) -> Playlist: @@ -398,7 +398,7 @@ def push_plan( with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH): plan = plan_merge(db, user, pl) except YouTubeError as exc: - raise youtube_refusal(exc) from exc + raise youtube_refusal(exc, playlist=True) from exc plan["remaining_today"] = quota.remaining_today(db) plan["affordable"] = plan["units_estimate"] <= plan["remaining_today"] return plan @@ -439,7 +439,7 @@ def push( raise not_found("playlist") from None except YouTubeError as exc: db.rollback() - raise youtube_refusal(exc) from exc + raise youtube_refusal(exc, playlist=True) from exc @router.get("/{playlist_id}") @@ -539,7 +539,7 @@ def delete_playlist( with YouTubeClient(db, user) as yt: yt.delete_playlist(pl.yt_playlist_id) except YouTubeError as exc: - raise youtube_refusal(exc) from exc + raise youtube_refusal(exc, playlist=True) from exc # The playlist first, as every item write: the cascade deletes the items before the list row. lock_playlists(db, pl.id) db.delete(pl) # items cascade diff --git a/backend/app/sync/explore.py b/backend/app/sync/explore.py index 75b2aa45..f9bcc0b6 100644 --- a/backend/app/sync/explore.py +++ b/backend/app/sync/explore.py @@ -25,7 +25,7 @@ from app.models import ( ) from app.sync.videos import insert_stubs, stub_from_playlist_item, apply_video_details from app.utils import now_utc as _now -from app.youtube.client import YouTubeClient +from app.youtube.client import YouTubeClient, YouTubeError, is_playlist_gone log = logs.get_logger(__name__) @@ -81,7 +81,15 @@ def explore_ingest_page( the organic deep backfill) is never touched.""" if not channel.uploads_playlist_id: return {"ingested": 0, "next_page_token": None} - data = yt.get_playlist_items(channel.uploads_playlist_id, page_token) + try: + data = yt.get_playlist_items(channel.uploads_playlist_id, page_token) + except YouTubeError as exc: + # A channel with no uploads has an uploads id that YouTube answers as not found: an empty + # back-catalogue, not a failure (and not a "playlist gone" — the user picked a channel). + if not is_playlist_gone(exc): + raise + log.info("explore ingest channel=%s has no uploads playlist", channel.id) + return {"ingested": 0, "next_page_token": None} stubs: list[dict] = [] for item in data.get("items", []): stub = stub_from_playlist_item(item, channel.id) diff --git a/backend/app/sync/playlists.py b/backend/app/sync/playlists.py index 255b4bbf..eff61053 100644 --- a/backend/app/sync/playlists.py +++ b/backend/app/sync/playlists.py @@ -26,7 +26,7 @@ from app.sync.playlist_merge import merge_plan from app.sync.runner import token_users from app.sync.subscriptions import blank_listing, refuse_blank_listing, vanished from app.sync.videos import apply_video_details, parse_dt -from app.youtube.client import YouTubeClient, YouTubeError +from app.youtube.client import YouTubeClient, YouTubeError, is_playlist_gone log = logs.get_logger(__name__) # Cost of a single YouTube write op (playlists/playlistItems insert/update/delete). @@ -319,6 +319,24 @@ def user_sync_claim(db: Session, user_id: int) -> AbstractContextManager[bool]: return _advisory_claim(db, _USER_SYNC_LOCK_NS, user_id) +def _mirror_listing(yt: YouTubeClient, ytid: str, count: object, pl_id: int) -> list[str] | None: + """A mirror's video ids from YouTube, or None when there is nothing to believe: the list was + deleted between the pull's listing and this read (the next pull drops the mirror as vanished — + not a reason to fail every other list, E32 review round 2), or YouTube listed no items for a + list it counts as non-empty (`blank_listing`).""" + try: + ids = list(yt.iter_my_playlist_video_ids(ytid)) + except YouTubeError as exc: + if not is_playlist_gone(exc): + raise + log.info("YouTube playlist of %s vanished mid-pull; skipped", pl_id) + return None + if blank_listing(ids, count): + log.warning("YouTube listed no items for non-empty playlist %s; kept", pl_id) + return None + return ids + + def pull_user_playlists(db: Session, user: User) -> dict: """The mirror pull itself; the caller holds `user_sync_claim`.""" synced = 0 @@ -377,9 +395,9 @@ def pull_user_playlists(db: Session, user: User) -> dict: # The user is pushing or re-pulling this very list: it writes the same rows. # The next scheduled run refreshes it. continue - ids = list(yt.iter_my_playlist_video_ids(ytid)) - if blank_listing(ids, p.get("item_count")): - log.warning("YouTube listed no items for non-empty playlist %s; kept", pl.id) + ids = _mirror_listing(yt, ytid, p.get("item_count"), pl.id) + if ids is None: + db.rollback() # a mirror this run just created is not kept empty either continue # Ingest first — it commits — so the row lock below is held from the dirty check # to the commit that replaces the items. diff --git a/backend/app/youtube/client.py b/backend/app/youtube/client.py index c2eae421..400ea78c 100644 --- a/backend/app/youtube/client.py +++ b/backend/app/youtube/client.py @@ -117,17 +117,28 @@ _DEAD_GRANT = "invalid_grant" YouTubeRefusal = Literal["quota", "reconnect", "unavailable", "gone"] -def refusal_reason(exc: YouTubeError) -> YouTubeRefusal: +def is_playlist_gone(exc: YouTubeError) -> bool: + """Whether YouTube answered that the playlist read or written does not exist. Only the caller + knows which playlist that was: a user's list deleted on YouTube, or a channel's uploads list, + which YouTube also answers so for a channel with no uploads at all.""" + return "playlistNotFound" in exc.reason.split(",") + + +def refusal_reason(exc: YouTubeError, *, playlist: bool = False) -> YouTubeRefusal: """Sort a failure by what the user can do about it. A dead grant (Google refused the refresh with `invalid_grant`, which also drops the grant — see `_drop_dead_grant`) or 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. Any other token-endpoint failure — a 5xx, a 429, a misconfigured client — leaves the grant as it was: reconnecting would not fix it, and - the account would still read as connected, with no connect button to press.""" + the account would still read as connected, with no connect button to press. + + `playlist`: the action was on ONE playlist the user picked, so a missing playlist is that list, + gone (deleted on YouTube) — neither a retry nor a reconnect helps. Anywhere else the playlist + is one the user never named (a channel's uploads, one list of a bulk pull), and "this playlist + no longer exists" would describe nothing on their screen (E32 review round 2).""" if is_quota_refusal(exc): return "quota" - if exc.reason == "playlistNotFound": - # The list itself is gone (deleted on YouTube): neither a retry nor a reconnect helps. + if playlist and is_playlist_gone(exc): return "gone" if exc.path == "oauth2/token": return "reconnect" if exc.reason == _DEAD_GRANT else "unavailable" diff --git a/backend/tests/test_empty_listing_guards.py b/backend/tests/test_empty_listing_guards.py index 139200a6..57a2c92d 100644 --- a/backend/tests/test_empty_listing_guards.py +++ b/backend/tests/test_empty_listing_guards.py @@ -163,6 +163,33 @@ def test_a_playlist_answer_with_something_in_it_still_drops_a_vanished_mirror( assert "PL1" not in ytids +class _YTOneGone(_YT): + """A list deleted on YouTube between the listing and the read of its items.""" + + def iter_my_playlist_video_ids(self, ytid): + if ytid == "PLgone": + raise YouTubeError("m", reason="playlistNotFound", path="playlistItems") + return [] + + +def test_a_list_deleted_mid_pull_is_skipped_and_the_rest_still_sync(db, make_user, monkeypatch): + # Not "this playlist no longer exists" for a bulk pull the user never named a list in, and not + # a failure for every other list either (E32 review round 2): the next pull drops the mirror. + user = make_user() + db.commit() + listing = [ + {"id": "PLgone", "title": "Gone", "item_count": 3}, + {"id": "PL2", "title": "Kept", "item_count": 0}, + ] + monkeypatch.setattr(plsync, "YouTubeClient", _YTOneGone(listing)) + assert plsync.pull_user_playlists(db, user) == {"synced": 1} + db.expire_all() + ytids = set( + db.execute(select(Playlist.yt_playlist_id).where(Playlist.user_id == user.id)).scalars() + ) + assert ytids == {"PL2"} + + def _mirror_with_an_item(db, user, **kw): db.add(Channel(id="UC_pl", title="C")) db.flush() diff --git a/backend/tests/test_youtube_refusals.py b/backend/tests/test_youtube_refusals.py index 15f67b55..356e67f8 100644 --- a/backend/tests/test_youtube_refusals.py +++ b/backend/tests/test_youtube_refusals.py @@ -9,6 +9,7 @@ import pytest from app.auth import has_read_scope from app.models import OAuthToken +from app.sync.explore import explore_ingest_page from app.youtube import client as yt from app.youtube.client import YouTubeError, refusal_reason @@ -27,8 +28,8 @@ from app.youtube.client import YouTubeError, refusal_reason (YouTubeError("No refresh token; user must re-authenticate"), "reconnect"), (YouTubeError("m", reason="transport"), "unavailable"), (YouTubeError("m", reason="backendError", path="subscriptions"), "unavailable"), - # A playlist deleted on YouTube: neither a retry nor a reconnect helps (it said "reconnect"). - (YouTubeError("m", reason="playlistNotFound", path="playlists"), "gone"), + # A missing playlist the user never named (a channel's uploads): nothing they can act on. + (YouTubeError("m", reason="playlistNotFound", path="playlistItems"), "unavailable"), (YouTubeError("m", reason="emptyAnswer", path="playlistItems"), "unavailable"), ], ) @@ -36,6 +37,36 @@ def test_a_failure_is_sorted_by_what_the_user_can_do(exc, reason): assert refusal_reason(exc) == reason +def test_a_missing_playlist_is_gone_only_on_a_route_about_that_playlist(): + # A playlist deleted on YouTube: neither a retry nor a reconnect helps (it said "reconnect"). + # Said only where the user picked that one list — anywhere else "this playlist no longer + # exists" names nothing on their screen (E32 review round 2). + exc = YouTubeError("m", reason="playlistNotFound", path="playlists") + assert refusal_reason(exc, playlist=True) == "gone" + assert refusal_reason(exc) == "unavailable" + assert refusal_reason(YouTubeError("m", reason="quotaExceeded"), playlist=True) == "quota" + + +def test_a_channel_without_uploads_explores_as_an_empty_page(): + # YouTube answers a channel's uploads playlist as not found when the channel has no uploads. + class _Yt: + def get_playlist_items(self, *_): + raise YouTubeError("m", reason="playlistNotFound", path="playlistItems") + + channel = SimpleNamespace(id="UCempty", uploads_playlist_id="UUempty") + assert explore_ingest_page(None, _Yt(), channel) == {"ingested": 0, "next_page_token": None} + + +def test_any_other_explore_failure_still_raises(): + class _Yt: + def get_playlist_items(self, *_): + raise YouTubeError("m", reason="quotaExceeded", path="playlistItems") + + channel = SimpleNamespace(id="UCx", uploads_playlist_id="UUx") + with pytest.raises(YouTubeError): + explore_ingest_page(None, _Yt(), channel) + + @pytest.mark.parametrize( ("route", "target"), [