Merge bug/watched-state-500-race: fix watched not persisting at end-of-playback

backend/app/routes/feed.py

@@ -2,7 +2,9 @@ from datetime import date, datetime, timedelta, timezone
 
 from fastapi import APIRouter, Depends, HTTPException, Query
 from sqlalchemy import Select, and_, false, func, or_, select
+from sqlalchemy.dialects.postgresql import insert as pg_insert
 from sqlalchemy.orm import Session, aliased
+from sqlalchemy.orm.exc import StaleDataError
 
 from app import quota
 from app.auth import current_user
@@ -387,29 +389,48 @@ def set_video_state(
     if db.get(Video, video_id) is None:
         raise HTTPException(status_code=404, detail="Unknown video")
 
-    row = db.execute(
+    now = datetime.now(timezone.utc)
-        select(VideoState).where(
-            VideoState.user_id == user.id, VideoState.video_id == video_id
-        )
-    ).scalar_one_or_none()
 
     if status == "new":
+        # Un-marking: keep the row if it still holds a resume position (un-marking
+        # "watched" should restore the in-progress state, not wipe where the user left
+        # off); otherwise drop it. The in-app player fires this alongside a progress
+        # checkpoint that may DELETE the same row, so tolerate it vanishing under us.
+        row = db.execute(
+            select(VideoState).where(
+                VideoState.user_id == user.id, VideoState.video_id == video_id
+            )
+        ).scalar_one_or_none()
         if row is not None:
-            # Keep the row if it still holds a resume position (un-marking "watched"
-            # should restore the in-progress state, not wipe where the user left off).
             if row.position_seconds:
                 row.status = "new"
                 row.watched_at = None
             else:
                 db.delete(row)
-            db.commit()
+            try:
+                db.commit()
+            except StaleDataError:
+                db.rollback()
         return {"video_id": video_id, "status": "new"}
 
-    if row is None:
+    # watched / hidden: atomic upsert. The player marks "watched" at end-of-playback at
-        row = VideoState(user_id=user.id, video_id=video_id)
+    # the same instant a progress checkpoint may DELETE the in-progress row; a plain
-        db.add(row)
+    # SELECT-then-UPDATE then raises StaleDataError ("0 rows matched") on that race. An
-    row.status = status
+    # INSERT … ON CONFLICT DO UPDATE keyed on uq_user_video is immune to it.
-    row.watched_at = datetime.now(timezone.utc) if status == "watched" else row.watched_at
+    set_: dict = {"status": status, "updated_at": now}
+    if status == "watched":
+        set_["watched_at"] = now  # hidden keeps any existing watched_at
+    stmt = (
+        pg_insert(VideoState)
+        .values(
+            user_id=user.id,
+            video_id=video_id,
+            status=status,
+            watched_at=now if status == "watched" else None,
+        )
+        .on_conflict_do_update(constraint="uq_user_video", set_=set_)
+    )
+    db.execute(stmt)
     db.commit()
     return {"video_id": video_id, "status": status}
 
@@ -455,7 +476,12 @@ def set_video_progress(
             else:
                 row.position_seconds = 0
                 row.progress_updated_at = datetime.now(timezone.utc)
-            db.commit()
+            # A concurrent state change (e.g. the end-of-playback "watched" mark) may
+            # have removed/updated this row already — tolerate the lost race.
+            try:
+                db.commit()
+            except StaleDataError:
+                db.rollback()
         return {"video_id": video_id, "position_seconds": 0}
 
     if row is None:

frontend/src/components/Feed.tsx

@@ -147,7 +147,15 @@ export default function Feed({
       api
         .setState(id, status)
         .then(() => qc.invalidateQueries({ queryKey: ["feed"] }))
-        .catch(() => {});
+        .catch(() =>
+          // Server rejected the change — drop the optimistic override so the card
+          // reverts to the real (unchanged) state instead of staying phantom-hidden.
+          setOverrides((o) => {
+            const next = { ...o };
+            delete next[id];
+            return next;
+          })
+        );
       if (status === "hidden") {
         const v = loadedRef.current.find((x) => x.id === id);
         notify({

frontend/src/components/PlayerModal.tsx

@@ -339,12 +339,14 @@ export default function PlayerModal({
 
     // Auto-watch only applies to the active item — not to other videos the player
     // navigated to via description links.
-    const maybeAutoWatch = (current: number, duration: number) => {
+    const maybeAutoWatch = (current: number, duration: number): boolean => {
-      if (autoMarkedRef.current || duration <= 0 || currentIdRef.current !== id) return;
+      if (autoMarkedRef.current || duration <= 0 || currentIdRef.current !== id) return false;
       if (current > duration - FINISH_MARGIN) {
         autoMarkedRef.current = true;
         setWatched(true);
+        return true;
       }
+      return false;
     };
     const persist = (): Promise<unknown> | void => {
       const p = playerRef.current;
@@ -352,7 +354,10 @@ export default function PlayerModal({
       try {
         const cur = p.getCurrentTime();
         const dur = typeof p.getDuration === "function" ? p.getDuration() : 0;
-        maybeAutoWatch(cur, dur);
+        // If we just auto-marked watched, skip the near-end progress checkpoint: it would
+        // only clear the resume position, and firing both at once needlessly races the
+        // watched write against the progress row it deletes.
+        if (maybeAutoWatch(cur, dur)) return;
         // Only checkpoint the feed video we opened with — a navigated-to video may not
         // be in this user's library, so the server would 404 on it.
         if (currentIdRef.current === id) {