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.
This commit is contained in:
@@ -60,14 +60,17 @@ def owned_or_404(
|
|||||||
return obj
|
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
|
"""The answer to an action YouTube refused: a 422 whose reason the UI translates
|
||||||
(`errors.youtube.<reason>`). One definition, because the routes had grown three shapes of it —
|
(`errors.youtube.<reason>`). 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
|
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
|
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
|
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."""
|
YouTube being unavailable to us — the retryable answer. `playlist`: the route acts on one
|
||||||
reason = refusal_reason(exc) if isinstance(exc, YouTubeError) else "unavailable"
|
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})
|
return HTTPException(status_code=422, detail={"code": "youtube", "reason": reason})
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -372,7 +372,7 @@ def revert_youtube(
|
|||||||
raise not_found("playlist") from None
|
raise not_found("playlist") from None
|
||||||
except YouTubeError as exc:
|
except YouTubeError as exc:
|
||||||
db.rollback()
|
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:
|
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):
|
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH):
|
||||||
plan = plan_merge(db, user, pl)
|
plan = plan_merge(db, user, pl)
|
||||||
except YouTubeError as exc:
|
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["remaining_today"] = quota.remaining_today(db)
|
||||||
plan["affordable"] = plan["units_estimate"] <= plan["remaining_today"]
|
plan["affordable"] = plan["units_estimate"] <= plan["remaining_today"]
|
||||||
return plan
|
return plan
|
||||||
@@ -439,7 +439,7 @@ def push(
|
|||||||
raise not_found("playlist") from None
|
raise not_found("playlist") from None
|
||||||
except YouTubeError as exc:
|
except YouTubeError as exc:
|
||||||
db.rollback()
|
db.rollback()
|
||||||
raise youtube_refusal(exc) from exc
|
raise youtube_refusal(exc, playlist=True) from exc
|
||||||
|
|
||||||
|
|
||||||
@router.get("/{playlist_id}")
|
@router.get("/{playlist_id}")
|
||||||
@@ -539,7 +539,7 @@ def delete_playlist(
|
|||||||
with YouTubeClient(db, user) as yt:
|
with YouTubeClient(db, user) as yt:
|
||||||
yt.delete_playlist(pl.yt_playlist_id)
|
yt.delete_playlist(pl.yt_playlist_id)
|
||||||
except YouTubeError as exc:
|
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.
|
# The playlist first, as every item write: the cascade deletes the items before the list row.
|
||||||
lock_playlists(db, pl.id)
|
lock_playlists(db, pl.id)
|
||||||
db.delete(pl) # items cascade
|
db.delete(pl) # items cascade
|
||||||
|
|||||||
@@ -25,7 +25,7 @@ from app.models import (
|
|||||||
)
|
)
|
||||||
from app.sync.videos import insert_stubs, stub_from_playlist_item, apply_video_details
|
from app.sync.videos import insert_stubs, stub_from_playlist_item, apply_video_details
|
||||||
from app.utils import now_utc as _now
|
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__)
|
log = logs.get_logger(__name__)
|
||||||
|
|
||||||
@@ -81,7 +81,15 @@ def explore_ingest_page(
|
|||||||
the organic deep backfill) is never touched."""
|
the organic deep backfill) is never touched."""
|
||||||
if not channel.uploads_playlist_id:
|
if not channel.uploads_playlist_id:
|
||||||
return {"ingested": 0, "next_page_token": None}
|
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] = []
|
stubs: list[dict] = []
|
||||||
for item in data.get("items", []):
|
for item in data.get("items", []):
|
||||||
stub = stub_from_playlist_item(item, channel.id)
|
stub = stub_from_playlist_item(item, channel.id)
|
||||||
|
|||||||
@@ -26,7 +26,7 @@ from app.sync.playlist_merge import merge_plan
|
|||||||
from app.sync.runner import token_users
|
from app.sync.runner import token_users
|
||||||
from app.sync.subscriptions import blank_listing, refuse_blank_listing, vanished
|
from app.sync.subscriptions import blank_listing, refuse_blank_listing, vanished
|
||||||
from app.sync.videos import apply_video_details, parse_dt
|
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__)
|
log = logs.get_logger(__name__)
|
||||||
# Cost of a single YouTube write op (playlists/playlistItems insert/update/delete).
|
# 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)
|
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:
|
def pull_user_playlists(db: Session, user: User) -> dict:
|
||||||
"""The mirror pull itself; the caller holds `user_sync_claim`."""
|
"""The mirror pull itself; the caller holds `user_sync_claim`."""
|
||||||
synced = 0
|
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 user is pushing or re-pulling this very list: it writes the same rows.
|
||||||
# The next scheduled run refreshes it.
|
# The next scheduled run refreshes it.
|
||||||
continue
|
continue
|
||||||
ids = list(yt.iter_my_playlist_video_ids(ytid))
|
ids = _mirror_listing(yt, ytid, p.get("item_count"), pl.id)
|
||||||
if blank_listing(ids, p.get("item_count")):
|
if ids is None:
|
||||||
log.warning("YouTube listed no items for non-empty playlist %s; kept", pl.id)
|
db.rollback() # a mirror this run just created is not kept empty either
|
||||||
continue
|
continue
|
||||||
# Ingest first — it commits — so the row lock below is held from the dirty check
|
# Ingest first — it commits — so the row lock below is held from the dirty check
|
||||||
# to the commit that replaces the items.
|
# to the commit that replaces the items.
|
||||||
|
|||||||
@@ -117,17 +117,28 @@ _DEAD_GRANT = "invalid_grant"
|
|||||||
YouTubeRefusal = Literal["quota", "reconnect", "unavailable", "gone"]
|
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
|
"""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
|
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
|
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
|
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
|
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):
|
if is_quota_refusal(exc):
|
||||||
return "quota"
|
return "quota"
|
||||||
if exc.reason == "playlistNotFound":
|
if playlist and is_playlist_gone(exc):
|
||||||
# The list itself is gone (deleted on YouTube): neither a retry nor a reconnect helps.
|
|
||||||
return "gone"
|
return "gone"
|
||||||
if exc.path == "oauth2/token":
|
if exc.path == "oauth2/token":
|
||||||
return "reconnect" if exc.reason == _DEAD_GRANT else "unavailable"
|
return "reconnect" if exc.reason == _DEAD_GRANT else "unavailable"
|
||||||
|
|||||||
@@ -163,6 +163,33 @@ def test_a_playlist_answer_with_something_in_it_still_drops_a_vanished_mirror(
|
|||||||
assert "PL1" not in ytids
|
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):
|
def _mirror_with_an_item(db, user, **kw):
|
||||||
db.add(Channel(id="UC_pl", title="C"))
|
db.add(Channel(id="UC_pl", title="C"))
|
||||||
db.flush()
|
db.flush()
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ import pytest
|
|||||||
|
|
||||||
from app.auth import has_read_scope
|
from app.auth import has_read_scope
|
||||||
from app.models import OAuthToken
|
from app.models import OAuthToken
|
||||||
|
from app.sync.explore import explore_ingest_page
|
||||||
from app.youtube import client as yt
|
from app.youtube import client as yt
|
||||||
from app.youtube.client import YouTubeError, refusal_reason
|
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("No refresh token; user must re-authenticate"), "reconnect"),
|
||||||
(YouTubeError("m", reason="transport"), "unavailable"),
|
(YouTubeError("m", reason="transport"), "unavailable"),
|
||||||
(YouTubeError("m", reason="backendError", path="subscriptions"), "unavailable"),
|
(YouTubeError("m", reason="backendError", path="subscriptions"), "unavailable"),
|
||||||
# A playlist deleted on YouTube: neither a retry nor a reconnect helps (it said "reconnect").
|
# A missing playlist the user never named (a channel's uploads): nothing they can act on.
|
||||||
(YouTubeError("m", reason="playlistNotFound", path="playlists"), "gone"),
|
(YouTubeError("m", reason="playlistNotFound", path="playlistItems"), "unavailable"),
|
||||||
(YouTubeError("m", reason="emptyAnswer", 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
|
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(
|
@pytest.mark.parametrize(
|
||||||
("route", "target"),
|
("route", "target"),
|
||||||
[
|
[
|
||||||
|
|||||||
Reference in New Issue
Block a user