[Enhancement] Retry a download using cookies if doing so might help (#641)

* Sources that use cookies when_needed now retry downloads if we think it'd help

* tweaked error message we're checking on to match media_download_worker

Author: kieraneglin

lib/pinchflat/downloading/media_downloader.ex

@@ -52,6 +52,8 @@ defmodule Pinchflat.Downloading.MediaDownloader do
   # 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}
+  #   - However, if the precheck fails in a way that we think can be fixed by using cookies, we retry with cookies
+  #     and return the result of that
   # - 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.
@@ -68,6 +70,8 @@ defmodule Pinchflat.Downloading.MediaDownloader do
   #   - `: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
+  # - If we retry using cookies, all of the above return values apply. The cookie retry
+  #   logic is handled transparently as far as the caller is concerned
   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])
@@ -152,14 +156,48 @@ defmodule Pinchflat.Downloading.MediaDownloader do
 
   defp download_with_options(url, item_with_preloads, output_filepath, override_opts) do
     {:ok, options} = DownloadOptionBuilder.build(item_with_preloads, override_opts)
+    force_use_cookies = Keyword.get(override_opts, :force_use_cookies, false)
+    source_uses_cookies = Sources.use_cookies?(item_with_preloads.source, :downloading)
+    should_use_cookies = force_use_cookies || source_uses_cookies
 
-    use_cookies = Sources.use_cookies?(item_with_preloads.source, :downloading)
-    runner_opts = [output_filepath: output_filepath, use_cookies: use_cookies]
+    runner_opts = [output_filepath: output_filepath, use_cookies: should_use_cookies]
 
-    case YtDlpMedia.get_downloadable_status(url, use_cookies: use_cookies) do
-      {:ok, :downloadable} -> YtDlpMedia.download(url, options, runner_opts)
-      {:ok, :ignorable} -> {:error, :unsuitable_for_download}
-      err -> err
+    case {YtDlpMedia.get_downloadable_status(url, use_cookies: should_use_cookies), should_use_cookies} do
+      {{:ok, :downloadable}, _} ->
+        YtDlpMedia.download(url, options, runner_opts)
+
+      {{:ok, :ignorable}, _} ->
+        {:error, :unsuitable_for_download}
+
+      {{:error, _message, _exit_code} = err, false} ->
+        # If there was an error and we don't have cookies, this method will retry with cookies
+        # if doing so would help AND the source allows. Otherwise, it will return the error as-is
+        maybe_retry_with_cookies(url, item_with_preloads, output_filepath, override_opts, err)
+
+      # This gets hit if cookies are enabled which, importantly, also covers the case where we
+      # retry a download with cookies and it fails again
+      {{:error, message, exit_code}, true} ->
+        {:error, message, exit_code}
+
+      {err, _} ->
+        err
+    end
+  end
+
+  defp maybe_retry_with_cookies(url, item_with_preloads, output_filepath, override_opts, err) do
+    {:error, message, _} = err
+    source = item_with_preloads.source
+    message_contains_cookie_error = String.contains?(to_string(message), recoverable_cookie_errors())
+
+    if Sources.use_cookies?(source, :error_recovery) && message_contains_cookie_error do
+      download_with_options(
+        url,
+        item_with_preloads,
+        output_filepath,
+        Keyword.put(override_opts, :force_use_cookies, true)
+      )
+    else
+      err
     end
   end
 
@@ -168,4 +206,11 @@ defmodule Pinchflat.Downloading.MediaDownloader do
       "Unable to communicate with SponsorBlock"
     ]
   end
+
+  defp recoverable_cookie_errors do
+    [
+      "Sign in to confirm",
+      "This video is available to this channel's members"
+    ]
+  end
 end

lib/pinchflat/sources/sources.ex

