From d62c37df77b8cada8904f778b25f36e13af51dcd Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Sat, 31 Oct 2026 22:39:12 -0700 Subject: [PATCH] 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. --- py/api/preview_location.py | 2 +- py/api/preview_store.py | 6 +++--- py/api/thumbnail_location.py | 2 +- py/api/thumbnail_move.py | 19 ++++++++++++++++--- py/api/thumbnail_move_files.py | 17 ++++++++++++++--- py/api/thumbnail_move_plan.py | 2 +- tests/test_thumbnail_move.py | 17 ++++++++++++++++- 7 files changed, 52 insertions(+), 13 deletions(-) diff --git a/py/api/preview_location.py b/py/api/preview_location.py index dab96151..f2cd6337 100644 --- a/py/api/preview_location.py +++ b/py/api/preview_location.py @@ -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() diff --git a/py/api/preview_store.py b/py/api/preview_store.py index 50deb422..e4c1a932 100644 --- a/py/api/preview_store.py +++ b/py/api/preview_store.py @@ -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") diff --git a/py/api/thumbnail_location.py b/py/api/thumbnail_location.py index 210b39a7..c9a1a086 100644 --- a/py/api/thumbnail_location.py +++ b/py/api/thumbnail_location.py @@ -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 diff --git a/py/api/thumbnail_move.py b/py/api/thumbnail_move.py index 412af1c7..9a5a9eda 100644 --- a/py/api/thumbnail_move.py +++ b/py/api/thumbnail_move.py @@ -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: diff --git a/py/api/thumbnail_move_files.py b/py/api/thumbnail_move_files.py index 4b0821fc..cd3da88a 100644 --- a/py/api/thumbnail_move_files.py +++ b/py/api/thumbnail_move_files.py @@ -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 ───────────────────────────────────────────────────────── diff --git a/py/api/thumbnail_move_plan.py b/py/api/thumbnail_move_plan.py index afbc1b09..df70500d 100644 --- a/py/api/thumbnail_move_plan.py +++ b/py/api/thumbnail_move_plan.py @@ -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): diff --git a/tests/test_thumbnail_move.py b/tests/test_thumbnail_move.py index 52543f1e..0189fc90 100644 --- a/tests/test_thumbnail_move.py +++ b/tests/test_thumbnail_move.py @@ -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")