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.
This commit is contained in:
Daniel Han 2026-07-07 11:57:37 +00:00
commit fda725241d
2 changed files with 49 additions and 13 deletions

View file

@ -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

View file

@ -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"))