fix(thumbnails): report removals that fail and explain the quiet excepts (#320)
An old thumbnail the finishing move cannot remove (held open on Windows) is now an error row of the job with its path, and logged; a failed copy's temp that cannot go is logged with its path. Every except that only passes says why, and the thread runners say their caller re-raises.
This commit is contained in:
@@ -531,7 +531,7 @@ def _timed(func, timeout):
|
||||
def run():
|
||||
try:
|
||||
result["value"] = func()
|
||||
except BaseException as error: # handed to the caller
|
||||
except BaseException as error: # _timed() re-raises it in the caller
|
||||
result["error"] = error
|
||||
finally:
|
||||
done.set()
|
||||
|
||||
@@ -288,7 +288,7 @@ class _Probe:
|
||||
def run():
|
||||
try:
|
||||
result["value"] = preview_location.folder_state(path, expected_id)
|
||||
except BaseException as error: # handed to the caller
|
||||
except BaseException as error: # check() re-raises it in the caller
|
||||
result["error"] = error
|
||||
finally:
|
||||
done.set()
|
||||
@@ -428,7 +428,7 @@ def ensure_store(root, location=None, *, create=False):
|
||||
with store_io():
|
||||
os.mkdir(store)
|
||||
except FileExistsError:
|
||||
pass
|
||||
pass # already there: another writer made the key folder
|
||||
except OSError:
|
||||
return None
|
||||
return store
|
||||
@@ -528,7 +528,7 @@ def make_parent(dst, location=None):
|
||||
try:
|
||||
os.mkdir(current_folder)
|
||||
except FileExistsError:
|
||||
pass
|
||||
pass # already there: another writer made this folder
|
||||
if preview_location.is_link_like(current_folder):
|
||||
raise PermissionError(errno.EACCES, "a link in the thumbnail folder")
|
||||
|
||||
|
||||
@@ -82,7 +82,7 @@ class ThumbnailLocationMixin:
|
||||
if folder:
|
||||
contained.append(str(folder))
|
||||
except ImportError:
|
||||
pass
|
||||
pass # folder_paths exists only inside ComfyUI (tests run without it)
|
||||
try:
|
||||
contained.append(library_location.config_location()[1])
|
||||
except Exception: # no data folder can be decided
|
||||
|
||||
@@ -529,10 +529,23 @@ class ThumbnailMoveMixin:
|
||||
removed += 1
|
||||
else:
|
||||
complete = False
|
||||
relative = path.relative_to(store).as_posix()
|
||||
self.logger.warning(
|
||||
f"Could not remove the old thumbnail {path}; it is "
|
||||
"removed at the next start"
|
||||
)
|
||||
if job is not None:
|
||||
job.log_line(
|
||||
"In use by another program; removed at the next "
|
||||
f"start: {name}"
|
||||
# D10A: a removal that failed is an error the job lists.
|
||||
job.record(
|
||||
"errors",
|
||||
{
|
||||
"path": relative,
|
||||
"root": store.name,
|
||||
"detail": "In use by another program; removed "
|
||||
"at the next start",
|
||||
},
|
||||
f"In use by another program; removed at the next "
|
||||
f"start: {store.name}/{relative}",
|
||||
)
|
||||
files.remove_empty_folders(store)
|
||||
if job is not None and removed:
|
||||
|
||||
@@ -24,6 +24,7 @@ unlink (Windows).
|
||||
"""
|
||||
|
||||
import errno
|
||||
import logging
|
||||
import os
|
||||
import re
|
||||
import secrets
|
||||
@@ -41,6 +42,8 @@ try:
|
||||
except ImportError: # tests import the package top-level
|
||||
from utils.media_folders import in_trash_folder
|
||||
|
||||
logger = logging.getLogger("prompt_manager.thumbnails")
|
||||
|
||||
MOVE_TEMP = re.compile(r"\.pm-move-[0-9a-f]{8}\.tmp$")
|
||||
MOVE_TEMP_SUFFIX = ".pm-move-{token}.tmp"
|
||||
# The temp name adds this much to a destination path (".pm-move-xxxxxxxx.tmp").
|
||||
@@ -206,7 +209,7 @@ def _make_folders(base, relative_dir, created):
|
||||
os.mkdir(current)
|
||||
created.append(str(current))
|
||||
except FileExistsError:
|
||||
pass
|
||||
pass # already there: made by an earlier copy into this folder
|
||||
except OSError as error:
|
||||
if error.errno in (errno.ENOSPC, errno.EDQUOT, errno.EROFS):
|
||||
raise FolderGone(str(error)) from error
|
||||
@@ -263,10 +266,18 @@ def copy_one(source, destination, size, mtime_ns, created, *, long_paths=None):
|
||||
|
||||
|
||||
def _unlink(path):
|
||||
"""Remove a copy's temp file after a failed copy; True when it is gone.
|
||||
|
||||
One that cannot go (a Windows lock, permissions) is logged with its path;
|
||||
the next start's temp sweep removes it (``.pm-move-*.tmp``)."""
|
||||
try:
|
||||
os.unlink(path)
|
||||
except OSError:
|
||||
pass
|
||||
return True
|
||||
except FileNotFoundError:
|
||||
return True # never written, or already renamed into place
|
||||
except OSError as error:
|
||||
logger.warning(f"Could not remove the move's temp file {path}: {error}")
|
||||
return False
|
||||
|
||||
|
||||
# ── removing ─────────────────────────────────────────────────────────
|
||||
|
||||
@@ -94,7 +94,7 @@ def _remove(path):
|
||||
try:
|
||||
os.unlink(path)
|
||||
except FileNotFoundError:
|
||||
pass
|
||||
pass # already gone: removing it is the goal
|
||||
|
||||
|
||||
def _is_network(path):
|
||||
|
||||
@@ -165,8 +165,10 @@ class TestMoveToAFolder(MoveCase):
|
||||
make_png(self.folder / "old-0123abcd" / "x_thumb.webp", size=(4, 4))
|
||||
preview_store.refresh()
|
||||
status, plan = await self.plan({"mode": "source"})
|
||||
self.assertEqual(status, 200)
|
||||
self.assertEqual(plan["totals"]["unlisted_sources"], 1)
|
||||
_plan, job = await self.move({"mode": "source"})
|
||||
_, job = await self.move({"mode": "source"})
|
||||
self.assertEqual(job.state, "finished", job.log)
|
||||
self.assertFalse(self.folder.exists())
|
||||
|
||||
async def test_confirmation_and_a_stale_token(self):
|
||||
@@ -296,6 +298,11 @@ class TestRestart(MoveCase):
|
||||
_plan, job = await self.move({"mode": "folder", "path": str(self.folder)})
|
||||
self.assertTrue(preview_store.setting().cleanup)
|
||||
self.assertTrue((self.old_store / "a_thumb.webp").exists())
|
||||
# Each removal that failed is an error the job counts and lists by path.
|
||||
rows = job.outcomes["errors"]
|
||||
self.assertEqual(job.counters["errors"], len(rows))
|
||||
self.assertIn("sub/b_thumb.webp", {row["path"] for row in rows})
|
||||
self.assertTrue(all("next start" in row["detail"] for row in rows))
|
||||
job = await self.api.finish_thumbnail_move()
|
||||
await job.task
|
||||
self.assertEqual(job.title, "Move thumbnails (finishing)")
|
||||
@@ -401,6 +408,14 @@ class TestFiles(unittest.TestCase):
|
||||
self.assertTrue(files.too_long("D:\\" + "x" * 240, long_paths=False))
|
||||
self.assertFalse(files.too_long("D:\\x", long_paths=False))
|
||||
|
||||
def test_a_failed_copys_temp_that_cannot_go_is_logged(self):
|
||||
with patch.object(files.os, "unlink", side_effect=PermissionError("held")):
|
||||
with self.assertLogs("prompt_manager.thumbnails", "WARNING") as logs:
|
||||
self.assertFalse(files._unlink("D:/thumbs/.pm-move-1.tmp"))
|
||||
self.assertIn(".pm-move-1.tmp", logs.output[0])
|
||||
with patch.object(files.os, "unlink", side_effect=FileNotFoundError()):
|
||||
self.assertTrue(files._unlink("gone"))
|
||||
|
||||
def test_a_held_open_file_is_retried_then_left(self):
|
||||
waits = []
|
||||
error = OSError("in use")
|
||||
|
||||
Reference in New Issue
Block a user