fix: delete folder before DB row, and add orphan-folder recovery
Two follow-ups to the previous fix that resolved the CWD-relative-path bug in delete_series: 1. Reorder deletion: filesystem first, database second. When both delete_database=True and delete_folder=True were set and the folder delete failed (any reason — path math bug, permission error, missing volume, etc.), the database row was already gone by the time the folder delete was attempted. This left an orphan folder on disk that the user had no normal way to clean up — the delete UI returns 'Series not found' when the row is gone. Doing folder delete first means a folder-side failure preserves the DB row, so the user can retry the delete once the underlying issue is resolved. 2. Orphan-folder recovery: when the user requests delete_folder=True for a key whose DB row no longer exists, scan the configured anime directory for a folder that uniquely matches the key (normalized: strip trailing (YYYY), lowercase, remove hyphens/underscores/non-alphanumerics) and delete it. This recovers the Beyblade Burst scenario: a previous delete attempt removed the row but the folder stayed on disk; retrying the delete now cleans up the orphan. Safety: recovery only triggers when exactly one folder matches the normalized key. Zero matches → clear 'no folder matching' error. Multiple matches (ambiguous) → refuses to delete anything. Adds four regression tests: - test_delete_series_db_preserved_when_folder_fails - test_delete_series_orphan_folder_recovery (the Beyblade Burst case) - test_delete_series_orphan_folder_no_match - test_delete_series_orphan_folder_ambiguous
This commit is contained in:
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user