diff --git a/lib/pinchflat/downloading/media_downloader.ex b/lib/pinchflat/downloading/media_downloader.ex index 22c1370..1a1ee2f 100644 --- a/lib/pinchflat/downloading/media_downloader.ex +++ b/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 diff --git a/lib/pinchflat/sources/sources.ex b/lib/pinchflat/sources/sources.ex index 8842a05..edd37f1 100644 --- a/lib/pinchflat/sources/sources.ex +++ b/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 diff --git a/test/pinchflat/downloading/media_downloader_test.exs b/test/pinchflat/downloading/media_downloader_test.exs index 712102d..eb66b99 100644 --- a/test/pinchflat/downloading/media_downloader_test.exs +++ b/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 diff --git a/test/pinchflat/sources_test.exs b/test/pinchflat/sources_test.exs index 7c347e8..44bae14 100644 --- a/test/pinchflat/sources_test.exs +++ b/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