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

@@ -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}
 
 

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()

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

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()