diff --git a/py/services/model_scanner.py b/py/services/model_scanner.py index 22555254..7948da03 100644 --- a/py/services/model_scanner.py +++ b/py/services/model_scanner.py @@ -1005,14 +1005,29 @@ class ModelScanner: await self._broadcast_scan_progress('started', 'reconcile_scan', 0, False) # Get current cached file paths + cached_size_before = len(self._cache.raw_data) cached_paths = {item['file_path'] for item in self._cache.raw_data} path_to_item = {item['file_path']: item for item in self._cache.raw_data} - cached_real_paths = {} - for cached_path in cached_paths: - try: - cached_real_paths.setdefault(os.path.realpath(cached_path), cached_path) - except Exception: - continue + + # physical path -> cached business path, for the alias case where the + # same file is reachable under a different path than the cached one + # (overlapping roots / symlink layout changes): keep the existing + # entry instead of delete + re-add (which would re-read metadata and + # re-hash every file). Built lazily on the first miss, because a + # realpath per cached entry is ~half the cost of a no-change + # reconcile and the map is only ever consulted for misses. + cached_real_paths: Optional[Dict[str, str]] = None + + def lookup_cached_real_path(real_path: str) -> Optional[str]: + nonlocal cached_real_paths + if cached_real_paths is None: + cached_real_paths = {} + for cached_path in cached_paths: + try: + cached_real_paths.setdefault(os.path.realpath(cached_path), cached_path) + except Exception: + continue + return cached_real_paths.get(real_path) # Track found files and new files found_paths = set() @@ -1038,14 +1053,18 @@ class ModelScanner: if ext in self.file_extensions: # Construct paths exactly as they would be in cache file_path = os.path.join(root, file).replace(os.sep, '/') - real_file_path = os.path.realpath(os.path.join(root, file)) - + # Check if this file is already in cache if file_path in cached_paths: found_paths.add(file_path) continue - cached_real_match = cached_real_paths.get(real_file_path) + # Only a cache miss needs the physical path, so the + # realpath syscalls are paid per changed file rather + # than per file in the library. + real_file_path = os.path.realpath(os.path.join(root, file)) + + cached_real_match = lookup_cached_real_path(real_file_path) if cached_real_match: found_paths.add(cached_real_match) continue @@ -1090,6 +1109,9 @@ class ModelScanner: total_new = len(new_files) processed_new = 0 last_progress_time = time.time() + # Snapshot the roots once: this matches the walk above (which + # also snapshots them) and avoids a config read per new file. + model_roots = self.get_model_roots() for i in range(0, total_new, batch_size): batch = new_files[i:i+batch_size] for path in batch: @@ -1098,12 +1120,10 @@ class ModelScanner: try: # Find the appropriate root path for this file root_path = None - model_roots = self.get_model_roots() + normalized_path = os.path.normpath(path) for potential_root in model_roots: # Normalize both paths for comparison - normalized_path = os.path.normpath(path) - normalized_root = os.path.normpath(potential_root) - if normalized_path.startswith(normalized_root): + if normalized_path.startswith(os.path.normpath(potential_root)): root_path = potential_root break @@ -1200,24 +1220,32 @@ class ModelScanner: # Update cache data self._cache.raw_data = [item for item in self._cache.raw_data if item['file_path'] not in missing_files] - dedup_removed = 0 - seen_paths: set[str] = set() - deduped: list[Dict[str, Any]] = [] - for item in reversed(self._cache.raw_data): - path = item.get('file_path', '') - if path not in seen_paths: - seen_paths.add(path) - deduped.append(item) - else: - for tag in item.get('tags', []): - if tag in self._tags_count: - self._tags_count[tag] = max(0, self._tags_count[tag] - 1) - if self._tags_count[tag] == 0: - del self._tags_count[tag] - dedup_removed += 1 - if dedup_removed > 0: - self._cache.raw_data = list(reversed(deduped)) - total_removed += dedup_removed + # Defensive integrity pass: drop entries sharing a business path. + # Duplicates can only be introduced by external code rewriting + # raw_data directly or by this pass's own appends, so an unchanged + # filesystem walk over a clean cache has nothing to clean. The size + # mismatch is an O(1) tell that the snapshot already contained + # duplicates; skipping the O(N) pass when it is provably clean is + # what keeps a no-change Refresh cheap. + if cached_size_before != len(cached_paths) or total_added > 0: + dedup_removed = 0 + seen_paths: set[str] = set() + deduped: list[Dict[str, Any]] = [] + for item in reversed(self._cache.raw_data): + path = item.get('file_path', '') + if path not in seen_paths: + seen_paths.add(path) + deduped.append(item) + else: + for tag in item.get('tags', []): + if tag in self._tags_count: + self._tags_count[tag] = max(0, self._tags_count[tag] - 1) + if self._tags_count[tag] == 0: + del self._tags_count[tag] + dedup_removed += 1 + if dedup_removed > 0: + self._cache.raw_data = list(reversed(deduped)) + total_removed += dedup_removed # Resort cache if changes were made if total_added > 0 or total_removed > 0: diff --git a/tests/services/test_model_scanner.py b/tests/services/test_model_scanner.py index 8b313ffb..df1adbaf 100644 --- a/tests/services/test_model_scanner.py +++ b/tests/services/test_model_scanner.py @@ -732,6 +732,130 @@ async def test_reconcile_cache_removes_duplicate_alias_when_same_real_file_seen_ assert cached_paths == {_normalize_path(loras_root / "link" / "one.txt")} +@pytest.mark.asyncio +async def test_reconcile_cache_keeps_cached_path_when_walk_yields_a_live_alias( + tmp_path: Path, +): + """A root-order / symlink change can make the walk produce a *different but + still live* business path for a file already in the cache. The realpath + alias map must keep the cached entry instead of re-processing the file and + swapping the path (which would re-read metadata and re-hash the weights).""" + loras_root = tmp_path / "loras" + loras_root.mkdir() + extra_root = tmp_path / "extra" + extra_root.mkdir() + (extra_root / "one.txt").write_text("one", encoding="utf-8") + (loras_root / "link").symlink_to(extra_root, target_is_directory=True) + + # `extra_root` comes first, so the cache entry is stored under its path. + scanner = MultiRootDummyScanner([extra_root, loras_root]) + await scanner._initialize_cache() + + cached_before = {item["file_path"] for item in scanner._cache.raw_data} + assert cached_before == {_normalize_path(extra_root / "one.txt")} + + # The symlinked path now wins the walk; the file itself is unchanged. + scanner._roots = [str(loras_root), str(extra_root)] + processed: List[str] = [] + + async def _record_process(file_path: str, root_path: str, *args, **kwargs): + processed.append(file_path) + return await DummyScanner._process_model_file( + scanner, file_path, root_path, *args, **kwargs + ) + + scanner._process_model_file = _record_process # type: ignore[method-assign] + + await scanner._reconcile_cache() + + cache = await scanner.get_cached_data() + assert {item["file_path"] for item in cache.raw_data} == cached_before + assert processed == [] + + +@pytest.mark.asyncio +async def test_reconcile_cache_defers_realpath_to_cache_misses( + tmp_path: Path, monkeypatch +): + """A no-change reconcile must not call realpath for unchanged files or for + every cached entry: both the alias map and the per-file realpath are only + needed for cache misses (they dominate the cost of a Refresh otherwise).""" + root = tmp_path / "loras" + root.mkdir() + for i in range(5): + (root / f"model{i}.txt").write_text("x", encoding="utf-8") + + scanner = DummyScanner(root) + await scanner._initialize_cache() + + real_realpath = model_scanner.os.path.realpath + realpath_args: List[str] = [] + + def _recording_realpath(path, *args, **kwargs): + realpath_args.append(os.fspath(path)) + return real_realpath(path, *args, **kwargs) + + monkeypatch.setattr(model_scanner.os.path, "realpath", _recording_realpath) + + await scanner._reconcile_cache() + + model_files = {_normalize_path(path) for path in root.glob("*.txt")} + assert not (set(realpath_args) & model_files) + + +@pytest.mark.asyncio +async def test_reconcile_cache_cleans_pre_existing_duplicate_paths(tmp_path: Path): + """External code rewrites raw_data directly, so a reconcile must still drop + duplicate business paths even when nothing changed on disk: the O(1) + integrity check may only skip the pass for a provably clean cache.""" + root = tmp_path / "loras" + root.mkdir() + (root / "one.txt").write_text("one", encoding="utf-8") + (root / "two.txt").write_text("two", encoding="utf-8") + + scanner = DummyScanner(root) + await scanner._initialize_cache() + + first_path = _normalize_path(root / "one.txt") + duplicate = dict(next(i for i in scanner._cache.raw_data if i["file_path"] == first_path)) + duplicate["model_name"] = "duplicate-wins" + scanner._cache.raw_data.append(duplicate) + + await scanner._reconcile_cache() + + cache = await scanner.get_cached_data() + assert len(cache.raw_data) == 2 + survivor = next(i for i in cache.raw_data if i["file_path"] == first_path) + assert survivor["model_name"] == "duplicate-wins" + + +@pytest.mark.asyncio +async def test_reconcile_cache_reads_model_roots_once_per_phase(tmp_path: Path, monkeypatch): + """get_model_roots() must be snapshotted once for the walk and once for the + new-file pass, not re-read for every new file.""" + root = tmp_path / "loras" + root.mkdir() + scanner = DummyScanner(root) + await scanner._initialize_cache() + + calls = 0 + real_get_model_roots = scanner.get_model_roots + + def _counting_get_model_roots() -> List[str]: + nonlocal calls + calls += 1 + return real_get_model_roots() + + monkeypatch.setattr(scanner, "get_model_roots", _counting_get_model_roots) + + for i in range(3): + (root / f"new{i}.txt").write_text("x", encoding="utf-8") + + await scanner._reconcile_cache() + + assert calls == 2 + + @pytest.mark.asyncio async def test_log_duplicate_filename_summary_logs_warning(tmp_path: Path, caplog): """When duplicate filenames exist, _log_duplicate_filename_summary should emit