diff --git a/TESTING.md b/TESTING.md index 57f32c7e..72c4a82e 100644 --- a/TESTING.md +++ b/TESTING.md @@ -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 diff --git a/py/api/images.py b/py/api/images.py index 6cf3f6c0..d7e5094d 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -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 diff --git a/tests/test_api_images.py b/tests/test_api_images.py index 1b405bee..1f3b20b2 100644 --- a/tests/test_api_images.py +++ b/tests/test_api_images.py @@ -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 diff --git a/tests/test_gallery_listing.py b/tests/test_gallery_listing.py index 8c4cb02a..ca796bfe 100644 --- a/tests/test_gallery_listing.py +++ b/tests/test_gallery_listing.py @@ -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):