diff --git a/py/services/model_file_service.py b/py/services/model_file_service.py index fc5fec27..8768dfeb 100644 --- a/py/services/model_file_service.py +++ b/py/services/model_file_service.py @@ -12,7 +12,10 @@ from ..utils.recipes_paths import path_is_or_contains_recipes_dir from ..utils.sidecar_paths import is_centralized, resolve_centralized_dir_for_dir from ..services.settings_manager import get_settings_manager from ..services.model_lifecycle_service import _require_path_in_library_roots -from ..services.pending_delete_service import PENDING_DELETE_DIR_NAME +from ..services.pending_delete_service import ( + PENDING_DELETE_DIR_NAME, + staging_dir_has_content, +) logger = logging.getLogger(__name__) @@ -881,7 +884,12 @@ class ModelMoveService: excluded_paths = self._excluded_model_paths() for dirpath, dirnames, filenames in os.walk(absolute_path): - if PENDING_DELETE_DIR_NAME in dirnames: + # The container alone means nothing: an empty `.lm-pending-delete` + # is a leftover from a purge, an undo or a failed staging, and + # treating it as pending made the folder undeletable for good. + if PENDING_DELETE_DIR_NAME in dirnames and staging_dir_has_content( + os.path.join(dirpath, PENDING_DELETE_DIR_NAME) + ): pending_delete_job = True for name in dirnames: @@ -1072,9 +1080,15 @@ class ModelMoveService: @staticmethod def _has_pending_delete_job(absolute_path: str) -> bool: - """Return True when a staged-delete batch lives inside the subtree.""" - for _dirpath, dirnames, _filenames in os.walk(absolute_path): - if PENDING_DELETE_DIR_NAME in dirnames: + """Return True when a staged-delete batch lives inside the subtree. + + An empty ``.lm-pending-delete`` container does not count: it is a + leftover, not a batch, and a rename must not be refused because of one. + """ + for dirpath, dirnames, _filenames in os.walk(absolute_path): + if PENDING_DELETE_DIR_NAME in dirnames and staging_dir_has_content( + os.path.join(dirpath, PENDING_DELETE_DIR_NAME) + ): return True return False diff --git a/py/services/pending_delete_service.py b/py/services/pending_delete_service.py index 8d579715..38b77376 100644 --- a/py/services/pending_delete_service.py +++ b/py/services/pending_delete_service.py @@ -55,6 +55,28 @@ MANIFEST_FILE_NAME = "manifest.json" # deleted by the sweep. ORPHANED_SUFFIX = ".orphaned" + +def staging_dir_has_content(directory: str) -> bool: + """Whether a staging directory still holds a batch (or a quarantined one). + + ``.lm-pending-delete`` is only a container: the batch dir inside it is what + carries the staged files, and the purge, the undo and the staging rollback + all remove it. An empty container left behind must not be mistaken for a + staged delete -- it used to make every later folder delete answer "A + deletion is still pending inside this folder" forever, with no undo window + to wait for and nothing in the UI to clear it (issue #1145 follow-up). + + Quarantined batches (``*.orphaned``) are kept on purpose, so anything left + inside still counts. An unreadable directory counts as content too: never + delete over something we could not list. + """ + + try: + return bool(os.listdir(directory)) + except OSError: + return True + + # Map scanner.model_type (singular) to the manifest page type values. _MODEL_TYPE_PAGE_MAP = { "lora": "loras", @@ -201,6 +223,7 @@ class PendingDeleteService: if batch_dir: self._rollback_model_staging(batch_dir, staged_pairs) self._remove_empty_dir(batch_dir) + self._remove_empty_staging_parent(batch_dir) return None except Exception as exc: # defensive - never block the delete flow logger.warning( @@ -266,6 +289,7 @@ class PendingDeleteService: if batch_dir: self._rollback_recipe_staging(batch_dir, staged_pairs) self._remove_empty_dir(batch_dir) + self._remove_empty_staging_parent(batch_dir) return None except Exception as exc: # defensive - never block the delete flow logger.warning( @@ -454,6 +478,7 @@ class PendingDeleteService: # Remove the manifest + batch dir only after all entries restored. self._remove_manifest(batch_dir) self._remove_empty_dir(batch_dir) + self._remove_empty_staging_parent(batch_dir) await self._forget_batch(batch_id) # Clean up merged loser dirs (their staged files were restored # above) and drop them from the registry too. @@ -832,6 +857,7 @@ class PendingDeleteService: continue self._remove_manifest(src) self._remove_empty_dir(src) + self._remove_empty_staging_parent(src) removed.append(os.path.basename(src)) return removed @@ -842,6 +868,13 @@ class PendingDeleteService: manifest = self._read_manifest(batch_dir) if manifest is None: + if not staging_dir_has_content(batch_dir): + # Crash leftover: the batch dir was created but nothing was ever + # staged into it. Quarantining an empty directory would leave a + # container that blocks folder deletion for good. + self._remove_empty_dir(batch_dir) + self._remove_empty_staging_parent(batch_dir) + return True # Corrupted or manifest-less batch: quarantine, NEVER delete the # staged files (they may be the only copy of the user's data). self._quarantine_batch_dir(batch_dir) @@ -885,6 +918,7 @@ class PendingDeleteService: self._remove_manifest(batch_dir) self._remove_empty_dir(batch_dir) + self._remove_empty_staging_parent(batch_dir) self._remove_merged_batch_dirs(manifest) return True @@ -1132,6 +1166,18 @@ class PendingDeleteService: except OSError as exc: logger.debug("Directory %s not empty or missing: %s", directory, exc) + def _remove_empty_staging_parent(self, batch_dir: str) -> None: + """Drop the staging container once its last batch is gone. + + Only an empty ``.lm-pending-delete`` directory is removed (``rmdir`` + refuses anything else), and only when the path really is one, so a model + folder is never at risk. + """ + parent = os.path.dirname(batch_dir) + if os.path.basename(parent) != PENDING_DELETE_DIR_NAME: + return + self._remove_empty_dir(parent) + def _new_batch_id(self) -> str: return uuid.uuid4().hex diff --git a/tests/services/test_model_file_service.py b/tests/services/test_model_file_service.py index 47291a5b..fa2b32b0 100644 --- a/tests/services/test_model_file_service.py +++ b/tests/services/test_model_file_service.py @@ -359,7 +359,9 @@ async def test_delete_folder_success_manifest_carries_excluded_model_count(tmp_p @pytest.mark.asyncio async def test_delete_folder_refuses_while_a_staged_delete_is_pending(tmp_path: Path): target = _make_nested(tmp_path) - (target / ".lm-pending-delete").mkdir() + batch = target / ".lm-pending-delete" / "batch1" + batch.mkdir(parents=True) + (batch / "manifest.json").write_text("{}", encoding="utf-8") scanner = FakeScanner([tmp_path]) service = ModelMoveService(scanner, "lora") @@ -370,6 +372,43 @@ async def test_delete_folder_refuses_while_a_staged_delete_is_pending(tmp_path: assert target.exists() +@pytest.mark.asyncio +async def test_delete_folder_ignores_an_empty_staging_container(tmp_path: Path): + """An empty `.lm-pending-delete` is a leftover, not a pending deletion. + + Every purge, undo and failed staging used to leave the container behind, and + the name match alone made the folder undeletable for good: "A deletion is + still pending inside this folder. Wait for the undo window to expire." with + no window left to wait for (issue #1145 follow-up). + """ + target = _make_nested(tmp_path) + (target / ".lm-pending-delete").mkdir() + scanner = FakeScanner([tmp_path]) + service = ModelMoveService(scanner, "lora") + + result = await service.delete_folder(str(target)) + + assert result["success"] is True + assert not target.exists() + + +@pytest.mark.asyncio +async def test_delete_folder_refuses_over_a_quarantined_batch(tmp_path: Path): + """Quarantine preserves the user's staged files, so it still blocks.""" + target = _make_nested(tmp_path) + quarantined = target / ".lm-pending-delete" / "batch1.orphaned" + quarantined.mkdir(parents=True) + (quarantined / "model.safetensors").write_bytes(b"user-data") + scanner = FakeScanner([tmp_path]) + service = ModelMoveService(scanner, "lora") + + result = await service.delete_folder(str(target)) + + assert result["success"] is False + assert result["code"] == "busy" + assert (quarantined / "model.safetensors").read_bytes() == b"user-data" + + @pytest.mark.asyncio async def test_delete_folder_refuses_the_library_root_itself(tmp_path: Path): scanner = FakeScanner([tmp_path]) @@ -669,7 +708,9 @@ async def test_rename_folder_refuses_symlinked_directory(tmp_path: Path): @pytest.mark.asyncio async def test_rename_folder_refuses_while_a_staged_delete_is_pending(tmp_path: Path): target = _make_nested(tmp_path) - (target / ".lm-pending-delete").mkdir() + batch = target / ".lm-pending-delete" / "batch1" + batch.mkdir(parents=True) + (batch / "manifest.json").write_text("{}", encoding="utf-8") scanner = FakeScanner([tmp_path]) service = ModelMoveService(scanner, "lora") @@ -681,6 +722,20 @@ async def test_rename_folder_refuses_while_a_staged_delete_is_pending(tmp_path: assert scanner.renamed_folders == [] +@pytest.mark.asyncio +async def test_rename_folder_ignores_an_empty_staging_container(tmp_path: Path): + """The leftover container must not block a rename either.""" + target = _make_nested(tmp_path) + (target / ".lm-pending-delete").mkdir() + scanner = FakeScanner([tmp_path]) + service = ModelMoveService(scanner, "lora") + + result = await service.rename_folder(str(target), "animation") + + assert result["success"] is True + assert scanner.renamed_folders + + @pytest.mark.asyncio async def test_rename_folder_requires_path(tmp_path: Path): service = ModelMoveService(FakeScanner([tmp_path]), "lora") diff --git a/tests/services/test_pending_delete_service.py b/tests/services/test_pending_delete_service.py index 865bfbd3..76942b12 100644 --- a/tests/services/test_pending_delete_service.py +++ b/tests/services/test_pending_delete_service.py @@ -2298,3 +2298,89 @@ async def test_symlink5_cross_volume_merge_then_one_undo_restores_everything( assert not (root_b / PENDING_DELETE_DIR_NAME / bid_b).exists() staging = root_a / PENDING_DELETE_DIR_NAME assert not staging.exists() or not any(staging.iterdir()) + + +# --------------------------------------------------------------------------- +# (n) the staging CONTAINER is dropped with its last batch (issue #1145) +# --------------------------------------------------------------------------- +async def test_n_purge_removes_the_empty_staging_container( + tmp_path: Path, monkeypatch +) -> None: + """`.lm-pending-delete` is a container, not a record of a pending delete. + + Leaving it behind made every later folder delete (and rename) answer "A + deletion is still pending inside this folder" forever, because the folder + manifest only matched the directory name. + """ + root = tmp_path / "loras" + root.mkdir() + await _register_model_root(monkeypatch, lora_roots=[root]) + + service = await PendingDeleteService.get_instance() + batch_id = await _stage_simple(service, root, "expired") + + manifest_path = root / PENDING_DELETE_DIR_NAME / batch_id / "manifest.json" + manifest = json.loads(manifest_path.read_text(encoding="utf-8")) + manifest["expires_at"] = int(time.time()) - 10 + manifest_path.write_text(json.dumps(manifest)) + + await service.purge_expired() + + assert not (root / PENDING_DELETE_DIR_NAME).exists() + + +async def test_n_undo_removes_the_empty_staging_container( + tmp_path: Path, monkeypatch +) -> None: + root = tmp_path / "loras" + root.mkdir() + await _register_model_root(monkeypatch, lora_roots=[root]) + + service = await PendingDeleteService.get_instance() + batch_id = await _stage_simple(service, root, "restored") + + await service.undo(batch_id) + + assert (root / "restored.safetensors").exists() + assert not (root / PENDING_DELETE_DIR_NAME).exists() + + +async def test_n_container_survives_while_another_batch_is_staged( + tmp_path: Path, monkeypatch +) -> None: + """Only the last batch out drops the container.""" + root = tmp_path / "loras" + root.mkdir() + await _register_model_root(monkeypatch, lora_roots=[root]) + + service = await PendingDeleteService.get_instance() + expired_id = await _stage_simple(service, root, "expired") + fresh_id = await _stage_simple(service, root, "fresh") + + manifest_path = root / PENDING_DELETE_DIR_NAME / expired_id / "manifest.json" + manifest = json.loads(manifest_path.read_text(encoding="utf-8")) + manifest["expires_at"] = int(time.time()) - 10 + manifest_path.write_text(json.dumps(manifest)) + + await service.purge_expired() + + staging = root / PENDING_DELETE_DIR_NAME + assert staging.is_dir() + assert not (staging / expired_id).exists() + assert (staging / fresh_id).exists() + + +async def test_n_empty_manifestless_batch_is_dropped_not_quarantined( + tmp_path: Path, monkeypatch +) -> None: + """A crash between makedirs and the manifest leaves nothing to preserve.""" + root = tmp_path / "loras" + root.mkdir() + staging = root / PENDING_DELETE_DIR_NAME + (staging / "batch1").mkdir(parents=True) + await _register_model_root(monkeypatch, lora_roots=[root]) + + service = await PendingDeleteService.get_instance() + await service.purge_expired(scan_roots=True) + + assert not staging.exists()