@@ -37,11 +37,11 @@ defmodule Pinchflat.Sources do
 
   Returns boolean()
   """
-  def use_cookies?(source, operation) when operation in [:indexing, :downloading, :metadata] do
+  def use_cookies?(source, operation) when operation in [:indexing, :downloading, :metadata, :error_recovery] do
     case source.cookie_behaviour do
       :disabled -> false
       :all_operations -> true
-      :when_needed -> operation == :indexing
+      :when_needed -> operation in [:indexing, :error_recovery]
     end
   end
 

test/pinchflat/downloading/media_downloader_test.exs

@@ -204,7 +204,7 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do
     end
   end
 
-  describe "download_for_media_item/3 when testing retries" do
+  describe "download_for_media_item/3 when testing non-cookie retries" do
     test "returns a recovered tuple on recoverable errors", %{media_item: media_item} do
       message = "Unable to communicate with SponsorBlock"
 
@@ -298,6 +298,68 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do
     end
   end
 
+  describe "download_for_media_item/3 when testing cookie retries" do
+    test "retries with cookies if we think it would help and the source allows" do
+      expect(YtDlpRunnerMock, :run, 4, fn
+        _url, :get_downloadable_status, _opts, _ot, [use_cookies: false] ->
+          {:error, "Sign in to confirm your age", 1}
+
+        _url, :get_downloadable_status, _opts, _ot, [use_cookies: true] ->
+          {:ok, "{}"}
+
+        _url, :download, _opts, _ot, addl ->
+          assert {:use_cookies, true} in addl
+          {:ok, render_metadata(:media_metadata)}
+
+        _url, :download_thumbnail, _opts, _ot, _addl ->
+          {:ok, ""}
+      end)
+
+      source = source_fixture(%{cookie_behaviour: :when_needed})
+      media_item = media_item_fixture(%{source_id: source.id})
+
+      assert {:ok, _} = MediaDownloader.download_for_media_item(media_item)
+    end
+
+    test "does not retry with cookies if we don't think it would help even the source allows" do
+      expect(YtDlpRunnerMock, :run, 1, fn
+        _url, :get_downloadable_status, _opts, _ot, [use_cookies: false] ->
+          {:error, "Some other error", 1}
+      end)
+
+      source = source_fixture(%{cookie_behaviour: :when_needed})
+      media_item = media_item_fixture(%{source_id: source.id})
+
+      assert {:error, :download_failed, "Some other error"} = MediaDownloader.download_for_media_item(media_item)
+    end
+
+    test "does not retry with cookies even if we think it would help but source doesn't allow" do
+      expect(YtDlpRunnerMock, :run, 1, fn
+        _url, :get_downloadable_status, _opts, _ot, [use_cookies: false] ->
+          {:error, "Sign in to confirm your age", 1}
+      end)
+
+      source = source_fixture(%{cookie_behaviour: :disabled})
+      media_item = media_item_fixture(%{source_id: source.id})
+
+      assert {:error, :download_failed, "Sign in to confirm your age"} =
+               MediaDownloader.download_for_media_item(media_item)
+    end
+
+    test "does not retry with cookies if cookies were already used" do
+      expect(YtDlpRunnerMock, :run, 1, fn
+        _url, :get_downloadable_status, _opts, _ot, [use_cookies: true] ->
+          {:error, "This video is available to this channel's members", 1}
+      end)
+
+      source = source_fixture(%{cookie_behaviour: :all_operations})
+      media_item = media_item_fixture(%{source_id: source.id})
+
+      assert {:error, :download_failed, "This video is available to this channel's members"} =
+               MediaDownloader.download_for_media_item(media_item)
+    end
+  end
+
   describe "download_for_media_item/3 when testing media_item attributes" do
     setup do
       stub(YtDlpRunnerMock, :run, fn

test/pinchflat/sources_test.exs

@@ -80,6 +80,11 @@ defmodule Pinchflat.SourcesTest do
       source = source_fixture(%{cookie_behaviour: :when_needed})
       refute Sources.use_cookies?(source, :downloading)
     end
+
+    test "returns true if the action is error_recovery and the source is set to :when_needed" do
+      source = source_fixture(%{cookie_behaviour: :when_needed})
+      assert Sources.use_cookies?(source, :error_recovery)
+    end
   end
 
   describe "list_sources/0" do