From fda725241d12d6077f366935e5b1999c17fe540c Mon Sep 17 00:00:00 2001
From: Daniel Han <danielhanchen@gmail.com>
Date: Tue, 7 Jul 2026 11:57:37 +0000
Subject: [PATCH] Delete gallery videos MP4-first so a locked MP4 can't orphan
 the record

list_videos globs *.mp4 but requires a readable sidecar, so a video whose sidecar
is gone is skipped. delete()/clear() dropped the sidecar first, so if the mp4
unlink then failed (a Windows lock from a concurrent stream/transcode), the
still-present mp4 vanished from the gallery with no way to retry the delete. Unlink
the mp4 first and only then best-effort the sidecar: the worst case is now an
orphaned sidecar, which list_videos already ignores.
---
 .../backend/core/inference/video_gallery.py   | 35 +++++++++++--------
 studio/backend/tests/test_video_gallery.py    | 29 +++++++++++++++
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/studio/backend/core/inference/video_gallery.py b/studio/backend/core/inference/video_gallery.py
index 6a207f9780..b031bcd4bd 100644
--- a/studio/backend/core/inference/video_gallery.py
+++ b/studio/backend/core/inference/video_gallery.py
@@ -221,18 +221,24 @@ def delete(video_id: str) -> bool:
     path = video_path(video_id)
     if path is None:
         return False
-    # Best-effort on the sidecar: a leftover json without its mp4 is skipped by
-    # list_videos anyway, so a failed sidecar unlink must not fail the delete.
+    # Delete the MP4 payload FIRST. list_videos globs *.mp4 but requires a readable sidecar, so
+    # a video whose sidecar is gone is skipped as an orphan. If the sidecar were dropped first and
+    # the mp4 unlink then failed (a Windows lock from a concurrent stream/transcode, or a
+    # permission change), the still-present mp4 would vanish from the gallery with no way to retry
+    # the delete. Ordering mp4-first means the worst case is an orphaned sidecar, which
+    # list_videos ignores.
+    try:
+        path.unlink()
+    except OSError as exc:
+        logger.warning("video_gallery.delete_failed: %s", exc)
+        return False
+    # Best-effort on the sidecar: a leftover json without its mp4 is skipped by list_videos
+    # anyway, so a failed sidecar unlink must not fail the delete.
     try:
         _sidecar_path(video_id).unlink()
     except OSError:
         pass
-    try:
-        path.unlink()
-        return True
-    except OSError as exc:
-        logger.warning("video_gallery.delete_failed: %s", exc)
-        return False
+    return True
 
 
 def clear() -> int:
@@ -243,14 +249,15 @@ def clear() -> int:
     except OSError:
         return 0
     for path in paths:
-        # Drop the sidecar first; an orphaned json is harmless (skipped by list).
+        # Delete the mp4 first; if it can't be unlinked, leave the sidecar so the video stays
+        # listable (an orphaned mp4 would vanish from the gallery). An orphaned json is harmless.
+        try:
+            path.unlink()
+        except OSError:
+            continue
+        removed += 1
         try:
             _sidecar_path(path.stem).unlink()
         except OSError:
             pass
-        try:
-            path.unlink()
-            removed += 1
-        except OSError:
-            continue
     return removed
diff --git a/studio/backend/tests/test_video_gallery.py b/studio/backend/tests/test_video_gallery.py
index 8df0bcff3b..9ad0ec630a 100644
--- a/studio/backend/tests/test_video_gallery.py
+++ b/studio/backend/tests/test_video_gallery.py
@@ -117,6 +117,35 @@ def test_delete_removes_both_files():
     assert len(gallery.list_videos()) == 1
 
 
+def test_delete_keeps_sidecar_listable_when_mp4_unlink_fails(monkeypatch):
+    # delete() must remove the MP4 FIRST: list_videos globs *.mp4 but requires a readable sidecar,
+    # so if the sidecar were dropped first and the mp4 unlink then failed (a Windows lock from a
+    # concurrent stream/transcode), the still-present mp4 would vanish from the gallery with no way
+    # to retry. Simulate the mp4 unlink failing and assert the video stays listable (sidecar kept).
+    record = gallery.save(_mp4(), _meta(prompt = "keep"))
+    directory = gallery.gallery_dir()
+    mp4 = directory / f"{record['id']}.mp4"
+    sidecar = directory / f"{record['id']}.json"
+
+    real_unlink = os.unlink
+
+    def _fail_on_mp4(path, *a, **k):
+        if str(path).endswith(".mp4"):
+            raise PermissionError("mp4 locked")
+        return real_unlink(path, *a, **k)
+
+    # Scope the os.unlink patch to its own context so undoing it does NOT also revert the autouse
+    # fixture's studio_root redirect (both share the function-scoped monkeypatch); otherwise
+    # list_videos below would read the real home dir instead of the tmp gallery.
+    with pytest.MonkeyPatch.context() as m:
+        m.setattr(os, "unlink", _fail_on_mp4)
+        assert gallery.delete(record["id"]) is False  # mp4 unlink failed
+    # The sidecar was NOT dropped, so the record is still listable and the user can retry.
+    assert sidecar.exists() and mp4.exists()
+    assert [r["prompt"] for r in gallery.list_videos()] == ["keep"]
+    assert gallery.delete(record["id"]) is True  # retry now succeeds
+
+
 def test_clear_returns_count():
     gallery.save(_mp4(), _meta(prompt = "a"))
     gallery.save(_mp4(), _meta(prompt = "b"))