From 88e44bb068f5c5a56e34cd180aa6c14bb513b1d3 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Mon, 26 Oct 2026 22:46:53 -0700 Subject: [PATCH] fix(lora): treat an offline Windows share as unvalidated and make cache tests portable (#362) Review findings on phase 1: Windows raises FileNotFoundError with winerror 53/67 for an offline share, which is a temporary failure, not a missing file. Test times are whole milliseconds (NTFS keeps 100 ns steps) and a file system that rounds them skips; cleanup no longer ignores errors; tests added for ValueError, NotADirectoryError, a failed reload, resolve failures and the watched-file set. --- py/lora_layout_cache.py | 9 ++- tests/test_lora_layout_cache.py | 103 ++++++++++++++++++++++++++++---- 2 files changed, 100 insertions(+), 12 deletions(-) diff --git a/py/lora_layout_cache.py b/py/lora_layout_cache.py index aeceb55b..944ac946 100644 --- a/py/lora_layout_cache.py +++ b/py/lora_layout_cache.py @@ -46,6 +46,9 @@ MAX_AGE_SECONDS = 30 # The signature of a watched file that does not exist (a valid state). MISSING = object() +# ERROR_BAD_NETPATH and ERROR_BAD_NET_NAME, which Windows raises as +# FileNotFoundError. +_OFFLINE_SHARE_ERRORS = frozenset({53, 67}) logger = logging.getLogger("prompt_manager.lora_layout_cache") @@ -160,7 +163,11 @@ class LayoutCache: def _signature(self, path: str): try: result = self._stat(path) - except (FileNotFoundError, NotADirectoryError): + except (FileNotFoundError, NotADirectoryError) as error: + # Windows reports an offline share as "not found": that is a + # temporary failure, not a missing file. + if getattr(error, "winerror", None) in _OFFLINE_SHARE_ERRORS: + raise _CannotValidate(path) from error return MISSING except (OSError, ValueError) as error: raise _CannotValidate(path) from error diff --git a/tests/test_lora_layout_cache.py b/tests/test_lora_layout_cache.py index 510c0776..fdfd0c01 100644 --- a/tests/test_lora_layout_cache.py +++ b/tests/test_lora_layout_cache.py @@ -31,6 +31,10 @@ from py.lora_layout_cache import ( from py.lora_manager_layout import UNREADABLE, LoraManagerLayout, _canonical SECOND_NS = 10**9 +# NTFS keeps modification times in 100 ns steps and FAT in 2 s steps: every +# time a test sets is a whole millisecond, and a test whose file system rounds +# it anyway skips (``set_mtime``). +TICK_NS = 10**6 def make_layout(settings_path=None, media_roots=(), error=None, source="user config"): @@ -52,7 +56,7 @@ class CacheTestCase(unittest.TestCase): def setUp(self): self.tmp = Path(tempfile.mkdtemp()).resolve() - self.addCleanup(shutil.rmtree, self.tmp, True) + self.addCleanup(shutil.rmtree, self.tmp) self.install_dir = self.tmp / "custom_nodes" / "ComfyUI-Lora-Manager" self.install_dir.mkdir(parents=True) self.plugin_settings = self.install_dir / "settings.json" @@ -60,9 +64,9 @@ class CacheTestCase(unittest.TestCase): self.settings.parent.mkdir() self.root_dir = self.tmp / "loras" self.root_dir.mkdir() - # The wall clock is an hour past every file written here, so nothing - # is racy unless a test sets a recent modification time. - self.now_ns = time.time_ns() + 3600 * SECOND_NS + # Files are written ten minutes before the injected wall clock, so + # nothing is racy unless a test sets a recent modification time. + self.now_ns = time.time_ns() // TICK_NS * TICK_NS self.mono = 1000.0 self.resolved = [] self.loaded = [] @@ -76,7 +80,12 @@ class CacheTestCase(unittest.TestCase): def write(self, path, text, mtime_ns=None): Path(path).write_text(text, encoding="utf-8") stamp = mtime_ns if mtime_ns is not None else self.now_ns - 600 * SECOND_NS + self.set_mtime(path, stamp) + + def set_mtime(self, path, stamp): os.utime(path, ns=(stamp, stamp)) + if os.stat(path).st_mtime_ns != stamp: + self.skipTest("this file system rounds modification times") def resolve(self, key): self.resolved.append(key) @@ -117,8 +126,7 @@ class ValidationTests(CacheTestCase): def test_settings_mtime_change_reloads_once(self): cache = self.cache() cache.get("") - stamp = self.settings.stat().st_mtime_ns + 1 - os.utime(self.settings, ns=(stamp, stamp)) + self.set_mtime(self.settings, self.settings.stat().st_mtime_ns + TICK_NS) cache.get("") cache.get("") self.assertEqual(len(self.loaded), 2) @@ -176,13 +184,13 @@ class ValidationTests(CacheTestCase): self.assertEqual(len(self.loaded), 3) def test_racy_boundary(self): - # Exactly RACY_SECONDS old is trusted; one nanosecond newer is not. + # Exactly RACY_SECONDS old is trusted; anything newer is not. boundary = self.now_ns - RACY_SECONDS * SECOND_NS - self.write(self.settings, "{}", mtime_ns=boundary + 1) + self.write(self.settings, "{}", mtime_ns=boundary + TICK_NS) cache = self.cache() cache.get("") self.assertIsNone(cache._entry) - os.utime(self.settings, ns=(boundary, boundary)) + self.set_mtime(self.settings, boundary) cache.get("") self.assertIsNotNone(cache._entry) @@ -294,15 +302,86 @@ class ValidationTests(CacheTestCase): cache.get("") self.assertEqual(len(self.loaded), 2) - def test_a_root_that_no_longer_resolves_reloads(self): + def test_a_root_whose_realpath_fails_reloads(self): cache = self.cache() cache.get("") with patch.object( - cache_module, "_canonical", side_effect=OSError("unreachable share") + cache_module, "_canonical", side_effect=OSError("realpath failed") ): cache.get("") self.assertEqual(len(self.loaded), 2) + def test_a_stat_value_error_cannot_validate(self): + def stat(path): + raise ValueError("embedded null character in path") + + cache = self.cache(stat=stat) + cache.get("") + self.assertIsNone(cache._entry) + + def test_not_a_directory_is_missing(self): + def stat(path): + raise NotADirectoryError(20, "Not a directory", path) + + cache = self.cache(stat=stat) + cache.get("") + self.assertTrue( + all(value is MISSING for _path, value in cache._entry.signatures) + ) + cache.get("") + self.assertEqual(len(self.loaded), 1) + + def test_an_offline_windows_share_cannot_validate(self): + def stat(path): + error = FileNotFoundError(2, "The network path was not found", path) + error.winerror = 53 + raise error + + cache = self.cache(stat=stat) + cache.get("") + self.assertIsNone(cache._entry) + + def test_a_failed_reload_leaves_no_stale_entry(self): + failing = {"on": False} + + def load(install): + self.loaded.append(install) + if failing["on"]: + raise RuntimeError("boom") + return self.layout + + cache = self.cache(load=load) + cache.get("") + self.mono += MAX_AGE_SECONDS + failing["on"] = True + with self.assertRaises(RuntimeError): + cache.get("") + self.assertIsNone(cache._entry) + + def test_a_resolve_failure_propagates_and_stores_nothing(self): + def resolve(key): + raise RuntimeError("boom") + + cache = self.cache(resolve=resolve) + with self.assertRaises(RuntimeError): + cache.get("") + self.assertIsNone(cache._entry) + + def test_without_a_settings_path_only_the_plugin_file_is_watched(self): + self.layout = make_layout(media_roots=self.layout.media_roots) + cache = self.cache() + cache.get("") + self.assertEqual( + [path for path, _sig in cache._entry.signatures], + [os.path.join(self.install, "settings.json")], + ) + + def test_portable_settings_are_watched_once(self): + self.layout = make_layout(settings_path=self.install_dir / "settings.json") + cache = self.cache() + cache.get("") + self.assertEqual(len(cache._entry.signatures), 1) + def test_a_load_failure_propagates_and_stores_nothing(self): calls = [] @@ -345,6 +424,8 @@ class ValidationTests(CacheTestCase): return SimpleNamespace(st_mtime_ns=self.now_ns - 600 * SECOND_NS, st_size=2) install = _canonical("C:\\ComfyUI\\custom_nodes\\LM", ntpath) + # The hit check needs ntpath's realpath to be idempotent here. + self.assertEqual(install, _canonical(install, ntpath)) self.layout = make_layout( settings_path=PureWindowsPath( "C:\\ComfyUI\\custom_nodes\\lm\\SETTINGS.JSON"