nodes/ComfyAssets-ComfyUI_PromptManager
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(images): review fixes for adds without a walk (#348, PR 2)

Browse Source 
- _newest_step steps the cached newest order only when the per-file cache
  still holds exactly the values this call built. A stale page read racing
  a newer warm build (both run in executor threads) stored an order
  labelled with older values than it held, and the next read inserted the
  new file a second time. It also sorts in full when a roots reset dropped
  the per-file values meanwhile.
- The stored spelling is checked on Windows too (nt._getfinalpathname:
  realpath keeps the given case when it cannot open the file), and before
  the name filters, which compare the walk's stored spelling. macOS opens
  non-blocking, so a FIFO named like an image never holds the executor.
- An output in a folder the walk cannot list (no read permission) is not
  added, as the walk skips it.
- Windows-portable unit tests: POSIX paths in the insert tests, and a
  str.lower fold for the length guard (Windows' normcase keeps lengths).
This commit is contained in:
Vito Sansevero
2026-10-26 15:11:04 -07:00
parent 40c89625b9
commit aeffe87dc1
4 changed files with 194 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
TESTING.md
+14 -5
View File
@@ -348,9 +348,14 @@ hold a walk open with a gated scan instead of sleeping.
root in its own case (Windows compares folded, slices the original);
`_run_adds` places the pending files of one root in one executor batch
(`_patch_listing`): `_settled_entry` applies the walk's filters
(`_name_suffix` in `MEDIA_EXTENSIONS`, no `thumbnails` folder, at most
`MAX_SCAN_DEPTH` folders, not a symlink or folder by `lstat`; on macOS the
`F_GETPATH` spelling must match) and `insert_entry` builds a new list with
(not a symlink or folder by `lstat`, `_name_suffix` in `MEDIA_EXTENSIONS`,
no `thumbnails` folder, at most `MAX_SCAN_DEPTH` folders, every folder on
the way listable by `os.access`). Where names fold case the stored
spelling must match first, as the filters compare it: Windows through
`nt._getfinalpathname` (its `realpath` keeps the given case when it cannot
open the file), macOS through `F_GETPATH` on a non-blocking open (a FIFO
never holds the executor); a case-insensitive mount on Linux is not
checked and is corrected by the next walk. `insert_entry` builds a new list with
the `(Path, mtime)` entry where the walk's newest-first sort puts it,
replacing the same path's entry. `add_folder` copies both folder sets. The
batch is stored only when no walk started and no drop happened while it
@@ -361,7 +366,8 @@ hold a walk open with a gated scan instead of sleeping.
- **When it walks instead.** The root is marked dirty (`mark_root_dirty`,
`_mark_dirty`) when the add cannot be exact: a walk is running (it may have
passed the folder: the trailing walk), the path cannot be sliced, the
`lstat` fails, another file has the same mtime (a walk orders those by its
`lstat` or the spelling read fails, the stored spelling differs, another
file has the same mtime (a walk orders those by its
visit order), the listing is at `MAX_GALLERY_FILES`, or the batch raised.
A file the walk's filters exclude is neither added nor walked. A root
without a listing, or a dropped one, is walked by its next read. A dirty
@@ -385,7 +391,10 @@ hold a walk open with a gated scan instead of sleeping.
the change by identity (a walk builds new tuples and is rebuilt in full,
as is any change of more than `MAX_DELTA` entries). The per-file values
build only the added files (`_workspace_listed_step`), the newest order
places them by bisection (`_newest_step`), the favorites lookup is copied
places them by bisection (`_newest_step`, only when the per-file cache
still holds exactly the values this call built: a page read and the warm
build run in executor threads at once, and an order labelled with older
values than it holds would add a file twice), the favorites lookup is copied
and extended (`_favorite_listing_step`, rebuilt when a key collides), and
the other orders, wanted paths and locators rebuild lazily. On a synthetic
20,000-file listing a one-file add costs about 3 ms in the executor and
py/api/images.py
+50 -25
View File
@@ -865,37 +865,53 @@ def _settled_entry(root, relative):
"""``(Path, mtime)`` of a settled output as a walk would list it.
*relative* is its part below *root* (``gallery_listing.relative_part``).
None when a walk would not list it (``_walk_media``'s filters); raises
``OSError`` when that cannot be told (the file is unreadable or gone, or
its on-disk spelling differs). Blocking: run it in the executor.
None when a walk would not list it (``_walk_media``'s filters, and every
folder on the way must be listable); raises ``OSError`` when that cannot
be told (the file is gone or unreadable, or its on-disk spelling differs
or cannot be read). Blocking: run it in the executor.
"""
path = Path(os.path.join(os.fspath(root), relative))
info = os.lstat(path)
if stat.S_ISLNK(info.st_mode) or stat.S_ISDIR(info.st_mode):
return None
# Before the name filters: they compare the walk's (stored) spelling.
on_disk = _on_disk_path(path)
if on_disk is not None and not on_disk.endswith(os.sep + relative):
raise OSError(f"{relative} is spelled differently on disk")
*folders, name = relative.split(os.sep)
if _name_suffix(name).lower() not in MEDIA_EXTENSIONS:
return None
if "thumbnails" in folders or len(folders) > MAX_SCAN_DEPTH:
return None
path = Path(os.path.join(os.fspath(root), relative))
info = os.lstat(path)
if stat.S_ISLNK(info.st_mode) or stat.S_ISDIR(info.st_mode):
return None
on_disk = _on_disk_path(path)
if on_disk is not None and not on_disk.endswith(os.sep + relative):
raise OSError(f"{relative} is spelled differently on disk")
folder = os.fspath(root)
for part in ("", *folders):
folder = os.path.join(folder, part) if part else folder
if not os.access(folder, os.R_OK | os.X_OK):
return None # the walk cannot list this folder
return path, info.st_mtime
def _on_disk_path(path):
"""macOS: the file's path in its on-disk case (``F_GETPATH``); else None.
def _on_disk_path(path, platform=None):
"""The file's path as its folders store the names, where that can differ.
macOS file systems ignore case and ``realpath`` keeps the case it was
given, while a walk lists the names as stored. Windows' ``realpath``
already returns the stored case.
Windows and macOS file systems ignore case. Windows' ``realpath`` gives
the stored case, but keeps the given one when it cannot open the file;
macOS' ``realpath`` always keeps the given one. A walk lists the names as
stored, so an add checks them: Windows through ``GetFinalPathNameByHandle``
(``nt._getfinalpathname``), macOS through ``F_GETPATH``. Raises OSError
when it cannot be read; None elsewhere (case-sensitive file systems).
"""
if sys.platform != "darwin":
platform = platform or sys.platform
if platform == "win32":
import nt
return nt._getfinalpathname(os.fspath(path))
if platform != "darwin":
return None
import fcntl
descriptor = os.open(path, os.O_RDONLY)
# Non-blocking: a FIFO named like an image must not hold the executor.
descriptor = os.open(path, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0))
try:
raw = fcntl.fcntl(descriptor, fcntl.F_GETPATH, bytes(1024))
finally:
@@ -1980,7 +1996,9 @@ class ImageRoutesMixin:
old is new for old, new in zip(cached[0], parts, strict=True)
):
return cached[1]
newest = self._newest_step(listings, cached) if cached is not None else None
newest = None
if cached is not None:
newest = self._newest_step(listings, parts, cached)
if newest is None:
newest = sorted(
(entry for part in parts for entry in part),
@@ -1991,19 +2009,26 @@ class ImageRoutesMixin:
self._workspace_order_cache[key] = (parts, orders)
return orders
def _newest_step(self, listings: list[tuple], cached: tuple) -> list | None:
def _newest_step(self, listings, parts, cached) -> list | None:
"""The cached newest order with each part's last step applied, or None.
Only when every changed part is one step (``_workspace_listed_step``)
past the part this order was built from: the added values are placed
by bisection instead of sorting the whole listing again.
Only when every changed part is exactly the value list this call
built (*parts*) and one step (``_workspace_listed_step``) past the
part the cached order was built from: the added values are placed by
bisection instead of sorting the whole listing again. Page reads and
the warm build run in executor threads at once, so the per-file cache
may already hold a newer step than *parts*; then this sorts in full.
"""
old_parts, orders = cached
added, gone = [], []
for (index, root, _files), old in zip(listings, old_parts, strict=True):
listed = self._workspace_listed_cache.get((index, str(root)))
if listed[1] is old:
for (index, root, _files), part, old in zip(
listings, parts, old_parts, strict=True
):
if part is old:
continue
listed = self._workspace_listed_cache.get((index, str(root)))
if listed is None or listed[1] is not part:
return None
step = listed[3]
if step is None or step[0] is not old:
return None
tests/test_api_images.py
+116 -10
View File
@@ -4015,17 +4015,123 @@ class TestSharedListing(ImageAPITestCase):
[entry.path for entry in newest], [path for path, _m, _i in expected]
)
def test_the_on_disk_spelling_is_read_on_macos_only(self):
with patch.object(images_module.sys, "platform", "linux"):
self.assertIsNone(images_module._on_disk_path(__file__))
def test_the_on_disk_spelling_is_read_where_names_fold_case(self):
read = images_module._on_disk_path
self.assertIsNone(read(__file__, platform="linux"))
fake_fcntl = MagicMock(F_GETPATH=50)
fake_fcntl.fcntl.return_value = b"/Out/Sub/A.png" + bytes(10)
fake_nt = MagicMock()
fake_nt._getfinalpathname.return_value = "\\\\?\\C:\\Out\\A.png"
with patch.dict(sys.modules, {"fcntl": fake_fcntl, "nt": fake_nt}):
self.assertEqual(read(__file__, platform="darwin"), "/Out/Sub/A.png")
self.assertEqual(read(__file__, platform="win32"), "\\\\?\\C:\\Out\\A.png")
self.assertEqual(fake_fcntl.fcntl.call_args.args[1], 50)
fake_nt._getfinalpathname.assert_called_once_with(__file__)
def test_a_fifo_named_like_an_image_never_blocks_the_spelling_read(self):
if not hasattr(os, "mkfifo"):
self.skipTest("no FIFOs here")
fifo = self.output_dir / "pipe.png"
os.mkfifo(fifo)
fake = MagicMock(F_GETPATH=50)
fake.fcntl.return_value = b"/Out/Sub/A.png" + bytes(10)
with (
patch.dict(sys.modules, {"fcntl": fake}),
patch.object(images_module.sys, "platform", "darwin"),
):
self.assertEqual(images_module._on_disk_path(__file__), "/Out/Sub/A.png")
self.assertEqual(fake.fcntl.call_args.args[1], 50)
fake.fcntl.return_value = os.fsencode(str(fifo))
done = []
def read():
with patch.dict(sys.modules, {"fcntl": fake}):
done.append(images_module._on_disk_path(fifo, platform="darwin"))
reader = threading.Thread(target=read, daemon=True)
reader.start()
reader.join(_HANG_LIMIT)
if reader.is_alive():
# Unblock the open so the thread ends, then fail.
with open(fifo, "wb"):
pass
self.fail("opening the FIFO blocked")
self.assertEqual(done, [str(fifo)])
async def test_a_folder_the_walk_cannot_list_gets_no_add(self):
if os.name == "nt" or not hasattr(os, "geteuid") or os.geteuid() == 0:
self.skipTest("needs POSIX permissions and a non-root user")
self._at("a.png", 1000)
record = await self._listed()
report = self._at(os.path.join("locked", "b.png"), 2000)
locked = self.output_dir / "locked"
os.chmod(locked, 0o300) # enter but not list
try:
self.assertEqual(self._walked(), record.files) # the walk skips it
files = record.files
self.api.output_settled(report)
await self._settle()
finally:
os.chmod(locked, 0o700)
self.assertIs(record.files, files)
self._no_walk_pending(record)
async def test_a_folder_named_like_an_image_is_not_added(self):
self._at("a.png", 1000)
record = await self._listed()
folder = self.output_dir / "folder.png"
folder.mkdir()
files = record.files
self.api.output_settled(os.path.realpath(folder))
await self._settle()
self.assertIs(record.files, files)
self._no_walk_pending(record)
async def test_the_filters_read_the_stored_spelling(self):
# Reported as thumbnails/, stored as Thumbnails/: a walk lists it, so
# the add must not skip it as a preview folder; the spelling differs,
# so the root is walked.
self._at("a.png", 1000)
record = await self._listed()
report = self._at(os.path.join("thumbnails", "t.png"), 2000)
stored = os.path.join(str(self.output_dir), "Thumbnails", "t.png")
with patch.object(images_module, "_on_disk_path", return_value=stored):
await self._assert_walks_instead(record, report)
def test_a_stale_read_racing_a_newer_build_sorts_in_full(self):
root = self.output_dir
f0 = self._synthetic(root, 50)
f1 = gallery_listing.insert_entry(f0, (root / "n1.png", 5000.0), 100)
f2 = gallery_listing.insert_entry(f1, (root / "n2.png", 6000.0), 100)
self.api._workspace_orders([(0, root, f0)])
self.api._workspace_listed(0, root, f1) # the warm build of add 1
real = self.api._workspace_listed
armed = [True]
def listed(index, part_root, files):
values = real(index, part_root, files)
if armed[0] and files is f1:
armed[0] = False
# Another executor thread reads f2 in full meanwhile.
self.api._workspace_orders([(0, root, f2)])
return values
with patch.object(self.api, "_workspace_listed", side_effect=listed):
stale = self.api._workspace_orders([(0, root, f1)])["newest"]
self.assertEqual([entry.path for entry in stale], [p for p, _m in f1])
newest = self.api._workspace_orders([(0, root, f2)])["newest"]
self.assertEqual([entry.path for entry in newest], [p for p, _m in f2])
def test_a_roots_change_during_an_order_build_sorts_in_full(self):
root = self.output_dir
f0 = self._synthetic(root, 20)
f1 = gallery_listing.insert_entry(f0, (root / "n1.png", 5000.0), 100)
self.api._workspace_orders([(0, root, f0)])
real = self.api._workspace_listed
def listed(index, part_root, files):
values = real(index, part_root, files)
# A roots reset in another thread, between this call's two cache
# reads: the per-file values are gone, the order not yet.
self.api._workspace_listed_cache = {}
return values
with patch.object(self.api, "_workspace_listed", side_effect=listed):
newest = self.api._workspace_orders([(0, root, f1)])["newest"]
self.assertEqual([entry.path for entry in newest], [p for p, _m in f1])
def test_the_favorite_listing_step_rebuilds_when_it_cannot_follow(self):
root = self.output_dir
tests/test_gallery_listing.py
+14 -8
View File
@@ -6,7 +6,8 @@ import posixpath
import sys
import tempfile
import unittest
from pathlib import Path, PurePath, PureWindowsPath
from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath
from types import SimpleNamespace
from unittest.mock import patch
sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
@@ -354,10 +355,15 @@ class RelativePartTests(unittest.TestCase):
self.assertEqual(relative_part(drive, "D:\\X\\a.png", ntpath), "X\\a.png")
def test_a_case_fold_that_changes_the_length_is_not_sliced(self):
root = ntpath.normcase("C:\\Out")
path = "C:\\Out\\\u0130mage.png" # Turkish dotted I lowers to two chars
self.assertNotEqual(len(ntpath.normcase(path)), len(path))
self.assertIsNone(relative_part(root, path, ntpath))
# str.lower folds the Turkish dotted I to two characters (Windows'
# own normcase keeps the length; the guard covers any fold that not).
folding = SimpleNamespace(
sep="\\", normcase=lambda text: text.replace("/", "\\").lower()
)
path = "C:\\Out\\\u0130mage.png"
self.assertNotEqual(len(folding.normcase(path)), len(path))
self.assertIsNone(relative_part("c:\\out", path, folding))
self.assertEqual(relative_part("c:\\out", "C:\\Out\\Ab.png", folding), "Ab.png")
def test_dot_parts_are_never_sliced(self):
for path in ("/srv/out/../x.png", "/srv/out/./x.png", "/srv/out//x.png"):
@@ -366,7 +372,7 @@ class RelativePartTests(unittest.TestCase):
class InsertEntryTests(unittest.TestCase):
def setUp(self):
self.root = PurePath("/out")
self.root = PurePosixPath("/out")
self.files = [
(self.root / "c.png", 30.0),
(self.root / "b.png", 20.0),
@@ -386,7 +392,7 @@ class InsertEntryTests(unittest.TestCase):
self.assertTrue(all(new is old for new, old in zip(patched[1:], self.files)))
def test_the_same_path_with_a_new_mtime_moves(self):
patched = insert_entry(self.files, (PurePath("/out/a.png"), 35.0), 100)
patched = insert_entry(self.files, (PurePosixPath("/out/a.png"), 35.0), 100)
self.assertEqual(
[(str(path), mtime) for path, mtime in patched],
[("/out/a.png", 35.0), ("/out/c.png", 30.0), ("/out/b.png", 20.0)],
@@ -394,7 +400,7 @@ class InsertEntryTests(unittest.TestCase):
def test_an_entry_already_listed_returns_the_listing_itself(self):
self.assertIs(
insert_entry(self.files, (PurePath("/out/b.png"), 20.0), 3), self.files
insert_entry(self.files, (PurePosixPath("/out/b.png"), 20.0), 3), self.files
)
def test_a_tie_in_mtime_is_not_placed(self):