From e68cd66af5eb67a333bcc7895684d821ceae5f05 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Mon, 26 Oct 2026 12:53:10 -0700 Subject: [PATCH] fix(images): keep removed roots and cancelled walks from walking again (#348, PR 2) Review findings on the monitor-triggered walks: - reset_gallery_roots_cache now retires every listing record: its pending timer is cancelled, dirty is cleared, and no mark, timer or trailing walk starts a walk of it until a reader or the warm-up asks for that root again. Before, a timer or trailing walk pending when the roots changed stored a listing for the removed root, and the monitor (which watches the old roots until restart) kept it walking on every new output. - A walk task cancelled outside cleanup_gallery_listings no longer starts a trailing walk while the loop shuts down. - root_contains ignores a Windows extended-length prefix (\\?\ and \\?\UNC\) on either side, which realpath can keep on a file past MAX_PATH. - The REWALK comment states the ~20% GIL share only for walks up to 3 s. --- TESTING.md | 7 +++- py/api/__init__.py | 22 +++++++++--- py/api/gallery_listing.py | 34 ++++++++++++++---- py/api/images.py | 11 +++--- tests/test_api_images.py | 65 +++++++++++++++++++++++++++++++++-- tests/test_gallery_listing.py | 18 ++++++++++ 6 files changed, 138 insertions(+), 19 deletions(-) diff --git a/TESTING.md b/TESTING.md index 4c4dca3d..94444b07 100644 --- a/TESTING.md +++ b/TESTING.md @@ -349,7 +349,12 @@ hold a walk open with a gated scan instead of sleeping. arms at most one per root and never sooner than 0.5 s, and keeps the earlier deadline when the stale refresh and a dirty root both want one; every walk start (read, Rescan, warm-up or timer) cancels it. A failed walk - clears `dirty`. Tests patch `REWALK_FLOOR`/`REWALK_CAP` and run the pending + clears `dirty`; a cancelled walk starts no trailing walk. When the output + roots change (`reset_gallery_roots_cache`) every record is `retired`: its + timer is cancelled and no mark, timer or trailing walk walks it until a + reader or the warm-up asks for that root again (the monitor keeps watching + a removed root until restart). `root_contains` ignores a Windows `\\?\` + (or `\\?\UNC\`) prefix on either side. Tests patch `REWALK_FLOOR`/`REWALK_CAP` and run the pending timer's callback by hand (`_fire`) instead of waiting for it. - **Measuring it.** `tools/vendor/perf/images_load_probe.mjs` is a maintainer tool, never run in CI. After `npm ci --prefix tools/vendor` and diff --git a/py/api/__init__.py b/py/api/__init__.py index c7bd911f..7f66f6b2 100644 --- a/py/api/__init__.py +++ b/py/api/__init__.py @@ -495,7 +495,7 @@ class PromptManagerAPI( """The shared listing record of one output root (created empty).""" return self._root_listings.setdefault(str(root), RootListing()) - def invalidate_gallery_cache(self): + def invalidate_gallery_cache(self, retire=False): """Drop every root's listing, so the next read waits for a new walk. Called after files are deleted through the app (duplicate cleanup and @@ -506,6 +506,9 @@ class PromptManagerAPI( _workspace_listed and _card_facts); its sort orders are dropped, as the walk replaces them anyway, and so is the favorites lookup, which holds the old list. + + ``retire`` (the roots changed): no background walk of a root starts + again until a reader or the warm-up asks for it. """ loop = self._listing_loop if loop is not None and loop.is_running() and _running_loop() is not loop: @@ -516,7 +519,7 @@ class PromptManagerAPI( def drop(): try: - self._drop_listings() + self._drop_listings(retire) finally: dropped.set() @@ -524,11 +527,19 @@ class PromptManagerAPI( if not dropped.wait(_OFF_LOOP_DROP_WAIT): self.logger.warning("The Images listing drop is still queued") return - self._drop_listings() + self._drop_listings(retire) - def _drop_listings(self): + def _drop_listings(self, retire=False): for record in list(self._root_listings.values()): record.drop() + if retire: + # The roots changed: a removed root must not come back through + # a pending timer, a trailing walk or a monitor mark. + record.retired = True + record.dirty = False + handle, record.rewalk_handle = record.rewalk_handle, None + if handle is not None: + handle.cancel() self._workspace_order_cache = {} self._favorite_listing_cache = {} @@ -546,6 +557,7 @@ class PromptManagerAPI( self._listings_closed = False try: for root in self._get_all_output_dirs(): + self._root_listing(root).retired = False self._schedule_walk(root) except Exception as e: self.logger.warning(f"Could not warm the Images listing: {e}") @@ -594,7 +606,7 @@ class PromptManagerAPI( def reset_gallery_roots_cache(self): """Drop every cached listing and derived workspace value (roots changed).""" - self.invalidate_gallery_cache() + self.invalidate_gallery_cache(retire=True) self._workspace_listed_cache = {} self._workspace_card_cache = {} diff --git a/py/api/gallery_listing.py b/py/api/gallery_listing.py index 518ab52c..b36d82b0 100644 --- a/py/api/gallery_listing.py +++ b/py/api/gallery_listing.py @@ -32,9 +32,10 @@ REFRESH_DELAY = 1.0 # No background walk of a root starts sooner than rewalk_interval(last walk's # duration) after its previous walk started: FACTOR times that duration, at -# least FLOOR and at most CAP seconds. Background walks then hold the GIL at -# most ~20% of the time; the cap keeps a new output listed within ~20 s on a -# slow machine. Read through this module so tests can patch them. +# least FLOOR and at most CAP seconds. Walks up to CAP / FACTOR (3 s) then +# hold the GIL at most ~20% of the time; the cap keeps a new output listed +# within ~20 s, at the cost of a higher share (back to back from 15 s) on +# slower walks. Read through this module so tests can patch them. REWALK_FLOOR = 5 REWALK_FACTOR = 5 REWALK_CAP = 15 @@ -102,11 +103,15 @@ class RootListing: # normcase(realpath(root)), computed by the walk in the executor: monitor # paths are matched against it without file I/O on the loop. resolved_root: str | None = None - # Monotonic start and duration of the last walk (REWALK_MIN_INTERVAL). + # Monotonic start and duration of the last walk (rewalk_interval). last_started_at: float | None = None last_duration: float | None = None # A settled output was reported under this root; cleared when a walk starts. dirty: bool = False + # Dropped because the output roots changed: no background walk until a + # reader (or the warm-up) asks for this root again. The monitor keeps + # watching a removed root until ComfyUI restarts. + retired: bool = False def drop(self) -> None: self.files = None @@ -147,7 +152,7 @@ def folder_sets(root, files) -> tuple[frozenset, frozenset]: def rewalk_interval(last_duration) -> float: - """REWALK_MIN_INTERVAL after a walk that took *last_duration* seconds.""" + """Seconds from a walk's start to the next background walk of its root.""" scaled = REWALK_FACTOR * (last_duration or 0.0) return min(REWALK_CAP, max(REWALK_FLOOR, scaled)) @@ -159,13 +164,28 @@ def root_contains(resolved_root: str, path, pathmod=os.path) -> bool: so Windows matches case-insensitively and with either separator. The match ends at a separator: ``/out2/x.png`` is not under ``/out``. """ - target = pathmod.normcase(str(path)) - prefix = resolved_root + target = _plain(pathmod.normcase(str(path)), pathmod) + prefix = _plain(resolved_root, pathmod) if not prefix.endswith(pathmod.sep): prefix += pathmod.sep return target.startswith(prefix) +def _plain(path: str, pathmod) -> str: + """*path* (normcased) without a Windows extended-length prefix. + + ``realpath`` may keep ``\\\\?\\`` on a file past MAX_PATH while its shorter + root resolves without it. + """ + if pathmod.sep != "\\": + return path + if path.startswith("\\\\?\\unc\\"): + return "\\\\" + path[8:] + if path.startswith("\\\\?\\"): + return path[4:] + return path + + def resolve_root(root) -> str: """``normcase(realpath(root))``, the spelling monitor paths are matched in. diff --git a/py/api/images.py b/py/api/images.py index 02c106fb..84b21a80 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -1408,6 +1408,7 @@ class ImageRoutesMixin: even if a previous listing exists. """ record = self._root_listing(output_path) + record.retired = False # a configured root: read, so listened to needed = record.walk_seq + 1 if fresh else 0 discarded = 0 while True: @@ -1456,7 +1457,7 @@ class ImageRoutesMixin: if self._listings_closed: return for key, record in list(self._root_listings.items()): - if record.files is None and record.flight is None: + if record.retired or (record.files is None and record.flight is None): continue resolved = record.resolved_root or os.path.normcase(key) if gallery_listing.root_contains(resolved, path): @@ -1476,7 +1477,7 @@ class ImageRoutesMixin: if self._listings_closed: return record = self._root_listing(output_path) - if record.flight is not None: + if record.flight is not None or record.retired: return now = _time.monotonic() due = now + delay @@ -1505,7 +1506,7 @@ class ImageRoutesMixin: start cancels this timer, so when it fires the walk is still due. """ record.rewalk_handle = None - if self._listings_closed or record.flight is not None: + if self._listings_closed or record.flight is not None or record.retired: return self._start_walk(output_path, record) @@ -1538,12 +1539,14 @@ class ImageRoutesMixin: error never reaches a future nobody awaits. """ outcome = None + cancelled = False started = record.last_started_at try: files, folders, with_ancestors, resolved = await self._run_in_executor( walk_root, self._scan_gallery_files_sync, output_path, record.files ) except asyncio.CancelledError: + cancelled = True outcome = WalkOutcome.failed(RuntimeError(WALK_STOPPED)) raise except Exception as exc: @@ -1577,7 +1580,7 @@ class ImageRoutesMixin: record.flight = None if not flight.done(): flight.set_result(outcome) - if record.dirty: + if record.dirty and not cancelled: # An output settled during this walk: the trailing walk. self._schedule_walk(output_path) diff --git a/tests/test_api_images.py b/tests/test_api_images.py index a1a62894..40d5861d 100644 --- a/tests/test_api_images.py +++ b/tests/test_api_images.py @@ -2542,7 +2542,7 @@ class TestSharedListing(ImageAPITestCase): record = self._record() shift = (seconds or gallery_listing.REFRESH_AFTER) + 1 record.walked_at -= shift - # An old listing's walk also started long ago (REWALK_MIN_INTERVAL). + # An old listing's walk also started long ago (rewalk_interval). record.last_started_at -= shift async def _settle(self): @@ -3079,7 +3079,7 @@ class TestSharedListing(ImageAPITestCase): # -- monitor-triggered walks (#348, PR 2) --------------------------- def _interval(self, seconds): - """Patch REWALK_MIN_INTERVAL to *seconds*, whatever the walk took.""" + """Patch rewalk_interval() to *seconds*, whatever the walk took.""" for name in ("REWALK_FLOOR", "REWALK_CAP"): patcher = patch.object(gallery_listing, name, seconds) patcher.start() @@ -3441,6 +3441,67 @@ class TestSharedListing(ImageAPITestCase): await self._settle() self.assertEqual(len(record.files), 2) + async def test_a_root_removed_from_the_roots_is_never_walked_again(self): + make_png(self.output_dir / "a.png") + record = await self._listed() + self._interval(30) + self.api.mark_root_dirty(self._output("b.png")) + handle = record.rewalk_handle + self.assertIsNotNone(handle) + self.api.reset_gallery_roots_cache() # e.g. the gallery root changed + self.assertTrue(handle.cancelled()) + self.assertIsNone(record.rewalk_handle) + self.assertFalse(record.dirty) + self._interval(0) + # The monitor still watches the old root until ComfyUI restarts. + self.api.mark_root_dirty(self._output("c.png")) + self.api._delayed_walk(self.output_dir, record) # a timer that raced + self.assertIsNone(record.flight) + self.assertIsNone(record.files) + self.assertEqual(self.scan.calls, 1) + + async def test_a_walk_running_when_the_roots_change_gets_no_trailing_walk(self): + make_png(self.output_dir / "a.png") + record = await self._listed() + self._interval(0) + gate = self.scan.hold_next() + self.api.mark_root_dirty(self._output("b.png")) + await self.scan.wait_entered() + self.api.reset_gallery_roots_cache() + self.api.mark_root_dirty(self._output("c.png")) + gate.set() + await self._settle() + self.assertEqual(self.scan.calls, 2) + self.assertIsNone(record.files) # the running walk was discarded + self.assertFalse(record.dirty) + + async def test_a_root_read_again_after_the_roots_change_listens_again(self): + make_png(self.output_dir / "a.png") + record = await self._listed() + self.api.reset_gallery_roots_cache() + record = await self._listed() # still configured: read and walked + self._interval(0) + self.api.mark_root_dirty(self._output("b.png")) + await self._settle() + self.assertEqual(len(record.files), 2) + + async def test_a_cancelled_walk_starts_no_trailing_walk(self): + make_png(self.output_dir / "a.png") + record = await self._listed() + self._interval(0) + gate = self.scan.hold_next() + self.api.mark_root_dirty(self._output("b.png")) + await self.scan.wait_entered() + self.api.mark_root_dirty(self._output("c.png")) + (task,) = self.api._listing_tasks + task.cancel() # e.g. the loop shutting down without on_cleanup + await asyncio.gather(task, return_exceptions=True) + gate.set() + self.assertTrue(task.cancelled()) + self.assertIsNone(record.flight) + self.assertIsNone(record.rewalk_handle) + self.assertEqual(self.api._listing_tasks, set()) + async def test_monitor_walks_during_a_workspace_build_add_one_more_build(self): make_png(self.output_dir / "a.png") release, entered = threading.Event(), threading.Event() diff --git a/tests/test_gallery_listing.py b/tests/test_gallery_listing.py index 77fe6467..e6d4045d 100644 --- a/tests/test_gallery_listing.py +++ b/tests/test_gallery_listing.py @@ -280,6 +280,24 @@ class RootContainsTests(unittest.TestCase): self.assertTrue(root_contains(share, "\\\\server\\share\\out\\a.png", ntpath)) self.assertFalse(root_contains(share, "\\\\server\\share\\a.png", ntpath)) + def test_a_windows_extended_length_prefix_is_ignored(self): + root = ntpath.normcase("C:\\ComfyUI\\Output") + long_file = "\\\\?\\C:\\ComfyUI\\Output\\deep\\a.png" + self.assertTrue(root_contains(root, long_file, ntpath)) + prefixed_root = ntpath.normcase("\\\\?\\C:\\ComfyUI\\Output") + self.assertTrue( + root_contains(prefixed_root, "C:\\ComfyUI\\Output\\a.png", ntpath) + ) + share = ntpath.normcase("\\\\Server\\Share\\Out") + unc_file = "\\\\?\\UNC\\server\\share\\out\\a.png" + self.assertTrue(root_contains(share, unc_file, ntpath)) + self.assertFalse( + root_contains(root, "\\\\?\\C:\\ComfyUI\\Output2\\a.png", ntpath) + ) + + def test_posix_keeps_a_literal_question_mark_folder(self): + self.assertFalse(root_contains("/srv/out", "//?/srv/out/a.png", posixpath)) + def test_the_default_is_this_platforms_rule(self): root = os.path.normcase(os.path.realpath(tempfile.gettempdir())) inside = os.path.join(root, "x.png")