From 528774d6405ce6accd134c62fda6a4b4e9a8acd6 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 27 Sep 2026 22:34:14 +0200 Subject: [PATCH] fix(playlists): a push stops at a list deleted mid-way, and a rename made during it stays unsynced A delete landing while a push read YouTube left a plan over no items, so the push emptied the YouTube playlist a local-only delete keeps: the push now stops at a vanished list (PlaylistGone, 404), and a delete is refused while a sync of the list runs. The closing baseline fingerprints the title YouTube got, not the one its lock re-reads, so a rename made during the writes stays dirty. The other-tab test helper bounds its lock wait, so a lock-order regression fails instead of hanging. --- backend/app/routes/playlists.py | 44 +++++++---- backend/app/sync/playlists.py | 78 +++++++++++++------ backend/tests/test_playlist_sync_claim.py | 5 ++ .../tests/test_playlist_sync_lock_order.py | 56 ++++++++++++- 4 files changed, 142 insertions(+), 41 deletions(-) diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 98e89d65..b2e33b51 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -14,6 +14,7 @@ from app.db import get_db from app.feedquery import feed_columns, serialize from app.models import Channel, Playlist, PlaylistItem, User, Video, VideoState from app.sync.playlists import ( + PlaylistGone, lock_playlists, merge_playlist, plan_merge, @@ -24,7 +25,7 @@ from app.sync.playlists import ( user_sync_claim, ) from app.ratelimit import RateLimiter -from app.routes._common import get_or_404, owned_or_404, youtube_refusal +from app.routes._common import get_or_404, not_found, owned_or_404, youtube_refusal from app.youtube.client import YouTubeClient, YouTubeError router = APIRouter(prefix="/api/playlists", tags=["playlists"]) @@ -367,6 +368,8 @@ def revert_youtube( try: with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PULL): return {"ok": True, **repull_playlist(db, user, pl)} + except PlaylistGone: + raise not_found("playlist") except YouTubeError as exc: db.rollback() raise youtube_refusal(exc) @@ -432,6 +435,8 @@ def push( if plan["units_estimate"] > quota.remaining_today(db): raise _refused(429, "quota_exceeded") return {"ok": True, **merge_playlist(db, user, pl, live=live)} + except PlaylistGone: + raise not_found("playlist") except YouTubeError as exc: db.rollback() raise youtube_refusal(exc) @@ -517,21 +522,28 @@ def delete_playlist( 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: - if not pl.yt_playlist_id: - raise _refused(400, "not_on_youtube") - if not has_write_scope(user): - raise _refused(403, "youtube_editing_off") - try: - with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_DELETE): - with YouTubeClient(db, user) as yt: - yt.delete_playlist(pl.yt_playlist_id) - except YouTubeError as exc: - raise youtube_refusal(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 - db.commit() + # Not in the middle of a sync of it (`sync_claim`): a push that read the list before the delete + # and wrote YouTube after it planned from no items at all, so it emptied the YouTube playlist a + # local-only delete means to keep. The push checks for a vanished list too (`PlaylistGone`); + # this refuses the delete while a sync runs rather than race it. + with sync_claim(db, pl.id) as free: + if not free: + raise _refused(409, "sync_busy") + if on_youtube: + if not pl.yt_playlist_id: + raise _refused(400, "not_on_youtube") + if not has_write_scope(user): + raise _refused(403, "youtube_editing_off") + try: + with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_DELETE): + with YouTubeClient(db, user) as yt: + yt.delete_playlist(pl.yt_playlist_id) + except YouTubeError as exc: + raise youtube_refusal(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 + db.commit() return {"deleted": playlist_id} diff --git a/backend/app/sync/playlists.py b/backend/app/sync/playlists.py index bde91b00..4c16e8e8 100644 --- a/backend/app/sync/playlists.py +++ b/backend/app/sync/playlists.py @@ -78,21 +78,37 @@ def sync_claim(db: Session, playlist_id: int) -> AbstractContextManager[bool]: return _advisory_claim(db, _SYNC_LOCK_NS, playlist_id) -def lock_playlists(db: Session, *playlist_ids: int) -> None: +class PlaylistGone(Exception): + """The list a sync was working on was deleted meanwhile (another tab, the mirror drop). Its + items went with it, so a plan made now reads "every item removed here" — and would delete them + all from the YouTube playlist the reader chose to keep. The route answers 404.""" + + +def lock_playlists(db: Session, *playlist_ids: int) -> set[int]: """Row-lock playlists before touching their items, several in id order. Every item write takes the PLAYLIST first — the route edits, a push, a re-pull, the scheduled mirror refresh and a delete alike: a writer that locked an item row first and the playlist second (a push's item replace beside a remove) deadlocks against one that went the other way, and the loser is a 500. The rows are re-read, so `dirty`, the name and the sync baseline are the ones a finished writer committed. Held to the caller's next commit — so no commit may come between this and - the write it guards.""" - db.scalars( + the write it guards. Returns the ids it found: a list deleted meanwhile is not among them.""" + rows = db.scalars( select(Playlist) .where(Playlist.id.in_(sorted(set(playlist_ids)))) .order_by(Playlist.id) .with_for_update() .execution_options(populate_existing=True) ).all() # consumed: rows fetched are what refreshes the loaded objects + return {pl.id for pl in rows} + + +def _lock_or_gone(db: Session, pl: Playlist) -> None: + """`lock_playlists` for a sync that goes on to write YouTube or the items: a list deleted since + it was read raises `PlaylistGone` (rolled back) instead of being planned as an empty one.""" + pl_id = pl.id # read first: the rollback expires `pl`, and its row is gone + if pl_id not in lock_playlists(db, pl_id): + db.rollback() + raise PlaylistGone(pl_id) def fingerprint(name: str, ids: list[str]) -> str: @@ -158,11 +174,18 @@ def parse_unavailable(pl: Playlist) -> set[str]: return _load_baseline(pl)[1] -def _set_baseline(pl: Playlist, ids: list[str], unavailable: list[str] | None = None) -> None: +def _set_baseline( + pl: Playlist, + ids: list[str], + unavailable: list[str] | None = None, + *, + name: str | None = None, +) -> None: """Record `ids` as both the hash (drives `dirty`, over the local ids only) and the raw ordered set (drives merge). `unavailable` = un-ingestable-but-live YouTube ids to carry forward so the - next merge skips them; stored only when non-empty (keeps the common case a plain list).""" - pl.synced_fingerprint = fingerprint(pl.name, ids) + next merge skips them; stored only when non-empty (keeps the common case a plain list). `name`: + the title YouTube now has, when that may differ from the list's own (see the push).""" + pl.synced_fingerprint = fingerprint(pl.name if name is None else name, ids) if unavailable: pl.synced_video_ids = json.dumps({"ids": ids, "unavailable": list(unavailable)}) else: @@ -171,14 +194,21 @@ def _set_baseline(pl: Playlist, ids: list[str], unavailable: list[str] | None = def _set_baseline_under_lock( - db: Session, pl: Playlist, ids: list[str], unavailable: list[str] | None = None + db: Session, + pl: Playlist, + ids: list[str], + unavailable: list[str] | None = None, + *, + name: str, ) -> None: - """A push's closing baseline: `ids` is what YouTube now holds. The push spent seconds on - YouTube writes since it replaced the local items, so an edit may have landed meanwhile — lock - the list and derive `dirty` from what is there, or the edit would read as synced and never - reach YouTube.""" - lock_playlists(db, pl.id) - _set_baseline(pl, ids, unavailable) + """A push's closing baseline: `ids` and `name` are what YouTube now holds. The push spent + seconds on YouTube writes since it replaced the local items, so an edit may have landed + meanwhile — lock the list and derive `dirty` from what is there, or the edit would read as + synced and never reach YouTube. The name too: the lock re-reads a rename made meanwhile, and a + baseline taken over THAT name matched it, so the list read clean with YouTube's title still the + old one (E28 review round 4).""" + _lock_or_gone(db, pl) + _set_baseline(pl, ids, unavailable, name=name) recompute_dirty(db, pl) @@ -599,7 +629,8 @@ def merge_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: breaker = _Breaker(playlist_id=pl.id) with YouTubeClient(db, user) as yt: if not pl.yt_playlist_id: - pl.yt_playlist_id = yt.create_playlist(pl.name) + pushed_name = pl.name + pl.yt_playlist_id = yt.create_playlist(pushed_name) db.commit() created = 1 # Brand-new playlist: just append every local video in order. @@ -612,7 +643,7 @@ def merge_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: if not isinstance(added, _Failed): inserted += 1 if not failures: - _set_baseline_under_lock(db, pl, local) + _set_baseline_under_lock(db, pl, local, name=pushed_name) db.commit() return { "created": created, @@ -656,8 +687,9 @@ def merge_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: # `local` was read before the YouTube calls above: an add or remove committed meanwhile # (another tab) would be undone by the replace below. Lock the list — the ingest's commit is # behind us — and plan again from what is there now. Nothing new needs ingesting: a video - # the re-read adds is already a local row. - lock_playlists(db, pl.id) + # the re-read adds is already a local row. A list deleted meanwhile stops here, before a plan + # over its now-empty items removes them all from YouTube. + _lock_or_gone(db, pl) local = _local_video_ids(db, pl) mp = merge_plan(parse_baseline(pl), local, yt_ids, effective_unavailable) # Only the ids that actually exist in the catalog can become local rows (a private/deleted @@ -673,10 +705,12 @@ def merge_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: _set_local_items(db, pl, final_local) db.commit() - # --- YouTube side (metered, breaker-guarded). - if snippet is not None and (snippet.get("title") or "") != pl.name: + # --- YouTube side (metered, breaker-guarded). The name as of the lock above: the closing + # baseline records THIS one, whatever a rename made during the writes below says. + pushed_name = pl.name + if snippet is not None and (snippet.get("title") or "") != pushed_name: try: - yt.update_playlist_title(pl.yt_playlist_id, pl.name) + yt.update_playlist_title(pl.yt_playlist_id, pushed_name) except YouTubeError as exc: # All the caller passes on is the word "title", which says the rename did not # happen but not that YouTube refused it, or why. @@ -744,7 +778,7 @@ def merge_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: (effective_unavailable | {v for v in mp.add_to_local if v not in present}) & yt_id_set ) - _set_baseline_under_lock(db, pl, final_local, unavailable) + _set_baseline_under_lock(db, pl, final_local, unavailable, name=pushed_name) db.commit() return { "created": created, @@ -772,7 +806,7 @@ def repull_playlist(db: Session, user: User, pl: Playlist) -> dict: raise YouTubeError("Playlist no longer exists on YouTube") ids = list(yt.iter_my_playlist_video_ids(pl.yt_playlist_id)) _ensure_videos(db, yt, ids) - lock_playlists(db, pl.id) + _lock_or_gone(db, pl) pl.name = snippet.get("title") or pl.name ordered = _replace_items_from_youtube(db, pl, ids) db.commit() diff --git a/backend/tests/test_playlist_sync_claim.py b/backend/tests/test_playlist_sync_claim.py index 083e610f..b627cef7 100644 --- a/backend/tests/test_playlist_sync_claim.py +++ b/backend/tests/test_playlist_sync_claim.py @@ -67,7 +67,12 @@ def test_push_and_revert_refuse_while_the_list_is_claimed( r = c.post(f"/api/playlists/{pl.id}/{path}") assert r.status_code == 409 assert r.json()["detail"] == {"code": "playlist", "reason": "sync_busy"} + # a delete too: a push mid-way would plan the deleted list as emptied (E28 round 4) + r = c.delete(f"/api/playlists/{pl.id}") + assert r.status_code == 409 + assert r.json()["detail"] == {"code": "playlist", "reason": "sync_busy"} assert ran == [] + assert db.get(Playlist, pl.id) is not None # Released on the way out: the same list syncs again, and a claim is per list. assert c.post(f"/api/playlists/{pl.id}/push").status_code == 200 diff --git a/backend/tests/test_playlist_sync_lock_order.py b/backend/tests/test_playlist_sync_lock_order.py index c3a141d4..5c631510 100644 --- a/backend/tests/test_playlist_sync_lock_order.py +++ b/backend/tests/test_playlist_sync_lock_order.py @@ -10,11 +10,12 @@ during its YouTube writes. The "other tab" here is a second session that commits talking to YouTube — when the push holds no lock, as a real one does not. """ +import json import re from datetime import datetime, timezone import pytest -from sqlalchemy import event +from sqlalchemy import delete, event, text from app.db import SessionLocal from app.models import Channel, Playlist, PlaylistItem, Video @@ -151,9 +152,16 @@ def _playlist(db, user, local, *, source="local", baseline=None): return pl -def _other_tab(pl_id, *, add=None, remove=None): - """A second session's edit, committed at once — what another tab's request does.""" +def _other_tab(pl_id, *, add=None, remove=None, rename=None, drop=False): + """A second session's edit, committed at once — what another tab's request does. It runs on + the push's own thread: a lock the push wrongly held here would be waited on forever, so the + wait is bounded and a regression fails instead of hanging the gate.""" with SessionLocal() as s: + s.execute(text("SET LOCAL lock_timeout = '2s'")) + if drop: + s.execute(delete(Playlist).where(Playlist.id == pl_id)) + if rename: + s.get(Playlist, pl_id).name = rename if add: pos = len(s.query(PlaylistItem).filter_by(playlist_id=pl_id).all()) s.add(PlaylistItem(playlist_id=pl_id, video_id=add, position=pos)) @@ -227,6 +235,48 @@ def test_an_add_made_while_a_push_reads_youtube_survives_it(db, make_user, fake_ assert "x" in [v for _, v in fake.items] # and it reached YouTube as a local add +@pytest.mark.parametrize("linked", [True, False], ids=["linked", "first-push"]) +def test_a_rename_made_during_a_push_s_youtube_writes_stays_dirty(db, make_user, fake_yt, linked): + """The closing baseline is YouTube's title, not the one the lock re-reads: over the new name it + matched the rename, and the list read synced with YouTube still on the old title.""" + user = make_user() + _seed(db, ["a", "b"]) + pl = _playlist(db, user, ["a", "b"], baseline=["a"] if linked else None) + if not linked: + pl.yt_playlist_id = None + db.commit() + fake = fake_yt( + FakeYT(["a"] if linked else [], on_add=lambda: _other_tab(pl.id, rename="Mix 2")) + ) + pl_mod.merge_playlist(db, user, pl) + db.refresh(pl) + assert (pl.name, fake.title) == ("Mix 2", "Mix") + assert pl.dirty is True # the rename has not reached YouTube; the list must say so + + +def test_a_push_stops_at_a_list_deleted_while_it_read_youtube(db, make_user, fake_yt): + """A delete landing mid-push left a plan over no items: every item "removed here", so the push + emptied the YouTube playlist the delete meant to keep.""" + user = make_user() + _seed(db, ["a", "b"]) + pl = _playlist(db, user, ["a", "b"]) + # a video YouTube would not return last time, retried now — the read the delete lands in + pl.synced_video_ids = json.dumps({"ids": ["a", "b"], "unavailable": ["ghost"]}) + db.commit() + fake = fake_yt( + FakeYT( + ["a", "b", "ghost"], unavailable={"ghost"}, on_get=lambda: _other_tab(pl.id, drop=True) + ) + ) + raised = None + try: + pl_mod.merge_playlist(db, user, pl) + except Exception as exc: # noqa: BLE001 — the regression raises too, AFTER its YouTube writes + raised = exc + assert [v for _, v in fake.items] == ["a", "b", "ghost"] # YouTube untouched + assert isinstance(raised, pl_mod.PlaylistGone) # which the route answers with a 404 + + @pytest.mark.parametrize("linked", [True, False], ids=["linked", "first-push"]) def test_an_edit_made_during_a_push_s_youtube_writes_stays_dirty(db, make_user, fake_yt, linked): user = make_user()