mirror of
https://github.com/willmiao/ComfyUI-Lora-Manager.git
synced 2026-10-07 18:12:12 -03:00
fix(scanner): keep folder records honest across the model roots
The recorded folder list is a union over the model roots keyed by relative path, but remove_known_folder() dropped an entry unconditionally. Deleting <rootA>/test in the sidebar therefore hid a "test" that <rootB> still held: the node disappeared from the next tree load and came back after the next scan, which reads as "the delete did not work". The same call purged cache entries by relative folder, so removing an empty <rootA>/test2 also evicted the model cards of <rootB>/test2 until the next scan, and rename_known_folder() re-keyed both the recorded folder and the "folder" field of models that never moved. * _folders_present_on_disk() answers "which of these relative folders does some root still hold?" with stats off the event loop (model roots can live on slow network shares) and reports nothing for stand-in scanners without roots, which preserves their previous behaviour. * remove_known_folder(folder, absolute_path=None) keeps the entries another root still owns and purges by the removed directory's absolute path when the caller knows it. Without a path the relative-folder filter stays as the documented fallback: it prunes the entry (disk-verified either way) but cannot tell same-named copies apart. * rename_known_folder() re-adds the survivors of the old name and only touches cache entries whose file_path sits inside the renamed directory. Tests: the two delete tests now remove the directory from disk first, which is what the contract always assumed, plus three new scanner tests (a twin keeps the entry, the entry goes once no root holds it, the twin's cards survive the purge), one for the legacy fallback and one for a rename that leaves a twin behind.
This commit is contained in:
+100
-13
@@ -1965,7 +1965,9 @@ class ModelScanner:
|
||||
await self._persist_current_cache()
|
||||
self.bump_cache_version()
|
||||
|
||||
async def remove_known_folder(self, folder: str) -> None:
|
||||
async def remove_known_folder(
|
||||
self, folder: str, absolute_path: Optional[str] = None
|
||||
) -> None:
|
||||
"""Forget a folder (and its subtree) that no longer exists on disk.
|
||||
|
||||
Counterpart of :meth:`add_known_folder`, called after a directory is
|
||||
@@ -1974,11 +1976,18 @@ class ModelScanner:
|
||||
full rescan. Ancestors are kept on purpose: every recorded ancestor
|
||||
exists on disk in its own right, so only the removed subtree is dropped.
|
||||
|
||||
Cache entries that referenced the now-missing directory are purged as
|
||||
well, which keeps a stale (phantom) model card from surviving the
|
||||
deletion. When ``all_folders`` has not been recorded yet (legacy
|
||||
snapshot) only the cache purge runs — the scheduled backfill walk
|
||||
rebuilds the folder list from disk.
|
||||
The recorded folder list is a *union over the model roots*, so an entry
|
||||
is dropped only when no root still holds that directory: deleting
|
||||
``<rootA>/test`` must not hide a ``test`` that ``<rootB>`` still has,
|
||||
which made the node vanish on the next tree load and reappear after the
|
||||
next scan. The cache purge is keyed on the removed directory's absolute
|
||||
path when the caller knows it (``absolute_path``); without that, a
|
||||
folder deleted in one root would evict the model cards of its
|
||||
same-named twin in another root.
|
||||
|
||||
When ``all_folders`` has not been recorded yet (legacy snapshot) only
|
||||
the cache purge runs — the scheduled backfill walk rebuilds the folder
|
||||
list from disk.
|
||||
"""
|
||||
normalized = folder.replace("\\", "/").strip("/")
|
||||
if not normalized:
|
||||
@@ -1992,20 +2001,23 @@ class ModelScanner:
|
||||
folders_changed = False
|
||||
recorded = getattr(cache, "all_folders", None)
|
||||
if recorded is not None:
|
||||
removed = [
|
||||
entry
|
||||
for entry in recorded
|
||||
if entry == normalized or entry.startswith(prefix)
|
||||
]
|
||||
still_present = await self._folders_present_on_disk(removed)
|
||||
updated = [
|
||||
entry
|
||||
for entry in recorded
|
||||
if entry != normalized and not entry.startswith(prefix)
|
||||
if entry in still_present
|
||||
or (entry != normalized and not entry.startswith(prefix))
|
||||
]
|
||||
if updated != list(recorded):
|
||||
cache.all_folders = updated
|
||||
folders_changed = True
|
||||
|
||||
stale_paths = [
|
||||
item.get("file_path")
|
||||
for item in (cache.raw_data or [])
|
||||
if self._folder_within(item.get("folder", ""), normalized)
|
||||
]
|
||||
stale_paths = self._folder_cache_purge_paths(cache, normalized, absolute_path)
|
||||
if stale_paths:
|
||||
# The purge persists the cache — including the already updated
|
||||
# all_folders list — and bumps the version itself.
|
||||
@@ -2017,6 +2029,63 @@ class ModelScanner:
|
||||
|
||||
self.bump_cache_version()
|
||||
|
||||
def _folder_cache_purge_paths(
|
||||
self, cache: "ModelCache", normalized: str, absolute_path: Optional[str]
|
||||
) -> List[str]:
|
||||
"""Cache entries that removing *normalized* invalidates.
|
||||
|
||||
With an absolute path the purge is exact: only models that lived inside
|
||||
the removed directory. Without one (legacy caller) the relative folder is
|
||||
the only handle available, which over-purges same-named folders in other
|
||||
roots and is therefore a fallback rather than the norm.
|
||||
"""
|
||||
if absolute_path:
|
||||
prefix = f"{str(absolute_path).replace(chr(92), '/').rstrip('/')}/"
|
||||
return [
|
||||
item.get("file_path")
|
||||
for item in (cache.raw_data or [])
|
||||
if str(item.get("file_path", "")).replace(chr(92), "/").startswith(prefix)
|
||||
]
|
||||
|
||||
return [
|
||||
item.get("file_path")
|
||||
for item in (cache.raw_data or [])
|
||||
if self._folder_within(item.get("folder", ""), normalized)
|
||||
]
|
||||
|
||||
async def _folders_present_on_disk(self, folders: Sequence[str]) -> Set[str]:
|
||||
"""Subset of *folders* that at least one model root still holds.
|
||||
|
||||
Folder records are relative, so "does this folder still exist?" is a
|
||||
question about every root at once. The check is stat-only and runs off
|
||||
the event loop because model roots can live on slow network shares.
|
||||
Stand-in scanners without roots report nothing, which preserves the
|
||||
caller's previous behaviour.
|
||||
"""
|
||||
candidates = [folder for folder in folders if folder]
|
||||
if not candidates:
|
||||
return set()
|
||||
|
||||
try:
|
||||
roots = self.get_model_roots()
|
||||
except NotImplementedError:
|
||||
return set()
|
||||
if not roots:
|
||||
return set()
|
||||
|
||||
return await asyncio.to_thread(self._folders_present_sync, candidates, roots)
|
||||
|
||||
@staticmethod
|
||||
def _folders_present_sync(folders: Sequence[str], roots: Sequence[str]) -> Set[str]:
|
||||
present: Set[str] = set()
|
||||
for folder in folders:
|
||||
for root in roots:
|
||||
if os.path.isdir(os.path.join(root, folder)):
|
||||
present.add(folder)
|
||||
break
|
||||
return present
|
||||
|
||||
|
||||
@staticmethod
|
||||
def _folder_within(candidate: str, target: str) -> bool:
|
||||
"""Return True when *candidate* is *target* or lives below it."""
|
||||
@@ -2103,6 +2172,17 @@ class ModelScanner:
|
||||
),
|
||||
key=lambda entry: entry.lower(),
|
||||
)
|
||||
# The recorded list is a union over the roots, and a same-named
|
||||
# folder in another root keeps the old name. Those entries still
|
||||
# exist on disk under the old relative path, so re-adding them is
|
||||
# what stops the rename from hiding the other root's twin.
|
||||
survivors = await self._folders_present_on_disk([
|
||||
entry
|
||||
for entry in recorded
|
||||
if entry == previous or entry.startswith(old_rel_prefix)
|
||||
])
|
||||
if survivors:
|
||||
rekeyed = sorted(set(rekeyed) | survivors, key=lambda entry: entry.lower())
|
||||
if rekeyed != list(recorded):
|
||||
cache.all_folders = rekeyed
|
||||
changed = True
|
||||
@@ -2119,13 +2199,20 @@ class ModelScanner:
|
||||
|
||||
touched: List[Dict[str, Any]] = []
|
||||
for item in cache.raw_data or []:
|
||||
old_file_path = item.get("file_path", "")
|
||||
normalized_old_path = str(old_file_path).replace(chr(92), "/")
|
||||
# Only records physically inside the renamed directory. Matching on
|
||||
# the relative folder alone would also re-key the same-named folder
|
||||
# in another root, whose files never moved.
|
||||
if old_file_path and not normalized_old_path.startswith(old_abs_prefix):
|
||||
continue
|
||||
|
||||
folder_value = item.get("folder", "") or self._calculate_folder(
|
||||
item.get("file_path", "")
|
||||
)
|
||||
if not self._folder_within(folder_value, previous):
|
||||
continue
|
||||
|
||||
old_file_path = item.get("file_path", "")
|
||||
if old_file_path:
|
||||
cache.remove_from_version_index(item)
|
||||
item["file_path"] = self._rekey_path(
|
||||
|
||||
@@ -3,6 +3,7 @@ from __future__ import annotations
|
||||
import asyncio
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
import sqlite3
|
||||
import threading
|
||||
import time
|
||||
@@ -146,6 +147,17 @@ def _stub_service_registry_getters(monkeypatch) -> None:
|
||||
monkeypatch.setattr(ServiceRegistry, "get_embedding_scanner", _none)
|
||||
|
||||
|
||||
class TwoRootScanner(DummyScanner):
|
||||
"""Scanner whose library spans two roots (primary + extra folder paths)."""
|
||||
|
||||
def __init__(self, primary: Path, extra: Path):
|
||||
super().__init__(primary)
|
||||
self._extra_root = str(extra)
|
||||
|
||||
def get_model_roots(self) -> List[str]:
|
||||
return [self._root, self._extra_root]
|
||||
|
||||
|
||||
def _create_files(root: Path) -> tuple[Path, Path, Path]:
|
||||
first = root / "one.txt"
|
||||
first.write_text("one", encoding="utf-8")
|
||||
@@ -1568,12 +1580,16 @@ async def test_add_known_folder_ignores_empty_input(tmp_path: Path):
|
||||
@pytest.mark.asyncio
|
||||
async def test_remove_known_folder_drops_subtree_and_keeps_ancestors(tmp_path: Path):
|
||||
_create_files(tmp_path)
|
||||
(tmp_path / "nested" / "deep" / "leaf").mkdir(parents=True)
|
||||
scanner = DummyScanner(tmp_path)
|
||||
await scanner._initialize_cache()
|
||||
cache = await scanner.get_cached_data()
|
||||
await scanner.add_known_folder("nested/deep/leaf")
|
||||
assert "nested/deep" in cache.all_folders
|
||||
|
||||
await scanner.remove_known_folder("nested/deep")
|
||||
# The delete the call reports has happened on disk.
|
||||
shutil.rmtree(tmp_path / "nested" / "deep")
|
||||
await scanner.remove_known_folder("nested/deep", str(tmp_path / "nested" / "deep"))
|
||||
|
||||
assert "nested/deep" not in cache.all_folders
|
||||
assert "nested/deep/leaf" not in cache.all_folders
|
||||
@@ -1589,7 +1605,8 @@ async def test_remove_known_folder_purges_stale_cache_entries(tmp_path: Path):
|
||||
cache = await scanner.get_cached_data()
|
||||
assert "nested" in cache.folders
|
||||
|
||||
await scanner.remove_known_folder("nested")
|
||||
shutil.rmtree(tmp_path / "nested")
|
||||
await scanner.remove_known_folder("nested", str(tmp_path / "nested"))
|
||||
|
||||
assert "nested" not in cache.all_folders
|
||||
assert "nested" not in cache.folders
|
||||
@@ -1598,6 +1615,102 @@ async def test_remove_known_folder_purges_stale_cache_entries(tmp_path: Path):
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_remove_known_folder_legacy_call_prunes_the_entry_and_over_purges(
|
||||
tmp_path: Path,
|
||||
):
|
||||
# Callers that only know the relative folder keep the old purge behaviour:
|
||||
# it matches by folder name, so a surviving twin in another root loses its
|
||||
# model cards until the next scan. That is exactly why the delete flow hands
|
||||
# over the removed directory's absolute path.
|
||||
primary = tmp_path / "primary"
|
||||
extra = tmp_path / "extra"
|
||||
(primary / "shared").mkdir(parents=True)
|
||||
(extra / "shared").mkdir(parents=True)
|
||||
twin_model = extra / "shared" / "twin.txt"
|
||||
twin_model.write_text("twin", encoding="utf-8")
|
||||
|
||||
scanner = TwoRootScanner(primary, extra)
|
||||
await scanner._initialize_cache()
|
||||
cache = await scanner.get_cached_data()
|
||||
(primary / "shared").rmdir()
|
||||
|
||||
await scanner.remove_known_folder("shared")
|
||||
|
||||
# The entry itself is disk-verified even on this path: the twin keeps it.
|
||||
assert "shared" in cache.all_folders
|
||||
# ... while the purge, lacking a path, cannot tell the copies apart.
|
||||
assert _normalize_path(twin_model) not in {
|
||||
item["file_path"] for item in cache.raw_data
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_remove_known_folder_keeps_a_twin_another_root_still_holds(tmp_path: Path):
|
||||
primary = tmp_path / "primary"
|
||||
extra = tmp_path / "extra"
|
||||
(primary / "test").mkdir(parents=True)
|
||||
(extra / "test").mkdir(parents=True)
|
||||
scanner = TwoRootScanner(primary, extra)
|
||||
await scanner._initialize_cache()
|
||||
cache = await scanner.get_cached_data()
|
||||
assert "test" in cache.all_folders
|
||||
|
||||
# The user deleted the primary copy through the sidebar.
|
||||
(primary / "test").rmdir()
|
||||
await scanner.remove_known_folder("test", str(primary / "test"))
|
||||
|
||||
# The merged folder list is a union: the node stays while any root holds it,
|
||||
# so it can no longer vanish and reappear on the next scan.
|
||||
assert "test" in cache.all_folders
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_remove_known_folder_drops_the_entry_once_no_root_holds_it(tmp_path: Path):
|
||||
primary = tmp_path / "primary"
|
||||
extra = tmp_path / "extra"
|
||||
(primary / "test").mkdir(parents=True)
|
||||
(extra / "test").mkdir(parents=True)
|
||||
scanner = TwoRootScanner(primary, extra)
|
||||
await scanner._initialize_cache()
|
||||
cache = await scanner.get_cached_data()
|
||||
assert "test" in cache.all_folders
|
||||
|
||||
(primary / "test").rmdir()
|
||||
(extra / "test").rmdir()
|
||||
await scanner.remove_known_folder("test", str(primary / "test"))
|
||||
|
||||
assert "test" not in cache.all_folders
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_remove_known_folder_purges_only_the_deleted_root_cards(tmp_path: Path):
|
||||
primary = tmp_path / "primary"
|
||||
extra = tmp_path / "extra"
|
||||
(primary / "shared").mkdir(parents=True)
|
||||
(extra / "shared").mkdir(parents=True)
|
||||
twin_model = extra / "shared" / "twin.txt"
|
||||
twin_model.write_text("twin", encoding="utf-8")
|
||||
|
||||
scanner = TwoRootScanner(primary, extra)
|
||||
await scanner._initialize_cache()
|
||||
cache = await scanner.get_cached_data()
|
||||
assert _normalize_path(twin_model) in {
|
||||
item["file_path"] for item in cache.raw_data
|
||||
}
|
||||
|
||||
# Empty copy in the primary root goes away.
|
||||
(primary / "shared").rmdir()
|
||||
await scanner.remove_known_folder("shared", str(primary / "shared"))
|
||||
|
||||
# Purging by the removed directory, not by its relative name: the twin's
|
||||
# model card must survive in the other root.
|
||||
assert _normalize_path(twin_model) in {
|
||||
item["file_path"] for item in cache.raw_data
|
||||
}
|
||||
assert "shared" in cache.folders
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_remove_known_folder_noop_without_recorded_folders(tmp_path: Path):
|
||||
_create_files(tmp_path)
|
||||
@@ -1627,6 +1740,41 @@ async def test_remove_known_folder_ignores_empty_input(tmp_path: Path):
|
||||
assert cache.all_folders == before
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rename_known_folder_keeps_a_twin_in_another_root(tmp_path: Path):
|
||||
primary = tmp_path / "primary"
|
||||
extra = tmp_path / "extra"
|
||||
(primary / "characters").mkdir(parents=True)
|
||||
(extra / "characters").mkdir(parents=True)
|
||||
twin_model = extra / "characters" / "twin.txt"
|
||||
twin_model.write_text("twin", encoding="utf-8")
|
||||
|
||||
scanner = TwoRootScanner(primary, extra)
|
||||
await scanner._initialize_cache()
|
||||
cache = await scanner.get_cached_data()
|
||||
twin_entry = next(
|
||||
item for item in cache.raw_data
|
||||
if item["file_path"] == _normalize_path(twin_model)
|
||||
)
|
||||
|
||||
old_abs = _normalize_path(primary / "characters")
|
||||
new_abs = _normalize_path(primary / "anime")
|
||||
os.rename(primary / "characters", primary / "anime")
|
||||
|
||||
await scanner.rename_known_folder(
|
||||
"characters", "anime", previous_path=old_abs, new_path=new_abs
|
||||
)
|
||||
|
||||
# The renamed root contributes the new name; the other root still owns the
|
||||
# old one, so both stay in the merged list.
|
||||
assert "anime" in cache.all_folders
|
||||
assert "characters" in cache.all_folders
|
||||
# And the twin's records are untouched: its files never moved.
|
||||
assert twin_entry["folder"] == "characters"
|
||||
assert twin_entry["file_path"] == _normalize_path(twin_model)
|
||||
assert "characters" in cache.folders
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rename_known_folder_rekeys_folders_cache_and_sidecar(tmp_path: Path):
|
||||
_, second, _ = _create_files(tmp_path)
|
||||
|
||||
Reference in New Issue
Block a user