diff --git a/py/services/model_scanner.py b/py/services/model_scanner.py index 5ee78291..477873b6 100644 --- a/py/services/model_scanner.py +++ b/py/services/model_scanner.py @@ -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 + ``/test`` must not hide a ``test`` that ```` 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( diff --git a/tests/services/test_model_scanner.py b/tests/services/test_model_scanner.py index 3ba31da4..19cdbc64 100644 --- a/tests/services/test_model_scanner.py +++ b/tests/services/test_model_scanner.py @@ -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)