fix(playlists): a racing move is not a 500; ownership through owned_or_404 everywhere

move_item checked "already in the target?" and then re-parented without a
lock, so a second tab's add or the scheduler's YouTube merge committing the
same video in between hit uq_playlist_video and answered a raw 500. The
re-parent now runs in a savepoint and a conflict falls back to the
already-present outcome. The target is touched only when the row landed in
it (an untouched list was promoted in the rail's "recently changed" sort),
and _touch stamps the database clock like the mixin's own onupdate.

The canonical-patterns table names owned_or_404 against ad-hoc ownership
helpers; six were left (_own_playlist, _own_playlist_or_404, _own_view,
_own_tag, notifications._owned, downloads._own_job), two of them
hand-rolled with a bare English 404. All call sites now use owned_or_404,
and the download/link/notification 404s are structured like the rest. The
contract test reads every "what is gone" the routes name, and the YouTube
refusal reasons, and requires both languages to name each.
This commit is contained in:
2026-09-19 07:01:18 +02:00
parent 2df45b27d3
commit e74986cc9a
10 changed files with 166 additions and 85 deletions
+15 -20
View File
@@ -38,7 +38,7 @@ from app.models import (
MediaAsset,
User,
)
from app.routes._common import get_or_404, not_found
from app.routes._common import get_or_404, not_found, owned_or_404
from app.security import hash_password
from app.userscope import is_messageable_user, messageable_clauses
from app.utils import iso
@@ -166,15 +166,8 @@ def _serialize_job(db: Session, job: DownloadJob) -> dict:
return _serialize(job, asset)
def _own_job(db: Session, user: User, job_id: int) -> DownloadJob:
job = db.get(DownloadJob, job_id)
if job is None or job.user_id != user.id:
raise HTTPException(status_code=404, detail="Unknown download")
return job
def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob:
"""`_own_job` for a transition that moves its asset's ref count (resume, cancel, delete, redownload)
"""The ownership check (`owned_or_404`) for a transition that moves its asset's ref count (resume, cancel, delete, redownload)
or writes the status those decide on (pause). The job is re-read under a row lock, so two
overlapping requests for one job run one after the other and the second decides on the first's
committed result — never both releasing the same hold, and never a pause turning a job a cancel
@@ -183,7 +176,7 @@ def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob:
The asset is locked BEFORE the job: the order a finishing worker child takes them in
(`service.finalize_if_running`), so the two can never deadlock."""
while True:
asset_id = _own_job(db, user, job_id).asset_id
asset_id = owned_or_404(db, DownloadJob, job_id, user, what="download").asset_id
if asset_id is not None:
db.execute(select(MediaAsset.id).where(MediaAsset.id == asset_id).with_for_update())
job = db.execute(
@@ -193,7 +186,7 @@ def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob:
.execution_options(populate_existing=True)
).scalar_one_or_none()
if job is None:
raise HTTPException(status_code=404, detail="Unknown download")
raise not_found("download")
if job.asset_id == asset_id:
return job
db.rollback() # another request moved the job to another asset meanwhile: lock that one
@@ -614,7 +607,7 @@ def update_download_meta(
) -> dict:
"""Update a download's display metadata: title, channel name + link, and extra reference URLs.
All are display-only overrides — the on-disk file keeps its system-decided, id-bound name."""
job = _own_job(db, user, job_id)
job = owned_or_404(db, DownloadJob, job_id, user, what="download")
sent = body.model_fields_set
if "display_name" in sent:
job.display_name = ((body.display_name or "").strip())[:255] or None
@@ -699,7 +692,7 @@ def delete_download(
def _accessible_job(db: Session, user: User, job_id: int) -> DownloadJob:
job = db.get(DownloadJob, job_id)
if job is None:
raise HTTPException(status_code=404, detail="Unknown download")
raise not_found("download")
if job.user_id == user.id:
return job
shared = db.execute(
@@ -708,7 +701,7 @@ def _accessible_job(db: Session, user: User, job_id: int) -> DownloadJob:
)
).first()
if shared is None:
raise HTTPException(status_code=404, detail="Unknown download")
raise not_found("download")
return job
@@ -820,7 +813,7 @@ def share_download(
user: User = Depends(require_human),
db: Session = Depends(get_db),
) -> dict:
job = _own_job(db, user, job_id)
job = owned_or_404(db, DownloadJob, job_id, user, what="download")
if job.status != "done" or job.asset_id is None:
raise HTTPException(status_code=409, detail="Only a finished download can be shared.")
target = body.email.strip().lower()
@@ -863,7 +856,9 @@ def unshare_download(
user: User = Depends(require_human),
db: Session = Depends(get_db),
) -> dict:
_own_job(db, user, job_id) # ownership guard (404s if not the caller's job)
owned_or_404(
db, DownloadJob, job_id, user, what="download"
) # ownership guard (404s if not the caller's job)
recipient = db.execute(
select(User).where(func.lower(User.email) == email.strip().lower())
).scalar_one_or_none()
@@ -925,10 +920,10 @@ def share_recipients(
def _own_link(db: Session, user: User, link_id: int) -> DownloadLink:
link = db.get(DownloadLink, link_id)
if link is None:
raise HTTPException(status_code=404, detail="Unknown link")
raise not_found("link")
job = db.get(DownloadJob, link.job_id)
if job is None or job.user_id != user.id:
raise HTTPException(status_code=404, detail="Unknown link")
raise not_found("link")
return link
@@ -944,7 +939,7 @@ def _link_expiry(days: int | None) -> datetime | None:
def list_links(
job_id: int, user: User = Depends(require_human), db: Session = Depends(get_db)
) -> list[dict]:
_own_job(db, user, job_id)
owned_or_404(db, DownloadJob, job_id, user, what="download")
rows = (
db.execute(
select(DownloadLink)
@@ -975,7 +970,7 @@ def create_link(
user: User = Depends(require_human),
db: Session = Depends(get_db),
) -> dict:
job = _own_job(db, user, job_id)
job = owned_or_404(db, DownloadJob, job_id, user, what="download")
if job.status != "done" or job.asset_id is None:
raise HTTPException(status_code=409, detail="Only a finished download can be shared.")
pw = body.password.strip()
+4 -10
View File
@@ -5,7 +5,7 @@ All endpoints use `current_user` (not `require_human`): notifications and their
state are personal UI state with no quota or YouTube involvement, so the shared demo account
manages its own inbox normally."""
from fastapi import APIRouter, Depends, HTTPException, Query
from fastapi import APIRouter, Depends, Query
from sqlalchemy import func, select, update
from sqlalchemy.orm import Session
@@ -14,6 +14,7 @@ from app.db import get_db
from app.models import Notification, User
from app.notifications import trim_read
from app.utils import iso
from app.routes._common import owned_or_404
router = APIRouter(prefix="/api/me/notifications", tags=["notifications"])
@@ -72,20 +73,13 @@ def unread_count(user: User = Depends(current_user), db: Session = Depends(get_d
return {"count": count or 0}
def _owned(db: Session, notification_id: int, user: User) -> Notification:
n = db.get(Notification, notification_id)
if n is None or n.user_id != user.id:
raise HTTPException(status_code=404, detail="Notification not found")
return n
@router.post("/{notification_id}/read")
def mark_read(
notification_id: int,
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
n = _owned(db, notification_id, user)
n = owned_or_404(db, Notification, notification_id, user, what="notification")
n.read = True
db.commit()
trim_read(db, user.id)
@@ -110,7 +104,7 @@ def dismiss(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
n = _owned(db, notification_id, user)
n = owned_or_404(db, Notification, notification_id, user, what="notification")
# Dismiss implies read (it no longer counts toward the unread badge).
n.dismissed = True
n.read = True
+37 -27
View File
@@ -3,11 +3,10 @@ CRUD (create / rename / delete, add / remove / reorder items) plus YouTube two-w
mirror the user's own YouTube playlists in (`/sync-youtube`) and push/revert local edits back
out (`/{id}/push-plan`, `/{id}/revert-youtube`), behind the optional write scope."""
from datetime import datetime, timezone
from fastapi import APIRouter, Depends, HTTPException
from pydantic import BaseModel
from sqlalchemy import func, select, and_
from sqlalchemy.exc import IntegrityError
from sqlalchemy.orm import Session, aliased
from app import quota
@@ -48,10 +47,6 @@ def _check_pull_cooldown(user: User) -> None:
raise HTTPException(status_code=429, detail={"code": "sync_cooldown"})
def _own_playlist(db: Session, user: User, playlist_id: int) -> Playlist:
return owned_or_404(db, Playlist, playlist_id, user, what="playlist")
def _refused(status: int, reason: str) -> HTTPException:
"""A playlist action the server refuses, as a reason the UI translates
(`playlists.errors.<reason>`) — a bare English detail reached the Hungarian UI verbatim."""
@@ -64,8 +59,11 @@ def _touch(pl: Playlist) -> None:
`UpdatedAtMixin`'s `onupdate` fires on an UPDATE of the PLAYLIST row, and add / remove /
reorder / move write the item rows instead — so without this the rail's "recently changed"
sort would only ever see renames. The assignment is what makes the column mean "last time
this list changed", which is what the rail promises the reader."""
pl.updated_at = datetime.now(timezone.utc)
this list changed", which is what the rail promises the reader.
The DATABASE's clock, like the mixin's own `onupdate`: one sort key written from two clocks
(this process's and Postgres's) could order a rename and an item edit the wrong way round."""
pl.updated_at = func.now()
def _next_playlist_position(db: Session, user_id: int) -> int:
@@ -339,7 +337,7 @@ def revert_youtube(
"""Reset one YouTube-linked playlist to its current YouTube state, discarding local
edits (and clearing the dirty flag). The user-facing 'undo all my changes' for a list
once the in-session undo history is gone."""
pl = _own_playlist(db, user, playlist_id)
pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist")
if not pl.yt_playlist_id:
raise _refused(400, "not_on_youtube")
if not has_read_scope(user):
@@ -356,7 +354,7 @@ def revert_youtube(
def _pushable(db: Session, user: User, playlist_id: int) -> Playlist:
"""A local playlist the user may push to YouTube (create or update). The built-in
Watch later and read-only YouTube mirrors are never pushable."""
pl = _own_playlist(db, user, playlist_id)
pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist")
if pl.kind == "watch_later":
raise _refused(400, "watch_later_not_pushable")
return pl
@@ -419,7 +417,7 @@ def get_playlist(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
pl = _own_playlist(db, user, playlist_id)
pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist")
state = aliased(VideoState)
rows = db.execute(
select(*feed_columns(user, state))
@@ -451,7 +449,7 @@ def rename_playlist(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
pl = _own_playlist(db, user, playlist_id)
pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist")
if "name" in body.model_fields_set:
name = (body.name or "").strip()
if not name:
@@ -488,7 +486,7 @@ def delete_playlist(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
pl = _own_playlist(db, user, playlist_id)
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:
@@ -514,7 +512,7 @@ def add_item(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
pl = _own_playlist(db, user, playlist_id)
pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist")
video_id = body.video_id
get_or_404(db, Video, video_id, what="video")
_add_item(db, pl, video_id, mark_dirty=True)
@@ -528,7 +526,7 @@ def remove_item(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
pl = _own_playlist(db, user, playlist_id)
pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist")
_remove_item(db, pl, video_id, mark_dirty=True)
return {"playlist_id": pl.id, "video_id": video_id}
@@ -552,7 +550,7 @@ def move_item(
exactly the kind of half-applied state this codebase treats as a bug rather than an edge
case. Idempotent: if the video is already in the target, the source row is simply dropped
(moving into a list that already has it is a removal, not a duplicate)."""
src = _own_playlist(db, user, playlist_id)
src = owned_or_404(db, Playlist, playlist_id, user, what="playlist")
# Its own reason: "this playlist doesn't exist any more" about a TARGET that is gone was read as
# the list on screen — which is still there.
dst = owned_or_404(db, Playlist, body.target_playlist_id, user, what="move_target")
@@ -575,19 +573,31 @@ def move_item(
PlaylistItem.playlist_id == dst.id, PlaylistItem.video_id == video_id
)
)
if already is not None:
db.delete(item) # the uq_playlist_video constraint would reject a second row anyway
else:
dst_changed = False
if already is None:
# Re-parent the existing row rather than delete+insert: `added_at` ("when I saved this")
# survives the move, and there is no window where the video belongs to neither list.
item.playlist_id = dst.id
item.position = _next_item_position(db, dst.id)
# Both sides changed, so both get the dirty recompute and the timestamp — the asymmetric
# version of this (touching only the source) is the recurring smell CLAUDE.md warns about.
# survives the move, and there is no window where the video belongs to neither list. The
# check above is not a lock: a second tab's add or the scheduler's YouTube merge can insert
# the same (playlist, video) in between, and uq_playlist_video then refuses the re-parent.
# That is the already-present outcome arriving late, not a failure — answer it the same way
# instead of a raw 500.
try:
with db.begin_nested():
item.playlist_id = dst.id
item.position = _next_item_position(db, dst.id)
db.flush()
dst_changed = True
except IntegrityError:
pass # the savepoint put the row back in the source list; drop it below
if not dst_changed:
db.delete(item)
# The source always changed; the target only when the row landed in it. `_touch` on a target
# that already had the video promoted an untouched list in the rail's "recently changed" sort.
recompute_dirty(db, src)
recompute_dirty(db, dst)
_touch(src)
_touch(dst)
if dst_changed:
recompute_dirty(db, dst)
_touch(dst)
db.commit()
return {"ok": True, "from_playlist_id": src.id, "to_playlist_id": dst.id, "video_id": video_id}
@@ -603,7 +613,7 @@ def reorder_items(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
pl = _own_playlist(db, user, playlist_id)
pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist")
order = body.video_ids
items = {
it.video_id: it
+8 -12
View File
@@ -24,10 +24,6 @@ from app.routes._common import owned_or_404
router = APIRouter(prefix="/api/plex", tags=["plex"])
def _own_playlist_or_404(db: Session, pid: int, user: User) -> PlexPlaylist:
return owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
def _playlist_card(pl: PlexPlaylist, count: int, thumb_rk: str | None) -> dict:
return {
"id": pl.id,
@@ -140,7 +136,7 @@ def create_playlist(
def playlist_detail(
pid: int, user: User = Depends(current_user), db: Session = Depends(get_db)
) -> dict:
pl = _own_playlist_or_404(db, pid, user)
pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
rows = db.execute(_playlist_item_query(pid)).all()
items = [it for (_pi, it) in rows]
states = {
@@ -183,7 +179,7 @@ def rename_playlist(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
pl = _own_playlist_or_404(db, pid, user)
pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
title = body.title.strip()
if not title:
raise HTTPException(status_code=400, detail="title is required")
@@ -203,7 +199,7 @@ def rename_playlist(
def delete_playlist(
pid: int, user: User = Depends(current_user), db: Session = Depends(get_db)
) -> dict:
pl = _own_playlist_or_404(db, pid, user)
pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
db.delete(pl)
db.commit()
return {"deleted": pid}
@@ -220,7 +216,7 @@ def playlist_add_item(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
pl = _own_playlist_or_404(db, pid, user)
pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
it = by_rating_key(db, PlexItem, body.item_rating_key)
if it is None:
raise HTTPException(status_code=404, detail="Unknown item")
@@ -252,7 +248,7 @@ def playlist_remove_item(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
pl = _own_playlist_or_404(db, pid, user)
pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
it = by_rating_key(db, PlexItem, item_rating_key)
if it is not None:
db.execute(
@@ -297,7 +293,7 @@ def playlist_add_bulk(
) -> dict:
"""Append many items at once (a whole season/show). Already-present items are skipped; the input
order is preserved for the newly-added ones. Returns how many were added + the new total."""
pl = _own_playlist_or_404(db, pid, user)
pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
items = _items_by_rks(db, body.item_rating_keys)
existing = set(
db.execute(
@@ -340,7 +336,7 @@ def playlist_remove_bulk(
) -> dict:
"""Remove many items at once (a whole season/show, or a multi-select). Returns how many were
removed + the new total."""
pl = _own_playlist_or_404(db, pid, user)
pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
ids = [it.id for it in _items_by_rks(db, body.item_rating_keys)]
removed = 0
if ids:
@@ -370,7 +366,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 = _own_playlist_or_404(db, pid, user)
pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
order = body.item_rating_keys
id_by_rk = {
it.rating_key: it.id
+2 -6
View File
@@ -90,10 +90,6 @@ def create_view(
return _serialize(view)
def _own_view(db: Session, user: User, view_id: int) -> SavedView:
return owned_or_404(db, SavedView, view_id, user, what="view")
class UpdateViewIn(BaseModel):
# PATCH: presence decides what's touched (read via model_fields_set). filters, when sent,
# must be a real object — an explicit null is rejected (a view can't have null filters).
@@ -109,7 +105,7 @@ def update_view(
user: User = Depends(require_human),
db: Session = Depends(get_db),
) -> dict:
view = _own_view(db, user, view_id)
view = owned_or_404(db, SavedView, view_id, user, what="view")
sent = body.model_fields_set
if "name" in sent:
name = (body.name or "").strip()
@@ -148,7 +144,7 @@ def delete_view(
user: User = Depends(require_human),
db: Session = Depends(get_db),
) -> dict:
view = _own_view(db, user, view_id)
view = owned_or_404(db, SavedView, view_id, user, what="view")
db.delete(view)
db.commit()
return {"deleted": view_id}
+4 -7
View File
@@ -78,11 +78,6 @@ def create_tag(
return {"id": tag.id, "name": tag.name, "color": tag.color, "category": tag.category}
def _own_tag(db: Session, user: User, tag_id: int) -> Tag:
# System tags (user_id NULL) are not user-editable → owned_or_404 404s them too.
return owned_or_404(db, Tag, tag_id, user, what="tag")
class TagUpdateIn(BaseModel):
# PATCH: presence matters, and `color: null` explicitly CLEARS it — so the endpoint reads
# model_fields_set rather than testing for None (which couldn't tell absent from cleared).
@@ -97,7 +92,8 @@ def update_tag(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
tag = _own_tag(db, user, tag_id)
# System tags (user_id NULL) are not user-editable: owned_or_404 404s them too.
tag = owned_or_404(db, Tag, tag_id, user, what="tag")
sent = body.model_fields_set
if "name" in sent:
name = (body.name or "").strip()
@@ -120,7 +116,8 @@ def delete_tag(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
tag = _own_tag(db, user, tag_id)
# System tags (user_id NULL) are not user-editable: owned_or_404 404s them too.
tag = owned_or_404(db, Tag, tag_id, user, what="tag")
db.delete(tag) # ChannelTag rows cascade on tag delete.
db.commit()
return {"deleted": tag_id}
+46
View File
@@ -242,3 +242,49 @@ def test_the_list_endpoint_publishes_updated_at(db, make_user, as_user):
rows = as_user(user).get("/api/playlists").json()
assert len(rows) == 1
assert rows[0]["updated_at"] # the rail cannot sort by a field the API never sends
def test_a_target_that_already_had_it_is_not_recently_changed(db, make_user, as_user):
"""Moving into a list that already holds the video only removes it from the source: the
target did not change, and bumping it promoted an untouched list in the rail's sort."""
user = make_user()
_seed_videos(db, ["v1"])
src = _playlist(db, user, "Source", ["v1"])
dst = _playlist(db, user, "Target", ["v1"])
old = datetime.now(timezone.utc) - timedelta(days=3)
src.updated_at = old
dst.updated_at = old
db.commit()
assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200
db.expire_all()
assert db.get(Playlist, src.id).updated_at > old
assert db.get(Playlist, dst.id).updated_at == old
def test_a_concurrent_add_to_the_target_lands_as_the_already_present_move(
db, make_user, as_user, monkeypatch
):
"""The already-in-target check is a read, not a lock: another tab's add (or the scheduler's
YouTube merge) can commit the same video to the target in between, and the re-parent then
hits uq_playlist_video. That is the idempotent outcome arriving late — not a raw 500."""
from app.db import SessionLocal
from app.routes import playlists as plroutes
user = make_user()
_seed_videos(db, ["v1"])
src = _playlist(db, user, "Source", ["v1"])
dst = _playlist(db, user, "Target", [])
real = plroutes._next_item_position
def add_first(session, playlist_id):
with SessionLocal() as other: # the other tab, committing between check and re-parent
other.add(PlaylistItem(playlist_id=dst.id, video_id="v1", position=0))
other.commit()
return real(session, playlist_id)
monkeypatch.setattr(plroutes, "_next_item_position", add_first)
assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200
db.expire_all()
assert _ids(db, src.id) == []
assert _ids(db, dst.id) == ["v1"]