mirror of
https://github.com/willmiao/ComfyUI-Lora-Manager.git
synced 2026-09-28 14:34:09 -03:00
fix(sidecars): carry the identity map through a sidecar root move
A model root that moved earlier is mirrored under a pinned name derived from its old path. If anything resolved against the new sidecar path before the relocation ran, the destination got a map naming the mirror after the *current* path. migrate_root's keep-newer transfer then dropped the source map, so the moved metadata stayed orphaned under the pinned component while reads followed the new name and rebuilt defaults — losing favorites, notes and tags a second time. The identity map is now relocated by relocate_root_map() instead of the generic transfer: entries recorded under the old sidecar root win for the roots they describe, destination-only entries are preserved, and the cache is dropped so the next resolution reloads the merged map. A merge that cannot be written is reported as a migration error rather than silently stranding the moved metadata. Reported by the Codex review on #1131.
This commit is contained in:
@@ -32,7 +32,7 @@ In centralized mode, sidecars and previews mirror each model root's directory st
|
||||
- If an identity cannot be re-anchored unambiguously (e.g. two same-named candidate roots), nothing is guessed: the mirror stays on disk untouched and surfaces as an orphan in **Doctor → Centralized Sidecars** (and in the log). Restoring the original root path re-links it automatically.
|
||||
- `.civitai.info` files always stay next to the model file, in both modes.
|
||||
- Changing the mode does **not** move existing files automatically — run the migration (`POST /api/lm/sidecars/migrate` with `{"direction": "to_centralized" | "to_alongside"}`, or the "Migrate Sidecars Now" button in settings). The migration covers excluded (hidden) models too, so un-excluding one later never strands its sidecar in the old layout. The result payload includes a `sidecar_root` field with the resolved centralized root, and the settings UI shows the outcome counters plus an "Open Folder" shortcut.
|
||||
- Changing `sidecar_storage_path` while centralized likewise needs a root relocation: `{"direction": "relocate_root", "old_root": "<previous path>"}` moves the whole mirror tree to the new root (the settings UI offers this automatically).
|
||||
- Changing `sidecar_storage_path` while centralized likewise needs a root relocation: `{"direction": "relocate_root", "old_root": "<previous path>"}` moves the whole mirror tree to the new root (the settings UI offers this automatically). The identity map travels with the tree, and its entries win over any map the destination acquired beforehand — so a mirror that was re-anchored earlier keeps its name even if something resolved against the new path before the relocation ran.
|
||||
- The settings UI always shows the resolved effective storage root (via the `sidecar_storage_root*` fields in `GET /api/lm/settings`), with `POST /api/lm/sidecars/open-location` opening it in the file manager. When the resolved root lies inside the plugin installation folder (portable settings mode), the UI warns: reinstalling or clean-updating the plugin would delete the sidecars, so an explicit path outside the installation folder is recommended. The repo `.gitignore` excludes the portable-mode default (`/sidecars/`).
|
||||
- All sidecar/preview path derivation goes through the helpers in `py/utils/sidecar_paths.py`; never construct paths inline. In the default `alongside` mode these helpers do no extra I/O at all — the identity map is only loaded and reconciled when centralized storage is actually in use.
|
||||
|
||||
|
||||
@@ -52,10 +52,12 @@ from ...utils.file_utils import find_preview_file, get_preview_extension
|
||||
from ...utils.metadata_manager import MetadataManager
|
||||
from ...utils.sidecar_paths import (
|
||||
METADATA_SUFFIX,
|
||||
ROOT_MAP_FILENAME,
|
||||
STORAGE_MODE_CENTRALIZED,
|
||||
get_configured_sidecar_root,
|
||||
get_sidecar_root,
|
||||
get_storage_mode,
|
||||
relocate_root_map,
|
||||
resolve_centralized_dir_for_dir,
|
||||
)
|
||||
|
||||
@@ -200,14 +202,19 @@ class SidecarMigrationUseCase:
|
||||
)
|
||||
|
||||
files: List[Tuple[str, str]] = []
|
||||
source_map_path = os.path.join(old, ROOT_MAP_FILENAME)
|
||||
if os.path.isdir(old):
|
||||
for dirpath, _dirnames, filenames in os.walk(old):
|
||||
rel = os.path.relpath(dirpath, old)
|
||||
target_dir = new_root if rel == os.curdir else os.path.join(new_root, rel)
|
||||
for filename in filenames:
|
||||
files.append(
|
||||
(os.path.join(dirpath, filename), os.path.join(target_dir, filename))
|
||||
)
|
||||
source = os.path.join(dirpath, filename)
|
||||
# The identity map is handled by relocate_root_map below:
|
||||
# _transfer's keep-newer rule would let a destination map
|
||||
# written before the relocation displace it.
|
||||
if source == source_map_path:
|
||||
continue
|
||||
files.append((source, os.path.join(target_dir, filename)))
|
||||
|
||||
errors: List[Dict[str, str]] = []
|
||||
counters: Dict[str, Any] = {"moved": 0, "conflicts": 0}
|
||||
@@ -243,6 +250,23 @@ class SidecarMigrationUseCase:
|
||||
errors.append({"model": os.path.basename(src), "error": str(exc)})
|
||||
await emit("processing", processed=index, current=os.path.basename(src))
|
||||
|
||||
# The identity map names the directories just moved, so it travels with
|
||||
# them and wins over any map the destination acquired beforehand.
|
||||
# A failure here strands the moved metadata, so it is a real error.
|
||||
try:
|
||||
if not relocate_root_map(old, new_root):
|
||||
errors.append(
|
||||
{
|
||||
"model": ROOT_MAP_FILENAME,
|
||||
"error": "sidecar root map could not be written to the new root",
|
||||
}
|
||||
)
|
||||
except Exception as exc:
|
||||
self._logger.error(
|
||||
"Sidecar root relocation failed for the root map: %s", exc, exc_info=True
|
||||
)
|
||||
errors.append({"model": ROOT_MAP_FILENAME, "error": str(exc)})
|
||||
|
||||
old_prefix = old.replace(os.sep, "/").rstrip("/") + "/"
|
||||
new_prefix = new_root.replace(os.sep, "/").rstrip("/") + "/"
|
||||
for sidecar in moved_sidecars:
|
||||
|
||||
+73
-10
@@ -357,11 +357,8 @@ def _persistable(sidecar_root: str) -> bool:
|
||||
return bool(probe) and os.access(probe, os.W_OK)
|
||||
|
||||
|
||||
def _save_root_map(sidecar_root: str, state: _RootMapState) -> bool:
|
||||
"""Atomically persist the root map; returns False when it cannot be written."""
|
||||
|
||||
if state.persist_disabled:
|
||||
return False
|
||||
def _write_root_map(sidecar_root: str, entries: Dict[str, Dict[str, object]]) -> bool:
|
||||
"""Atomically persist ``entries`` as the root map for ``sidecar_root``."""
|
||||
|
||||
path = _root_map_path(sidecar_root)
|
||||
# Snapshot before serializing: sample directories are appended from the
|
||||
@@ -375,7 +372,7 @@ def _save_root_map(sidecar_root: str, state: _RootMapState) -> bool:
|
||||
"last_path": entry.get("last_path", ""),
|
||||
"sample_rel_dirs": list(entry.get("sample_rel_dirs") or []),
|
||||
}
|
||||
for root_id, entry in state.entries.items()
|
||||
for root_id, entry in entries.items()
|
||||
},
|
||||
}
|
||||
temp_path = f"{path}.tmp"
|
||||
@@ -385,12 +382,23 @@ def _save_root_map(sidecar_root: str, state: _RootMapState) -> bool:
|
||||
json.dump(payload, handle, indent=2, ensure_ascii=False)
|
||||
os.replace(temp_path, path)
|
||||
except OSError as exc:
|
||||
logger.warning("sidecar_paths: cannot persist the root map %s: %s", path, exc)
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
def _save_root_map(sidecar_root: str, state: _RootMapState) -> bool:
|
||||
"""Persist a reconciled state; disables persistence when it cannot write."""
|
||||
|
||||
if state.persist_disabled:
|
||||
return False
|
||||
|
||||
if not _write_root_map(sidecar_root, state.entries):
|
||||
state.persist_disabled = True
|
||||
logger.warning(
|
||||
"sidecar_paths: cannot persist the root map %s (%s); mirror directory "
|
||||
"names fall back to path-derived components",
|
||||
path,
|
||||
exc,
|
||||
"sidecar_paths: mirror directory names fall back to path-derived "
|
||||
"components for %s",
|
||||
sidecar_root,
|
||||
)
|
||||
return False
|
||||
state.dirty_samples = False
|
||||
@@ -398,6 +406,61 @@ def _save_root_map(sidecar_root: str, state: _RootMapState) -> bool:
|
||||
return True
|
||||
|
||||
|
||||
def relocate_root_map(source_root: str, destination_root: str) -> bool:
|
||||
"""Carry the root map from a relocated sidecar root to its destination.
|
||||
|
||||
Call this after the mirror tree itself has been moved. Entries recorded
|
||||
under ``source_root`` win over any identity the destination picked up on
|
||||
its own: resolving against the new sidecar path *before* the relocation
|
||||
writes a map that names mirrors after the current model-root path, while
|
||||
the directories actually being moved are still named after the pinned
|
||||
identity. Destination-only entries are preserved, and the source file is
|
||||
always removed so the emptied tree can be pruned.
|
||||
|
||||
Returns False only when a source map existed but could not be written to
|
||||
the destination — the caller must surface that, since the moved metadata
|
||||
would otherwise be unreachable. Cached state is dropped either way so the
|
||||
next resolution reloads the merged map.
|
||||
"""
|
||||
|
||||
source_path = _root_map_path(source_root)
|
||||
source_entries = _load_root_map(source_root)
|
||||
|
||||
with _ROOT_MAPS_LOCK:
|
||||
if source_entries:
|
||||
destination_path = _root_map_path(destination_root)
|
||||
merged: Dict[str, Dict[str, object]] = {}
|
||||
if os.path.exists(destination_path):
|
||||
merged.update(_load_root_map(destination_root))
|
||||
source_paths = {
|
||||
_normalize_for_match(str(entry["last_path"]))
|
||||
for entry in source_entries.values()
|
||||
if entry.get("last_path")
|
||||
}
|
||||
preserved = {
|
||||
root_id: entry
|
||||
for root_id, entry in merged.items()
|
||||
if not entry.get("last_path")
|
||||
or _normalize_for_match(str(entry.get("last_path"))) not in source_paths
|
||||
}
|
||||
preserved.update(source_entries)
|
||||
if not _write_root_map(destination_root, preserved):
|
||||
return False
|
||||
|
||||
if os.path.exists(source_path):
|
||||
try:
|
||||
os.remove(source_path)
|
||||
except OSError as exc: # pragma: no cover - defensive cleanup
|
||||
logger.debug(
|
||||
"sidecar_paths: cannot remove relocated root map %s: %s",
|
||||
source_path,
|
||||
exc,
|
||||
)
|
||||
|
||||
reset_root_map_cache()
|
||||
return True
|
||||
|
||||
|
||||
def _new_root_id() -> str:
|
||||
return uuid.uuid4().hex[:8]
|
||||
|
||||
|
||||
@@ -4,6 +4,7 @@ from __future__ import annotations
|
||||
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
from pathlib import Path
|
||||
from typing import Any, Dict, List
|
||||
|
||||
@@ -12,7 +13,11 @@ import pytest
|
||||
from py.config import config
|
||||
from py.services.settings_manager import get_settings_manager
|
||||
from py.services.use_cases.sidecar_migration_use_case import SidecarMigrationUseCase
|
||||
from py.utils.sidecar_paths import reset_root_map_cache, root_mirror_component
|
||||
from py.utils.sidecar_paths import (
|
||||
get_metadata_path,
|
||||
reset_root_map_cache,
|
||||
root_mirror_component,
|
||||
)
|
||||
|
||||
|
||||
def _normalize(path) -> str:
|
||||
@@ -490,6 +495,61 @@ async def test_migrate_root_relocates_tree_and_reconciles(
|
||||
assert not old_root.exists()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migrate_root_preserves_a_reanchored_identity(
|
||||
library_root: Path, sidecar_root: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
):
|
||||
"""Relocating the sidecar root must not displace a pinned mirror name.
|
||||
|
||||
A root that moved earlier is mirrored under a name derived from its *old*
|
||||
path. If anything resolves against the destination sidecar root before the
|
||||
relocation runs, it writes a map naming the mirror after the *current*
|
||||
path; the relocation must keep the pinned entry, because the directories
|
||||
actually being moved are named after it.
|
||||
"""
|
||||
|
||||
_set_mode("centralized")
|
||||
first_sidecars = tmp_path / "first_sidecars"
|
||||
get_settings_manager().set("sidecar_storage_path", str(first_sidecars))
|
||||
reset_root_map_cache()
|
||||
|
||||
model = _write_model(library_root / "sub", "model")
|
||||
pinned = root_mirror_component(str(library_root))
|
||||
original = Path(get_metadata_path(str(model)))
|
||||
original.parent.mkdir(parents=True, exist_ok=True)
|
||||
original.write_text(json.dumps({"favorite": True}), encoding="utf-8")
|
||||
|
||||
# The model root moves; the recorded identity is re-anchored to it.
|
||||
moved_root = tmp_path / "relocated" / "loras"
|
||||
shutil.copytree(library_root, moved_root)
|
||||
monkeypatch.setattr(config, "loras_roots", [str(moved_root)], raising=False)
|
||||
reset_root_map_cache()
|
||||
moved_model = moved_root / "sub" / "model.safetensors"
|
||||
assert Path(get_metadata_path(str(moved_model))) == original
|
||||
assert root_mirror_component(str(moved_root)) == pinned
|
||||
|
||||
# Settings now point at the destination and something resolves first: the
|
||||
# destination map is written naming the mirror after the *current* path.
|
||||
get_settings_manager().set("sidecar_storage_path", str(sidecar_root))
|
||||
reset_root_map_cache()
|
||||
before_move = Path(get_metadata_path(str(moved_model)))
|
||||
assert before_move.parent.parent.name != pinned
|
||||
assert not before_move.exists()
|
||||
|
||||
use_case = _make_use_case([str(moved_model)])
|
||||
summary = await use_case.migrate_root(str(first_sidecars), force=True)
|
||||
assert summary["success"] is True
|
||||
|
||||
# The metadata travelled with the tree and is still found under the
|
||||
# pinned identity that names it.
|
||||
relocated = sidecar_root / original.relative_to(first_sidecars)
|
||||
assert relocated.exists()
|
||||
assert json.loads(relocated.read_text(encoding="utf-8")) == {"favorite": True}
|
||||
|
||||
reset_root_map_cache()
|
||||
assert Path(get_metadata_path(str(moved_model))) == relocated
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migrate_root_guards(
|
||||
library_root: Path, sidecar_root: Path, tmp_path: Path
|
||||
|
||||
@@ -24,6 +24,7 @@ from py.utils.sidecar_paths import (
|
||||
get_unmatched_sidecar_components,
|
||||
is_centralized,
|
||||
is_metadata_path,
|
||||
relocate_root_map,
|
||||
resolve_centralized_dir,
|
||||
resolve_centralized_dir_for_dir,
|
||||
resolve_metadata_path,
|
||||
@@ -32,6 +33,15 @@ from py.utils.sidecar_paths import (
|
||||
)
|
||||
|
||||
|
||||
def _write_map(root: Path, entries: dict) -> Path:
|
||||
root.mkdir(parents=True, exist_ok=True)
|
||||
path = root / ROOT_MAP_FILENAME
|
||||
path.write_text(
|
||||
json.dumps({"version": 1, "roots": entries}), encoding="utf-8"
|
||||
)
|
||||
return path
|
||||
|
||||
|
||||
def _normalize(path: Path) -> str:
|
||||
return str(path).replace(os.sep, "/")
|
||||
|
||||
@@ -467,6 +477,72 @@ class TestRootIdentityMap:
|
||||
assert Path(get_metadata_path(str(model))) == expected
|
||||
assert json.loads(expected.read_text(encoding="utf-8")) == {"favorite": True}
|
||||
|
||||
def test_relocate_root_map_prefers_the_source_entries(self, tmp_path: Path):
|
||||
"""A map written at the destination before a relocation must not win."""
|
||||
|
||||
source = tmp_path / "old-sidecars"
|
||||
destination = tmp_path / "new-sidecars"
|
||||
_write_map(
|
||||
source,
|
||||
{
|
||||
"aaaa1111": {
|
||||
"component": "loras-pinned",
|
||||
"basename": "loras",
|
||||
"last_path": "/models/loras",
|
||||
"sample_rel_dirs": [],
|
||||
}
|
||||
},
|
||||
)
|
||||
_write_map(
|
||||
destination,
|
||||
{
|
||||
"bbbb2222": {
|
||||
"component": "loras-recomputed",
|
||||
"basename": "loras",
|
||||
"last_path": "/models/loras",
|
||||
"sample_rel_dirs": [],
|
||||
},
|
||||
"cccc3333": {
|
||||
"component": "vae-other",
|
||||
"basename": "vae",
|
||||
"last_path": "/models/vae",
|
||||
"sample_rel_dirs": [],
|
||||
},
|
||||
},
|
||||
)
|
||||
|
||||
assert relocate_root_map(str(source), str(destination)) is True
|
||||
|
||||
merged = json.loads(
|
||||
(destination / ROOT_MAP_FILENAME).read_text(encoding="utf-8")
|
||||
)["roots"]
|
||||
assert merged["aaaa1111"]["component"] == "loras-pinned"
|
||||
assert "bbbb2222" not in merged # superseded for the same root path
|
||||
assert merged["cccc3333"]["component"] == "vae-other"
|
||||
assert not (source / ROOT_MAP_FILENAME).exists()
|
||||
|
||||
def test_relocate_root_map_without_a_source_map_is_a_noop(self, tmp_path: Path):
|
||||
source = tmp_path / "old-sidecars"
|
||||
destination = tmp_path / "new-sidecars"
|
||||
_write_map(
|
||||
destination,
|
||||
{
|
||||
"bbbb2222": {
|
||||
"component": "loras-keep",
|
||||
"basename": "loras",
|
||||
"last_path": "/models/loras",
|
||||
"sample_rel_dirs": [],
|
||||
}
|
||||
},
|
||||
)
|
||||
|
||||
assert relocate_root_map(str(source), str(destination)) is True
|
||||
|
||||
kept = json.loads(
|
||||
(destination / ROOT_MAP_FILENAME).read_text(encoding="utf-8")
|
||||
)["roots"]
|
||||
assert kept["bbbb2222"]["component"] == "loras-keep"
|
||||
|
||||
def test_corrupt_map_file_is_ignored_and_rebuilt(
|
||||
self, model_roots: dict, centralized: Path
|
||||
):
|
||||
|
||||
Reference in New Issue
Block a user