fix(playlists): lock the playlist row before reading and writing item positions

Two adds read the same max(position) and a reorder racing an add collided with
it. Every add/insert/reorder takes the parent row FOR UPDATE; the YouTube list
breaks position ties by id and a Plex reorder renumbers rows it was not sent.

backend/app/routes/_common.py

@@ -28,10 +28,13 @@ def not_found(what: str = "generic") -> HTTPException:
     return HTTPException(status_code=404, detail={"code": "not_found", "reason": what})
 
 
-def get_or_404(db: Session, model: type[M], id_: Any, *, what: str = "generic") -> M:
+def get_or_404(
+    db: Session, model: type[M], id_: Any, *, what: str = "generic", lock: bool = False
+) -> M:
     """Fetch a row by primary key or raise 404. Replaces the `db.get(Model, id); if None: 404`
-    idiom repeated across routes."""
+    idiom repeated across routes. `lock` takes it FOR UPDATE (bypassing the identity map), so two
-    obj = db.get(model, id_)
+    requests that read-then-write its children (a playlist's item positions) run one after the other."""
+    obj = db.get(model, id_, with_for_update=True if lock else None)
     if obj is None:
         raise not_found(what)
     return obj
@@ -45,12 +48,13 @@ def owned_or_404(
     *,
     owner_attr: str = "user_id",
     what: str = "generic",
+    lock: bool = False,
 ) -> M:
-    """Fetch a row by primary key that `user` owns, or raise 404. A row owned by someone
+    """Fetch a row by primary key that `user` owns, or raise 404 (`lock`: see `get_or_404`). A row owned by someone
     else 404s (NOT 403) on purpose — a 403 would confirm the id exists to a stranger.
     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)
+    obj = get_or_404(db, model, id_, what=what, lock=lock)
     if getattr(obj, owner_attr) != user.id:
         raise not_found(what)
     return obj

backend/app/routes/playlists.py

@@ -79,7 +79,10 @@ def _next_playlist_position(db: Session, user_id: int) -> int:
 
 
 def _next_item_position(db: Session, playlist_id: int) -> int:
