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.
2026-10-26 12:53:10 -07:00
parent e84e56aa3b
commit e68cd66af5
6 changed files with 138 additions and 19 deletions
@@ -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
@@ -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 = {}

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

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

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