From b456232d0041b1663ca98beacfed7d251dc8be8f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 27 Sep 2026 13:58:23 +0200 Subject: [PATCH] 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 | 14 +- backend/app/routes/playlists.py | 13 +- backend/app/routes/plex/playlists.py | 38 ++++-- backend/tests/test_playlist_position_lock.py | 136 +++++++++++++++++++ 4 files changed, 180 insertions(+), 21 deletions(-) create mode 100644 backend/tests/test_playlist_position_lock.py diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py index 3526248f..f4105db3 100644 --- a/backend/app/routes/_common.py +++ b/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.""" - obj = db.get(model, id_) + idiom repeated across routes. `lock` takes it FOR UPDATE (bypassing the identity map), so two + 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 diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 7ab2f2d9..a4e0e741 100644 --- a/backend/app/routes/playlists.py +++ b/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 diff --git a/backend/app/routes/plex/playlists.py b/backend/app/routes/plex/playlists.py index 687ff2ba..0f618cf9 100644 --- a/backend/app/routes/plex/playlists.py +++ b/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): - iid = id_by_rk.get(rk) - if iid is not None: - db.execute( - update(PlexPlaylistItem) - .where(PlexPlaylistItem.playlist_id == pid, PlexPlaylistItem.item_id == iid) - .values(position=pos) - ) + rows = { + r.item_id: r + for r in db.execute( + select(PlexPlaylistItem).where(PlexPlaylistItem.playlist_id == pid) + ).scalars() + } + 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} diff --git a/backend/tests/test_playlist_position_lock.py b/backend/tests/test_playlist_position_lock.py new file mode 100644 index 00000000..8a2e8007 --- /dev/null +++ b/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 … 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")]