Error messages are now persisted to the last_error field

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

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
+  returned by yt-dlp. Encountered errors are saved to the Media Item record. Saves
-  media_metadata record.
+  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)
+          attempt_recovery_from_error(media_with_preloads, output_filepath, message)
-
-          {:recovered, 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
 

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

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

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 media_item.last_error == "Unable to communicate with SponsorBlock"
-      assert String.ends_with?(media_item.media_filepath, ".mkv")
     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

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