mirror of
https://github.com/willmiao/ComfyUI-Lora-Manager.git
synced 2026-10-12 04:22:13 -03:00
fix(scanner): verify recorded folders per entry, not per scan (#1145)
`_reconcile_cache()` replaced the recorded folder list only when *nothing* was left unverified: one switched-off drive, one directory `os.walk` could not enter, or one symlink whose target was gone, and the whole list fell back to a union with what the walk found. A folder moved with a file manager therefore stayed in the sidebar for good — the walk kept adding the new location while the old entry was never dropped, a restart re-hydrated the union from SQLite, and the ghost could not be deleted in the UI either because its directory was gone. * `_UnreachablePaths.covers_folder()` answers "could this specific entry have been read?" for a library-relative folder: it is kept only while a directory the walk failed to enter (or an offline symlink) may still hold it. A root that is unreadable *as a whole* deliberately does not count: library-relative names carry no root, so a missing drive would trivially cover every entry and no ghost could ever be cleared. Its folders still show while they hold cached models (`get_all_folders()` unions the model-derived folders) and a genuinely empty one comes back the next time that root is walked. * The merge keeps today's union for a scoped scan, and for a root-scoped scan that does not cover every root: those cannot speak for the roots they were not asked about. Asking for every root is a full refresh and cleans again. Verified: a folder moved externally is dropped while another root is offline, the offline root keeps its own folders and cached models, and a scoped scan still never drops a folder of a root it did not scan. The Windows-only half of this (a backslash `os.sep` making the configured twin shadow the live root, so deleted models were never pruned) is now reproduced in CI by patching `os.sep` instead of needing a Windows runner.
This commit is contained in:
@@ -22,9 +22,11 @@ Implementation notes / deviations from the draft below:
|
||||
they are outside every configured root prefix. The `root_unreachable` path therefore covers the
|
||||
*mid-session* case (drive switched off while LM runs), which is the workflow that motivated the
|
||||
issue; both paths keep the entries, which is what matters.
|
||||
* Known limitation to keep in mind: after a scoped scan (or one with unreadable paths) the recorded
|
||||
empty-folder list is unioned instead of replaced, so an empty folder deleted on disk can linger in
|
||||
the sidebar until the next full refresh. Models are unaffected.
|
||||
* Known limitation to keep in mind: after a *scoped* scan the recorded empty-folder list is unioned
|
||||
instead of replaced, so an empty folder deleted on disk can linger in the sidebar until the next
|
||||
full refresh. A full refresh keeps only the entries a specific unreadable area (a directory
|
||||
`os.walk` could not enter, an offline symlink) may still hold, so one switched-off drive no longer
|
||||
keeps every other root's ghosts alive (issue #1145).
|
||||
|
||||
## TL;DR (For humans)
|
||||
|
||||
|
||||
@@ -309,6 +309,36 @@ class _UnreachablePaths:
|
||||
return prefix
|
||||
return None
|
||||
|
||||
def covers_folder(self, root: str, folder: str) -> bool:
|
||||
"""Whether *folder* (relative to *root*) sits in an unreadable area.
|
||||
|
||||
Used when refreshing the recorded folder list: an entry the walk did not
|
||||
find is only kept while a *specific* area we could not read may still hold
|
||||
it -- a directory ``os.walk`` failed to enter, or a first-level symlink
|
||||
whose target is gone. Both directions count: the folder may be inside the
|
||||
unreadable directory, or hold it.
|
||||
|
||||
A root that is unreadable *as a whole* (drive switched off) deliberately
|
||||
does not count here. The recorded list holds library-relative names, so a
|
||||
missing root would trivially "cover" every entry and no ghost could ever be
|
||||
cleared (issue #1145). Its folders still show up while they hold cached
|
||||
models -- ``get_all_folders()`` unions the model-derived folders -- and a
|
||||
genuinely empty folder comes back the next time that root is walked.
|
||||
"""
|
||||
relative = (folder or "").replace("\\", "/").strip("/")
|
||||
if not relative:
|
||||
return False
|
||||
|
||||
root_prefix = _normalized_root_prefix(root)
|
||||
candidate = f"{root_prefix}{relative}/"
|
||||
if _CASE_INSENSITIVE_PATHS:
|
||||
candidate = candidate.lower()
|
||||
return any(
|
||||
prefix != root_prefix
|
||||
and (candidate.startswith(prefix) or prefix.startswith(candidate))
|
||||
for prefix in self._prefixes
|
||||
)
|
||||
|
||||
def record_kept(self, prefix: str, amount: int = 1) -> None:
|
||||
detail = self._details.get(prefix)
|
||||
if detail is not None:
|
||||
@@ -2197,22 +2227,43 @@ class ModelScanner:
|
||||
total_removed += dedup_removed
|
||||
|
||||
# The walk above visited every directory *in scope*, so refresh the
|
||||
# recorded folder list (including empty folders) when nothing was
|
||||
# left unverified — e.g. an empty folder was created or removed
|
||||
# externally. A scoped scan, or one with unreadable paths, can only
|
||||
# add to the list: dropping folders this walk never looked at would
|
||||
# empty the sidebar for every drive that was not scanned.
|
||||
# recorded folder list (including empty folders) when this run could
|
||||
# speak for every root: e.g. an empty folder was created or removed
|
||||
# externally.
|
||||
#
|
||||
# A folder the walk did not find is dropped unless a root we could not
|
||||
# read still covers it — the drive is off, ``os.walk`` failed to enter
|
||||
# a directory, or a first-level symlink lost its target. Keeping the
|
||||
# *whole* list whenever anything was unreadable is what made a ghost
|
||||
# folder outlive every refresh, a restart and even the folder being
|
||||
# deleted in the UI (issue #1145). A scoped scan cannot speak for the
|
||||
# roots it was not asked about, so it still only adds: dropping
|
||||
# folders it never looked at would empty the sidebar for every drive
|
||||
# that was not scanned.
|
||||
sorted_discovered = sorted(discovered_folders, key=lambda x: x.lower())
|
||||
fully_verified = (
|
||||
len(roots) == len(configured_roots)
|
||||
and scope_folder is None
|
||||
and not unreachable.has_entries()
|
||||
discovered_set = set(sorted_discovered)
|
||||
recorded = set(self._cache.all_folders or [])
|
||||
speaks_for_every_root = scope_folder is None and (
|
||||
scope is None
|
||||
or scope.roots is None
|
||||
or len(scope_roots) == len(configured_roots)
|
||||
)
|
||||
if fully_verified:
|
||||
merged_folders = sorted_discovered
|
||||
if speaks_for_every_root:
|
||||
merged_folders = sorted(
|
||||
discovered_set
|
||||
| {
|
||||
folder
|
||||
for folder in recorded - discovered_set
|
||||
if any(
|
||||
unreachable.covers_folder(root, folder)
|
||||
for root in configured_roots + configured_only
|
||||
)
|
||||
},
|
||||
key=lambda x: x.lower(),
|
||||
)
|
||||
else:
|
||||
merged_folders = sorted(
|
||||
set(self._cache.all_folders or []) | set(sorted_discovered),
|
||||
recorded | discovered_set,
|
||||
key=lambda x: x.lower(),
|
||||
)
|
||||
folders_changed = self._cache.all_folders != merged_folders
|
||||
|
||||
@@ -2964,6 +2964,18 @@ def _windows_spelling(path: Path) -> str:
|
||||
return str(path).replace("/", "\\")
|
||||
|
||||
|
||||
def _simulate_windows_separators(monkeypatch) -> None:
|
||||
"""Make ``os.sep`` a backslash, as it is on the reporting machine.
|
||||
|
||||
``os.path`` stays ``posixpath`` (its separator is its own constant), so this
|
||||
only changes code that folds separators through ``os.sep`` -- which is exactly
|
||||
what turned the backslash twin into the live root's unreachable prefix on
|
||||
Windows and stopped deleted models from being pruned. Without it that half of
|
||||
#1145/#1146 can only be reproduced on Windows.
|
||||
"""
|
||||
monkeypatch.setattr(os, "sep", "\\")
|
||||
|
||||
|
||||
async def _windows_spelling_scanner(tmp_path: Path, monkeypatch):
|
||||
"""Scanner configured with backslash roots plus a never-created output dir.
|
||||
|
||||
@@ -2983,6 +2995,7 @@ async def _windows_spelling_scanner(tmp_path: Path, monkeypatch):
|
||||
"get_configured_model_roots",
|
||||
lambda: [_windows_spelling(primary), _windows_spelling(never_created)],
|
||||
)
|
||||
_simulate_windows_separators(monkeypatch)
|
||||
return scanner, primary, never_created
|
||||
|
||||
|
||||
@@ -3079,3 +3092,165 @@ async def test_scoped_scan_accepts_the_host_path_spelling(tmp_path: Path, monkey
|
||||
|
||||
assert summary is not None
|
||||
assert summary["scanned_roots"] == [primary.name]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_external_folder_move_leaves_no_ghost(tmp_path: Path, monkeypatch):
|
||||
"""A folder moved with a file manager must leave no ghost (issue #1145).
|
||||
|
||||
Both reported symptoms share the cause fixed for #1146: while the configured
|
||||
twin of a live root was registered as unreachable, the cached rows under the
|
||||
old path were "kept" instead of pruned (the same model was then listed at its
|
||||
old and its new location), and the walk could never be considered fully
|
||||
verified -- so `all_folders` only ever grew and the deleted folder stayed in
|
||||
the sidebar, even though the delete dialog correctly reported it as gone.
|
||||
"""
|
||||
primary = tmp_path / "primary"
|
||||
(primary / "Krea2" / "Women").mkdir(parents=True)
|
||||
original = primary / "Krea2" / "Women" / "a.txt"
|
||||
original.write_text("a", encoding="utf-8")
|
||||
|
||||
scanner = MultiRootDummyScanner([primary])
|
||||
await scanner._initialize_cache()
|
||||
assert "Krea2/Women" in await scanner.get_all_folders()
|
||||
|
||||
# What Dopus did: the folder is gone and its file lives somewhere else.
|
||||
(primary / "Krea2" / "New").mkdir()
|
||||
shutil.move(str(original), str(primary / "Krea2" / "New" / "a.txt"))
|
||||
(primary / "Krea2" / "Women").rmdir()
|
||||
monkeypatch.setattr(
|
||||
scanner, "get_configured_model_roots", lambda: [_windows_spelling(primary)]
|
||||
)
|
||||
_simulate_windows_separators(monkeypatch)
|
||||
|
||||
summary = await scanner._reconcile_cache()
|
||||
|
||||
# One model, listed once: the old path is pruned and the new one added.
|
||||
assert summary is not None
|
||||
assert summary["removed"] == 1
|
||||
assert summary["added"] == 1
|
||||
assert summary["kept_unreachable"] == 0
|
||||
assert [item["file_path"] for item in scanner._cache.raw_data] == [
|
||||
_normalize_path(primary / "Krea2" / "New" / "a.txt")
|
||||
]
|
||||
# ...and the folder record follows the filesystem instead of only growing.
|
||||
all_folders = await scanner.get_all_folders()
|
||||
assert "Krea2/Women" not in all_folders
|
||||
assert "Krea2/New" in all_folders
|
||||
|
||||
|
||||
def test_unreachable_paths_cover_whole_folders():
|
||||
"""The folder refresh asks "could this specific entry have been read?"."""
|
||||
unreachable = model_scanner._UnreachablePaths()
|
||||
unreachable.add("/mnt/drive-z/loras", "root_unreachable", kept=1)
|
||||
unreachable.add("/mnt/a/loras/locked", "unreadable_dir")
|
||||
|
||||
# A directory the walk could not enter covers itself and everything below it,
|
||||
# whichever way the root is spelled.
|
||||
assert unreachable.covers_folder("/mnt/a/loras", "locked") is True
|
||||
assert unreachable.covers_folder("/mnt/a/loras", "locked/deep") is True
|
||||
assert unreachable.covers_folder("\\mnt\\a\\loras", "locked/deep") is True
|
||||
|
||||
# A root that is unreadable as a whole does not: library-relative names carry
|
||||
# no root, so it would cover every entry and keep every ghost alive (#1145).
|
||||
assert unreachable.covers_folder("/mnt/drive-z/loras", "pack") is False
|
||||
|
||||
assert unreachable.covers_folder("/mnt/a/loras", "free") is False
|
||||
assert unreachable.covers_folder("/mnt/a/loras", "locked_other") is False
|
||||
assert unreachable.covers_folder("/mnt/a/loras", "") is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ghost_folder_cleared_while_another_root_is_offline(
|
||||
tmp_path: Path, monkeypatch
|
||||
):
|
||||
"""A switched-off drive must not keep the other roots' ghosts alive (#1145).
|
||||
|
||||
Keeping the whole folder list whenever anything was unreadable is what made a
|
||||
folder moved in a file manager outlive every refresh. Only the entries the
|
||||
unreadable root can actually account for may be kept.
|
||||
"""
|
||||
online = tmp_path / "online"
|
||||
offline = tmp_path / "offline"
|
||||
(online / "old" / "pack").mkdir(parents=True)
|
||||
(online / "old" / "pack" / "a.txt").write_text("a", encoding="utf-8")
|
||||
(offline / "keepme").mkdir(parents=True)
|
||||
(offline / "keepme" / "b.txt").write_text("b", encoding="utf-8")
|
||||
(offline / "empty_folder").mkdir(parents=True)
|
||||
|
||||
scanner = MultiRootDummyScanner([online, offline])
|
||||
await scanner._initialize_cache()
|
||||
assert "old/pack" in scanner._cache.all_folders
|
||||
assert "keepme" in scanner._cache.all_folders
|
||||
|
||||
# The folder is moved externally...
|
||||
(online / "new" / "pack").mkdir(parents=True)
|
||||
shutil.move(
|
||||
str(online / "old" / "pack" / "a.txt"),
|
||||
str(online / "new" / "pack" / "a.txt"),
|
||||
)
|
||||
shutil.rmtree(online / "old")
|
||||
# ...while the other drive is switched off.
|
||||
shutil.rmtree(offline)
|
||||
|
||||
summary = await scanner._reconcile_cache()
|
||||
|
||||
assert summary is not None
|
||||
assert summary["added"] == 1
|
||||
assert summary["removed"] == 1
|
||||
assert summary["kept_unreachable"] == 1
|
||||
|
||||
folders = await scanner.get_all_folders()
|
||||
assert "old" not in folders
|
||||
assert "old/pack" not in folders
|
||||
assert "new/pack" in folders
|
||||
# The unreadable root keeps its model-bearing folders and its cached models
|
||||
# (model-derived folders are always unioned in)...
|
||||
assert "keepme" in folders
|
||||
assert _normalize_path(offline / "keepme" / "b.txt") in {
|
||||
item["file_path"] for item in scanner._cache.raw_data
|
||||
}
|
||||
# ...while an *empty* folder on it cannot be attributed to any root, so it is
|
||||
# not kept alive. It comes back the next time that root is walked.
|
||||
assert "empty_folder" not in folders
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_scoped_scan_never_drops_folders_of_other_roots(tmp_path: Path):
|
||||
"""A scoped scan cannot speak for the roots it was not asked about."""
|
||||
root_a = tmp_path / "a"
|
||||
root_b = tmp_path / "b"
|
||||
(root_a / "a_folder").mkdir(parents=True)
|
||||
(root_b / "b_folder").mkdir(parents=True)
|
||||
|
||||
scanner = MultiRootDummyScanner([root_a, root_b])
|
||||
await scanner._initialize_cache()
|
||||
|
||||
shutil.rmtree(root_a / "a_folder")
|
||||
await scanner._reconcile_cache(scope=ReconcileScope(roots=(str(root_b),)))
|
||||
|
||||
folders = await scanner.get_all_folders()
|
||||
assert "a_folder" in folders
|
||||
assert "b_folder" in folders
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_root_scoped_scan_of_every_root_still_cleans_folders(tmp_path: Path):
|
||||
"""Asking for every root is a full refresh: ghosts are dropped."""
|
||||
root_a = tmp_path / "a"
|
||||
root_b = tmp_path / "b"
|
||||
(root_a / "a_folder").mkdir(parents=True)
|
||||
(root_b / "b_folder").mkdir(parents=True)
|
||||
|
||||
scanner = MultiRootDummyScanner([root_a, root_b])
|
||||
await scanner._initialize_cache()
|
||||
|
||||
shutil.rmtree(root_a / "a_folder")
|
||||
await scanner._reconcile_cache(
|
||||
scope=ReconcileScope(roots=(str(root_a), str(root_b)))
|
||||
)
|
||||
|
||||
folders = await scanner.get_all_folders()
|
||||
assert "a_folder" not in folders
|
||||
assert "b_folder" in folders
|
||||
|
||||
|
||||
Reference in New Issue
Block a user