[Enhancement] Add rate limiting to yt-dlp requests; prevent saving Media Items when throttled by YouTube (#559)

* Added sleep interval to settings

* Added new sleep setting to yt-dlp runner and added tests

* Added setting for form; updated setting name

* Updated form label

* Prevented saving/updating of media items if being throttled by youtube

* Added the bot message to the list of non-retryable errors

* Fixed typo

test/pinchflat/downloading/media_download_worker_test.exs

@@ -127,6 +127,19 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do
       end)
     end
 
+    test "does not set the job to retryable if youtube thinks you're a bot", %{media_item: media_item} do
+      expect(YtDlpRunnerMock, :run, 2, fn
+        _url, :get_downloadable_status, _opts, _ot, _addl -> {:ok, "{}"}
+        _url, :download, _opts, _ot, _addl -> {:error, "Sign in to confirm you're not a bot", 1}
+      end)
+
+      Oban.Testing.with_testing_mode(:inline, fn ->
+        {:ok, job} = Oban.insert(MediaDownloadWorker.new(%{id: media_item.id, quality_upgrade?: true}))
+
+        assert job.state == "completed"
+      end)
+    end
+
     test "it ensures error are returned in a 2-item tuple", %{media_item: media_item} do
       expect(YtDlpRunnerMock, :run, 2, fn
         _url, :get_downloadable_status, _opts, _ot, _addl -> {:ok, "{}"}

test/pinchflat/media_test.exs

@@ -921,6 +921,14 @@ defmodule Pinchflat.MediaTest do
       media_item = media_item_fixture()
       assert %Ecto.Changeset{} = Media.change_media_item(media_item)
     end
+
+    test "validates the title doesn't start with 'youtube video #'" do
+      # This is to account for youtube restricting indexing. See issue #549 for more
+      media_item = media_item_fixture()
+
+      assert %Ecto.Changeset{valid?: false} = Media.change_media_item(media_item, %{title: "youtube video #123"})
+      assert %Ecto.Changeset{valid?: true} = Media.change_media_item(media_item, %{title: "any other title"})
+    end
   end
 
   describe "change_media_item/1 when testing upload_date_index and source is a channel" do

test/pinchflat/settings_test.exs

@@ -77,5 +77,13 @@ defmodule Pinchflat.SettingsTest do
 
       assert %Ecto.Changeset{} = Settings.change_setting(setting, %{onboarding: true})
     end
+
+    test "ensures the extractor sleep interval is positive" do
+      setting = Settings.record()
+
+      assert %Ecto.Changeset{valid?: true} = Settings.change_setting(setting, %{extractor_sleep_interval_seconds: 1})
+      assert %Ecto.Changeset{valid?: true} = Settings.change_setting(setting, %{extractor_sleep_interval_seconds: 0})
+      assert %Ecto.Changeset{valid?: false} = Settings.change_setting(setting, %{extractor_sleep_interval_seconds: -1})
+    end
   end
 end

test/pinchflat/slow_indexing/slow_indexing_helpers_test.exs

@@ -290,6 +290,29 @@ defmodule Pinchflat.SlowIndexing.SlowIndexingHelpersTest do
       assert %Ecto.Changeset{} = changeset
     end
 
+    test "doesn't blow up if the media item cannot be saved", %{source: source} do
+      stub(YtDlpRunnerMock, :run, fn _url, :get_media_attributes_for_collection, _opts, _ot, _addl_opts ->
+        response =
+          Phoenix.json_library().encode!(%{
+            id: "video1",
+            # This is a disallowed title - see MediaItem changeset or issue #549
+            title: "youtube video #123",
+            original_url: "https://example.com/video1",
+            live_status: "not_live",
+            description: "desc1",
+            aspect_ratio: 1.67,
+            duration: 12.34,
+            upload_date: "20210101"
+          })
+
+        {:ok, response}
+      end)
+
+      assert [changeset] = SlowIndexingHelpers.index_and_enqueue_download_for_media_items(source)
+
+      assert %Ecto.Changeset{} = changeset
+    end
+
     test "passes the source's download options to the yt-dlp runner", %{source: source} do
       expect(YtDlpRunnerMock, :run, fn _url, :get_media_attributes_for_collection, opts, _ot, _addl_opts ->
         assert {:output, "/tmp/test/media/%(title)S.%(ext)S"} in opts

test/pinchflat/sources_test.exs

@@ -354,7 +354,56 @@ defmodule Pinchflat.SourcesTest do
     end
   end
 
-  describe "create_source/2 when testing options" do
+  describe "create_source/2 when testing yt-dlp options" do
+    test "sets use_cookies to true if the source has been set to use cookies" do
+      expect(YtDlpRunnerMock, :run, fn _url, :get_source_details, _opts, _ot, addl ->
+        assert Keyword.get(addl, :use_cookies)
+
+        {:ok, playlist_return()}
+      end)
+
+      valid_attrs = %{
+        media_profile_id: media_profile_fixture().id,
+        original_url: "https://www.youtube.com/channel/abc123",
+        use_cookies: true
+      }
+
+      assert {:ok, %Source{}} = Sources.create_source(valid_attrs)
+    end
+
+    test "does not set use_cookies to false if the source has not been set to use cookies" do
+      expect(YtDlpRunnerMock, :run, fn _url, :get_source_details, _opts, _ot, addl ->
+        refute Keyword.get(addl, :use_cookies)
+
+        {:ok, playlist_return()}
+      end)
+
+      valid_attrs = %{
+        media_profile_id: media_profile_fixture().id,
+        original_url: "https://www.youtube.com/channel/abc123",
+        use_cookies: false
+      }
+
+      assert {:ok, %Source{}} = Sources.create_source(valid_attrs)
+    end
+
+    test "skips sleep interval" do
+      expect(YtDlpRunnerMock, :run, fn _url, :get_source_details, _opts, _ot, addl ->
+        assert Keyword.get(addl, :skip_sleep_interval)
+
+        {:ok, playlist_return()}
+      end)
+
+      valid_attrs = %{
+        media_profile_id: media_profile_fixture().id,
+        original_url: "https://www.youtube.com/channel/abc123"
+      }
+
+      assert {:ok, %Source{}} = Sources.create_source(valid_attrs)
+    end
+  end
+
+  describe "create_source/2 when testing its options" do
     test "run_post_commit_tasks: false won't enqueue post-commit tasks" do
       expect(YtDlpRunnerMock, :run, &channel_mock/5)
 
@@ -902,28 +951,30 @@ defmodule Pinchflat.SourcesTest do
   end
 
   defp playlist_mock(_url, :get_source_details, _opts, _ot, _addl) do
-    {
-      :ok,
-      Phoenix.json_library().encode!(%{
-        channel: nil,
-        channel_id: nil,
-        playlist_id: "some_playlist_id_#{:rand.uniform(1_000_000)}",
-        playlist_title: "some playlist name"
-      })
-    }
+    {:ok, playlist_return()}
   end
 
   defp channel_mock(_url, :get_source_details, _opts, _ot, _addl) do
+    {:ok, channel_return()}
+  end
+
+  defp playlist_return do
+    Phoenix.json_library().encode!(%{
+      channel: nil,
+      channel_id: nil,
+      playlist_id: "some_playlist_id_#{:rand.uniform(1_000_000)}",
+      playlist_title: "some playlist name"
+    })
+  end
+
+  defp channel_return do
     channel_id = "some_channel_id_#{:rand.uniform(1_000_000)}"
 
-    {
-      :ok,
-      Phoenix.json_library().encode!(%{
-        channel: "some channel name",
-        channel_id: channel_id,
-        playlist_id: channel_id,
-        playlist_title: "some channel name - videos"
-      })
-    }
+    Phoenix.json_library().encode!(%{
+      channel: "some channel name",
+      channel_id: channel_id,
+      playlist_id: channel_id,
+      playlist_title: "some channel name - videos"
+    })
   end
 end

test/pinchflat/utils/number_utils_test.exs

@@ -47,4 +47,21 @@ defmodule Pinchflat.Utils.NumberUtilsTest do
       assert NumberUtils.human_byte_size(nil) == {0, "B"}
     end
   end
+
+  describe "add_jitter/2" do
+    test "returns 0 when the number is less than or equal to 0" do
+      assert NumberUtils.add_jitter(0) == 0
+      assert NumberUtils.add_jitter(-1) == 0
+    end
+
+    test "returns the number with jitter added" do
+      assert NumberUtils.add_jitter(100) in 100..150
+    end
+
+    test "optionally takes a jitter percentage" do
+      assert NumberUtils.add_jitter(100, 0.1) in 90..110
+      assert NumberUtils.add_jitter(100, 0.5) in 50..150
+      assert NumberUtils.add_jitter(100, 1) in 0..200
+    end
+  end
 end

test/pinchflat/yt_dlp/command_runner_test.exs

@@ -1,6 +1,7 @@
 defmodule Pinchflat.YtDlp.CommandRunnerTest do
   use Pinchflat.DataCase
 
+  alias Pinchflat.Settings
   alias Pinchflat.Utils.FilesystemUtils
 
   alias Pinchflat.YtDlp.CommandRunner, as: Runner
@@ -95,6 +96,36 @@ defmodule Pinchflat.YtDlp.CommandRunnerTest do
     end
   end
 
+  describe "run/4 when testing sleep interval options" do
+    test "includes sleep interval options by default" do
+      Settings.set(extractor_sleep_interval_seconds: 5)
+
+      assert {:ok, output} = Runner.run(@media_url, :foo, [], "")
+
+      assert String.contains?(output, "--sleep-interval")
+      assert String.contains?(output, "--sleep-requests")
+      assert String.contains?(output, "--sleep-subtitles")
+    end
+
+    test "doesn't include sleep interval options when skip_sleep_interval is true" do
+      assert {:ok, output} = Runner.run(@media_url, :foo, [], "", skip_sleep_interval: true)
+
+      refute String.contains?(output, "--sleep-interval")
+      refute String.contains?(output, "--sleep-requests")
+      refute String.contains?(output, "--sleep-subtitles")
+    end
+
+    test "doesn't include sleep interval options when extractor_sleep_interval_seconds is 0" do
+      Settings.set(extractor_sleep_interval_seconds: 0)
+
+      assert {:ok, output} = Runner.run(@media_url, :foo, [], "")
+
+      refute String.contains?(output, "--sleep-interval")
+      refute String.contains?(output, "--sleep-requests")
+      refute String.contains?(output, "--sleep-subtitles")
+    end
+  end
+
   describe "run/4 when testing global options" do
     test "creates windows-safe filenames" do
       assert {:ok, output} = Runner.run(@media_url, :foo, [], "")