From 0c00ee22fc0a62a2604d2ae3f7a3f0028b1b0455 Mon Sep 17 00:00:00 2001 From: Will Miao Date: Tue, 11 Aug 2026 18:48:03 +0800 Subject: [PATCH] fix(delete): stage model deletes into the model folder (avoid EXDEV) --- py/services/pending_delete_service.py | 17 +- tests/services/test_pending_delete_service.py | 187 ++++++++++++++++++ 2 files changed, 197 insertions(+), 7 deletions(-) diff --git a/py/services/pending_delete_service.py b/py/services/pending_delete_service.py index 4267d480..dd19c102 100644 --- a/py/services/pending_delete_service.py +++ b/py/services/pending_delete_service.py @@ -45,8 +45,8 @@ logger = logging.getLogger(__name__) # Undo window in seconds before a staged batch becomes purge-eligible. PENDING_DELETE_TTL_SECONDS = 30 -# Hidden staging directory name placed under each model root (and the settings -# dir for recipes). +# Hidden staging directory name placed inside each deleted model's own folder +# (sibling of the model artifacts) and under the settings dir for recipes. PENDING_DELETE_DIR_NAME = ".lm-pending-delete" # Manifest file name inside every batch directory. MANIFEST_FILE_NAME = "manifest.json" @@ -116,11 +116,14 @@ class PendingDeleteService: original_file_path: str, cached_entry: Optional[Dict[str, Any]], ) -> Optional[str]: - """Rename a model's artifacts into a per-root staging batch. + """Rename a model's artifacts into a sibling-of-model staging batch. - Returns the batch id, or ``None`` when undo is disabled, the staging - root cannot be resolved, or staging failed (caller falls back to a - hard delete). + The batch dir is created inside the model file's OWN directory + (``target_dir``), so staging/undo renames stay within one real + directory - EXDEV is impossible even when the business path traverses + nested symlinks to other volumes. Returns the batch id, or ``None`` + when undo is disabled, the model root cannot be resolved, or staging + failed (caller falls back to a hard delete). """ # LOCK-FREE section: opportunistic purge must never run while holding # the ops lock (the lock is not re-entrant). @@ -153,7 +156,7 @@ class PendingDeleteService: batch_id = self._new_batch_id() batch_dir = os.path.join( - os.path.join(root, PENDING_DELETE_DIR_NAME), batch_id + os.path.abspath(target_dir), PENDING_DELETE_DIR_NAME, batch_id ) os.makedirs(batch_dir, exist_ok=True) diff --git a/tests/services/test_pending_delete_service.py b/tests/services/test_pending_delete_service.py index f45d86ce..c525f920 100644 --- a/tests/services/test_pending_delete_service.py +++ b/tests/services/test_pending_delete_service.py @@ -1864,3 +1864,190 @@ async def test_reg_j_startup_sweep_passes_scan_roots_true( # The startup sweep must invoke purge_expired with scan_roots=True (the # reconciliation flag) - forgetting it would break restart cleanup. assert sweep_calls == [{"scan_roots": True}] + + +# --------------------------------------------------------------------------- +# Todo 2: SIBLING-OF-MODEL STAGING (model file in a SUBDIR of the scanner root) +# --------------------------------------------------------------------------- + +# (a) staging lands in /.lm-pending-delete/, NOT under the +# scanner root - manifest entries' staged paths live under the sibling dir. +async def test_sibling1_stage_model_in_subdir_uses_sibling_dir(tmp_path: Path) -> None: + root = tmp_path / "loras" + root.mkdir() + sub = root / "nested" + sub.mkdir() + model = sub / "model.safetensors" + model.write_bytes(b"sibling-data") + metadata = sub / "model.metadata.json" + metadata.write_bytes(b"{}") + + service = await PendingDeleteService.get_instance() + batch_id = await service.stage_model_delete( + scanner=ScannerForStage([root]), + target_dir=str(sub), + file_name="model", + main_extension=".safetensors", + original_file_path=str(model), + cached_entry=None, + ) + assert batch_id is not None + + sibling_dir = sub / PENDING_DELETE_DIR_NAME / batch_id + assert sibling_dir.is_dir() + # The OLD location (under the scanner root) must NOT be created. + assert not (root / PENDING_DELETE_DIR_NAME).exists() + + manifest = json.loads((sibling_dir / "manifest.json").read_text(encoding="utf-8")) + assert len(manifest["entries"]) == 2 + for entry in manifest["entries"]: + assert str(entry["staged"]).startswith(str(sibling_dir)) + assert (sibling_dir / "model.safetensors").read_bytes() == b"sibling-data" + assert (sibling_dir / "model.metadata.json").exists() + assert not model.exists() + assert not metadata.exists() + + +# (b) undo of a sibling-staged batch restores the files byte-identically. +async def test_sibling2_undo_restores_byte_identically(tmp_path: Path) -> None: + root = tmp_path / "loras" + root.mkdir() + sub = root / "nested" + sub.mkdir() + model = sub / "model.safetensors" + model.write_bytes(b"payload-1") + preview = sub / "model.preview.png" + preview.write_bytes(b"payload-2") + + service = await PendingDeleteService.get_instance() + batch_id = await service.stage_model_delete( + scanner=ScannerForStage([root]), + target_dir=str(sub), + file_name="model", + main_extension=".safetensors", + original_file_path=str(model), + cached_entry=None, + ) + assert batch_id is not None + sibling_dir = sub / PENDING_DELETE_DIR_NAME / batch_id + assert sibling_dir.is_dir() + assert not model.exists() + assert not preview.exists() + + await service.undo(batch_id) + + assert model.read_bytes() == b"payload-1" + assert preview.read_bytes() == b"payload-2" + assert not sibling_dir.exists() + assert batch_id not in service._known_batch_dirs + + +# (c) ROOT GATING: _find_model_root -> None skips staging entirely. +async def test_sibling3_root_gating_skips_staging(tmp_path: Path, monkeypatch) -> None: + root = tmp_path / "loras" + root.mkdir() + model = root / "model.safetensors" + model.write_bytes(b"keep-me") + + service = await PendingDeleteService.get_instance() + monkeypatch.setattr(service, "_find_model_root", lambda _scanner, _path: None) + + batch_id = await service.stage_model_delete( + scanner=ScannerForStage([root]), + target_dir=str(root), + file_name="model", + main_extension=".safetensors", + original_file_path=str(model), + cached_entry=None, + ) + + assert batch_id is None + assert model.read_bytes() == b"keep-me" + assert not (root / PENDING_DELETE_DIR_NAME).exists() + + +# QA scenario: simulated OSError on the 2nd artifact during sibling staging -> +# rollback renames the 1st back, returns None, and leaves no orphaned sibling +# batch dir behind. +async def test_sibling4_staging_oserror_rolls_back_sibling_dir( + tmp_path: Path, monkeypatch +) -> None: + root = tmp_path / "loras" + root.mkdir() + sub = root / "nested" + sub.mkdir() + a = sub / "model.safetensors" + a.write_bytes(b"a-bytes") + b = sub / "model.metadata.json" + b.write_bytes(b"b-bytes") + + service = await PendingDeleteService.get_instance() + + real_rename = os.rename + calls = {"n": 0} + + def flaky_rename(src: str, dst: str) -> None: + calls["n"] += 1 + if calls["n"] == 2: + raise OSError("simulated sibling staging failure") + return real_rename(src, dst) + + monkeypatch.setattr("py.services.pending_delete_service.os.rename", flaky_rename) + + batch_id = await service.stage_model_delete( + scanner=ScannerForStage([root]), + target_dir=str(sub), + file_name="model", + main_extension=".safetensors", + original_file_path=str(a), + cached_entry=None, + ) + + assert batch_id is None + # Both artifacts rolled back; no orphaned sibling batch dir holds data. + assert a.read_bytes() == b"a-bytes" + assert b.read_bytes() == b"b-bytes" + sibling = sub / PENDING_DELETE_DIR_NAME + if sibling.exists(): + assert not any(sibling.iterdir()) + + +# (d) SCANNER EXCLUSION at a NESTED staging dir: a model staged into +# /sub/.lm-pending-delete is excluded from the walk just like the +# root-level one (depth independence). +async def test_p_model_walk_excludes_nested_staging_dir( + tmp_path: Path, monkeypatch +) -> None: + root = tmp_path / "loras" + root.mkdir() + (root / "normal.safetensors").write_bytes(b"normal") + sub = root / "sub" + sub.mkdir() + (sub / "real.safetensors").write_bytes(b"real") + nested_staging = sub / PENDING_DELETE_DIR_NAME / "x" + nested_staging.mkdir(parents=True) + (nested_staging / "model.safetensors").write_bytes(b"ghost") + (nested_staging / "model.metadata.json").write_bytes(b'{"hash_status": "pending"}') + # Root-level staging dir for comparison. + root_staging = root / PENDING_DELETE_DIR_NAME / "y" + root_staging.mkdir(parents=True) + (root_staging / "ghost2.safetensors").write_bytes(b"ghost2") + + from py.services import model_scanner as model_scanner_module + + async def _noop_register(*_args: Any, **_kwargs: Any) -> None: + return None + + monkeypatch.setattr(model_scanner_module.ServiceRegistry, "register_service", _noop_register) + monkeypatch.setenv("LORA_MANAGER_DISABLE_PERSISTENT_CACHE", "1") + + scanner = DummyScannerForWalk(root) + + result = await scanner._gather_model_data() + paths = [entry["file_path"] for entry in result.raw_data] + assert not any(PENDING_DELETE_DIR_NAME in p for p in paths) + # Real files at both depths are still discovered. + assert any(p.endswith("normal.safetensors") for p in paths) + assert any(p.endswith("sub/real.safetensors") for p in paths) + + assert scanner._count_model_files() == 2