diff --git a/py/config.py b/py/config.py index eb5a1828..3769df20 100644 --- a/py/config.py +++ b/py/config.py @@ -4,7 +4,6 @@ # import cycles. Breaking them would require an architectural refactor. import os import platform -import posixpath import threading from pathlib import Path import folder_paths # pyright: ignore[reportMissingImports] @@ -20,6 +19,7 @@ from .utils.cache_paths import CacheType, get_cache_file_path, get_legacy_cache_ from .utils.constants import ( OTHER_MODEL_FOLDER_SUBTYPES, ) +from .utils.root_paths import normalize_root_identity from .utils.settings_paths import ( ensure_settings_file, get_settings_dir, @@ -34,14 +34,15 @@ standalone_mode = ( logger = logging.getLogger(__name__) - -def _normalize_root_identity(path: str) -> str: - """Normalize a root path for comparisons across slash styles.""" - - normalized = posixpath.normpath(path.strip().replace("\\", "/")) - if len(normalized) >= 2 and normalized[1] == ":": - return normalized.lower() - return normalized +# Saved folder-path keys mapped to the model type whose cache can tell whether a +# root that is missing right now is still worth keeping (issue #1146). The +# checkpoint page holds both checkpoints and unet roots. +_CACHE_MODEL_TYPE_BY_FOLDER_KEY = { + "loras": "lora", + "checkpoints": "checkpoint", + "unet": "checkpoint", + "embeddings": "embedding", +} def _append_new_paths(current: List[str], candidates: Iterable[str]) -> List[str]: @@ -52,11 +53,11 @@ def _append_new_paths(current: List[str], candidates: Iterable[str]) -> List[str to the root *order* would move user data. """ merged = list(current) - seen = {_normalize_root_identity(path) for path in merged} + seen = {normalize_root_identity(path) for path in merged} for path in candidates: if not isinstance(path, str) or not path.strip(): continue - identity = _normalize_root_identity(path) + identity = normalize_root_identity(path) if identity in seen: continue seen.add(identity) @@ -78,15 +79,15 @@ def _resolve_valid_default_root( stripped = path.strip() if not stripped: continue - identity = _normalize_root_identity(stripped) + identity = normalize_root_identity(stripped) if identity in seen: continue seen.add(identity) fallback_paths.append(stripped) - allowed = {_normalize_root_identity(path) for path in fallback_paths} + allowed = {normalize_root_identity(path) for path in fallback_paths} - if current and _normalize_root_identity(current) in allowed: + if current and normalize_root_identity(current) in allowed: return current if not valid_paths: @@ -320,6 +321,77 @@ class Config: "Could not load extra paths from library settings: %s", exc ) + def _drop_missing_roots_without_cache( + self, + folder_paths_map: Dict[str, List[str]], + default_roots: Mapping[str, str], + ) -> Dict[str, List[str]]: + """Drop saved roots that neither exist nor hold cached entries (issue #1146). + + The startup sync persists what is *configured* rather than what is readable, + so a switched-off drive keeps its path (see ``save_folder_paths_to_settings``). + A directory the host registered without ever creating it -- ComfyUI adds + ``/loras`` and friends for its save nodes whether or not they exist -- + came along for the ride and stayed there as a permanently "unavailable" root. + + A root that still holds cached entries is exactly the switched-off drive the + persistence exists for, so it is kept. A root named by the library's + ``default_*_root`` is kept even when it is empty: dropping it would let the + next boot repair the default onto another root. + + Best effort: when the cache cannot be consulted, every root is kept. + """ + + try: + from .services.persistent_model_cache import get_persistent_cache + + cache = get_persistent_cache() + except Exception as exc: # pragma: no cover - defensive logging + logger.debug("Keeping every configured model root: %s", exc) + return folder_paths_map + + protected = { + identity + for identity in ( + normalize_root_identity(path) for path in default_roots.values() + ) + if identity + } + + filtered: Dict[str, List[str]] = {} + for key, paths in folder_paths_map.items(): + missing = [path for path in paths if not os.path.exists(path)] + if not missing: + filtered[key] = list(paths) + continue + + probe = [ + path + for path in missing + if normalize_root_identity(path) not in protected + ] + try: + cached = cache.roots_with_cached_entries( + _CACHE_MODEL_TYPE_BY_FOLDER_KEY.get(key, "other"), probe + ) + except Exception as exc: + logger.debug( + "Keeping every configured %s root (%s): %s", key, missing, exc + ) + filtered[key] = list(paths) + continue + + cached_ids = {normalize_root_identity(path) for path in cached} + filtered[key] = [ + path + for path in paths + if os.path.exists(path) + or normalize_root_identity(path) in protected + or normalize_root_identity(path) in cached_ids + ] + + return filtered + def save_folder_paths_to_settings(self): """Persist ComfyUI-derived folder paths to the multi-library settings.""" try: @@ -533,9 +605,26 @@ class Config: or self._should_activate_comfy_library(libraries, libraries_changed) ) + # A root that the host registered but never created (and that holds no + # cached models) is not a library path: persisting it would bring it + # back as an unavailable root on every boot, and in standalone mode the + # saved paths are the only source of truth (issue #1146). + default_roots = { + "loras": default_lora_root + or comfy_library.get("default_lora_root", ""), + "checkpoints": default_checkpoint_root + or comfy_library.get("default_checkpoint_root", ""), + "unet": comfy_library.get("default_unet_root", ""), + "embeddings": default_embedding_root + or comfy_library.get("default_embedding_root", ""), + } + persisted_folder_paths = self._drop_missing_roots_without_cache( + target_folder_paths, default_roots + ) + settings_service.upsert_library( "comfyui", - folder_paths=target_folder_paths, + folder_paths=persisted_folder_paths, extra_folder_paths=extra_folder_paths, default_lora_root=default_lora_root, default_checkpoint_root=default_checkpoint_root, diff --git a/py/routes/handlers/model_handlers.py b/py/routes/handlers/model_handlers.py index ff9730f7..f7a81633 100644 --- a/py/routes/handlers/model_handlers.py +++ b/py/routes/handlers/model_handlers.py @@ -55,6 +55,7 @@ from ...utils.civitai_utils import resolve_license_payload from ...utils.file_utils import calculate_sha256 from ...utils.metadata_manager import MetadataManager from ...utils.paid_access import is_early_access_deadline_active +from ...utils.root_paths import normalize_root_identity from ...utils.sidecar_paths import get_metadata_path from ...utils.url_utils import relative_root_prefix @@ -1170,7 +1171,18 @@ class ModelQueryHandler: if requested_roots: configured = self._service.get_model_roots() - unknown = [root for root in requested_roots if root not in configured] + # Compare by identity, not by string: callers may pass the host's + # own spelling of a root (backslashes or a differently cased drive + # letter on Windows, as settings.json and ComfyUI show it), which + # names the same directory the live list holds forward-slashed. + known_roots = { + normalize_root_identity(root) for root in configured + } + unknown = [ + root + for root in requested_roots + if normalize_root_identity(root) not in known_roots + ] if unknown: return web.json_response( {"error": "Unknown model root(s)", "roots": unknown}, status=400 diff --git a/py/services/model_scanner.py b/py/services/model_scanner.py index d70cd73b..eb40af79 100644 --- a/py/services/model_scanner.py +++ b/py/services/model_scanner.py @@ -25,6 +25,7 @@ from ..utils.recipes_paths import ( get_effective_recipes_dir, normalized_recipes_dir_key, ) +from ..utils.root_paths import normalize_root_identity from .model_cache import ModelCache from .model_hash_index import ModelHashIndex from .model_lifecycle_service import delete_model_artifacts, _require_path_in_library_roots @@ -203,16 +204,33 @@ def _root_display_labels(roots: Sequence[str]) -> Dict[str, str]: return {root: _truncate_label(label) for root, label in labels.items()} +def _root_identity(root: str) -> str: + """Comparable identity of a model root, folding the host's spelling (issue #1146). + + The live root list is forward-slashed while ``Config`` keeps the configured + paths exactly as the host handed them over -- ComfyUI's ``os.path.normpath()`` + output is all backslashes on Windows. Comparing the two as raw strings turns + one directory into two roots: a duplicated menu row, a bogus "unavailable" + report, and -- worse -- an unreachable prefix registered over a root the walk + just read, which stops deleted models from ever being pruned. + """ + return normalize_root_identity(root) + + def _normalized_root_prefix(root: str) -> str: """Forward-slash root prefix used to attribute cached paths to a root.""" - prefix = os.path.normpath(root).replace(os.sep, "/") - if not prefix.endswith("/"): + prefix = _root_identity(root) + if prefix and not prefix.endswith("/"): prefix += "/" - return prefix.lower() if _CASE_INSENSITIVE_PATHS else prefix + return prefix def _path_matches_prefix(path: str, prefix: str) -> bool: """Match a cached business path against a normalized scope prefix.""" + if not prefix: + # An empty prefix would match every path; callers filter empty roots, so + # keep it inert instead of turning it into a catch-all. + return False candidate = path.lower() if _CASE_INSENSITIVE_PATHS else path return candidate.startswith(prefix) @@ -332,7 +350,8 @@ def _count_cached_entries_for_prefixes( for path in cached_paths: candidate = path.lower() if case_insensitive else path for key, prefix in ordered: - if candidate.startswith(prefix): + # An empty prefix would claim every path; see _path_matches_prefix. + if prefix and candidate.startswith(prefix): counts[key] += 1 break return counts @@ -1621,8 +1640,9 @@ class ModelScanner: seen: Set[str] = set() for key in keys: for root in config.configured_roots_for(key): - if root and root not in seen: - seen.add(root) + identity = _root_identity(root) + if identity and identity not in seen: + seen.add(identity) roots.append(root) return roots @@ -1651,32 +1671,44 @@ class ModelScanner: roots: List[str] = [] seen: Set[str] = set() for root in self.get_model_roots(): - if not root or root in seen: + identity = _root_identity(root) + if not identity or identity in seen: continue - seen.add(root) + seen.add(identity) roots.append(root) # Configured roots that are missing from the live list: their directory # did not exist when it was built (a drive switched off at startup). - # They are reported so the menu offers a row in both states instead of - # hiding the root until the process is restarted. - unavailable = [ - root for root in self.get_configured_model_roots() if root not in seen - ] + # Compared by identity, never by string -- the live list is forward-slashed + # while Config keeps the host's own spelling, which is all backslashes on + # Windows (issue #1146). Only roots that still hold cached entries are + # reported: a path that neither exists nor was ever scanned has nothing to + # keep, and ComfyUI registers `/loras` for its save nodes whether or + # not that directory is there. + candidates: List[str] = [] + reported: Set[str] = set() + for root in self.get_configured_model_roots(): + identity = _root_identity(root) + if not identity or identity in seen or identity in reported: + continue + reported.add(identity) + candidates.append(root) - labels = _root_display_labels(roots + unavailable) cached_paths = ( {item.get('file_path', '') for item in self._cache.raw_data} if self._cache is not None else set() ) - counts = _count_cached_entries_per_root(cached_paths, roots + unavailable) + counts = _count_cached_entries_per_root(cached_paths, roots + candidates) + unavailable = [root for root in candidates if counts.get(root, 0)] + + labels = _root_display_labels(roots + unavailable) return [ { 'path': root.replace(os.sep, '/'), 'label': labels[root], 'reachable': os.path.exists(root), - 'available': root in seen, + 'available': _root_identity(root) in seen, 'models': counts.get(root, 0), } for root in roots + unavailable @@ -1801,25 +1833,37 @@ class ModelScanner: configured_roots: List[str] = [] seen_roots: Set[str] = set() for root_path in self.get_model_roots(): - if not root_path or root_path in seen_roots: + identity = _root_identity(root_path) + if not identity or identity in seen_roots: continue - seen_roots.add(root_path) + seen_roots.add(identity) configured_roots.append(root_path) # Configured roots that never made it into the live list (their # directory was missing when it was built). They are outside the scan # scope entirely — which is exactly why their cached entries survive # — so they only need reporting, and their labels must not collide - # with the live ones. - configured_only = [ - root for root in self.get_configured_model_roots() - if root and root not in seen_roots - ] + # with the live ones. Identity, not string: the live list is + # forward-slashed while Config keeps the host's own spelling -- all + # backslashes on Windows -- and a raw compare would report every root + # twice and shadow it as unreachable (issue #1146). + configured_only: List[str] = [] + for root in self.get_configured_model_roots(): + identity = _root_identity(root) + if not identity or identity in seen_roots: + continue + seen_roots.add(identity) + configured_only.append(root) all_labels = _root_display_labels(configured_roots + configured_only) + scope_root_ids = ( + {_root_identity(root) for root in scope.roots} + if scope is not None and scope.roots is not None + else None + ) scope_roots = [ root for root in configured_roots - if scope is None or scope.roots is None or root in scope.roots + if scope_root_ids is None or _root_identity(root) in scope_root_ids ] scope_folder = scope.folder if scope is not None else None @@ -1853,7 +1897,12 @@ class ModelScanner: ) for root_path, _prefix in folder_candidates: kept = folder_counts.get(root_path, 0) - if scope_folder is not None and not kept: + # Only a root that still holds cached entries is reported -- + # and protected from pruning. A configured path that neither + # exists nor was ever scanned has nothing to keep, and ComfyUI + # registers `/loras` for its save nodes even when that + # directory is absent (issue #1146). + if not kept: continue unreachable.add(root_path, 'root_unavailable', kept=kept) skipped_roots.append({ diff --git a/py/services/persistent_model_cache.py b/py/services/persistent_model_cache.py index 8d9eefc2..e7362f29 100644 --- a/py/services/persistent_model_cache.py +++ b/py/services/persistent_model_cache.py @@ -4,16 +4,33 @@ import os import sqlite3 import threading from dataclasses import dataclass, field -from typing import Any, Dict, List, Mapping, Optional, Sequence, Tuple +from typing import Any, Dict, List, Mapping, Optional, Sequence, Set, Tuple from ..utils.cache_db import connect_cache_db from ..utils.cache_paths import CacheType, resolve_cache_path_with_migration from ..utils.file_lock import exclusive_lock +from ..utils.root_paths import normalize_root_identity from .model_sources import normalize_metadata_source logger = logging.getLogger(__name__) +def _like_prefix_pattern(identity: str) -> str: + """SQLite LIKE pattern matching every path inside *identity*. + + ``%``/``_`` are legal characters in a path and ``\\`` is the escape character, + so all three are escaped. The separator is added only when the identity does not + already end in one, so a filesystem root (``/``) still matches everything below + it instead of only itself. + """ + + escaped = ( + identity.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_") + ) + prefix = escaped if escaped.endswith("/") else f"{escaped}/" + return f"{prefix}%" + + @dataclass class PersistedCacheData: """Lightweight structure returned by the persistent cache.""" @@ -254,6 +271,61 @@ class PersistentModelCache: all_folders=all_folders, ) + def roots_with_cached_entries( + self, model_type: str, roots: Sequence[str] + ) -> Set[str]: + """Return the subset of *roots* that hold at least one cached entry. + + Used by the settings sync to tell a root that was registered but never + scanned -- ComfyUI registers ``/loras`` for its save nodes whether + or not that directory exists -- from a real root whose drive happens to be + switched off: only the latter must keep its place in the saved library + paths (issue #1146). Paths are matched on their forward-slash identity, so + the host's own slash style does not matter. + + Raises when the answer is unknown (a disabled cache, a missing database, an + unreadable file): the caller keeps every root in that case. + """ + + if not self.is_enabled(): + raise RuntimeError("persistent model cache is disabled") + + candidates: Dict[str, str] = {} + for root in roots: + identity = normalize_root_identity(root) + if identity: + candidates.setdefault(identity, root) + if not candidates: + return set() + + matched: Set[str] = set() + # One scan per model type rather than one per root: the probe runs at + # startup for every configured root that is missing right now, which on + # Windows includes all of ComfyUI's `/` folders. + clause = "replace(file_path, '\\', '/') LIKE ? ESCAPE '\\'" + sql = ( + "SELECT DISTINCT replace(file_path, '\\', '/') FROM models " + f"WHERE model_type = ? AND ({' OR '.join([clause] * len(candidates))})" + ) + params: List[Any] = [model_type] + params.extend(_like_prefix_pattern(identity) for identity in candidates) + + with self._db_lock: + conn = self._connect(readonly=True) + try: + rows = conn.execute(sql, params).fetchall() + finally: + conn.close() + + for (cached_path,) in rows: + folded = normalize_root_identity(cached_path) + for identity, root in candidates.items(): + prefix = identity if identity.endswith("/") else f"{identity}/" + if folded.startswith(prefix): + matched.add(root) + break + return matched + def save_cache(self, model_type: str, raw_data: Sequence[Dict[str, Any]], hash_index: Dict[str, List[str]], excluded_models: Sequence[str], autov3_hash_index: Optional[Dict[str, List[str]]] = None, all_folders: Optional[Sequence[str]] = None) -> None: if not self.is_enabled(): return diff --git a/py/services/settings_manager.py b/py/services/settings_manager.py index 21b71a12..c927c347 100644 --- a/py/services/settings_manager.py +++ b/py/services/settings_manager.py @@ -2,7 +2,6 @@ import asyncio import copy import json import os -import posixpath import shutil import tempfile import logging @@ -36,6 +35,7 @@ from ..utils.constants import ( normalize_other_sub_types, ) from ..utils.preview_selection import VALID_MATURE_BLUR_LEVELS +from ..utils.root_paths import normalize_root_identity from ..utils.settings_paths import ( APP_NAME, _portable_env_override, @@ -156,15 +156,6 @@ DEFAULT_SETTINGS: Dict[str, Any] = { } -def _normalize_root_identity(path: str) -> str: - """Normalize a root path for equality checks across slash styles.""" - - normalized = posixpath.normpath(path.strip().replace("\\", "/")) - if len(normalized) >= 2 and normalized[1] == ":": - return normalized.lower() - return normalized - - class SettingsManager: def __init__(self): self.settings_file = ensure_settings_file(logger) @@ -1058,7 +1049,7 @@ class SettingsManager: return False allowed_roots = self._get_allowed_roots(key) - if current and _normalize_root_identity(current) in allowed_roots: + if current and normalize_root_identity(current) in allowed_roots: return False self.settings[setting_key] = primary_candidates[0] @@ -1094,7 +1085,7 @@ class SettingsManager: candidate_identities: set[str] = set() for folder_key in OTHER_SUB_TYPE_FOLDER_KEYS.get(sub_type, []): for candidate in self._get_valid_root_candidates(folder_key): - identity = _normalize_root_identity(candidate) + identity = normalize_root_identity(candidate) if identity in candidate_identities: continue candidate_identities.add(identity) @@ -1102,7 +1093,7 @@ class SettingsManager: if not candidates: continue current = other_roots.get(sub_type, "") - if current and _normalize_root_identity(current) in candidate_identities: + if current and normalize_root_identity(current) in candidate_identities: continue other_roots[sub_type] = candidates[0] if current: @@ -1152,7 +1143,7 @@ class SettingsManager: normalized = value.strip() if not normalized: continue - identity = _normalize_root_identity(normalized) + identity = normalize_root_identity(normalized) if identity in seen: continue seen.add(identity) @@ -1162,7 +1153,7 @@ class SettingsManager: def _get_allowed_roots(self, key: str) -> set[str]: """Return all valid roots for a model type, including extra roots.""" - return {_normalize_root_identity(path) for path in self._get_valid_root_candidates(key)} + return {normalize_root_identity(path) for path in self._get_valid_root_candidates(key)} def _check_environment_variables(self) -> None: """Check for environment variables and update settings if needed""" diff --git a/py/utils/root_paths.py b/py/utils/root_paths.py new file mode 100644 index 00000000..8cbe80ab --- /dev/null +++ b/py/utils/root_paths.py @@ -0,0 +1,36 @@ +"""Path identity helpers for model roots. + +ComfyUI hands its model folders over in whatever spelling the host uses -- +``os.path.normpath()`` output, which is all backslashes on Windows -- while LoRA +Manager's own root lists and cached model paths are forward-slashed. Comparing +the two spellings as raw strings makes one directory look like two roots, so every +root identity check goes through :func:`normalize_root_identity` (issue #1146). +""" + +from __future__ import annotations + +import os +import posixpath + + +def normalize_root_identity(path: str) -> str: + """Return the comparable identity of a model root path. + + Folds the separator style (``\\`` and ``/`` are both accepted as separators, + because the host may spell a Windows path either way), redundant and trailing + separators, and the case where the filesystem does not distinguish it: Windows + compares whole paths case-insensitively, and every platform folds a drive + letter (``E:`` == ``e:``). Symlinks are never resolved -- business paths stay + business paths, matching the rest of the codebase. + """ + + if not isinstance(path, str): + return "" + candidate = path.strip() + if not candidate: + return "" + + normalized = posixpath.normpath(candidate.replace("\\", "/")) + if os.name == "nt" or (len(normalized) >= 2 and normalized[1] == ":"): + return normalized.lower() + return normalized diff --git a/tests/config/test_config_save_paths.py b/tests/config/test_config_save_paths.py index 599c3099..8437988b 100644 --- a/tests/config/test_config_save_paths.py +++ b/tests/config/test_config_save_paths.py @@ -968,3 +968,197 @@ def test_save_paths_keeps_configured_root_of_a_switched_off_drive( assert config_instance.loras_roots == [readable.replace("\\", "/")] # The user's default root survives because it is merely unavailable. assert fake_settings.payload["default_lora_root"] == switched_off.replace("\\", "/") + + +# --- roots the host registered but never created (issue #1146) -------------- + + +class _FakePersistentCache: + """Stand-in for PersistentModelCache in the startup sync.""" + + def __init__(self, cached_roots=None, error: Exception | None = None): + self._cached_roots = cached_roots or {} + self._error = error + self.queries: List[tuple] = [] + + def roots_with_cached_entries(self, model_type: str, roots): + self.queries.append((model_type, list(roots))) + if self._error is not None: + raise self._error + known = {path.replace("\\", "/") for path in self._cached_roots.get(model_type, [])} + return { + root for root in roots if root.replace("\\", "/") in known + } + + +def _stub_persistent_cache(monkeypatch: pytest.MonkeyPatch, cache) -> None: + from py.services import persistent_model_cache as persistent_model_cache_module + + monkeypatch.setattr( + persistent_model_cache_module, "get_persistent_cache", lambda: cache + ) + + +class _LibrarySettingsService: + """Minimal settings service recording what the startup sync persists.""" + + active_library = "comfyui" + + def __init__(self, library: Dict[str, Any]): + self._library = library + self.payload: Dict[str, Any] = {} + + def get_libraries(self): + return {"comfyui": self._library} + + def rename_library(self, *_): # pragma: no cover - defensive guard + raise AssertionError("rename_library should not be invoked") + + def delete_library(self, *_): # pragma: no cover - defensive guard + raise AssertionError("delete_library should not be invoked") + + def get_active_library_name(self): + return self.active_library + + def upsert_library(self, name: str, **payload): + self.payload = payload + + +def test_save_paths_drops_registered_directories_that_never_existed( + monkeypatch: pytest.MonkeyPatch, tmp_path +): + """ComfyUI registers `/` for its save nodes unconditionally. + + Those directories are not library paths when they do not exist and hold no + cached models: persisting them brings them back as "unavailable" roots on + every boot. + """ + folder_paths = _setup_config_environment(monkeypatch, tmp_path) + unet_dir = tmp_path / "unet" + unet_dir.mkdir() + folder_paths["unet"] = [str(unet_dir)] + output_loras = str(tmp_path / "output" / "loras") + output_checkpoints = str(tmp_path / "output" / "checkpoints") + output_unet = str(tmp_path / "output" / "diffusion_models") + output_embeddings = str(tmp_path / "output" / "embeddings") + folder_paths["loras"].append(output_loras) + folder_paths["checkpoints"].append(output_checkpoints) + folder_paths["unet"].append(output_unet) + folder_paths["embeddings"].append(output_embeddings) + + fake_settings = _LibrarySettingsService( + { + "folder_paths": {key: list(value) for key, value in folder_paths.items()}, + "default_lora_root": folder_paths["loras"][0], + "default_checkpoint_root": folder_paths["checkpoints"][0], + "default_embedding_root": folder_paths["embeddings"][0], + } + ) + monkeypatch.setattr(settings_manager_module, "settings", fake_settings) + cache = _FakePersistentCache() + _stub_persistent_cache(monkeypatch, cache) + + config_module.Config() + + persisted = fake_settings.payload["folder_paths"] + for key, never_created in ( + ("loras", output_loras), + ("checkpoints", output_checkpoints), + ("unet", output_unet), + ("embeddings", output_embeddings), + ): + assert never_created.replace("\\", "/") not in persisted[key], key + # The real root of that key is untouched. + assert folder_paths[key][0].replace("\\", "/") in persisted[key], key + # Lora and checkpoint roots are looked up in their own model type. + assert [model_type for model_type, _roots in cache.queries] == [ + "lora", + "checkpoint", + "checkpoint", + "embedding", + ] + + +def test_save_paths_keeps_an_unavailable_root_that_has_cached_models( + monkeypatch: pytest.MonkeyPatch, tmp_path +): + """A switched-off drive keeps its path: its models are still cached.""" + folder_paths = _setup_config_environment(monkeypatch, tmp_path) + readable = folder_paths["loras"][0] + switched_off = str(tmp_path / "drive-Z" / "loras") + folder_paths["loras"] = [readable, switched_off] + + fake_settings = _LibrarySettingsService( + { + "folder_paths": {"loras": [readable]}, + "default_lora_root": readable, + } + ) + monkeypatch.setattr(settings_manager_module, "settings", fake_settings) + _stub_persistent_cache( + monkeypatch, _FakePersistentCache({"lora": [switched_off]}) + ) + + config_module.Config() + + persisted = fake_settings.payload["folder_paths"]["loras"] + assert switched_off.replace("\\", "/") in persisted + assert readable.replace("\\", "/") in persisted + + +def test_save_paths_keeps_every_root_when_the_cache_cannot_be_read( + monkeypatch: pytest.MonkeyPatch, tmp_path +): + """No answer from the cache means no root is dropped.""" + folder_paths = _setup_config_environment(monkeypatch, tmp_path) + readable = folder_paths["loras"][0] + unknown = str(tmp_path / "drive-Z" / "loras") + folder_paths["loras"] = [readable, unknown] + + fake_settings = _LibrarySettingsService( + { + "folder_paths": {"loras": [readable]}, + "default_lora_root": readable, + } + ) + monkeypatch.setattr(settings_manager_module, "settings", fake_settings) + _stub_persistent_cache( + monkeypatch, _FakePersistentCache(error=FileNotFoundError("no cache")) + ) + + config_module.Config() + + persisted = fake_settings.payload["folder_paths"]["loras"] + assert unknown.replace("\\", "/") in persisted + + +def test_save_paths_keeps_the_root_a_default_setting_points_at( + monkeypatch: pytest.MonkeyPatch, tmp_path +): + """An empty unavailable default root must not be dropped. + + Dropping it would let the next boot "repair" the default onto another root and + silently retarget downloads. ``default_unet_root`` is only known from the + stored library (the sync resolves the lora/checkpoint/embedding ones itself), + so this covers the fallback to the stored value. + """ + folder_paths = _setup_config_environment(monkeypatch, tmp_path) + unet_dir = tmp_path / "unet" + unet_dir.mkdir() + default_target = str(tmp_path / "drive-Z" / "unet") + folder_paths["unet"] = [str(unet_dir), default_target] + + fake_settings = _LibrarySettingsService( + { + "folder_paths": {"unet": [str(unet_dir)]}, + "default_unet_root": default_target, + } + ) + monkeypatch.setattr(settings_manager_module, "settings", fake_settings) + _stub_persistent_cache(monkeypatch, _FakePersistentCache()) + + config_module.Config() + + persisted = fake_settings.payload["folder_paths"]["unet"] + assert default_target.replace("\\", "/") in persisted + assert str(unet_dir).replace("\\", "/") in persisted diff --git a/tests/routes/test_model_query_handler.py b/tests/routes/test_model_query_handler.py index 0e2cd134..0755213a 100644 --- a/tests/routes/test_model_query_handler.py +++ b/tests/routes/test_model_query_handler.py @@ -316,6 +316,32 @@ async def test_scan_models_rejects_unknown_root(): assert service.received_scope == "not-called" +@pytest.mark.asyncio +async def test_scan_models_accepts_a_root_in_the_hosts_own_spelling(): + """Windows callers may pass a root the way ComfyUI spells it (issue #1146). + + ``settings.json`` and the ComfyUI logs show ``E:\\loras`` while the live list + holds ``E:/loras``; both name the same directory, so a scoped scan must select + it instead of answering 400 "Unknown model root(s)". + """ + service = ScopedScanService( + roots=["E:/StableDiffusion/cc_models/loras"], summary=SUMMARY + ) + handler = ModelQueryHandler(service=service, logger=logging.getLogger(__name__)) + + for requested in ( + "E:\\StableDiffusion\\cc_models\\loras", + "e:/StableDiffusion/cc_models/loras", + ): + service.received_scope = "not-called" + response = await handler.scan_models( + SimpleNamespace(query=QueryParams({"roots": [requested]})) + ) + + assert response.status == 200, requested + assert service.received_scope.roots == (requested,) + + @pytest.mark.asyncio async def test_scan_models_rejects_roots_with_full_rebuild(): service = ScopedScanService(roots=["/mnt/a"], summary=SUMMARY) diff --git a/tests/services/test_model_scanner.py b/tests/services/test_model_scanner.py index d3e7c576..d69e1bac 100644 --- a/tests/services/test_model_scanner.py +++ b/tests/services/test_model_scanner.py @@ -2949,3 +2949,133 @@ def test_reconcile_walk_skips_the_recipes_dir(tmp_path: Path, monkeypatch): assert "keep" in result.discovered_folders assert "recipes" not in result.discovered_folders assert "recipes/sub" not in result.discovered_folders + + +# --- the host's path spelling (issue #1146) --------------------------------- + + +def _windows_spelling(path: Path) -> str: + """Return a path the way ComfyUI hands it over on Windows. + + ``utils/extra_config.py`` normalizes every configured model folder with + ``os.path.normpath``, so on Windows the configured list is all backslashes + while the live root list is forward-slashed. + """ + return str(path).replace("/", "\\") + + +async def _windows_spelling_scanner(tmp_path: Path, monkeypatch): + """Scanner configured with backslash roots plus a never-created output dir. + + ``/loras`` is what ComfyUI registers for its save nodes whether or not + the directory exists (``main.py`` calls ``add_model_folder_path`` with it + unconditionally). + """ + primary = tmp_path / "primary" + (primary / "pack").mkdir(parents=True) + (primary / "pack" / "a.txt").write_text("a", encoding="utf-8") + never_created = tmp_path / "output" / "loras" + + scanner = MultiRootDummyScanner([primary]) + await scanner._initialize_cache() + monkeypatch.setattr( + scanner, + "get_configured_model_roots", + lambda: [_windows_spelling(primary), _windows_spelling(never_created)], + ) + return scanner, primary, never_created + + +@pytest.mark.asyncio +async def test_describe_roots_folds_the_host_path_spelling(tmp_path: Path, monkeypatch): + """One directory is one root, whatever separator the host used.""" + scanner, primary, _never_created = await _windows_spelling_scanner( + tmp_path, monkeypatch + ) + + described = scanner.describe_model_roots() + + assert [entry["path"] for entry in described] == [_normalize_path(primary)] + assert described[0]["available"] is True + assert described[0]["reachable"] is True + assert described[0]["models"] == 1 + + +@pytest.mark.asyncio +async def test_reconcile_does_not_shadow_a_backslash_configured_root( + tmp_path: Path, monkeypatch +): + """A configured twin must not register its live root as unreachable. + + ``_UnreachablePaths`` normalizes the prefixes it is given, so the backslash + spelling of a root the walk just read normalized to the same prefix: every + cached entry under it counted as "kept", which is what made the refresh toast + claim thousands of reachable models were inaccessible -- and what stopped + deleted models from ever being pruned. + """ + scanner, primary, _never_created = await _windows_spelling_scanner( + tmp_path, monkeypatch + ) + (primary / "pack" / "a.txt").unlink() + + summary = await scanner._reconcile_cache() + + assert summary is not None + assert summary["skipped_roots"] == [] + assert summary["unavailable_paths"] == [] + assert summary["kept_unreachable"] == 0 + assert summary["removed"] == 1 + assert scanner._cache.raw_data == [] + + +@pytest.mark.asyncio +async def test_root_that_never_existed_is_not_reported(tmp_path: Path, monkeypatch): + """A registered directory that neither exists nor holds cache is not a root. + + Only roots the user really configured and whose models are cached are worth an + offline row or a "models kept" notice. + """ + primary = tmp_path / "primary" + (primary / "pack").mkdir(parents=True) + (primary / "pack" / "a.txt").write_text("a", encoding="utf-8") + never_created = tmp_path / "output" / "loras" + + scanner = MultiRootDummyScanner([primary]) + await scanner._initialize_cache() + monkeypatch.setattr( + scanner, + "get_configured_model_roots", + lambda: [str(primary), str(never_created)], + ) + + assert [entry["path"] for entry in scanner.describe_model_roots()] == [ + _normalize_path(primary) + ] + + summary = await scanner._reconcile_cache() + + assert summary is not None + assert summary["skipped_roots"] == [] + assert summary["kept_unreachable"] == 0 + + +@pytest.mark.asyncio +async def test_scoped_scan_accepts_the_host_path_spelling(tmp_path: Path, monkeypatch): + """A caller may name a root the way the host spells it (issue #1146). + + The Refresh menu sends back the path `/roots` rendered, but the browser + extension and scripts can pass what settings.json holds -- backslashes on + Windows -- and that must select the root rather than scan nothing. + """ + primary = tmp_path / "primary" + (primary / "pack").mkdir(parents=True) + (primary / "pack" / "a.txt").write_text("a", encoding="utf-8") + + scanner = MultiRootDummyScanner([primary]) + await scanner._initialize_cache() + + scope = ReconcileScope(roots=(_windows_spelling(primary),)) + summary = await scanner._reconcile_cache(scope=scope) + + assert summary is not None + assert summary["scanned_roots"] == [primary.name] diff --git a/tests/services/test_persistent_model_cache.py b/tests/services/test_persistent_model_cache.py index b48b2af0..d45a2a0d 100644 --- a/tests/services/test_persistent_model_cache.py +++ b/tests/services/test_persistent_model_cache.py @@ -451,3 +451,100 @@ def test_get_models_missing_autov3_self_terminates_after_marking(tmp_path: Path, store.update_single_model('dummy', new_item, old_item=old_item) assert store.get_models_missing_autov3('dummy') == [] + + +# --- which roots still hold cached entries (issue #1146) -------------------- + + +def _cache_with_paths(tmp_path: Path, monkeypatch, entries) -> PersistentModelCache: + """Build a cache holding ``entries`` (``(model_type, file_path)`` pairs).""" + + import sqlite3 + + monkeypatch.setenv('LORA_MANAGER_DISABLE_PERSISTENT_CACHE', '0') + db_path = tmp_path / 'roots.sqlite' + store = PersistentModelCache(db_path=str(db_path)) + with sqlite3.connect(str(db_path)) as conn: + conn.executemany( + "INSERT INTO models (model_type, file_path) VALUES (?, ?)", entries + ) + conn.commit() + return store + + +def test_roots_with_cached_entries_matches_the_hosts_slash_style( + tmp_path: Path, monkeypatch +) -> None: + """The configured root may be spelled with backslashes (Windows).""" + + store = _cache_with_paths( + tmp_path, + monkeypatch, + [("lora", "E:/StableDiffusion/cc_models/loras/pack/a.safetensors")], + ) + + matched = store.roots_with_cached_entries( + "lora", ["E:\\StableDiffusion\\cc_models\\loras", "E:\\empty\\loras"] + ) + + assert matched == {"E:\\StableDiffusion\\cc_models\\loras"} + + +def test_roots_with_cached_entries_compares_whole_path_segments( + tmp_path: Path, monkeypatch +) -> None: + """``loras-extra`` and ``loras2`` are not inside ``loras``.""" + + store = _cache_with_paths( + tmp_path, + monkeypatch, + [ + ("lora", "E:/loras-extra/a.safetensors"), + ("lora", "E:/loras2/b.safetensors"), + ("checkpoint", "E:/loras/c.safetensors"), + ], + ) + + assert store.roots_with_cached_entries("lora", ["E:/loras"]) == set() + assert store.roots_with_cached_entries("checkpoint", ["E:/loras"]) == {"E:/loras"} + + +def test_roots_with_cached_entries_escapes_like_wildcards( + tmp_path: Path, monkeypatch +) -> None: + """A directory name may contain ``_``/``%`` without becoming a wildcard.""" + + store = _cache_with_paths( + tmp_path, + monkeypatch, + [("lora", "E:/myXloras/a.safetensors"), ("lora", "E:/100%/a.safetensors")], + ) + + assert store.roots_with_cached_entries("lora", ["E:/my_loras"]) == set() + assert store.roots_with_cached_entries("lora", ["E:/100x"]) == set() + assert store.roots_with_cached_entries("lora", ["E:/100%"]) == {"E:/100%"} + + +def test_roots_with_cached_entries_raises_when_unknown( + tmp_path: Path, monkeypatch +) -> None: + """No cache, no answer: the caller must not drop roots on a guess.""" + + monkeypatch.setenv('LORA_MANAGER_DISABLE_PERSISTENT_CACHE', '1') + disabled = PersistentModelCache(db_path=str(tmp_path / 'disabled.sqlite')) + with pytest.raises(RuntimeError): + disabled.roots_with_cached_entries("lora", ["E:/loras"]) + + monkeypatch.setenv('LORA_MANAGER_DISABLE_PERSISTENT_CACHE', '0') + missing = PersistentModelCache(db_path=str(tmp_path / 'missing.sqlite')) + (tmp_path / 'missing.sqlite').unlink() + with pytest.raises(FileNotFoundError): + missing.roots_with_cached_entries("lora", ["E:/loras"]) + + +def test_roots_with_cached_entries_without_candidates(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv('LORA_MANAGER_DISABLE_PERSISTENT_CACHE', '0') + store = PersistentModelCache(db_path=str(tmp_path / 'empty.sqlite')) + + assert store.roots_with_cached_entries("lora", []) == set() + assert store.roots_with_cached_entries("lora", ["", " "]) == set() diff --git a/tests/utils/test_root_paths.py b/tests/utils/test_root_paths.py new file mode 100644 index 00000000..d6f06967 --- /dev/null +++ b/tests/utils/test_root_paths.py @@ -0,0 +1,69 @@ +import os + +import pytest + +from py.utils.root_paths import normalize_root_identity + + +def test_folds_windows_and_posix_separators(): + """ComfyUI's normpath output and LoRA Manager's forward slashes agree.""" + + assert normalize_root_identity("E:\\StableDiffusion\\cc_models\\loras") == ( + normalize_root_identity("E:/StableDiffusion/cc_models/loras") + ) + + +def test_folds_drive_letter_case_on_every_platform(): + """A drive letter is case-insensitive even where the filesystem is not.""" + + assert normalize_root_identity("E:/models/loras") == normalize_root_identity( + "e:/models/loras" + ) + assert normalize_root_identity("E:\\models\\loras") == normalize_root_identity( + "e:/MODELS/loras" + ) + + +def test_folds_trailing_and_redundant_separators(): + assert normalize_root_identity("E:/models/loras/") == normalize_root_identity( + "E:/models/loras" + ) + assert normalize_root_identity("E:/models/other/../loras") == ( + normalize_root_identity("E:/models/loras") + ) + + +def test_keeps_posix_case_significant(): + """Only Windows compares whole paths case-insensitively.""" + + if os.name == "nt": + assert normalize_root_identity("C:/Models/Loras") == ( + normalize_root_identity("c:/models/loras") + ) + else: + assert normalize_root_identity("/data/Loras") != normalize_root_identity( + "/data/loras" + ) + + +def test_folds_whole_path_case_on_windows(monkeypatch): + """The Windows branch is exercised on every platform.""" + + monkeypatch.setattr(os, "name", "nt") + + assert normalize_root_identity("C:/Models/Loras") == normalize_root_identity( + "c:/models/loras" + ) + assert normalize_root_identity("C:\\Models\\Loras\\") == "c:/models/loras" + + +def test_never_resolves_symlinks_or_abspath(): + """Business paths stay business paths: only the spelling is folded.""" + + assert normalize_root_identity("./models/loras") == "models/loras" + assert normalize_root_identity("/srv/link/loras") == "/srv/link/loras" + + +@pytest.mark.parametrize("value", ["", " ", None, 42]) +def test_blank_inputs_have_no_identity(value): + assert normalize_root_identity(value) == ""