From 7697ae7b6e8908fd4e6c7ff55e50bbee77267bb8 Mon Sep 17 00:00:00 2001 From: Kyrre Gjerstad Date: Mon, 10 Mar 2025 16:44:26 +0100 Subject: [PATCH] Fix: Prevent individual track failures from aborting entire playlist/album downloads (#814) * add tests (failing) * Improve error handling and logging in media download processes - Add exception handling and logging for track and media item downloads - Capture and log errors during album, playlist, and media ripping - Provide summary of failed items when download process completes --- streamrip/media/album.py | 17 ++++-- streamrip/media/playlist.py | 18 +++++-- streamrip/rip/main.py | 12 ++++- tests/test_error_handling.py | 101 +++++++++++++++++++++++++++++++++++ 4 files changed, 137 insertions(+), 11 deletions(-) create mode 100644 tests/test_error_handling.py diff --git a/streamrip/media/album.py b/streamrip/media/album.py index bdc3dc4..5197b11 100644 --- a/streamrip/media/album.py +++ b/streamrip/media/album.py @@ -32,12 +32,19 @@ class Album(Media): async def download(self): async def _resolve_and_download(pending: Pending): - track = await pending.resolve() - if track is None: - return - await track.rip() + try: + track = await pending.resolve() + if track is None: + return + await track.rip() + except Exception as e: + logger.error(f"Error downloading track: {e}") - await asyncio.gather(*[_resolve_and_download(p) for p in self.tracks]) + results = await asyncio.gather(*[_resolve_and_download(p) for p in self.tracks], return_exceptions=True) + + for result in results: + if isinstance(result, Exception): + logger.error(f"Album track processing error: {result}") async def postprocess(self): progress.remove_title(self.meta.album) diff --git a/streamrip/media/playlist.py b/streamrip/media/playlist.py index 7faf03e..0f3f703 100644 --- a/streamrip/media/playlist.py +++ b/streamrip/media/playlist.py @@ -121,17 +121,25 @@ class Playlist(Media): track_resolve_chunk_size = 20 async def _resolve_download(item: PendingPlaylistTrack): - track = await item.resolve() - if track is None: - return - await track.rip() + try: + track = await item.resolve() + if track is None: + return + await track.rip() + except Exception as e: + logger.error(f"Error downloading track: {e}") batches = self.batch( [_resolve_download(track) for track in self.tracks], track_resolve_chunk_size, ) + for batch in batches: - await asyncio.gather(*batch) + results = await asyncio.gather(*batch, return_exceptions=True) + + for result in results: + if isinstance(result, Exception): + logger.error(f"Batch processing error: {result}") @staticmethod def batch(iterable, n=1): diff --git a/streamrip/rip/main.py b/streamrip/rip/main.py index 23effda..5dae320 100644 --- a/streamrip/rip/main.py +++ b/streamrip/rip/main.py @@ -164,7 +164,17 @@ class Main: async def rip(self): """Download all resolved items.""" - await asyncio.gather(*[item.rip() for item in self.media]) + results = await asyncio.gather(*[item.rip() for item in self.media], return_exceptions=True) + + failed_items = 0 + for result in results: + if isinstance(result, Exception): + logger.error(f"Error processing media item: {result}") + failed_items += 1 + + if failed_items > 0: + total_items = len(self.media) + logger.info(f"Download completed with {failed_items} failed items out of {total_items} total items.") async def search_interactive(self, source: str, media_type: str, query: str): client = await self.get_logged_in_client(source) diff --git a/tests/test_error_handling.py b/tests/test_error_handling.py new file mode 100644 index 0000000..a2c2008 --- /dev/null +++ b/tests/test_error_handling.py @@ -0,0 +1,101 @@ +import asyncio +import json +import pytest +from unittest.mock import AsyncMock, MagicMock, patch + +from streamrip.media.playlist import Playlist, PendingPlaylistTrack +from streamrip.media.album import Album, PendingTrack +from streamrip.exceptions import NonStreamableError + + +class TestErrorHandling: + """Test error handling in playlist and album downloads.""" + + @pytest.mark.asyncio + async def test_playlist_handles_failed_track(self): + """Test that a playlist download continues even if one track fails.""" + mock_config = MagicMock() + mock_client = MagicMock() + mock_db = MagicMock() + + mock_track_success = MagicMock() + mock_track_success.resolve = AsyncMock(return_value=MagicMock()) + mock_track_success.resolve.return_value.rip = AsyncMock() + + mock_track_failure = MagicMock() + mock_track_failure.resolve = AsyncMock(side_effect=json.JSONDecodeError("Expecting value", "", 0)) + + playlist = Playlist( + name="Test Playlist", + config=mock_config, + client=mock_client, + tracks=[mock_track_success, mock_track_failure] + ) + + await playlist.download() + + mock_track_success.resolve.assert_called_once() + mock_track_success.resolve.return_value.rip.assert_called_once() + mock_track_failure.resolve.assert_called_once() + + @pytest.mark.asyncio + async def test_album_handles_failed_track(self): + """Test that an album download continues even if one track fails.""" + mock_config = MagicMock() + mock_client = MagicMock() + mock_db = MagicMock() + mock_meta = MagicMock() + + # Create a list of mock tracks - one will succeed, one will fail + mock_track_success = MagicMock() + mock_track_success.resolve = AsyncMock(return_value=MagicMock()) + mock_track_success.resolve.return_value.rip = AsyncMock() + + # This track will raise a JSONDecodeError when resolved + mock_track_failure = MagicMock() + mock_track_failure.resolve = AsyncMock(side_effect=json.JSONDecodeError("Expecting value", "", 0)) + + album = Album( + meta=mock_meta, + config=mock_config, + tracks=[mock_track_success, mock_track_failure], + folder="/test/folder", + db=mock_db + ) + + await album.download() + + mock_track_success.resolve.assert_called_once() + mock_track_success.resolve.return_value.rip.assert_called_once() + mock_track_failure.resolve.assert_called_once() + + @pytest.mark.asyncio + async def test_main_rip_handles_failed_media(self): + """Test that the Main.rip method handles failed media items.""" + from streamrip.rip.main import Main + + mock_config = MagicMock() + + mock_config.session.downloads.requests_per_minute = 0 + mock_config.session.database.downloads_enabled = False + mock_config.session.database.failed_downloads_enabled = False + + with patch('streamrip.rip.main.QobuzClient'), \ + patch('streamrip.rip.main.TidalClient'), \ + patch('streamrip.rip.main.DeezerClient'), \ + patch('streamrip.rip.main.SoundcloudClient'): + + main = Main(mock_config) + + mock_media_success = MagicMock() + mock_media_success.rip = AsyncMock() + + mock_media_failure = MagicMock() + mock_media_failure.rip = AsyncMock(side_effect=Exception("Media download failed")) + + main.media = [mock_media_success, mock_media_failure] + + await main.rip() + + mock_media_success.rip.assert_called_once() + mock_media_failure.rip.assert_called_once() \ No newline at end of file