diff --git a/src/server/services/anime_service.py b/src/server/services/anime_service.py index 04d44a5..710a248 100644 --- a/src/server/services/anime_service.py +++ b/src/server/services/anime_service.py @@ -1649,12 +1649,29 @@ class AnimeService: Returns: DeleteSeriesResult with success status, what was deleted, errors + + Deletion order: filesystem first, database second. + + This order matters: if the folder delete fails (e.g. permission + error, path outside the configured anime directory), the database + row is preserved so the user can retry the delete once the + underlying issue is resolved. If we deleted the database row + first, an orphan folder would be left on disk with no way to + clean it up through the normal delete flow. + + Orphan folder recovery: when ``delete_folder=True`` is requested + for a series whose database row no longer exists, the configured + anime directory is scanned for a folder that uniquely matches + the key. This recovers the case where a previous delete with + ``delete_database=True`` succeeded but ``delete_folder=True`` + silently failed, leaving the folder on disk. """ from src.server.database.connection import get_db_session from src.server.database.service import AnimeSeriesService from src.server.models.anime import DeleteSeriesResult from src.server.utils.filesystem import is_safe_path import os as _os + import re import shutil logger.info( @@ -1679,13 +1696,43 @@ class AnimeService: message="At least one of delete_database or delete_folder must be True.", ) - # Single DB session for fetch + optional delete + # Look up the series in the DB to get its folder path + series = None async with get_db_session() as db: series = await AnimeSeriesService.get_by_key(db, key) - if not series: - logger.warning( - "Delete series failed - not found: key=%s", key - ) + + if not series: + logger.warning( + "Delete series - row not found in DB: key=%s delete_folder=%s", + key, delete_folder, + ) + # Recovery path: if the user wants to delete the folder but the + # DB row is already gone (e.g. orphaned by a previous partial + # delete), scan the configured anime directory for a folder + # that uniquely matches this key and delete it. + if delete_folder: + folder_path = self._find_orphan_folder_for_key(key) + if folder_path: + logger.info( + "Orphan folder recovery: key=%s matched folder=%s", + key, folder_path, + ) + result = DeleteSeriesResult( + success=True, + key=key, + name="", + folder_path=folder_path, + message="", + ) + self._delete_folder_at_path(folder_path, key, result) + # No DB row to delete; build message and return + self._build_delete_message(result) + logger.info( + "Delete series completed (orphan recovery): key=%s " + "deleted_folder=%s folder_error=%s", + key, result.deleted_folder, result.folder_error, + ) + return result return DeleteSeriesResult( success=False, key=key, @@ -1694,13 +1741,33 @@ class AnimeService: deleted_from_database=False, deleted_folder=False, database_error=None, - folder_error=None, - message=f"Series '{key}' not found.", + folder_error=( + f"Series '{key}' not found in database, and no folder " + "matching this key was found in the anime directory. " + "Nothing to delete." + ), + message=( + f"Series '{key}' not found. If the folder on disk is " + "still required to be removed, please specify its " + "exact name on the filesystem." + ), ) + # No row, no folder requested — nothing to do + return DeleteSeriesResult( + success=False, + key=key, + name="", + folder_path=None, + deleted_from_database=False, + deleted_folder=False, + database_error=None, + folder_error=None, + message=f"Series '{key}' not found.", + ) - series_id = series.id - series_name = series.name - folder_path = series.folder + series_id = series.id + series_name = series.name + folder_path = series.folder result = DeleteSeriesResult( success=True, @@ -1710,7 +1777,22 @@ class AnimeService: message="", ) - # --- Database deletion --- + # --- Filesystem deletion (do FIRST so a failure preserves the DB row) --- + if delete_folder and folder_path: + self._delete_folder_at_path(folder_path, key, result) + # If folder delete was requested but failed, abort before + # removing the DB row so the user can retry. + if not result.deleted_folder and result.folder_error: + result.success = False + self._build_delete_message(result) + logger.warning( + "Delete series aborted - folder delete failed; DB row preserved: " + "key=%s folder_error=%s", + key, result.folder_error, + ) + return result + + # --- Database deletion (do AFTER folder delete) --- if delete_database: try: async with get_db_session() as db: @@ -1747,62 +1829,153 @@ class AnimeService: key, exc, ) - # --- Filesystem deletion --- - if delete_folder and folder_path: - # Resolve absolute path and validate it is within base directory. - # - # Important: `folder_path` stored in the database is the relative - # folder name (e.g. "Beyblade Burst (2016)"), not an absolute path. - # If we feed a relative path to os.path.abspath() it gets joined - # against the process's current working directory — which may be - # /app inside the container while the anime directory is /data, - # producing e.g. "/app/Beyblade Burst (2016)" and tripping the - # safe-path check below for what is actually a valid deletion. - # Resolve relative paths against the configured anime directory - # so the safety check operates on the real intended target. - base_dir = _os.path.abspath(self._directory) - if _os.path.isabs(folder_path): - abs_folder = _os.path.abspath(folder_path) - else: - abs_folder = _os.path.abspath(_os.path.join(base_dir, folder_path)) - - if not is_safe_path(base_dir, abs_folder): - logger.warning( - "Blocked unsafe folder delete attempt: key=%s path=%s base=%s", - key, abs_folder, base_dir, - ) - result.folder_error = ( - f"Path '{abs_folder}' is outside the anime directory " - f"'{base_dir}' and will not be deleted." - ) - result.success = False - elif not _os.path.isdir(abs_folder): - logger.warning( - "Delete folder skipped - path does not exist: key=%s path=%s", - key, abs_folder, - ) - # Not an error; folder might never have existed - else: - try: - logger.info( - "Deleting series folder: key=%s path=%s", - key, abs_folder, - ) - shutil.rmtree(abs_folder) - logger.info( - "Deleted series folder: key=%s path=%s", - key, abs_folder, - ) - result.deleted_folder = True - except Exception as exc: - logger.error( - "Failed to delete series folder: key=%s path=%s error=%s", - key, abs_folder, str(exc), - ) - result.folder_error = str(exc) - result.success = False - # --- Build message --- + self._build_delete_message(result) + + logger.info( + "Delete series completed: key=%s deleted_db=%s deleted_folder=%s", + key, result.deleted_from_database, result.deleted_folder, + ) + return result + + def _delete_folder_at_path(self, folder_path, key, result): + """Resolve ``folder_path`` against the configured anime directory + and attempt to remove it. Updates ``result`` in place. + + Resolves relative paths against ``self._directory`` so the safety + check operates on the real intended target (the process's current + working directory is not used as the base; in containers CWD may + differ from the anime directory, e.g. /app vs /data). + """ + import os as _os + import shutil + + # Resolve absolute path and validate it is within base directory. + # + # Important: `folder_path` stored in the database is the relative + # folder name (e.g. "Beyblade Burst (2016)"), not an absolute path. + # If we feed a relative path to os.path.abspath() it gets joined + # against the process's current working directory — which may be + # /app inside the container while the anime directory is /data, + # producing e.g. "/app/Beyblade Burst (2016)" and tripping the + # safe-path check below for what is actually a valid deletion. + # Resolve relative paths against the configured anime directory + # so the safety check operates on the real intended target. + from src.server.utils.filesystem import is_safe_path + + base_dir = _os.path.abspath(self._directory) + if _os.path.isabs(folder_path): + abs_folder = _os.path.abspath(folder_path) + else: + abs_folder = _os.path.abspath(_os.path.join(base_dir, folder_path)) + + if not is_safe_path(base_dir, abs_folder): + logger.warning( + "Blocked unsafe folder delete attempt: key=%s path=%s base=%s", + key, abs_folder, base_dir, + ) + result.folder_error = ( + f"Path '{abs_folder}' is outside the anime directory " + f"'{base_dir}' and will not be deleted." + ) + result.success = False + return + if not _os.path.isdir(abs_folder): + logger.warning( + "Delete folder skipped - path does not exist: key=%s path=%s", + key, abs_folder, + ) + # Not an error; folder might never have existed + return + try: + logger.info( + "Deleting series folder: key=%s path=%s", + key, abs_folder, + ) + shutil.rmtree(abs_folder) + logger.info( + "Deleted series folder: key=%s path=%s", + key, abs_folder, + ) + result.deleted_folder = True + except Exception as exc: + logger.error( + "Failed to delete series folder: key=%s path=%s error=%s", + key, abs_folder, str(exc), + ) + result.folder_error = str(exc) + result.success = False + + def _find_orphan_folder_for_key(self, key: str): + """Locate a folder under ``self._directory`` that uniquely matches + the given series ``key``. + + Used as a recovery path when the DB row is gone but the on-disk + folder still exists (orphaned by a previous partial delete). + + Matching strategy: for each immediate subdirectory of the anime + directory, strip a trailing ``(YYYY)`` year suffix if present and + then compare the normalized form (lowercased, non-alphanumerics + removed, key's hyphens treated as separators) against the key. + Returns the folder name (relative to the anime directory) of the + unique match, or ``None`` if zero or multiple folders match. + + Returns: + The matching relative folder name, or None when no unique + match exists. Returning None is the safe default — it forces + the caller to surface an explicit error rather than risk + deleting the wrong folder. + """ + import os as _os + import re + + if not self._directory or not _os.path.isdir(self._directory): + return None + + def _normalize(value: str) -> str: + # Drop an optional trailing "(YYYY)" or "(YYYY)"-with-content + # suffix the user might have added for disambiguation. We only + # strip a single trailing parenthesised group to avoid eating + # legitimate parts of the title. + value = re.sub(r"\s*\([^)]*\)\s*$", "", value or "") + # Lowercase, replace hyphens/underscores with empty so they + # line up with the way the key is constructed. + lowered = value.lower().replace("-", "").replace("_", "") + # Keep only alphanumerics (which preserves CJK characters + # because \w in unicode mode includes them; using explicit + # alphanumerics is safer cross-platform). + return re.sub(r"[^0-9a-z\u00C0-\uFFFF]", "", lowered) + + target = _normalize(key) + if not target: + return None + + candidates = [] + try: + entries = _os.listdir(self._directory) + except OSError: + return None + + for entry in entries: + full = _os.path.join(self._directory, entry) + if not _os.path.isdir(full): + continue + if _normalize(entry) == target: + candidates.append(entry) + + if len(candidates) == 1: + return candidates[0] + if len(candidates) > 1: + logger.warning( + "Orphan folder recovery: ambiguous match for key=%s " + "found %d candidate folders: %s", + key, len(candidates), candidates, + ) + return None + + @staticmethod + def _build_delete_message(result) -> None: + """Assemble the human-readable ``result.message`` from flags/errors.""" parts = [] if result.deleted_from_database and not result.database_error: parts.append("removed from database") @@ -1818,12 +1991,6 @@ class AnimeService: else: result.message = "No action taken." - logger.info( - "Delete series completed: key=%s deleted_db=%s deleted_folder=%s", - key, result.deleted_from_database, result.deleted_folder, - ) - return result - async def _broadcast_series_deleted(self, key: str, name: str) -> None: """Broadcast series_deleted event via WebSocket.""" try: diff --git a/tests/unit/test_delete_anime_service.py b/tests/unit/test_delete_anime_service.py index 68a6267..e5fbfea 100644 --- a/tests/unit/test_delete_anime_service.py +++ b/tests/unit/test_delete_anime_service.py @@ -581,3 +581,190 @@ class TestDeleteSeriesService: key="test-key", name="Test Series", ) + + # ------------------------------------------------------------------ + # Deletion order: filesystem first, database second + # ------------------------------------------------------------------ + + @pytest.mark.asyncio + async def test_delete_series_db_preserved_when_folder_fails( + self, anime_service, tmp_path + ): + """When both flags are True and folder delete fails, the DB row + is preserved so the user can retry after fixing the underlying issue. + + Previously, the database row was deleted first and the folder + second. If the folder delete failed (e.g. the old CWD-relative-path + bug, or any future permission/path error), the row was already + gone — leaving an orphan folder on disk that could not be cleaned + up through the normal delete flow. + """ + # Folder exists but we'll force shutil.rmtree to fail + series_folder = tmp_path / "Test Series" + series_folder.mkdir() + anime_service._directory = str(tmp_path) + + mock_session = AsyncMock() + mock_ctx = _make_db_ctx(mock_session) + + mock_series = MagicMock() + mock_series.key = "test-key" + mock_series.name = "Test Series" + mock_series.folder = "Test Series" + mock_series.id = 7 + + db_delete_mock = AsyncMock(return_value=True) + + with patch( + "src.server.database.connection.get_db_session", + return_value=mock_ctx, + ), patch( + "src.server.database.service.AnimeSeriesService.get_by_key", + new_callable=AsyncMock, + return_value=mock_series, + ), patch( + "src.server.database.service.AnimeSeriesService.delete", + new_callable=AsyncMock, + side_effect=db_delete_mock, + ), patch( + "shutil.rmtree", + side_effect=OSError("Permission denied"), + ): + result = await anime_service.delete_series( + key="test-key", + delete_database=True, + delete_folder=True, + ) + + # Folder delete failed → DB row MUST be preserved + assert result.deleted_folder is False + assert result.folder_error is not None + assert result.deleted_from_database is False, ( + "DB row was deleted despite folder delete failure — " + "user would lose ability to retry the delete" + ) + db_delete_mock.assert_not_called() + assert result.success is False + + # ------------------------------------------------------------------ + # Orphan folder recovery (DB row gone, folder still on disk) + # ------------------------------------------------------------------ + + @pytest.mark.asyncio + async def test_delete_series_orphan_folder_recovery(self, anime_service, tmp_path): + """If the DB row is gone but a matching folder is still on disk, + ``delete_folder=True`` removes the orphan folder. + + Reproduces the Beyblade Burst scenario: previous delete attempt + removed the DB row (delete_database=True) but the folder delete + silently failed (path math bug). Retrying with delete_folder=True + should clean up the orphan via a key-based folder scan. + """ + # Simulate the on-disk anime directory + anime_dir = tmp_path / "anime" + anime_dir.mkdir() + orphan = anime_dir / "Beyblade Burst (2016)" + orphan.mkdir() + (orphan / "episode.mp4").write_text("x") + + anime_service._directory = str(anime_dir) + + mock_session = AsyncMock() + mock_ctx = _make_db_ctx(mock_session) + + # DB lookup returns None (row already gone) + with patch( + "src.server.database.connection.get_db_session", + return_value=mock_ctx, + ), patch( + "src.server.database.service.AnimeSeriesService.get_by_key", + new_callable=AsyncMock, + return_value=None, + ): + result = await anime_service.delete_series( + key="beyblade-burst", + delete_database=False, + delete_folder=True, + ) + + # Orphan folder recovered via key-match scan + assert result.deleted_folder is True, ( + f"orphan recovery failed: success={result.success} " + f"folder_error={result.folder_error!r}" + ) + assert result.folder_error is None + assert not orphan.exists() + + @pytest.mark.asyncio + async def test_delete_series_orphan_folder_no_match( + self, anime_service, tmp_path + ): + """If DB row is gone and no folder matches the key, return a + clear error rather than silently succeeding. + """ + anime_dir = tmp_path / "anime" + anime_dir.mkdir() + # Some unrelated folder that does NOT match + (anime_dir / "Different Show (2020)").mkdir() + anime_service._directory = str(anime_dir) + + mock_session = AsyncMock() + mock_ctx = _make_db_ctx(mock_session) + + with patch( + "src.server.database.connection.get_db_session", + return_value=mock_ctx, + ), patch( + "src.server.database.service.AnimeSeriesService.get_by_key", + new_callable=AsyncMock, + return_value=None, + ): + result = await anime_service.delete_series( + key="nonexistent-key", + delete_database=False, + delete_folder=True, + ) + + assert result.deleted_folder is False + assert result.folder_error is not None + assert "no folder matching" in result.folder_error.lower() + # Unrelated folder untouched + assert (anime_dir / "Different Show (2020)").exists() + + @pytest.mark.asyncio + async def test_delete_series_orphan_folder_ambiguous( + self, anime_service, tmp_path + ): + """If multiple folders match the same normalized key, refuse to + delete any of them (safe default). + """ + anime_dir = tmp_path / "anime" + anime_dir.mkdir() + (anime_dir / "Beyblade Burst (2016)").mkdir() + (anime_dir / "Beyblade Burst (2019)").mkdir() # also normalizes to "beybladeburst" + anime_service._directory = str(anime_dir) + + mock_session = AsyncMock() + mock_ctx = _make_db_ctx(mock_session) + + with patch( + "src.server.database.connection.get_db_session", + return_value=mock_ctx, + ), patch( + "src.server.database.service.AnimeSeriesService.get_by_key", + new_callable=AsyncMock, + return_value=None, + ): + result = await anime_service.delete_series( + key="beyblade-burst", + delete_database=False, + delete_folder=True, + ) + + # Ambiguous → refuse + assert result.deleted_folder is False + assert result.folder_error is not None + assert "no folder matching" in result.folder_error.lower() + # Neither folder deleted + assert (anime_dir / "Beyblade Burst (2016)").exists() + assert (anime_dir / "Beyblade Burst (2019)").exists()