fix(scanner): compare model roots by identity, not by spelling (#1146)

ComfyUI hands its model folders over in the host's own spelling --
`utils/extra_config.py` runs every configured path through `os.path.normpath()`,
which is all backslashes on Windows, and `main.py` registers `<output>/loras`
unconditionally for its save nodes. `Config` kept that spelling verbatim while
`_prepare_*_paths()` normalized the live root lists to forward slashes, and the
scanner compared the two as raw strings. On Windows every configured root
therefore looked like a root that was not in the live list: the Refresh menu
listed each directory twice (the twin showing "0 models"), the completion toast
claimed reachable roots were "currently inaccessible" and kept their whole cache
count as `kept_unreachable`, and the settings sync persisted host-registered
directories that were never created.

The duplication was not only cosmetic. `_UnreachablePaths.add()` normalizes the
prefix it is given, so the configured twin of a live root registered exactly the
live root's prefix as unreadable: in the prune phase every cached-but-missing
entry under it matched and was "kept", so models deleted from disk were never
removed from the cache again.

* `py/utils/root_paths.py` holds the single `normalize_root_identity()`
  (separator style, redundant/trailing separators, and case where the filesystem
  folds it: whole paths on Windows, drive letters everywhere). `config.py` and
  `settings_manager.py` each carried a private copy and `model_scanner.py`
  compared raw strings.
* `ModelScanner` compares root identities in `get_configured_model_roots()`,
  `describe_model_roots()` (dedupe + `available`), the reconcile's
  `configured_only` list and the `ReconcileScope.roots` membership test, so one
  directory is one root whatever the host called it. `_normalized_root_prefix()`
  is built on the same identity, and an empty prefix no longer matches every
  path.
* A configured root that never made it into the live list is reported only while
  it still holds cached entries -- in the menu and in `skipped_roots` /
  `unavailable_paths` alike. A directory that neither exists nor was ever scanned
  has nothing to keep, so `<output>/loras` stops being an unavailable root.
* The startup sync drops saved roots that neither exist nor hold cached entries
  (`PersistentModelCache.roots_with_cached_entries()`, one scan per model type,
  separator-agnostic and wildcard-escaped). A root named by the library's
  `default_*_root` is kept even when empty: dropping it would let the next boot
  "repair" the default onto another root. When the cache cannot be consulted,
  every root is kept.
* `GET /scan?roots=` validates requested roots by identity too, so the browser
  extension or a script passing the spelling settings.json holds selects the root
  instead of answering 400 "Unknown model root(s)".

Verified: 8 of the new tests fail against the pre-fix source (checked with the
fix stashed); 3798 passed, 7 skipped, 11 deselected. The probe costs 71ms for a
100k-model library with seven missing roots (357ms when probing root by root).
This commit is contained in:
Will Miao
2026-10-12 08:43:06 +08:00
parent e056142669
commit 6d0d7cb15b
11 changed files with 822 additions and 57 deletions
+194
View File
@@ -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 `<output>/<type>` 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
+26
View File
@@ -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)
+130
View File
@@ -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.
``<output>/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]
@@ -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()
+69
View File
@@ -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) == ""