fix: address review — injective mirror roots, root relocation, full preview coverage, EXDEV-safe rollback

Codex review on #1124:

- P1: mirror layout root component is now <basename>-<roothash>
  (sha256 of the normalized root path), so two roots sharing a basename
  no longer map to the same mirror directory and overwrite each other's
  sidecars
- P1: changing sidecar_storage_path while centralized no longer strands
  assets in the old root — new relocate_root migration direction moves
  the whole mirror tree, rewrites preview_url prefixes inside sidecars,
  reconciles scanner caches, and prunes the emptied old tree; the
  settings UI detects the path change and offers the relocation
- P2: migration enumerates the same preview candidates as
  find_preview_file — case-insensitive variants (model.WEBP) and the
  legacy .example.0.jpeg suffix — instead of exact lowercase
  PREVIEW_EXTENSIONS only
- P2: _rollback_model_staging restores staged files with the
  EXDEV-tolerant mover, so a failed undoable-delete staging no longer
  strands a cross-filesystem centralized sidecar copy

Tests: same-basename root injectivity, mixed-case/example preview
migration, relocate_root happy path + guards + route 400, frontend
relocation prompt flow. Verified end-to-end in a sandboxed standalone
server: uppercase/legacy previews migrate, root relocation moves the
tree and the list API serves the new locations immediately without a
rescan.
This commit is contained in:
Will Miao
2026-09-26 12:24:17 +08:00
parent 16430aef21
commit a6fca8612f
22 changed files with 592 additions and 71 deletions
@@ -281,4 +281,60 @@ describe('SettingsManager sidecar storage', () => {
expect(migrateBtn.disabled).toBe(false);
});
});
describe('handleSidecarStoragePathChange', () => {
it('offers root relocation when the path changes in centralized mode', async () => {
const manager = createManager();
const { pathInput } = appendSidecarControls();
const modal = appendMigrationModal();
state.global.settings = { sidecar_storage_mode: 'centralized', sidecar_storage_path: '/old/root' };
manager._loadedSidecarStoragePath = '/old/root';
pathInput.value = '/new/root';
mockFetchOk();
const changePromise = manager.handleSidecarStoragePathChange();
await vi.waitFor(() => expect(modal.classList.contains('show')).toBe(true));
modal.querySelector('[data-action="confirm-sidecar-migration"]').click();
await changePromise;
expect(global.fetch).toHaveBeenCalledWith('/api/lm/sidecars/migrate', expect.objectContaining({
body: JSON.stringify({ direction: 'relocate_root', force: true, old_root: '/old/root' }),
}));
expect(manager._loadedSidecarStoragePath).toBe('/new/root');
});
it('does not prompt when the path changes in alongside mode', async () => {
const manager = createManager();
const { pathInput } = appendSidecarControls();
appendMigrationModal();
state.global.settings = { sidecar_storage_mode: 'alongside', sidecar_storage_path: '/old/root' };
manager._loadedSidecarStoragePath = '/old/root';
pathInput.value = '/new/root';
mockFetchOk();
await manager.handleSidecarStoragePathChange();
const migrateCalls = global.fetch.mock.calls.filter(([url]) => url === '/api/lm/sidecars/migrate');
expect(migrateCalls).toHaveLength(0);
});
it('shows a deferred notice when relocation is cancelled', async () => {
const manager = createManager();
const { pathInput } = appendSidecarControls();
const modal = appendMigrationModal();
state.global.settings = { sidecar_storage_mode: 'centralized', sidecar_storage_path: '/old/root' };
manager._loadedSidecarStoragePath = '/old/root';
pathInput.value = '/new/root';
mockFetchOk();
const changePromise = manager.handleSidecarStoragePathChange();
await vi.waitFor(() => expect(modal.classList.contains('show')).toBe(true));
modal.querySelector('[data-action="cancel-sidecar-migration"]').click();
await changePromise;
const migrateCalls = global.fetch.mock.calls.filter(([url]) => url === '/api/lm/sidecars/migrate');
expect(migrateCalls).toHaveLength(0);
expect(showToast).toHaveBeenCalledWith('settings.sidecarStorage.migrationDeferred', {}, 'info');
});
});
});
+42 -5
View File
@@ -2565,8 +2565,8 @@ class DummySidecarMigrationUseCase:
self.result = result
self.calls = []
async def execute_with_error_handling(self, *, direction, progress_cb=None, force=False):
self.calls.append({"direction": direction, "force": force})
async def execute_with_error_handling(self, *, direction, progress_cb=None, force=False, old_root=None):
self.calls.append({"direction": direction, "force": force, "old_root": old_root})
return self.result
@@ -2592,7 +2592,7 @@ async def test_sidecar_migration_handler_runs_to_centralized():
assert response.status == 200
assert payload["success"] is True
assert payload["moved"] == 3
assert use_case.calls == [{"direction": "to_centralized", "force": True}]
assert use_case.calls == [{"direction": "to_centralized", "force": True, "old_root": ""}]
@pytest.mark.asyncio
@@ -2624,7 +2624,7 @@ async def test_sidecar_migration_handler_accepts_get_query_params():
assert response.status == 200
assert payload["success"] is True
assert use_case.calls == [{"direction": "to_alongside", "force": True}]
assert use_case.calls == [{"direction": "to_alongside", "force": True, "old_root": ""}]
@pytest.mark.asyncio
@@ -2640,4 +2640,41 @@ async def test_sidecar_migration_handler_guard_refusal_is_400():
assert response.status == 400
assert payload["success"] is False
assert "already centralized" in payload["error"]
assert use_case.calls == [{"direction": "to_centralized", "force": False}]
assert use_case.calls == [{"direction": "to_centralized", "force": False, "old_root": ""}]
@pytest.mark.asyncio
async def test_sidecar_migration_handler_relocate_root_passes_old_root():
result = {"success": True, "direction": "relocate_root", "moved": 5}
handler, use_case = _sidecar_migration_handler(result)
response = await handler.migrate_sidecars(
FakeRequest( # pyright: ignore[reportArgumentType]
json_data={
"direction": "relocate_root",
"old_root": "/old/sidecars",
"force": True,
}
)
)
payload = _json_payload(response)
assert response.status == 200
assert payload["success"] is True
assert use_case.calls == [
{"direction": "relocate_root", "force": True, "old_root": "/old/sidecars"}
]
@pytest.mark.asyncio
async def test_sidecar_migration_handler_relocate_root_requires_old_root():
handler, use_case = _sidecar_migration_handler({"success": True})
response = await handler.migrate_sidecars(
FakeRequest(json_data={"direction": "relocate_root"}) # pyright: ignore[reportArgumentType]
)
payload = _json_payload(response)
assert response.status == 400
assert "old_root" in payload["error"]
assert use_case.calls == []
@@ -19,6 +19,7 @@ from py.services.model_lifecycle_service import (
from py.services.pending_delete_service import PendingDeleteService
from py.services.settings_manager import get_settings_manager
from py.utils.metadata_manager import MetadataManager
from py.utils.sidecar_paths import root_mirror_component
def _normalize(path) -> str:
@@ -57,11 +58,12 @@ def centralized(library_root: Path, tmp_path: Path) -> Path:
return sidecar_root
def _mirror_dir(sidecar_root: Path, *rel: str) -> Path:
def _mirror_dir(library_root: Path, sidecar_root: Path, *rel: str) -> Path:
"""Expected mirror directory for a library-relative path."""
library = get_settings_manager().get_active_library_name()
return sidecar_root.joinpath(library, "checkpoints", *rel)
component = root_mirror_component(str(library_root))
return sidecar_root.joinpath(library, component, *rel)
def _write_sidecar(
@@ -93,7 +95,7 @@ async def test_delete_model_artifacts_centralized(
):
model = library_root / "model.safetensors"
model.write_bytes(b"weights")
mirror = _mirror_dir(centralized)
mirror = _mirror_dir(library_root, centralized)
_write_sidecar(mirror, "model", file_path=model, preview_name="model.preview.webp")
deleted = await delete_model_artifacts(str(library_root), "model")
@@ -131,7 +133,7 @@ def test_enumerate_model_artifacts_centralized(
):
model = library_root / "model.safetensors"
model.write_bytes(b"weights")
mirror = _mirror_dir(centralized)
mirror = _mirror_dir(library_root, centralized)
_write_sidecar(mirror, "model", file_path=model, preview_name="model.preview.png")
service = PendingDeleteService.__new__(PendingDeleteService)
@@ -170,7 +172,7 @@ async def _json_metadata_loader(path: str) -> Dict[str, object]:
async def test_rename_model_centralized(library_root: Path, centralized: Path):
model = library_root / "model.safetensors"
model.write_bytes(b"weights")
mirror = _mirror_dir(centralized)
mirror = _mirror_dir(library_root, centralized)
_write_sidecar(mirror, "model", file_path=model, preview_name="model.preview.webp")
service = ModelLifecycleService(
@@ -213,7 +215,7 @@ async def test_move_model_centralized(
model.write_bytes(b"weights")
target_dir = library_root / "new"
old_mirror = _mirror_dir(centralized, "old")
old_mirror = _mirror_dir(library_root, centralized, "old")
_write_sidecar(
old_mirror, "model", file_path=model, preview_name="model.preview.webp"
)
@@ -230,7 +232,7 @@ async def test_move_model_centralized(
assert moved_model.exists()
assert not model.exists()
new_mirror = _mirror_dir(centralized, "new")
new_mirror = _mirror_dir(library_root, centralized, "new")
assert (new_mirror / "model.metadata.json").exists()
assert (new_mirror / "model.preview.webp").exists()
assert not (old_mirror / "model.metadata.json").exists()
@@ -271,7 +273,7 @@ async def test_rename_known_folder_centralized(
model.write_bytes(b"weights")
old_model_path = old_dir / "model.safetensors"
old_mirror = _mirror_dir(centralized, "oldfolder")
old_mirror = _mirror_dir(library_root, centralized, "oldfolder")
_write_sidecar(
old_mirror, "model", file_path=old_model_path, preview_name="model.preview.webp"
)
@@ -295,7 +297,7 @@ async def test_rename_known_folder_centralized(
assert changed is True
new_mirror = _mirror_dir(centralized, "newfolder")
new_mirror = _mirror_dir(library_root, centralized, "newfolder")
assert (new_mirror / "model.metadata.json").exists()
assert (new_mirror / "model.preview.webp").exists()
assert not old_mirror.exists()
@@ -315,7 +317,7 @@ async def test_rename_known_folder_centralized(
async def test_pending_models_mirror_walk(library_root: Path, centralized: Path):
model = library_root / "model.safetensors"
model.write_bytes(b"weights")
mirror = _mirror_dir(centralized)
mirror = _mirror_dir(library_root, centralized)
_write_sidecar(
mirror,
"model",
@@ -346,7 +348,7 @@ async def test_pending_models_mirror_walk_uses_stem_fallback(
model = library_root / "model.safetensors"
model.write_bytes(b"weights")
mirror = _mirror_dir(centralized)
mirror = _mirror_dir(library_root, centralized)
_write_sidecar(
mirror,
"model",
@@ -12,6 +12,7 @@ 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 root_mirror_component
def _normalize(path) -> str:
@@ -52,11 +53,12 @@ def _set_mode(mode: str) -> None:
get_settings_manager().set("sidecar_storage_mode", mode)
def _mirror_dir(sidecar_root: Path, *rel: str) -> Path:
def _mirror_dir(library_root: Path, sidecar_root: Path, *rel: str) -> Path:
"""Expected mirror directory for a library-relative path."""
library = get_settings_manager().get_active_library_name()
return sidecar_root.joinpath(library, "loras", *rel)
component = root_mirror_component(str(library_root))
return sidecar_root.joinpath(library, component, *rel)
def _write_model(directory: Path, stem: str) -> Path:
@@ -152,7 +154,7 @@ async def test_migrate_to_centralized_moves_sidecar_and_previews(
assert summary["conflicts"] == 0
assert summary["errors"] == []
mirror = _mirror_dir(sidecar_root, "sub")
mirror = _mirror_dir(library_root, sidecar_root, "sub")
assert not sidecar.exists()
assert not preview.exists()
assert not extra_preview.exists()
@@ -186,7 +188,7 @@ async def test_migrate_to_alongside_reverses_layout(
):
_set_mode("alongside")
model = _write_model(library_root / "sub", "model")
mirror = _mirror_dir(sidecar_root, "sub")
mirror = _mirror_dir(library_root, sidecar_root, "sub")
sidecar = _write_sidecar(mirror, "model", model)
preview = mirror / "model.preview.webp"
preview.write_bytes(b"preview")
@@ -221,7 +223,7 @@ async def test_migrate_conflict_keeps_newer_file(
library_root: Path, sidecar_root: Path
):
_set_mode("centralized")
mirror = _mirror_dir(sidecar_root)
mirror = _mirror_dir(library_root, sidecar_root)
# Model A: destination (mirror) sidecar is newer -> destination wins.
model_a = _write_model(library_root, "model_a")
@@ -384,6 +386,121 @@ async def test_migrate_reconcile_survives_per_model_errors(
# The healthy model's cache entry is still reconciled and persisted.
ok_entry = use_case._test_scanner._cache.raw_data[0]
assert ok_entry["preview_url"] == _normalize(
_mirror_dir(sidecar_root) / "ok.preview.webp"
_mirror_dir(library_root, sidecar_root) / "ok.preview.webp"
)
assert use_case._test_scanner.persist_calls == 1
@pytest.mark.asyncio
async def test_migrate_covers_mixed_case_and_example_previews(
library_root: Path, sidecar_root: Path
):
"""Previews like model.WEBP / model.example.0.jpeg migrate too (#225 compat)."""
_set_mode("centralized")
model = _write_model(library_root, "model")
_write_sidecar(library_root, "model", model, preview_ext=".preview.WEBP")
(library_root / "model.preview.WEBP").write_bytes(b"preview")
(library_root / "model.example.0.jpeg").write_bytes(b"example")
use_case = _make_use_case([str(model)])
summary = await use_case.migrate_to_centralized(force=True)
assert summary["success"] is True
assert summary["moved"] == 3 # sidecar + 2 previews
mirror = _mirror_dir(library_root, sidecar_root)
assert (mirror / "model.preview.WEBP").exists()
assert (mirror / "model.example.0.jpeg").exists()
assert not (library_root / "model.preview.WEBP").exists()
assert not (library_root / "model.example.0.jpeg").exists()
metadata = json.loads((mirror / "model.metadata.json").read_text(encoding="utf-8"))
assert metadata["preview_url"] == _normalize(mirror / "model.preview.WEBP")
@pytest.mark.asyncio
async def test_migrate_root_relocates_tree_and_reconciles(
library_root: Path, sidecar_root: Path, tmp_path: Path
):
_set_mode("centralized")
library = get_settings_manager().get_active_library_name()
component = root_mirror_component(str(library_root))
# Assets under the OLD root, mirroring the layout.
old_root = tmp_path / "old_sidecars"
old_mirror = old_root / library / component / "sub"
old_mirror.mkdir(parents=True)
model = _write_model(library_root / "sub", "model")
payload = {
"file_name": "model",
"file_path": _normalize(model),
"preview_url": _normalize(old_mirror / "model.preview.png"),
}
(old_mirror / "model.metadata.json").write_text(json.dumps(payload), encoding="utf-8")
(old_mirror / "model.preview.png").write_bytes(b"preview")
entries = [
{
"file_path": str(model),
"preview_url": _normalize(old_mirror / "model.preview.png"),
"preview_nsfw_level": 2,
}
]
use_case = _make_use_case_with_entries(entries)
summary = await use_case.migrate_root(str(old_root), force=True)
assert summary["success"] is True
assert summary["moved"] == 2
new_mirror = sidecar_root / library / component / "sub"
assert (new_mirror / "model.metadata.json").exists()
assert (new_mirror / "model.preview.png").exists()
# Sidecar preview_url rewritten onto the new root.
migrated = json.loads((new_mirror / "model.metadata.json").read_text(encoding="utf-8"))
assert migrated["preview_url"] == _normalize(new_mirror / "model.preview.png")
# Model path fields untouched — model files never move.
assert migrated["file_path"] == _normalize(model)
# Scanner cache preview URLs repointed and persisted.
entry = use_case._test_scanner._cache.raw_data[0]
assert entry["preview_url"] == _normalize(new_mirror / "model.preview.png")
assert use_case._test_scanner.persist_calls == 1
# Emptied old tree pruned.
assert not old_root.exists()
@pytest.mark.asyncio
async def test_migrate_root_guards(
library_root: Path, sidecar_root: Path, tmp_path: Path
):
_set_mode("centralized")
use_case = _make_use_case([])
summary = await use_case.migrate_root("")
assert summary["success"] is False
assert "old_root is required" in summary["error"]
summary = await use_case.migrate_root(str(sidecar_root))
assert summary["success"] is False
assert "matches the configured" in summary["error"]
_set_mode("alongside")
summary = await use_case.migrate_root(str(tmp_path / "old_sidecars"))
assert summary["success"] is False
assert "not centralized" in summary["error"]
@pytest.mark.asyncio
async def test_migrate_root_missing_old_tree_is_noop(
library_root: Path, sidecar_root: Path, tmp_path: Path
):
_set_mode("centralized")
use_case = _make_use_case([])
summary = await use_case.migrate_root(str(tmp_path / "nonexistent"), force=True)
assert summary["success"] is True
assert summary["moved"] == 0
+39 -4
View File
@@ -22,6 +22,7 @@ from py.utils.sidecar_paths import (
resolve_centralized_dir,
resolve_centralized_dir_for_dir,
resolve_metadata_path,
root_mirror_component,
sanitize_path_component,
)
@@ -117,16 +118,48 @@ class TestCentralizedMode:
def test_mirror_layout(self, model_roots: dict, centralized: Path):
model = model_roots["loras"] / "styles" / "anime" / "model.safetensors"
library = get_settings_manager().get_active_library_name()
root_component = root_mirror_component(str(model_roots["loras"]))
metadata_path = get_metadata_path(str(model))
expected = os.path.join(
str(centralized), library, "loras", "styles", "anime", "model" + METADATA_SUFFIX
str(centralized), library, root_component, "styles", "anime", "model" + METADATA_SUFFIX
)
assert metadata_path == expected
assert get_preview_dir(str(model)) == os.path.dirname(expected)
assert is_centralized()
def test_same_basename_roots_get_distinct_mirrors(
self, model_roots: dict, centralized: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
):
from py.config import config
# Two roots sharing the basename "loras" must not share a mirror dir.
other_parent = tmp_path / "elsewhere"
other_root = other_parent / "loras"
other_root.mkdir(parents=True)
monkeypatch.setattr(
config,
"loras_roots",
[str(model_roots["loras"]), str(other_root)],
raising=False,
)
model_a = model_roots["loras"] / "model.safetensors"
model_b = other_root / "model.safetensors"
dir_a = resolve_centralized_dir(str(model_a))
dir_b = resolve_centralized_dir(str(model_b))
assert dir_a is not None and dir_b is not None
assert dir_a != dir_b
assert root_mirror_component(str(model_roots["loras"])) != root_mirror_component(
str(other_root)
)
# Same root always maps to the same component (stable hash).
assert root_mirror_component(str(model_roots["loras"])) == root_mirror_component(
str(model_roots["loras"]) + os.sep
)
def test_longest_root_wins(self, model_roots: dict, centralized: Path, monkeypatch: pytest.MonkeyPatch):
from py.config import config
@@ -142,7 +175,7 @@ class TestCentralizedMode:
model = nested / "model.safetensors"
assert get_metadata_path(str(model)) == os.path.join(
str(centralized), library, "nested", "model" + METADATA_SUFFIX
str(centralized), library, root_mirror_component(str(nested)), "model" + METADATA_SUFFIX
)
def test_outside_roots_falls_back_to_alongside(
@@ -174,7 +207,7 @@ class TestCentralizedMode:
library = get_settings_manager().get_active_library_name()
assert resolve_centralized_dir_for_dir(str(model_roots["loras"])) == os.path.join(
str(centralized), library, "loras"
str(centralized), library, root_mirror_component(str(model_roots["loras"]))
)
def test_empty_path_uses_default_sidecar_root(self, model_roots: dict, tmp_path: Path):
@@ -223,7 +256,9 @@ class TestModeIndependentResolution:
assert resolve_centralized_dir_for_dir(str(model_dir)) is None
assert resolve_centralized_dir_for_dir(
str(model_dir), sidecar_root=str(sidecar_root)
) == os.path.join(str(sidecar_root), library, "loras", "sub")
) == os.path.join(
str(sidecar_root), library, root_mirror_component(str(model_roots["loras"])), "sub"
)
class TestSettingsValidation: