diff --git a/lib/pinchflat/downloading/media_download_worker.ex b/lib/pinchflat/downloading/media_download_worker.ex index 6ed5787..a0fbceb 100644 --- a/lib/pinchflat/downloading/media_download_worker.ex +++ b/lib/pinchflat/downloading/media_download_worker.ex @@ -105,13 +105,13 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do :ok - {:recovered, _} -> + {:recovered, _media_item, _message} -> {:error, :retry} - {:error, :unsuitable_for_download} -> + {:error, :unsuitable_for_download, _message} -> {:ok, :non_retry} - {:error, message} -> + {:error, _error_atom, message} -> action_on_error(message) end end diff --git a/lib/pinchflat/downloading/media_downloader.ex b/lib/pinchflat/downloading/media_downloader.ex index 1103abc..312c149 100644 --- a/lib/pinchflat/downloading/media_downloader.ex +++ b/lib/pinchflat/downloading/media_downloader.ex @@ -10,6 +10,7 @@ defmodule Pinchflat.Downloading.MediaDownloader do alias Pinchflat.Repo alias Pinchflat.Media alias Pinchflat.Media.MediaItem + alias Pinchflat.Utils.StringUtils alias Pinchflat.Metadata.NfoBuilder alias Pinchflat.Metadata.MetadataParser alias Pinchflat.Metadata.MetadataFileHelpers @@ -20,16 +21,53 @@ defmodule Pinchflat.Downloading.MediaDownloader do @doc """ Downloads media for a media item, updating the media item based on the metadata - returned by yt-dlp. Also saves the entire metadata response to the associated - media_metadata record. + returned by yt-dlp. Encountered errors are saved to the Media Item record. Saves + the entire metadata response to the associated media_metadata record. - NOTE: related methods (like the download worker) won't download if the media item's source + NOTE: related methods (like the download worker) won't download if Pthe media item's source is set to not download media. However, I'm not enforcing that here since I need this for testing. This may change in the future but I'm not stressed. - Returns {:ok, %MediaItem{}} | {:error, any, ...any} + Returns {:ok, %MediaItem{}} | {:error, atom(), String.t()} | {:recovered, %MediaItem{}, String.t()} """ def download_for_media_item(%MediaItem{} = media_item, override_opts \\ []) do + case attempt_download_and_update_for_media_item(media_item, override_opts) do + {:ok, media_item} -> + # Returns {:ok, %MediaItem{}} + Media.update_media_item(media_item, %{last_error: nil}) + + {:error, error_atom, message} -> + Media.update_media_item(media_item, %{last_error: StringUtils.wrap_string(message)}) + + {:error, error_atom, message} + + {:recovered, media_item, message} -> + {:ok, updated_media_item} = Media.update_media_item(media_item, %{last_error: StringUtils.wrap_string(message)}) + + {:recovered, updated_media_item, message} + end + end + + # Looks complicated, but here's the key points: + # - download_with_options runs a pre-check to see if the media item is suitable for download. + # - If the media item fails the precheck, it returns {:error, :unsuitable_for_download, message} + # - If the precheck passes but the download fails, it normally returns {:error, :download_failed, message} + # - However, there are some errors we can recover from (eg: failure to communicate with SponsorBlock). + # In this case, we attempt the download anyway and update the media item with what details we do have. + # This case returns {:recovered, updated_media_item, message} + # - If we attempt a retry but it fails, we return {:error, :unrecoverable, message} + # - If there is an unknown error unrelated to the above, we return {:error, :unknown, message} + # - Finally, if there is no error, we update the media item with the parsed JSON and return {:ok, updated_media_item} + # + # Restated, here are the return values for each case: + # - On success: {:ok, updated_media_item} + # - On initial failure but successfully recovered: {:recovered, updated_media_item, message} + # - On error: {:error, error_atom, message} where error_atom is one of: + # - `:unsuitable_for_download` if the media item fails the precheck + # - `:unrecoverable` if there was an initial failure and the recovery attempt failed + # - `:download_failed` for all other yt-dlp-related downloading errors + # - `:unknown` for any other errors, including those not related to yt-dlp + defp attempt_download_and_update_for_media_item(media_item, override_opts) do output_filepath = FilesystemUtils.generate_metadata_tmpfile(:json) media_with_preloads = Repo.preload(media_item, [:metadata, source: :media_profile]) @@ -38,31 +76,30 @@ defmodule Pinchflat.Downloading.MediaDownloader do update_media_item_from_parsed_json(media_with_preloads, parsed_json) {:error, :unsuitable_for_download} -> - Logger.warning( + message = "Media item ##{media_with_preloads.id} isn't suitable for download yet. May be an active or processing live stream" - ) - {:error, :unsuitable_for_download} + Logger.warning(message) + + {:error, :unsuitable_for_download, message} {:error, message, _exit_code} -> Logger.error("yt-dlp download error for media item ##{media_with_preloads.id}: #{inspect(message)}") if String.contains?(to_string(message), recoverable_errors()) do - attempt_update_media_item(media_with_preloads, output_filepath) - - {:recovered, message} + attempt_recovery_from_error(media_with_preloads, output_filepath, message) else - {:error, message} + {:error, :download_failed, message} end err -> Logger.error("Unknown error downloading media item ##{media_with_preloads.id}: #{inspect(err)}") - {:error, "Unknown error: #{inspect(err)}"} + {:error, :unknown, "Unknown error: #{inspect(err)}"} end end - defp attempt_update_media_item(media_with_preloads, output_filepath) do + defp attempt_recovery_from_error(media_with_preloads, output_filepath, error_message) do with {:ok, contents} <- File.read(output_filepath), {:ok, parsed_json} <- Phoenix.json_library().decode(contents) do Logger.info(""" @@ -71,12 +108,13 @@ defmodule Pinchflat.Downloading.MediaDownloader do anyway """) - update_media_item_from_parsed_json(media_with_preloads, parsed_json) + {:ok, updated_media_item} = update_media_item_from_parsed_json(media_with_preloads, parsed_json) + {:recovered, updated_media_item, error_message} else err -> Logger.error("Unable to recover error for media item ##{media_with_preloads.id}: #{inspect(err)}") - {:error, :retry_failed} + {:error, :unrecoverable, error_message} end end diff --git a/lib/pinchflat/utils/string_utils.ex b/lib/pinchflat/utils/string_utils.ex index 49328b2..66efc5e 100644 --- a/lib/pinchflat/utils/string_utils.ex +++ b/lib/pinchflat/utils/string_utils.ex @@ -35,4 +35,13 @@ defmodule Pinchflat.Utils.StringUtils do def double_brace(string) do "{{ #{string} }}" end + + @doc """ + Wraps a string in quotes if it's not already a string. Useful for working with + error messages whose types can vary. + + Returns binary() + """ + def wrap_string(message) when is_binary(message), do: message + def wrap_string(message), do: "#{inspect(message)}" end diff --git a/priv/repo/erd.png b/priv/repo/erd.png index 2acc8aa..18a55f5 100644 Binary files a/priv/repo/erd.png and b/priv/repo/erd.png differ diff --git a/priv/repo/migrations/20250210201413_add_last_error_to_media_item.exs b/priv/repo/migrations/20250210201413_add_last_error_to_media_item.exs index d8723ba..ce9b881 100644 --- a/priv/repo/migrations/20250210201413_add_last_error_to_media_item.exs +++ b/priv/repo/migrations/20250210201413_add_last_error_to_media_item.exs @@ -5,5 +5,8 @@ defmodule Pinchflat.Repo.Migrations.AddLastErrorToMediaItem do alter table(:media_items) do add :last_error, :string end + + execute "CREATE INDEX media_items_last_error_index ON media_items (last_error);", + "DROP INDEX media_items_last_error_index;" end end diff --git a/test/pinchflat/downloading/media_downloader_test.exs b/test/pinchflat/downloading/media_downloader_test.exs index 8a60f93..5bda8cc 100644 --- a/test/pinchflat/downloading/media_downloader_test.exs +++ b/test/pinchflat/downloading/media_downloader_test.exs @@ -60,7 +60,8 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do {:ok, Phoenix.json_library().encode!(%{"live_status" => "is_live"})} end) - assert {:error, :unsuitable_for_download} = MediaDownloader.download_for_media_item(media_item) + assert {:error, :unsuitable_for_download, message} = MediaDownloader.download_for_media_item(media_item) + assert message =~ "Media item ##{media_item.id} isn't suitable for download yet." end test "non-recoverable errors are passed through", %{media_item: media_item} do @@ -69,7 +70,7 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do _url, :download, _opts, _ot, _addl -> {:error, :some_error, 1} end) - assert {:error, :some_error} = MediaDownloader.download_for_media_item(media_item) + assert {:error, :download_failed, :some_error} = MediaDownloader.download_for_media_item(media_item) end test "unknown errors are passed through", %{media_item: media_item} do @@ -78,7 +79,7 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do _url, :download, _opts, _ot, _addl -> {:error, :some_error} end) - assert {:error, message} = MediaDownloader.download_for_media_item(media_item) + assert {:error, :unknown, message} = MediaDownloader.download_for_media_item(media_item) assert message == "Unknown error: {:error, :some_error}" end end @@ -107,13 +108,15 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do expect(YtDlpRunnerMock, :run, 0, fn _url, :download, _opts, _ot, _addl -> {:ok, ""} end) - assert {:error, :unsuitable_for_download} = MediaDownloader.download_for_media_item(media_item) + assert {:error, :unsuitable_for_download, message} = MediaDownloader.download_for_media_item(media_item) + assert message =~ "Media item ##{media_item.id} isn't suitable for download yet." end test "returns unexpected errors from the download status determination method", %{media_item: media_item} do expect(YtDlpRunnerMock, :run, fn _url, :get_downloadable_status, _opts, _ot, _addl -> {:error, :what_tha} end) - assert {:error, "Unknown error: {:error, :what_tha}"} = MediaDownloader.download_for_media_item(media_item) + assert {:error, :unknown, "Unknown error: {:error, :what_tha}"} = + MediaDownloader.download_for_media_item(media_item) end end @@ -184,15 +187,21 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do test "returns a recovered tuple on recoverable errors", %{media_item: media_item} do message = "Unable to communicate with SponsorBlock" - expect(YtDlpRunnerMock, :run, 2, fn + expect(YtDlpRunnerMock, :run, 3, fn _url, :get_downloadable_status, _opts, _ot, _addl -> {:ok, "{}"} - _url, :download, _opts, _ot, _addl -> + _url, :download, _opts, _ot, addl -> + [{:output_filepath, filepath} | _] = addl + File.write(filepath, render_metadata(:media_metadata)) + {:error, message, 1} + + _url, :download_thumbnail, _opts, _ot, _addl -> + {:ok, ""} end) - assert {:recovered, ^message} = MediaDownloader.download_for_media_item(media_item) + assert {:recovered, _media_item, ^message} = MediaDownloader.download_for_media_item(media_item) end test "attempts to update the media item on recoverable errors", %{media_item: media_item} do @@ -212,11 +221,59 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do {:ok, ""} end) - assert {:recovered, ^message} = MediaDownloader.download_for_media_item(media_item) + assert {:recovered, updated_media_item, ^message} = MediaDownloader.download_for_media_item(media_item) + + assert DateTime.diff(DateTime.utc_now(), updated_media_item.media_downloaded_at) < 2 + assert String.ends_with?(updated_media_item.media_filepath, ".mkv") + end + + test "returns an unrecoverable tuple if recovery fails", %{media_item: media_item} do + message = "Unable to communicate with SponsorBlock" + + expect(YtDlpRunnerMock, :run, 2, fn + _url, :get_downloadable_status, _opts, _ot, _addl -> + {:ok, "{}"} + + _url, :download, _opts, _ot, _addl -> + # This errors because the metadata is not written to the file so JSON parsing fails + {:error, message, 1} + end) + + assert {:error, :unrecoverable, ^message} = MediaDownloader.download_for_media_item(media_item) + end + + test "sets the last_error appropriately when recovered", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, 3, fn + _url, :download, _opts, _ot, addl -> + [{:output_filepath, filepath} | _] = addl + File.write(filepath, render_metadata(:media_metadata)) + + {:error, "Unable to communicate with SponsorBlock", 1} + + _url, :get_downloadable_status, _opts, _ot, _addl -> + {:ok, "{}"} + + _url, :download_thumbnail, _opts, _ot, _addl -> + {:ok, ""} + end) + + assert {:recovered, updated_media_item, _message} = MediaDownloader.download_for_media_item(media_item) + assert updated_media_item.last_error == "Unable to communicate with SponsorBlock" + end + + test "sets the last_error appropriately when unrecoverable", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, 2, fn + _url, :get_downloadable_status, _opts, _ot, _addl -> + {:ok, "{}"} + + _url, :download, _opts, _ot, _addl -> + {:error, "Unable to communicate with SponsorBlock", 1} + end) + + assert {:error, :unrecoverable, _message} = MediaDownloader.download_for_media_item(media_item) media_item = Repo.reload(media_item) - assert DateTime.diff(DateTime.utc_now(), media_item.media_downloaded_at) < 2 - assert String.ends_with?(media_item.media_filepath, ".mkv") + assert media_item.last_error == "Unable to communicate with SponsorBlock" end end @@ -324,6 +381,25 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do File.rm(updated_media_item.metadata_filepath) end + + test "sets the last_error to nil on success" do + media_item = media_item_fixture(%{last_error: "Some error"}) + + assert {:ok, updated_media_item} = MediaDownloader.download_for_media_item(media_item) + assert updated_media_item.last_error == nil + end + + test "sets the last_error to the error message on failure", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, 2, fn + _url, :get_downloadable_status, _opts, _ot, _addl -> {:ok, "{}"} + _url, :download, _opts, _ot, _addl -> {:error, :some_error} + end) + + assert {:error, :unknown, _message} = MediaDownloader.download_for_media_item(media_item) + media_item = Repo.reload(media_item) + + assert media_item.last_error == "Unknown error: {:error, :some_error}" + end end describe "download_for_media_item/3 when testing NFO generation" do diff --git a/test/pinchflat/utils/string_utils_test.exs b/test/pinchflat/utils/string_utils_test.exs index a9f79ad..767f74b 100644 --- a/test/pinchflat/utils/string_utils_test.exs +++ b/test/pinchflat/utils/string_utils_test.exs @@ -33,4 +33,14 @@ defmodule Pinchflat.Utils.StringUtilsTest do assert StringUtils.double_brace("hello") == "{{ hello }}" end end + + describe "wrap_string/1" do + test "returns strings as-is" do + assert StringUtils.wrap_string("hello") == "hello" + end + + test "returns other values as inspected strings" do + assert StringUtils.wrap_string(1) == "1" + end + end end