-    """Append position for a new item at the end of a playlist."""
+    """Append position for a new item at the end of a playlist. Locks the playlist row first: two
+    adds (two tabs, the rail and the page) otherwise both read the same max and land on one
+    position, and the list's order between them is then arbitrary."""
+    db.execute(select(Playlist.id).where(Playlist.id == playlist_id).with_for_update())
     return (
         db.scalar(
             select(func.coalesce(func.max(PlaylistItem.position), -1)).where(
@@ -427,7 +430,8 @@ def get_playlist(
         .join(Channel, Channel.id == Video.channel_id)
         .outerjoin(state, and_(state.video_id == Video.id, state.user_id == user.id))
         .where(PlaylistItem.playlist_id == pl.id)
-        .order_by(PlaylistItem.position)
+        # `id` breaks a position tie the same way on every read (the renumbering add does too).
+        .order_by(PlaylistItem.position, PlaylistItem.id)
     ).all()
     return {
         "id": pl.id,
@@ -532,7 +536,7 @@ def add_item(
             db.execute(
                 select(PlaylistItem)
                 .where(PlaylistItem.playlist_id == pl.id)
-                .order_by(PlaylistItem.position)
+                .order_by(PlaylistItem.position, PlaylistItem.id)
             ).scalars()
         )
         new = next(r for r in rows if r.video_id == video_id)
@@ -642,7 +646,8 @@ def reorder_items(
     user: User = Depends(current_user),
     db: Session = Depends(get_db),
 ) -> dict:
-    pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist")
+    # Locked: a concurrent add between reading the items and writing the order would collide.
+    pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist", lock=True)
     order = body.video_ids
     items = {
         it.video_id: it

backend/app/routes/plex/playlists.py

@@ -6,7 +6,7 @@ from fastapi import APIRouter, Depends
 from typing import Annotated
 
 from pydantic import BaseModel, Field, model_validator
-from sqlalchemy import delete, func, select, update
+from sqlalchemy import delete, func, select
 from sqlalchemy.orm import Session
 
 from app.auth import current_user
@@ -219,7 +219,8 @@ def playlist_add_item(
     user: User = Depends(current_user),
     db: Session = Depends(get_db),
 ) -> dict:
-    pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
+    # Locked (like every add / reorder here): two adds otherwise read the same max position.
+    pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist", lock=True)
     it = by_rating_key(db, PlexItem, body.item_rating_key)
     if it is None:
         raise not_found("item")
@@ -337,7 +338,7 @@ def playlist_add_bulk(
     """Append many items at once (a whole season/show). Already-present items are skipped; the input
     order is preserved for the newly-added ones. With `positions`, each key is put back at its index
     instead of appended. Returns how many were added + the new total."""
-    pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
+    pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist", lock=True)
     if body.positions is not None:
         by_rk = {it.rating_key: it for it in _items_by_rks(db, body.item_rating_keys)}
         placed = [
@@ -428,7 +429,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 = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
+    pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist", lock=True)
     order = body.item_rating_keys
     id_by_rk = {
         it.rating_key: it.id
@@ -436,14 +437,27 @@ def reorder_playlist(
             select(PlexItem).where(PlexItem.rating_key.in_(order or [""]))
         ).scalars()
     }
-    for pos, rk in enumerate(order):
+    rows = {
-        iid = id_by_rk.get(rk)
+        r.item_id: r
-        if iid is not None:
+        for r in db.execute(
-            db.execute(
+            select(PlexPlaylistItem).where(PlexPlaylistItem.playlist_id == pid)
-                update(PlexPlaylistItem)
+        ).scalars()
-                .where(PlexPlaylistItem.playlist_id == pid, PlexPlaylistItem.item_id == iid)
+    }
-                .values(position=pos)
+    pos = 0
-            )
+    seen: set[int] = set()
+    for rk in order:
+        row = rows.get(id_by_rk.get(rk, -1))
+        if row is not None and row.item_id not in seen:
+            row.position = pos
+            pos += 1
+            seen.add(row.item_id)
+    # Rows the payload omitted (added in another tab meanwhile) keep their relative order after
+    # the given ones, so no two rows share a position (the YouTube reorder does the same).
+    for row in sorted(
+        (r for iid, r in rows.items() if iid not in seen), key=lambda r: (r.position, r.id)
+    ):
+        row.position = pos
+        pos += 1
     pl.updated_at = datetime.now(timezone.utc)
     db.commit()
     return {"ok": True}

backend/tests/test_playlist_position_lock.py

@@ -0,0 +1,136 @@
+"""E28 review — every request that reads a playlist's item positions and writes new ones locks the
+playlist row first.
+
+Without it two adds (two tabs; the rail and the page) both read the same `max(position)` and land
+on one position, and a reorder racing an add collides with it — the list's order between the two
+rows is then whatever the planner returns. A true two-connection race is not reproducible on
+demand, so the test holds the mechanism: the parent row is taken `FOR UPDATE` by each such route.
+A Plex reorder that omits a row (added in another tab meanwhile) must not collide with it either.
+"""
+
+import re
+from datetime import datetime, timezone
+
+import pytest
+from sqlalchemy import event
+
+from app.models import (
+    Channel,
+    Playlist,
+    PlaylistItem,
+    PlexItem,
+    PlexLibrary,
+    PlexPlaylist,
+    PlexPlaylistItem,
+    Video,
+)
+
+
+@pytest.fixture
+def locks(db):
+    """The tables each request took a row lock on (`SELECT … FROM <t> … FOR UPDATE`)."""
+    seen: list[str] = []
+
+    def _record(conn, cursor, statement, *_):
+        if "FOR UPDATE" in statement:
+            seen.extend(re.findall(r"FROM\s+(\w+)", statement)[:1])
+
+    engine = db.get_bind()
+    event.listen(engine, "before_cursor_execute", _record)
+    yield seen
+    event.remove(engine, "before_cursor_execute", _record)
+
+
+def _yt(db, user):
+    db.add(Channel(id="UCtest", title="Test"))
+    now = datetime.now(timezone.utc)
+    for v in ("v1", "v2", "v3"):
+        db.add(Video(id=v, channel_id="UCtest", published_at=now))
+    pl = Playlist(user_id=user.id, name="L")
+    other = Playlist(user_id=user.id, name="M")
+    db.add_all([pl, other])
+    db.flush()
+    db.add(PlaylistItem(playlist_id=pl.id, video_id="v1", position=0))
+    db.commit()
+    return pl.id, other.id
+
+
+def _plex(db, user):
+    lib = PlexLibrary(plex_key="p1", title="Movies", kind="movie", enabled=True)
+    db.add(lib)
+    db.flush()
+    items = [PlexItem(rating_key=rk, library_id=lib.id, kind="movie", title=rk) for rk in "abcd"]
+    db.add_all(items)
+    pl = PlexPlaylist(user_id=user.id, title="L")
+    db.add(pl)
+    db.flush()
+    for i, it in enumerate(items[:3]):
+        db.add(PlexPlaylistItem(playlist_id=pl.id, item_id=it.id, position=i))
+    db.commit()
+    return pl.id
+
+
+YT = Playlist.__table__.name
+PLEX = PlexPlaylist.__table__.name
+
+
+@pytest.mark.parametrize(
+    "call",
+    [
+        lambda c, pid, other: c.post(f"/api/playlists/{pid}/items", json={"video_id": "v2"}),
+        lambda c, pid, other: c.post(
+            f"/api/playlists/{pid}/items", json={"video_id": "v2", "position": 0}
+        ),
+        lambda c, pid, other: c.post("/api/playlists/watch-later", json={"video_id": "v2"}),
+        lambda c, pid, other: c.post(
+            f"/api/playlists/{pid}/items/v1/move", json={"target_playlist_id": other}
+        ),
+        lambda c, pid, other: c.put(f"/api/playlists/{pid}/order", json={"video_ids": ["v1"]}),
+    ],
+    ids=["add", "add-at", "watch-later", "move", "reorder"],
+)
+def test_a_youtube_playlist_write_locks_the_playlist(db, make_user, as_user, locks, call):
+    user = make_user()
+    pid, other = _yt(db, user)
+    r = call(as_user(user), pid, other)
+    assert r.status_code == 200, r.text
+    assert YT in locks
+
+
+@pytest.mark.parametrize(
+    "call",
+    [
+        lambda c, pid: c.post(f"/api/plex/playlists/{pid}/items", json={"item_rating_key": "d"}),
+        lambda c, pid: c.post(
+            f"/api/plex/playlists/{pid}/items/bulk", json={"item_rating_keys": ["d"]}
+        ),
+        lambda c, pid: c.post(
+            f"/api/plex/playlists/{pid}/items/bulk",
+            json={"item_rating_keys": ["d"], "positions": [0]},
+        ),
+        lambda c, pid: c.put(
+            f"/api/plex/playlists/{pid}/order", json={"item_rating_keys": ["c", "b", "a"]}
+        ),
+    ],
+    ids=["add", "bulk", "bulk-at", "reorder"],
+)
+def test_a_plex_playlist_write_locks_the_playlist(db, make_user, as_user, locks, call):
+    user = make_user()
+    pid = _plex(db, user)
+    r = call(as_user(user), pid)
+    assert r.status_code == 200, r.text
+    assert PLEX in locks
+
+
+def test_a_plex_reorder_that_omits_a_row_does_not_collide_with_it(db, make_user, as_user):
+    """The row "a" was added in another tab after this page loaded its order ["c", "b"]: it keeps
+    its place after the given rows instead of sharing position 0 with "c"."""
+    user = make_user()
+    pid = _plex(db, user)
+    r = as_user(user).put(f"/api/plex/playlists/{pid}/order", json={"item_rating_keys": ["c", "b"]})
+    assert r.status_code == 200
+    db.expire_all()
+    rows = db.query(PlexPlaylistItem).filter_by(playlist_id=pid).all()
+    by_id = {it.id: it.rating_key for it in db.query(PlexItem).all()}
+    got = sorted((r.position, by_id[r.item_id]) for r in rows)
+    assert got == [(0, "c"), (1, "b"), (2, "a")]