From 0508daa0fbad605323e10d075b293b9fde100a09 Mon Sep 17 00:00:00 2001 From: Will Miao Date: Mon, 12 Oct 2026 09:31:17 +0800 Subject: [PATCH] fix(scanner): verify recorded folders per entry, not per scan (#1145) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_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. --- docs/plans/issue-1108-scoped-scan.md | 8 +- py/services/model_scanner.py | 75 ++++++++++-- tests/services/test_model_scanner.py | 175 +++++++++++++++++++++++++++ 3 files changed, 243 insertions(+), 15 deletions(-) diff --git a/docs/plans/issue-1108-scoped-scan.md b/docs/plans/issue-1108-scoped-scan.md index ebce80c8..6b2373f2 100644 --- a/docs/plans/issue-1108-scoped-scan.md +++ b/docs/plans/issue-1108-scoped-scan.md @@ -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) diff --git a/py/services/model_scanner.py b/py/services/model_scanner.py index eb40af79..14a3e7ef 100644 --- a/py/services/model_scanner.py +++ b/py/services/model_scanner.py @@ -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 diff --git a/tests/services/test_model_scanner.py b/tests/services/test_model_scanner.py index d69e1bac..af5f956b 100644 --- a/tests/services/test_model_scanner.py +++ b/tests/services/test_model_scanner.py @@ -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 +